Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add childstate_getStorageEntries RPC #9459

Merged
19 commits merged into from Sep 13, 2021
Merged

Conversation

hirschenberger
Copy link
Contributor

@hirschenberger hirschenberger commented Jul 29, 2021

Maybe it would make sense to deprecate getStorage as it can be substituted with getStorageEntries now?

fixes #9203

client/rpc/src/state/state_full.rs Outdated Show resolved Hide resolved
};
let client = self.client.clone();
Box::new(
join_all(keys.into_iter().map(move |key| {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -66,6 +66,15 @@ pub trait ChildStateApi<Hash> {
hash: Option<Hash>,
) -> FutureResult<Option<StorageData>>;

/// Returns child storage entries for multiple keys at a specific block's state.
#[rpc(name = "childstate_getStorages")]
fn storages(
Copy link
Member

Choose a reason for hiding this comment

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

storage_entries maybe? storages sounds weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's better.

Another question is if we should deprecate storage because of the overlapping functionality?

let fetcher = self.fetcher.clone();
let block = self.block_or_best(block);
Box::new(join_all(keys.into_iter().map(move |key| {
storage(&*remote_blockchain, fetcher.clone(), block, vec![key.0.clone()])
Copy link
Member

Choose a reason for hiding this comment

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

You should request all keys in one request and not one request per key.

.map(move |mut values| {
values
.remove(&key)
.expect("successful request has entries for all requested keys; qed")
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least it's the claim that was made in the storage function. But I'll add a test to check it.

block,
header,
storage_key,
keys: vec![key.0.clone()],
Copy link
Member

Choose a reason for hiding this comment

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

Again you are only requesting one key after another.

Copy link
Contributor Author

@hirschenberger hirschenberger left a comment

Choose a reason for hiding this comment

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

followup PR coming...

client/rpc/src/state/state_full.rs Outdated Show resolved Hide resolved
.map(move |mut values| {
values
.remove(&key)
.expect("successful request has entries for all requested keys; qed")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least it's the claim that was made in the storage function. But I'll add a test to check it.

@@ -66,6 +66,15 @@ pub trait ChildStateApi<Hash> {
hash: Option<Hash>,
) -> FutureResult<Option<StorageData>>;

/// Returns child storage entries for multiple keys at a specific block's state.
#[rpc(name = "childstate_getStorages")]
fn storages(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's better.

Another question is if we should deprecate storage because of the overlapping functionality?

@hirschenberger
Copy link
Contributor Author

Can I have another review, please?

@hirschenberger
Copy link
Contributor Author

@bkchr Can I have another review?

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Some last nitpicks, then we are good to merge this

client/rpc/src/state/state_full.rs Outdated Show resolved Hide resolved
client/rpc/src/state/state_full.rs Outdated Show resolved Hide resolved
client/rpc/src/state/mod.rs Outdated Show resolved Hide resolved
client/rpc/src/state/state_full.rs Outdated Show resolved Hide resolved
client/rpc/src/state/state_full.rs Outdated Show resolved Hide resolved
client/rpc/src/state/state_full.rs Outdated Show resolved Hide resolved
client/rpc/src/state/state_light.rs Outdated Show resolved Hide resolved
client/rpc/src/state/state_light.rs Outdated Show resolved Hide resolved
@bkchr bkchr changed the title Add storage query functions for multiple keys Add childstate_getStorageEntries RPC Sep 9, 2021
@bkchr bkchr added B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. A0-please_review Pull request needs code review. labels Sep 9, 2021
hirschenberger and others added 11 commits September 9, 2021 11:58
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@hirschenberger
Copy link
Contributor Author

Should be ready now.

@bkchr
Copy link
Member

bkchr commented Sep 9, 2021

@hirschenberger please run cargo fmt

@bkchr
Copy link
Member

bkchr commented Sep 9, 2021

bot merge

@ghost
Copy link

ghost commented Sep 9, 2021

Waiting for commit status.

@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Sep 9, 2021
@ghost
Copy link

ghost commented Sep 9, 2021

Bot will approve on the behalf of @bkchr, since they are a team lead, in an attempt to reach the minimum approval count

@ghost
Copy link

ghost commented Sep 9, 2021

Merge failed: Could not recover from: "Required status check \"continuous-integration/gitlab-check-polkadot-companion-build\" is failing. At least 2 approving reviews are required by reviewers with write access." due to: WithIssue: Error merging: "Required status check \"continuous-integration/gitlab-check-polkadot-companion-build\" is failing."

@hirschenberger
Copy link
Contributor Author

@bkchr There's some CI issue here?

@bkchr
Copy link
Member

bkchr commented Sep 13, 2021

bot merge

@ghost
Copy link

ghost commented Sep 13, 2021

Trying merge.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for childstate queries for multiple child keys
2 participants