From 196761c7b0312af020c669c4b21e0d5e4f036da1 Mon Sep 17 00:00:00 2001 From: Tin Chung <56880684+chungquantin@users.noreply.github.com> Date: Tue, 18 Jun 2024 19:30:13 +0800 Subject: [PATCH] Add set_partial_params dispatchable function (#3843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # ISSUE - Link to issue: https://github.com/paritytech/polkadot-sdk/issues/3617 # Description > Any set parameter / update config call with multiple arguments should have each argument to be an Option field. Please put this to some best practice document. This allows new update config call does not need to duplicate the fields that does not need to update. It also makes concurrent votes of update call possible, otherwise there will be race condition. It also helps with review such proposal otherwise reviewers need to check the other fields should remain the same. - [ ] Concurrent call & race condition testing - [x] Each argument of the `ParamsType` is an `Option` field. Introduce through ```rust pub type PartialParamsOf = ParamsType>::Balance>, Option>, RANK_COUNT>; ``` # Outcome ```rust let params = ParamsType { active_salary: [None; 9], passive_salary: [None; 9], demotion_period: [None, Some(10), None, None, None, None, None, None, None], min_promotion_period: [None; 9], offboard_timeout: Some(1), }; CoreFellowship::set_partial_params(signed(2), Box::new(params.clone())), ``` Test coverage ```diff running 21 tests test tests::unit::__construct_runtime_integrity_test::runtime_integrity_tests ... ok test tests::unit::basic_stuff ... ok test tests::integration::test_genesis_config_builds ... ok test tests::integration::__construct_runtime_integrity_test::runtime_integrity_tests ... ok test tests::unit::auto_demote_offboard_works ... ok test tests::unit::auto_demote_works ... ok test tests::unit::get_salary_works ... ok test tests::unit::active_changing_get_salary_works ... ok test tests::integration::swap_bad_noops ... ok test tests::unit::promote_postpones_auto_demote ... ok test tests::unit::infinite_demotion_period_works ... ok test tests::unit::proof_postpones_auto_demote ... ok test tests::unit::induct_works ... ok test tests::unit::set_params_works ... ok test tests::unit::test_genesis_config_builds ... ok test tests::unit::offboard_works ... ok test tests::unit::sync_works ... ok + test tests::unit::set_partial_params_works ... ok test tests::integration::swap_exhaustive_works ... ok test tests::unit::promote_works ... ok test tests::integration::swap_simple_works ... ok test result: ok. 21 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s Doc-tests pallet_core_fellowship running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s ``` polkadot address: 19nSqFQorfF2HxD3oBzWM3oCh4SaCRKWt1yvmgaPYGCo71J --------- Co-authored-by: Dónal Murray Co-authored-by: Oliver Tale-Yazdi Co-authored-by: Bastian Köcher --- .../pallet_core_fellowship_ambassador_core.rs | 11 ++++ .../pallet_core_fellowship_fellowship_core.rs | 11 ++++ prdoc/pr_3843.prdoc | 17 ++++++ .../frame/core-fellowship/src/benchmarking.rs | 39 +++++++++++++ substrate/frame/core-fellowship/src/lib.rs | 55 +++++++++++++++++++ .../frame/core-fellowship/src/tests/unit.rs | 34 ++++++++++++ .../frame/core-fellowship/src/weights.rs | 21 +++++++ 7 files changed, 188 insertions(+) create mode 100644 prdoc/pr_3843.prdoc diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs index f40940a8b25f..dbe681f51bb2 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs @@ -58,6 +58,17 @@ impl pallet_core_fellowship::WeightInfo for WeightInfo< .saturating_add(Weight::from_parts(0, 0)) .saturating_add(T::DbWeight::get().writes(1)) } + /// Storage: `AmbassadorCore::Params` (r:0 w:1) + /// Proof: `AmbassadorCore::Params` (`max_values`: Some(1), `max_size`: Some(364), added: 859, mode: `MaxEncodedLen`) + fn set_partial_params() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 11_000_000 picoseconds. + Weight::from_parts(11_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } /// Storage: `AmbassadorCore::Member` (r:1 w:1) /// Proof: `AmbassadorCore::Member` (`max_values`: None, `max_size`: Some(49), added: 2524, mode: `MaxEncodedLen`) /// Storage: `AmbassadorCollective::Members` (r:1 w:1) diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs index 471ee82ead72..7e6264c0c10d 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs @@ -57,6 +57,17 @@ impl pallet_core_fellowship::WeightInfo for WeightInfo< .saturating_add(Weight::from_parts(0, 0)) .saturating_add(T::DbWeight::get().writes(1)) } + /// Storage: `FellowshipCore::Params` (r:0 w:1) + /// Proof: `FellowshipCore::Params` (`max_values`: Some(1), `max_size`: Some(364), added: 859, mode: `MaxEncodedLen`) + fn set_partial_params() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 11_000_000 picoseconds. + Weight::from_parts(12_000_000, 0) + .saturating_add(Weight::from_parts(0, 0)) + .saturating_add(T::DbWeight::get().writes(1)) + } /// Storage: `FellowshipCore::Member` (r:1 w:1) /// Proof: `FellowshipCore::Member` (`max_values`: None, `max_size`: Some(49), added: 2524, mode: `MaxEncodedLen`) /// Storage: `FellowshipCollective::Members` (r:1 w:1) diff --git a/prdoc/pr_3843.prdoc b/prdoc/pr_3843.prdoc new file mode 100644 index 000000000000..e01900dcc25b --- /dev/null +++ b/prdoc/pr_3843.prdoc @@ -0,0 +1,17 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Introduce a new dispatchable function `set_partial_params` in `pallet-core-fellowship` + +doc: + - audience: Runtime Dev + description: | + This PR adds a new dispatchable function `set_partial_params` + to update config with multiple arguments without duplicating the + fields that does not need to update. + +crates: + - name: pallet-core-fellowship + bump: major + - name: collectives-westend-runtime + bump: patch diff --git a/substrate/frame/core-fellowship/src/benchmarking.rs b/substrate/frame/core-fellowship/src/benchmarking.rs index b3ee3ab7d165..2dabab3983d0 100644 --- a/substrate/frame/core-fellowship/src/benchmarking.rs +++ b/substrate/frame/core-fellowship/src/benchmarking.rs @@ -85,6 +85,45 @@ mod benchmarks { Ok(()) } + #[benchmark] + fn set_partial_params() -> Result<(), BenchmarkError> { + let max_rank = T::MaxRank::get().try_into().unwrap(); + + // Set up the initial default state for the Params storage + let params = ParamsType { + active_salary: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(), + passive_salary: BoundedVec::try_from(vec![10u32.into(); max_rank]).unwrap(), + demotion_period: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(), + min_promotion_period: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(), + offboard_timeout: 1u32.into(), + }; + CoreFellowship::::set_params(RawOrigin::Root.into(), Box::new(params))?; + + let default_params = Params::::get(); + let expected_params = ParamsType { + active_salary: default_params.active_salary, + passive_salary: BoundedVec::try_from(vec![10u32.into(); max_rank]).unwrap(), + demotion_period: default_params.demotion_period, + min_promotion_period: BoundedVec::try_from(vec![100u32.into(); max_rank]).unwrap(), + offboard_timeout: 1u32.into(), + }; + + let params_payload = ParamsType { + active_salary: BoundedVec::try_from(vec![None; max_rank]).unwrap(), + passive_salary: BoundedVec::try_from(vec![Some(10u32.into()); max_rank]).unwrap(), + demotion_period: BoundedVec::try_from(vec![None; max_rank]).unwrap(), + min_promotion_period: BoundedVec::try_from(vec![Some(100u32.into()); max_rank]) + .unwrap(), + offboard_timeout: None, + }; + + #[extrinsic_call] + _(RawOrigin::Root, Box::new(params_payload.clone())); + + assert_eq!(Params::::get(), expected_params); + Ok(()) + } + #[benchmark] fn bump_offboard() -> Result<(), BenchmarkError> { set_benchmark_params::()?; diff --git a/substrate/frame/core-fellowship/src/lib.rs b/substrate/frame/core-fellowship/src/lib.rs index 94339b85d052..6f0bb77714d9 100644 --- a/substrate/frame/core-fellowship/src/lib.rs +++ b/substrate/frame/core-fellowship/src/lib.rs @@ -222,6 +222,11 @@ pub mod pallet { pub type ParamsOf = ParamsType<>::Balance, BlockNumberFor, >::MaxRank>; + pub type PartialParamsOf = ParamsType< + Option<>::Balance>, + Option>, + >::MaxRank, + >; pub type MemberStatusOf = MemberStatus>; pub type RankOf = <>::Members as RankedMembers>::Rank; @@ -558,9 +563,59 @@ pub mod pallet { Ok(Pays::No.into()) } + + /// Set the parameters partially. + /// + /// - `origin`: An origin complying with `ParamsOrigin` or root. + /// - `partial_params`: The new parameters for the pallet. + /// + /// This update config with multiple arguments without duplicating + /// the fields that does not need to update (set to None). + #[pallet::weight(T::WeightInfo::set_partial_params())] + #[pallet::call_index(9)] + pub fn set_partial_params( + origin: OriginFor, + partial_params: Box>, + ) -> DispatchResult { + T::ParamsOrigin::ensure_origin_or_root(origin)?; + let params = Params::::mutate(|p| { + Self::set_partial_params_slice(&mut p.active_salary, partial_params.active_salary); + Self::set_partial_params_slice( + &mut p.passive_salary, + partial_params.passive_salary, + ); + Self::set_partial_params_slice( + &mut p.demotion_period, + partial_params.demotion_period, + ); + Self::set_partial_params_slice( + &mut p.min_promotion_period, + partial_params.min_promotion_period, + ); + if let Some(new_offboard_timeout) = partial_params.offboard_timeout { + p.offboard_timeout = new_offboard_timeout; + } + p.clone() + }); + Self::deposit_event(Event::::ParamsChanged { params }); + Ok(()) + } } impl, I: 'static> Pallet { + /// Partially update the base slice with a new slice + /// + /// Only elements in the base slice which has a new value in the new slice will be updated. + pub(crate) fn set_partial_params_slice( + base_slice: &mut BoundedVec>::MaxRank>, + new_slice: BoundedVec, >::MaxRank>, + ) { + for (base_element, new_element) in base_slice.iter_mut().zip(new_slice) { + if let Some(element) = new_element { + *base_element = element; + } + } + } /// Convert a rank into a `0..RANK_COUNT` index suitable for the arrays in Params. /// /// Rank 1 becomes index 0, rank `RANK_COUNT` becomes index `RANK_COUNT - 1`. Any rank not diff --git a/substrate/frame/core-fellowship/src/tests/unit.rs b/substrate/frame/core-fellowship/src/tests/unit.rs index 9245e5159a90..5d6d59c5c891 100644 --- a/substrate/frame/core-fellowship/src/tests/unit.rs +++ b/substrate/frame/core-fellowship/src/tests/unit.rs @@ -187,6 +187,40 @@ fn set_params_works() { }); } +#[test] +fn set_partial_params_works() { + new_test_ext().execute_with(|| { + let params = ParamsType { + active_salary: bounded_vec![None; 9], + passive_salary: bounded_vec![None; 9], + demotion_period: bounded_vec![None, Some(10), None, None, None, None, None, None, None], + min_promotion_period: bounded_vec![None; 9], + offboard_timeout: Some(2), + }; + assert_noop!( + CoreFellowship::set_partial_params(signed(2), Box::new(params.clone())), + DispatchError::BadOrigin + ); + assert_ok!(CoreFellowship::set_partial_params(signed(1), Box::new(params))); + + // Update params from the base params value declared in `new_test_ext` + let raw_updated_params = ParamsType { + active_salary: bounded_vec![10, 20, 30, 40, 50, 60, 70, 80, 90], + passive_salary: bounded_vec![1, 2, 3, 4, 5, 6, 7, 8, 9], + demotion_period: bounded_vec![2, 10, 6, 8, 10, 12, 14, 16, 18], + min_promotion_period: bounded_vec![3, 6, 9, 12, 15, 18, 21, 24, 27], + offboard_timeout: 2, + }; + // Updated params stored in Params storage value + let updated_params = Params::::get(); + assert_eq!(raw_updated_params, updated_params); + + System::assert_last_event( + Event::::ParamsChanged { params: updated_params }.into(), + ); + }); +} + #[test] fn induct_works() { new_test_ext().execute_with(|| { diff --git a/substrate/frame/core-fellowship/src/weights.rs b/substrate/frame/core-fellowship/src/weights.rs index 8fad6f585c11..c1042d0ddfaf 100644 --- a/substrate/frame/core-fellowship/src/weights.rs +++ b/substrate/frame/core-fellowship/src/weights.rs @@ -50,6 +50,7 @@ use core::marker::PhantomData; /// Weight functions needed for `pallet_core_fellowship`. pub trait WeightInfo { fn set_params() -> Weight; + fn set_partial_params() -> Weight; fn bump_offboard() -> Weight; fn bump_demote() -> Weight; fn set_active() -> Weight; @@ -74,6 +75,16 @@ impl WeightInfo for SubstrateWeight { Weight::from_parts(8_018_000, 0) .saturating_add(T::DbWeight::get().writes(1_u64)) } + /// Storage: CoreFellowship Params (r:0 w:1) + /// Proof: CoreFellowship Params (max_values: Some(1), max_size: Some(364), added: 859, mode: MaxEncodedLen) + fn set_partial_params() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 9_454_000 picoseconds. + Weight::from_parts(9_804_000, 0) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } /// Storage: `CoreFellowship::Member` (r:1 w:1) /// Proof: `CoreFellowship::Member` (`max_values`: None, `max_size`: Some(49), added: 2524, mode: `MaxEncodedLen`) /// Storage: `RankedCollective::Members` (r:1 w:1) @@ -245,6 +256,16 @@ impl WeightInfo for () { Weight::from_parts(8_018_000, 0) .saturating_add(RocksDbWeight::get().writes(1_u64)) } + /// Storage: CoreFellowship Params (r:0 w:1) + /// Proof: CoreFellowship Params (max_values: Some(1), max_size: Some(364), added: 859, mode: MaxEncodedLen) + fn set_partial_params() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 9_454_000 picoseconds. + Weight::from_parts(9_804_000, 0) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } /// Storage: `CoreFellowship::Member` (r:1 w:1) /// Proof: `CoreFellowship::Member` (`max_values`: None, `max_size`: Some(49), added: 2524, mode: `MaxEncodedLen`) /// Storage: `RankedCollective::Members` (r:1 w:1)