From 50ad9469b3cac6a3b374c518cf3830f553672af9 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Thu, 11 Jun 2020 13:48:35 +0200 Subject: [PATCH 1/9] almost works --- frame/balances/src/lib.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index b6dc4a11f0391..8700d88864cb3 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -217,6 +217,7 @@ impl, I: Instance> Subtrait for T { decl_event!( pub enum Event where ::AccountId, + ::Status, >::Balance { /// An account was created with some free balance. @@ -230,6 +231,11 @@ decl_event!( BalanceSet(AccountId, Balance, Balance), /// Some amount was deposited (e.g. for transaction fees). Deposit(AccountId, Balance), + /// Some balance was unreserved (moved from reserved to free). + Unreserved(AccountId, Balance), + /// Some balance was moved from the reserve of the first account to the second account. + /// Final argument indicates the destination balance type. + ReserveRepatriated(AccountId, AccountId, Balance, Status), } ); @@ -1166,6 +1172,7 @@ impl, I: Instance> ReservableCurrency for Module // defensive only: this can never fail since total issuance which is at least free+reserved // fits into the same data type. account.free = account.free.saturating_add(actual); + Self::deposit_event(RawEvent::Unreserved(who.clone(), actual.clone())); value - actual }) } @@ -1217,6 +1224,9 @@ impl, I: Instance> ReservableCurrency for Module Status::Reserved => to_account.reserved = to_account.reserved.checked_add(&actual).ok_or(Error::::Overflow)?, } from_account.reserved -= actual; + Self::deposit_event(RawEvent::ReserveRepatriated( + slashed.clone(), beneficiary.clone(), actual.clone(), status + )); Ok(value - actual) }) }) From 176c055484787cb1e6f5d15e84219a55053ea202 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Thu, 11 Jun 2020 14:01:58 +0200 Subject: [PATCH 2/9] add clone to BalanceStatus --- frame/balances/src/lib.rs | 5 +---- frame/support/src/traits.rs | 1 + 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 8700d88864cb3..b63004e08edf9 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -217,7 +217,6 @@ impl, I: Instance> Subtrait for T { decl_event!( pub enum Event where ::AccountId, - ::Status, >::Balance { /// An account was created with some free balance. @@ -1224,9 +1223,7 @@ impl, I: Instance> ReservableCurrency for Module Status::Reserved => to_account.reserved = to_account.reserved.checked_add(&actual).ok_or(Error::::Overflow)?, } from_account.reserved -= actual; - Self::deposit_event(RawEvent::ReserveRepatriated( - slashed.clone(), beneficiary.clone(), actual.clone(), status - )); + Self::deposit_event(RawEvent::ReserveRepatriated(slashed.clone(), beneficiary.clone(), actual.clone(), status)); Ok(value - actual) }) }) diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index df47def8702bb..e6c4b917d31c4 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -1006,6 +1006,7 @@ pub trait Currency { } /// Status of funds. +#[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, RuntimeDebug)] pub enum BalanceStatus { /// Funds are free, as corresponding to `free` item in Balances. Free, From 48529398108a05ab5adbdf0ed0627f62d8a19a12 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Thu, 11 Jun 2020 14:33:27 +0200 Subject: [PATCH 3/9] reserve event --- frame/balances/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index b63004e08edf9..f33a04222367e 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -230,6 +230,8 @@ decl_event!( BalanceSet(AccountId, Balance, Balance), /// Some amount was deposited (e.g. for transaction fees). Deposit(AccountId, Balance), + /// Some balance was reserved (moved from free to reserved). + Reserved(AccountId, Balance), /// Some balance was unreserved (moved from reserved to free). Unreserved(AccountId, Balance), /// Some balance was moved from the reserve of the first account to the second account. @@ -1155,6 +1157,7 @@ impl, I: Instance> ReservableCurrency for Module 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::deposit_event(RawEvent::Reserved(who.clone(), value.clone())); Self::ensure_can_withdraw(who, value, WithdrawReason::Reserve.into(), account.free) }) } From 49df6670bab63d7b61fe36543b52028f76ba3e61 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Thu, 11 Jun 2020 18:34:07 +0200 Subject: [PATCH 4/9] fix staking tests --- frame/staking/src/tests.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 8a7ae011c9134..8e2a496d52b7a 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -24,7 +24,7 @@ use sp_runtime::{ }; use sp_staking::offence::OffenceDetails; use frame_support::{ - assert_ok, assert_noop, StorageMap, + assert_ok, assert_noop, assert_err, StorageMap, traits::{Currency, ReservableCurrency, OnInitialize, OnFinalize}, }; use pallet_balances::Error as BalancesError; @@ -337,8 +337,8 @@ fn staking_should_work() { claimed_rewards: vec![0], }) ); - // e.g. it cannot spend more than 500 that it has free from the total 2000 - assert_noop!( + // e.g. it cannot reserve more than 500 that it has free from the total 2000 + assert_err!( Balances::reserve(&3, 501), BalancesError::::LiquidityRestrictions ); @@ -783,11 +783,10 @@ fn cannot_reserve_staked_balance() { assert_eq!(Balances::free_balance(11), 1000); // Confirm account 11 (via controller 10) is totally staked assert_eq!(Staking::eras_stakers(Staking::active_era().unwrap().index, 11).own, 1000); - // Confirm account 11 cannot transfer as a result - assert_noop!( - Balances::reserve(&11, 1), - BalancesError::::LiquidityRestrictions - ); + // Confirm account 11 cannot reserve as a result + assert_eq!(Balances::reserved_balance(11), 0); + let _ = Balances::reserve(&11, 1); + assert_eq!(Balances::reserved_balance(11), 0); // Give account 11 extra free balance let _ = Balances::make_free_balance_be(&11, 10000); From f6fc9aed2011379b64a5d899cc78dfcdc66c45ad Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Thu, 11 Jun 2020 19:27:14 +0200 Subject: [PATCH 5/9] fix balances tests --- frame/balances/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index c49a04ae56572..7ea849bc55208 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -168,7 +168,7 @@ macro_rules! decl_tests { >::transfer(&1, &2, 1, AllowDeath), Error::<$test, _>::LiquidityRestrictions ); - assert_noop!( + assert_err!( >::reserve(&1, 1), Error::<$test, _>::LiquidityRestrictions ); From b06fe9ad1941ddb21ae3614af1e1fdf6c9d35778 Mon Sep 17 00:00:00 2001 From: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Date: Thu, 11 Jun 2020 21:03:49 +0200 Subject: [PATCH 6/9] Update frame/balances/src/tests.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/balances/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 7ea849bc55208..5f4630fade0f7 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -170,7 +170,7 @@ macro_rules! decl_tests { ); assert_err!( >::reserve(&1, 1), - Error::<$test, _>::LiquidityRestrictions + Error::<$test, _>::LiquidityRestrictions, ); assert!( as SignedExtension>::pre_dispatch( ChargeTransactionPayment::from(1), From 861b1c5a4935d0bed4525d18e7106e30a27276e5 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 12 Jun 2020 13:59:45 +0200 Subject: [PATCH 7/9] restore tests and move event emission --- frame/balances/src/lib.rs | 18 +++++++----- frame/balances/src/tests.rs | 55 ++++++++++++++++++++++++++++++++++++- frame/staking/src/tests.rs | 11 ++++---- 3 files changed, 71 insertions(+), 13 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index f33a04222367e..c35dcfd203639 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -1157,9 +1157,11 @@ impl, I: Instance> ReservableCurrency for Module 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::deposit_event(RawEvent::Reserved(who.clone(), value.clone())); - Self::ensure_can_withdraw(who, value, WithdrawReason::Reserve.into(), account.free) - }) + Self::ensure_can_withdraw(&who, value.clone(), WithdrawReason::Reserve.into(), account.free) + })?; + + Self::deposit_event(RawEvent::Reserved(who.clone(), value)); + Ok(()) } /// Unreserve some funds, returning any amount that was unable to be unreserved. @@ -1168,15 +1170,17 @@ impl, I: Instance> ReservableCurrency for Module fn unreserve(who: &T::AccountId, value: Self::Balance) -> Self::Balance { if value.is_zero() { return Zero::zero() } - Self::mutate_account(who, |account| { + let actual = Self::mutate_account(who, |account| { let actual = cmp::min(account.reserved, value); account.reserved -= actual; // defensive only: this can never fail since total issuance which is at least free+reserved // fits into the same data type. account.free = account.free.saturating_add(actual); - Self::deposit_event(RawEvent::Unreserved(who.clone(), actual.clone())); - value - actual - }) + actual + }); + + Self::deposit_event(RawEvent::Unreserved(who.clone(), actual.clone())); + value - actual } /// Slash from reserved balance, returning the negative imbalance created, diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 7ea849bc55208..82837cf6ce187 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -168,7 +168,7 @@ macro_rules! decl_tests { >::transfer(&1, &2, 1, AllowDeath), Error::<$test, _>::LiquidityRestrictions ); - assert_err!( + assert_noop!( >::reserve(&1, 1), Error::<$test, _>::LiquidityRestrictions ); @@ -485,6 +485,17 @@ macro_rules! decl_tests { let _ = Balances::deposit_creating(&2, 1); assert_ok!(Balances::reserve(&1, 110)); assert_ok!(Balances::repatriate_reserved(&1, &2, 41, Status::Free), 0); + assert_eq!( + events(), + [ + Event::system(system::RawEvent::NewAccount(1)), + Event::balances(RawEvent::Endowed(1, 110)), + Event::system(system::RawEvent::NewAccount(2)), + Event::balances(RawEvent::Endowed(2, 1)), + Event::balances(RawEvent::Reserved(1, 110)), + Event::balances(RawEvent::ReserveRepatriated(1, 2, 41, Status::Free)), + ] + ); assert_eq!(Balances::reserved_balance(1), 69); assert_eq!(Balances::free_balance(1), 0); assert_eq!(Balances::reserved_balance(2), 0); @@ -683,6 +694,48 @@ macro_rules! decl_tests { }); } + #[test] + fn emit_events_with_reserve_and_unreserve() { + <$ext_builder>::default() + .build() + .execute_with(|| { + let _ = Balances::deposit_creating(&1, 100); + + System::set_block_number(2); + let _ = Balances::reserve(&1, 10); + + assert_eq!( + events(), + [ + Event::system(system::RawEvent::NewAccount(1)), + Event::balances(RawEvent::Endowed(1, 100)), + Event::balances(RawEvent::Reserved(1, 10)), + ] + ); + + System::set_block_number(3); + let _ = Balances::unreserve(&1, 5); + + assert_eq!( + events(), + [ + Event::balances(RawEvent::Unreserved(1, 5)), + ] + ); + + System::set_block_number(4); + let _ = Balances::unreserve(&1, 6); + + // should only unreserve 5 + assert_eq!( + events(), + [ + Event::balances(RawEvent::Unreserved(1, 5)), + ] + ); + }); + } + #[test] fn emit_events_with_existential_deposit() { <$ext_builder>::default() diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 8e2a496d52b7a..078f5e0a79927 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -24,7 +24,7 @@ use sp_runtime::{ }; use sp_staking::offence::OffenceDetails; use frame_support::{ - assert_ok, assert_noop, assert_err, StorageMap, + assert_ok, assert_noop, StorageMap, traits::{Currency, ReservableCurrency, OnInitialize, OnFinalize}, }; use pallet_balances::Error as BalancesError; @@ -338,7 +338,7 @@ fn staking_should_work() { }) ); // e.g. it cannot reserve more than 500 that it has free from the total 2000 - assert_err!( + assert_noop!( Balances::reserve(&3, 501), BalancesError::::LiquidityRestrictions ); @@ -784,9 +784,10 @@ fn cannot_reserve_staked_balance() { // Confirm account 11 (via controller 10) is totally staked assert_eq!(Staking::eras_stakers(Staking::active_era().unwrap().index, 11).own, 1000); // Confirm account 11 cannot reserve as a result - assert_eq!(Balances::reserved_balance(11), 0); - let _ = Balances::reserve(&11, 1); - assert_eq!(Balances::reserved_balance(11), 0); + assert_noop!( + Balances::reserve(&11, 1), + BalancesError::::LiquidityRestrictions, + ); // Give account 11 extra free balance let _ = Balances::make_free_balance_be(&11, 10000); From 556dcffc796b4f7c644ae7c8cebafbaf0dcc0cd5 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 12 Jun 2020 14:18:05 +0200 Subject: [PATCH 8/9] move repatriate reserved event outside of mutate_account --- frame/balances/src/lib.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index c35dcfd203639..48b2d425f1032 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -1221,7 +1221,7 @@ impl, I: Instance> ReservableCurrency for Module }; } - Self::try_mutate_account(beneficiary, |to_account, is_new| -> Result { + let actual = 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); @@ -1230,10 +1230,12 @@ impl, I: Instance> ReservableCurrency for Module Status::Reserved => to_account.reserved = to_account.reserved.checked_add(&actual).ok_or(Error::::Overflow)?, } from_account.reserved -= actual; - Self::deposit_event(RawEvent::ReserveRepatriated(slashed.clone(), beneficiary.clone(), actual.clone(), status)); - Ok(value - actual) + Ok(actual) }) - }) + })?; + + Self::deposit_event(RawEvent::ReserveRepatriated(slashed.clone(), beneficiary.clone(), actual, status)); + Ok(value - actual) } } From b400f761256f40808fc04ba96cc4d5e046005601 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Fri, 12 Jun 2020 14:48:26 +0200 Subject: [PATCH 9/9] clean up events in tests --- frame/balances/src/tests.rs | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 00e98ebc88f0f..2724291f14cdb 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -69,6 +69,10 @@ macro_rules! decl_tests { evt } + fn last_event() -> Event { + system::Module::::events().pop().expect("Event expected").event + } + #[test] fn basic_locking_should_work() { <$ext_builder>::default().existential_deposit(1).monied(true).build().execute_with(|| { @@ -486,15 +490,8 @@ macro_rules! decl_tests { assert_ok!(Balances::reserve(&1, 110)); assert_ok!(Balances::repatriate_reserved(&1, &2, 41, Status::Free), 0); assert_eq!( - events(), - [ - Event::system(system::RawEvent::NewAccount(1)), - Event::balances(RawEvent::Endowed(1, 110)), - Event::system(system::RawEvent::NewAccount(2)), - Event::balances(RawEvent::Endowed(2, 1)), - Event::balances(RawEvent::Reserved(1, 110)), - Event::balances(RawEvent::ReserveRepatriated(1, 2, 41, Status::Free)), - ] + last_event(), + Event::balances(RawEvent::ReserveRepatriated(1, 2, 41, Status::Free)), ); assert_eq!(Balances::reserved_balance(1), 69); assert_eq!(Balances::free_balance(1), 0); @@ -705,22 +702,16 @@ macro_rules! decl_tests { let _ = Balances::reserve(&1, 10); assert_eq!( - events(), - [ - Event::system(system::RawEvent::NewAccount(1)), - Event::balances(RawEvent::Endowed(1, 100)), - Event::balances(RawEvent::Reserved(1, 10)), - ] + last_event(), + Event::balances(RawEvent::Reserved(1, 10)), ); System::set_block_number(3); let _ = Balances::unreserve(&1, 5); assert_eq!( - events(), - [ - Event::balances(RawEvent::Unreserved(1, 5)), - ] + last_event(), + Event::balances(RawEvent::Unreserved(1, 5)), ); System::set_block_number(4); @@ -728,10 +719,8 @@ macro_rules! decl_tests { // should only unreserve 5 assert_eq!( - events(), - [ - Event::balances(RawEvent::Unreserved(1, 5)), - ] + last_event(), + Event::balances(RawEvent::Unreserved(1, 5)), ); }); }