[NPoS] Some simple refactors to Delegate Staking (#4981)

## Changes
- `fn update_payee` is renamed to `fn set_payee` in the trait
`StakingInterface` since there is also a call `Staking::update_payee`
which does something different, ie used for migrating deprecated
`Controller` accounts.
- `set_payee` does not re-dispatch, only mutates ledger.
- Fix rustdocs for `NominationPools::join`.
- Add an implementation note about why we cannot allow existing stakers
to join/bond_extra into the pool.

substrate/frame/delegated-staking/src/lib.rs

@@ -411,6 +411,11 @@ pub mod pallet {
 				Delegation::<T>::can_delegate(&delegator, &agent),
 				Error::<T>::InvalidDelegation
 			);
+
+			// Implementation note: Staking uses deprecated locks (similar to freeze) which are not
+			// mutually exclusive of holds. This means, if we allow delegating for existing stakers,
+			// already staked funds might be reused for delegation. We avoid that by just blocking
+			// this.
 			ensure!(!Self::is_direct_staker(&delegator), Error::<T>::AlreadyStaking);
 
 			// ensure agent is sane.
@@ -505,7 +510,7 @@ impl<T: Config> Pallet<T> {
 			Preservation::Expendable,
 		)?;
 
-		T::CoreStaking::update_payee(who, reward_account)?;
+		T::CoreStaking::set_payee(who, reward_account)?;
 		// delegate all transferred funds back to agent.
 		Self::do_delegate(proxy_delegator, Agent::from(who.clone()), amount_to_transfer)?;
 

substrate/frame/delegated-staking/src/tests.rs

@@ -573,14 +573,14 @@ mod staking_integration {
 				100
 			));
 
-			// update_payee to self fails.
+			// set_payee to self fails.
 			assert_noop!(
-				<Staking as StakingInterface>::update_payee(&200, &200),
+				<Staking as StakingInterface>::set_payee(&200, &200),
 				StakingError::<T>::RewardDestinationRestricted
 			);
 
 			// passing correct reward destination works
-			assert_ok!(<Staking as StakingInterface>::update_payee(&200, &201));
+			assert_ok!(<Staking as StakingInterface>::set_payee(&200, &201));
 
 			// amount is staked correctly
 			assert!(eq_stake(200, 100, 100));

substrate/frame/nomination-pools/src/lib.rs

@@ -1985,8 +1985,13 @@ pub mod pallet {
 
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
-		/// Stake funds with a pool. The amount to bond is transferred from the member to the
-		/// pools account and immediately increases the pools bond.
+		/// Stake funds with a pool. The amount to bond is transferred from the member to the pool
+		/// account and immediately increases the pools bond.
+		///
+		/// The method of transferring the amount to the pool account is determined by
+		/// [`adapter::StakeStrategyType`]. If the pool is configured to use
+		/// [`adapter::StakeStrategyType::Delegate`], the funds remain in the account of
+		/// the `origin`, while the pool gains the right to use these funds for staking.
 		///
 		/// # Note
 		///

substrate/frame/nomination-pools/src/mock.rs

@@ -136,7 +136,7 @@ impl sp_staking::StakingInterface for StakingMock {
 		Ok(())
 	}
 
-	fn update_payee(_stash: &Self::AccountId, _reward_acc: &Self::AccountId) -> DispatchResult {
+	fn set_payee(_stash: &Self::AccountId, _reward_acc: &Self::AccountId) -> DispatchResult {
 		unimplemented!("method currently not used in testing")
 	}
 

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

@@ -1779,7 +1779,7 @@ impl<T: Config> StakingInterface for Pallet<T> {
 			.map(|_| ())
 	}
 
-	fn update_payee(stash: &Self::AccountId, reward_acc: &Self::AccountId) -> DispatchResult {
+	fn set_payee(stash: &Self::AccountId, reward_acc: &Self::AccountId) -> DispatchResult {
 		// Since virtual stakers are not allowed to compound their rewards as this pallet does not
 		// manage their locks, we do not allow reward account to be set same as stash. For
 		// external pallets that manage the virtual bond, they can claim rewards and re-bond them.
@@ -1788,12 +1788,12 @@ impl<T: Config> StakingInterface for Pallet<T> {
 			Error::<T>::RewardDestinationRestricted
 		);
 
-		// since controller is deprecated and this function is never used for old ledgers with
-		// distinct controllers, we can safely assume that stash is the controller.
-		Self::set_payee(
-			RawOrigin::Signed(stash.clone()).into(),
-			RewardDestination::Account(reward_acc.clone()),
-		)
+		let ledger = Self::ledger(Stash(stash.clone()))?;
+		let _ = ledger
+			.set_payee(RewardDestination::Account(reward_acc.clone()))
+			.defensive_proof("ledger was retrieved from storage, thus its bonded; qed.")?;
+
+		Ok(())
 	}
 
 	fn chill(who: &Self::AccountId) -> DispatchResult {

substrate/frame/staking/src/tests.rs

@@ -7157,7 +7157,7 @@ mod staking_unchecked {
 
 			// cannot set via set_payee as well.
 			assert_noop!(
-				<Staking as StakingInterface>::update_payee(&10, &10),
+				<Staking as StakingInterface>::set_payee(&10, &10),
 				Error::<Test>::RewardDestinationRestricted
 			);
 		});
@@ -7219,7 +7219,7 @@ mod staking_unchecked {
 			// migrate them to virtual staker
 			<Staking as StakingUnchecked>::migrate_to_virtual_staker(&200);
 			// payee needs to be updated to a non-stash account.
-			assert_ok!(<Staking as StakingInterface>::update_payee(&200, &201));
+			assert_ok!(<Staking as StakingInterface>::set_payee(&200, &201));
 
 			// ensure the balance is not locked anymore
 			assert_eq!(Balances::balance_locked(crate::STAKING_ID, &200), 0);
@@ -7246,7 +7246,7 @@ mod staking_unchecked {
 				// make 101 a virtual nominator
 				<Staking as StakingUnchecked>::migrate_to_virtual_staker(&101);
 				// set payee different to self.
-				assert_ok!(<Staking as StakingInterface>::update_payee(&101, &102));
+				assert_ok!(<Staking as StakingInterface>::set_payee(&101, &102));
 
 				// cache values
 				let nominator_stake = Staking::ledger(101.into()).unwrap().active;
@@ -7321,7 +7321,7 @@ mod staking_unchecked {
 				// make 101 a virtual nominator
 				<Staking as StakingUnchecked>::migrate_to_virtual_staker(&101);
 				// set payee different to self.
-				assert_ok!(<Staking as StakingInterface>::update_payee(&101, &102));
+				assert_ok!(<Staking as StakingInterface>::set_payee(&101, &102));
 
 				// cache values
 				let validator_balance = Balances::free_balance(&11);

substrate/primitives/staking/src/lib.rs

@@ -254,8 +254,8 @@ pub trait StakingInterface {
 	/// schedules have reached their unlocking era should allow more calls to this function.
 	fn unbond(stash: &Self::AccountId, value: Self::Balance) -> DispatchResult;
 
-	/// Update the reward destination for the ledger associated with the stash.
-	fn update_payee(stash: &Self::AccountId, reward_acc: &Self::AccountId) -> DispatchResult;
+	/// Set the reward destination for the ledger associated with the stash.
+	fn set_payee(stash: &Self::AccountId, reward_acc: &Self::AccountId) -> DispatchResult;
 
 	/// Unlock any funds schedule to unlock before or at the current era.
 	///