-
Notifications
You must be signed in to change notification settings - Fork 254
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
Storage: Support iterating over NMaps with partial keys #1079
Storage: Support iterating over NMaps with partial keys #1079
Conversation
* codegen: Fetch and decode metadata version then fallback Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * cli: Add `unstable-metadata` attribute to the subxt macro Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * docs: Add missing comma Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * macro: Add `GenerateRuntimeApi` Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * Update subxt/src/lib.rs Co-authored-by: James Wilson <james@jsdw.me> * Update macro/src/lib.rs Co-authored-by: James Wilson <james@jsdw.me> * subxt: Adjust docs Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> * cli: Import `GenerateRuntimeApi` Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: James Wilson <james@jsdw.me>
Bumps [darling](https://github.com/TedDriggs/darling) from 0.20.1 to 0.20.3. - [Release notes](https://github.com/TedDriggs/darling/releases) - [Changelog](https://github.com/TedDriggs/darling/blob/master/CHANGELOG.md) - [Commits](TedDriggs/darling@v0.20.1...v0.20.3) --- updated-dependencies: - dependency-name: darling dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Wilson <james@jsdw.me>
Bumps [either](https://github.com/bluss/either) from 1.8.1 to 1.9.0. - [Commits](rayon-rs/either@1.8.1...1.9.0) --- updated-dependencies: - dependency-name: either dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Wilson <james@jsdw.me>
Bumps [clap](https://github.com/clap-rs/clap) from 4.3.11 to 4.3.19. - [Release notes](https://github.com/clap-rs/clap/releases) - [Changelog](https://github.com/clap-rs/clap/blob/master/CHANGELOG.md) - [Commits](clap-rs/clap@v4.3.11...v4.3.19) --- updated-dependencies: - dependency-name: clap dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Wilson <james@jsdw.me> Co-authored-by: Tadeo Hepperle <62739623+tadeohepperle@users.noreply.github.com>
Bumps [trybuild](https://github.com/dtolnay/trybuild) from 1.0.81 to 1.0.82. - [Release notes](https://github.com/dtolnay/trybuild/releases) - [Commits](dtolnay/trybuild@1.0.81...1.0.82) --- updated-dependencies: - dependency-name: trybuild dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Wilson <james@jsdw.me>
Bumps [serde_json](https://github.com/serde-rs/json) from 1.0.103 to 1.0.104. - [Release notes](https://github.com/serde-rs/json/releases) - [Commits](serde-rs/json@v1.0.103...v1.0.104) --- updated-dependencies: - dependency-name: serde_json dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [serde](https://github.com/serde-rs/serde) from 1.0.175 to 1.0.179. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.175...v1.0.179) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…1102) * Support 'substrate-node' too and allow multiple binary paths * fmt * clippy * fix path
…ps-with-partial-keys
CHANGELOG.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all of these extra spaces an unintentional side effect of some search and replace? They look wrong to me :)
…ps-with-partial-keys
subxt/src/storage/storage_address.rs
Outdated
@@ -229,7 +229,7 @@ pub fn make_static_storage_map_key<T: codec::Encode>(t: T) -> StaticStorageMapKe | |||
} | |||
|
|||
/// Construct a new dynamic storage lookup to the root of some entry. | |||
pub fn dynamic_root( | |||
pub fn dynamic_iter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but now the dynamic_iter
doesn't actually provide the ability to iterate over subsets of keys like the static one does.
Maybe we should just remove this function entirely, since people can just use dynamic
below with an empty vec to achieve the same, or with a subset of keys to hopefully be able to iterate whatever they like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and if we remove this, remember to update the guide :)
Looks good overall! Just a few small comments ( a load of spaces in CHANGELOG to revert, comment update, remove Also, I had al ook and found these storage entries with multiple values that might be easier to "set up" for testing:
Multisigs can be created and so maybe that's not too hard to write a test for? I mainly included the bottom one as the only example I could see of more than 2 keys :) It would be great to have a test if possible to check that this stuff works (and with the multisig one we could also test the |
…ps-with-partial-keys
* implement test for type alias being used * Bump serde from 1.0.179 to 1.0.183 (#1112) Bumps [serde](https://github.com/serde-rs/serde) from 1.0.179 to 1.0.183. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.179...v1.0.183) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump regex from 1.9.1 to 1.9.3 (#1110) Bumps [regex](https://github.com/rust-lang/regex) from 1.9.1 to 1.9.3. - [Release notes](https://github.com/rust-lang/regex/releases) - [Changelog](https://github.com/rust-lang/regex/blob/master/CHANGELOG.md) - [Commits](rust-lang/regex@1.9.1...1.9.3) --- updated-dependencies: - dependency-name: regex dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * revert yaml changes --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: James Wilson <james@jsdw.me>
…ps-with-partial-keys
…ps-with-partial-keys
if let Some(ty) = type_path.vec_type_param() { | ||
return quote!([#ty]); | ||
} | ||
// String is cast to str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
let (fn_name, is_fetchable, is_iterable) = if n_keys == keys.len() { | ||
let fn_name = format_ident!("{snake_case_name}"); | ||
(fn_name, true, false) | ||
} else { | ||
let fn_name = if n_keys == 0 { | ||
format_ident!("{snake_case_name}_iter") | ||
} else { | ||
format_ident!("{snake_case_name}_iter{}", n_keys) | ||
}; | ||
(fn_name, false, true) | ||
}; | ||
let is_fetchable_type = is_fetchable.then_some(quote!(#crate_path::storage::address::Yes)).unwrap_or(quote!(())); | ||
let is_iterable_type = is_iterable.then_some(quote!(#crate_path::storage::address::Yes)).unwrap_or(quote!(())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I found this a bit hard to follow, maybe a match would state the intent a bit better.
And since the is_fetchable
and is_iterable
are mutually exclusive, we could probably do with just one of them, up to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a go and came up with something like this
let is_iterator = n_keys != keys.len();
let fn_name = match (is_iterator, keys.len()) {
(false, _) => format_ident!("{snake_case_name}"),
(true, 0) => format_ident!("{snake_case_name}_iter"),
(true, n) => format_ident!("{snake_case_name}_iter{}", n)
};
let yes_ty = || quote!(#crate_path::storage::address::Yes);
let no_ty = || quote!(());
let is_fetchable_type = is_iterator.then(no_ty).unwrap_or_else(yes_ty);
let is_iterable_type = is_iterator.then(yes_ty).unwrap_or_else(no_ty);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me :)
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this partial example should (also) be part of the integration-tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that but had some issues. Let me see again, it is a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
fixes #1044
Changes to the Codegen to support iterating over partial keys.
Instead of
We now have: