[Staking] Extrinsic migrate_currency handles any scenario where an old staking lock exists

polkadot/runtime/westend/src/tests.rs

@@ -263,7 +263,14 @@ mod remote_tests {
 			transport,
 			state_snapshot: maybe_state_snapshot.clone(),
 			child_trie: false,
-			pallets: vec!["Staking".into(), "System".into(), "Balances".into()],
+			pallets: vec![
+				"Staking".into(),
+				"System".into(),
+				"Balances".into(),
+				"NominationPools".into(),
+				"DelegatedStaking".into(),
+				"VoterList".into(),
+			],
 			..Default::default()
 		};
 		let mut ext = Builder::<Block>::default()
@@ -286,8 +293,11 @@ mod remote_tests {
 
 			let mut success = 0;
 			let mut err = 0;
+			let mut no_migration_needed = 0;
 			let mut force_withdraw_acc = 0;
-			// iterate over all pools
+			let mut force_withdraw_count = 0;
+			let mut max_force_withdraw = 0;
+			// iterate over all stakers
 			pallet_staking::Ledger::<Runtime>::iter().for_each(|(ctrl, ledger)| {
 				match pallet_staking::Pallet::<Runtime>::migrate_currency(
 					RuntimeOrigin::signed(alice.clone()).into(),
@@ -299,23 +309,32 @@ mod remote_tests {
 						let force_withdraw = ledger.total - updated_ledger.total;
 						if force_withdraw > 0 {
 							force_withdraw_acc += force_withdraw;
-							log::info!(target: "remote_test", "Force withdraw from stash {:?}: value {:?}", ledger.stash, force_withdraw);
+							force_withdraw_count += 1;
+							max_force_withdraw = max_force_withdraw.max(force_withdraw);
+							log::debug!(target: "remote_test", "Force withdraw from stash {:?}: value {:?}", ledger.stash, force_withdraw);
 						}
 						success += 1;
 					},
 					Err(e) => {
-						log::error!(target: "remote_test", "Error migrating {:?}: {:?}", ledger.stash, e);
-						err += 1;
+						if e == pallet_staking::Error::<Runtime>::AlreadyMigrated.into() {
+							no_migration_needed += 1;
+						} else {
+							log::error!(target: "remote_test", "Error migrating {:?}: {:?}", ledger.stash, e);
+							err += 1;
+						}
 					},
 				}
 			});
 
 			log::info!(
 				target: "remote_test",
-				"Migration stats: success: {}, err: {}, total force withdrawn stake: {}",
+				"Migration stats: success: {}, err: {}, total force withdrawn stake: {}, count {}, maximum amount {}, no_migration_needed: {}",
 				success,
 				err,
-				force_withdraw_acc
+				force_withdraw_acc,
+				force_withdraw_count,
+				max_force_withdraw,
+				no_migration_needed
 			);
 		});
 

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

@@ -1160,24 +1160,29 @@ impl<T: Config> Pallet<T> {
 			return Self::do_migrate_virtual_staker(stash);
 		}
 
-		let ledger = Self::ledger(Stash(stash.clone()))?;
-		let staked: BalanceOf<T> = T::OldCurrency::balance_locked(STAKING_ID, stash).into();
-		ensure!(!staked.is_zero(), Error::<T>::AlreadyMigrated);
-		ensure!(ledger.total == staked, Error::<T>::BadState);
+		let locked: BalanceOf<T> = T::OldCurrency::balance_locked(STAKING_ID, stash).into();
+		ensure!(!locked.is_zero(), Error::<T>::AlreadyMigrated);
 
 		// remove old staking lock
 		T::OldCurrency::remove_lock(STAKING_ID, &stash);
 
-		// check if we can hold all stake.
-		let max_hold = asset::free_to_stake::<T>(&stash);
-		let force_withdraw = if max_hold >= staked {
+		// Get rid of the extra consumer we used to have with OldCurrency.
+		frame_system::Pallet::<T>::dec_consumers(&stash);
+
+		let Ok(ledger) = Self::ledger(Stash(stash.clone())) else {
+			// User is no longer bonded. Removing the lock is enough.
+			return Ok(());
+		};
+
+		// Ensure we can hold all stake.
+		let max_hold = asset::stakeable_balance::<T>(&stash);
+		let force_withdraw = if max_hold >= ledger.total {
 			// this means we can hold all stake. yay!
-			asset::update_stake::<T>(&stash, staked)?;
+			asset::update_stake::<T>(&stash, ledger.total)?;
 			Zero::zero()
 		} else {
 			// if we are here, it means we cannot hold all user stake. We will do a force withdraw
 			// from ledger, but that's okay since anyways user do not have funds for it.
-
 			let old_total = ledger.total;
 			// update ledger with total stake as max_hold.
 			let updated_ledger = ledger.update_total_stake(max_hold);
@@ -1193,9 +1198,6 @@ impl<T: Config> Pallet<T> {
 			old_total.defensive_saturating_sub(new_total)
 		};
 
-		// Get rid of the extra consumer we used to have with OldCurrency.
-		frame_system::Pallet::<T>::dec_consumers(&stash);
-
 		Self::deposit_event(Event::<T>::CurrencyMigrated { stash: stash.clone(), force_withdraw });
 		Ok(())
 	}

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

@@ -2318,11 +2318,11 @@ pub mod pallet {
 			Ok(())
 		}
 
-		/// Migrates permissionlessly a stash from locks to holds.
+		/// Removes the legacy Staking locks if they exist.
 		///
-		/// This removes the old lock on the stake and creates a hold on it atomically. If all
-		/// stake cannot be held, the best effort is made to hold as much as possible. The remaining
-		/// stake is removed from the ledger.
+		/// This removes the legacy lock on the stake with [`Config::OldCurrency`] and creates a
+		/// hold on it if needed. If all stake cannot be held, the best effort is made to hold as
+		/// much as possible. The remaining stake is forced withdrawn from the ledger.
 		///
 		/// The fee is waived if the migration is successful.
 		#[pallet::call_index(30)]

substrate/frame/staking/src/tests.rs

@@ -9032,6 +9032,7 @@ mod getters {
 
 mod hold_migration {
 	use super::*;
+	use frame_support::traits::fungible::Mutate;
 	use sp_staking::{Stake, StakingInterface};
 
 	#[test]
@@ -9360,6 +9361,121 @@ mod hold_migration {
 			);
 		});
 	}
+
+	#[test]
+	fn remove_old_lock_when_stake_already_on_hold() {
+		// When the hold is already migrated because of interactions with the ledger, we still
+		// want to remove the old lock via the explicit `migrate_currency`.
+		ExtBuilder::default().has_stakers(true).build_and_execute(|| {
+			// GIVEN alice and bob who are bonded with old currency.
+			let alice = 300;
+			let bob = 301;
+			Balances::set_balance(&alice, 3000);
+			Balances::set_balance(&bob, 3000);
+
+			mock::start_active_era(1);
+			let init_stake = 1000;
+			assert_ok!(Staking::bond(
+				RuntimeOrigin::signed(alice),
+				init_stake,
+				RewardDestination::Staked
+			));
+			assert_ok!(Staking::bond(
+				RuntimeOrigin::signed(bob),
+				init_stake,
+				RewardDestination::Staked
+			));
+
+			// convert hold to lock.
+			testing_utils::migrate_to_old_currency::<Test>(alice);
+			testing_utils::migrate_to_old_currency::<Test>(bob);
+
+			// this returns the hold balance which is 0 because of the above migration.
+			assert_eq!(asset::staked::<Test>(&alice), 0);
+			assert_eq!(asset::staked::<Test>(&bob), 0);
+
+			// but instead of hold, the balance is locked.
+			assert_eq!(Balances::balance_locked(STAKING_ID, &alice), init_stake);
+			assert_eq!(Balances::balance_locked(STAKING_ID, &bob), init_stake);
+
+			// ledger has 1000 staked.
+			assert_eq!(
+				<Staking as StakingInterface>::stake(&alice),
+				Ok(Stake { total: init_stake, active: init_stake })
+			);
+			assert_eq!(
+				<Staking as StakingInterface>::stake(&bob),
+				Ok(Stake { total: init_stake, active: init_stake })
+			);
+
+			// -- WHEN Alice interacts with ledger that updates the hold.
+			assert_ok!(Staking::bond_extra(RuntimeOrigin::signed(alice), 500));
+
+			// this will update the ledger and the held balance.
+			assert_eq!(asset::staked::<Test>(&alice), init_stake + 500);
+			// but the locked balance remains
+			assert_eq!(Balances::balance_locked(STAKING_ID, &alice), init_stake);
+
+			// clear events
+			System::reset_events();
+
+			// To remove the old locks, alice needs to migrate currency.
+			// AND alice currency is migrated.
+			assert_ok!(Staking::migrate_currency(RuntimeOrigin::signed(1), alice));
+
+			// THEN
+			let expected_hold = init_stake + 500;
+			// ensure no lock
+			assert_eq!(Balances::balance_locked(STAKING_ID, &alice), 0);
+			// ensure stake and hold are same.
+			assert_eq!(
+				<Staking as StakingInterface>::stake(&alice),
+				Ok(Stake { total: expected_hold, active: expected_hold })
+			);
+			assert_eq!(asset::staked::<Test>(&alice), expected_hold);
+
+			// ensure events are emitted.
+			assert_eq!(
+				staking_events_since_last_call(),
+				vec![Event::CurrencyMigrated { stash: alice, force_withdraw: 0 }]
+			);
+
+			// ensure cannot migrate again.
+			assert_noop!(
+				Staking::migrate_currency(RuntimeOrigin::signed(1), alice),
+				Error::<Test>::AlreadyMigrated
+			);
+
+			// -- WHEN Bob withdraws all stake before migration.
+			assert_ok!(Staking::unbond(RuntimeOrigin::signed(bob), init_stake));
+
+			mock::start_active_era(4);
+			assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(bob), 0));
+
+			// assert lock still exists but there is no stake.
+			assert_eq!(Balances::balance_locked(STAKING_ID, &bob), init_stake);
+			assert_eq!(asset::staked::<Test>(&bob), 0);
+			assert_eq!(
+				<Staking as StakingInterface>::stake(&bob).unwrap_err(),
+				Error::<Test>::NotStash.into()
+			);
+
+			// clear events
+			System::reset_events();
+
+			// AND Bob wants to remove the old lock.
+			assert_ok!(Staking::migrate_currency(RuntimeOrigin::signed(1), bob));
+
+			// THEN ensure no lock
+			assert_eq!(Balances::balance_locked(STAKING_ID, &bob), 0);
+
+			// And they cannot migrate again.
+			assert_noop!(
+				Staking::migrate_currency(RuntimeOrigin::signed(1), bob),
+				Error::<Test>::AlreadyMigrated
+			);
+		});
+	}
 }
 
 // Tests for manual_slash extrinsic