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.

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

Conversation

tadeohepperle
Copy link
Contributor

fixes #1044

Changes to the Codegen to support iterating over partial keys.

Instead of

runtime::storage().pallet().entry(key1, key2, key3); // fetchable and iterable (redirects to entry_root())
runtime::storage().pallet().entry_root(); // iterable

We now have:

runtime::storage().pallet().entry(key1, key2, key3); // only fetchable
runtime::storage().pallet().entry_iter(); // only iterable
runtime::storage().pallet().entry_iter1(key1); // only iterable
runtime::storage().pallet().entry_iter2(key1, key2); // only iterable

@tadeohepperle tadeohepperle requested a review from a team as a code owner July 21, 2023 14:40
lexnv and others added 13 commits August 2, 2023 14:02
* 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
CHANGELOG.md Outdated
Copy link
Collaborator

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 :)

@@ -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(
Copy link
Collaborator

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.

Copy link
Collaborator

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 :)

@jsdw
Copy link
Collaborator

jsdw commented Aug 8, 2023

Looks good overall! Just a few small comments ( a load of spaces in CHANGELOG to revert, comment update, remove dynamic_root now I think is best).

Also, I had al ook and found these storage entries with multiple values that might be easier to "set up" for testing:

multisig.multisigs(AccountId32, [u8; 32])
staking.erasStakers(u32, AccountId32)
xcmPallet.remoteLockedFungibles(u32, AccountId32, XcmVersionedAssetId)

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 dynamic version to check it can do the same.

tadeohepperle and others added 3 commits August 8, 2023 13:28
* 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>
if let Some(ty) = type_path.vec_type_param() {
return quote!([#ty]);
}
// String is cast to str
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

Comment on lines +113 to +125
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!(()));
Copy link
Collaborator

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

Copy link
Collaborator

@jsdw jsdw Aug 11, 2023

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);

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 great to me :)

Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

👍

@tadeohepperle tadeohepperle merged commit 43809ee into master Aug 11, 2023
8 checks passed
@tadeohepperle tadeohepperle deleted the tadeo-hepperle-support-iterating-over-n-maps-with-partial-keys branch August 11, 2023 14:06
@jsdw jsdw mentioned this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage: Support iterating over NMaps with partial keys
4 participants