-
Notifications
You must be signed in to change notification settings - Fork 444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New WebsocketProvider and Subscriptions implementation #446
Conversation
case .newBlockFilter: | ||
return 0 | ||
case .getFilterLogs: | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between nil
and 0
?
Looks like the meaning is the same but now requiredNumOfParameters
has to be unwrapped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nil
means that there are variable number of parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a documentation comment to requiredNumOfParameters
mentioning that explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment to requiredNumOfParameters
in 54cf7e9
return values as? T | ||
} | ||
guard let value = self.value as? T else {return nil} | ||
return value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor improvement: return self.value as? T
without guard statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@odanylovych Great job! Will fork it and try it out. Could you please update the documentation part related to the websockets? |
public let transactionsRoot: String | ||
} | ||
|
||
public struct LogItem: Decodable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This struct duplicates EventLog
at the line 331.
I've tested this branch and was able to get the same result using EventLog
instead of LogItem
.
For the sake of interoperability I think it will be better to use EventLog
for web3.web3contract.getIndexedEvents
, web3.web3contract.getIndexedEventsPromise
and for the new subscription API. Especially considering that these structs do not have public init
(e.g. t won't be convenient to create EventLog
out of LogItem
) to avoid duplicates on the side of developers using this library it will make more sense to use EventLog
as a single model for logs whether it's from web socket or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example input (web socket message received) and output (decoded):
received text: {
"jsonrpc":"2.0",
"method":"eth_subscription",
"params":{
"result":{
"address":"0x6eb84ef768ffe65bca008af0dea3175eaf4df2ed",
"blockHash":"0x6474699c14877b8b614c9ed417295913b283158a29964943f90c1802e6d1320a",
"blockNumber":"0xbcf944",
"data":"0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000014414a6e2930000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000015ef83ad9559033e6e941db7d7c495acdce616347d28e90c7ce47cbfcfcad3bc50000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000596f357c6a7b0ce6b9327b83166db09820ec6703e1a1ff0c769747199f59130abc9c314ad5697066733a2f2f516d51474369465539657258756175396752537234695a48766b6a6e57636576625968784d324462664e47486f550000000000000000000000000000000000000000000000000000000000000000000000",
"logIndex":"0x1",
"removed":false,
"topics":[
"0x2e733a17851169f232b3859260eb3ad2a086afd54e999eb4ea9afb7791702e41",
"0x0000000000000000000000000000000000000000000000000000000000000000"
],
"transactionHash":"0x12b9f9312bfac44b95c82cf139a4a102efbe784a38e4eb973d1397a8b0f34a0a",
"transactionIndex":"0x0",
"transactionLogIndex":"0x1",
"type":"mined"
},
"subscription":"0x451a9281a0d8a6a4"
}
}
EventLog(address: web3swift.EthereumAddress(_address: "0x6eb84ef768ffe65bca008af0dea3175eaf4df2ed", type: web3swift.EthereumAddress.AddressType.normal), blockHash: 32 bytes, blockNumber: 12384580, data: 416 bytes, logIndex: 1, removed: false, topics: [32 bytes, 32 bytes], transactionHash: 32 bytes, transactionIndex: 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in 2d90894
Thanks @odanylovych !
public let stateRoot: String | ||
public let timestamp: String | ||
public let transactionsRoot: String | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This model should be updated.
These attributes should be optional:
public let logsBloom: String
public let nonce: String
public let number: String
And public let hash: String?
should be added.
Source from web3js library.
Also in Ethereum-Go repo I can see that nonce
is also optional but other fields are required which is confusing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example of receiving new heads.
{
"jsonrpc":"2.0",
"method":"eth_subscription",
"params":{
"result":{
"author":"0xcc271fdb440c0a37c51a770f415521d0e95a5518",
"difficulty":"0xfffffffffffffffffffffffffffffffd",
"extraData":"0xde830207028f5061726974792d457468657265756d86312e34312e30826c69",
"gasLimit":"0x2faf080",
"gasUsed":"0x0",
"hash":"0x53ce7a56293bc8e93d85abeb1c342bdf7ae1cc191aa0237d853f73ed1899a8e2",
"logsBloom":"0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
"miner":"0xcc271fdb440c0a37c51a770f415521d0e95a5518",
"number":"0xbd1c06",
"parentHash":"0xa30928f385aa798a74fef9079d9579a3400c50ff2e6cfbd80bcd0302c556f75b",
"receiptsRoot":"0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421",
"sealFields":[
"0x139840c4",
"0xf232e5ae28957588aac4f5556785ffeb7f9246c7277bc06c5e5aea6c933f5e52260faa2d9c016844c38dd3b3f8190ba1d08d23fe87c576b7d1b85a99e11a07f101"
],
"sha3Uncles":"0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
"signature":"f232e5ae28957588aac4f5556785ffeb7f9246c7277bc06c5e5aea6c933f5e52260faa2d9c016844c38dd3b3f8190ba1d08d23fe87c576b7d1b85a99e11a07f101",
"size":"0x248",
"stateRoot":"0xadf409adae4774a2171a702f4441c5669b798eb0113fc8765c891ebf3c6c7bb1",
"step":"328745156",
"timestamp":"0x61f943d4",
"transactionsRoot":"0x56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421"
},
"subscription":"0x1364cf3b35e2605f"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, I suggest modifying this function to accept a generic type: https://github.com/skywinder/web3swift/pull/446/files#diff-9743ac53f1b42c7462dbf4d5dde6f2f434845bf1bdc66c25858bf6b4cdacf0f9R27
Depending on the network used model can be specified and it will be up to the user of this library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also researching this part of the lib right now and i've confused either.
In web3js regarding to the docs these 4 fields are optional, in the ether.js all fields are non optional.
More than that in Ethereum github JSON-RPC schema all of the fields are marked as required so there's should not be optional at all.
Speaking about go implementation, as i saw their code it seems that explicitly optional checking for external API struct fields are best practices for that language, which is not in swift.
@odanylovych hi, due to last merged PR in development branch your one have some conflicts, could you please resolve it. Also it seems that ci/cd pipeline fails on carthage building, fix it please. |
Also looking forward for it to be reviewed and if accepted - merged. It's a good update to the library. |
Just saying: You're able to run CI/CD pipeline within your fork following by this hint. Also please take a look at PR rules, to make sure that you're passing all the points. |
Updated the documentation |
@odanylovych sorry for it taking so long to review your pr. Just saying, that currently i'm focused on 3.0.0-RC02 release which will include almost totally rewritten http network layer. I'll be able to review your pr afterwards, but please be noticed, that kindly unlikely that there'll be any 2.7.0 release to where could be included some massive change, rather then 3.0.0 because of kindly likely huge problems with merging that updates into 3.0.0 branch, but that's not for sure, because 3.0.0 haven't touches any Websocket code parts yet. |
No problem 👍 |
public enum BlockNumber { | ||
case pending | ||
case latest | ||
case earliest | ||
case exact(BigUInt) | ||
|
||
public var stringValue: String { | ||
switch self { | ||
case .pending: | ||
return "pending" | ||
case .latest: | ||
return "latest" | ||
case .earliest: | ||
return "earliest" | ||
case .exact(let number): | ||
return String(number, radix: 16).addHexPrefix() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just started to reviewing this PR, thinking it could take a while (like up to the end of this week).
But just yet i have question like what's the purpose of this dropping BlockNumber
enum?
I see that you've making an abstraction over the API spec, which is more close to given enum that to any literal type itself.
@yaroslavyaroslav do you have any news about the 3.0 version? We will update our PRs |
@ypopovych thank you for asking. We're done with implementing new network layer for http part. Please take a look at unstable branch and especially to 3.0.0-RC2 release. |
Sorry to we failed to pass this PR through our pipeline for so long. Really bad time it was for that. So I'm closing it for now, it you'll get some effort to start again I'll be happy to help, I mean really help, since there wouldn't be such dramatic changes in lib in foreseeable feature. |
@yaroslavyaroslav @ypopovych @odanylovych Probably, as the first version of this feature, it will be implemented in such a way that only Apple's WebSocket API will be available but later we could provide an abstraction where it will be the choice between the default implementation that we provide and the implementation provided by the user of the library. I'll use this branch and will build new add the required modifications. |
Hi.
First of all, thanks for the library!
We are working on the Avalanche library and we want to use this library as a submodule for Avalanche Web3 implementation, but right now we can't integrate it properly.
To fix this we plan to do 2 PRs to your library. They will decouple networking and subscription parts, so they can be replaced with our implementations. This is the first PR related to networking.
In this PR:
If you need more info, please, tell us. Will be glad to provide it.
Thanks.