integration with jsonrpsee v2 #214

niklasad1 · 2021-01-08T19:28:56Z

Integration to update jsonrpsee to v2

The way the proc-macros are designed in substrate-subxt made it not possible to add another trait bound on the client to do:

ClientBuilder<T: Runtime, R: jsonrpsee_types::traits::Client> { ... }

Then the trait is not object safe and doesn't work as a trait object, thus I introduced enum wrapper for Http and WebSocket instead. Another annoyance is that the HTTP client must be spawned on tokio 0.2 or tokio 1 which may be confusing for some users. So one option is to just support WebSocket transport because it's most likely the best option for most users

Kudos to @gregdhill for fixing the embedded substrate to work with jsonrpsee v2

ascjones · 2021-01-11T10:08:21Z

Nice! My intention is to remove the embedded client and do integration testing via a precompiled substrate node. Ultimately I want this repo as minimially coupled to substrate as possible.

dvdplm · 2021-01-12T08:15:38Z

Nice indeed. Would be good to get this into the hands of users to collect early feedback.

gregdhill · 2021-01-22T14:36:42Z

We would quite like to still be able to use our embedded node for integration tests FWIW.

ascjones · 2021-01-22T14:53:50Z

We would quite like to still be able to use our embedded node for integration tests FWIW.

If we do choose to remove it (not decided yet), it will be moved to its own repository.

gregdhill · 2021-03-03T09:09:34Z

Any progress on this? We're running into a number of issues with jsonrpsee v1 and it would be great to upgrade subxt soon!

niklasad1 · 2021-03-03T09:35:47Z

We could upgrade soon if we could remove the substrate embedded node integration tests because we decided against having an abstract client in jsonrpsee for now

src/rpc.rs

gregdhill · 2021-03-04T08:20:58Z

How would you expect to test parachain integration though? If we were able to leverage full code-generation from the runtime itself I would see this as less of an issue, but in my opinion testing is essential for subxt (and downstream projects) since types are re-declared. I guess the only option would be to spawn a full-node from inside the tests which could make concurrent testing difficult.

gregdhill · 2021-03-04T08:22:47Z

src/lib.rs

            page_size: None,
            event_type_registry: EventTypeRegistry::new(),
            skip_type_sizes_check: false,
        }
    }

-    /// Sets the jsonrpsee client.
-    pub fn set_client<P: Into<jsonrpsee::Client>>(mut self, client: P) -> Self {


Probably better to split this into set_http_client and set_ws_client or so. We currently set the client this way since we require access for custom rpc calls.

Additionally I see no reason why we couldn't pass a mock client that implements these traits, or am I missing something?

Good question, "you could" but the problem is how the proc macros for frame/Call/Storage is generated I wrote a few lines about it in the PR description.

Basically you would need to change to:

struct ClientBuilder<T: Runtime, Rpc: jsonrpsee::traits::Client> { client: Option<Rpc>, .... }

This type param is then leaked into lower layers and it would require major refactoring which I prefer not do in this PR. Another option is to use traits object Box<dyn jsonrpsee::traits::Client> and that won't work because the trait itself is not object safe.

Thoughts? :)

@niklasad1 perhaps we can adopt the following approach: #236 (untested)

What do you think? I would be happy to finalize the update if you think it is a viable workaround.

Commented in the PR but I don't think it will work "out-of-the-box", I will try it out.

Replied and followed up, took me most day but the embedded client should now work...

niklasad1 · 2021-03-04T09:36:07Z

I guess the only option would be to spawn a full-node from inside the tests which could make concurrent testing difficult.

Yeah, that was my intention but that's a good observation w.r.t to concurrent testing and state on the substrate node

An option might be to mock a substrate node instead but let's hear what @ascjones thinks :)

…te-subxt into na-jsonrpsee-v2-hacky

* workaround for embedded subxt client Signed-off-by: Gregory Hill <gregorydhill@outlook.com> * increase default channel size on subxt client Signed-off-by: Gregory Hill <gregorydhill@outlook.com> * remove client tests due to inference problem on From Signed-off-by: Gregory Hill <gregorydhill@outlook.com>

niklasad1 · 2021-03-07T20:14:12Z

client/src/lib.rs

@@ -311,108 +489,3 @@ impl<C: ChainSpec + 'static> SubxtClientConfig<C> {
        service_config
    }
 }
-
-#[cfg(test)]
-mod tests {


removed these tests because of #236 (comment)

src/rpc.rs

niklasad1 · 2021-03-07T20:34:45Z

src/subscription.rs

+                Some(c) => c,
+                None => {
+                    return Some(Err(
+                        RpcError::Custom("RPC subscription dropped".into()).into()


this is called from fn submit_and_watch_extrinsic so should return an error there and be fine.

https://github.com/paritytech/substrate-subxt/pull/214/files#diff-5cdcc3261dd77eadb37bcd60c5f85affcc9306454ae7bb8128e6d096ae51a631R561-R571

niklasad1 · 2021-03-07T20:46:35Z

src/rpc.rs

@@ -434,7 +527,7 @@ impl<T: Runtime> Rpc<T> {
        let events_sub = self.subscribe_events().await?;
        let mut xt_sub = self.watch_extrinsic(extrinsic).await?;

-        while let status = xt_sub.next().await {
+        while let Some(status) = xt_sub.next().await {


we might run into paritytech/jsonrpsee#220 when the subscription is dropped, should be fixed soon I have pending PR opened.

ascjones · 2021-03-08T10:09:19Z

I guess the only option would be to spawn a full-node from inside the tests which could make concurrent testing difficult.

Yeah, that was my intention but that's a good observation w.r.t to concurrent testing and state on the substrate node

An option might be to mock a substrate node instead but let's hear what @ascjones thinks :)

I had in mind spawning the substrate node, as I have done here: https://github.com/paritytech/cargo-contract/pull/79/files#diff-a0262fea108af44a98cc49e5eb72641963dd816513f2d0a22eb738fcfed55ebeR45.

Then for concurrent testing just generate and fund an account per test, so they can run in parallel, as I have done here: https://github.com/paritytech/substrate-subxt/blob/master/src/frame/contracts.rs#L157

gregdhill · 2021-03-08T10:17:35Z

I guess the only option would be to spawn a full-node from inside the tests which could make concurrent testing difficult.

Yeah, that was my intention but that's a good observation w.r.t to concurrent testing and state on the substrate node
An option might be to mock a substrate node instead but let's hear what @ascjones thinks :)

I had in mind spawning the substrate node, as I have done here: https://github.com/paritytech/cargo-contract/pull/79/files#diff-a0262fea108af44a98cc49e5eb72641963dd816513f2d0a22eb738fcfed55ebeR45.

Then for concurrent testing just generate and fund an account per test, so they can run in parallel, as I have done here: https://github.com/paritytech/substrate-subxt/blob/master/src/frame/contracts.rs#L157

Account balance isn't the only state I would care about though.

gregdhill · 2021-03-08T10:17:52Z

@ascjones is this good to merge?

ascjones · 2021-03-08T10:20:50Z

Account balance isn't the only state I would care about though.

Would have to spawn a fresh node per test in that case. That would be true for an embedded node too as well as a separate process. Of course you would need to deal with port numbers but that should be straightforward enough. Or just run tests seqentially.

ascjones

LGTM

niklasad1 added 2 commits January 8, 2021 20:13

hacky integration with jsonrpsee v2

434cb7c

stray todos

15646a3

Merge remote-tracking branch 'origin/master' into na-jsonrpsee-v2-hacky

e3d67c7

Merge remote-tracking branch 'origin/master' into na-jsonrpsee-v2-hacky

a36b16c

niklasad1 added 4 commits March 3, 2021 14:04

Merge remote-tracking branch 'origin/master' into na-jsonrpsee-v2-hacky

c3521c7

fmt

413a1a2

add http support

60a68d1

make test build compile

393d33c

niklasad1 commented Mar 3, 2021

View reviewed changes

src/rpc.rs Outdated Show resolved Hide resolved

Update src/rpc.rs

73ac44b

niklasad1 changed the title ~~hacky integration with jsonrpsee v2~~ integration with jsonrpsee v2 Mar 3, 2021

gregdhill reviewed Mar 4, 2021

View reviewed changes

niklasad1 and others added 5 commits March 4, 2021 11:20

bring back set_client

5b704e8

Merge branch 'na-jsonrpsee-v2-hacky' of github.com:paritytech/substra…

818daa6

…te-subxt into na-jsonrpsee-v2-hacky

use crates.io version jsonrpsee

375e971

add comments for missing impls

a7ae851

niklasad1 commented Mar 7, 2021

View reviewed changes

src/rpc.rs Outdated Show resolved Hide resolved

more verbose errors

15afbf1

niklasad1 marked this pull request as ready for review March 7, 2021 20:27

niklasad1 commented Mar 7, 2021

View reviewed changes

niklasad1 requested a review from ascjones March 7, 2021 20:36

make subscription notifs buffer bigger

013245f

niklasad1 commented Mar 7, 2021

View reviewed changes

fmt

a666ed1

ascjones approved these changes Mar 8, 2021

View reviewed changes

ascjones merged commit a920e34 into master Mar 8, 2021

ascjones deleted the na-jsonrpsee-v2-hacky branch March 8, 2021 10:52

ascjones mentioned this pull request Mar 8, 2021

implement variant of subscription that returns finalized storage changes #237

Merged

niklasad1 mentioned this pull request Mar 8, 2021

[jsonrpsee]: Dropping subscription terminates the RPC client. #241

Closed

ascjones mentioned this pull request Mar 22, 2021

Release v0.15 #248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integration with jsonrpsee v2 #214

integration with jsonrpsee v2 #214

niklasad1 commented Jan 8, 2021 •

edited

ascjones commented Jan 11, 2021

dvdplm commented Jan 12, 2021

gregdhill commented Jan 22, 2021

ascjones commented Jan 22, 2021

gregdhill commented Mar 3, 2021

niklasad1 commented Mar 3, 2021

gregdhill commented Mar 4, 2021

gregdhill Mar 4, 2021

gregdhill Mar 4, 2021

niklasad1 Mar 4, 2021 •

edited

gregdhill Mar 4, 2021 •

edited

gregdhill Mar 4, 2021

niklasad1 Mar 4, 2021

gregdhill Mar 5, 2021

niklasad1 commented Mar 4, 2021

niklasad1 Mar 7, 2021

niklasad1 Mar 7, 2021

niklasad1 Mar 7, 2021

niklasad1 Mar 7, 2021

ascjones commented Mar 8, 2021

gregdhill commented Mar 8, 2021

gregdhill commented Mar 8, 2021

ascjones commented Mar 8, 2021

ascjones left a comment

integration with jsonrpsee v2 #214

integration with jsonrpsee v2 #214

Conversation

niklasad1 commented Jan 8, 2021 • edited

ascjones commented Jan 11, 2021

dvdplm commented Jan 12, 2021

gregdhill commented Jan 22, 2021

ascjones commented Jan 22, 2021

gregdhill commented Mar 3, 2021

niklasad1 commented Mar 3, 2021

gregdhill commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Mar 4, 2021 • edited

Choose a reason for hiding this comment

gregdhill Mar 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones commented Mar 8, 2021

gregdhill commented Mar 8, 2021

gregdhill commented Mar 8, 2021

ascjones commented Mar 8, 2021

ascjones left a comment

Choose a reason for hiding this comment

niklasad1 commented Jan 8, 2021 •

edited

niklasad1 Mar 4, 2021 •

edited

gregdhill Mar 4, 2021 •

edited