Repository: paritytech/substrate
File: frame/nfts/src/lib.rs (Rust)

Code before:
		/// - `issuer`: The new Issuer of this collection.
		/// - `admin`: The new Admin of this collection.
		/// - `freezer`: The new Freezer of this collection.
		///
		/// Emits `TeamChanged`.
		///
		/// Weight: `O(1)`
		#[pallet::weight(T::WeightInfo::set_team())]
		pub fn set_team(
			origin: OriginFor<T>,
			collection: T::CollectionId,
			issuer: AccountIdLookupOf<T>,
			admin: AccountIdLookupOf<T>,
			freezer: AccountIdLookupOf<T>,
		) -> DispatchResult {
			let origin = ensure_signed(origin)?;
			let issuer = T::Lookup::lookup(issuer)?;
			let admin = T::Lookup::lookup(admin)?;
			let freezer = T::Lookup::lookup(freezer)?;

			Collection::<T, I>::try_mutate(collection, |maybe_details| {
				let details = maybe_details.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;
				ensure!(origin == details.owner, Error::<T, I>::NoPermission);

				// delete previous values
				let _ = CollectionRoleOf::<T, I>::clear_prefix(&collection, 3, None);

				let account_to_role = Self::group_roles_by_account(vec![
					(issuer.clone(), CollectionRole::Issuer),
					(admin.clone(), CollectionRole::Admin),
					(freezer.clone(), CollectionRole::Freezer),
				]);
				for (account, roles) in account_to_role {
					CollectionRoleOf::<T, I>::insert(&collection, &account, roles);
				}

				Self::deposit_event(Event::TeamChanged { collection, issuer, admin, freezer });
				Ok(())
			})
		}

		/// Approve an item to be transferred by a delegated third-party account.
		///
		/// Origin must be Signed and must be the owner of the `item`.
		///
		/// - `collection`: The collection of the item to be approved for delegated transfer.
		/// - `item`: The item of the item to be approved for delegated transfer.
		/// - `delegate`: The account to delegate permission to transfer the item.
		/// - `maybe_deadline`: Optional deadline for the approval. Specified by providing the
		/// 	number of blocks after which the approval will expire
		///

Review comment (bug):
It might become more than 3, and this piece of code can be forgotten to be updated.
As I remember from the returned value you can know if there is anything left with the same prefix.
You can assert it, and fail if there is inconsistency.
Also would be good to have some tests to cover it, this quite important. It should guarantee the removal of all previous privileges.

Code after:
		///
		/// - `collection`: The collection whose team should be changed.
		/// - `issuer`: The new Issuer of this collection.
		/// - `admin`: The new Admin of this collection.
		/// - `freezer`: The new Freezer of this collection.
		///
		/// Emits `TeamChanged`.
		///
		/// Weight: `O(1)`
		#[pallet::weight(T::WeightInfo::set_team())]
		pub fn set_team(
			origin: OriginFor<T>,
			collection: T::CollectionId,
			issuer: AccountIdLookupOf<T>,
			admin: AccountIdLookupOf<T>,
			freezer: AccountIdLookupOf<T>,
		) -> DispatchResult {
			let origin = ensure_signed(origin)?;
			let issuer = T::Lookup::lookup(issuer)?;
			let admin = T::Lookup::lookup(admin)?;
			let freezer = T::Lookup::lookup(freezer)?;

			Collection::<T, I>::try_mutate(collection, |maybe_details| {
				let details = maybe_details.as_mut().ok_or(Error::<T, I>::UnknownCollection)?;
				ensure!(origin == details.owner, Error::<T, I>::NoPermission);

				// delete previous values
				Self::clear_roles(&collection)?;

				let account_to_role = Self::group_roles_by_account(vec![
					(issuer.clone(), CollectionRole::Issuer),
					(admin.clone(), CollectionRole::Admin),
					(freezer.clone(), CollectionRole::Freezer),
				]);
				for (account, roles) in account_to_role {
					CollectionRoleOf::<T, I>::insert(&collection, &account, roles);
				}

				Self::deposit_event(Event::TeamChanged { collection, issuer, admin, freezer });
				Ok(())
			})
		}

		/// Approve an item to be transferred by a delegated third-party account.
		///
		/// Origin must be Signed and must be the owner of the `item`.
		///
		/// - `collection`: The collection of the item to be approved for delegated transfer.
		/// - `item`: The item of the item to be approved for delegated transfer.
		/// - `delegate`: The account to delegate permission to transfer the item.
		/// - `maybe_deadline`: Optional deadline for the approval. Specified by providing the