From 4fc7f6e6929dbfa4fe0555ae1dd0e246b735a86a Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 26 Apr 2023 15:26:07 +0100 Subject: [PATCH] Various minor fixes (#13945) * Fix: Incorrect implementation of can_reserve check * Fix: Incorrect migration of consumer counting for existing accounts with frozen amounts * Fix: Inconsistent implementation between assets can_deposit and new_account * Fixes * Fixes * Another fix * Update tests.rs * Update fungible_tests.rs * Use `can_accrue_consumers` in the body of `can_inc_consumer` --------- Co-authored-by: Keith Yeung Co-authored-by: Oliver Tale-Yazdi Co-authored-by: parity-processbot <> --- frame/assets/src/functions.rs | 2 +- frame/assets/src/tests.rs | 51 ++++++++++++++++++---- frame/balances/src/impl_currency.rs | 4 +- frame/balances/src/lib.rs | 2 +- frame/balances/src/tests/currency_tests.rs | 10 +++++ frame/balances/src/tests/fungible_tests.rs | 25 +++++++++++ frame/system/src/lib.rs | 20 +++++++-- 7 files changed, 98 insertions(+), 16 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index af9e269acf8bc..1e10e0066e851 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -138,7 +138,7 @@ impl, I: 'static> Pallet { if amount < details.min_balance { return DepositConsequence::BelowMinimum } - if !details.is_sufficient && !frame_system::Pallet::::can_inc_consumer(who) { + if !details.is_sufficient && !frame_system::Pallet::::can_accrue_consumers(who, 2) { return DepositConsequence::CannotCreate } if details.is_sufficient && details.sufficients.checked_add(1).is_none() { diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index afd224ad66642..b47fb6486cb27 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -21,7 +21,7 @@ use super::*; use crate::{mock::*, Error}; use frame_support::{ assert_noop, assert_ok, - traits::{fungibles::InspectEnumerable, Currency}, + traits::{fungibles::InspectEnumerable, tokens::Preservation::Protect, Currency}, }; use pallet_balances::Error as BalancesError; use sp_io::storage; @@ -33,6 +33,25 @@ fn asset_ids() -> Vec { s } +#[test] +fn transfer_should_never_burn() { + new_test_ext().execute_with(|| { + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, false, 1)); + Balances::make_free_balance_be(&1, 100); + Balances::make_free_balance_be(&2, 100); + + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); + assert_eq!(Assets::balance(0, 1), 100); + + while System::inc_consumers(&2).is_ok() {} + let _ = System::dec_consumers(&2); + // Exactly one consumer ref remaining. + + let _ = >::transfer(0, &1, &2, 50, Protect); + assert_eq!(Assets::balance(0, 1) + Assets::balance(0, 2), 100); + }); +} + #[test] fn basic_minting_should_work() { new_test_ext().execute_with(|| { @@ -57,10 +76,7 @@ fn minting_too_many_insufficient_assets_fails() { Balances::make_free_balance_be(&1, 100); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 1, 1, 100)); - assert_noop!( - Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), - Error::::UnavailableConsumer - ); + assert_noop!(Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), TokenError::CannotCreate); Balances::make_free_balance_be(&2, 1); assert_ok!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 100)); @@ -78,10 +94,7 @@ fn minting_insufficient_asset_with_deposit_should_work_when_consumers_exhausted( Balances::make_free_balance_be(&1, 100); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 1, 1, 100)); - assert_noop!( - Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), - Error::::UnavailableConsumer - ); + assert_noop!(Assets::mint(RuntimeOrigin::signed(1), 2, 1, 100), TokenError::CannotCreate); assert_ok!(Assets::touch(RuntimeOrigin::signed(1), 2)); assert_eq!(Balances::reserved_balance(&1), 10); @@ -1322,3 +1335,23 @@ fn asset_create_and_destroy_is_reverted_if_callback_fails() { ); }); } + +#[test] +fn multiple_transfer_alls_work_ok() { + new_test_ext().execute_with(|| { + // Only run PoC when the system pallet is enabled, since the underlying bug is in the + // system pallet it won't work with BalancesAccountStore + // Start with a balance of 100 + Balances::force_set_balance(RuntimeOrigin::root(), 1, 100).unwrap(); + // Emulate a sufficient, in reality this could be reached by transferring a sufficient + // asset to the account + assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1)); + assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); + // Spend the same balance multiple times + assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false)); + assert_ok!(Balances::transfer_all(RuntimeOrigin::signed(1), 1337, false)); + + assert_eq!(Balances::free_balance(&1), 0); + assert_eq!(Balances::free_balance(&1337), 100); + }); +} diff --git a/frame/balances/src/impl_currency.rs b/frame/balances/src/impl_currency.rs index ff8cc71d6224a..9f764a37b8b89 100644 --- a/frame/balances/src/impl_currency.rs +++ b/frame/balances/src/impl_currency.rs @@ -490,7 +490,9 @@ where return true } Self::account(who).free.checked_sub(&value).map_or(false, |new_balance| { - Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance).is_ok() + new_balance >= T::ExistentialDeposit::get() && + Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance) + .is_ok() }) } diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 6835d3c8148bb..c87b01d77bae5 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -788,7 +788,7 @@ pub mod pallet { return false } a.flags.set_new_logic(); - if !a.reserved.is_zero() || !a.frozen.is_zero() { + if !a.reserved.is_zero() && a.frozen.is_zero() { if system::Pallet::::providers(who) == 0 { // Gah!! We have no provider refs :( // This shouldn't practically happen, but we need a failsafe anyway: let's give diff --git a/frame/balances/src/tests/currency_tests.rs b/frame/balances/src/tests/currency_tests.rs index 034c92f65689b..e25b122c1fcf0 100644 --- a/frame/balances/src/tests/currency_tests.rs +++ b/frame/balances/src/tests/currency_tests.rs @@ -1180,6 +1180,16 @@ fn named_reserve_should_work() { }); } +#[test] +fn reserve_must_succeed_if_can_reserve_does() { + ExtBuilder::default().build_and_execute_with(|| { + let _ = Balances::deposit_creating(&1, 1); + let _ = Balances::deposit_creating(&2, 2); + assert!(Balances::can_reserve(&1, 1) == Balances::reserve(&1, 1).is_ok()); + assert!(Balances::can_reserve(&2, 1) == Balances::reserve(&2, 1).is_ok()); + }); +} + #[test] fn reserved_named_to_yourself_should_work() { ExtBuilder::default().build_and_execute_with(|| { diff --git a/frame/balances/src/tests/fungible_tests.rs b/frame/balances/src/tests/fungible_tests.rs index 185396019b13d..ab2606c53ff71 100644 --- a/frame/balances/src/tests/fungible_tests.rs +++ b/frame/balances/src/tests/fungible_tests.rs @@ -398,6 +398,31 @@ fn unholding_frees_hold_slot() { }); } +#[test] +fn sufficients_work_properly_with_reference_counting() { + ExtBuilder::default() + .existential_deposit(1) + .monied(true) + .build_and_execute_with(|| { + // Only run PoC when the system pallet is enabled, since the underlying bug is in the + // system pallet it won't work with BalancesAccountStore + if UseSystem::get() { + // Start with a balance of 100 + >::set_balance(&1, 100); + // Emulate a sufficient, in reality this could be reached by transferring a + // sufficient asset to the account + System::inc_sufficients(&1); + // Spend the same balance multiple times + assert_ok!(>::transfer(&1, &1337, 100, Expendable)); + assert_eq!(Balances::free_balance(&1), 0); + assert_noop!( + >::transfer(&1, &1337, 100, Expendable), + TokenError::FundsUnavailable + ); + } + }); +} + #[test] fn emit_events_with_changing_freezes() { ExtBuilder::default().build_and_execute_with(|| { diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index 88291c326edba..6bbebb870594c 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -1222,10 +1222,20 @@ impl Pallet { a.consumers == 0 || a.providers > 1 } - /// True if the account has at least one provider reference. - pub fn can_inc_consumer(who: &T::AccountId) -> bool { + /// True if the account has at least one provider reference and adding `amount` consumer + /// references would not take it above the the maximum. + pub fn can_accrue_consumers(who: &T::AccountId, amount: u32) -> bool { let a = Account::::get(who); - a.providers > 0 && a.consumers < T::MaxConsumers::max_consumers() + match a.consumers.checked_add(amount) { + Some(c) => a.providers > 0 && c <= T::MaxConsumers::max_consumers(), + None => false, + } + } + + /// True if the account has at least one provider reference and fewer consumer references than + /// the maximum. + pub fn can_inc_consumer(who: &T::AccountId) -> bool { + Self::can_accrue_consumers(who, 1) } /// Deposits an event into this block's event record. @@ -1679,8 +1689,10 @@ impl StoredMap for Pallet { let is_default = account.data == T::AccountData::default(); let mut some_data = if is_default { None } else { Some(account.data) }; let result = f(&mut some_data)?; - if Self::providers(k) > 0 { + if Self::providers(k) > 0 || Self::sufficients(k) > 0 { Account::::mutate(k, |a| a.data = some_data.unwrap_or_default()); + } else { + Account::::remove(k) } Ok(result) }