From 8b66cc0647d3a661df19dd2c89945634c4775f6b Mon Sep 17 00:00:00 2001 From: James Wilson Date: Fri, 18 Feb 2022 10:58:48 +0000 Subject: [PATCH 01/11] Distinct handling for N fields + 1 hasher vs N fields + N hashers --- codegen/src/api/storage.rs | 60 +++++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/codegen/src/api/storage.rs b/codegen/src/api/storage.rs index 1a6a8bb5ce..911b5acdf8 100644 --- a/codegen/src/api/storage.rs +++ b/codegen/src/api/storage.rs @@ -115,26 +115,58 @@ fn generate_storage_entry_fns( (field_name, field_type) }) .collect::>(); - // toddo: [AJ] use unzip here? - let tuple_struct_fields = - fields.iter().map(|(_, field_type)| field_type); - let field_names = fields.iter().map(|(field_name, _)| field_name); + + let field_names = fields.iter().map(|(n,_)| n); + let field_types = fields.iter().map(|(_,t)| t); + let entry_struct = quote! { - pub struct #entry_struct_ident( #( pub #tuple_struct_fields ),* ); + pub struct #entry_struct_ident( #( pub #field_types ),* ); }; let constructor = quote!( #entry_struct_ident( #( #field_names ),* ) ); - let keys = (0..tuple.fields().len()).into_iter().zip(hashers).map( - |(field, hasher)| { - let index = syn::Index::from(field); - quote!( ::subxt::StorageMapKey::new(&self.#index, #hasher) ) - }, - ); - let key_impl = quote! { - ::subxt::StorageEntryKey::Map( - vec![ #( #keys ),* ] + + let key_impl = if hashers.len() == fields.len() { + // If the number of hashers matches the number of fields, we're dealing with a + // StorageNMap, and each field should be hashed separately according to the + // corresponding hasher. + let keys = (0..tuple.fields().len()) + .into_iter() + .zip(hashers) + .map(|(field_idx, hasher)| { + let index = syn::Index::from(field_idx); + quote!( ::subxt::StorageMapKey::new(&self.#index, #hasher) ) + }); + quote!{ + ::subxt::StorageEntryKey::Map( + vec![ #( #keys ),* ] + ) + } + } else if hashers.len() == 1 { + // If there is one hasher, then however many fields we have, we want to hash a + // tuple of them using the one hasher we're told about. This corresponds to a + // StorageMap. + let hasher = hashers.get(0).expect("checked for 1 hashed"); + let items = (0..tuple.fields().len()).into_iter().map( + |field_idx| { + let index = syn::Index::from(field_idx); + quote!( &self.#index ) + } + ); + quote!{ + ::subxt::StorageEntryKey::Map( + vec![ ::subxt::StorageMapKey::new(&(#( #items ),*), #hasher) ] + ) + } + } else { + // If we hit this condition, we don't know how to handle the number of hashes vs fields + // that we've been handed, so abort. + abort_call_site!( + "Number of hashers ({}) does not equal 1 for StorageMap, or match number of fields ({}) for StorageNMap", + hashers.len(), + fields.len() ) }; + (fields, entry_struct, constructor, key_impl) } _ => { From 4963a56fd6307f810ae7b049ca587584b05d485e Mon Sep 17 00:00:00 2001 From: James Wilson Date: Fri, 18 Feb 2022 11:26:23 +0000 Subject: [PATCH 02/11] tweak comment --- codegen/src/api/storage.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/codegen/src/api/storage.rs b/codegen/src/api/storage.rs index 911b5acdf8..d0445fd97c 100644 --- a/codegen/src/api/storage.rs +++ b/codegen/src/api/storage.rs @@ -126,9 +126,9 @@ fn generate_storage_entry_fns( quote!( #entry_struct_ident( #( #field_names ),* ) ); let key_impl = if hashers.len() == fields.len() { - // If the number of hashers matches the number of fields, we're dealing with a - // StorageNMap, and each field should be hashed separately according to the - // corresponding hasher. + // If the number of hashers matches the number of fields, we're dealing with + // something shaped like a StorageNMap, and each field should be hashed separately + // according to the corresponding hasher. let keys = (0..tuple.fields().len()) .into_iter() .zip(hashers) From da9db3ed5b8939d50063f90af8fd265cbd005ee9 Mon Sep 17 00:00:00 2001 From: James Wilson Date: Fri, 18 Feb 2022 11:27:28 +0000 Subject: [PATCH 03/11] cargo fmt --- codegen/src/api/storage.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/codegen/src/api/storage.rs b/codegen/src/api/storage.rs index d0445fd97c..583f6809e3 100644 --- a/codegen/src/api/storage.rs +++ b/codegen/src/api/storage.rs @@ -116,8 +116,8 @@ fn generate_storage_entry_fns( }) .collect::>(); - let field_names = fields.iter().map(|(n,_)| n); - let field_types = fields.iter().map(|(_,t)| t); + let field_names = fields.iter().map(|(n, _)| n); + let field_types = fields.iter().map(|(_, t)| t); let entry_struct = quote! { pub struct #entry_struct_ident( #( pub #field_types ),* ); @@ -136,7 +136,7 @@ fn generate_storage_entry_fns( let index = syn::Index::from(field_idx); quote!( ::subxt::StorageMapKey::new(&self.#index, #hasher) ) }); - quote!{ + quote! { ::subxt::StorageEntryKey::Map( vec![ #( #keys ),* ] ) @@ -146,13 +146,12 @@ fn generate_storage_entry_fns( // tuple of them using the one hasher we're told about. This corresponds to a // StorageMap. let hasher = hashers.get(0).expect("checked for 1 hashed"); - let items = (0..tuple.fields().len()).into_iter().map( - |field_idx| { + let items = + (0..tuple.fields().len()).into_iter().map(|field_idx| { let index = syn::Index::from(field_idx); quote!( &self.#index ) - } - ); - quote!{ + }); + quote! { ::subxt::StorageEntryKey::Map( vec![ ::subxt::StorageMapKey::new(&(#( #items ),*), #hasher) ] ) From 1093afb0d9710ca6d2e0c9e40067064db449ba89 Mon Sep 17 00:00:00 2001 From: James Wilson Date: Fri, 18 Feb 2022 11:29:19 +0000 Subject: [PATCH 04/11] fix typo --- codegen/src/api/storage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codegen/src/api/storage.rs b/codegen/src/api/storage.rs index 583f6809e3..a55d246bd8 100644 --- a/codegen/src/api/storage.rs +++ b/codegen/src/api/storage.rs @@ -145,7 +145,7 @@ fn generate_storage_entry_fns( // If there is one hasher, then however many fields we have, we want to hash a // tuple of them using the one hasher we're told about. This corresponds to a // StorageMap. - let hasher = hashers.get(0).expect("checked for 1 hashed"); + let hasher = hashers.get(0).expect("checked for 1 hasher"); let items = (0..tuple.fields().len()).into_iter().map(|field_idx| { let index = syn::Index::from(field_idx); From 8a2125ae7450dd8247c64514a1a501ab2ef4bb20 Mon Sep 17 00:00:00 2001 From: James Wilson Date: Fri, 18 Feb 2022 15:15:10 +0000 Subject: [PATCH 05/11] Add a few storage specific tests --- subxt/tests/integration/main.rs | 2 + subxt/tests/integration/storage.rs | 100 +++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 subxt/tests/integration/storage.rs diff --git a/subxt/tests/integration/main.rs b/subxt/tests/integration/main.rs index 6540d25029..4a44d07b98 100644 --- a/subxt/tests/integration/main.rs +++ b/subxt/tests/integration/main.rs @@ -23,6 +23,8 @@ mod client; mod events; #[cfg(test)] mod frame; +#[cfg(test)] +mod storage; use test_runtime::node_runtime; use utils::*; diff --git a/subxt/tests/integration/storage.rs b/subxt/tests/integration/storage.rs new file mode 100644 index 0000000000..d8149651b4 --- /dev/null +++ b/subxt/tests/integration/storage.rs @@ -0,0 +1,100 @@ +// Copyright 2019-2022 Parity Technologies (UK) Ltd. +// This file is part of subxt. +// +// subxt is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// subxt is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with subxt. If not, see . + +use crate::{ + node_runtime::DispatchError, + pair_signer, + test_context, +}; +use sp_keyring::AccountKeyring; + +#[async_std::test] +async fn storage_plain_lookup() -> Result<(), subxt::Error> { + let ctx = test_context().await; + + // Look up a plain value + let entry = ctx.api.storage().timestamp().now(None).await?; + assert!(entry > 0); + + Ok(()) +} + +#[async_std::test] +async fn storage_map_lookup() -> Result<(), subxt::Error> { + let ctx = test_context().await; + + let signer = pair_signer(AccountKeyring::Alice.pair()); + let alice = AccountKeyring::Alice.to_account_id(); + + // Do some transaction to bump the Alice nonce to 1: + ctx.api + .tx() + .system() + .remark(vec![1, 2, 3, 4, 5]) + .sign_and_submit_then_watch(&signer) + .await? + .wait_for_finalized_success() + .await?; + + // Look up the nonce for the user (we expect it to be 1). + let entry = ctx + .api + .storage() + .system() + .account(alice.into(), None) + .await?; + assert_eq!(entry.nonce, 1); + + Ok(()) +} + +#[async_std::test] +async fn storage_n_map_storage_lookup() -> Result<(), subxt::Error> { + let ctx = test_context().await; + + // Boilerplate; we create a new asset class with ID 99, and then + // we "approveTransfer" of some of this asset class. This Gives us an + // entry in the `Approvals` StorageNMap that we can try to look up. + let signer = pair_signer(AccountKeyring::Alice.pair()); + let alice = AccountKeyring::Alice.to_account_id(); + let bob = AccountKeyring::Bob.to_account_id(); + ctx.api + .tx() + .assets() + .create(99, alice.clone().into(), 1) + .sign_and_submit_then_watch(&signer) + .await? + .wait_for_finalized_success() + .await?; + ctx.api + .tx() + .assets() + .approve_transfer(99, bob.clone().into(), 123) + .sign_and_submit_then_watch(&signer) + .await? + .wait_for_finalized_success() + .await?; + + // The actual test; look up this approval in storage: + let entry = ctx + .api + .storage() + .assets() + .approvals(99, alice.into(), bob.into(), None) + .await?; + assert_eq!(entry.map(|a| a.amount), Some(123)); + Ok(()) +} From 0be3e14ce5ea7be8d16f31ed6c1d1505c0bd8bcc Mon Sep 17 00:00:00 2001 From: James Wilson Date: Fri, 18 Feb 2022 15:27:16 +0000 Subject: [PATCH 06/11] clippy fixes --- subxt/tests/integration/storage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/subxt/tests/integration/storage.rs b/subxt/tests/integration/storage.rs index d8149651b4..1476c5c12e 100644 --- a/subxt/tests/integration/storage.rs +++ b/subxt/tests/integration/storage.rs @@ -54,7 +54,7 @@ async fn storage_map_lookup() -> Result<(), subxt::Error> { .api .storage() .system() - .account(alice.into(), None) + .account(alice, None) .await?; assert_eq!(entry.nonce, 1); @@ -93,7 +93,7 @@ async fn storage_n_map_storage_lookup() -> Result<(), subxt::Error Date: Fri, 18 Feb 2022 15:39:12 +0000 Subject: [PATCH 07/11] cargo fmt --- subxt/tests/integration/storage.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/subxt/tests/integration/storage.rs b/subxt/tests/integration/storage.rs index 1476c5c12e..905ded4ad5 100644 --- a/subxt/tests/integration/storage.rs +++ b/subxt/tests/integration/storage.rs @@ -50,12 +50,7 @@ async fn storage_map_lookup() -> Result<(), subxt::Error> { .await?; // Look up the nonce for the user (we expect it to be 1). - let entry = ctx - .api - .storage() - .system() - .account(alice, None) - .await?; + let entry = ctx.api.storage().system().account(alice, None).await?; assert_eq!(entry.nonce, 1); Ok(()) From b34f5d80b68e798f3d3671753eb00793f7d716e2 Mon Sep 17 00:00:00 2001 From: James Wilson Date: Fri, 18 Feb 2022 16:34:29 +0000 Subject: [PATCH 08/11] Add a test to specifically address this fix --- subxt/tests/integration/storage.rs | 44 +++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/subxt/tests/integration/storage.rs b/subxt/tests/integration/storage.rs index 905ded4ad5..a7e6943fbd 100644 --- a/subxt/tests/integration/storage.rs +++ b/subxt/tests/integration/storage.rs @@ -15,7 +15,10 @@ // along with subxt. If not, see . use crate::{ - node_runtime::DispatchError, + node_runtime::{ + self, + DispatchError, + }, pair_signer, test_context, }; @@ -56,6 +59,45 @@ async fn storage_map_lookup() -> Result<(), subxt::Error> { Ok(()) } +// This fails until the fix in https://github.com/paritytech/subxt/pull/458 is introduced. +// Here we create a key that looks a bit like a StorageNMap key, but should in fact be +// treated as a StorageKey (ie we should hash both values together with one hasher, rather +// than hash both values separately, or ignore the second value). +#[async_std::test] +async fn storage_n_mapish_key_is_properly_created( +) -> Result<(), subxt::Error> { + use codec::Encode; + use node_runtime::{ + runtime_types::sp_core::crypto::KeyTypeId, + session::storage::KeyOwner, + }; + use subxt::{ + storage::StorageKeyPrefix, + StorageEntry, + }; + + // This is what the generate code hashes a `session().key_owner(..)` key into: + let actual_key_bytes = KeyOwner(KeyTypeId([1, 2, 3, 4]), vec![5u8, 6, 7, 8]) + .key() + .final_key(StorageKeyPrefix::new::()) + .0; + + // Let's manually hash to what we assume it should be and compare: + let expected_key_bytes = { + // Hash the prefix to the storage entry: + let mut bytes = sp_core::twox_128("Session".as_bytes()).to_vec(); + bytes.extend(&sp_core::twox_128("KeyOwner".as_bytes())[..]); + // twox64_concat a *tuple* of the args expected: + let suffix = (KeyTypeId([1, 2, 3, 4]), vec![5u8, 6, 7, 8]).encode(); + bytes.extend(&sp_core::twox_64(&suffix)); + bytes.extend(&suffix); + bytes + }; + + assert_eq!(actual_key_bytes, expected_key_bytes); + Ok(()) +} + #[async_std::test] async fn storage_n_map_storage_lookup() -> Result<(), subxt::Error> { let ctx = test_context().await; From fa93f2d0b76bfa55fc0fc231c93ba5dd3d85bb56 Mon Sep 17 00:00:00 2001 From: James Wilson Date: Fri, 18 Feb 2022 16:56:52 +0000 Subject: [PATCH 09/11] comment typo --- subxt/tests/integration/storage.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subxt/tests/integration/storage.rs b/subxt/tests/integration/storage.rs index a7e6943fbd..3493ce876f 100644 --- a/subxt/tests/integration/storage.rs +++ b/subxt/tests/integration/storage.rs @@ -76,7 +76,7 @@ async fn storage_n_mapish_key_is_properly_created( StorageEntry, }; - // This is what the generate code hashes a `session().key_owner(..)` key into: + // 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]) .key() .final_key(StorageKeyPrefix::new::()) From 0f4f8999c1430916c791b75e2c8ff64c024a6f24 Mon Sep 17 00:00:00 2001 From: James Wilson Date: Mon, 21 Feb 2022 14:27:23 +0000 Subject: [PATCH 10/11] Address niggles --- codegen/src/api/storage.rs | 12 +++++------- subxt/tests/integration/storage.rs | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/codegen/src/api/storage.rs b/codegen/src/api/storage.rs index a55d246bd8..4c48eb3c0b 100644 --- a/codegen/src/api/storage.rs +++ b/codegen/src/api/storage.rs @@ -129,8 +129,7 @@ fn generate_storage_entry_fns( // If the number of hashers matches the number of fields, we're dealing with // something shaped like a StorageNMap, and each field should be hashed separately // according to the corresponding hasher. - let keys = (0..tuple.fields().len()) - .into_iter() + let keys = (0..fields.len()) .zip(hashers) .map(|(field_idx, hasher)| { let index = syn::Index::from(field_idx); @@ -146,11 +145,10 @@ fn generate_storage_entry_fns( // tuple of them using the one hasher we're told about. This corresponds to a // StorageMap. let hasher = hashers.get(0).expect("checked for 1 hasher"); - let items = - (0..tuple.fields().len()).into_iter().map(|field_idx| { - let index = syn::Index::from(field_idx); - quote!( &self.#index ) - }); + let items = (0..fields.len()).map(|field_idx| { + let index = syn::Index::from(field_idx); + quote!( &self.#index ) + }); quote! { ::subxt::StorageEntryKey::Map( vec![ ::subxt::StorageMapKey::new(&(#( #items ),*), #hasher) ] diff --git a/subxt/tests/integration/storage.rs b/subxt/tests/integration/storage.rs index 3493ce876f..31093a4d9d 100644 --- a/subxt/tests/integration/storage.rs +++ b/subxt/tests/integration/storage.rs @@ -103,7 +103,7 @@ async fn storage_n_map_storage_lookup() -> Result<(), subxt::Error Date: Mon, 21 Feb 2022 14:31:00 +0000 Subject: [PATCH 11/11] slgihtly nicer iter code --- codegen/src/api/storage.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codegen/src/api/storage.rs b/codegen/src/api/storage.rs index 4c48eb3c0b..8bd99675fe 100644 --- a/codegen/src/api/storage.rs +++ b/codegen/src/api/storage.rs @@ -129,8 +129,9 @@ fn generate_storage_entry_fns( // If the number of hashers matches the number of fields, we're dealing with // something shaped like a StorageNMap, and each field should be hashed separately // according to the corresponding hasher. - let keys = (0..fields.len()) - .zip(hashers) + let keys = hashers + .into_iter() + .enumerate() .map(|(field_idx, hasher)| { let index = syn::Index::from(field_idx); quote!( ::subxt::StorageMapKey::new(&self.#index, #hasher) )