Reference key storage api #447

lexnv · 2022-02-14T15:14:20Z

A StorageEntryType::Plain type requires no changes as it receives no arguments.

Changes

Structs receive a named lifetime and take a reference to what they previously contained.
If they contain multiple fields, all are bounded to the same lifetime.
With the exception of the storage::Account, which is an associated type for DefaultAccountData trait's implementation.

pub struct PendingSwap(pub runtime_types::polkadot_parachain::primitives::Id)

pub struct PendingSwap<'a>(pub &'a runtime_types::polkadot_parachain::primitives::Id);

StorageEntry trait is implemented for an anonymous lifetime.

impl ::subxt::StorageEntry for PendingSwap {

impl ::subxt::StorageEntry for PendingSwap<'_> {

Storage API uses reference for key arguments

pub async fn pending_swap(
  &self,
  _0: runtime_types::polkadot_parachain::primitives::Id,
  hash: ::core::option::Option<T::Hash>,
)

pub async fn pending_swap(
  &self,
  _0: &runtime_types::polkadot_parachain::primitives::Id,
  hash: ::core::option::Option<T::Hash>,
)

Vector references are replaced by slices

_0: &::std::vec::Vec<::core::primitive::u8>

0: &[::core::primitive::u8]

Lastly, the iter methods bound the lifetime of the generated structs to the named lifetime of the StorageClient.

pub async fn pending_swap_iter(
    &self,
    hash: ::core::option::Option<T::Hash>,
) -> ::core::result::Result<
    ::subxt::KeyIter<'a, T, PendingSwap>,
    ::subxt::BasicError,
> {

pub async fn pending_swap_iter(
    &self,
    hash: ::core::option::Option<T::Hash>,
) -> ::core::result::Result<
    ::subxt::KeyIter<'a, T, PendingSwap<'a>>,
    ::subxt::BasicError,
> {

closes #411.

Example Output

Staking active era: index: 1, start: Some(1645114260002)
  Alice account id:        d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d (5GrwvaEF...)
  Alice//stash account id: be5ddb1579b72e84524fc29e78609e3caf42e85aa118ebfe0b0ad404b5bdd25f (5GNJqTPy...)
    account controlled by: d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d (5GrwvaEF...)
Era reward points: EraRewardPoints { total: 20740, individual: {be5ddb1579b72e84524fc29e78609e3caf42e85aa118ebfe0b0ad404b5bdd25f (5GNJqTPy...): 20740} }

Next Steps

Fix clippy due to &Vec<_>
Add storage example

polkadot commit-hash: d96d3bea85 polkadot tag: v0.9.16-rc2 Signed-off-by: Alexandru Vasile <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

cli/src/main.rs

This reverts commit 2970d05. Revert "cli: Fix polkadot.rs license check" This reverts commit 6fe8818.

jsdw · 2022-02-14T16:52:22Z

codegen/src/api/storage.rs

                }
                _ => {
+                    let should_ref = storage_entry.name != "Account";


So I think this is the case because of the DefaultAccountData thing that's generated not taking a ref?

I don't really like the inconsistency this would introduce in the storage API (this call doesn't take a ref but other similar calls do). I think it'd be good to see whether the generated DefaultAccountData impl can be tweaked to take/allow the ref. @ascjones any thoughts on this?

jsdw

Looking good overall! I'm wondering whether we can avoid the inconsistency/hardcoded string w.r.t that Account entry though :)

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw · 2022-02-15T17:23:03Z

So I had a bit of a dig into this area, and it looks like the purpose of AccountData is basically to generate the correct storage key, and then decode a nonce from the storage value that comes back when node storage is queried with that key (the nonce being the number of transactions from the account, and is needed to be provided with each transaction).

See

subxt/subxt/src/client.rs

Line 268 in cb9177a

A::storage_entry(signer.account_id().clone().into());

.

By default this trait is implemented for some default enum which points at the Account storage key (see

subxt/codegen/src/api/mod.rs

Line 452 in cb9177a

impl ::subxt::AccountData for #default_impl_name {

). This makes it hard or impossible to use an Account struct that has a lifetime at the mo, which is why you currently are not adding a lifetime to a struct that looks like "Account".

In order to have a consistent storage API where everything provided is a reference (rather than having some exceptional thing that is not a reference), one approach I think would be to generate a version of the "system::Account" type with a reference to be consistent with the rest of the API, but to also generate a version of it without a reference. This non-reference version could then be used by that default AccountData impl instead :)

You'll need to look for the correct type to do this with sortof like you do now, but it's worth making sure that the entire path of the type is correct (ie it is ["system", "Account"], and not for instance ["balances", "Account"]) (I had to do the same thing here:

subxt/codegen/src/api/errors.rs

Line 80 in cb9177a

.find(|&ty| ty.ty().path().segments() == ["sp_runtime", "DispatchError"])

).

@ascjones does that sound reasonable to you?

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw · 2022-02-16T11:37:03Z

codegen/src/api/storage.rs

                    let ty_path = type_gen.resolve_type_path(key.id(), &[]);
                    let fields = vec![(format_ident!("_0"), ty_path.clone())];
+                    // `::system::storage::Account` was utilized as associated type `StorageEntry`
+                    // for `::subxt::AccountData` implementation of generated `DefaultAccountData`.


Suggested change

// for `::subxt::AccountData` implementation of generated `DefaultAccountData`.

// for `::subxt::AccountData` implementation by the generated `DefaultAccountData`.

codegen/src/api/storage.rs

jsdw · 2022-02-16T11:47:56Z

codegen/src/api/storage.rs

@@ -74,12 +74,15 @@ fn generate_storage_entry_fns(
    storage_entry: &StorageEntryMetadata<PortableForm>,
 ) -> (TokenStream2, TokenStream2) {
    let entry_struct_ident = format_ident!("{}", storage_entry.name);
-    let (fields, entry_struct, constructor, key_impl) = match storage_entry.ty {
+    let is_account_wrapper = pallet.name == "System" && storage_entry.name == "Account";
+    let wrapper_struct_ident = format_ident!("{}DefaultData", storage_entry.name);


nitpicking: I'm not super fond of the name AccountDefaultData; I'd lean towards something like AccountOwned I guess.. but I'm not too fussed :D

AccountOwned sounds better :D Thanks

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw · 2022-02-17T16:25:19Z

codegen/src/types/type_path.rs

+        match self {
+            TypePath::Type(ty) => {
+                match ty.ty.type_def() {
+                    TypeDef::Sequence(_) => {


I think things like HashMap and HashSet and such also encode to a TypeDef::Sequence, so you'll probably also need to check that the .path() == &["Vec"] :)

I think we currently substitute any Sequence<ty> with std::vec<ty> (from type_path.rs). It does make sense to constrain it further, but the ty.ty.path() is empty in this case. Any other ways we could constrain it to just vec?

As we discussed, my comment wasn't correct in fact, and it looks like only Vec and VecDeque are represented as Sequences, and so I think this is ok!

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw · 2022-03-01T15:00:08Z

codegen/src/types/type_path.rs

+            TypePath::Type(ty) => {
+                match ty.ty.type_def() {
+                    TypeDef::Sequence(_) => {
+                        if ty.params.is_empty() {


Looking at the scale-info crate it looks like you could more safely do:

TypeDef::Sequence(seq) => { let type_param = seq.type_param() // ...

I'd also lean towards not bothering to quote! the type_param before returning it (I think it'll work just fine if you don't), but I'm not too bothered either way!

Also as a matter of personal taste I'd probably avoid the indentation with some early returns, a bit like:

let ty = match self { TypePath::Type(ty) => ty, _ => return None }; let sequence = match ty.ty.type_def() { TypeDef::Sequence(seq) => seq, _ => return None; }; // ...

That makes a lot of sense, and indeed the code looks a lot better 😄

I did give the type_param() a try, but it seems to return a UntrackedSymbol<TypeId> which does not implement the ToToken trait.
Looking below in the code generation, it seems under TypeDef::Sequence there is the assumption of always having at least one type_param. I've removed the is_empty() check here for consistency.
Wondering if we could run into problems along the line due to this assumption. 🤔

jsdw · 2022-03-01T15:02:59Z

examples/examples/staking_details.rs

+// You should have received a copy of the GNU General Public License
+// along with subxt.  If not, see <http://www.gnu.org/licenses/>.
+
+//! To run this example, a local polkadot node should be running. Example verified against polkadot 0.9.13-82616422d0-aarch64-macos.


Question: Is this "Example verified against $version" correct for you? I'd either remove it or, better, use polkadot --version to put whatever version you're using in (if it's different) :)

Or, looking at the below, perhaps the example is using a substrate instance rather than a polkadot one (which the other examples are using)? I'd prolly stick to polkadot just to be the same as the other examples (and consistent with the comments) :)

jsdw · 2022-03-01T15:10:08Z

examples/examples/staking_details.rs

+    let controller_acc = api
+        .storage()
+        .staking()
+        .bonded(&alice_stash_id, None)


Cool!

I'm wondering whether it is worth renaming this ewxample to fetch_staking_details.rs so that it lines up with the other fetch example (both are about fetching stuff out of storage)?

jsdw · 2022-03-01T15:10:47Z

subxt/tests/integration/frame/balances.rs

        .await?;
    let bob_pre = api
        .storage()
        .system()
-        .account(bob.account_id().clone(), None)
+        .account(bob.account_id(), None)


It's nice not having to clone these any more!

jsdw · 2022-03-01T15:11:41Z

subxt/tests/integration/frame/staking.rs

-    let ctx = test_context().await;
-    let current_era_result = ctx
+    let cxt = test_context().await;
+    let eras = 0;


I think it's just era :)

jsdw · 2022-03-01T15:12:10Z

subxt/tests/integration/storage.rs

@@ -79,7 +79,7 @@ async fn storage_n_mapish_key_is_properly_created(
    };

    // This is what the generated code hashes a `session().key_owner(..)` key into:
-    let actual_key_bytes = KeyOwner(KeyTypeId([1, 2, 3, 4]), vec![5u8, 6, 7, 8])
+    let actual_key_bytes = KeyOwner(&KeyTypeId([1, 2, 3, 4]), &[5u8, 6, 7, 8])


Thanks! Having the API as reference does improve the ergonomics IMO :D

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw · 2022-03-01T17:45:20Z

codegen/src/types/type_path.rs

+        match ty.ty.type_def() {
+            TypeDef::Sequence(_) => Some(&ty.params[0]),
+            _ => None,
+        }


Looking neater; nice :)

jsdw · 2022-03-01T17:54:36Z

examples/examples/fetch_staking_details.rs

+    // Get Alice' Stash account ID
+    //
+    // *Note*: This is pre-funded, as declared in the [genesis state]
+    // (https://github.com/substrate-developer-hub/substrate-node-template/blob/main/node/src/chain_spec.rs#L49)


Because it's polkadot and not substrate, I think the link would point to something more like:

https://github.com/paritytech/polkadot/blob/dc784f9b47e4681897cfd477b4f0760330875a87/node/service/src/chain_spec.rs#L1277

But in other examples we've taken for granted that some test accounts have funds in, so I wouldn't bother with a comment about it myself :)

Thanks, i ve removed it for now :D

jsdw

You'll need to merge in the latest stuff on master and then re-generate that polkadot.rs file, but otherwise lgtm!

Signed-off-by: Alexandru Vasile <[email protected]>

ascjones

This is a huge usability improvement!

While I'm not a huge fan of the speciaI cased codegen for the Account storage item, I can see that the AccountOwned workaround is necessary to make this work with the current storage API, and I can't come up with anything better.

However we should keep an open mind as to how we can improve this in the future to make the codegen less convoluted.

One avenue to explore would be to modify fetch and fetch_or_default to accept preconstructed StorageKeys, and have the macro construct those keys inline instead of generating and calling StorageEntry::key. Then could modify the AccountData trait to have a method which constructs the StorageKey from the AccountId.

This will all need a bit more thought, so not in the scope of this PR.

lexnv added 6 commits February 14, 2022 14:27

codegen: Update polkadot.rs

b9d419f

polkadot commit-hash: d96d3bea85 polkadot tag: v0.9.16-rc2 Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Reference key storage api

eb768d8

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Regenerate polkadot.rs with reference api

0419623

Signed-off-by: Alexandru Vasile <[email protected]>

tests: Update tests with reference interface

9a2c9ca

Signed-off-by: Alexandru Vasile <[email protected]>

cli: Fix polkadot.rs license check

6fe8818

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Update polkadot.rs with copyright

2970d05

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw reviewed Feb 14, 2022

View reviewed changes

cli/src/main.rs Outdated Show resolved Hide resolved

Revert "codegen: Update polkadot.rs with copyright"

bfc7fe1

This reverts commit 2970d05. Revert "cli: Fix polkadot.rs license check" This reverts commit 6fe8818.

jsdw reviewed Feb 14, 2022

View reviewed changes

codegen: Implement AccountData trait in the expected order

553573a

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv added 7 commits February 15, 2022 20:10

codegen: Store implementation of StorageEntry

cd65398

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Generate AccountDefaultData wrapper struct

9651879

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Allow Account references

9816182

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Update polkadot.rs

7c19fde

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Utilize AccountDefaultData instead of Account

f02ccf8

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Update polkadot.rs

7cb306a

Signed-off-by: Alexandru Vasile <[email protected]>

tests: Update tests to utilize Account reference

a8d709c

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw reviewed Feb 16, 2022

View reviewed changes

codegen/src/api/storage.rs Show resolved Hide resolved

jsdw reviewed Feb 16, 2022

View reviewed changes

lexnv added 8 commits February 16, 2022 14:54

codegen: Rename AccountDefaultData to AccountOwned

e5c8db7

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Add comments for wrapper account

8925c3f

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Obtain vector type parameter for TypePath::Type

b06264d

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Use slices instead of & std::vec in storage API

4b7806e

Signed-off-by: Alexandru Vasile <[email protected]>

codegen: Update polkadot.rs

508116d

Signed-off-by: Alexandru Vasile <[email protected]>

Merge remote-tracking branch 'origin/master' into 411_ref_key_storage

f09a213

codegen: Fix documentation

763a836

Signed-off-by: Alexandru Vasile <[email protected]>

tests: Remove extra reference

fc352ac

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw reviewed Feb 17, 2022

View reviewed changes

lexnv added 2 commits February 17, 2022 19:56

examples: Add staking example to exercise storage API

c94a21f

Signed-off-by: Alexandru Vasile <[email protected]>

Merge remote-tracking branch 'origin/master' into 411_ref_key_storage

4465ef2

jsdw requested a review from ascjones March 1, 2022 10:30

lexnv added 4 commits March 1, 2022 14:15

Merge remote-tracking branch 'origin/master' into 411_temp

8b13c4c

codegen: Update polkadot.rs

fa9f03a

Signed-off-by: Alexandru Vasile <[email protected]>

tests: Update storage tests

d541736

Signed-off-by: Alexandru Vasile <[email protected]>

Fix cargo clippy

2bcd009

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw reviewed Mar 1, 2022

View reviewed changes

lexnv added 5 commits March 1, 2022 18:17

codegen: Simplify vec_type_param

b9e84c6

Signed-off-by: Alexandru Vasile <[email protected]>

examples: Rename staking_details.rs to fetch_staking_details.rs

bb97e6a

Signed-off-by: Alexandru Vasile <[email protected]>

tests: Remove dummy variable

ac98865

Signed-off-by: Alexandru Vasile <[email protected]>

examples: Update polkadot version

6bee03b

Signed-off-by: Alexandru Vasile <[email protected]>

Apply rust-fmt

d485513

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw reviewed Mar 1, 2022

View reviewed changes

jsdw approved these changes Mar 1, 2022

View reviewed changes

lexnv added 3 commits March 2, 2022 11:12

Merge remote-tracking branch 'origin/master' into 411_ref_key_storage

3420b86

codegen: Regenerate polkadot.rs

a17058a

Signed-off-by: Alexandru Vasile <[email protected]>

examples: Remove comment

66fc95a

Signed-off-by: Alexandru Vasile <[email protected]>

ascjones approved these changes Mar 2, 2022

View reviewed changes

lexnv merged commit 08369f3 into paritytech:master Mar 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reference key storage api #447

Reference key storage api #447

lexnv commented Feb 14, 2022 •

edited

Loading

jsdw Feb 14, 2022

jsdw left a comment

jsdw commented Feb 15, 2022 •

edited

Loading

jsdw Feb 16, 2022

jsdw Feb 16, 2022

lexnv Feb 16, 2022

jsdw Feb 17, 2022

lexnv Feb 18, 2022

jsdw Feb 18, 2022

jsdw Mar 1, 2022

lexnv Mar 1, 2022

jsdw Mar 1, 2022

jsdw Mar 1, 2022 •

edited

Loading

jsdw Mar 1, 2022

jsdw Mar 1, 2022

jsdw Mar 1, 2022

jsdw Mar 1, 2022

lexnv Mar 1, 2022

jsdw Mar 1, 2022

jsdw Mar 1, 2022

lexnv Mar 2, 2022

jsdw left a comment

ascjones left a comment

	// for `::subxt::AccountData` implementation of generated `DefaultAccountData`.
	// for `::subxt::AccountData` implementation by the generated `DefaultAccountData`.

Reference key storage api #447

Reference key storage api #447

Conversation

lexnv commented Feb 14, 2022 • edited Loading

Changes

Example Output

Next Steps

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

jsdw commented Feb 15, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

jsdw Mar 1, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

lexnv commented Feb 14, 2022 •

edited

Loading

jsdw commented Feb 15, 2022 •

edited

Loading

jsdw Mar 1, 2022 •

edited

Loading