Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@tomusdrw
Copy link
Contributor

The main point of the PR is to generalize the RPC extension so that they can be used in down stream projects (see paritytech/polkadot#460).
Before they were part of node-rpc crate, and were too closely tied with node-primitives. Now they are enclosed under srml directory inside the module they are related to.

@tomusdrw tomusdrw added A0-please_review Pull request needs code review. M4-core labels Oct 10, 2019
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Seems like a very sensible refactor 👍 . Someone (or me) should add an example of this to srml/example module of how to create and expose an RPC.

@tomusdrw
Copy link
Contributor Author

@kianenigma I actually have on my list to automate this process, cause there is a lot of boilerplate code needed now, and I imagine that most of the RPCs will be super simple - basically directly expose the RuntimeApi. We can discuss how this could be done, but then the example would be basically:

  1. Specify RuntimeApi
  2. Add some macro magic
  3. ???
  4. RPC exposed.

let mut io = jsonrpc_core::IoHandler::default();
io.extend_with(
AccountsApi::to_delegate(Accounts::new(client.clone(), pool))
SystemApi::to_delegate(System::new(client.clone(), pool))
Copy link
Contributor

Choose a reason for hiding this comment

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

Very detailed but to avoid confusion with similar names defined in node/runtime I would consider renaming these structs that implement rpc api to some name that suggests more clearly what they are doing, e.g.

Suggested change
SystemApi::to_delegate(System::new(client.clone(), pool))
SystemApi::to_delegate(SystemRpcHandler::new(client.clone(), pool))

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

@kianenigma I actually have on my list to automate this process, cause there is a lot of boilerplate code needed now, and I imagine that most of the RPCs will be super simple - basically directly expose the RuntimeApi. We can discuss how this could be done, but then the example would be basically:

Specify RuntimeApi
Add some macro magic
???
RPC exposed.

I assume ideally we could add a #[attribute] to a runtime api and that magically exposes it through rpc? same arguments, same return type, same everything.

But there seems to be hidden fact that I missed before, and you can double check maybe:

This RPC code, despite being placed in srml is non-runtime code. Hence, almost all meaningful interactions with it would have to use the runtime api (I assume for this reason we keep a ref to client). So it seems the whole concept of rpc api for srml module is quite entangled with runtime apis.

@tomusdrw
Copy link
Contributor Author

tomusdrw commented Oct 10, 2019

This RPC code, despite being placed in srml is non-runtime code. Hence, almost all meaningful interactions with would have to use the runtime api (I assume for this reason we keep a ref to client). So it seems the whole concept of rpc api for srml module is quite entangled with runtime apis.

Yes, to all 3 questions. This is a conscious choice to be able to migrate the runtime in the future, but keep the RPCs backward compatible. Otherwise the RPC implementation would have to be tightly coupled with things like storage layout of srml, etc. So definitely every RPC we expose will have some RuntimeApi behind it.

@kianenigma kianenigma added A8-looksgood and removed A0-please_review Pull request needs code review. labels Oct 12, 2019
@gavofyork gavofyork merged commit ead7e0e into master Oct 16, 2019
@gavofyork gavofyork deleted the td-srml-rpc branch October 16, 2019 10:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants