refactor: assign pool fee id during propose (#1706)

* refactor: assign pool fee id during propose

* fix: missing AppendFee refactor application

pallets/pool-fees/src/benchmarking.rs

@@ -62,6 +62,7 @@ mod benchmarks {
 		let change_id = T::ChangeGuard::note(
 			T::PoolId::default(),
 			Change::<T>::AppendFee(
+				(n + 1).into(),
 				PoolFeeBucket::Top,
 				PoolFees::<T>::get_default_fixed_fee_info(),
 			)

pallets/pool-fees/src/lib.rs

@@ -231,6 +231,7 @@ pub mod pallet {
 		/// A new pool fee was proposed.
 		Proposed {
 			pool_id: T::PoolId,
+			fee_id: T::FeeId,
 			bucket: PoolFeeBucket,
 			fee: PoolFeeInfoOf<T>,
 		},
@@ -275,6 +276,8 @@ pub mod pallet {
 
 	#[pallet::error]
 	pub enum Error<T> {
+		/// Attempted to add a fee id which already exists.
+		FeeIdAlreadyExists,
 		/// A fee could not be found.
 		FeeNotFound,
 		/// A pool could not be found.
@@ -317,10 +320,15 @@ pub mod pallet {
 				Error::<T>::NotPoolAdmin
 			);
 
-			T::ChangeGuard::note(pool_id, Change::AppendFee(bucket, fee.clone()).into())?;
+			let fee_id = Self::generate_fee_id()?;
+			T::ChangeGuard::note(
+				pool_id,
+				Change::AppendFee(fee_id, bucket, fee.clone()).into(),
+			)?;
 
 			Self::deposit_event(Event::<T>::Proposed {
 				pool_id,
+				fee_id,
 				bucket,
 				fee,
 			});
@@ -345,10 +353,10 @@ pub mod pallet {
 				T::PoolReserve::pool_exists(pool_id),
 				Error::<T>::PoolNotFound
 			);
-			let (bucket, fee) = Self::get_released_change(pool_id, change_id)
-				.map(|Change::AppendFee(bucket, fee)| (bucket, fee))?;
+			let (fee_id, bucket, fee) = Self::get_released_change(pool_id, change_id)
+				.map(|Change::AppendFee(id, bucket, fee)| (id, bucket, fee))?;
 
-			Self::add_fee(pool_id, bucket, fee)?;
+			Self::add_fee_with_id(pool_id, fee_id, bucket, fee)?;
 
 			Ok(())
 		}
@@ -515,7 +523,7 @@ pub mod pallet {
 		}
 
 		/// Return the the last fee id and bump it for the next query
-		fn generate_fee_id() -> Result<T::FeeId, ArithmeticError> {
+		pub(crate) fn generate_fee_id() -> Result<T::FeeId, ArithmeticError> {
 			LastFeeId::<T>::try_mutate(|last_fee_id| {
 				last_fee_id.ensure_add_assign(One::one())?;
 				Ok(*last_fee_id)
@@ -719,26 +727,24 @@ pub mod pallet {
 
 			Ok((valuation, values.len().saturated_into()))
 		}
-	}
-
-	impl<T: Config> PoolFees for Pallet<T> {
-		type FeeInfo = PoolFeeInfoOf<T>;
-		type PoolId = T::PoolId;
 
-		fn add_fee(
-			pool_id: Self::PoolId,
+		fn add_fee_with_id(
+			pool_id: T::PoolId,
+			fee_id: T::FeeId,
 			bucket: PoolFeeBucket,
-			fee: Self::FeeInfo,
-		) -> Result<(), DispatchError> {
-			let fee_id = Self::generate_fee_id()?;
-
+			fee: PoolFeeInfoOf<T>,
+		) -> DispatchResult {
+			ensure!(
+				!FeeIdsToPoolBucket::<T>::contains_key(fee_id),
+				Error::<T>::FeeIdAlreadyExists
+			);
+			FeeIdsToPoolBucket::<T>::insert(fee_id, (pool_id, bucket));
 			FeeIds::<T>::mutate(pool_id, bucket, |list| list.try_push(fee_id))
 				.map_err(|_| Error::<T>::MaxPoolFeesPerBucket)?;
 			ActiveFees::<T>::mutate(pool_id, bucket, |list| {
 				list.try_push(PoolFeeOf::<T>::from_info(fee.clone(), fee_id))
 			})
 			.map_err(|_| Error::<T>::MaxPoolFeesPerBucket)?;
-			FeeIdsToPoolBucket::<T>::insert(fee_id, (pool_id, bucket));
 
 			Self::deposit_event(Event::<T>::Added {
 				pool_id,
@@ -749,6 +755,20 @@ pub mod pallet {
 
 			Ok(())
 		}
+	}
+
+	impl<T: Config> PoolFees for Pallet<T> {
+		type FeeInfo = PoolFeeInfoOf<T>;
+		type PoolId = T::PoolId;
+
+		fn add_fee(
+			pool_id: Self::PoolId,
+			bucket: PoolFeeBucket,
+			fee: Self::FeeInfo,
+		) -> Result<(), DispatchError> {
+			let fee_id = Self::generate_fee_id()?;
+			Self::add_fee_with_id(pool_id, fee_id, bucket, fee)
+		}
 
 		fn get_max_fees_per_bucket() -> u32 {
 			T::MaxPoolFeesPerBucket::get()

pallets/pool-fees/src/mock.rs

@@ -383,6 +383,7 @@ pub(crate) fn init_mocks() {
 	#[cfg(feature = "runtime-benchmarks")]
 	MockChangeGuard::mock_released(|_, _| {
 		Ok(Change::AppendFee(
+			PoolFees::generate_fee_id().unwrap(),
 			PoolFeeBucket::Top,
 			<PoolFees as cfg_traits::benchmarking::PoolFeesBenchmarkHelper>::get_default_fixed_fee_info(),
 		))
@@ -405,7 +406,14 @@ pub(crate) fn config_change_mocks(fee: &PoolFeeInfoOf<Runtime>) {
 	MockChangeGuard::mock_note({
 		move |pool_id, change| {
 			assert_eq!(pool_id, POOL);
-			assert_eq!(change, Change::AppendFee(BUCKET, pool_fee.clone()));
+			assert_eq!(
+				change,
+				Change::AppendFee(
+					PoolFees::generate_fee_id().unwrap(),
+					BUCKET,
+					pool_fee.clone()
+				)
+			);
 			Ok(CHANGE_ID)
 		}
 	});
@@ -415,7 +423,11 @@ pub(crate) fn config_change_mocks(fee: &PoolFeeInfoOf<Runtime>) {
 		move |pool_id, change_id| {
 			assert_eq!(pool_id, POOL);
 			assert_eq!(change_id, CHANGE_ID);
-			Ok(Change::AppendFee(BUCKET, pool_fee.clone()))
+			Ok(Change::AppendFee(
+				PoolFees::generate_fee_id().unwrap(),
+				BUCKET,
+				pool_fee.clone(),
+			))
 		}
 	});
 }

pallets/pool-fees/src/tests.rs

@@ -38,6 +38,7 @@ mod extrinsics {
 
 					System::assert_last_event(
 						Event::<Runtime>::Proposed {
+							fee_id: (i + 1) as u64,
 							pool_id: POOL,
 							bucket: BUCKET,
 							fee,
@@ -399,7 +400,7 @@ mod extrinsics {
 					add_fees(vec![default_fixed_fee()]);
 				}
 				MockChangeGuard::mock_released(|_, _| {
-					Ok(Change::AppendFee(BUCKET, default_fixed_fee()))
+					Ok(Change::AppendFee(u64::MAX, BUCKET, default_fixed_fee()))
 				});
 
 				assert_noop!(
@@ -408,6 +409,22 @@ mod extrinsics {
 				);
 			});
 		}
+
+		#[test]
+		fn fee_id_already_exists() {
+			ExtBuilder::default().build().execute_with(|| {
+				add_fees(vec![default_fixed_fee()]);
+
+				MockChangeGuard::mock_released(|_, _| {
+					Ok(Change::AppendFee(1u64, BUCKET, default_fixed_fee()))
+				});
+
+				assert_noop!(
+					PoolFees::apply_new_fee(RuntimeOrigin::signed(ANY), POOL, CHANGE_ID),
+					Error::<Runtime>::FeeIdAlreadyExists
+				);
+			});
+		}
 	}
 }
 

pallets/pool-fees/src/types.rs

@@ -21,5 +21,5 @@ use crate::{Config, PoolFeeInfoOf};
 #[derive(Debug, Encode, Decode, TypeInfo, MaxEncodedLen, PartialEq, Eq, Clone)]
 #[scale_info(skip_type_params(T))]
 pub enum Change<T: Config> {
-	AppendFee(PoolFeeBucket, PoolFeeInfoOf<T>),
+	AppendFee(T::FeeId, PoolFeeBucket, PoolFeeInfoOf<T>),
 }

runtime/common/src/changes.rs

@@ -67,7 +67,7 @@ impl<T: Changeable, Options: Clone> RuntimeChange<T, Options> {
 				OracleCollectionChange::CollectionInfo(_) => vec![],
 			},
 			RuntimeChange::PoolFee(pool_fees_change) => match pool_fees_change {
-				PoolFeesChange::AppendFee(_, _) => vec![week],
+				PoolFeesChange::AppendFee(_, _, _) => vec![week],
 			},
 			RuntimeChange::_Unreachable(_) => vec![],
 		}