From da9f88dcac328d8f6de38dd61e33078c20bd6685 Mon Sep 17 00:00:00 2001 From: gupnik Date: Tue, 11 Apr 2023 20:40:10 +0530 Subject: [PATCH] Makes storage hashers optional in dev mode (#13815) * Initial changes * Adds UI test for error when _ is used without dev_mode * Minor * ".git/.scripts/commands/fmt/fmt.sh" * Adds test to verify hasher --------- Co-authored-by: command-bot <> --- .../procedural/src/pallet/expand/storage.rs | 13 +++ .../procedural/src/pallet/parse/storage.rs | 44 +++++++--- .../dev_mode_without_arg_default_hasher.rs | 33 +++++++ ...dev_mode_without_arg_default_hasher.stderr | 19 +++++ .../tests/pallet_ui/pass/dev_mode_valid.rs | 85 ++++++++++++++++++- 5 files changed, 182 insertions(+), 12 deletions(-) create mode 100644 frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.rs create mode 100644 frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.stderr diff --git a/frame/support/procedural/src/pallet/expand/storage.rs b/frame/support/procedural/src/pallet/expand/storage.rs index 8ba9ee7681ca9..efc803033d3bf 100644 --- a/frame/support/procedural/src/pallet/expand/storage.rs +++ b/frame/support/procedural/src/pallet/expand/storage.rs @@ -269,6 +269,19 @@ pub fn process_generics(def: &mut Def) -> syn::Result (5, 6, 7), }; + if storage_def.use_default_hasher { + let hasher_indices: Vec = match storage_def.metadata { + Metadata::Map { .. } | Metadata::CountedMap { .. } => vec![1], + Metadata::DoubleMap { .. } => vec![1, 3], + _ => vec![], + }; + for hasher_idx in hasher_indices { + args.args[hasher_idx] = syn::GenericArgument::Type( + syn::parse_quote!(#frame_support::Blake2_128Concat), + ); + } + } + if query_idx < args.args.len() { if let syn::GenericArgument::Type(query_kind) = args.args.index_mut(query_idx) { set_result_query_type_parameter(query_kind)?; diff --git a/frame/support/procedural/src/pallet/parse/storage.rs b/frame/support/procedural/src/pallet/parse/storage.rs index 3ed7ce54ebd64..fee6ec9f1ca87 100644 --- a/frame/support/procedural/src/pallet/parse/storage.rs +++ b/frame/support/procedural/src/pallet/parse/storage.rs @@ -185,6 +185,8 @@ pub struct StorageDef { pub unbounded: bool, /// Whether or not reads to this storage key will be ignored by benchmarking pub whitelisted: bool, + /// Whether or not a default hasher is allowed to replace `_` + pub use_default_hasher: bool, } /// The parsed generic from the @@ -325,12 +327,12 @@ fn check_generics( } } -/// Returns `(named generics, metadata, query kind)` +/// Returns `(named generics, metadata, query kind, use_default_hasher)` fn process_named_generics( storage: &StorageKind, args_span: proc_macro2::Span, args: &[syn::Binding], -) -> syn::Result<(Option, Metadata, Option)> { +) -> syn::Result<(Option, Metadata, Option, bool)> { let mut parsed = HashMap::::new(); // Ensure no duplicate. @@ -480,15 +482,16 @@ fn process_named_generics( let metadata = generics.metadata()?; let query_kind = generics.query_kind(); - Ok((Some(generics), metadata, query_kind)) + Ok((Some(generics), metadata, query_kind, false)) } -/// Returns `(named generics, metadata, query kind)` +/// Returns `(named generics, metadata, query kind, use_default_hasher)` fn process_unnamed_generics( storage: &StorageKind, args_span: proc_macro2::Span, args: &[syn::Type], -) -> syn::Result<(Option, Metadata, Option)> { + dev_mode: bool, +) -> syn::Result<(Option, Metadata, Option, bool)> { let retrieve_arg = |arg_pos| { args.get(arg_pos).cloned().ok_or_else(|| { let msg = format!( @@ -510,18 +513,28 @@ fn process_unnamed_generics( err })?; + let use_default_hasher = |arg_pos| { + if let Some(arg) = retrieve_arg(arg_pos).ok() { + dev_mode && syn::parse2::(arg.to_token_stream()).is_ok() + } else { + false + } + }; + let res = match storage { StorageKind::Value => - (None, Metadata::Value { value: retrieve_arg(1)? }, retrieve_arg(2).ok()), + (None, Metadata::Value { value: retrieve_arg(1)? }, retrieve_arg(2).ok(), false), StorageKind::Map => ( None, Metadata::Map { key: retrieve_arg(2)?, value: retrieve_arg(3)? }, retrieve_arg(4).ok(), + use_default_hasher(1), ), StorageKind::CountedMap => ( None, Metadata::CountedMap { key: retrieve_arg(2)?, value: retrieve_arg(3)? }, retrieve_arg(4).ok(), + use_default_hasher(1), ), StorageKind::DoubleMap => ( None, @@ -531,21 +544,28 @@ fn process_unnamed_generics( value: retrieve_arg(5)?, }, retrieve_arg(6).ok(), + use_default_hasher(1) && use_default_hasher(3), ), StorageKind::NMap => { let keygen = retrieve_arg(1)?; let keys = collect_keys(&keygen)?; - (None, Metadata::NMap { keys, keygen, value: retrieve_arg(2)? }, retrieve_arg(3).ok()) + ( + None, + Metadata::NMap { keys, keygen, value: retrieve_arg(2)? }, + retrieve_arg(3).ok(), + false, + ) }, }; Ok(res) } -/// Returns `(named generics, metadata, query kind)` +/// Returns `(named generics, metadata, query kind, use_default_hasher)` fn process_generics( segment: &syn::PathSegment, -) -> syn::Result<(Option, Metadata, Option)> { + dev_mode: bool, +) -> syn::Result<(Option, Metadata, Option, bool)> { let storage_kind = match &*segment.ident.to_string() { "StorageValue" => StorageKind::Value, "StorageMap" => StorageKind::Map, @@ -583,7 +603,7 @@ fn process_generics( _ => unreachable!("It is asserted above that all generics are types"), }) .collect::>(); - process_unnamed_generics(&storage_kind, args_span, &args) + process_unnamed_generics(&storage_kind, args_span, &args, dev_mode) } else if args.args.iter().all(|gen| matches!(gen, syn::GenericArgument::Binding(_))) { let args = args .args @@ -711,7 +731,8 @@ impl StorageDef { return Err(syn::Error::new(item.ty.span(), msg)) } - let (named_generics, metadata, query_kind) = process_generics(&typ.path.segments[0])?; + let (named_generics, metadata, query_kind, use_default_hasher) = + process_generics(&typ.path.segments[0], dev_mode)?; let query_kind = query_kind .map(|query_kind| { @@ -832,6 +853,7 @@ impl StorageDef { named_generics, unbounded, whitelisted, + use_default_hasher, }) } } diff --git a/frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.rs b/frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.rs new file mode 100644 index 0000000000000..fb1139479566a --- /dev/null +++ b/frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.rs @@ -0,0 +1,33 @@ +#![cfg_attr(not(feature = "std"), no_std)] + +pub use pallet::*; + +#[frame_support::pallet] +pub mod pallet { + use frame_support::pallet_prelude::*; + + // The struct on which we build all of our Pallet logic. + #[pallet::pallet] + pub struct Pallet(_); + + // Your Pallet's configuration trait, representing custom external types and interfaces. + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::storage] + type MyStorage = StorageValue<_, Vec>; + + #[pallet::storage] + type MyStorageMap = StorageMap<_, _, u32, u64>; + + #[pallet::storage] + type MyStorageDoubleMap = StorageDoubleMap<_, _, u32, _, u64, u64>; + + #[pallet::storage] + type MyCountedStorageMap = CountedStorageMap<_, _, u32, u64>; + + // Your Pallet's internal functions. + impl Pallet {} +} + +fn main() {} diff --git a/frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.stderr b/frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.stderr new file mode 100644 index 0000000000000..5317f5c52b9b6 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/dev_mode_without_arg_default_hasher.stderr @@ -0,0 +1,19 @@ +error[E0121]: the placeholder `_` is not allowed within types on item signatures for type aliases + --> tests/pallet_ui/dev_mode_without_arg_default_hasher.rs:21:47 + | +21 | type MyStorageMap = StorageMap<_, _, u32, u64>; + | ^ not allowed in type signatures + +error[E0121]: the placeholder `_` is not allowed within types on item signatures for type aliases + --> tests/pallet_ui/dev_mode_without_arg_default_hasher.rs:24:59 + | +24 | type MyStorageDoubleMap = StorageDoubleMap<_, _, u32, _, u64, u64>; + | ^ ^ not allowed in type signatures + | | + | not allowed in type signatures + +error[E0121]: the placeholder `_` is not allowed within types on item signatures for type aliases + --> tests/pallet_ui/dev_mode_without_arg_default_hasher.rs:27:61 + | +27 | type MyCountedStorageMap = CountedStorageMap<_, _, u32, u64>; + | ^ not allowed in type signatures diff --git a/frame/support/test/tests/pallet_ui/pass/dev_mode_valid.rs b/frame/support/test/tests/pallet_ui/pass/dev_mode_valid.rs index 97e0d585d0ce9..483ed9579bd0a 100644 --- a/frame/support/test/tests/pallet_ui/pass/dev_mode_valid.rs +++ b/frame/support/test/tests/pallet_ui/pass/dev_mode_valid.rs @@ -1,5 +1,11 @@ #![cfg_attr(not(feature = "std"), no_std)] +use frame_support::{ + traits::{ + ConstU32, + }, +}; + pub use pallet::*; #[frame_support::pallet(dev_mode)] @@ -19,6 +25,16 @@ pub mod pallet { #[pallet::storage] type MyStorage = StorageValue<_, Vec>; + // The Hasher requirement skipped by `dev_mode`. + #[pallet::storage] + pub type MyStorageMap = StorageMap<_, _, u32, u64>; + + #[pallet::storage] + type MyStorageDoubleMap = StorageDoubleMap<_, _, u32, _, u64, u64>; + + #[pallet::storage] + type MyCountedStorageMap = CountedStorageMap<_, _, u32, u64>; + // Your Pallet's callable functions. #[pallet::call] impl Pallet { @@ -32,4 +48,71 @@ pub mod pallet { impl Pallet {} } -fn main() {} +impl frame_system::Config for Runtime { + type BaseCallFilter = frame_support::traits::Everything; + type RuntimeOrigin = RuntimeOrigin; + type Index = u64; + type BlockNumber = u32; + type RuntimeCall = RuntimeCall; + type Hash = sp_runtime::testing::H256; + type Hashing = sp_runtime::traits::BlakeTwo256; + type AccountId = u64; + type Lookup = sp_runtime::traits::IdentityLookup; + type Header = Header; + type RuntimeEvent = RuntimeEvent; + type BlockHashCount = ConstU32<250>; + type BlockWeights = (); + type BlockLength = (); + type DbWeight = (); + type Version = (); + type PalletInfo = PalletInfo; + type AccountData = (); + type OnNewAccount = (); + type OnKilledAccount = (); + type SystemWeightInfo = (); + type SS58Prefix = (); + type OnSetCode = (); + type MaxConsumers = ConstU32<16>; +} + +pub type Header = sp_runtime::generic::Header; +pub type Block = sp_runtime::generic::Block; +pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic; + +frame_support::construct_runtime!( + pub struct Runtime where + Block = Block, + NodeBlock = Block, + UncheckedExtrinsic = UncheckedExtrinsic + { + // Exclude part `Storage` in order not to check its metadata in tests. + System: frame_system exclude_parts { Pallet, Storage }, + Example: pallet, + } +); + +impl pallet::Config for Runtime { + +} + +fn main() { + use frame_support::{pallet_prelude::*}; + use storage::unhashed; + use sp_io::{ + hashing::{blake2_128, twox_128}, + TestExternalities, + }; + + fn blake2_128_concat(d: &[u8]) -> Vec { + let mut v = blake2_128(d).to_vec(); + v.extend_from_slice(d); + v + } + + TestExternalities::default().execute_with(|| { + pallet::MyStorageMap::::insert(1, 2); + let mut k = [twox_128(b"Example"), twox_128(b"MyStorageMap")].concat(); + k.extend(1u32.using_encoded(blake2_128_concat)); + assert_eq!(unhashed::get::(&k), Some(2u64)); + }); +}