rework staking::reap_stash

frame/staking/src/benchmarking.rs

@@ -28,7 +28,7 @@ use frame_support::{
 	traits::{Currency, CurrencyToVote, Get, Imbalance},
 };
 use sp_runtime::{
-	traits::{StaticLookup, Zero},
+	traits::{Bounded, One, StaticLookup, Zero},
 	Perbill, Percent,
 };
 use sp_staking::SessionIndex;
@@ -38,7 +38,6 @@ pub use frame_benchmarking::{
 	account, benchmarks, impl_benchmark_test_suite, whitelist_account, whitelisted_caller,
 };
 use frame_system::RawOrigin;
-use sp_runtime::traits::{Bounded, One};
 
 const SEED: u32 = 0;
 const MAX_SPANS: u32 = 100;
@@ -695,7 +694,7 @@ benchmarks! {
 		let stash = scenario.origin_stash1.clone();
 
 		add_slashing_spans::<T>(&stash, s);
-		T::Currency::make_free_balance_be(&stash, T::Currency::minimum_balance());
+		Ledger::<T>::insert(&controller, StakingLedger { active: T::Currency::minimum_balance() - One::one(), total: T::Currency::minimum_balance() - One::one(), ..Default::default() });
 
 		assert!(Bonded::<T>::contains_key(&stash));
 		assert!(T::SortedListProvider::contains(&stash));

frame/staking/src/pallet/mod.rs

@@ -1425,33 +1425,37 @@ pub mod pallet {
 			Ok(())
 		}
 
-		/// Remove all data structure concerning a staker/stash once its balance is at the minimum.
-		/// This is essentially equivalent to `withdraw_unbonded` except it can be called by anyone
-		/// and the target `stash` must have no funds left beyond the ED.
+		/// Remove all data structures concerning a staker/stash once it is at a state where it can
+		/// be considered `dust` in the staking system. The requirements are:
 		///
-		/// This can be called from any origin.
+		/// 1. the `total_balance` of the stash is below existential deposit.
+		/// 2. or, the `ledger.total` of the stash is below existential deposit.
 		///
-		/// - `stash`: The stash account to reap. Its balance must be zero.
+		/// The former can happen in cases like a slash; the latter when a fully unbonded account
+		/// is still receiving staking rewards in `RewardDestination::Staked`.
 		///
-		/// # <weight>
-		/// Complexity: O(S) where S is the number of slashing spans on the account.
-		/// DB Weight:
-		/// - Reads: Stash Account, Bonded, Slashing Spans, Locks
-		/// - Writes: Bonded, Slashing Spans (if S > 0), Ledger, Payee, Validators, Nominators,
-		///   Stash Account, Locks
-		/// - Writes Each: SpanSlash * S
-		/// # </weight>
+		/// It can be called by anyone, as long as `stash` meets the above requirements.
+		///
+		/// Refunds the transaction fees upon successful execution.
 		#[pallet::weight(T::WeightInfo::reap_stash(*num_slashing_spans))]
 		pub fn reap_stash(
-			_origin: OriginFor<T>,
+			origin: OriginFor<T>,
 			stash: T::AccountId,
 			num_slashing_spans: u32,
-		) -> DispatchResult {
-			let at_minimum = T::Currency::total_balance(&stash) == T::Currency::minimum_balance();
-			ensure!(at_minimum, Error::<T>::FundedTarget);
+		) -> DispatchResultWithPostInfo {
+			let _ = ensure_signed(origin)?;
+
+			let ed = T::Currency::minimum_balance();
+			let reapable = T::Currency::total_balance(&stash) < ed ||
+				Self::ledger(Self::bonded(stash.clone()).ok_or(Error::<T>::NotStash)?)
+					.map(|l| l.total)
+					.unwrap_or_default() < ed;
+			ensure!(reapable, Error::<T>::FundedTarget);
+
 			Self::kill_stash(&stash, num_slashing_spans)?;
 			T::Currency::remove_lock(STAKING_ID, &stash);
-			Ok(())
+
+			Ok(Pays::No.into())
 		}
 
 		/// Remove the given nominations from the calling validator.

frame/staking/src/tests.rs

@@ -1633,115 +1633,49 @@ fn reward_to_stake_works() {
 }
 
 #[test]
-fn on_free_balance_zero_stash_removes_validator() {
-	// Tests that validator storage items are cleaned up when stash is empty
-	// Tests that storage items are untouched when controller is empty
+fn reap_stash_works() {
 	ExtBuilder::default()
 		.existential_deposit(10)
 		.balance_factor(10)
 		.build_and_execute(|| {
-			// Check the balance of the validator account
+			// given
 			assert_eq!(Balances::free_balance(10), 10);
-			// Check the balance of the stash account
-			assert_eq!(Balances::free_balance(11), 10 * 1000);
-			// Check these two accounts are bonded
-			assert_eq!(Staking::bonded(&11), Some(10));
-
-			// Set payee information
-			assert_ok!(Staking::set_payee(Origin::signed(10), RewardDestination::Stash));
-
-			// Check storage items that should be cleaned up
-			assert!(<Ledger<Test>>::contains_key(&10));
-			assert!(<Bonded<Test>>::contains_key(&11));
-			assert!(<Validators<Test>>::contains_key(&11));
-			assert!(<Payee<Test>>::contains_key(&11));
-
-			// Reduce free_balance of controller to 0
-			let _ = Balances::slash(&10, Balance::max_value());
-
-			// Check the balance of the stash account has not been touched
 			assert_eq!(Balances::free_balance(11), 10 * 1000);
-			// Check these two accounts are still bonded
 			assert_eq!(Staking::bonded(&11), Some(10));
 
-			// Check storage items have not changed
 			assert!(<Ledger<Test>>::contains_key(&10));
 			assert!(<Bonded<Test>>::contains_key(&11));
 			assert!(<Validators<Test>>::contains_key(&11));
 			assert!(<Payee<Test>>::contains_key(&11));
 
-			// Reduce free_balance of stash to 0
-			let _ = Balances::slash(&11, Balance::max_value());
-			// Check total balance of stash
-			assert_eq!(Balances::total_balance(&11), 10);
-
-			// Reap the stash
-			assert_ok!(Staking::reap_stash(Origin::none(), 11, 0));
-
-			// Check storage items do not exist
-			assert!(!<Ledger<Test>>::contains_key(&10));
-			assert!(!<Bonded<Test>>::contains_key(&11));
-			assert!(!<Validators<Test>>::contains_key(&11));
-			assert!(!<Nominators<Test>>::contains_key(&11));
-			assert!(!<Payee<Test>>::contains_key(&11));
-		});
-}
-
-#[test]
-fn on_free_balance_zero_stash_removes_nominator() {
-	// Tests that nominator storage items are cleaned up when stash is empty
-	// Tests that storage items are untouched when controller is empty
-	ExtBuilder::default()
-		.existential_deposit(10)
-		.balance_factor(10)
-		.build_and_execute(|| {
-			// Make 10 a nominator
-			assert_ok!(Staking::nominate(Origin::signed(10), vec![20]));
-			// Check that account 10 is a nominator
-			assert!(<Nominators<Test>>::contains_key(11));
-			// Check the balance of the nominator account
-			assert_eq!(Balances::free_balance(10), 10);
-			// Check the balance of the stash account
-			assert_eq!(Balances::free_balance(11), 10_000);
-
-			// Set payee information
-			assert_ok!(Staking::set_payee(Origin::signed(10), RewardDestination::Stash));
-
-			// Check storage items that should be cleaned up
-			assert!(<Ledger<Test>>::contains_key(&10));
-			assert!(<Bonded<Test>>::contains_key(&11));
-			assert!(<Nominators<Test>>::contains_key(&11));
-			assert!(<Payee<Test>>::contains_key(&11));
-
-			// Reduce free_balance of controller to 0
-			let _ = Balances::slash(&10, Balance::max_value());
-			// Check total balance of account 10
-			assert_eq!(Balances::total_balance(&10), 0);
-
-			// Check the balance of the stash account has not been touched
-			assert_eq!(Balances::free_balance(11), 10_000);
-			// Check these two accounts are still bonded
-			assert_eq!(Staking::bonded(&11), Some(10));
-
-			// Check storage items have not changed
-			assert!(<Ledger<Test>>::contains_key(&10));
-			assert!(<Bonded<Test>>::contains_key(&11));
-			assert!(<Nominators<Test>>::contains_key(&11));
-			assert!(<Payee<Test>>::contains_key(&11));
+			// stash is not reapable
+			assert_noop!(
+				Staking::reap_stash(Origin::signed(20), 11, 0),
+				Error::<Test>::FundedTarget
+			);
+			// controller or any other account is not reapable
+			assert_noop!(Staking::reap_stash(Origin::signed(20), 10, 0), Error::<Test>::NotStash);
 
-			// Reduce free_balance of stash to 0
-			let _ = Balances::slash(&11, Balance::max_value());
-			// Check total balance of stash
-			assert_eq!(Balances::total_balance(&11), 10);
+			// no easy way to cause an account to go below ED, we tweak their staking ledger
+			// instead.
+			Ledger::<Test>::insert(
+				10,
+				StakingLedger {
+					stash: 11,
+					total: 5,
+					active: 5,
+					unlocking: vec![],
+					claimed_rewards: vec![],
+				},
+			);
 
-			// Reap the stash
-			assert_ok!(Staking::reap_stash(Origin::none(), 11, 0));
+			// reap-able
+			assert_ok!(Staking::reap_stash(Origin::signed(20), 11, 0));
 
-			// Check storage items do not exist
+			// then
 			assert!(!<Ledger<Test>>::contains_key(&10));
 			assert!(!<Bonded<Test>>::contains_key(&11));
 			assert!(!<Validators<Test>>::contains_key(&11));
-			assert!(!<Nominators<Test>>::contains_key(&11));
 			assert!(!<Payee<Test>>::contains_key(&11));
 		});
 }
@@ -2556,10 +2490,10 @@ fn garbage_collection_after_slashing() {
 
 			// reap_stash respects num_slashing_spans so that weight is accurate
 			assert_noop!(
-				Staking::reap_stash(Origin::none(), 11, 0),
+				Staking::reap_stash(Origin::signed(20), 11, 0),
 				Error::<Test>::IncorrectSlashingSpans
 			);
-			assert_ok!(Staking::reap_stash(Origin::none(), 11, 2));
+			assert_ok!(Staking::reap_stash(Origin::signed(20), 11, 2));
 
 			assert!(<Staking as crate::Store>::SlashingSpans::get(&11).is_none());
 			assert_eq!(<Staking as crate::Store>::SpanSlash::get(&(11, 0)).amount_slashed(), &0);