changes to nfts pallet for xcm integration (#14395)

* Use Incrementable from frame_support::traits

* Chore

* make incremental fallible and new nfts function for custom collection ids

* fmt

* fix benchmark tests nfts

* add test

* fmt

* add safety comment to CollectionId

* fmt

* add comments to Incrementable

* line wrapping

* rewrap comments

* address feedback

* fmt

* change unwrap for expect

---------

Co-authored-by: Jegor Sidorenko <jegor@parity.io>
Co-authored-by: parity-processbot <>

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::<T>::get().unwrap_or(T::PoolAssetId::initial_value());
-			let next_lp_token_id = lp_token.increment();
+			let lp_token = NextPoolAssetId::<T>::get()
+				.or(T::PoolAssetId::initial_value())
+				.ok_or(Error::<T>::IncorrectPoolAssetId)?;
+			let next_lp_token_id = lp_token.increment().ok_or(Error::<T>::IncorrectPoolAssetId)?;
 			NextPoolAssetId::<T>::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::<T>::get().unwrap_or(T::PoolAssetId::initial_value())
+			NextPoolAssetId::<T>::get()
+				.or(T::PoolAssetId::initial_value())
+				.expect("Next pool asset ID can not be None")
 		}
 	}
 }

substrate/frame/nfts/src/benchmarking.rs

@@ -247,7 +247,7 @@ benchmarks_instance_pallet! {
 		let call = Call::<T, I>::create { admin, config: default_collection_config::<T, I>() };
 	}: { call.dispatch_bypass_filter(origin)? }
 	verify {
-		assert_last_event::<T, I>(Event::Created { collection: T::Helper::collection(0), creator: caller.clone(), owner: caller }.into());
+		assert_last_event::<T, I>(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::<T, I>())
 	verify {
-		assert_last_event::<T, I>(Event::ForceCreated { collection: T::Helper::collection(0), owner: caller }.into());
+		assert_last_event::<T, I>(Event::NextCollectionIdIncremented { next_id: Some(T::Helper::collection(1)) }.into());
 	}
 
 	destroy {

substrate/frame/nfts/src/common_functions.rs

@@ -55,6 +55,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		Ok(())
 	}
 
+	pub(crate) fn set_next_collection_id(collection: T::CollectionId) {
+		let next_id = collection.increment();
+		NextCollectionId::<T, I>::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::<T, I>::set(Some(id));
@@ -62,6 +68,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 	#[cfg(test)]
 	pub fn get_next_id() -> T::CollectionId {
-		NextCollectionId::<T, I>::get().unwrap_or(T::CollectionId::initial_value())
+		NextCollectionId::<T, I>::get()
+			.or(T::CollectionId::initial_value())
+			.expect("Failed to get next collection ID")
 	}
 }

substrate/frame/nfts/src/features/create_delete_collection.rs

@@ -50,13 +50,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			),
 		);
 
-		let next_id = collection.increment();
-
 		CollectionConfigOf::<T, I>::insert(&collection, config);
 		CollectionAccount::<T, I>::insert(&owner, &collection, ());
-		NextCollectionId::<T, I>::set(Some(next_id));
-
-		Self::deposit_event(Event::NextCollectionIdIncremented { next_id });
 		Self::deposit_event(event);
 		Ok(())
 	}

substrate/frame/nfts/src/impl_nonfungibles.rs

@@ -162,8 +162,9 @@ impl<T: Config<I>, I: 'static> Create<<T as SystemConfig>::AccountId, Collection
 			Error::<T, I>::WrongSetting
 		);
 
-		let collection =
-			NextCollectionId::<T, I>::get().unwrap_or(T::CollectionId::initial_value());
+		let collection = NextCollectionId::<T, I>::get()
+			.or(T::CollectionId::initial_value())
+			.ok_or(Error::<T, I>::UnknownCollection)?;
 
 		Self::do_create_collection(
 			collection,
@@ -173,8 +174,40 @@ impl<T: Config<I>, I: 'static> Create<<T as SystemConfig>::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<T, I>,
+	) -> Result<(), DispatchError> {
+		// DepositRequired can be disabled by calling the force_create() only
+		ensure!(
+			!config.has_disabled_setting(CollectionSetting::DepositRequired),
+			Error::<T, I>::WrongSetting
+		);
+
+		Self::do_create_collection(
+			collection,
+			who.clone(),
+			admin.clone(),
+			*config,
+			T::CollectionDeposit::get(),
+			Event::Created { collection, creator: who.clone(), owner: admin.clone() },
+		)
+	}
 }
 
 impl<T: Config<I>, I: 'static> Destroy<<T as SystemConfig>::AccountId> for Pallet<T, I> {

substrate/frame/nfts/src/lib.rs

@@ -101,6 +101,14 @@ pub mod pallet {
 			+ IsType<<Self as frame_system::Config>::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<T::CollectionId> },
 		/// The price was set for the item.
 		ItemPriceSet {
 			collection: T::CollectionId,
@@ -665,8 +673,9 @@ pub mod pallet {
 			admin: AccountIdLookupOf<T>,
 			config: CollectionConfigFor<T, I>,
 		) -> DispatchResult {
-			let collection =
-				NextCollectionId::<T, I>::get().unwrap_or(T::CollectionId::initial_value());
+			let collection = NextCollectionId::<T, I>::get()
+				.or(T::CollectionId::initial_value())
+				.ok_or(Error::<T, I>::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::<T, I>::get().unwrap_or(T::CollectionId::initial_value());
+			let collection = NextCollectionId::<T, I>::get()
+				.or(T::CollectionId::initial_value())
+				.ok_or(Error::<T, I>::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.

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::<Test>::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::<Test>::CollectionIdInUse
+		);
+	});
+}

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<Self> {
 					let mut val = self.clone();
 					val.saturating_inc();
-					val
+					Some(val)
 				}
 
-				fn initial_value() -> Self {
-					0
+				fn initial_value() -> Option<Self> {
+					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<Self>;
+
+	/// Returns the initial value.
+	///
+	/// Returns `Some` with the initial value if it is available, or `None` if it is not.
+	fn initial_value() -> Option<Self>;
 }
 
 impl_incrementable!(u8, u16, u32, u64, u128, i8, i16, i32, i64, i128);

substrate/frame/support/src/traits/tokens/nonfungibles_v2.rs

@@ -198,6 +198,13 @@ pub trait Create<AccountId, CollectionConfig>: Inspect<AccountId> {
 		admin: &AccountId,
 		config: &CollectionConfig,
 	) -> Result<Self::CollectionId, DispatchError>;
+
+	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.