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.

Sign up for GitHub

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
Closed

add rpc_client to Backend trait #1266

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19940db
add `rpc_client` to Backend trait
yjhmelody Nov 16, 2023
63204bb
fix docs
yjhmelody Nov 16, 2023
dcb2032
improve by suggestions; add method `rpc_client`
yjhmelody Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions subxt/src/backend/legacy/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ pub mod rpc_methods;

use self::rpc_methods::TransactionStatus as RpcTransactionStatus;
use crate::backend::{
rpc::RpcClient, Backend, BlockRef, RuntimeVersion, StorageResponse, StreamOf, StreamOfResults,
TransactionStatus,
rpc::RpcClient, Backend, BlockRef, RpcClientT, RuntimeVersion, StorageResponse, StreamOf,
StreamOfResults, TransactionStatus,
};
use crate::{config::Header, Config, Error};
use async_trait::async_trait;
Expand Down Expand Up @@ -41,6 +41,10 @@ impl<T: Config> super::sealed::Sealed for LegacyBackend<T> {}

#[async_trait]
impl<T: Config + Send + Sync + 'static> Backend<T> for LegacyBackend<T> {
fn rpc_client(&self) -> &dyn RpcClientT {
&*self.methods.client
}

async fn storage_fetch_values(
&self,
keys: Vec<Vec<u8>>,
Expand Down
2 changes: 1 addition & 1 deletion subxt/src/backend/legacy/rpc_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use serde::{Deserialize, Serialize};
#[derive(Derivative)]
#[derivative(Clone(bound = ""), Debug(bound = ""))]
pub struct LegacyRpcMethods<T> {
client: RpcClient,
pub(crate) client: RpcClient,
_marker: std::marker::PhantomData<T>,
}

Expand Down
4 changes: 4 additions & 0 deletions subxt/src/backend/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub mod legacy;
pub mod rpc;
pub mod unstable;

use crate::backend::rpc::RpcClientT;
use crate::error::Error;
use crate::metadata::Metadata;
use crate::Config;
Expand All @@ -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 •
edited
Loading

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).


/// Fetch values from storage.
async fn storage_fetch_values(
&self,
Expand Down
8 changes: 6 additions & 2 deletions subxt/src/backend/unstable/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ use self::rpc_methods::{
FollowEvent, MethodResponse, RuntimeEvent, StorageQuery, StorageQueryType, StorageResultType,
};
use crate::backend::{
rpc::RpcClient, Backend, BlockRef, BlockRefT, RuntimeVersion, StorageResponse, StreamOf,
StreamOfResults, TransactionStatus,
rpc::RpcClient, Backend, BlockRef, BlockRefT, RpcClientT, RuntimeVersion, StorageResponse,
StreamOf, StreamOfResults, TransactionStatus,
};
use crate::config::BlockHash;
use crate::error::{Error, RpcError};
Expand Down Expand Up @@ -186,6 +186,10 @@ impl<T: Config> super::sealed::Sealed for UnstableBackend<T> {}

#[async_trait]
impl<T: Config + Send + Sync + 'static> Backend<T> for UnstableBackend<T> {
fn rpc_client(&self) -> &dyn RpcClientT {
&*self.methods.client
}

async fn storage_fetch_values(
&self,
keys: Vec<Vec<u8>>,
Expand Down
2 changes: 1 addition & 1 deletion subxt/src/backend/unstable/rpc_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::task::Poll;
#[derive(Derivative)]
#[derivative(Clone(bound = ""), Debug(bound = ""))]
pub struct UnstableRpcMethods<T> {
client: RpcClient,
pub(crate) client: RpcClient,
_marker: std::marker::PhantomData<T>,
}

Expand Down