From fe800bc9d61ef1efb9941cdab8a273c9dd2794f1 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 14 May 2021 11:18:05 -0700 Subject: [PATCH 1/5] Use StorageNMap for Approvals in assets pallet --- frame/assets/src/lib.rs | 41 ++++++++++++++++++--------------------- frame/assets/src/types.rs | 9 --------- 2 files changed, 19 insertions(+), 31 deletions(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 3a2b1a6ce21dd..b498cd7990363 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -237,12 +237,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, I: 'static = ()> = StorageDoubleMap< + pub(super) type Approvals, I: 'static = ()> = StorageNMap< _, - Blake2_128Concat, - T::AssetId, - Blake2_128Concat, - ApprovalKey, + ( + NMapKey, + NMapKey, + NMapKey, + ), Approval>, OptionQuery, >; @@ -502,7 +503,7 @@ pub mod pallet { details.deposit.saturating_add(metadata.deposit), ); - Approvals::::remove_prefix(&id); + Approvals::::remove_prefix((id,)); Self::deposit_event(Event::Destroyed(id)); // NOTE: could use postinfo to reflect the actual number of accounts/sufficient/approvals @@ -1118,19 +1119,18 @@ pub mod pallet { let owner = ensure_signed(origin)?; let delegate = T::Lookup::lookup(delegate)?; - let key = ApprovalKey { owner, delegate }; - Approvals::::try_mutate(id, &key, |maybe_approved| -> DispatchResult { + Approvals::::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(()) } @@ -1156,11 +1156,10 @@ pub mod pallet { ) -> DispatchResult { let owner = ensure_signed(origin)?; let delegate = T::Lookup::lookup(delegate)?; - let key = ApprovalKey { owner, delegate }; - let approval = Approvals::::take(id, &key).ok_or(Error::::Unknown)?; - T::Currency::unreserve(&key.owner, approval.deposit); + let approval = Approvals::::take((id, &owner, &delegate)).ok_or(Error::::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(()) } @@ -1196,11 +1195,10 @@ pub mod pallet { let owner = T::Lookup::lookup(owner)?; let delegate = T::Lookup::lookup(delegate)?; - let key = ApprovalKey { owner, delegate }; - let approval = Approvals::::take(id, &key).ok_or(Error::::Unknown)?; - T::Currency::unreserve(&key.owner, approval.deposit); + let approval = Approvals::::take((id, &owner, &delegate)).ok_or(Error::::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(()) } @@ -1234,8 +1232,7 @@ pub mod pallet { let owner = T::Lookup::lookup(owner)?; let destination = T::Lookup::lookup(destination)?; - let key = ApprovalKey { owner, delegate }; - Approvals::::try_mutate_exists(id, &key, |maybe_approved| -> DispatchResult { + Approvals::::try_mutate_exists((id, &owner, delegate), |maybe_approved| -> DispatchResult { let mut approved = maybe_approved.take().ok_or(Error::::Unapproved)?; let remaining = approved .amount @@ -1247,10 +1244,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); diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index f3f17c00a218f..0cfcb64e137f2 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -65,15 +65,6 @@ impl AssetDetails { - /// 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 { From a6e7c7ff5ba9d331789d67128f7a0d737b1abc41 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 14 May 2021 11:33:51 -0700 Subject: [PATCH 2/5] Use EncodeLike on HashKeyPrefix trait bounds --- frame/assets/src/lib.rs | 2 +- frame/support/src/storage/types/key.rs | 50 +++++++++++++++----------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index b498cd7990363..228d286fee10f 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -503,7 +503,7 @@ pub mod pallet { details.deposit.saturating_add(metadata.deposit), ); - Approvals::::remove_prefix((id,)); + Approvals::::remove_prefix((&id,)); Self::deposit_event(Event::Destroyed(id)); // NOTE: could use postinfo to reflect the actual number of accounts/sufficient/approvals diff --git a/frame/support/src/storage/types/key.rs b/frame/support/src/storage/types/key.rs index fb3c69ff20cde..c951a7f8fe942 100755 --- a/frame/support/src/storage/types/key.rs +++ b/frame/support/src/storage/types/key.rs @@ -248,19 +248,19 @@ pub trait HasReversibleKeyPrefix

: ReversibleKeyGenerator + HasKeyPrefix

{ macro_rules! impl_key_prefix_for { (($($keygen:ident),+), ($($prefix:ident),+), ($($suffix:ident),+)) => { paste! { - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher),+> - HasKeyPrefix<($($prefix),+)> for + impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher,)+ $( []: EncodeLike<$prefix> ),+> + HasKeyPrefix<($( [] ),+)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { type Suffix = ($($suffix),+); - fn partial_key(prefix: ($($prefix),+)) -> Vec { + fn partial_key(prefix: ($( [] ),+)) -> Vec { <($(Key<[<$prefix $prefix>], $prefix>),+)>::final_key(prefix) } } - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher),+> - HasReversibleKeyPrefix<($($prefix),+)> for + impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ $( []: EncodeLike<$prefix> ),+> + HasReversibleKeyPrefix<($( [] ),+)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { fn decode_partial_key(key_material: &[u8]) -> Result { @@ -271,19 +271,19 @@ macro_rules! impl_key_prefix_for { }; (($($keygen:ident),+), $prefix:ident, ($($suffix:ident),+)) => { paste! { - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher),+> - HasKeyPrefix<($prefix,)> for + impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher,)+ []: EncodeLike<$prefix>> + HasKeyPrefix<( [] ,)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { type Suffix = ($($suffix),+); - fn partial_key(prefix: ($prefix,)) -> Vec { + fn partial_key(prefix: ( [] ,)) -> Vec { ], $prefix>>::final_key(prefix) } } - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher),+> - HasReversibleKeyPrefix<($prefix,)> for + impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ []: EncodeLike<$prefix>> + HasReversibleKeyPrefix<( [] ,)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { fn decode_partial_key(key_material: &[u8]) -> Result { @@ -294,19 +294,19 @@ macro_rules! impl_key_prefix_for { }; (($($keygen:ident),+), ($($prefix:ident),+), $suffix:ident) => { paste! { - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher),+> - HasKeyPrefix<($($prefix),+)> for + impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher,)+ $( []: EncodeLike<$prefix>),+> + HasKeyPrefix<($( [] ),+)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { type Suffix = $suffix; - fn partial_key(prefix: ($($prefix),+)) -> Vec { + fn partial_key(prefix: ($( [] ),+)) -> Vec { <($(Key<[<$prefix $prefix>], $prefix>),+)>::final_key(prefix) } } - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher),+> - HasReversibleKeyPrefix<($($prefix),+)> for + impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ $( []: EncodeLike<$prefix> ),+> + HasReversibleKeyPrefix<($( [] ),+)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { fn decode_partial_key(key_material: &[u8]) -> Result { @@ -317,18 +317,28 @@ macro_rules! impl_key_prefix_for { }; } -impl HasKeyPrefix<(A,)> - for (Key, Key) +impl HasKeyPrefix<(KArg,)> for (Key, Key) +where + A: FullCodec, + B: FullCodec, + X: StorageHasher, + Y: StorageHasher, + KArg: EncodeLike, { type Suffix = B; - fn partial_key(prefix: (A,)) -> Vec { + fn partial_key(prefix: (KArg,)) -> Vec { >::final_key(prefix) } } -impl - HasReversibleKeyPrefix<(A,)> for (Key, Key) +impl HasReversibleKeyPrefix<(KArg,)> for (Key, Key) +where + A: FullCodec, + B: FullCodec, + X: ReversibleStorageHasher, + Y: ReversibleStorageHasher, + KArg: EncodeLike, { fn decode_partial_key(key_material: &[u8]) -> Result { >::decode_final_key(key_material).map(|k| k.0) From 9fefd5b2d2067456028183ee7dfb6dddd1ece0b3 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 14 May 2021 11:45:39 -0700 Subject: [PATCH 3/5] Add comments clarifying AccountId roles --- frame/assets/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 228d286fee10f..82753c8fb3965 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -241,8 +241,10 @@ pub mod pallet { _, ( NMapKey, - NMapKey, - NMapKey, + // owner + NMapKey, // owner + // delegate + NMapKey, // delegate ), Approval>, OptionQuery, From cf00594a531f24b9326b8edac0568da8d534b60b Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 14 May 2021 11:49:55 -0700 Subject: [PATCH 4/5] Properly document the keys in the Approvals storage --- frame/assets/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index 82753c8fb3965..9cdd4c0b914eb 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -237,13 +237,12 @@ 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. + /// First key is the asset ID, second key is the owner and third key is the delegate. pub(super) type Approvals, I: 'static = ()> = StorageNMap< _, ( NMapKey, - // owner NMapKey, // owner - // delegate NMapKey, // delegate ), Approval>, From 3f59869615a0ae9913be39e3086327118fb5e695 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 14 May 2021 11:57:49 -0700 Subject: [PATCH 5/5] Fix line width --- frame/support/src/storage/types/key.rs | 48 ++++++++++++++++---------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/frame/support/src/storage/types/key.rs b/frame/support/src/storage/types/key.rs index c951a7f8fe942..5eb608233b85d 100755 --- a/frame/support/src/storage/types/key.rs +++ b/frame/support/src/storage/types/key.rs @@ -248,10 +248,11 @@ pub trait HasReversibleKeyPrefix

: ReversibleKeyGenerator + HasKeyPrefix

{ macro_rules! impl_key_prefix_for { (($($keygen:ident),+), ($($prefix:ident),+), ($($suffix:ident),+)) => { paste! { - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher,)+ $( []: EncodeLike<$prefix> ),+> - HasKeyPrefix<($( [] ),+)> for - ($(Key<[<$keygen $keygen>], $keygen>),+) - { + impl< + $($keygen: FullCodec,)+ + $( [<$keygen $keygen>]: StorageHasher,)+ + $( []: EncodeLike<$prefix> ),+ + > HasKeyPrefix<($( [] ),+)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { type Suffix = ($($suffix),+); fn partial_key(prefix: ($( [] ),+)) -> Vec { @@ -259,8 +260,11 @@ macro_rules! impl_key_prefix_for { } } - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ $( []: EncodeLike<$prefix> ),+> - HasReversibleKeyPrefix<($( [] ),+)> for + impl< + $($keygen: FullCodec,)+ + $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ + $( []: EncodeLike<$prefix> ),+ + > HasReversibleKeyPrefix<($( [] ),+)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { fn decode_partial_key(key_material: &[u8]) -> Result { @@ -271,10 +275,11 @@ macro_rules! impl_key_prefix_for { }; (($($keygen:ident),+), $prefix:ident, ($($suffix:ident),+)) => { paste! { - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher,)+ []: EncodeLike<$prefix>> - HasKeyPrefix<( [] ,)> for - ($(Key<[<$keygen $keygen>], $keygen>),+) - { + impl< + $($keygen: FullCodec,)+ + $( [<$keygen $keygen>]: StorageHasher,)+ + []: EncodeLike<$prefix> + > HasKeyPrefix<( [] ,)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { type Suffix = ($($suffix),+); fn partial_key(prefix: ( [] ,)) -> Vec { @@ -282,8 +287,11 @@ macro_rules! impl_key_prefix_for { } } - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ []: EncodeLike<$prefix>> - HasReversibleKeyPrefix<( [] ,)> for + impl< + $($keygen: FullCodec,)+ + $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ + []: EncodeLike<$prefix> + > HasReversibleKeyPrefix<( [] ,)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { fn decode_partial_key(key_material: &[u8]) -> Result { @@ -294,10 +302,11 @@ macro_rules! impl_key_prefix_for { }; (($($keygen:ident),+), ($($prefix:ident),+), $suffix:ident) => { paste! { - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: StorageHasher,)+ $( []: EncodeLike<$prefix>),+> - HasKeyPrefix<($( [] ),+)> for - ($(Key<[<$keygen $keygen>], $keygen>),+) - { + impl< + $($keygen: FullCodec,)+ + $( [<$keygen $keygen>]: StorageHasher,)+ + $( []: EncodeLike<$prefix>),+ + > HasKeyPrefix<($( [] ),+)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { type Suffix = $suffix; fn partial_key(prefix: ($( [] ),+)) -> Vec { @@ -305,8 +314,11 @@ macro_rules! impl_key_prefix_for { } } - impl<$($keygen: FullCodec,)+ $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ $( []: EncodeLike<$prefix> ),+> - HasReversibleKeyPrefix<($( [] ),+)> for + impl< + $($keygen: FullCodec,)+ + $( [<$keygen $keygen>]: ReversibleStorageHasher,)+ + $( []: EncodeLike<$prefix> ),+ + > HasReversibleKeyPrefix<($( [] ),+)> for ($(Key<[<$keygen $keygen>], $keygen>),+) { fn decode_partial_key(key_material: &[u8]) -> Result {