From 1abc0b6b85bd0fc80a37b84883f704f1322382c8 Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Mon, 7 Mar 2022 09:12:03 +0100 Subject: [PATCH] Minor Uniques pallet improvements and XCM v3 preparations (#10896) * Introduce Helper to Uniques for benchmark stuff * Fixes * Formatting * Featuregate the Helper, include ContainsPair * Introduce & use EnsureOriginWithArg * Benchmarking * Docs * More ContainsBoth helpers * Formatting * Formatting * Fixes Co-authored-by: Shawn Tabrizi --- bin/node/runtime/src/lib.rs | 9 +- frame/support/src/traits.rs | 10 +- frame/support/src/traits/dispatch.rs | 48 +++++ frame/support/src/traits/members.rs | 150 ++++++++++++--- .../support/src/traits/tokens/nonfungible.rs | 9 +- .../support/src/traits/tokens/nonfungibles.rs | 6 +- frame/uniques/src/benchmarking.rs | 73 +++++--- frame/uniques/src/impl_nonfungibles.rs | 15 +- frame/uniques/src/lib.rs | 172 ++++++++++++------ frame/uniques/src/mock.rs | 5 +- frame/uniques/src/tests.rs | 18 ++ frame/uniques/src/weights.rs | 15 ++ 12 files changed, 417 insertions(+), 113 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index f12bf8a88365f..e2903c0b314da 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -27,9 +27,9 @@ use frame_election_provider_support::onchain; use frame_support::{ construct_runtime, parameter_types, traits::{ - ConstU128, ConstU16, ConstU32, Currency, EnsureOneOf, EqualPrivilegeOnly, Everything, - Imbalance, InstanceFilter, KeyOwnerProofSystem, LockIdentifier, Nothing, OnUnbalanced, - U128CurrencyToVote, + AsEnsureOriginWithArg, ConstU128, ConstU16, ConstU32, Currency, EnsureOneOf, + EqualPrivilegeOnly, Everything, Imbalance, InstanceFilter, KeyOwnerProofSystem, + LockIdentifier, Nothing, OnUnbalanced, U128CurrencyToVote, }, weights::{ constants::{BlockExecutionWeight, ExtrinsicBaseWeight, RocksDbWeight, WEIGHT_PER_SECOND}, @@ -1348,6 +1348,9 @@ impl pallet_uniques::Config for Runtime { type KeyLimit = KeyLimit; type ValueLimit = ValueLimit; type WeightInfo = pallet_uniques::weights::SubstrateWeight; + #[cfg(feature = "runtime-benchmarks")] + type Helper = (); + type CreateOrigin = AsEnsureOriginWithArg>; } impl pallet_transaction_storage::Config for Runtime { diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 88891c83276bd..a8ce78ae9dabc 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -34,8 +34,9 @@ mod members; #[allow(deprecated)] pub use members::{AllowAll, DenyAll, Filter}; pub use members::{ - AsContains, ChangeMembers, Contains, ContainsLengthBound, Everything, InitializeMembers, - IsInVec, Nothing, SortedMembers, + AsContains, ChangeMembers, Contains, ContainsLengthBound, ContainsPair, Everything, + EverythingBut, FromContainsPair, InitializeMembers, InsideBoth, IsInVec, Nothing, + SortedMembers, TheseExcept, }; mod validation; @@ -89,7 +90,10 @@ pub use storage::{ }; mod dispatch; -pub use dispatch::{EnsureOneOf, EnsureOrigin, OriginTrait, UnfilteredDispatchable}; +pub use dispatch::{ + AsEnsureOriginWithArg, EnsureOneOf, EnsureOrigin, EnsureOriginWithArg, OriginTrait, + UnfilteredDispatchable, +}; mod voting; pub use voting::{ diff --git a/frame/support/src/traits/dispatch.rs b/frame/support/src/traits/dispatch.rs index 1a4e9f6f7cc2a..250a31ebfb17a 100644 --- a/frame/support/src/traits/dispatch.rs +++ b/frame/support/src/traits/dispatch.rs @@ -27,10 +27,12 @@ use sp_runtime::{ pub trait EnsureOrigin { /// A return type. type Success; + /// Perform the origin check. fn ensure_origin(o: OuterOrigin) -> Result { Self::try_origin(o).map_err(|_| BadOrigin) } + /// Perform the origin check. fn try_origin(o: OuterOrigin) -> Result; @@ -41,6 +43,52 @@ pub trait EnsureOrigin { fn successful_origin() -> OuterOrigin; } +/// Some sort of check on the origin is performed by this object. +pub trait EnsureOriginWithArg { + /// A return type. + type Success; + + /// Perform the origin check. + fn ensure_origin(o: OuterOrigin, a: &Argument) -> Result { + Self::try_origin(o, a).map_err(|_| BadOrigin) + } + + /// Perform the origin check, returning the origin value if unsuccessful. This allows chaining. + fn try_origin(o: OuterOrigin, a: &Argument) -> Result; + + /// Returns an outer origin capable of passing `try_origin` check. + /// + /// ** Should be used for benchmarking only!!! ** + #[cfg(feature = "runtime-benchmarks")] + fn successful_origin(a: &Argument) -> OuterOrigin; +} + +pub struct AsEnsureOriginWithArg(sp_std::marker::PhantomData); +impl> + EnsureOriginWithArg for AsEnsureOriginWithArg +{ + /// A return type. + type Success = EO::Success; + + /// Perform the origin check. + fn ensure_origin(o: OuterOrigin, _: &Argument) -> Result { + EO::ensure_origin(o) + } + + /// Perform the origin check, returning the origin value if unsuccessful. This allows chaining. + fn try_origin(o: OuterOrigin, _: &Argument) -> Result { + EO::try_origin(o) + } + + /// Returns an outer origin capable of passing `try_origin` check. + /// + /// ** Should be used for benchmarking only!!! ** + #[cfg(feature = "runtime-benchmarks")] + fn successful_origin(_: &Argument) -> OuterOrigin { + EO::successful_origin() + } +} + /// Type that can be dispatched with an origin but without checking the origin filter. /// /// Implemented for pallet dispatchable type by `decl_module` and for runtime dispatchable by diff --git a/frame/support/src/traits/members.rs b/frame/support/src/traits/members.rs index ba72b4819933b..f3c586b64af04 100644 --- a/frame/support/src/traits/members.rs +++ b/frame/support/src/traits/members.rs @@ -25,6 +25,40 @@ pub trait Contains { fn contains(t: &T) -> bool; } +#[impl_trait_for_tuples::impl_for_tuples(1, 30)] +impl Contains for Tuple { + fn contains(t: &T) -> bool { + for_tuples!( #( + if Tuple::contains(t) { return true } + )* ); + false + } +} + +/// A trait for querying whether a type can be said to "contain" a pair-value. +pub trait ContainsPair { + /// Return `true` if this "contains" the pair-value `(a, b)`. + fn contains(a: &A, b: &B) -> bool; +} + +#[impl_trait_for_tuples::impl_for_tuples(0, 30)] +impl ContainsPair for Tuple { + fn contains(a: &A, b: &B) -> bool { + for_tuples!( #( + if Tuple::contains(a, b) { return true } + )* ); + false + } +} + +/// Converter `struct` to use a `ContainsPair` implementation for a `Contains` bound. +pub struct FromContainsPair(PhantomData); +impl> Contains<(A, B)> for FromContainsPair { + fn contains((ref a, ref b): &(A, B)) -> bool { + CP::contains(a, b) + } +} + /// A [`Contains`] implementation that contains every value. pub enum Everything {} impl Contains for Everything { @@ -32,6 +66,11 @@ impl Contains for Everything { true } } +impl ContainsPair for Everything { + fn contains(_: &A, _: &B) -> bool { + true + } +} /// A [`Contains`] implementation that contains no value. pub enum Nothing {} @@ -40,43 +79,112 @@ impl Contains for Nothing { false } } +impl ContainsPair for Nothing { + fn contains(_: &A, _: &B) -> bool { + false + } +} -#[deprecated = "Use `Everything` instead"] -pub type AllowAll = Everything; -#[deprecated = "Use `Nothing` instead"] -pub type DenyAll = Nothing; -#[deprecated = "Use `Contains` instead"] -pub trait Filter { - fn filter(t: &T) -> bool; +/// A [`Contains`] implementation that contains everything except the values in `Exclude`. +pub struct EverythingBut(PhantomData); +impl> Contains for EverythingBut { + fn contains(t: &T) -> bool { + !Exclude::contains(t) + } } -#[allow(deprecated)] -impl> Filter for C { - fn filter(t: &T) -> bool { - Self::contains(t) +impl> ContainsPair for EverythingBut { + fn contains(a: &A, b: &B) -> bool { + !Exclude::contains(a, b) } } -#[impl_trait_for_tuples::impl_for_tuples(1, 30)] -impl Contains for Tuple { +/// A [`Contains`] implementation that contains all members of `These` excepting any members in +/// `Except`. +pub struct TheseExcept(PhantomData<(These, Except)>); +impl, Except: Contains> Contains for TheseExcept { fn contains(t: &T) -> bool { - for_tuples!( #( - if Tuple::contains(t) { return true } - )* ); - false + These::contains(t) && !Except::contains(t) + } +} +impl, Except: ContainsPair> ContainsPair + for TheseExcept +{ + fn contains(a: &A, b: &B) -> bool { + These::contains(a, b) && !Except::contains(a, b) + } +} + +/// A [`Contains`] implementation which contains all members of `These` which are also members of +/// `Those`. +pub struct InsideBoth(PhantomData<(These, Those)>); +impl, Those: Contains> Contains for InsideBoth { + fn contains(t: &T) -> bool { + These::contains(t) && Those::contains(t) + } +} +impl, Those: ContainsPair> ContainsPair + for InsideBoth +{ + fn contains(a: &A, b: &B) -> bool { + These::contains(a, b) && Those::contains(a, b) } } /// Create a type which implements the `Contains` trait for a particular type with syntax similar /// to `matches!`. #[macro_export] -macro_rules! match_type { - ( pub type $n:ident: impl Contains<$t:ty> = { $phead:pat_param $( | $ptail:pat )* } ; ) => { +macro_rules! match_types { + ( + pub type $n:ident: impl Contains<$t:ty> = { + $phead:pat_param $( | $ptail:pat )* + }; + $( $rest:tt )* + ) => { pub struct $n; impl $crate::traits::Contains<$t> for $n { fn contains(l: &$t) -> bool { matches!(l, $phead $( | $ptail )* ) } } + $crate::match_types!( $( $rest )* ); + }; + ( + pub type $n:ident: impl ContainsPair<$a:ty, $b:ty> = { + $phead:pat_param $( | $ptail:pat )* + }; + $( $rest:tt )* + ) => { + pub struct $n; + impl $crate::traits::ContainsPair<$a, $b> for $n { + fn contains(a: &$a, b: &$b) -> bool { + matches!((a, b), $phead $( | $ptail )* ) + } + } + $crate::match_types!( $( $rest )* ); + }; + () => {} +} + +/// Create a type which implements the `Contains` trait for a particular type with syntax similar +/// to `matches!`. +#[macro_export] +#[deprecated = "Use `match_types!` instead"] +macro_rules! match_type { + ($( $x:tt )*) => { $crate::match_types!( $( $x )* ); } +} + +#[deprecated = "Use `Everything` instead"] +pub type AllowAll = Everything; +#[deprecated = "Use `Nothing` instead"] +pub type DenyAll = Nothing; +#[deprecated = "Use `Contains` instead"] +pub trait Filter { + fn filter(t: &T) -> bool; +} +#[allow(deprecated)] +impl> Filter for C { + fn filter(t: &T) -> bool { + Self::contains(t) } } @@ -84,12 +192,12 @@ macro_rules! match_type { mod tests { use super::*; - match_type! { + match_types! { pub type OneOrTenToTwenty: impl Contains = { 1 | 10..=20 }; } #[test] - fn match_type_works() { + fn match_types_works() { for i in 0..=255 { assert_eq!(OneOrTenToTwenty::contains(&i), i == 1 || i >= 10 && i <= 20); } diff --git a/frame/support/src/traits/tokens/nonfungible.rs b/frame/support/src/traits/tokens/nonfungible.rs index 08e9a3a18a4b8..5cf9638131b28 100644 --- a/frame/support/src/traits/tokens/nonfungible.rs +++ b/frame/support/src/traits/tokens/nonfungible.rs @@ -85,7 +85,10 @@ pub trait Mutate: Inspect { /// Burn some asset `instance`. /// /// By default, this is not a supported operation. - fn burn_from(_instance: &Self::InstanceId) -> DispatchResult { + fn burn( + _instance: &Self::InstanceId, + _maybe_check_owner: Option<&AccountId>, + ) -> DispatchResult { Err(TokenError::Unsupported.into()) } @@ -166,8 +169,8 @@ impl< fn mint_into(instance: &Self::InstanceId, who: &AccountId) -> DispatchResult { >::mint_into(&A::get(), instance, who) } - fn burn_from(instance: &Self::InstanceId) -> DispatchResult { - >::burn_from(&A::get(), instance) + fn burn(instance: &Self::InstanceId, maybe_check_owner: Option<&AccountId>) -> DispatchResult { + >::burn(&A::get(), instance, maybe_check_owner) } fn set_attribute(instance: &Self::InstanceId, key: &[u8], value: &[u8]) -> DispatchResult { >::set_attribute(&A::get(), instance, key, value) diff --git a/frame/support/src/traits/tokens/nonfungibles.rs b/frame/support/src/traits/tokens/nonfungibles.rs index 1172fb6022830..8bd731b20342c 100644 --- a/frame/support/src/traits/tokens/nonfungibles.rs +++ b/frame/support/src/traits/tokens/nonfungibles.rs @@ -165,7 +165,11 @@ pub trait Mutate: Inspect { /// Burn some asset `instance` of `class`. /// /// By default, this is not a supported operation. - fn burn_from(_class: &Self::ClassId, _instance: &Self::InstanceId) -> DispatchResult { + fn burn( + _class: &Self::ClassId, + _instance: &Self::InstanceId, + _maybe_check_owner: Option<&AccountId>, + ) -> DispatchResult { Err(TokenError::Unsupported.into()) } diff --git a/frame/uniques/src/benchmarking.rs b/frame/uniques/src/benchmarking.rs index f3f9b7b28df72..d6223ec88f81b 100644 --- a/frame/uniques/src/benchmarking.rs +++ b/frame/uniques/src/benchmarking.rs @@ -40,12 +40,13 @@ fn create_class, I: 'static>( ) -> (T::ClassId, T::AccountId, ::Source) { let caller: T::AccountId = whitelisted_caller(); let caller_lookup = T::Lookup::unlookup(caller.clone()); - let class = Default::default(); + let class = T::Helper::class(0); T::Currency::make_free_balance_be(&caller, DepositBalanceOf::::max_value()); - assert!(Uniques::::create( - SystemOrigin::Signed(caller.clone()).into(), + assert!(Uniques::::force_create( + SystemOrigin::Root.into(), class, caller_lookup.clone(), + false, ) .is_ok()); (class, caller, caller_lookup) @@ -53,14 +54,14 @@ fn create_class, I: 'static>( fn add_class_metadata, I: 'static>( ) -> (T::AccountId, ::Source) { - let caller = Class::::get(T::ClassId::default()).unwrap().owner; + let caller = Class::::get(T::Helper::class(0)).unwrap().owner; if caller != whitelisted_caller() { whitelist_account!(caller); } let caller_lookup = T::Lookup::unlookup(caller.clone()); assert!(Uniques::::set_class_metadata( SystemOrigin::Signed(caller.clone()).into(), - Default::default(), + T::Helper::class(0), vec![0; T::StringLimit::get() as usize].try_into().unwrap(), false, ) @@ -71,15 +72,15 @@ fn add_class_metadata, I: 'static>( fn mint_instance, I: 'static>( index: u16, ) -> (T::InstanceId, T::AccountId, ::Source) { - let caller = Class::::get(T::ClassId::default()).unwrap().admin; + let caller = Class::::get(T::Helper::class(0)).unwrap().admin; if caller != whitelisted_caller() { whitelist_account!(caller); } let caller_lookup = T::Lookup::unlookup(caller.clone()); - let instance = index.into(); + let instance = T::Helper::instance(index); assert!(Uniques::::mint( SystemOrigin::Signed(caller.clone()).into(), - Default::default(), + T::Helper::class(0), instance, caller_lookup.clone(), ) @@ -90,14 +91,14 @@ fn mint_instance, I: 'static>( fn add_instance_metadata, I: 'static>( instance: T::InstanceId, ) -> (T::AccountId, ::Source) { - let caller = Class::::get(T::ClassId::default()).unwrap().owner; + let caller = Class::::get(T::Helper::class(0)).unwrap().owner; if caller != whitelisted_caller() { whitelist_account!(caller); } let caller_lookup = T::Lookup::unlookup(caller.clone()); assert!(Uniques::::set_metadata( SystemOrigin::Signed(caller.clone()).into(), - Default::default(), + T::Helper::class(0), instance, vec![0; T::StringLimit::get() as usize].try_into().unwrap(), false, @@ -109,7 +110,7 @@ fn add_instance_metadata, I: 'static>( fn add_instance_attribute, I: 'static>( instance: T::InstanceId, ) -> (BoundedVec, T::AccountId, ::Source) { - let caller = Class::::get(T::ClassId::default()).unwrap().owner; + let caller = Class::::get(T::Helper::class(0)).unwrap().owner; if caller != whitelisted_caller() { whitelist_account!(caller); } @@ -117,7 +118,7 @@ fn add_instance_attribute, I: 'static>( let key: BoundedVec<_, _> = vec![0; T::KeyLimit::get() as usize].try_into().unwrap(); assert!(Uniques::::set_attribute( SystemOrigin::Signed(caller.clone()).into(), - Default::default(), + T::Helper::class(0), Some(instance), key.clone(), vec![0; T::ValueLimit::get() as usize].try_into().unwrap(), @@ -136,20 +137,24 @@ fn assert_last_event, I: 'static>(generic_event: >:: benchmarks_instance_pallet! { create { - let caller: T::AccountId = whitelisted_caller(); - let caller_lookup = T::Lookup::unlookup(caller.clone()); + let class = T::Helper::class(0); + let origin = T::CreateOrigin::successful_origin(&class); + let caller = T::CreateOrigin::ensure_origin(origin.clone(), &class).unwrap(); + whitelist_account!(caller); + let admin = T::Lookup::unlookup(caller.clone()); T::Currency::make_free_balance_be(&caller, DepositBalanceOf::::max_value()); - }: _(SystemOrigin::Signed(caller.clone()), Default::default(), caller_lookup) + let call = Call::::create { class, admin }; + }: { call.dispatch_bypass_filter(origin)? } verify { - assert_last_event::(Event::Created { class: Default::default(), creator: caller.clone(), owner: caller }.into()); + assert_last_event::(Event::Created { class: T::Helper::class(0), creator: caller.clone(), owner: caller }.into()); } force_create { let caller: T::AccountId = whitelisted_caller(); let caller_lookup = T::Lookup::unlookup(caller.clone()); - }: _(SystemOrigin::Root, Default::default(), caller_lookup, true) + }: _(SystemOrigin::Root, T::Helper::class(0), caller_lookup, true) verify { - assert_last_event::(Event::ForceCreated { class: Default::default(), owner: caller }.into()); + assert_last_event::(Event::ForceCreated { class: T::Helper::class(0), owner: caller }.into()); } destroy { @@ -163,10 +168,10 @@ benchmarks_instance_pallet! { mint_instance::(i as u16); } for i in 0..m { - add_instance_metadata::((i as u16).into()); + add_instance_metadata::(T::Helper::instance(i as u16)); } for i in 0..a { - add_instance_attribute::((i as u16).into()); + add_instance_attribute::(T::Helper::instance(i as u16)); } let witness = Class::::get(class).unwrap().destroy_witness(); }: _(SystemOrigin::Signed(caller), class, witness) @@ -176,7 +181,7 @@ benchmarks_instance_pallet! { mint { let (class, caller, caller_lookup) = create_class::(); - let instance = Default::default(); + let instance = T::Helper::instance(0); }: _(SystemOrigin::Signed(caller.clone()), class, instance, caller_lookup) verify { assert_last_event::(Event::Issued { class, instance, owner: caller }.into()); @@ -192,7 +197,7 @@ benchmarks_instance_pallet! { transfer { let (class, caller, caller_lookup) = create_class::(); - let (instance, ..) = mint_instance::(Default::default()); + let (instance, ..) = mint_instance::(0); let target: T::AccountId = account("target", 0, SEED); let target_lookup = T::Lookup::unlookup(target.clone()); @@ -222,15 +227,15 @@ benchmarks_instance_pallet! { freeze { let (class, caller, caller_lookup) = create_class::(); - let (instance, ..) = mint_instance::(Default::default()); - }: _(SystemOrigin::Signed(caller.clone()), Default::default(), Default::default()) + let (instance, ..) = mint_instance::(0); + }: _(SystemOrigin::Signed(caller.clone()), T::Helper::class(0), T::Helper::instance(0)) verify { - assert_last_event::(Event::Frozen { class: Default::default(), instance: Default::default() }.into()); + assert_last_event::(Event::Frozen { class: T::Helper::class(0), instance: T::Helper::instance(0) }.into()); } thaw { let (class, caller, caller_lookup) = create_class::(); - let (instance, ..) = mint_instance::(Default::default()); + let (instance, ..) = mint_instance::(0); Uniques::::freeze( SystemOrigin::Signed(caller.clone()).into(), class, @@ -262,6 +267,8 @@ benchmarks_instance_pallet! { let target: T::AccountId = account("target", 0, SEED); let target_lookup = T::Lookup::unlookup(target.clone()); T::Currency::make_free_balance_be(&target, T::Currency::minimum_balance()); + let origin = SystemOrigin::Signed(target.clone()).into(); + Uniques::::set_accept_ownership(origin, Some(class.clone()))?; }: _(SystemOrigin::Signed(caller), class, target_lookup) verify { assert_last_event::(Event::OwnerChanged { class, new_owner: target }.into()); @@ -272,7 +279,7 @@ benchmarks_instance_pallet! { let target0 = T::Lookup::unlookup(account("target", 0, SEED)); let target1 = T::Lookup::unlookup(account("target", 1, SEED)); let target2 = T::Lookup::unlookup(account("target", 2, SEED)); - }: _(SystemOrigin::Signed(caller), Default::default(), target0.clone(), target1.clone(), target2.clone()) + }: _(SystemOrigin::Signed(caller), class, target0.clone(), target1.clone(), target2.clone()) verify { assert_last_event::(Event::TeamChanged{ class, @@ -379,5 +386,17 @@ benchmarks_instance_pallet! { assert_last_event::(Event::ApprovalCancelled { class, instance, owner: caller, delegate }.into()); } + set_accept_ownership { + let caller: T::AccountId = whitelisted_caller(); + T::Currency::make_free_balance_be(&caller, DepositBalanceOf::::max_value()); + let class = T::Helper::class(0); + }: _(SystemOrigin::Signed(caller.clone()), Some(class.clone())) + verify { + assert_last_event::(Event::OwnershipAcceptanceChanged { + who: caller, + maybe_class: Some(class), + }.into()); + } + impl_benchmark_test_suite!(Uniques, crate::mock::new_test_ext(), crate::mock::Test); } diff --git a/frame/uniques/src/impl_nonfungibles.rs b/frame/uniques/src/impl_nonfungibles.rs index a9695e8e898ae..89b95fb770489 100644 --- a/frame/uniques/src/impl_nonfungibles.rs +++ b/frame/uniques/src/impl_nonfungibles.rs @@ -128,8 +128,19 @@ impl, I: 'static> Mutate<::AccountId> for Pallet Self::do_mint(class.clone(), instance.clone(), who.clone(), |_| Ok(())) } - fn burn_from(class: &Self::ClassId, instance: &Self::InstanceId) -> DispatchResult { - Self::do_burn(class.clone(), instance.clone(), |_, _| Ok(())) + fn burn( + class: &Self::ClassId, + instance: &Self::InstanceId, + maybe_check_owner: Option<&T::AccountId>, + ) -> DispatchResult { + Self::do_burn(class.clone(), instance.clone(), |_, d| { + if let Some(check_owner) = maybe_check_owner { + if &d.owner != check_owner { + Err(Error::::NoPermission)?; + } + } + Ok(()) + }) } } diff --git a/frame/uniques/src/lib.rs b/frame/uniques/src/lib.rs index f35bb3fd61a0f..1e14825454193 100644 --- a/frame/uniques/src/lib.rs +++ b/frame/uniques/src/lib.rs @@ -41,8 +41,10 @@ mod types; pub mod migration; pub mod weights; -use codec::{Decode, Encode, HasCompact}; -use frame_support::traits::{BalanceStatus::Reserved, Currency, ReservableCurrency}; +use codec::{Decode, Encode}; +use frame_support::traits::{ + BalanceStatus::Reserved, Currency, EnsureOriginWithArg, ReservableCurrency, +}; use frame_system::Config as SystemConfig; use sp_runtime::{ traits::{Saturating, StaticLookup, Zero}, @@ -64,6 +66,21 @@ pub mod pallet { #[pallet::generate_store(pub(super) trait Store)] pub struct Pallet(_); + #[cfg(feature = "runtime-benchmarks")] + pub trait BenchmarkHelper { + fn class(i: u16) -> ClassId; + fn instance(i: u16) -> InstanceId; + } + #[cfg(feature = "runtime-benchmarks")] + impl, InstanceId: From> BenchmarkHelper for () { + fn class(i: u16) -> ClassId { + i.into() + } + fn instance(i: u16) -> InstanceId { + i.into() + } + } + #[pallet::config] /// The module configuration trait. pub trait Config: frame_system::Config { @@ -71,16 +88,10 @@ pub mod pallet { type Event: From> + IsType<::Event>; /// Identifier for the class of asset. - type ClassId: Member + Parameter + Default + Copy + HasCompact + MaxEncodedLen; + type ClassId: Member + Parameter + MaxEncodedLen + Copy; /// The type used to identify a unique asset within an asset class. - type InstanceId: Member - + Parameter - + Default - + Copy - + HasCompact - + From - + MaxEncodedLen; + type InstanceId: Member + Parameter + MaxEncodedLen + Copy; /// The currency mechanism, used for paying for reserves. type Currency: ReservableCurrency; @@ -89,6 +100,14 @@ pub mod pallet { /// attributes. type ForceOrigin: EnsureOrigin; + /// Standard class creation is only allowed if the origin attempting it and the class are + /// in this set. + type CreateOrigin: EnsureOriginWithArg< + Success = Self::AccountId, + Self::Origin, + Self::ClassId, + >; + /// The basic amount of funds that must be reserved for an asset class. #[pallet::constant] type ClassDeposit: Get>; @@ -122,6 +141,10 @@ pub mod pallet { #[pallet::constant] type ValueLimit: Get; + #[cfg(feature = "runtime-benchmarks")] + /// A set of helper functions for benchmarking. + type Helper: BenchmarkHelper; + /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; } @@ -135,6 +158,11 @@ pub mod pallet { ClassDetails>, >; + #[pallet::storage] + /// The class, if any, of which an account is willing to take ownership. + pub(super) type OwnershipAcceptance, I: 'static = ()> = + StorageMap<_, Blake2_128Concat, T::AccountId, T::ClassId>; + #[pallet::storage] /// The assets held by any given account; set out this way so that assets owned by a single /// account can be enumerated. @@ -296,6 +324,8 @@ pub mod pallet { maybe_instance: Option, key: BoundedVec, }, + /// Ownership acceptance has changed for an account. + OwnershipAcceptanceChanged { who: T::AccountId, maybe_class: Option }, } #[pallet::error] @@ -320,6 +350,8 @@ pub mod pallet { NoDelegate, /// No approval exists that would allow the transfer. Unapproved, + /// The named owner has not signed ownership of the class is acceptable. + Unaccepted, } impl, I: 'static> Pallet { @@ -327,6 +359,11 @@ pub mod pallet { pub fn owner(class: T::ClassId, instance: T::InstanceId) -> Option { Asset::::get(class, instance).map(|i| i.owner) } + + /// Get the owner of the asset instance, if the asset exists. + pub fn class_owner(class: T::ClassId) -> Option { + Class::::get(class).map(|i| i.owner) + } } #[pallet::call] @@ -350,10 +387,10 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::create())] pub fn create( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, admin: ::Source, ) -> DispatchResult { - let owner = ensure_signed(origin)?; + let owner = T::CreateOrigin::ensure_origin(origin, &class)?; let admin = T::Lookup::lookup(admin)?; Self::do_create_class( @@ -385,7 +422,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::force_create())] pub fn force_create( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, owner: ::Source, free_holding: bool, ) -> DispatchResult { @@ -424,7 +461,7 @@ pub mod pallet { ))] pub fn destroy( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, witness: DestroyWitness, ) -> DispatchResultWithPostInfo { let maybe_check_owner = match T::ForceOrigin::try_origin(origin) { @@ -455,8 +492,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::mint())] pub fn mint( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, owner: ::Source, ) -> DispatchResult { let origin = ensure_signed(origin)?; @@ -484,8 +521,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::burn())] pub fn burn( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, check_owner: Option<::Source>, ) -> DispatchResult { let origin = ensure_signed(origin)?; @@ -520,8 +557,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::transfer())] pub fn transfer( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, dest: ::Source, ) -> DispatchResult { let origin = ensure_signed(origin)?; @@ -556,7 +593,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::redeposit(instances.len() as u32))] pub fn redeposit( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, instances: Vec, ) -> DispatchResult { let origin = ensure_signed(origin)?; @@ -616,8 +653,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::freeze())] pub fn freeze( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, ) -> DispatchResult { let origin = ensure_signed(origin)?; @@ -646,8 +683,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::thaw())] pub fn thaw( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, ) -> DispatchResult { let origin = ensure_signed(origin)?; @@ -673,10 +710,7 @@ pub mod pallet { /// /// Weight: `O(1)` #[pallet::weight(T::WeightInfo::freeze_class())] - pub fn freeze_class( - origin: OriginFor, - #[pallet::compact] class: T::ClassId, - ) -> DispatchResult { + pub fn freeze_class(origin: OriginFor, class: T::ClassId) -> DispatchResult { let origin = ensure_signed(origin)?; Class::::try_mutate(class, |maybe_details| { @@ -700,10 +734,7 @@ pub mod pallet { /// /// Weight: `O(1)` #[pallet::weight(T::WeightInfo::thaw_class())] - pub fn thaw_class( - origin: OriginFor, - #[pallet::compact] class: T::ClassId, - ) -> DispatchResult { + pub fn thaw_class(origin: OriginFor, class: T::ClassId) -> DispatchResult { let origin = ensure_signed(origin)?; Class::::try_mutate(class, |maybe_details| { @@ -722,7 +753,8 @@ pub mod pallet { /// Origin must be Signed and the sender should be the Owner of the asset `class`. /// /// - `class`: The asset class whose owner should be changed. - /// - `owner`: The new Owner of this asset class. + /// - `owner`: The new Owner of this asset class. They must have called + /// `set_accept_ownership` with `class` in order for this operation to succeed. /// /// Emits `OwnerChanged`. /// @@ -730,12 +762,15 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::transfer_ownership())] pub fn transfer_ownership( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, owner: ::Source, ) -> DispatchResult { let origin = ensure_signed(origin)?; let owner = T::Lookup::lookup(owner)?; + let acceptable_class = OwnershipAcceptance::::get(&owner); + ensure!(acceptable_class.as_ref() == Some(&class), Error::::Unaccepted); + Class::::try_mutate(class, |maybe_details| { let details = maybe_details.as_mut().ok_or(Error::::UnknownClass)?; ensure!(&origin == &details.owner, Error::::NoPermission); @@ -753,6 +788,7 @@ pub mod pallet { ClassAccount::::remove(&details.owner, &class); ClassAccount::::insert(&owner, &class, ()); details.owner = owner.clone(); + OwnershipAcceptance::::remove(&owner); Self::deposit_event(Event::OwnerChanged { class, new_owner: owner }); Ok(()) @@ -774,7 +810,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::set_team())] pub fn set_team( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, issuer: ::Source, admin: ::Source, freezer: ::Source, @@ -811,8 +847,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::approve_transfer())] pub fn approve_transfer( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, delegate: ::Source, ) -> DispatchResult { let maybe_check: Option = T::ForceOrigin::try_origin(origin) @@ -863,8 +899,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::cancel_approval())] pub fn cancel_approval( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, maybe_check_delegate: Option<::Source>, ) -> DispatchResult { let maybe_check: Option = T::ForceOrigin::try_origin(origin) @@ -915,7 +951,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::force_asset_status())] pub fn force_asset_status( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, owner: ::Source, issuer: ::Source, admin: ::Source, @@ -964,7 +1000,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::set_attribute())] pub fn set_attribute( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, maybe_instance: Option, key: BoundedVec, value: BoundedVec, @@ -1027,7 +1063,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::clear_attribute())] pub fn clear_attribute( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, maybe_instance: Option, key: BoundedVec, ) -> DispatchResult { @@ -1077,8 +1113,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::set_metadata())] pub fn set_metadata( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, data: BoundedVec, is_frozen: bool, ) -> DispatchResult { @@ -1139,8 +1175,8 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::clear_metadata())] pub fn clear_metadata( origin: OriginFor, - #[pallet::compact] class: T::ClassId, - #[pallet::compact] instance: T::InstanceId, + class: T::ClassId, + instance: T::InstanceId, ) -> DispatchResult { let maybe_check_owner = T::ForceOrigin::try_origin(origin) .map(|_| None) @@ -1188,7 +1224,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::set_class_metadata())] pub fn set_class_metadata( origin: OriginFor, - #[pallet::compact] class: T::ClassId, + class: T::ClassId, data: BoundedVec, is_frozen: bool, ) -> DispatchResult { @@ -1242,10 +1278,7 @@ pub mod pallet { /// /// Weight: `O(1)` #[pallet::weight(T::WeightInfo::clear_class_metadata())] - pub fn clear_class_metadata( - origin: OriginFor, - #[pallet::compact] class: T::ClassId, - ) -> DispatchResult { + pub fn clear_class_metadata(origin: OriginFor, class: T::ClassId) -> DispatchResult { let maybe_check_owner = T::ForceOrigin::try_origin(origin) .map(|_| None) .or_else(|origin| ensure_signed(origin).map(Some))?; @@ -1265,5 +1298,40 @@ pub mod pallet { Ok(()) }) } + + /// Set (or reset) the acceptance of ownership for a particular account. + /// + /// Origin must be `Signed` and if `maybe_class` is `Some`, then the signer must have a + /// provider reference. + /// + /// - `maybe_class`: The identifier of the asset class whose ownership the signer is willing + /// to accept, or if `None`, an indication that the signer is willing to accept no + /// ownership transferal. + /// + /// Emits `OwnershipAcceptanceChanged`. + #[pallet::weight(T::WeightInfo::set_accept_ownership())] + pub fn set_accept_ownership( + origin: OriginFor, + maybe_class: Option, + ) -> DispatchResult { + let who = ensure_signed(origin)?; + let old = OwnershipAcceptance::::get(&who); + match (old.is_some(), maybe_class.is_some()) { + (false, true) => { + frame_system::Pallet::::inc_consumers(&who)?; + }, + (true, false) => { + frame_system::Pallet::::dec_consumers(&who); + }, + _ => {}, + } + if let Some(class) = maybe_class.as_ref() { + OwnershipAcceptance::::insert(&who, class); + } else { + OwnershipAcceptance::::remove(&who); + } + Self::deposit_event(Event::OwnershipAcceptanceChanged { who, maybe_class }); + Ok(()) + } } } diff --git a/frame/uniques/src/mock.rs b/frame/uniques/src/mock.rs index 2a94fcbee347a..265142443ef48 100644 --- a/frame/uniques/src/mock.rs +++ b/frame/uniques/src/mock.rs @@ -22,7 +22,7 @@ use crate as pallet_uniques; use frame_support::{ construct_runtime, - traits::{ConstU32, ConstU64}, + traits::{AsEnsureOriginWithArg, ConstU32, ConstU64}, }; use sp_core::H256; use sp_runtime::{ @@ -89,6 +89,7 @@ impl Config for Test { type ClassId = u32; type InstanceId = u32; type Currency = Balances; + type CreateOrigin = AsEnsureOriginWithArg>; type ForceOrigin = frame_system::EnsureRoot; type ClassDeposit = ConstU64<2>; type InstanceDeposit = ConstU64<1>; @@ -99,6 +100,8 @@ impl Config for Test { type KeyLimit = ConstU32<50>; type ValueLimit = ConstU32<50>; type WeightInfo = (); + #[cfg(feature = "runtime-benchmarks")] + type Helper = (); } pub(crate) fn new_test_ext() -> sp_io::TestExternalities { diff --git a/frame/uniques/src/tests.rs b/frame/uniques/src/tests.rs index 12f39c78bfe3d..364073ad37cde 100644 --- a/frame/uniques/src/tests.rs +++ b/frame/uniques/src/tests.rs @@ -196,6 +196,9 @@ fn origin_guards_should_work() { new_test_ext().execute_with(|| { assert_ok!(Uniques::force_create(Origin::root(), 0, 1, true)); assert_ok!(Uniques::mint(Origin::signed(1), 0, 42, 1)); + + Balances::make_free_balance_be(&2, 100); + assert_ok!(Uniques::set_accept_ownership(Origin::signed(2), Some(0))); assert_noop!( Uniques::transfer_ownership(Origin::signed(2), 0, 2), Error::::NoPermission @@ -218,13 +221,20 @@ fn transfer_owner_should_work() { Balances::make_free_balance_be(&3, 100); assert_ok!(Uniques::create(Origin::signed(1), 0, 1)); assert_eq!(classes(), vec![(1, 0)]); + assert_noop!( + Uniques::transfer_ownership(Origin::signed(1), 0, 2), + Error::::Unaccepted + ); + assert_ok!(Uniques::set_accept_ownership(Origin::signed(2), Some(0))); assert_ok!(Uniques::transfer_ownership(Origin::signed(1), 0, 2)); + assert_eq!(classes(), vec![(2, 0)]); assert_eq!(Balances::total_balance(&1), 98); assert_eq!(Balances::total_balance(&2), 102); assert_eq!(Balances::reserved_balance(&1), 0); assert_eq!(Balances::reserved_balance(&2), 2); + assert_ok!(Uniques::set_accept_ownership(Origin::signed(1), Some(0))); assert_noop!( Uniques::transfer_ownership(Origin::signed(1), 0, 1), Error::::NoPermission @@ -234,12 +244,20 @@ fn transfer_owner_should_work() { assert_ok!(Uniques::set_class_metadata(Origin::signed(2), 0, bvec![0u8; 20], false)); assert_ok!(Uniques::mint(Origin::signed(1), 0, 42, 1)); assert_ok!(Uniques::set_metadata(Origin::signed(2), 0, 42, bvec![0u8; 20], false)); + assert_ok!(Uniques::set_accept_ownership(Origin::signed(3), Some(0))); assert_ok!(Uniques::transfer_ownership(Origin::signed(2), 0, 3)); assert_eq!(classes(), vec![(3, 0)]); assert_eq!(Balances::total_balance(&2), 57); assert_eq!(Balances::total_balance(&3), 145); assert_eq!(Balances::reserved_balance(&2), 0); assert_eq!(Balances::reserved_balance(&3), 45); + + // 2's acceptence from before is reset when it became owner, so it cannot be transfered + // without a fresh acceptance. + assert_noop!( + Uniques::transfer_ownership(Origin::signed(3), 0, 2), + Error::::Unaccepted + ); }); } diff --git a/frame/uniques/src/weights.rs b/frame/uniques/src/weights.rs index 1df8fe0ff6650..eb9067b7133a0 100644 --- a/frame/uniques/src/weights.rs +++ b/frame/uniques/src/weights.rs @@ -68,6 +68,7 @@ pub trait WeightInfo { fn clear_class_metadata() -> Weight; fn approve_transfer() -> Weight; fn cancel_approval() -> Weight; + fn set_accept_ownership() -> Weight; } /// Weights for pallet_uniques using the Substrate node and recommended hardware. @@ -249,6 +250,13 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(2 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } + // Storage: Uniques Class (r:1 w:0) + // Storage: Uniques Asset (r:1 w:1) + fn set_accept_ownership() -> Weight { + (19_417_000 as Weight) + .saturating_add(T::DbWeight::get().reads(1 as Weight)) + .saturating_add(T::DbWeight::get().writes(1 as Weight)) + } } // For backwards compatibility and tests @@ -429,4 +437,11 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(2 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } + // Storage: Uniques Class (r:1 w:0) + // Storage: Uniques Asset (r:1 w:1) + fn set_accept_ownership() -> Weight { + (19_417_000 as Weight) + .saturating_add(RocksDbWeight::get().reads(1 as Weight)) + .saturating_add(RocksDbWeight::get().writes(1 as Weight)) + } }