diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index d5f9aab37bc95..225f7f662eed0 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -365,9 +365,6 @@ decl_storage! { /// The balance of an account. /// - /// NOTE: THIS MAY NEVER BE IN EXISTENCE AND YET HAVE A `total().is_zero()`. If the total - /// is ever zero, then the entry *MUST* be removed. - /// /// NOTE: This is only used in the case that this module is used to store balances. pub Account: map hasher(blake2_128_concat) T::AccountId => AccountData; @@ -384,10 +381,6 @@ decl_storage! { config(balances): Vec<(T::AccountId, T::Balance)>; // ^^ begin, length, amount liquid at genesis build(|config: &GenesisConfig| { - assert!( - >::ExistentialDeposit::get() > Zero::zero(), - "The existential deposit should be greater than zero." - ); for (_, balance) in &config.balances { assert!( *balance >= >::ExistentialDeposit::get(), @@ -609,7 +602,7 @@ impl, I: Instance> Module { who: &T::AccountId, f: impl FnOnce(&mut AccountData) -> R ) -> R { - Self::try_mutate_account(who, |a| -> Result { Ok(f(a)) }) + Self::try_mutate_account(who, |a, _| -> Result { Ok(f(a)) }) .expect("Error is infallible; qed") } @@ -624,13 +617,13 @@ impl, I: Instance> Module { /// the caller will do this. fn try_mutate_account( who: &T::AccountId, - f: impl FnOnce(&mut AccountData) -> Result + f: impl FnOnce(&mut AccountData, bool) -> Result ) -> Result { T::AccountStore::try_mutate_exists(who, |maybe_account| { + let is_new = maybe_account.is_none(); let mut account = maybe_account.take().unwrap_or_default(); - let was_zero = account.total().is_zero(); - f(&mut account).map(move |result| { - let maybe_endowed = if was_zero { Some(account.free) } else { None }; + f(&mut account, is_new).map(move |result| { + let maybe_endowed = if is_new { Some(account.free) } else { None }; *maybe_account = Self::post_mutation(who, account); (maybe_endowed, result) }) @@ -966,8 +959,8 @@ impl, I: Instance> Currency for Module where ) -> DispatchResult { if value.is_zero() || transactor == dest { return Ok(()) } - Self::try_mutate_account(dest, |to_account| -> DispatchResult { - Self::try_mutate_account(transactor, |from_account| -> DispatchResult { + Self::try_mutate_account(dest, |to_account, _| -> DispatchResult { + Self::try_mutate_account(transactor, |from_account, _| -> DispatchResult { from_account.free = from_account.free.checked_sub(&value) .ok_or(Error::::InsufficientBalance)?; @@ -1038,8 +1031,8 @@ impl, I: Instance> Currency for Module where ) -> Result { if value.is_zero() { return Ok(PositiveImbalance::zero()) } - Self::try_mutate_account(who, |account| -> Result { - ensure!(!account.total().is_zero(), Error::::DeadAccount); + Self::try_mutate_account(who, |account, is_new| -> Result { + ensure!(!is_new, Error::::DeadAccount); account.free = account.free.checked_add(&value).ok_or(Error::::Overflow)?; Ok(PositiveImbalance::new(value)) }) @@ -1057,10 +1050,10 @@ impl, I: Instance> Currency for Module where ) -> Self::PositiveImbalance { if value.is_zero() { return Self::PositiveImbalance::zero() } - Self::try_mutate_account(who, |account| -> Result { + Self::try_mutate_account(who, |account, is_new| -> Result { // bail if not yet created and this operation wouldn't be enough to create it. let ed = T::ExistentialDeposit::get(); - ensure!(value >= ed || !account.total().is_zero(), Self::PositiveImbalance::zero()); + ensure!(value >= ed || !is_new, Self::PositiveImbalance::zero()); // defensive only: overflow should never happen, however in case it does, then this // operation is a no-op. @@ -1081,7 +1074,7 @@ impl, I: Instance> Currency for Module where ) -> result::Result { if value.is_zero() { return Ok(NegativeImbalance::zero()); } - Self::try_mutate_account(who, |account| + Self::try_mutate_account(who, |account, _| -> Result { let new_free_account = account.free.checked_sub(&value) @@ -1105,7 +1098,7 @@ impl, I: Instance> Currency for Module where fn make_free_balance_be(who: &T::AccountId, value: Self::Balance) -> SignedImbalance { - Self::try_mutate_account(who, |account| + Self::try_mutate_account(who, |account, is_new| -> Result, ()> { let ed = T::ExistentialDeposit::get(); @@ -1116,7 +1109,7 @@ impl, I: Instance> Currency for Module where // equal and opposite cause (returned as an Imbalance), then in the // instance that there's no other accounts on the system at all, we might // underflow the issuance and our arithmetic will be off. - ensure!(value.saturating_add(account.reserved) >= ed || !account.total().is_zero(), ()); + ensure!(value.saturating_add(account.reserved) >= ed || !is_new, ()); let imbalance = if account.free <= value { SignedImbalance::Positive(PositiveImbalance::new(value - account.free)) @@ -1154,7 +1147,7 @@ impl, I: Instance> ReservableCurrency for Module fn reserve(who: &T::AccountId, value: Self::Balance) -> DispatchResult { if value.is_zero() { return Ok(()) } - Self::try_mutate_account(who, |account| -> DispatchResult { + Self::try_mutate_account(who, |account, _| -> DispatchResult { account.free = account.free.checked_sub(&value).ok_or(Error::::InsufficientBalance)?; account.reserved = account.reserved.checked_add(&value).ok_or(Error::::Overflow)?; Self::ensure_can_withdraw(who, value, WithdrawReason::Reserve.into(), account.free) @@ -1215,9 +1208,9 @@ impl, I: Instance> ReservableCurrency for Module }; } - Self::try_mutate_account(beneficiary, |to_account| -> Result { - ensure!(!to_account.total().is_zero(), Error::::DeadAccount); - Self::try_mutate_account(slashed, |from_account| -> Result { + Self::try_mutate_account(beneficiary, |to_account, is_new| -> Result { + ensure!(!is_new, Error::::DeadAccount); + Self::try_mutate_account(slashed, |from_account, _| -> Result { let actual = cmp::min(from_account.reserved, value); match status { Status::Free => to_account.free = to_account.free.checked_add(&actual).ok_or(Error::::Overflow)?, @@ -1237,7 +1230,14 @@ impl, I: Instance> ReservableCurrency for Module /// storage (which is the default in most examples and tests) then there's no need.** impl, I: Instance> OnKilledAccount for Module { fn on_killed_account(who: &T::AccountId) { - Account::::remove(who); + Account::::mutate_exists(who, |account| { + let total = account.as_ref().map(|acc| acc.total()).unwrap_or_default(); + if !total.is_zero() { + T::DustRemoval::on_unbalanced(NegativeImbalance::new(total)); + Self::deposit_event(RawEvent::DustLost(who.clone(), total)); + } + *account = None; + }); } } @@ -1311,7 +1311,7 @@ impl, I: Instance> IsDeadAccount for Module wher T::Balance: MaybeSerializeDeserialize + Debug { fn is_dead_account(who: &T::AccountId) -> bool { - // this should always be exactly equivalent to `Self::account(who).total().is_zero()` + // this should always be exactly equivalent to `Self::account(who).total().is_zero()` if ExistentialDeposit > 0 !T::AccountStore::is_explicit(who) } } diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 149b9f07d223b..9be64fc74493b 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -61,6 +61,14 @@ macro_rules! decl_tests { DispatchInfo { weight: w, ..Default::default() } } + fn events() -> Vec { + let evt = System::events().into_iter().map(|evt| evt.event).collect::>(); + + System::reset_events(); + + evt + } + #[test] fn basic_locking_should_work() { <$ext_builder>::default().existential_deposit(1).monied(true).build().execute_with(|| { @@ -674,5 +682,67 @@ macro_rules! decl_tests { assert_eq!(Balances::reserved_balance(1), 0); }); } + + #[test] + fn emit_events_with_existential_deposit() { + <$ext_builder>::default() + .existential_deposit(100) + .build() + .execute_with(|| { + assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 100, 0)); + + assert_eq!( + events(), + [ + Event::system(system::RawEvent::NewAccount(1)), + Event::balances(RawEvent::Endowed(1, 100)), + Event::balances(RawEvent::BalanceSet(1, 100, 0)), + ] + ); + + let _ = Balances::slash(&1, 1); + + assert_eq!( + events(), + [ + Event::balances(RawEvent::DustLost(1, 99)), + Event::system(system::RawEvent::KilledAccount(1)) + ] + ); + }); + } + + #[test] + fn emit_events_with_no_existential_deposit_suicide() { + <$ext_builder>::default() + .existential_deposit(0) + .build() + .execute_with(|| { + assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 100, 0)); + + assert_eq!( + events(), + [ + Event::system(system::RawEvent::NewAccount(1)), + Event::balances(RawEvent::Endowed(1, 100)), + Event::balances(RawEvent::BalanceSet(1, 100, 0)), + ] + ); + + let _ = Balances::slash(&1, 100); + + // no events + assert_eq!(events(), []); + + assert_ok!(System::suicide(Origin::signed(1))); + + assert_eq!( + events(), + [ + Event::system(system::RawEvent::KilledAccount(1)) + ] + ); + }); + } } } diff --git a/frame/balances/src/tests_composite.rs b/frame/balances/src/tests_composite.rs index 7b9ec1f91eade..42036e624e6a3 100644 --- a/frame/balances/src/tests_composite.rs +++ b/frame/balances/src/tests_composite.rs @@ -26,7 +26,7 @@ use sp_runtime::{ }; use sp_core::H256; use sp_io; -use frame_support::{impl_outer_origin, parameter_types}; +use frame_support::{impl_outer_origin, impl_outer_event, parameter_types}; use frame_support::traits::Get; use frame_support::weights::{Weight, DispatchInfo, IdentityFee}; use std::cell::RefCell; @@ -37,6 +37,17 @@ impl_outer_origin!{ pub enum Origin for Test {} } +mod balances { + pub use crate::Event; +} + +impl_outer_event! { + pub enum Event for Test { + system, + balances, + } +} + thread_local! { static EXISTENTIAL_DEPOSIT: RefCell = RefCell::new(0); } @@ -65,7 +76,7 @@ impl frame_system::Trait for Test { type AccountId = u64; type Lookup = IdentityLookup; type Header = Header; - type Event = (); + type Event = Event; type BlockHashCount = BlockHashCount; type MaximumBlockWeight = MaximumBlockWeight; type DbWeight = (); @@ -93,7 +104,7 @@ impl pallet_transaction_payment::Trait for Test { impl Trait for Test { type Balance = u64; type DustRemoval = (); - type Event = (); + type Event = Event; type ExistentialDeposit = ExistentialDeposit; type AccountStore = system::Module; } @@ -138,7 +149,10 @@ impl ExtBuilder { vec![] }, }.assimilate_storage(&mut t).unwrap(); - t.into() + + let mut ext = sp_io::TestExternalities::new(t); + ext.execute_with(|| System::set_block_number(1)); + ext } } diff --git a/frame/balances/src/tests_local.rs b/frame/balances/src/tests_local.rs index 9ff76839f4cb2..30143a6c7edd1 100644 --- a/frame/balances/src/tests_local.rs +++ b/frame/balances/src/tests_local.rs @@ -26,7 +26,7 @@ use sp_runtime::{ }; use sp_core::H256; use sp_io; -use frame_support::{impl_outer_origin, parameter_types}; +use frame_support::{impl_outer_origin, impl_outer_event, parameter_types}; use frame_support::traits::{Get, StorageMapShim}; use frame_support::weights::{Weight, DispatchInfo, IdentityFee}; use std::cell::RefCell; @@ -37,6 +37,17 @@ impl_outer_origin!{ pub enum Origin for Test {} } +mod balances { + pub use crate::Event; +} + +impl_outer_event! { + pub enum Event for Test { + system, + balances, + } +} + thread_local! { static EXISTENTIAL_DEPOSIT: RefCell = RefCell::new(0); } @@ -65,7 +76,7 @@ impl frame_system::Trait for Test { type AccountId = u64; type Lookup = IdentityLookup; type Header = Header; - type Event = (); + type Event = Event; type BlockHashCount = BlockHashCount; type MaximumBlockWeight = MaximumBlockWeight; type DbWeight = (); @@ -93,7 +104,7 @@ impl pallet_transaction_payment::Trait for Test { impl Trait for Test { type Balance = u64; type DustRemoval = (); - type Event = (); + type Event = Event; type ExistentialDeposit = ExistentialDeposit; type AccountStore = StorageMapShim< super::Account, @@ -146,8 +157,45 @@ impl ExtBuilder { vec![] }, }.assimilate_storage(&mut t).unwrap(); - t.into() + + let mut ext = sp_io::TestExternalities::new(t); + ext.execute_with(|| System::set_block_number(1)); + ext } } decl_tests!{ Test, ExtBuilder, EXISTENTIAL_DEPOSIT } + +#[test] +fn emit_events_with_no_existential_deposit_suicide_with_dust() { + ::default() + .existential_deposit(0) + .build() + .execute_with(|| { + assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 100, 0)); + + assert_eq!( + events(), + [ + Event::system(system::RawEvent::NewAccount(1)), + Event::balances(RawEvent::Endowed(1, 100)), + Event::balances(RawEvent::BalanceSet(1, 100, 0)), + ] + ); + + let _ = Balances::slash(&1, 99); + + // no events + assert_eq!(events(), []); + + assert_ok!(System::suicide(Origin::signed(1))); + + assert_eq!( + events(), + [ + Event::balances(RawEvent::DustLost(1, 1)), + Event::system(system::RawEvent::KilledAccount(1)) + ] + ); + }); +} diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 0b48ac7f410e8..05e703a5e8a6d 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -105,20 +105,23 @@ impl< fn get(k: &K) -> T { S::get(k) } fn is_explicit(k: &K) -> bool { S::contains_key(k) } fn insert(k: &K, t: T) { + let existed = S::contains_key(&k); S::insert(k, t); - if !S::contains_key(&k) { + if !existed { Created::happened(k); } } fn remove(k: &K) { - if S::contains_key(&k) { + let existed = S::contains_key(&k); + S::remove(k); + if existed { Removed::happened(&k); } - S::remove(k); } fn mutate(k: &K, f: impl FnOnce(&mut T) -> R) -> R { + let existed = S::contains_key(&k); let r = S::mutate(k, f); - if !S::contains_key(&k) { + if !existed { Created::happened(k); } r diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 2221b0591d1a8..74e71b8cc595d 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -742,12 +742,12 @@ decl_module! { /// No DB Read or Write operations because caller is already in overlay /// # #[weight = (10_000_000, DispatchClass::Operational)] - fn suicide(origin) { + pub fn suicide(origin) { let who = ensure_signed(origin)?; let account = Account::::get(&who); ensure!(account.refcount == 0, Error::::NonZeroRefCount); ensure!(account.data == T::AccountData::default(), Error::::NonDefaultComposite); - Account::::remove(who); + Self::kill_account(&who); } } } @@ -1131,6 +1131,15 @@ impl Module { AllExtrinsicsLen::put(len as u32); } + /// Reset events. Can be used as an alternative to + /// `initialize` for tests that don't need to bother with the other environment entries. + #[cfg(any(feature = "std", feature = "runtime-benchmarks", test))] + pub fn reset_events() { + >::kill(); + EventCount::kill(); + >::remove_all(); + } + /// Return the chain's current runtime version. pub fn runtime_version() -> RuntimeVersion { T::Version::get() } @@ -1222,8 +1231,8 @@ impl Module { "WARNING: Referenced account deleted. This is probably a bug." ); } - Module::::on_killed_account(who.clone()); } + Module::::on_killed_account(who.clone()); } /// Determine whether or not it is possible to update the code.