diff --git a/substrate/frame/asset-conversion/src/lib.rs b/substrate/frame/asset-conversion/src/lib.rs index f442e7b7d0..f9aeeace11 100644 --- a/substrate/frame/asset-conversion/src/lib.rs +++ b/substrate/frame/asset-conversion/src/lib.rs @@ -356,6 +356,8 @@ pub mod pallet { PathError, /// The provided path must consists of unique assets. NonUniquePath, + /// It was not possible to get or increment the Id of the pool. + IncorrectPoolAssetId, /// Unable to find an element in an array/vec that should have one-to-one correspondence /// with another. For example, an array of assets constituting a `path` should have a /// corresponding array of `amounts` along the path. @@ -426,8 +428,10 @@ pub mod pallet { MultiAssetIdConversionResult::Native => (), } - let lp_token = NextPoolAssetId::::get().unwrap_or(T::PoolAssetId::initial_value()); - let next_lp_token_id = lp_token.increment(); + let lp_token = NextPoolAssetId::::get() + .or(T::PoolAssetId::initial_value()) + .ok_or(Error::::IncorrectPoolAssetId)?; + let next_lp_token_id = lp_token.increment().ok_or(Error::::IncorrectPoolAssetId)?; NextPoolAssetId::::set(Some(next_lp_token_id)); T::PoolAssets::create(lp_token.clone(), pool_account.clone(), false, 1u32.into())?; @@ -1223,7 +1227,9 @@ pub mod pallet { /// Returns the next pool asset id for benchmark purposes only. #[cfg(any(test, feature = "runtime-benchmarks"))] pub fn get_next_pool_asset_id() -> T::PoolAssetId { - NextPoolAssetId::::get().unwrap_or(T::PoolAssetId::initial_value()) + NextPoolAssetId::::get() + .or(T::PoolAssetId::initial_value()) + .expect("Next pool asset ID can not be None") } } } diff --git a/substrate/frame/nfts/src/benchmarking.rs b/substrate/frame/nfts/src/benchmarking.rs index 67ba292662..995c842036 100644 --- a/substrate/frame/nfts/src/benchmarking.rs +++ b/substrate/frame/nfts/src/benchmarking.rs @@ -247,7 +247,7 @@ benchmarks_instance_pallet! { let call = Call::::create { admin, config: default_collection_config::() }; }: { call.dispatch_bypass_filter(origin)? } verify { - assert_last_event::(Event::Created { collection: T::Helper::collection(0), creator: caller.clone(), owner: caller }.into()); + assert_last_event::(Event::NextCollectionIdIncremented { next_id: Some(T::Helper::collection(1)) }.into()); } force_create { @@ -255,7 +255,7 @@ benchmarks_instance_pallet! { let caller_lookup = T::Lookup::unlookup(caller.clone()); }: _(SystemOrigin::Root, caller_lookup, default_collection_config::()) verify { - assert_last_event::(Event::ForceCreated { collection: T::Helper::collection(0), owner: caller }.into()); + assert_last_event::(Event::NextCollectionIdIncremented { next_id: Some(T::Helper::collection(1)) }.into()); } destroy { diff --git a/substrate/frame/nfts/src/common_functions.rs b/substrate/frame/nfts/src/common_functions.rs index a3486edec2..b3dcef4cf3 100644 --- a/substrate/frame/nfts/src/common_functions.rs +++ b/substrate/frame/nfts/src/common_functions.rs @@ -55,6 +55,12 @@ impl, I: 'static> Pallet { Ok(()) } + pub(crate) fn set_next_collection_id(collection: T::CollectionId) { + let next_id = collection.increment(); + NextCollectionId::::set(next_id); + Self::deposit_event(Event::NextCollectionIdIncremented { next_id }); + } + #[cfg(any(test, feature = "runtime-benchmarks"))] pub fn set_next_id(id: T::CollectionId) { NextCollectionId::::set(Some(id)); @@ -62,6 +68,8 @@ impl, I: 'static> Pallet { #[cfg(test)] pub fn get_next_id() -> T::CollectionId { - NextCollectionId::::get().unwrap_or(T::CollectionId::initial_value()) + NextCollectionId::::get() + .or(T::CollectionId::initial_value()) + .expect("Failed to get next collection ID") } } diff --git a/substrate/frame/nfts/src/features/create_delete_collection.rs b/substrate/frame/nfts/src/features/create_delete_collection.rs index e943476062..aadad58689 100644 --- a/substrate/frame/nfts/src/features/create_delete_collection.rs +++ b/substrate/frame/nfts/src/features/create_delete_collection.rs @@ -50,13 +50,8 @@ impl, I: 'static> Pallet { ), ); - let next_id = collection.increment(); - CollectionConfigOf::::insert(&collection, config); CollectionAccount::::insert(&owner, &collection, ()); - NextCollectionId::::set(Some(next_id)); - - Self::deposit_event(Event::NextCollectionIdIncremented { next_id }); Self::deposit_event(event); Ok(()) } diff --git a/substrate/frame/nfts/src/impl_nonfungibles.rs b/substrate/frame/nfts/src/impl_nonfungibles.rs index 090a6a372c..4e2593b405 100644 --- a/substrate/frame/nfts/src/impl_nonfungibles.rs +++ b/substrate/frame/nfts/src/impl_nonfungibles.rs @@ -162,8 +162,9 @@ impl, I: 'static> Create<::AccountId, Collection Error::::WrongSetting ); - let collection = - NextCollectionId::::get().unwrap_or(T::CollectionId::initial_value()); + let collection = NextCollectionId::::get() + .or(T::CollectionId::initial_value()) + .ok_or(Error::::UnknownCollection)?; Self::do_create_collection( collection, @@ -173,8 +174,40 @@ impl, I: 'static> Create<::AccountId, Collection T::CollectionDeposit::get(), Event::Created { collection, creator: who.clone(), owner: admin.clone() }, )?; + + Self::set_next_collection_id(collection); + Ok(collection) } + + /// Create a collection of nonfungible items with `collection` Id to be owned by `who` and + /// managed by `admin`. Should be only used for applications that do not have an + /// incremental order for the collection IDs and is a replacement for the auto id creation. + /// + /// + /// SAFETY: This function can break the pallet if it is used in combination with the auto + /// increment functionality, as it can claim a value in the ID sequence. + fn create_collection_with_id( + collection: T::CollectionId, + who: &T::AccountId, + admin: &T::AccountId, + config: &CollectionConfigFor, + ) -> Result<(), DispatchError> { + // DepositRequired can be disabled by calling the force_create() only + ensure!( + !config.has_disabled_setting(CollectionSetting::DepositRequired), + Error::::WrongSetting + ); + + Self::do_create_collection( + collection, + who.clone(), + admin.clone(), + *config, + T::CollectionDeposit::get(), + Event::Created { collection, creator: who.clone(), owner: admin.clone() }, + ) + } } impl, I: 'static> Destroy<::AccountId> for Pallet { diff --git a/substrate/frame/nfts/src/lib.rs b/substrate/frame/nfts/src/lib.rs index 8124c71682..e1325d4cbb 100644 --- a/substrate/frame/nfts/src/lib.rs +++ b/substrate/frame/nfts/src/lib.rs @@ -101,6 +101,14 @@ pub mod pallet { + IsType<::RuntimeEvent>; /// Identifier for the collection of item. + /// + /// SAFETY: The functions in the `Incrementable` trait are fallible. If the functions + /// of the implementation both return `None`, the automatic CollectionId generation + /// should not be used. So the `create` and `force_create` extrinsics and the + /// `create_collection` function will return an `UnknownCollection` Error. Instead use + /// the `create_collection_with_id` function. However, if the `Incrementable` trait + /// implementation has an incremental order, the `create_collection_with_id` function + /// should not be used as it can claim a value in the ID sequence. type CollectionId: Member + Parameter + MaxEncodedLen + Copy + Incrementable; /// The type used to identify a unique item within a collection. @@ -476,7 +484,7 @@ pub mod pallet { /// Mint settings for a collection had changed. CollectionMintSettingsUpdated { collection: T::CollectionId }, /// Event gets emitted when the `NextCollectionId` gets incremented. - NextCollectionIdIncremented { next_id: T::CollectionId }, + NextCollectionIdIncremented { next_id: Option }, /// The price was set for the item. ItemPriceSet { collection: T::CollectionId, @@ -665,8 +673,9 @@ pub mod pallet { admin: AccountIdLookupOf, config: CollectionConfigFor, ) -> DispatchResult { - let collection = - NextCollectionId::::get().unwrap_or(T::CollectionId::initial_value()); + let collection = NextCollectionId::::get() + .or(T::CollectionId::initial_value()) + .ok_or(Error::::UnknownCollection)?; let owner = T::CreateOrigin::ensure_origin(origin, &collection)?; let admin = T::Lookup::lookup(admin)?; @@ -684,7 +693,10 @@ pub mod pallet { config, T::CollectionDeposit::get(), Event::Created { collection, creator: owner, owner: admin }, - ) + )?; + + Self::set_next_collection_id(collection); + Ok(()) } /// Issue a new collection of non-fungible items from a privileged origin. @@ -712,8 +724,9 @@ pub mod pallet { T::ForceOrigin::ensure_origin(origin)?; let owner = T::Lookup::lookup(owner)?; - let collection = - NextCollectionId::::get().unwrap_or(T::CollectionId::initial_value()); + let collection = NextCollectionId::::get() + .or(T::CollectionId::initial_value()) + .ok_or(Error::::UnknownCollection)?; Self::do_create_collection( collection, @@ -722,7 +735,10 @@ pub mod pallet { config, Zero::zero(), Event::ForceCreated { collection, owner }, - ) + )?; + + Self::set_next_collection_id(collection); + Ok(()) } /// Destroy a collection of fungible items. diff --git a/substrate/frame/nfts/src/tests.rs b/substrate/frame/nfts/src/tests.rs index 4df57cb132..c94e75d343 100644 --- a/substrate/frame/nfts/src/tests.rs +++ b/substrate/frame/nfts/src/tests.rs @@ -23,7 +23,7 @@ use frame_support::{ assert_noop, assert_ok, dispatch::Dispatchable, traits::{ - tokens::nonfungibles_v2::{Destroy, Mutate}, + tokens::nonfungibles_v2::{Create, Destroy, Mutate}, Currency, Get, }, }; @@ -3678,3 +3678,41 @@ fn pre_signed_attributes_should_work() { ); }) } + +#[test] +fn basic_create_collection_with_id_should_work() { + new_test_ext().execute_with(|| { + assert_noop!( + Nfts::create_collection_with_id( + 0u32, + &account(1), + &account(1), + &default_collection_config(), + ), + Error::::WrongSetting + ); + + Balances::make_free_balance_be(&account(1), 100); + Balances::make_free_balance_be(&account(2), 100); + + assert_ok!(Nfts::create_collection_with_id( + 0u32, + &account(1), + &account(1), + &collection_config_with_all_settings_enabled(), + )); + + assert_eq!(collections(), vec![(account(1), 0)]); + + // CollectionId already taken. + assert_noop!( + Nfts::create_collection_with_id( + 0u32, + &account(2), + &account(2), + &collection_config_with_all_settings_enabled(), + ), + Error::::CollectionIdInUse + ); + }); +} diff --git a/substrate/frame/support/src/traits/storage.rs b/substrate/frame/support/src/traits/storage.rs index 5947be57ae..829cd31e4c 100644 --- a/substrate/frame/support/src/traits/storage.rs +++ b/substrate/frame/support/src/traits/storage.rs @@ -132,24 +132,37 @@ macro_rules! impl_incrementable { ($($type:ty),+) => { $( impl Incrementable for $type { - fn increment(&self) -> Self { + fn increment(&self) -> Option { let mut val = self.clone(); val.saturating_inc(); - val + Some(val) } - fn initial_value() -> Self { - 0 + fn initial_value() -> Option { + Some(0) } } )+ }; } -/// For example: allows new identifiers to be created in a linear fashion. -pub trait Incrementable { - fn increment(&self) -> Self; - fn initial_value() -> Self; +/// A trait representing an incrementable type. +/// +/// The `increment` and `initial_value` functions are fallible. +/// They should either both return `Some` with a valid value, or `None`. +pub trait Incrementable +where + Self: Sized, +{ + /// Increments the value. + /// + /// Returns `Some` with the incremented value if it is possible, or `None` if it is not. + fn increment(&self) -> Option; + + /// Returns the initial value. + /// + /// Returns `Some` with the initial value if it is available, or `None` if it is not. + fn initial_value() -> Option; } impl_incrementable!(u8, u16, u32, u64, u128, i8, i16, i32, i64, i128); diff --git a/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs b/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs index 6f428c297e..345cce237b 100644 --- a/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs +++ b/substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs @@ -198,6 +198,13 @@ pub trait Create: Inspect { admin: &AccountId, config: &CollectionConfig, ) -> Result; + + fn create_collection_with_id( + collection: Self::CollectionId, + who: &AccountId, + admin: &AccountId, + config: &CollectionConfig, + ) -> Result<(), DispatchError>; } /// Trait for providing the ability to destroy collections of nonfungible items.