From 337cae1dde554f1063f9ba93d2a30f745b1426bb Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 17 Dec 2019 16:49:00 +0100 Subject: [PATCH] Update Balances Pallet events to help block explorers (#4389) * Dust moves from reserved <-> free if below ED * Add dust information to `ReapedAccount` event * Introduce `BalanceSet` event * More cleanly written `set_balance` logic --- substrate/frame/balances/src/lib.rs | 52 +++++++++++++++++++-------- substrate/frame/balances/src/tests.rs | 43 ++++++++++++++++++++-- 2 files changed, 79 insertions(+), 16 deletions(-) diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs index 8829964238..e0d54c6020 100644 --- a/substrate/frame/balances/src/lib.rs +++ b/substrate/frame/balances/src/lib.rs @@ -263,9 +263,11 @@ decl_event!( /// A new account was created. NewAccount(AccountId, Balance), /// An account was reaped. - ReapedAccount(AccountId), + ReapedAccount(AccountId, Balance), /// Transfer succeeded (from, to, value, fees). Transfer(AccountId, AccountId, Balance, Balance), + /// A balance was set by root (who, free, reserved). + BalanceSet(AccountId, Balance, Balance), } ); @@ -456,6 +458,10 @@ decl_module! { ) { ensure_root(origin)?; let who = T::Lookup::lookup(who)?; + let existential_deposit = T::ExistentialDeposit::get(); + + let new_free = if new_free < existential_deposit { Zero::zero() } else { new_free }; + let new_reserved = if new_reserved < existential_deposit { Zero::zero() } else { new_reserved }; let current_free = >::get(&who); if new_free > current_free { @@ -472,6 +478,8 @@ decl_module! { mem::drop(NegativeImbalance::::new(current_reserved - new_reserved)); } Self::set_reserved_balance(&who, new_reserved); + + Self::deposit_event(RawEvent::BalanceSet(who, new_free, new_reserved)); } /// Exactly as `transfer`, except the origin must be root and the source account may be @@ -564,9 +572,9 @@ impl, I: Instance> Module { /// Unregister an account. /// /// This just removes the nonce and leaves an event. - fn reap_account(who: &T::AccountId) { + fn reap_account(who: &T::AccountId, dust: T::Balance) { >::remove(who); - Self::deposit_event(RawEvent::ReapedAccount(who.clone())); + Self::deposit_event(RawEvent::ReapedAccount(who.clone(), dust)); } /// Account's free balance has dropped below existential deposit. Kill its @@ -577,15 +585,23 @@ impl, I: Instance> Module { let dust = >::take(who); >::remove(who); - // underflow should never happen, but if it does, there's not much we can do about it. - if !dust.is_zero() { - T::DustRemoval::on_unbalanced(NegativeImbalance::new(dust)); - } - T::OnFreeBalanceZero::on_free_balance_zero(who); - if Self::reserved_balance(who).is_zero() { - Self::reap_account(who); + let mut reserved_balance = Self::reserved_balance(who); + + if !dust.is_zero() { + if reserved_balance >= T::ExistentialDeposit::get() { + // any individual account cannot cause overflow in balance. + reserved_balance += dust; + Self::set_reserved_balance(who, reserved_balance); + } else { + // underflow should never happen, but if it does, there's not much we can do. + T::DustRemoval::on_unbalanced(NegativeImbalance::new(dust)); + } + } + + if reserved_balance.is_zero() { + Self::reap_account(who, dust); } } @@ -596,13 +612,21 @@ impl, I: Instance> Module { fn on_reserved_too_low(who: &T::AccountId) { let dust = >::take(who); - // underflow should never happen, but it if does, there's nothing to be done here. + let mut free_balance = Self::free_balance(who); + if !dust.is_zero() { - T::DustRemoval::on_unbalanced(NegativeImbalance::new(dust)); + if free_balance >= T::ExistentialDeposit::get() { + // any individual account cannot cause overflow in balance. + free_balance += dust; + Self::set_free_balance(who, free_balance); + } else { + // underflow should never happen, but it if does, there's nothing to be done here. + T::DustRemoval::on_unbalanced(NegativeImbalance::new(dust)); + } } - if Self::free_balance(who).is_zero() { - Self::reap_account(who); + if free_balance.is_zero() { + Self::reap_account(who, dust); } } } diff --git a/substrate/frame/balances/src/tests.rs b/substrate/frame/balances/src/tests.rs index bf0fa392fc..a58426462d 100644 --- a/substrate/frame/balances/src/tests.rs +++ b/substrate/frame/balances/src/tests.rs @@ -248,7 +248,7 @@ fn reserved_balance_should_prevent_reclaim_count() { assert_ok!(Balances::reserve(&2, 256 * 19 + 1)); // account 2 becomes mostly reserved assert_eq!(Balances::free_balance(&2), 0); // "free" account deleted." - assert_eq!(Balances::total_balance(&2), 256 * 19 + 1); // reserve still exists. + assert_eq!(Balances::total_balance(&2), 256 * 20); // reserve still exists. assert_eq!(Balances::is_dead_account(&2), false); assert_eq!(System::account_nonce(&2), 1); @@ -257,7 +257,7 @@ fn reserved_balance_should_prevent_reclaim_count() { assert_eq!(Balances::total_balance(&5), 256 * 1 + 0x69); assert_eq!(Balances::is_dead_account(&5), false); - assert!(Balances::slash(&2, 256 * 18 + 2).1.is_zero()); // account 2 gets slashed + assert!(Balances::slash(&2, 256 * 19 + 2).1.is_zero()); // account 2 gets slashed // "reserve" account reduced to 255 (below ED) so account deleted assert_eq!(Balances::total_balance(&2), 0); assert_eq!(System::account_nonce(&2), 0); // nonce zero @@ -769,3 +769,42 @@ fn cannot_set_genesis_value_below_ed() { vesting: vec![], }.assimilate_storage(&mut t).unwrap(); } + +#[test] +fn dust_moves_between_free_and_reserved() { + ExtBuilder::default() + .existential_deposit(100) + .build() + .execute_with(|| { + // Set balance to free and reserved at the existential deposit + assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 100, 100)); + assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 2, 100, 100)); + // Check balance + assert_eq!(Balances::free_balance(1), 100); + assert_eq!(Balances::reserved_balance(1), 100); + assert_eq!(Balances::free_balance(2), 100); + assert_eq!(Balances::reserved_balance(2), 100); + + // Drop 1 free_balance below ED + assert_ok!(Balances::transfer(Some(1).into(), 2, 1)); + // Check balance, the other 99 should move to reserved_balance + assert_eq!(Balances::free_balance(1), 0); + assert_eq!(Balances::reserved_balance(1), 199); + + // Reset accounts + assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 100, 100)); + assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 2, 100, 100)); + + // Drop 2 reserved_balance below ED + Balances::unreserve(&2, 1); + // Check balance, all 100 should move to free_balance + assert_eq!(Balances::free_balance(2), 200); + assert_eq!(Balances::reserved_balance(2), 0); + + // An account with both too little free and reserved is completely killed + assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 99, 99)); + // Check balance is 0 for everything + assert_eq!(Balances::free_balance(1), 0); + assert_eq!(Balances::reserved_balance(1), 0); + }); +}