Add set_partial_params dispatchable function (paritytech#3843)

# ISSUE
- Link to issue: paritytech#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<T, I> =
		ParamsType<Option<<T as Config<I>>::Balance>, Option<BlockNumberFor<T>>, 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 <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_ambassador_core.rs

@@ -58,6 +58,17 @@ impl<T: frame_system::Config> 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)

cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_core_fellowship_fellowship_core.rs

@@ -57,6 +57,17 @@ impl<T: frame_system::Config> 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)

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

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::<T, I>::set_params(RawOrigin::Root.into(), Box::new(params))?;
+
+		let default_params = Params::<T, I>::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::<T, I>::get(), expected_params);
+		Ok(())
+	}
+
 	#[benchmark]
 	fn bump_offboard() -> Result<(), BenchmarkError> {
 		set_benchmark_params::<T, I>()?;

substrate/frame/core-fellowship/src/lib.rs

@@ -222,6 +222,11 @@ pub mod pallet {
 
 	pub type ParamsOf<T, I> =
 		ParamsType<<T as Config<I>>::Balance, BlockNumberFor<T>, <T as Config<I>>::MaxRank>;
+	pub type PartialParamsOf<T, I> = ParamsType<
+		Option<<T as Config<I>>::Balance>,
+		Option<BlockNumberFor<T>>,
+		<T as Config<I>>::MaxRank,
+	>;
 	pub type MemberStatusOf<T> = MemberStatus<BlockNumberFor<T>>;
 	pub type RankOf<T, I> = <<T as Config<I>>::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<T>,
+			partial_params: Box<PartialParamsOf<T, I>>,
+		) -> DispatchResult {
+			T::ParamsOrigin::ensure_origin_or_root(origin)?;
+			let params = Params::<T, I>::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::<T, I>::ParamsChanged { params });
+			Ok(())
+		}
 	}
 
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
+		/// 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<S>(
+			base_slice: &mut BoundedVec<S, <T as Config<I>>::MaxRank>,
+			new_slice: BoundedVec<Option<S>, <T as Config<I>>::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

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::<Test>::get();
+		assert_eq!(raw_updated_params, updated_params);
+
+		System::assert_last_event(
+			Event::<Test, _>::ParamsChanged { params: updated_params }.into(),
+		);
+	});
+}
+
 #[test]
 fn induct_works() {
 	new_test_ext().execute_with(|| {