Deprecate RewardDestination::Controller (#2380)

Deprecates `RewardDestination::Controller` variant.

- [x] `RewardDestination::Controller` annotated with `#[deprecated]`.
- [x] `Controller` variant is now handled the same way as `Stash` in
`payout_stakers`.
- [x] `set_payee` errors if `RewardDestination::Controller` is provided.
- [x] Added `update_payee` call to lazily migrate
`RewardDestination::Controller` `Payee` storage entries to
`RewardDestination::Account(controller)` .
- [x] `payout_stakers_dead_controller` has been removed from benches &
weights - was not used.
- [x] Tests no longer use `RewardDestination::Controller`.

---------

Co-authored-by: command-bot <>
Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
Co-authored-by: georgepisaltu <52418509+georgepisaltu@users.noreply.github.com>

substrate/frame/staking/src/benchmarking.rs

@@ -464,16 +464,28 @@ benchmarks! {
 	}
 
 	set_payee {
-		let (stash, controller) = create_stash_controller::<T>(USER_SEED, 100, Default::default())?;
+		let (stash, controller) = create_stash_controller::<T>(USER_SEED, 100, RewardDestination::Staked)?;
 		assert_eq!(Payee::<T>::get(&stash), RewardDestination::Staked);
 		whitelist_account!(controller);
-	}: _(RawOrigin::Signed(controller), RewardDestination::Controller)
+	}: _(RawOrigin::Signed(controller.clone()), RewardDestination::Account(controller.clone()))
 	verify {
-		assert_eq!(Payee::<T>::get(&stash), RewardDestination::Controller);
+		assert_eq!(Payee::<T>::get(&stash), RewardDestination::Account(controller));
+	}
+
+	update_payee {
+		let (stash, controller) = create_stash_controller::<T>(USER_SEED, 100, RewardDestination::Staked)?;
+		Payee::<T>::insert(&stash, {
+			#[allow(deprecated)]
+			RewardDestination::Controller
+		});
+		whitelist_account!(controller);
+	}: _(RawOrigin::Signed(controller.clone()), controller.clone())
+	verify {
+		assert_eq!(Payee::<T>::get(&stash), RewardDestination::Account(controller));
 	}
 
 	set_controller {
-		let (stash, ctlr) = create_unique_stash_controller::<T>(9000, 100, Default::default(), false)?;
+		let (stash, ctlr) = create_unique_stash_controller::<T>(9000, 100, RewardDestination::Staked, false)?;
 		// ensure `ctlr` is the currently stored controller.
 		assert!(!Ledger::<T>::contains_key(&stash));
 		assert!(Ledger::<T>::contains_key(&ctlr));
@@ -551,40 +563,6 @@ benchmarks! {
 		assert_eq!(UnappliedSlashes::<T>::get(&era).len(), (MAX_SLASHES - s) as usize);
 	}
 
-	payout_stakers_dead_controller {
-		let n in 0 .. T::MaxExposurePageSize::get() as u32;
-		let (validator, nominators) = create_validator_with_nominators::<T>(
-			n,
-			T::MaxExposurePageSize::get() as u32,
-			true,
-			true,
-			RewardDestination::Controller,
-		)?;
-
-		let current_era = CurrentEra::<T>::get().unwrap();
-		// set the commission for this particular era as well.
-		<ErasValidatorPrefs<T>>::insert(current_era, validator.clone(), <Staking<T>>::validators(&validator));
-
-		let caller = whitelisted_caller();
-		let validator_controller = <Bonded<T>>::get(&validator).unwrap();
-		let balance_before = T::Currency::free_balance(&validator_controller);
-		for (_, controller) in &nominators {
-			let balance = T::Currency::free_balance(controller);
-			ensure!(balance.is_zero(), "Controller has balance, but should be dead.");
-		}
-	}: payout_stakers_by_page(RawOrigin::Signed(caller), validator, current_era, 0)
-	verify {
-		let balance_after = T::Currency::free_balance(&validator_controller);
-		ensure!(
-			balance_before < balance_after,
-			"Balance of validator controller should have increased after payout.",
-		);
-		for (_, controller) in &nominators {
-			let balance = T::Currency::free_balance(controller);
-			ensure!(!balance.is_zero(), "Payout not given to controller.");
-		}
-	}
-
 	payout_stakers_alive_staked {
 		let n in 0 .. T::MaxExposurePageSize::get() as u32;
 		let (validator, nominators) = create_validator_with_nominators::<T>(

substrate/frame/staking/src/lib.rs

@@ -240,9 +240,9 @@
 //! [`Payee`] storage item (see
 //! [`set_payee`](Call::set_payee)), to be one of the following:
 //!
-//! - Controller account, (obviously) not increasing the staked value.
 //! - Stash account, not increasing the staked value.
 //! - Stash account, also increasing the staked value.
+//! - Any other account, sent as free balance.
 //!
 //! ### Additional Fund Management Operations
 //!
@@ -404,7 +404,9 @@ pub enum RewardDestination<AccountId> {
 	Staked,
 	/// Pay into the stash account, not increasing the amount at stake.
 	Stash,
-	/// Pay into the controller account.
+	#[deprecated(
+		note = "`Controller` will be removed after January 2024. Use `Account(controller)` instead. This variant now behaves the same as `Stash` variant."
+	)]
 	Controller,
 	/// Pay into a specified account.
 	Account(AccountId),

substrate/frame/staking/src/mock.rs

@@ -594,7 +594,7 @@ pub(crate) fn current_era() -> EraIndex {
 
 pub(crate) fn bond(who: AccountId, val: Balance) {
 	let _ = Balances::make_free_balance_be(&who, val);
-	assert_ok!(Staking::bond(RuntimeOrigin::signed(who), val, RewardDestination::Controller));
+	assert_ok!(Staking::bond(RuntimeOrigin::signed(who), val, RewardDestination::Stash));
 }
 
 pub(crate) fn bond_validator(who: AccountId, val: Balance) {

substrate/frame/staking/src/pallet/impls.rs

@@ -338,9 +338,8 @@ impl<T: Config> Pallet<T> {
 
 		let dest = Self::payee(StakingAccount::Stash(stash.clone()));
 		let maybe_imbalance = match dest {
-			RewardDestination::Controller => Self::bonded(stash)
-				.map(|controller| T::Currency::deposit_creating(&controller, amount)),
-			RewardDestination::Stash => T::Currency::deposit_into_existing(stash, amount).ok(),
+			RewardDestination::Stash  =>
+				T::Currency::deposit_into_existing(stash, amount).ok(),
 			RewardDestination::Staked => Self::ledger(Stash(stash.clone()))
 				.and_then(|mut ledger| {
 					ledger.active += amount;
@@ -357,6 +356,14 @@ impl<T: Config> Pallet<T> {
 			RewardDestination::Account(dest_account) =>
 				Some(T::Currency::deposit_creating(&dest_account, amount)),
 			RewardDestination::None => None,
+			#[allow(deprecated)]
+			RewardDestination::Controller => Self::bonded(stash)
+					.map(|controller| {
+						defensive!("Paying out controller as reward destination which is deprecated and should be migrated");
+						// This should never happen once payees with a `Controller` variant have been migrated.
+						// But if it does, just pay the controller account.
+						T::Currency::deposit_creating(&controller, amount)
+		}),
 		};
 		maybe_imbalance
 			.map(|imbalance| (imbalance, Self::payee(StakingAccount::Stash(stash.clone()))))

substrate/frame/staking/src/pallet/mod.rs

@@ -842,6 +842,8 @@ pub mod pallet {
 		CommissionTooLow,
 		/// Some bound is not met.
 		BoundNotMet,
+		/// Used when attempting to use deprecated controller account logic.
+		ControllerDeprecated,
 	}
 
 	#[pallet::hooks]
@@ -1283,10 +1285,19 @@ pub mod pallet {
 			payee: RewardDestination<T::AccountId>,
 		) -> DispatchResult {
 			let controller = ensure_signed(origin)?;
-			let ledger = Self::ledger(Controller(controller))?;
+			let ledger = Self::ledger(Controller(controller.clone()))?;
+
+			ensure!(
+				(payee != {
+					#[allow(deprecated)]
+					RewardDestination::Controller
+				}),
+				Error::<T>::ControllerDeprecated
+			);
+
 			let _ = ledger
 				.set_payee(payee)
-				.defensive_proof("ledger was retrieved from storage, thus its bonded; qed.");
+				.defensive_proof("ledger was retrieved from storage, thus its bonded; qed.")?;
 
 			Ok(())
 		}
@@ -1872,6 +1883,36 @@ pub mod pallet {
 			ensure_signed(origin)?;
 			Self::do_payout_stakers_by_page(validator_stash, era, page)
 		}
+
+		/// Migrates an account's `RewardDestination::Controller` to
+		/// `RewardDestination::Account(controller)`.
+		///
+		/// Effects will be felt instantly (as soon as this function is completed successfully).
+		///
+		/// This will waive the transaction fee if the `payee` is successfully migrated.
+		#[pallet::call_index(27)]
+		#[pallet::weight(T::WeightInfo::update_payee())]
+		pub fn update_payee(
+			origin: OriginFor<T>,
+			controller: T::AccountId,
+		) -> DispatchResultWithPostInfo {
+			let _ = ensure_signed(origin)?;
+			let ledger = Self::ledger(StakingAccount::Controller(controller.clone()))?;
+
+			ensure!(
+				(Payee::<T>::get(&ledger.stash) == {
+					#[allow(deprecated)]
+					RewardDestination::Controller
+				}),
+				Error::<T>::NotController
+			);
+
+			let _ = ledger
+				.set_payee(RewardDestination::Account(controller))
+				.defensive_proof("ledger should have been previously retrieved from storage.")?;
+
+			Ok(Pays::No.into())
+		}
 	}
 }