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

expose jsonrpc-core client #672

Merged
merged 4 commits into from
Oct 4, 2022
Merged

expose jsonrpc-core client #672

merged 4 commits into from
Oct 4, 2022

Conversation

seunlanlege
Copy link
Contributor

@niklasad1
Copy link
Member

There is already an Deref impl for the rpc client, why can't you use that?

@seunlanlege
Copy link
Contributor Author

There is already an Deref impl for the rpc client, why can't you use that?

yes but this derefs to your custom trait, not jsonrpsee::ClientT. Exposing that trait doesn't even make it usable, i still have to do this ridiculously unsafe cast to use it:

https://github.com/ComposableFi/centauri/blob/903aa7ccefe218e8bcbd4e1dd1a7ec8081f864f0/algorithms/grandpa/prover/src/lib.rs#L119-L122

@niklasad1 niklasad1 requested a review from jsdw September 29, 2022 13:48
@jsdw
Copy link
Collaborator

jsdw commented Sep 30, 2022

How about instead of this, the existing Deref impl is modified to return &Arc<dyn RpcClientT> rather than &dyn rpcClientT as it does at the mo? the Arc will Deref anyway so it shouldn't alter anything, and it'll give you access to the arc as well so that you can clone it or whatever.

You'd still ened to cast from the Arc<dyn RpcClientT> to the jsonrpsee client though; the RPC client in subxt is generic and doesn't have to be jsonrpsee at all (in fact, given the current interface we do reserve the right to change that and invalidate said casting without a major version bump).

@jsdw
Copy link
Collaborator

jsdw commented Sep 30, 2022

Another, safer, approach for you may just be to clone and keep hold of a copy of the original jsonrpsee Client itself rather than having to cast a dyn RpcClientT? This means constructing it explicitly and then passing it to the OnlineClient::from_rpc_client method (as well as cloning it to keep separately), but if you want access to the same client subxt is using, that is probably the safest option and avoids any casting (which may break in the future) or whatever.

I don't mind exposing the Arc<dyn RpcClientT> but I don't think it's the best way to go.

@seunlanlege
Copy link
Contributor Author

Another, safer, approach for you may just be to clone and keep hold of a copy of the original jsonrpsee Client itself rather than having to cast a dyn RpcClientT? This means constructing it explicitly and then passing it to the OnlineClient::from_rpc_client method (as well as cloning it to keep separately), but if you want access to the same client subxt is using, that is probably the safest option and avoids any casting (which may break in the future) or whatever

I've modified the PR with this approach

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, just to run cargo fmt to make the CI happy.

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM once fmt is ran and we've fixed our tests; thanks!

@seunlanlege
Copy link
Contributor Author

LGTM, just to run cargo fmt to make the CI happy.

Done 🙏🏿

@jsdw jsdw merged commit 81175b2 into paritytech:master Oct 4, 2022
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.

None yet

3 participants