frame/utils: introduce `substrate-rpc-client` crate for RPC utils #12212

niklasad1 · 2022-09-08T09:54:05Z

So in general I like this approach of using the sc-rpc-api instead of re-defining this stuff all over the place however it requires some super ugly type annotations because the trait itself in sc-rpc-api takes a bunch a trait type params.

So for example:

		ChainApi::<(), _, B::Header, ()>::finalized_head(self.as_online().rpc_client())
			.await
			.map_err(|e| {
				error!(target: LOG_TARGET, "Error = {:?}", e);
				"rpc finalized_head failed."
			})

In that cause we only care about Header but because the trait itself has 3 other type params that are unused in that call and we don't care about makes the code almost unreadable.

I tried this trick:

    trait WrapperChainApi<Number, Block, SignedBlock>: ChainApi<Number, Block::Hash, Block::Header, SignedBlock> 
    where
    // bounds here
    {}

but it doesn't work because the jsonrpsee traits are not object-safe.

polkadot companion: paritytech/polkadot#6162

Cargo.toml

utils/frame/rpc-utils/Cargo.toml

utils/frame/rpc-utils/src/lib.rs

kianenigma · 2022-09-08T10:09:25Z

utils/frame/rpc-utils/src/lib.rs

+use sp_runtime::traits::{Block as BlockT, Header as HeaderT};
+use std::collections::VecDeque;
+
+pub use jsonrpsee::{


is there actually any logic change here as well?

I have increased connection timeout and request timeout but nothing else.

In addition I removed this keep connection stuff completely instead if one wants make a request then a client needs instantiated otherwise it's not possible to get access the RPC trait stuff from the macros.

kianenigma · 2022-09-08T10:10:03Z

utils/frame/rpc-utils/src/lib.rs

+}
+
+#[async_trait]
+impl<Block: BlockT> HeaderSubscription<Block> for Subscription<Block::Header>


Don't you want this abstraction available do all jsonrpsee users?

although it would make sense to have it here in substrate as well. Better than nothing and better than being ONLY on try-runtime.

I think it makes most sense to have this one in substrate and not in jsonrpsee but maybe we could have a shared crate where because we need it subxt and perhaps somewhere else :P

We could probably implement it for T instead of Block::Header to work

kianenigma

This looks good to me, just reshuffling things around for better usability.

What I really want to explore someday, and if it doesn't have an issue you should make one for it, are a set of extension traits that bridge the gap between a frame storage item tyoe and jsonrpsee.

Imagine you import the EMP pallet, and from it you fetch the #[pallet::storage] type CurrentPhase = .... Imagine you had some extension trait that allowed you to do CurrentPhase::get_remote(...) and it would get this value for you from a remove node.

The nice thing here is that the storage item itself knows how to build its key, which is all we need from it, and then we directly pass that key to jsonrpsee under the hood to fetch.

Although, now that I have uttered this out, I think the future of Rust tooling for substrate is less and less importing pallets directly and using pallet types from subxt.

I had this idea almost 2 years ago. I think things have changed too much in the meantime :D

kianenigma · 2022-09-08T10:15:37Z

In that cause we only care about Header but because the trait itself has 3 other type params that are unused in that call and we don't care about makes the code almost unreadable.

TBH I don't think the <_, .., _> is super bad. The _ in rust already helps a lot with this.

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

niklasad1 · 2022-09-08T10:31:13Z

TBH I don't think the <_, .., _> is super bad. The _ in rust already helps a lot with this.

The annoying thing is that jsonrpsee adds some trait bounds such as Send + Sync + DeserializeOwned + Serialize depending on the use-case so _ won't work that's why I used () in the code. I could add some comments about that.

pmikolajczyk41 · 2022-09-09T07:47:33Z

@niklasad1 I was about to start working on cleaning up remote-externalities create as a follow-up to our discussion to #12167 (step 2). However, if I'm not misguided, this PR will cover both steps (2. and 3.) of refactor at once, won't it? If so, let me know if I can help you with anything here :)

niklasad1 · 2022-09-09T08:15:51Z

@niklasad1 I was about to start working on cleaning up remote-externalities create as a follow-up to our discussion to #12167 (step 2). However, if I'm not misguided, this PR will cover both steps (2. and 3.) of refactor at once, won't it? If so, let me know if I can help you with anything here :)

Yeah, it should cover both step 2 and 3.
Just take a look at the PR and let me know if anything is missing or if I broke something :)

pmikolajczyk41

generally, for me, it looks quite well, surely it is a step into right direction

regarding these long type specifiers, I'm afraid that some macro is the only easy way of removing such opaque calls; or we could provide some type shortcuts for common Block types

pmikolajczyk41 · 2022-09-09T09:57:13Z

utils/frame/remote-externalities/src/lib.rs

+impl<B: BlockT> Builder<B>
+where
+	B::Hash: DeserializeOwned,
+	B::Header: DeserializeOwned,


it might be helpful to create a DeserializableBlock trait with all such constraints

utils/frame/try-runtime/cli/src/commands/follow_chain.rs

niklasad1 · 2022-09-09T13:41:34Z

regarding these long type specifiers, I'm afraid that some macro is the only easy way of removing such opaque calls; or we could provide some type shortcuts for common Block types

The reason why these type params are needed is because the rpc traits have generic params on the entire trait which could most likely be solved by moving the type params to each method instead of the entire trait but yeah a macro could work but then you would loose the help from the compiler...

stale · 2022-10-09T13:50:44Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

…tils

kianenigma · 2022-10-11T07:38:23Z

this can probably be used for #12439 cc @ruseinov

ruseinov · 2022-10-13T08:15:22Z

yes

ruseinov

approving this, looks broadly good. If we need some further modifications - we could make them within sub-du PR.

…tils

niklasad1 · 2022-10-18T16:39:11Z

bot merge

kianenigma · 2022-10-23T09:18:08Z

@niklasad1 you are no longer using keep_connection here, is that expected?

niklasad1 · 2022-10-23T09:28:41Z

@niklasad1 you are no longer using keep_connection here, is that expected?

Yeah because one must create client to make a request and the client has just to kept just somewhere to maintain the connection.
See #12212 (comment) I don't see a point of having that option... but maybe you have a use-case somewhere where it's needed?!

kianenigma · 2022-10-23T14:14:09Z

I am not into the details of the networking here, but either the logic related to this flag was removed by mistake, or the flag should have been removed too. Having the flag do nothing is wrong.

niklasad1 · 2022-10-23T15:10:15Z

Ok, the flag should be removed too then I thought I did that.

Polkadot-Forum · 2022-11-09T17:29:21Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis/1026/2

…ritytech#12212) * hack together a PoC * Update utils/frame/rpc-utils/Cargo.toml Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update utils/frame/rpc-utils/src/lib.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * rpc_utils -> substrate_rpc_client * try runtime: remove keep connection * make CI happy * cargo fmt * fix ci * update lock file * fix * fix Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: kianenigma <kian@parity.io>

hack together a PoC

714942d

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 8, 2022

kianenigma reviewed Sep 8, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

kianenigma reviewed Sep 8, 2022

View reviewed changes

utils/frame/rpc-utils/Cargo.toml Outdated Show resolved Hide resolved

kianenigma reviewed Sep 8, 2022

View reviewed changes

utils/frame/rpc-utils/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 8, 2022

View reviewed changes

kianenigma approved these changes Sep 8, 2022

View reviewed changes

niklasad1 and others added 2 commits September 8, 2022 12:26

Update utils/frame/rpc-utils/Cargo.toml

9c6526a

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

Update utils/frame/rpc-utils/src/lib.rs

c8fcffa

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

rpc_utils -> substrate_rpc_client

24aeafe

niklasad1 changed the title ~~[WIP]: introduce rpc-utils crate~~ [WIP]: introduce substrate-rpc-client crate with shared RPC stuff Sep 9, 2022

pmikolajczyk41 reviewed Sep 9, 2022

View reviewed changes

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 9, 2022

Merge remote-tracking branch 'origin/master' into na-jsonrpc-client-u…

827527d

…tils

niklasad1 changed the title ~~[WIP]: introduce substrate-rpc-client crate with shared RPC stuff~~ frame/utils: introduce substrate-rpc-client crate for RPC utils Oct 10, 2022

niklasad1 marked this pull request as ready for review October 10, 2022 09:03

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Oct 10, 2022

try runtime: remove keep connection

afbf88d

niklasad1 mentioned this pull request Oct 10, 2022

[Features] Reintroduce sub-du #12439

Closed

1 task

niklasad1 added 2 commits October 10, 2022 14:11

make CI happy

0105751

cargo fmt

dec1ca0

ruseinov self-requested a review October 13, 2022 08:22

ruseinov approved these changes Oct 13, 2022

View reviewed changes

niklasad1 and others added 6 commits October 13, 2022 11:23

fix ci

811dd91

Merge remote-tracking branch 'origin/master' into na-jsonrpc-client-u…

73b955c

…tils

Master.into()

f7f3f29

update lock file

12d28bf

fix

bf887f1

fix

4e2fc0d

niklasad1 mentioned this pull request Oct 18, 2022

companion for #12212 paritytech/polkadot#6162

Merged

Merge remote-tracking branch 'origin/master' into na-jsonrpc-client-u…

20420b4

…tils

paritytech-processbot bot merged commit 49734dd into master Oct 18, 2022

paritytech-processbot bot deleted the na-jsonrpc-client-utils branch October 18, 2022 16:39

aurexav mentioned this pull request Jan 10, 2023

To polkadot-v0.9.33 darwinia-network/darwinia-2.0#194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frame/utils: introduce `substrate-rpc-client` crate for RPC utils #12212

frame/utils: introduce `substrate-rpc-client` crate for RPC utils #12212

niklasad1 commented Sep 8, 2022 •

edited

Loading

kianenigma Sep 8, 2022

niklasad1 Sep 8, 2022

kianenigma Sep 8, 2022

niklasad1 Sep 8, 2022

kianenigma left a comment

kianenigma commented Sep 8, 2022

niklasad1 commented Sep 8, 2022

pmikolajczyk41 commented Sep 9, 2022

niklasad1 commented Sep 9, 2022 •

edited

Loading

pmikolajczyk41 left a comment

pmikolajczyk41 Sep 9, 2022

niklasad1 commented Sep 9, 2022 •

edited

Loading

stale bot commented Oct 9, 2022

kianenigma commented Oct 11, 2022

ruseinov commented Oct 13, 2022

ruseinov left a comment

niklasad1 commented Oct 18, 2022

kianenigma commented Oct 23, 2022

niklasad1 commented Oct 23, 2022 •

edited

Loading

kianenigma commented Oct 23, 2022

niklasad1 commented Oct 23, 2022

Polkadot-Forum commented Nov 9, 2022

frame/utils: introduce substrate-rpc-client crate for RPC utils #12212

frame/utils: introduce substrate-rpc-client crate for RPC utils #12212

Conversation

niklasad1 commented Sep 8, 2022 • edited Loading

kianenigma Sep 8, 2022

Choose a reason for hiding this comment

niklasad1 Sep 8, 2022

Choose a reason for hiding this comment

kianenigma Sep 8, 2022

Choose a reason for hiding this comment

niklasad1 Sep 8, 2022

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

kianenigma commented Sep 8, 2022

niklasad1 commented Sep 8, 2022

pmikolajczyk41 commented Sep 9, 2022

niklasad1 commented Sep 9, 2022 • edited Loading

pmikolajczyk41 left a comment

Choose a reason for hiding this comment

pmikolajczyk41 Sep 9, 2022

Choose a reason for hiding this comment

niklasad1 commented Sep 9, 2022 • edited Loading

stale bot commented Oct 9, 2022

kianenigma commented Oct 11, 2022

ruseinov commented Oct 13, 2022

ruseinov left a comment

Choose a reason for hiding this comment

niklasad1 commented Oct 18, 2022

kianenigma commented Oct 23, 2022

niklasad1 commented Oct 23, 2022 • edited Loading

kianenigma commented Oct 23, 2022

niklasad1 commented Oct 23, 2022

Polkadot-Forum commented Nov 9, 2022

frame/utils: introduce `substrate-rpc-client` crate for RPC utils #12212

frame/utils: introduce `substrate-rpc-client` crate for RPC utils #12212

niklasad1 commented Sep 8, 2022 •

edited

Loading

niklasad1 commented Sep 9, 2022 •

edited

Loading

niklasad1 commented Sep 9, 2022 •

edited

Loading

niklasad1 commented Oct 23, 2022 •

edited

Loading