From ed186519e7225f2032bb58263a6dcadb49187d6f Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 8 Feb 2022 22:04:04 +0100 Subject: [PATCH 1/4] bug found --- frame/balances/src/tests.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 638034d80cd52..cfc2cc371bcee 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -1239,5 +1239,17 @@ macro_rules! decl_tests { assert_eq!(Balances::free_balance(&3), 25); }); } + + #[test] + fn set_balance_handles_killing_account() { + <$ext_builder>::default().build().execute_with(|| { + let _ = Balances::deposit_creating(&1, 111); + assert_ok!(frame_system::Pallet::::inc_consumers(&1)); + assert_noop!( + Balances::set_balance(Origin::root(), 1, 0, 0), + DispatchError::ConsumerRemaining, + ); + }); + } } } From 2efb24e9095ea2b3a6cfdffd14676ef9cf3d0647 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 8 Feb 2022 22:32:41 +0100 Subject: [PATCH 2/4] fix logic --- frame/balances/src/lib.rs | 42 +++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 37114f385aa7f..62084dd64706b 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -314,25 +314,33 @@ pub mod pallet { let new_free = if wipeout { Zero::zero() } else { new_free }; let new_reserved = if wipeout { Zero::zero() } else { new_reserved }; - let (free, reserved) = Self::mutate_account(&who, |account| { - if new_free > account.free { - mem::drop(PositiveImbalance::::new(new_free - account.free)); - } else if new_free < account.free { - mem::drop(NegativeImbalance::::new(account.free - new_free)); - } - - if new_reserved > account.reserved { - mem::drop(PositiveImbalance::::new(new_reserved - account.reserved)); - } else if new_reserved < account.reserved { - mem::drop(NegativeImbalance::::new(account.reserved - new_reserved)); - } + // First we try to modify the account's balance to the forced balance. + let (old_free, old_reserved, new_free, new_reserved) = + Self::mutate_account(&who, |account| { + let old_free = account.free; + let old_reserved = account.reserved; + + account.free = new_free; + account.reserved = new_reserved; + + (old_free, old_reserved, new_free, new_reserved) + })?; + + // This will adjust the total issuance, which was not done by the `mutate_account` + // above. + if new_free > old_free { + mem::drop(PositiveImbalance::::new(new_free - old_free)); + } else if new_free < old_free { + mem::drop(NegativeImbalance::::new(old_free - new_free)); + } - account.free = new_free; - account.reserved = new_reserved; + if new_reserved > old_reserved { + mem::drop(PositiveImbalance::::new(new_reserved - old_reserved)); + } else if new_reserved < old_reserved { + mem::drop(NegativeImbalance::::new(old_reserved - new_reserved)); + } - (account.free, account.reserved) - })?; - Self::deposit_event(Event::BalanceSet { who, free, reserved }); + Self::deposit_event(Event::BalanceSet { who, free: new_free, reserved: new_reserved }); Ok(().into()) } From 4600baa39dd7cde4b74a1c77d093f406dcd2f82c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 8 Feb 2022 22:43:53 +0100 Subject: [PATCH 3/4] a little simpler --- frame/balances/src/lib.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 62084dd64706b..6bf37dfda037b 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -315,16 +315,15 @@ pub mod pallet { let new_reserved = if wipeout { Zero::zero() } else { new_reserved }; // First we try to modify the account's balance to the forced balance. - let (old_free, old_reserved, new_free, new_reserved) = - Self::mutate_account(&who, |account| { - let old_free = account.free; - let old_reserved = account.reserved; + let (old_free, old_reserved) = Self::mutate_account(&who, |account| { + let old_free = account.free; + let old_reserved = account.reserved; - account.free = new_free; - account.reserved = new_reserved; + account.free = new_free; + account.reserved = new_reserved; - (old_free, old_reserved, new_free, new_reserved) - })?; + (old_free, old_reserved) + })?; // This will adjust the total issuance, which was not done by the `mutate_account` // above. From 347f0158841fb10c92e0c41d5d408729274af7e9 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 9 Feb 2022 19:48:50 +0100 Subject: [PATCH 4/4] add test --- frame/balances/src/tests.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index cfc2cc371bcee..8f5470ae3cac2 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -1251,5 +1251,17 @@ macro_rules! decl_tests { ); }); } + + #[test] + fn set_balance_handles_total_issuance() { + <$ext_builder>::default().build().execute_with(|| { + let old_total_issuance = Balances::total_issuance(); + assert_ok!(Balances::set_balance(Origin::root(), 1337, 69, 42)); + assert_eq!(Balances::total_issuance(), old_total_issuance + 69 + 42); + assert_eq!(Balances::total_balance(&1337), 69 + 42); + assert_eq!(Balances::free_balance(&1337), 69); + assert_eq!(Balances::reserved_balance(&1337), 42); + }); + } } }