From 822fd91f7e325875693684b1657775e67a3c8fca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Mon, 22 May 2023 13:00:15 +0200 Subject: [PATCH] Deprecates the chill extrinsic in staking --- frame/staking/src/benchmarking.rs | 4 +- frame/staking/src/pallet/impls.rs | 4 +- frame/staking/src/pallet/mod.rs | 129 ++++++++++++++---------------- frame/staking/src/tests.rs | 54 ++++++------- 4 files changed, 93 insertions(+), 98 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 53589ecfe4dbc..83e7a1c066710 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -459,7 +459,7 @@ benchmarks! { assert!(T::VoterList::contains(&stash)); whitelist_account!(controller); - }: _(RawOrigin::Signed(controller)) + }: _(RawOrigin::Signed(controller), None) verify { assert!(!T::VoterList::contains(&stash)); } @@ -897,7 +897,7 @@ benchmarks! { )?; let caller = whitelisted_caller(); - }: _(RawOrigin::Signed(caller), controller) + }: chill(RawOrigin::Signed(caller), Some(controller)) verify { assert!(!T::VoterList::contains(&stash)); } diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 82a0956da7b61..38c9a2a0d1b63 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1631,7 +1631,9 @@ impl StakingInterface for Pallet { // defensive-only: any account bonded via this interface has the stash set as the // controller, but we have to be sure. Same comment anywhere else that we read this. let ctrl = Self::bonded(who).ok_or(Error::::NotStash)?; - Self::chill(RawOrigin::Signed(ctrl).into()) + Self::chill(RawOrigin::Signed(ctrl).into(), None) + .map_err(|with_post| with_post.error) + .map(|_| ()) } fn withdraw_unbonded( diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 2b33573ac210f..386816a7728fb 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -1087,7 +1087,7 @@ pub mod pallet { // Only check limits if they are not already a validator. if !Validators::::contains_key(stash) { // If this error is reached, we need to adjust the `MinValidatorBond` and start - // calling `chill_other`. Until then, we explicitly block new validators to protect + // calling `chill`. Until then, we explicitly block new validators to protect // the runtime. if let Some(max_validators) = MaxValidatorsCount::::get() { ensure!( @@ -1129,7 +1129,7 @@ pub mod pallet { // Only check limits if they are not already a nominator. if !Nominators::::contains_key(stash) { // If this error is reached, we need to adjust the `MinNominatorBond` and start - // calling `chill_other`. Until then, we explicitly block new nominators to protect + // calling `chill`. Until then, we explicitly block new nominators to protect // the runtime. if let Some(max_nominators) = MaxNominatorsCount::::get() { ensure!( @@ -1172,25 +1172,6 @@ pub mod pallet { Ok(()) } - /// Declare no desire to either validate or nominate. - /// - /// Effects will be felt at the beginning of the next era. - /// - /// The dispatch origin for this call must be _Signed_ by the controller, not the stash. - /// - /// ## Complexity - /// - Independent of the arguments. Insignificant complexity. - /// - Contains one read. - /// - Writes are limited to the `origin` account key. - #[pallet::call_index(6)] - #[pallet::weight(T::WeightInfo::chill())] - pub fn chill(origin: OriginFor) -> DispatchResult { - let controller = ensure_signed(origin)?; - let ledger = Self::ledger(&controller).ok_or(Error::::NotController)?; - Self::chill_stash(&ledger.stash); - Ok(()) - } - /// (Re-)set the payment target for a controller. /// /// Effects will be felt instantly (as soon as this function is completed successfully). @@ -1634,6 +1615,42 @@ pub mod pallet { config_op_exp!(MinCommission, min_commission); Ok(()) } + + /// Force a validator to have at least the minimum commission. This will not affect a + /// validator who already has a commission greater than or equal to the minimum. Any account + /// can call this. + #[pallet::call_index(24)] + #[pallet::weight(T::WeightInfo::force_apply_min_commission())] + pub fn force_apply_min_commission( + origin: OriginFor, + validator_stash: T::AccountId, + ) -> DispatchResult { + ensure_signed(origin)?; + let min_commission = MinCommission::::get(); + Validators::::try_mutate_exists(validator_stash, |maybe_prefs| { + maybe_prefs + .as_mut() + .map(|prefs| { + (prefs.commission < min_commission) + .then(|| prefs.commission = min_commission) + }) + .ok_or(Error::::NotStash) + })?; + Ok(()) + } + + /// Sets the minimum amount of commission that each validators must maintain. + /// + /// This call has lower privilege requirements than `set_staking_config` and can be called + /// by the `T::AdminOrigin`. Root can always call this. + #[pallet::call_index(25)] + #[pallet::weight(T::WeightInfo::set_min_commission())] + pub fn set_min_commission(origin: OriginFor, new: Perbill) -> DispatchResult { + T::AdminOrigin::ensure_origin(origin)?; + MinCommission::::put(new); + Ok(()) + } + /// Declare a `controller` to stop participating as either a validator or nominator. /// /// Effects will be felt at the beginning of the next era. @@ -1660,13 +1677,13 @@ pub mod pallet { /// /// This can be helpful if bond requirements are updated, and we need to remove old users /// who do not satisfy these requirements. - #[pallet::call_index(23)] + #[pallet::call_index(26)] #[pallet::weight(T::WeightInfo::chill_other())] - pub fn chill_other(origin: OriginFor, controller: T::AccountId) -> DispatchResult { - // Anyone can call this function. + pub fn chill( + origin: OriginFor, + controller: Option, + ) -> DispatchResultWithPostInfo { let caller = ensure_signed(origin)?; - let ledger = Self::ledger(&controller).ok_or(Error::::NotController)?; - let stash = ledger.stash; // In order for one user to chill another user, the following conditions must be met: // @@ -1683,13 +1700,16 @@ pub mod pallet { // threshold bond required. // // Otherwise, if caller is the same as the controller, this is just like `chill`. + let used_weight = if let Some(controller) = controller { + let ledger = Self::ledger(&controller).ok_or(Error::::NotController)?; + let stash = ledger.stash; + + // caller controlls the controller. + if Nominators::::contains_key(&stash) && Nominators::::get(&stash).is_none() { + Self::chill_stash(&stash); + return Ok(Some(T::WeightInfo::chill()).into()) + } - if Nominators::::contains_key(&stash) && Nominators::::get(&stash).is_none() { - Self::chill_stash(&stash); - return Ok(()) - } - - if caller != controller { let threshold = ChillThreshold::::get().ok_or(Error::::CannotChillOther)?; let min_active_bond = if Nominators::::contains_key(&stash) { let max_nominator_count = @@ -1714,45 +1734,18 @@ pub mod pallet { }; ensure!(ledger.active < min_active_bond, Error::::CannotChillOther); - } - Self::chill_stash(&stash); - Ok(()) - } + Self::chill_stash(&stash); + T::WeightInfo::chill_other() + } else { + let ledger = Self::ledger(&caller).ok_or(Error::::NotController)?; + let stash = ledger.stash; - /// Force a validator to have at least the minimum commission. This will not affect a - /// validator who already has a commission greater than or equal to the minimum. Any account - /// can call this. - #[pallet::call_index(24)] - #[pallet::weight(T::WeightInfo::force_apply_min_commission())] - pub fn force_apply_min_commission( - origin: OriginFor, - validator_stash: T::AccountId, - ) -> DispatchResult { - ensure_signed(origin)?; - let min_commission = MinCommission::::get(); - Validators::::try_mutate_exists(validator_stash, |maybe_prefs| { - maybe_prefs - .as_mut() - .map(|prefs| { - (prefs.commission < min_commission) - .then(|| prefs.commission = min_commission) - }) - .ok_or(Error::::NotStash) - })?; - Ok(()) - } + Self::chill_stash(&stash); + T::WeightInfo::chill() + }; - /// Sets the minimum amount of commission that each validators must maintain. - /// - /// This call has lower privilege requirements than `set_staking_config` and can be called - /// by the `T::AdminOrigin`. Root can always call this. - #[pallet::call_index(25)] - #[pallet::weight(T::WeightInfo::set_min_commission())] - pub fn set_min_commission(origin: OriginFor, new: Perbill) -> DispatchResult { - T::AdminOrigin::ensure_origin(origin)?; - MinCommission::::put(new); - Ok(()) + Ok(Some(used_weight).into()) } } } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index e3ee4cd1a8e9f..cae0b0d8207c1 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -243,7 +243,7 @@ fn change_controller_works() { assert_eq!(Staking::bonded(&stash), Some(controller)); // `controller` can control `stash` who is initially a validator. - assert_ok!(Staking::chill(RuntimeOrigin::signed(controller))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(controller), None)); // sets controller back to `stash`. assert_ok!(Staking::set_controller(RuntimeOrigin::signed(stash))); @@ -266,7 +266,7 @@ fn change_controller_already_paired_once_stash() { assert_eq!(Staking::bonded(&11), Some(11)); // 11 is initially a validator. - assert_ok!(Staking::chill(RuntimeOrigin::signed(11))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(11), None)); // Controller cannot change once matching with stash. assert_noop!( @@ -442,7 +442,7 @@ fn staking_should_work() { assert_eq_uvec!(validator_controllers(), vec![21, 3]); // --- Block 6: Unstake 4 as a validator, freeing up the balance stashed in 3 // 4 will chill - Staking::chill(RuntimeOrigin::signed(3)).unwrap(); + Staking::chill(RuntimeOrigin::signed(3), None).unwrap(); // --- Block 7: nothing. 3 is still there. start_session(7); @@ -540,7 +540,7 @@ fn no_candidate_emergency_condition() { MinimumValidatorCount::::put(11); // try to chill - let res = Staking::chill(RuntimeOrigin::signed(11)); + let res = Staking::chill(RuntimeOrigin::signed(11), None); assert_ok!(res); let current_era = CurrentEra::::get(); @@ -1969,7 +1969,7 @@ fn bond_with_little_staked_value_bounded() { .minimum_validator_count(1) .build_and_execute(|| { // setup - assert_ok!(Staking::chill(RuntimeOrigin::signed(31))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(31), None)); assert_ok!(Staking::set_payee( RuntimeOrigin::signed(11), RewardDestination::Controller @@ -2151,8 +2151,8 @@ fn phragmen_should_not_overflow() { // This is the maximum value that we can have as the outcome of CurrencyToVote. type Votes = u64; - let _ = Staking::chill(RuntimeOrigin::signed(10)); - let _ = Staking::chill(RuntimeOrigin::signed(20)); + let _ = Staking::chill(RuntimeOrigin::signed(10), None); + let _ = Staking::chill(RuntimeOrigin::signed(20), None); bond_validator(3, Votes::max_value() as Balance); bond_validator(5, Votes::max_value() as Balance); @@ -2965,7 +2965,7 @@ fn retroactive_deferred_slashes_one_before() { // unbond at slash era. mock::start_active_era(2); - assert_ok!(Staking::chill(RuntimeOrigin::signed(11))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(11), None)); assert_ok!(Staking::unbond(RuntimeOrigin::signed(11), 100)); mock::start_active_era(3); @@ -3021,7 +3021,7 @@ fn staker_cannot_bail_deferred_slash() { ); // now we chill - assert_ok!(Staking::chill(RuntimeOrigin::signed(101))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(101), None)); assert_ok!(Staking::unbond(RuntimeOrigin::signed(101), 500)); assert_eq!(Staking::current_era().unwrap(), 1); @@ -4354,7 +4354,7 @@ fn cannot_rebond_to_lower_than_ed() { ); // unbond all of it. must be chilled first. - assert_ok!(Staking::chill(RuntimeOrigin::signed(21))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(21), None)); assert_ok!(Staking::unbond(RuntimeOrigin::signed(21), 11 * 1000)); assert_eq!( Staking::ledger(&21).unwrap(), @@ -4394,7 +4394,7 @@ fn cannot_bond_extra_to_lower_than_ed() { ); // unbond all of it. must be chilled first. - assert_ok!(Staking::chill(RuntimeOrigin::signed(21))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(21), None)); assert_ok!(Staking::unbond(RuntimeOrigin::signed(21), 11 * 1000)); assert_eq!( Staking::ledger(&21).unwrap(), @@ -4748,7 +4748,7 @@ fn min_bond_checks_work() { ); // Once they are chilled they can unbond everything - assert_ok!(Staking::chill(RuntimeOrigin::signed(3))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(3), None)); assert_ok!(Staking::unbond(RuntimeOrigin::signed(3), 1000)); }) } @@ -4793,16 +4793,16 @@ fn chill_other_works() { // * Set a limit // * Set a threshold // - // If any of these are missing, we do not have enough information to allow the - // `chill_other` to succeed from one user to another. + // If any of these are missing, we do not have enough information to allow `chill` + // to succeed from one user to another. // Can't chill these users assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 0), + Staking::chill(RuntimeOrigin::signed(1337), Some(0)), Error::::CannotChillOther ); assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 2), + Staking::chill(RuntimeOrigin::signed(1337), Some(2)), Error::::CannotChillOther ); @@ -4819,11 +4819,11 @@ fn chill_other_works() { // Still can't chill these users assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 0), + Staking::chill(RuntimeOrigin::signed(1337), Some(0)), Error::::CannotChillOther ); assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 2), + Staking::chill(RuntimeOrigin::signed(1337), Some(2)), Error::::CannotChillOther ); @@ -4840,11 +4840,11 @@ fn chill_other_works() { // Still can't chill these users assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 0), + Staking::chill(RuntimeOrigin::signed(1337), Some(0)), Error::::CannotChillOther ); assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 2), + Staking::chill(RuntimeOrigin::signed(1337), Some(2)), Error::::CannotChillOther ); @@ -4861,11 +4861,11 @@ fn chill_other_works() { // Still can't chill these users assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 0), + Staking::chill(RuntimeOrigin::signed(1337), Some(0)), Error::::CannotChillOther ); assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 2), + Staking::chill(RuntimeOrigin::signed(1337), Some(2)), Error::::CannotChillOther ); @@ -4889,19 +4889,19 @@ fn chill_other_works() { for i in 6..15 { let b = 4 * i; let d = 4 * i + 2; - assert_ok!(Staking::chill_other(RuntimeOrigin::signed(1337), b)); - assert_ok!(Staking::chill_other(RuntimeOrigin::signed(1337), d)); + assert_ok!(Staking::chill(RuntimeOrigin::signed(1337), Some(b))); + assert_ok!(Staking::chill(RuntimeOrigin::signed(1337), Some(d))); } // chill a nominator. Limit is not reached, not chill-able assert_eq!(Nominators::::count(), 7); assert_noop!( - Staking::chill_other(RuntimeOrigin::signed(1337), 0), + Staking::chill(RuntimeOrigin::signed(1337), Some(0)), Error::::CannotChillOther ); // chill a validator. Limit is reached, chill-able. assert_eq!(Validators::::count(), 9); - assert_ok!(Staking::chill_other(RuntimeOrigin::signed(1337), 2)); + assert_ok!(Staking::chill(RuntimeOrigin::signed(1337), Some(2))); }) } @@ -5143,7 +5143,7 @@ fn change_of_max_nominations() { // or they can be chilled by any account. assert!(Nominators::::contains_key(101)); assert!(Nominators::::get(101).is_none()); - assert_ok!(Staking::chill_other(RuntimeOrigin::signed(71), 101)); + assert_ok!(Staking::chill(RuntimeOrigin::signed(71), Some(101))); assert!(!Nominators::::contains_key(101)); assert!(Nominators::::get(101).is_none()); })