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

add rpc_client to Backend trait #1266

Closed
wants to merge 3 commits into from

Conversation

yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Nov 16, 2023

User maybe need to add custom jsonrpc request so the internal rpc client need to be accessed.

Or maybe just return RpcClient is enough?

@yjhmelody yjhmelody requested a review from a team as a code owner November 16, 2023 03:43
Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me!

A minor nit wrt exploding the client on the RPC methods directly (LegacyRpcMethods and UnstableRpcMethods).

Another approach for users would be keeping a clone of the inner RPC client:

let rpc_client = ..;
// Clone the client here.
let unstable_backend(rpc_client.clone);

This PR should remove the need for any cloning. Thanks for contributing! 🙏

@@ -29,6 +30,9 @@ pub(crate) mod sealed {
/// a backend. Its goal is to be as minimal as possible.
#[async_trait]
pub trait Backend<T: Config>: sealed::Sealed + Send + Sync + 'static {
/// Return the rpc client of backend.
fn rpc_client(&self) -> &dyn RpcClientT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this would fit well with the Backend trait.

At the moment we are exposing two implementations and both fetch using jsonrpsee from a running node some details:

  • either from the legacy RPC
  • or from the unstable RPC backend

However, I believe there's nothing that should stop users from providing a completely offline experience using subxt:

  • using a raw static database
  • feeding data from some third party which may not have the concept of rpc methods, or just from some in memory socket that communicates with a daemon process

And because of that, exposing the rpc_client here feels like exposing an inner detail of some type of implementations, while possibly restricting others.

I believe the same could be achieved if this method would be moved instead on UnstableBackend and LegacyBackend, since it feels more inline with their inner details.

@niklasad1 @jsdw what do you think?

Copy link
Member

@niklasad1 niklasad1 Nov 16, 2023

Choose a reason for hiding this comment

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

I don't remember the reason why James made it an implementation detail of the Backend but I think it makes the rpc API in subxt less leaky and easier for us to break stuff internally.

However, it's still possible to access the rpc client but in a slightly different manner

After this change users have to do:

   let rpc_client = RpcClient::from_url("ws://localhost").await.unwrap();
   let subxt_rpc = LegacyBackend(rpc.clone());

  // rpc_client can be used make arbitrary rpc calls.
  // subxt_rpc only provides the APIs provided by the backend.

Thus, it's by design and my cents is to not accept this PR but let's wait for @jsdw to be back from
vacation and give his thoughts on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's be design not to expose an RPC client from the Backend trait yeah.

One of the main points of the Backend trait was to abstract away any details about specific RPC calls, so that we can have a legacy and "new" backend and be certain that eg no "stray" RPC calls to old APIs are used in the new backend impl. exposing an "rpc_client()" would allow higher level APIs to break this and call methods that they shouldn't.

Another more esoteric reason is just that the Backend trait as it stands is designed to provide exactly the interface needed for the higher level Subxt APIs and no more. This means that it would be possible to have a backend which doesn't care about RPCs at all (and eg uses some new binary interface or whatever in the future).

@jsdw
Copy link
Collaborator

jsdw commented Nov 22, 2023

Closing as we won't expose the RPC methods through the Backend trait in this way. We are adding an example though to make it clearer how to use RPC calls with Subxt (see #1279)

@jsdw jsdw closed this Nov 22, 2023
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