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

Jump to bottom

introduce jsonrpsee client abstraction + kill HTTP support. #341

Merged
merged 39 commits into from
Feb 4, 2022
Merged

introduce jsonrpsee client abstraction + kill HTTP support. #341

merged 39 commits into from
Feb 4, 2022

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Nov 30, 2021 •
edited
Loading

Brings in the abstract async jsonrpc client from jsonrpsee and removes HTTP support.

The benefit is that is possible for users can plug in their own transport layer as long the transport implements the traits TransportSenderT and TransportReceiverT. For example in memory to talk with an embedded substrate node...

Follow-up introduce proc macros from jsonrpsee.

@niklasad1 niklasad1 changed the title PoC: introduce jsonrpsee core client PoC: introduce jsonrpsee client abstraction Nov 30, 2021
niklasad1
niklasad1 commented Dec 1, 2021
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@jsdw
Copy link
Collaborator

jsdw commented Dec 1, 2021

If you resolve the conflict, you might find that test-runtime either builds OK now without getting stuck, or at least tells you why it's getting stuck (possibly you don't have substrate on your PATH locally?) :)

@niklasad1
Copy link
Member Author

If you resolve the conflict, you might find that test-runtime either builds OK now without getting stuck, or at least tells you why it's getting stuck (possibly you don't have substrate on your PATH locally?) :)

I have substrate in my path but it's some issue with connection to the substrate process in the integration tests but I tried submit_and_watch example and that worked.

However, thanks merged to master let's see that the CI tells us :)

@ascjones ascjones mentioned this pull request Dec 2, 2021
Closed
@sander2
Copy link
Contributor

sander2 commented Dec 2, 2021

Would that solve your issues w.r.t. using an embedded node? Then we can decouple the all those heavy substrate deps to another repo.

Yea, I think this should work great, thanks!

@niklasad1 niklasad1 mentioned this pull request Dec 3, 2021
Merged
@niklasad1 niklasad1 mentioned this pull request Dec 15, 2021
Merged
@niklasad1
Copy link
Member Author

niklasad1 commented Feb 3, 2022 •
edited
Loading

@gregdhill @sander2

I tested it locally with embedded client and JSON-RPC related stuff works as intended but I got some issues metadata issues in the embedded client (not sure exactly why) when submitting extrinsics but the RPC related code works as intended.

In addition it's quite hard/tricky to maintain the embedded client because subxt depends on substrate deps v4.0.0 and some crates such as sc-service are not released on crates.io so in practice I think one have to patch all substrate deps to make it compile.

For example substrate master won't work with subxt because it broke PairSigner, thus I used the tag when v4.0.0 was released, so you may have to fork subxt or maintain a separate branch for it to work with new polkadot releases or other features that you need (but I guess that you are already doing that)

To conclude, we will not include the embedded client in this repo but because we are migrating to the jsonrpsee core client you should be able to maintain the embedded client yourselves and copy to a few jsonrpsee snippets from here to get it running

@niklasad1 niklasad1 marked this pull request as ready for review February 3, 2022 14:56
niklasad1
niklasad1 commented Feb 3, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
niklasad1
niklasad1 commented Feb 3, 2022
View reviewed changes
subxt/Cargo.toml Outdated Show resolved Hide resolved
niklasad1
niklasad1 commented Feb 3, 2022
View reviewed changes
subxt/Cargo.toml Outdated Show resolved Hide resolved
jsdw
jsdw approved these changes Feb 3, 2022
View reviewed changes
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.

Looks really good to me; lovely to be able to lean on jsonrpsee more and trim some code from here :)

niklasad1
niklasad1 commented Feb 3, 2022
View reviewed changes
subxt/src/rpc.rs
@@ -332,7 +243,7 @@ impl<T: Config> Rpc<T> {
key: &StorageKey,
hash: Option<T::Hash>,
) -> Result<Option<StorageData>, BasicError> {
let params = &[to_json_value(key)?, to_json_value(hash)?];
let params = rpc_params![key, hash];
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE; this panics if the serialization fails

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there should be no reason for these things to fail to serialize (afaik it's just maps where you have to be careful with json serializing?)

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed,

I'm working on a follow-up to replace this with proc macros to get rid of this anyway, so view this as temporary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes these are infallible

ascjones
ascjones approved these changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Awesome!

subxt/src/rpc.rs
@@ -332,7 +243,7 @@ impl<T: Config> Rpc<T> {
key: &StorageKey,
hash: Option<T::Hash>,
) -> Result<Option<StorageData>, BasicError> {
let params = &[to_json_value(key)?, to_json_value(hash)?];
let params = rpc_params![key, hash];
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes these are infallible

subxt/src/rpc.rs Outdated
Ok(self.client.request("author_hasKey", params).await?)
}
}

/// Build WS RPC client from URL
pub async fn build_ws_client(url: &str) -> Result<RpcClient, RpcError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd like this named even shorter, perhaps just ws_client. Matter of taste though.

niklasad1
niklasad1 commented Feb 3, 2022
View reviewed changes
subxt/src/rpc.rs Outdated Show resolved Hide resolved
niklasad1
niklasad1 commented Feb 3, 2022
View reviewed changes
subxt/src/client.rs Outdated Show resolved Hide resolved
niklasad1
niklasad1 commented Feb 3, 2022
View reviewed changes
test-runtime/build.rs Outdated Show resolved Hide resolved
@jsdw jsdw merged commit abd7a41 into master Feb 4, 2022
@jsdw jsdw deleted the na-jsonrpsee-core-client branch February 4, 2022 10:36
0623forbidden pushed a commit to DEIPworld/substrate-subxt that referenced this pull request Feb 15, 2022
@niklasad1 @jsdw @0623forbidden
introduce jsonrpsee client abstraction + kill HTTP support. (parityte…
b7bad24 
…ch#341)

* PoC async rpc client

* add client example should be removed from this repo

* fmt

* cargo fmt

* subxt client tests

* cargo fmt

* fix some nits

* try nightly for all CI jobs

* need wasm also for CI

* wasm for nightly run too

* client: add missing features

* update jsonrpsee

* hacky update jsonrpsee

* use jsonrpsee crates.io release

* ci: pin nightly 2021-12-15

* pin nightly to 2021-12-15

* fix build

* fmt

* compile please

* rewrite me

* fixes

* fixes

* pre-generate metadata

* fix nit

* get rid of needless deps

* remove embedded client

* Update Cargo.toml

* Update subxt/Cargo.toml

* Update subxt/Cargo.toml

* Update subxt/src/client.rs

* Update subxt/src/rpc.rs

* Update test-runtime/build.rs

* cargo fmt

Co-authored-by: James Wilson <james@jsdw.me>
@gregdhill gregdhill mentioned this pull request Apr 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsdw jsdw jsdw approved these changes

@ascjones ascjones ascjones approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@niklasad1 @jsdw @sander2 @gregdhill @ascjones