From f7e1232d77b9aaa1a430fc77122406f2fc6367de Mon Sep 17 00:00:00 2001 From: William Freudenberger Date: Tue, 30 Jan 2024 18:11:31 +0100 Subject: [PATCH] refactor: assign pool fee id during propose (#1706) * refactor: assign pool fee id during propose * fix: missing AppendFee refactor application --- Cargo.lock | 2 - pallets/pool-fees/src/benchmarking.rs | 1 + pallets/pool-fees/src/lib.rs | 54 ++++++++++++++++++--------- pallets/pool-fees/src/mock.rs | 16 +++++++- pallets/pool-fees/src/tests.rs | 19 +++++++++- pallets/pool-fees/src/types.rs | 2 +- runtime/common/src/changes.rs | 2 +- 7 files changed, 72 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7410f8c484..107b4eaed3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7257,9 +7257,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", - "log", "parity-scale-codec 3.6.9", - "rand 0.8.5", "scale-info", "sp-core", "sp-io", diff --git a/pallets/pool-fees/src/benchmarking.rs b/pallets/pool-fees/src/benchmarking.rs index 710af27270..197885700f 100644 --- a/pallets/pool-fees/src/benchmarking.rs +++ b/pallets/pool-fees/src/benchmarking.rs @@ -62,6 +62,7 @@ mod benchmarks { let change_id = T::ChangeGuard::note( T::PoolId::default(), Change::::AppendFee( + (n + 1).into(), PoolFeeBucket::Top, PoolFees::::get_default_fixed_fee_info(), ) diff --git a/pallets/pool-fees/src/lib.rs b/pallets/pool-fees/src/lib.rs index df08da9781..879c5321bd 100644 --- a/pallets/pool-fees/src/lib.rs +++ b/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, }, @@ -275,6 +276,8 @@ pub mod pallet { #[pallet::error] pub enum Error { + /// 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::::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::::Proposed { pool_id, + fee_id, bucket, fee, }); @@ -345,10 +353,10 @@ pub mod pallet { T::PoolReserve::pool_exists(pool_id), Error::::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 { + pub(crate) fn generate_fee_id() -> Result { LastFeeId::::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 PoolFees for Pallet { - type FeeInfo = PoolFeeInfoOf; - 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, + ) -> DispatchResult { + ensure!( + !FeeIdsToPoolBucket::::contains_key(fee_id), + Error::::FeeIdAlreadyExists + ); + FeeIdsToPoolBucket::::insert(fee_id, (pool_id, bucket)); FeeIds::::mutate(pool_id, bucket, |list| list.try_push(fee_id)) .map_err(|_| Error::::MaxPoolFeesPerBucket)?; ActiveFees::::mutate(pool_id, bucket, |list| { list.try_push(PoolFeeOf::::from_info(fee.clone(), fee_id)) }) .map_err(|_| Error::::MaxPoolFeesPerBucket)?; - FeeIdsToPoolBucket::::insert(fee_id, (pool_id, bucket)); Self::deposit_event(Event::::Added { pool_id, @@ -749,6 +755,20 @@ pub mod pallet { Ok(()) } + } + + impl PoolFees for Pallet { + type FeeInfo = PoolFeeInfoOf; + 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() diff --git a/pallets/pool-fees/src/mock.rs b/pallets/pool-fees/src/mock.rs index b28f220d5b..df59d60841 100644 --- a/pallets/pool-fees/src/mock.rs +++ b/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, ::get_default_fixed_fee_info(), )) @@ -405,7 +406,14 @@ pub(crate) fn config_change_mocks(fee: &PoolFeeInfoOf) { 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) { 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(), + )) } }); } diff --git a/pallets/pool-fees/src/tests.rs b/pallets/pool-fees/src/tests.rs index 8a798b7064..2bd3ae6ff0 100644 --- a/pallets/pool-fees/src/tests.rs +++ b/pallets/pool-fees/src/tests.rs @@ -38,6 +38,7 @@ mod extrinsics { System::assert_last_event( Event::::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::::FeeIdAlreadyExists + ); + }); + } } } diff --git a/pallets/pool-fees/src/types.rs b/pallets/pool-fees/src/types.rs index 3db29dee1e..ef30a43acd 100644 --- a/pallets/pool-fees/src/types.rs +++ b/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 { - AppendFee(PoolFeeBucket, PoolFeeInfoOf), + AppendFee(T::FeeId, PoolFeeBucket, PoolFeeInfoOf), } diff --git a/runtime/common/src/changes.rs b/runtime/common/src/changes.rs index 0cfc6a22dd..45e4c2f71f 100644 --- a/runtime/common/src/changes.rs +++ b/runtime/common/src/changes.rs @@ -67,7 +67,7 @@ impl RuntimeChange { OracleCollectionChange::CollectionInfo(_) => vec![], }, RuntimeChange::PoolFee(pool_fees_change) => match pool_fees_change { - PoolFeesChange::AppendFee(_, _) => vec![week], + PoolFeesChange::AppendFee(_, _, _) => vec![week], }, RuntimeChange::_Unreachable(_) => vec![], }