-
Notifications
You must be signed in to change notification settings - Fork 268
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
Add newPendingTransactions and syncing To RSKj Eth_subscribe RPC Method #1516
base: master
Are you sure you want to change the base?
Conversation
…d message on pending transaction received
…d message on pending transaction received based on json serializer
…on a only one pending transaction received
…for each pending transaction received
…for each pending transaction received missing tests added and unsuscription form subscription implementation
…orm channel implementation
…ngTransactions emitter to EthSubscriptionNotificationEmitter
…PendingTransactions added to EthSubscriptionNotificationEmitter
…ngTransactions as subscriptionTypes in EthSubscribeParamsDeserializer
Add Syncing To RSKj Eth_subscribe RPC MethodThis pull request was updated with the rsksmart/rsk-gitcoin-hackathon-2021#16 implementation . |
pipeline: run |
2 similar comments
pipeline: run |
pipeline: run |
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.
@elmol thanks for your submission. Good work. There are a few comments. Please check.
|
||
private void emit(SubscriptionId id, Channel channel, boolean isStarted, Map<String, String> status) { | ||
|
||
SyncingNotification syncStatus = new SyncingNotification(isStarted,status); |
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.
according to the docs, result could either be false
or an object with sync statistics. Could you please return false
, if syncing is not active? Please refer to the examples of responses below:
< {"jsonrpc":"2.0","method":"eth_subscription","params":{"subscription":"0x8a70b5d09d8284c507b1cd1340330f73","result":false}}
< {"jsonrpc":"2.0","method":"eth_subscription","params":{"subscription":"0x8a70b5d09d8284c507b1cd1340330f73","result":{"syncing": true, "status": { "startingBlock": 0, "currentBlock": 0, "highestBlock": 24255}}}
pay attention that in the first example result
is false
, not {"syncing":false}
as reference, you can also check what the eth_syncing method does
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.
Thanks Vovchyk, I've fixed.
@@ -116,6 +118,8 @@ public void start() { | |||
}, WORKER_TIMEOUT, WORKER_TIMEOUT, TimeUnit.SECONDS | |||
); | |||
|
|||
emmitStartEvent(); |
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.
not sure this is a good place to emit start
/ stop
events for the case of syncing
. This SyncPool
service is being started even if a node is already fully synced. I guess we should mimic the behaviour of the eth_syncing method
I believe a good candidate where this can be done is the SyncProcessor
class. E.g. emitting start
event in the startDownloadingBodies and stop
- in stopSyncing. What do you think?
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.
Yes, totally agreed! I've moved from the events from SyncPool to SyncProcessor.
pipeline: run |
1 similar comment
pipeline: run |
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.
thanks @elmol for your corrections. Just posted a few minor comments. In general, LGTM
rskj-core/src/main/java/co/rsk/rpc/modules/eth/subscribe/SyncingNotification.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/co/rsk/rpc/modules/eth/subscribe/SyncingNotification.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/co/rsk/rpc/modules/eth/subscribe/SyncingNotificationEmitter.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/co/rsk/rpc/modules/eth/subscribe/SyncingNotificationEmitter.java
Outdated
Show resolved
Hide resolved
…ngNotification.java Co-authored-by: Vovchyk <volodymyr.kravets@outlook.com>
…ngNotification.java Co-authored-by: Vovchyk <volodymyr.kravets@outlook.com>
…ngNotificationEmitter.java Co-authored-by: Vovchyk <volodymyr.kravets@outlook.com>
…ngNotificationEmitter.java Co-authored-by: Vovchyk <volodymyr.kravets@outlook.com>
Thank you very much @Vovchyk for your comments and corrections. |
pipeline: run |
Add NewPendingTransactions To RSKj Eth_subscribe RPC Method
According to rsk-gitcoin-hackathon-2021 - Add NewPendingTransactions To RSKj Eth_subscribe RPC Method , I worked on newPendingTransactions subscription event
Code Coverage
Attached coverage.zip you can find all the code coverage reports.
You can find the main change in coverage/ns-33/sources/source-e.html in the attached coverage reports.
Demonstration
Here you can see a demonstration about a new pending transactions notification.
new transaction created
rskj-node initialization
The following code was used for the demo.