Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement StorageNMap #8635

Merged
37 commits merged into from
May 14, 2021
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
8456583
Implement StorageNMap
KiChjang Apr 5, 2021
a52724a
Change copyright date to 2021
KiChjang Apr 18, 2021
42513d2
Rewrite keys to use impl_for_tuples instead of recursion
KiChjang Apr 21, 2021
a89bf41
Implement prefix iteration on StorageNMap
KiChjang Apr 22, 2021
6b17d09
Implement EncodeLike for key arguments
KiChjang Apr 22, 2021
09b34ab
Rename KeyGenerator::Arg to KeyGenerator::KArg
KiChjang Apr 23, 2021
976e337
Support StorageNMap in decl_storage and #[pallet::storage] macros
KiChjang Apr 23, 2021
09be192
Use StorageNMap in assets pallet
KiChjang Apr 23, 2021
54b38e7
Support migrate_keys in StorageNMap
KiChjang Apr 23, 2021
a3b48a7
Reduce line characters on select files
KiChjang Apr 23, 2021
69fbed3
Refactor crate imports in decl_storage macros
KiChjang Apr 23, 2021
d5598e5
Some more line char reductions and doc comment update
KiChjang Apr 23, 2021
bbf8459
Update UI test expectations
KiChjang Apr 23, 2021
9fbf1ad
Revert whitespace changes to untouched files
KiChjang Apr 24, 2021
275f5a5
Merge branch 'master' into pr/8635
shawntabrizi Apr 24, 2021
7686c7b
Generate Key struct instead of a 1-tuple when only 1 pair of key and …
KiChjang Apr 24, 2021
b83dbf2
Revert formatting changes to unrelated files
KiChjang Apr 26, 2021
994e170
Introduce KeyGeneratorInner
KiChjang Apr 26, 2021
c3e4b9f
Add tests for StorageNMap in FRAMEv2 pallet macro
KiChjang Apr 27, 2021
55c8544
Small fixes to unit tests for StorageNMap
KiChjang Apr 27, 2021
c0b1516
Bump runtime metadata version
KiChjang Apr 27, 2021
fdc3e13
Remove unused import
KiChjang Apr 27, 2021
c041a6c
Update tests to use runtime metadata v13
KiChjang Apr 27, 2021
ce13f5e
Introduce and use EncodeLikeTuple as a trait bound for KArg
KiChjang Apr 28, 2021
1aeda76
Add some rustdocs
KiChjang Apr 30, 2021
3671857
Revert usage of StorageNMap in assets pallet
KiChjang Apr 30, 2021
707cf45
Make use of ext::PunctuatedTrailing
KiChjang Apr 30, 2021
6e264ef
Merge remote-tracking branch 'upstream/master' into storage-n-map
KiChjang May 3, 2021
e8f9740
Merge remote-tracking branch 'upstream/master' into storage-n-map
KiChjang May 12, 2021
2ff4de0
Add rustdoc for final_hash
KiChjang May 12, 2021
f75bc5a
Fix StorageNMap proc macro expansions for single key cases
KiChjang May 12, 2021
02b7ce3
Create associated const in KeyGenerator for hasher metadata
KiChjang May 12, 2021
e142267
Refactor code according to comments from Basti
KiChjang May 13, 2021
dbcb1f6
Add module docs for generator/nmap.rs
KiChjang May 13, 2021
d207d77
Re-export storage::Key as NMapKey in pallet prelude
KiChjang May 13, 2021
f259f46
Seal the EncodeLikeTuple trait
KiChjang May 13, 2021
b081885
Extract sealing code out of key.rs
KiChjang May 14, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 20 additions & 22 deletions frame/assets/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ pub use pallet::*;
pub mod pallet {
use frame_support::{
dispatch::DispatchResult,
storage,
pallet_prelude::*,
};
use frame_system::pallet_prelude::*;
Expand Down Expand Up @@ -237,12 +238,13 @@ pub mod pallet {
#[pallet::storage]
/// Approved balance transfers. First balance is the amount approved for transfer. Second
/// is the amount of `T::Currency` reserved for storing this.
pub(super) type Approvals<T: Config<I>, I: 'static = ()> = StorageDoubleMap<
pub(super) type Approvals<T: Config<I>, I: 'static = ()> = StorageNMap<
_,
Blake2_128Concat,
T::AssetId,
Blake2_128Concat,
ApprovalKey<T::AccountId>,
(
storage::Key<Blake2_128Concat, T::AssetId>,
storage::Key<Blake2_128Concat, T::AccountId>,
storage::Key<Blake2_128Concat, T::AccountId>,
),
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
Approval<T::Balance, DepositBalanceOf<T, I>>,
OptionQuery,
>;
Expand Down Expand Up @@ -504,7 +506,7 @@ pub mod pallet {
details.deposit.saturating_add(metadata.deposit),
);

Approvals::<T, I>::remove_prefix(&id);
Approvals::<T, I>::remove_prefix((id,));
Self::deposit_event(Event::Destroyed(id));

// NOTE: could use postinfo to reflect the actual number of accounts/sufficient/approvals
Expand Down Expand Up @@ -1120,19 +1122,18 @@ pub mod pallet {
let owner = ensure_signed(origin)?;
let delegate = T::Lookup::lookup(delegate)?;

let key = ApprovalKey { owner, delegate };
Approvals::<T, I>::try_mutate(id, &key, |maybe_approved| -> DispatchResult {
Approvals::<T, I>::try_mutate((id, &owner, &delegate), |maybe_approved| -> DispatchResult {
let mut approved = maybe_approved.take().unwrap_or_default();
let deposit_required = T::ApprovalDeposit::get();
if approved.deposit < deposit_required {
T::Currency::reserve(&key.owner, deposit_required - approved.deposit)?;
T::Currency::reserve(&owner, deposit_required - approved.deposit)?;
approved.deposit = deposit_required;
}
approved.amount = approved.amount.saturating_add(amount);
*maybe_approved = Some(approved);
Ok(())
})?;
Self::deposit_event(Event::ApprovedTransfer(id, key.owner, key.delegate, amount));
Self::deposit_event(Event::ApprovedTransfer(id, owner, delegate, amount));

Ok(())
}
Expand All @@ -1158,11 +1159,10 @@ pub mod pallet {
) -> DispatchResult {
let owner = ensure_signed(origin)?;
let delegate = T::Lookup::lookup(delegate)?;
let key = ApprovalKey { owner, delegate };
let approval = Approvals::<T, I>::take(id, &key).ok_or(Error::<T, I>::Unknown)?;
T::Currency::unreserve(&key.owner, approval.deposit);
let approval = Approvals::<T, I>::take((id, &owner, &delegate)).ok_or(Error::<T, I>::Unknown)?;
T::Currency::unreserve(&owner, approval.deposit);

Self::deposit_event(Event::ApprovalCancelled(id, key.owner, key.delegate));
Self::deposit_event(Event::ApprovalCancelled(id, owner, delegate));
Ok(())
}

Expand Down Expand Up @@ -1198,11 +1198,10 @@ pub mod pallet {
let owner = T::Lookup::lookup(owner)?;
let delegate = T::Lookup::lookup(delegate)?;

let key = ApprovalKey { owner, delegate };
let approval = Approvals::<T, I>::take(id, &key).ok_or(Error::<T, I>::Unknown)?;
T::Currency::unreserve(&key.owner, approval.deposit);
let approval = Approvals::<T, I>::take((id, &owner, &delegate)).ok_or(Error::<T, I>::Unknown)?;
T::Currency::unreserve(&owner, approval.deposit);

Self::deposit_event(Event::ApprovalCancelled(id, key.owner, key.delegate));
Self::deposit_event(Event::ApprovalCancelled(id, owner, delegate));
Ok(())
}

Expand Down Expand Up @@ -1236,8 +1235,7 @@ pub mod pallet {
let owner = T::Lookup::lookup(owner)?;
let destination = T::Lookup::lookup(destination)?;

let key = ApprovalKey { owner, delegate };
Approvals::<T, I>::try_mutate_exists(id, &key, |maybe_approved| -> DispatchResult {
Approvals::<T, I>::try_mutate_exists((id, &owner, delegate), |maybe_approved| -> DispatchResult {
let mut approved = maybe_approved.take().ok_or(Error::<T, I>::Unapproved)?;
let remaining = approved
.amount
Expand All @@ -1249,10 +1247,10 @@ pub mod pallet {
best_effort: false,
burn_dust: false
};
Self::do_transfer(id, &key.owner, &destination, amount, None, f)?;
Self::do_transfer(id, &owner, &destination, amount, None, f)?;

if remaining.is_zero() {
T::Currency::unreserve(&key.owner, approved.deposit);
T::Currency::unreserve(&owner, approved.deposit);
} else {
approved.amount = remaining;
*maybe_approved = Some(approved);
Expand Down
9 changes: 0 additions & 9 deletions frame/assets/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,6 @@ impl<Balance, AccountId, DepositBalance> AssetDetails<Balance, AccountId, Deposi
}
}

/// A pair to act as a key for the approval storage map.
#[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug)]
pub struct ApprovalKey<AccountId> {
/// The owner of the funds that are being approved.
pub(super) owner: AccountId,
/// The party to whom transfer of the funds is being delegated.
pub(super) delegate: AccountId,
}

/// Data concerning an approval.
#[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, Default)]
pub struct Approval<Balance, DepositBalance> {
Expand Down
17 changes: 12 additions & 5 deletions frame/metadata/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,11 @@ pub enum StorageEntryType {
value: DecodeDifferentStr,
key2_hasher: StorageHasher,
},
NMap {
keys: DecodeDifferentArray<&'static str, StringBuf>,
hashers: DecodeDifferentArray<StorageHasher>,
value: DecodeDifferentStr,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should bump the metadata version. Because third-parties won't be able to make sense of this new variant.

Thus releasing RuntimeMetadataV13:

diff --git a/frame/metadata/src/lib.rs b/frame/metadata/src/lib.rs
index a502af0798..ba232a88f1 100644
--- a/frame/metadata/src/lib.rs
+++ b/frame/metadata/src/lib.rs
@@ -369,8 +369,10 @@ pub enum RuntimeMetadata {
        V10(RuntimeMetadataDeprecated),
        /// Version 11 for runtime metadata. No longer used.
        V11(RuntimeMetadataDeprecated),
-       /// Version 12 for runtime metadata.
-       V12(RuntimeMetadataV12),
+       /// Version 12 for runtime metadata. No longer used.
+       V12(RuntimeMetadataDeprecated),
+       /// Version 13 for runtime metadata.
+       V13(RuntimeMetadataV13),
 }
 
 /// Enum that should fail.
@@ -394,7 +396,7 @@ impl Decode for RuntimeMetadataDeprecated {
 /// The metadata of a runtime.
 #[derive(Eq, Encode, PartialEq, RuntimeDebug)]
 #[cfg_attr(feature = "std", derive(Decode, Serialize))]
-pub struct RuntimeMetadataV12 {
+pub struct RuntimeMetadataV13 {
        /// Metadata of all the modules.
        pub modules: DecodeDifferentArray<ModuleMetadata>,
        /// Metadata of the extrinsic.
@@ -402,7 +404,7 @@ pub struct RuntimeMetadataV12 {
 }
 
 /// The latest version of the metadata.
-pub type RuntimeMetadataLastVersion = RuntimeMetadataV12;
+pub type RuntimeMetadataLastVersion = RuntimeMetadataV13;
 
 /// All metadata about an runtime module.
 #[derive(Clone, PartialEq, Eq, Encode, RuntimeDebug)]
@@ -430,6 +432,6 @@ impl Into<sp_core::OpaqueMetadata> for RuntimeMetadataPrefixed {
 
 impl Into<RuntimeMetadataPrefixed> for RuntimeMetadataLastVersion {
        fn into(self) -> RuntimeMetadataPrefixed {
-               RuntimeMetadataPrefixed(META_RESERVED, RuntimeMetadata::V12(self))
+               RuntimeMetadataPrefixed(META_RESERVED, RuntimeMetadata::V13(self))
        }
 }

Also we could remove other variant as they can all be expressed as a NMap but I think it is better for compatibility to keep them (basically Plain would be a NMap with keys and hashers being array of length 0, Map would be a NMap with keys and arrays of length 1, etc..). cc @jacogr

(Also it would make more sense to have something like DecodeDifferentArray<(DecodeDifferentStr, StorageHasher)> or something like this maybe, to ensure keys and hashers are of same length.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. This is the first I have seen this. This is a major revamp on my side for everything storage related.

Copy link
Contributor

@jacogr jacogr Apr 26, 2021

Choose a reason for hiding this comment

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

But yes, this is correct bumping - with metadata you can just change at will - it needs a bump to avoid issue like we have in the v9-late iteration metadata.

Copy link
Member

Choose a reason for hiding this comment

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

@jacogr we wont spring anything on you here. This should be unused in the runtime for now, and might only get used in assets in the near future for a triple map.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is fine, appreciate it. Do bear in mind that once merged, it does immediately make Substrate master non-supported (if any enum changes are made to the storage descriptors or the metadata version is bumped), even if unused in practice.

}

/// A storage entry modifier.
Expand Down Expand Up @@ -364,8 +369,10 @@ pub enum RuntimeMetadata {
V10(RuntimeMetadataDeprecated),
/// Version 11 for runtime metadata. No longer used.
V11(RuntimeMetadataDeprecated),
/// Version 12 for runtime metadata.
V12(RuntimeMetadataV12),
/// Version 12 for runtime metadata. No longer used.
V12(RuntimeMetadataDeprecated),
/// Version 13 for runtime metadata.
V13(RuntimeMetadataV13),
}

/// Enum that should fail.
Expand All @@ -389,15 +396,15 @@ impl Decode for RuntimeMetadataDeprecated {
/// The metadata of a runtime.
#[derive(Eq, Encode, PartialEq, RuntimeDebug)]
#[cfg_attr(feature = "std", derive(Decode, Serialize))]
pub struct RuntimeMetadataV12 {
pub struct RuntimeMetadataV13 {
/// Metadata of all the modules.
pub modules: DecodeDifferentArray<ModuleMetadata>,
/// Metadata of the extrinsic.
pub extrinsic: ExtrinsicMetadata,
}

/// The latest version of the metadata.
pub type RuntimeMetadataLastVersion = RuntimeMetadataV12;
pub type RuntimeMetadataLastVersion = RuntimeMetadataV13;

/// All metadata about an runtime module.
#[derive(Clone, PartialEq, Eq, Encode, RuntimeDebug)]
Expand Down Expand Up @@ -425,6 +432,6 @@ impl Into<sp_core::OpaqueMetadata> for RuntimeMetadataPrefixed {

impl Into<RuntimeMetadataPrefixed> for RuntimeMetadataLastVersion {
fn into(self) -> RuntimeMetadataPrefixed {
RuntimeMetadataPrefixed(META_RESERVED, RuntimeMetadata::V12(self))
RuntimeMetadataPrefixed(META_RESERVED, RuntimeMetadata::V13(self))
}
}
60 changes: 60 additions & 0 deletions frame/support/procedural/src/pallet/expand/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use crate::pallet::Def;
use crate::pallet::parse::storage::{Metadata, QueryKind};
use frame_support_procedural_tools::clean_type_string;
use syn::spanned::Spanned;

/// Generate the prefix_ident related the the storage.
/// prefix_ident is used for the prefix struct to be given to storage as first generic param.
Expand Down Expand Up @@ -90,6 +91,9 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
Metadata::DoubleMap { .. } => quote::quote_spanned!(storage.attr_span =>
#frame_support::storage::types::StorageDoubleMapMetadata
),
Metadata::NMap { .. } => quote::quote_spanned!(storage.attr_span =>
#frame_support::storage::types::StorageNMapMetadata
),
};

let ty = match &storage.metadata {
Expand Down Expand Up @@ -126,6 +130,31 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
value: #frame_support::metadata::DecodeDifferent::Encode(#value),
}
)
},
Metadata::NMap { keys, hashers, value, .. } => {
let keys = keys
.iter()
.map(|key| clean_type_string(&quote::quote!(#key).to_string()))
.collect::<Vec<_>>();
let hashers = hashers
.iter()
.map(|hasher| syn::Ident::new(
&clean_type_string(&quote::quote!(#hasher).to_string()),
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
hasher.span(),
))
.collect::<Vec<_>>();
let value = clean_type_string(&quote::quote!(#value).to_string());
quote::quote_spanned!(storage.attr_span =>
#frame_support::metadata::StorageEntryType::NMap {
keys: #frame_support::metadata::DecodeDifferent::Encode(&[
#( #keys, )*
]),
hashers: #frame_support::metadata::DecodeDifferent::Encode(&[
#( #frame_support::metadata::StorageHasher::#hashers, )*
]),
value: #frame_support::metadata::DecodeDifferent::Encode(#value),
}
)
}
};

Expand Down Expand Up @@ -227,6 +256,37 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream {
}
)
},
Metadata::NMap { keys, keygen, value, .. } => {
let query = match storage.query_kind.as_ref().expect("Checked by def") {
QueryKind::OptionQuery => quote::quote_spanned!(storage.attr_span =>
Option<#value>
),
QueryKind::ValueQuery => quote::quote!(#value),
};
let key_arg = if keys.len() == 1 {
quote::quote!((key,))
} else {
quote::quote!(key)
};
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
quote::quote_spanned!(storage.attr_span =>
#(#cfg_attrs)*
impl<#type_impl_gen> #pallet_ident<#type_use_gen> #completed_where_clause {
#( #docs )*
pub fn #getter<KArg>(key: KArg) -> #query
where
KArg: #frame_support::codec::EncodeLike<
<#keygen as #frame_support::storage::types::KeyGenerator>::KArg
>
+ #frame_support::storage::types::TupleToEncodedIter,
{
<
#full_ident as
#frame_support::storage::StorageNMap<#keygen, #value>
>::get(#key_arg)
}
}
)
}
}
} else {
Default::default()
Expand Down
97 changes: 94 additions & 3 deletions frame/support/procedural/src/pallet/parse/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,20 @@ impl syn::parse::Parse for PalletStorageAttr {
}

/// The value and key types used by storages. Needed to expand metadata.
pub enum Metadata{
pub enum Metadata {
Value { value: syn::GenericArgument },
Map { value: syn::GenericArgument, key: syn::GenericArgument },
DoubleMap {
value: syn::GenericArgument,
key1: syn::GenericArgument,
key2: syn::GenericArgument
},
NMap {
keys: Vec<syn::Type>,
hashers: Vec<syn::Type>,
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
keygen: syn::GenericArgument,
value: syn::GenericArgument,
},
}

pub enum QueryKind {
Expand Down Expand Up @@ -115,6 +121,80 @@ fn retrieve_arg(
}
}

/// Parse the 2nd type argument to `StorageNMap` and return its keys and hashers.
fn collect_keys_and_hashers(
keygen: &syn::GenericArgument,
) -> syn::Result<(Vec<syn::Type>, Vec<syn::Type>)> {
if let syn::GenericArgument::Type(syn::Type::Tuple(tup)) = keygen {
tup
.elems
.iter()
.try_fold((vec![], vec![]), |mut acc, ty| {
let (key, hasher) = extract_key_and_hasher(ty)?;

acc.0.push(key);
acc.1.push(hasher);
Ok(acc)
})
} else if let syn::GenericArgument::Type(ty) = keygen {
let (key, hasher) = extract_key_and_hasher(ty)?;

Ok((vec![key], vec![hasher]))
} else {
let msg = format!("Invalid pallet::storage, expected tuple of Key structs or Key struct");
Err(syn::Error::new(keygen.span(), msg))
}
}

/// In `Key<H, K>`, extract H and K and return (K, H).
fn extract_key_and_hasher(ty: &syn::Type) -> syn::Result<(syn::Type, syn::Type)> {
let typ = if let syn::Type::Path(typ) = ty {
typ
} else {
let msg = format!("Invalid pallet::storage, expected type path");
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
return Err(syn::Error::new(ty.span(), msg));
};

let key_struct = typ.path.segments.last().ok_or_else(|| {
let msg = format!("Invalid pallet::storage, expected type path with at least one segment");
syn::Error::new(typ.path.span(), msg)
})?;
if key_struct.ident != "Key" {
let msg = format!("Invalid pallet::storage, expected Key struct");
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
return Err(syn::Error::new(key_struct.ident.span(), msg));
}

let ty_params = if let syn::PathArguments::AngleBracketed(args) = &key_struct.arguments {
args
} else {
let msg = format!("Invalid pallet::storage, expected angle bracketed arguments");
KiChjang marked this conversation as resolved.
Show resolved Hide resolved
return Err(syn::Error::new(key_struct.arguments.span(), msg));
};

if ty_params.args.len() != 2 {
let msg = format!("Invalid pallet::storage, unexpected number of generic arguments \
for Key struct, expected 2 args, found {}", ty_params.args.len());
return Err(syn::Error::new(ty_params.span(), msg));
}

let hasher = match &ty_params.args[0] {
syn::GenericArgument::Type(hasher_ty) => hasher_ty.clone(),
_ => {
let msg = format!("Invalid pallet::storage, expected type");
return Err(syn::Error::new(ty_params.args[0].span(), msg));
}
};
let key = match &ty_params.args[1] {
syn::GenericArgument::Type(key_ty) => key_ty.clone(),
_ => {
let msg = format!("Invalid pallet::storage, expected type");
return Err(syn::Error::new(ty_params.args[1].span(), msg));
}
};

Ok((key, hasher))
}

impl StorageDef {
pub fn try_from(
attr_span: proc_macro2::Span,
Expand Down Expand Up @@ -177,11 +257,22 @@ impl StorageDef {
value: retrieve_arg(&typ.path.segments[0], 5)?,
}
}
"StorageNMap" => {
query_kind = retrieve_arg(&typ.path.segments[0], 3);
let keygen = retrieve_arg(&typ.path.segments[0], 1)?;
let (keys, hashers) = collect_keys_and_hashers(&keygen)?;
Metadata::NMap {
keys,
hashers,
keygen,
value: retrieve_arg(&typ.path.segments[0], 2)?,
}
}
found => {
let msg = format!(
"Invalid pallet::storage, expected ident: `StorageValue` or \
`StorageMap` or `StorageDoubleMap` in order to expand metadata, found \
`{}`",
`StorageMap` or `StorageDoubleMap` or `StorageNMap` in order \
to expand metadata, found `{}`",
found,
);
return Err(syn::Error::new(item.ty.span(), msg));
Expand Down
Loading