Nfts: Fix Auto-Increment (#12223)

* commit

* passing benchmarks

* clean up

* sync

* runtime implementation

* fix

* fmt

* fix benchmark

* cfg

* remove try-increment-id

* remove unused error

* impl Incrementable for unsigned types

* clean up

* fix in tests

* not needed anymore

* Use OptionQuery

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

frame/nfts/src/benchmarking.rs

@@ -42,13 +42,8 @@ fn create_collection<T: Config<I>, I: 'static>(
 	let caller_lookup = T::Lookup::unlookup(caller.clone());
 	let collection = T::Helper::collection(0);
 	T::Currency::make_free_balance_be(&caller, DepositBalanceOf::<T, I>::max_value());
-	assert!(Nfts::<T, I>::force_create(
-		SystemOrigin::Root.into(),
-		collection,
-		caller_lookup.clone(),
-		false,
-	)
-	.is_ok());
+	assert!(Nfts::<T, I>::force_create(SystemOrigin::Root.into(), caller_lookup.clone(), false,)
+		.is_ok());
 	(collection, caller, caller_lookup)
 }
 
@@ -142,7 +137,7 @@ benchmarks_instance_pallet! {
 		whitelist_account!(caller);
 		let admin = T::Lookup::unlookup(caller.clone());
 		T::Currency::make_free_balance_be(&caller, DepositBalanceOf::<T, I>::max_value());
-		let call = Call::<T, I>::create { collection, admin };
+		let call = Call::<T, I>::create { admin };
 	}: { call.dispatch_bypass_filter(origin)? }
 	verify {
 		assert_last_event::<T, I>(Event::Created { collection: T::Helper::collection(0), creator: caller.clone(), owner: caller }.into());
@@ -151,7 +146,7 @@ benchmarks_instance_pallet! {
 	force_create {
 		let caller: T::AccountId = whitelisted_caller();
 		let caller_lookup = T::Lookup::unlookup(caller.clone());
-	}: _(SystemOrigin::Root, T::Helper::collection(0), caller_lookup, true)
+	}: _(SystemOrigin::Root, caller_lookup, true)
 	verify {
 		assert_last_event::<T, I>(Event::ForceCreated { collection: T::Helper::collection(0), owner: caller }.into());
 	}

frame/nfts/src/functions.rs

@@ -94,7 +94,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			},
 		);
 
+		let next_id = collection.increment();
+
 		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(())
 	}
@@ -284,4 +289,14 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 
 		Ok(())
 	}
+
+	#[cfg(any(test, feature = "runtime-benchmarks"))]
+	pub fn set_next_id(id: T::CollectionId) {
+		NextCollectionId::<T, I>::set(Some(id));
+	}
+
+	#[cfg(test)]
+	pub fn get_next_id() -> T::CollectionId {
+		NextCollectionId::<T, I>::get().unwrap_or(T::CollectionId::initial_value())
+	}
 }

frame/nfts/src/lib.rs

@@ -61,6 +61,29 @@ pub use weights::WeightInfo;
 
 type AccountIdLookupOf<T> = <<T as SystemConfig>::Lookup as StaticLookup>::Source;
 
+pub trait Incrementable {
+	fn increment(&self) -> Self;
+	fn initial_value() -> Self;
+}
+
+macro_rules! impl_incrementable {
+	($($type:ty),+) => {
+		$(
+			impl Incrementable for $type {
+				fn increment(&self) -> Self {
+					self.saturating_add(1)
+				}
+
+				fn initial_value() -> Self {
+					0
+				}
+			}
+		)+
+	};
+}
+
+impl_incrementable!(u8, u16, u32, u64, u128, i8, i16, i32, i64, i128);
+
 #[frame_support::pallet]
 pub mod pallet {
 	use super::*;
@@ -94,7 +117,7 @@ pub mod pallet {
 			+ IsType<<Self as frame_system::Config>::RuntimeEvent>;
 
 		/// Identifier for the collection of item.
-		type CollectionId: Member + Parameter + MaxEncodedLen + Copy;
+		type CollectionId: Member + Parameter + MaxEncodedLen + Copy + Incrementable;
 
 		/// The type used to identify a unique item within a collection.
 		type ItemId: Member + Parameter + MaxEncodedLen + Copy;
@@ -278,6 +301,12 @@ pub mod pallet {
 	pub(super) type CollectionMaxSupply<T: Config<I>, I: 'static = ()> =
 		StorageMap<_, Blake2_128Concat, T::CollectionId, u32, OptionQuery>;
 
+	#[pallet::storage]
+	/// Stores the `CollectionId` that is going to be used for the next collection.
+	/// This gets incremented by 1 whenever a new collection is created.
+	pub(super) type NextCollectionId<T: Config<I>, I: 'static = ()> =
+		StorageValue<_, T::CollectionId, OptionQuery>;
+
 	#[pallet::event]
 	#[pallet::generate_deposit(pub(super) fn deposit_event)]
 	pub enum Event<T: Config<I>, I: 'static = ()> {
@@ -372,6 +401,8 @@ pub mod pallet {
 		OwnershipAcceptanceChanged { who: T::AccountId, maybe_collection: Option<T::CollectionId> },
 		/// Max supply has been set for a collection.
 		CollectionMaxSupplySet { collection: T::CollectionId, max_supply: u32 },
+		/// Event gets emmited when the `NextCollectionId` gets incremented.
+		NextCollectionIdIncremented { next_id: T::CollectionId },
 		/// The price was set for the instance.
 		ItemPriceSet {
 			collection: T::CollectionId,
@@ -458,19 +489,17 @@ pub mod pallet {
 		/// `ItemDeposit` funds of sender are reserved.
 		///
 		/// Parameters:
-		/// - `collection`: The identifier of the new collection. This must not be currently in use.
 		/// - `admin`: The admin of this collection. The admin is the initial address of each
 		/// member of the collection's admin team.
 		///
 		/// Emits `Created` event when successful.
 		///
 		/// Weight: `O(1)`
 		#[pallet::weight(T::WeightInfo::create())]
-		pub fn create(
-			origin: OriginFor<T>,
-			collection: T::CollectionId,
-			admin: AccountIdLookupOf<T>,
-		) -> DispatchResult {
+		pub fn create(origin: OriginFor<T>, admin: AccountIdLookupOf<T>) -> DispatchResult {
+			let collection =
+				NextCollectionId::<T, I>::get().unwrap_or(T::CollectionId::initial_value());
+
 			let owner = T::CreateOrigin::ensure_origin(origin, &collection)?;
 			let admin = T::Lookup::lookup(admin)?;
 
@@ -492,7 +521,6 @@ pub mod pallet {
 		///
 		/// Unlike `create`, no funds are reserved.
 		///
-		/// - `collection`: The identifier of the new item. This must not be currently in use.
 		/// - `owner`: The owner of this collection of items. The owner has full superuser
 		///   permissions
 		/// over this item, but may later change and configure the permissions using
@@ -504,13 +532,15 @@ pub mod pallet {
 		#[pallet::weight(T::WeightInfo::force_create())]
 		pub fn force_create(
 			origin: OriginFor<T>,
-			collection: T::CollectionId,
 			owner: AccountIdLookupOf<T>,
 			free_holding: bool,
 		) -> DispatchResult {
 			T::ForceOrigin::ensure_origin(origin)?;
 			let owner = T::Lookup::lookup(owner)?;
 
+			let collection =
+				NextCollectionId::<T, I>::get().unwrap_or(T::CollectionId::initial_value());
+
 			Self::do_create_collection(
 				collection,
 				owner.clone(),