Skip to content
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

Implements PUB-SUB for newHeads (eth_subscribe) #571

Merged
merged 5 commits into from
Jun 1, 2018
Merged

Conversation

colltoaction
Copy link
Contributor

Now that we have WebSockets, we're able to do PUB-SUB. We focused on the newHeads notification because web3js:1.0 uses it when you call sendTransaction. With this feature, we're now able to run our integration test suite over WS.

New RPC commands

  • eth_subscribe, but only accepting newHeads parameter
  • eth_unsubscribe

Implementation notes

jsonrpc4j dependency

This featured presented us with a hard scenario. The jsonrpc4j library wasn't extensible enough to easily support PUB-SUB. It abstracted away the network layer and only allowed a request-response interaction using reflection (Web3Impl classes)

new co.rsk.jsonrpc package

This package has many classes representing a generic JSON-RPC message. You will notice it doesn't have any dependencies on other co.rsk packages, it just models the specification: http://www.jsonrpc.org/specification

We use jackson to automatically decode incoming requests into objects of these classes, while performing strict validations on the input data according to the spec. We also use these classes to encode responses.

classes in co.rsk.rpc.modules.eth.subscribe package

These classes also model JSON-RPC commands (requests/responses/notifications), but specific to RSK. They are also used for encoding/decoding with strict validations.

RskJsonRpcHandler

This class is in the Netty pipeline just before the jsonrpc4j code. If it detects an eth_subscribe or eth_unsubscribe message, it intercepts and handles it. Otherwise, the preexisting handlers are executed.

Since jackson is able to decode to specific classes, we implement the Visitor pattern to easily dispatch specific commands. This could be extended to handle other commands and eventually replace jsonrpc4j and the Web3Impl classes if desired.

EthSubscriptionNotificationEmitter

Works along RskJsonRpcHandler. It basically stores subscriptions/handles unsubscriptions and listens to onBlock events to notify clients.

diega
diega previously approved these changes May 28, 2018
Copy link
Contributor

@diega diega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great move forward onto a working solution of eth_subscribe. I think we should work harder to abstract a lot of the Jackson magic occurring underneath, but I think this is fine for integrating into master as it's right now. I would start migrating other methods ASAP so we can have a real measure on how easy or not that task become.

*
* Note that the JSON-RPC 2.0 spec allows using strings as IDs, but our implementation doesn't.
*/
public abstract class JsonRpcRequestOrResponse extends JsonRpcMessage {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about renaming it to JsonRpcIdentifiableMessage?

@SuppressWarnings("unused")
public Map<String, JsonRpcResultOrError> getResultOrError() {
// we dynamically calculate the name of the property for the result or error
String propertyName = resultOrError.isError() ? "error" : "result";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this boolean be resolved through polymorphism of the JsonRpcResultOrError hierarchy?

/**
* The block header DTO for JSON serialization purposes.
*/
public class BlockHeader {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think on renaming this, mostly to avoid consuming mind cycles when you search for the BlockHeader and most of the time you would mean org.ethereum.core.BlockHeader. What about BlockHeaderSubscription or BlockHeaderEvent?

);
}

@Bean
public JsonRpcSerializer getJsonRpcSerializer() {
return new JsonRpcSerializer();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would let JsonRpcSerializer to be an interface and the actual implementation to be JacksonBasedRpcSerializer. I'm not against injecting the ObjectMapper directly in the EthSubscriptionNotificationEmitter though

aeidelman
aeidelman previously approved these changes May 31, 2018
Copy link
Collaborator

@aeidelman aeidelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@rskops
Copy link
Collaborator

rskops commented May 31, 2018

SonarQube analysis reported 2 issues

  1. MINOR RskJsonRpcHandler.java#L92: Replace this "switch" statement by "if" statements to increase readability. rule
  2. INFO RskJsonRpcHandler.java#L64: Complete the task associated to this TODO comment. rule

Copy link
Collaborator

@aeidelman aeidelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@aeidelman aeidelman merged commit cd02166 into master Jun 1, 2018
@aeidelman aeidelman deleted the eth_subscribe branch June 1, 2018 13:24
@aeidelman aeidelman added this to the Bamboo v0.4.3 milestone Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants