diff --git a/prdoc/pr_9176.prdoc b/prdoc/pr_9176.prdoc new file mode 100644 index 0000000000000..5ac4b8d3069a9 --- /dev/null +++ b/prdoc/pr_9176.prdoc @@ -0,0 +1,10 @@ +title: Allow locking to bump consumer without limits +doc: +- audience: Runtime Dev + description: |- + Locking is a system-level operation, and can only increment the consumer limit at most once. Therefore, it should use `inc_consumer_without_limits`. This behavior is optional, and is only used in the call path of `LockableCurrency`. Reserves, Holds and Freezes (and other operations like transfer etc.) have the ability to return `DispatchResult` and don't need this bypass. This is demonstrated in the unit tests added. +crates: +- name: pallet-balances + bump: major +- name: frame-support + bump: patch diff --git a/substrate/frame/balances/src/impl_currency.rs b/substrate/frame/balances/src/impl_currency.rs index f453b23420c40..86b70c5045aa5 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/substrate/frame/balances/src/impl_currency.rs @@ -363,6 +363,7 @@ where let result = match Self::try_mutate_account_handling_dust( who, + false, |account, _is_new| -> Result<(Self::NegativeImbalance, Self::Balance), DispatchError> { // Best value is the most amount we can slash following liveness rules. let ed = T::ExistentialDeposit::get(); @@ -400,6 +401,7 @@ where Self::try_mutate_account_handling_dust( who, + false, |account, is_new| -> Result { ensure!(!is_new, Error::::DeadAccount); account.free = account.free.checked_add(&value).ok_or(ArithmeticError::Overflow)?; @@ -425,6 +427,7 @@ where Self::try_mutate_account_handling_dust( who, + false, |account, is_new| -> Result { let ed = T::ExistentialDeposit::get(); ensure!(value >= ed || !is_new, Error::::ExistentialDeposit); @@ -458,6 +461,7 @@ where Self::try_mutate_account_handling_dust( who, + false, |account, _| -> Result { let new_free_account = account.free.checked_sub(&value).ok_or(Error::::InsufficientBalance)?; @@ -485,6 +489,7 @@ where ) -> SignedImbalance { Self::try_mutate_account_handling_dust( who, + false, |account, is_new| -> Result, DispatchError> { @@ -542,7 +547,7 @@ where return Ok(()) } - Self::try_mutate_account_handling_dust(who, |account, _| -> DispatchResult { + Self::try_mutate_account_handling_dust(who, false, |account, _| -> DispatchResult { account.free = account.free.checked_sub(&value).ok_or(Error::::InsufficientBalance)?; account.reserved = @@ -567,7 +572,7 @@ where return value } - let actual = match Self::mutate_account_handling_dust(who, |account| { + let actual = match Self::mutate_account_handling_dust(who, false, |account| { let actual = cmp::min(account.reserved, value); account.reserved -= actual; // defensive only: this can never fail since total issuance which is at least @@ -606,7 +611,7 @@ where // NOTE: `mutate_account` may fail if it attempts to reduce the balance to the point that an // account is attempted to be illegally destroyed. - match Self::mutate_account_handling_dust(who, |account| { + match Self::mutate_account_handling_dust(who, false, |account| { let actual = value.min(account.reserved); account.reserved.saturating_reduce(actual); diff --git a/substrate/frame/balances/src/impl_fungible.rs b/substrate/frame/balances/src/impl_fungible.rs index 4470c3cc9eb1a..55149d251f540 100644 --- a/substrate/frame/balances/src/impl_fungible.rs +++ b/substrate/frame/balances/src/impl_fungible.rs @@ -160,7 +160,7 @@ impl, I: 'static> fungible::Unbalanced for Pallet Result, DispatchError> { let max_reduction = >::reducible_balance(who, Expendable, Force); - let (result, maybe_dust) = Self::mutate_account(who, |account| -> DispatchResult { + let (result, maybe_dust) = Self::mutate_account(who, false, |account| -> DispatchResult { // Make sure the reduction (if there is one) is no more than the maximum allowed. let reduction = account.free.saturating_sub(amount); ensure!(reduction <= max_reduction, Error::::InsufficientBalance); @@ -278,10 +278,11 @@ impl, I: 'static> fungible::UnbalancedHold for Pallet new_account.reserved.checked_sub(&delta).ok_or(ArithmeticError::Underflow)? }; - let (result, maybe_dust) = Self::try_mutate_account(who, |a, _| -> DispatchResult { - *a = new_account; - Ok(()) - })?; + let (result, maybe_dust) = + Self::try_mutate_account(who, false, |a, _| -> DispatchResult { + *a = new_account; + Ok(()) + })?; debug_assert!( maybe_dust.is_none(), "Does not alter main balance; dust only happens when it is altered; qed" diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs index 7c558bf6acfed..82ab09ed3677b 100644 --- a/substrate/frame/balances/src/lib.rs +++ b/substrate/frame/balances/src/lib.rs @@ -397,6 +397,20 @@ pub mod pallet { Thawed { who: T::AccountId, amount: T::Balance }, /// The `TotalIssuance` was forcefully changed. TotalIssuanceForced { old: T::Balance, new: T::Balance }, + /// An unexpected/defensive event was triggered. + Unexpected(UnexpectedKind), + } + + /// Defensive/unexpected errors/events. + /// + /// In case of observation in explorers, report it as an issue in polkadot-sdk. + #[derive(Clone, Encode, Decode, DecodeWithMemTracking, PartialEq, TypeInfo, RuntimeDebug)] + pub enum UnexpectedKind { + /// Balance was altered/dusted during an operation that should have NOT done so. + BalanceUpdated, + /// Mutating the account failed unexpectedly. This might lead to storage items in + /// `Balances` and the underlying account in `System` to be out of sync. + FailedToMutateAccount, } #[pallet::error] @@ -593,26 +607,8 @@ pub mod pallet { } #[cfg(feature = "try-runtime")] - fn try_state(_n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { - Holds::::iter_keys().try_for_each(|k| { - if Holds::::decode_len(k).unwrap_or(0) > - T::RuntimeHoldReason::VARIANT_COUNT as usize - { - Err("Found `Hold` with too many elements") - } else { - Ok(()) - } - })?; - - Freezes::::iter_keys().try_for_each(|k| { - if Freezes::::decode_len(k).unwrap_or(0) > T::MaxFreezes::get() as usize { - Err("Found `Freeze` with too many elements") - } else { - Ok(()) - } - })?; - - Ok(()) + fn try_state(n: BlockNumberFor) -> Result<(), sp_runtime::TryRuntimeError> { + Self::do_try_state(n) } } @@ -778,7 +774,7 @@ pub mod pallet { let new_free = if wipeout { Zero::zero() } else { new_free }; // First we try to modify the account's balance to the forced balance. - let old_free = Self::mutate_account_handling_dust(&who, |account| { + let old_free = Self::mutate_account_handling_dust(&who, false, |account| { let old_free = account.free; account.free = new_free; old_free @@ -955,9 +951,10 @@ pub mod pallet { /// the caller will do this. pub(crate) fn mutate_account_handling_dust( who: &T::AccountId, + force_consumer_bump: bool, f: impl FnOnce(&mut AccountData) -> R, ) -> Result { - let (r, maybe_dust) = Self::mutate_account(who, f)?; + let (r, maybe_dust) = Self::mutate_account(who, force_consumer_bump, f)?; if let Some(dust) = maybe_dust { >::handle_raw_dust(dust); } @@ -977,9 +974,10 @@ pub mod pallet { /// the caller will do this. pub(crate) fn try_mutate_account_handling_dust>( who: &T::AccountId, + force_consumer_bump: bool, f: impl FnOnce(&mut AccountData, bool) -> Result, ) -> Result { - let (r, maybe_dust) = Self::try_mutate_account(who, f)?; + let (r, maybe_dust) = Self::try_mutate_account(who, force_consumer_bump, f)?; if let Some(dust) = maybe_dust { >::handle_raw_dust(dust); } @@ -998,11 +996,18 @@ pub mod pallet { /// /// NOTE: LOW-LEVEL: This will not attempt to maintain total issuance. It is expected that /// the caller will do this. + /// + /// NOTE: LOW-LEVEL: `force_consumer_bump` is mainly there to accomodate for locks, which + /// have no ability in their API to return an error, and therefore better force increment + /// the consumer, or else the system will be inconsistent. See `consumer_limits_tests`. pub(crate) fn mutate_account( who: &T::AccountId, + force_consumer_bump: bool, f: impl FnOnce(&mut AccountData) -> R, ) -> Result<(R, Option), DispatchError> { - Self::try_mutate_account(who, |a, _| -> Result { Ok(f(a)) }) + Self::try_mutate_account(who, force_consumer_bump, |a, _| -> Result { + Ok(f(a)) + }) } /// Returns `true` when `who` has some providers or `insecure_zero_ed` feature is disabled. @@ -1034,6 +1039,7 @@ pub mod pallet { /// the caller will do this. pub(crate) fn try_mutate_account>( who: &T::AccountId, + force_consumer_bump: bool, f: impl FnOnce(&mut AccountData, bool) -> Result, ) -> Result<(R, Option), E> { Self::ensure_upgraded(who); @@ -1057,7 +1063,12 @@ pub mod pallet { frame_system::Pallet::::dec_consumers(who); } if !did_consume && does_consume { - frame_system::Pallet::::inc_consumers(who)?; + if force_consumer_bump { + // If we are forcing a consumer bump, we do it without limit. + frame_system::Pallet::::inc_consumers_without_limit(who)?; + } else { + frame_system::Pallet::::inc_consumers(who)?; + } } if does_consume && frame_system::Pallet::::consumers(who) == 0 { // NOTE: This is a failsafe and should not happen for normal accounts. A normal @@ -1139,9 +1150,9 @@ pub mod pallet { let freezes = Freezes::::get(who); let mut prev_frozen = Zero::zero(); let mut after_frozen = Zero::zero(); - // No way this can fail since we do not alter the existential balances. - // TODO: Revisit this assumption. - let res = Self::mutate_account(who, |b| { + // We do not alter ED, so the account will not get dusted. Yet, consumer limit might be + // full, therefore we pass `true` into `mutate_account` to make sure this cannot fail + let res = Self::mutate_account(who, true, |b| { prev_frozen = b.frozen; b.frozen = Zero::zero(); for l in locks.iter() { @@ -1152,9 +1163,18 @@ pub mod pallet { } after_frozen = b.frozen; }); - debug_assert!(res.is_ok()); - if let Ok((_, maybe_dust)) = res { - debug_assert!(maybe_dust.is_none(), "Not altering main balance; qed"); + match res { + Ok((_, None)) => { + // expected -- all good. + }, + Ok((_, Some(_dust))) => { + Self::deposit_event(Event::Unexpected(UnexpectedKind::BalanceUpdated)); + defensive!("caused unexpected dusting/balance update."); + }, + _ => { + Self::deposit_event(Event::Unexpected(UnexpectedKind::FailedToMutateAccount)); + defensive!("errored in mutate_account"); + }, } match locks.is_empty() { @@ -1178,7 +1198,7 @@ pub mod pallet { ) -> DispatchResult { let mut prev_frozen = Zero::zero(); let mut after_frozen = Zero::zero(); - let (_, maybe_dust) = Self::mutate_account(who, |b| { + let (_, maybe_dust) = Self::mutate_account(who, false, |b| { prev_frozen = b.frozen; b.frozen = Zero::zero(); for l in Locks::::get(who).iter() { @@ -1189,7 +1209,10 @@ pub mod pallet { } after_frozen = b.frozen; })?; - debug_assert!(maybe_dust.is_none(), "Not altering main balance; qed"); + if maybe_dust.is_some() { + Self::deposit_event(Event::Unexpected(UnexpectedKind::BalanceUpdated)); + defensive!("caused unexpected dusting/balance update."); + } if freezes.is_empty() { Freezes::::remove(who); } else { @@ -1240,9 +1263,10 @@ pub mod pallet { let ((_, maybe_dust_1), maybe_dust_2) = Self::try_mutate_account( beneficiary, + false, |to_account, is_new| -> Result<((), Option), DispatchError> { ensure!(!is_new, Error::::DeadAccount); - Self::try_mutate_account(slashed, |from_account, _| -> DispatchResult { + Self::try_mutate_account(slashed, false, |from_account, _| -> DispatchResult { match status { Status::Free => to_account.free = to_account @@ -1305,11 +1329,83 @@ pub mod pallet { .expect(&format!("Failed to decode public key from pair: {:?}", pair.public())); // Set the balance for the generated account. - Self::mutate_account_handling_dust(&who, |account| { + Self::mutate_account_handling_dust(&who, false, |account| { account.free = balance; }) .expect(&format!("Failed to add account to keystore: {:?}", who)); } } } + + #[cfg(any(test, feature = "try-runtime"))] + impl, I: 'static> Pallet { + pub(crate) fn do_try_state( + _n: BlockNumberFor, + ) -> Result<(), sp_runtime::TryRuntimeError> { + Self::hold_and_freeze_count()?; + Self::account_frozen_greater_than_locks()?; + Self::account_frozen_greater_than_freezes()?; + Ok(()) + } + + fn hold_and_freeze_count() -> Result<(), sp_runtime::TryRuntimeError> { + Holds::::iter_keys().try_for_each(|k| { + if Holds::::decode_len(k).unwrap_or(0) > + T::RuntimeHoldReason::VARIANT_COUNT as usize + { + Err("Found `Hold` with too many elements") + } else { + Ok(()) + } + })?; + + Freezes::::iter_keys().try_for_each(|k| { + if Freezes::::decode_len(k).unwrap_or(0) > T::MaxFreezes::get() as usize { + Err("Found `Freeze` with too many elements") + } else { + Ok(()) + } + })?; + + Ok(()) + } + + fn account_frozen_greater_than_locks() -> Result<(), sp_runtime::TryRuntimeError> { + Locks::::iter().try_for_each(|(who, locks)| { + let max_locks = locks.iter().map(|l| l.amount).max().unwrap_or_default(); + let frozen = T::AccountStore::get(&who).frozen; + if max_locks > frozen { + log::warn!( + target: crate::LOG_TARGET, + "Maximum lock of {:?} ({:?}) is greater than the frozen balance {:?}", + who, + max_locks, + frozen + ); + Err("bad locks".into()) + } else { + Ok(()) + } + }) + } + + fn account_frozen_greater_than_freezes() -> Result<(), sp_runtime::TryRuntimeError> { + Freezes::::iter().try_for_each(|(who, freezes)| { + let max_locks = freezes.iter().map(|l| l.amount).max().unwrap_or_default(); + let frozen = T::AccountStore::get(&who).frozen; + if max_locks > frozen { + log::warn!( + target: crate::LOG_TARGET, + "Maximum freeze of {:?} ({:?}) is greater than the frozen balance {:?}", + who, + max_locks, + frozen + ); + Err("bad freezes".into()) + } else { + Ok(()) + } + }) + } + } } diff --git a/substrate/frame/balances/src/tests/consumer_limit_tests.rs b/substrate/frame/balances/src/tests/consumer_limit_tests.rs new file mode 100644 index 0000000000000..b4aa36aac74fb --- /dev/null +++ b/substrate/frame/balances/src/tests/consumer_limit_tests.rs @@ -0,0 +1,153 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Tests for consumer limit behavior in balance locks. + +use super::*; +use frame_support::traits::{ + fungible::{InspectFreeze, MutateFreeze, MutateHold}, + Currency, Get, LockIdentifier, LockableCurrency, ReservableCurrency, WithdrawReasons, +}; + +const ID_1: LockIdentifier = *b"1 "; + +#[test] +fn lock_behavior_when_consumer_limit_fully_exhausted() { + ExtBuilder::default() + .existential_deposit(1) + .monied(true) + .build() + .execute_with(|| { + // Account 1 starts with 100 balance + Balances::make_free_balance_be(&1, 100); + assert_eq!(System::providers(&1), 1); + assert_eq!(System::consumers(&1), 0); + + // Fill up all consumer refs. + // Note: asset-pallets prevents all the consumers to be filled and leaves one untouched. + // But other operations in the runtime, notably `uniques::set_accept_ownership` might + // overrule it. + let max_consumers: u32 = ::MaxConsumers::get(); + for _ in 0..max_consumers { + assert_ok!(System::inc_consumers(&1)); + } + assert_eq!(System::consumers(&1), max_consumers); + + // We cannot manually increment consumers beyond the limit + assert_noop!(System::inc_consumers(&1), DispatchError::TooManyConsumers); + + // Although without limits it would work + frame_support::hypothetically!({ + assert_ok!(System::inc_consumers_without_limit(&1)); + }); + + // Now try to set a lock - this will still work because we use + // `inc_consumers_without_limit` in `update_lock`. + Balances::set_lock(ID_1, &1, 20, WithdrawReasons::all()); + assert_eq!(Balances::locks(&1).len(), 1); + assert_eq!(Balances::locks(&1)[0].amount, 20); + + // frozen amount is also updated + assert_eq!(get_test_account_data(1).frozen, 20); + + // now this account has 1 more consumer reference for the lock + assert_eq!(System::consumers(&1), max_consumers + 1); + + // And this account cannot transfer any funds out. + assert_noop!( + Balances::transfer_allow_death(frame_system::RawOrigin::Signed(1).into(), 2, 90), + DispatchError::Token(TokenError::Frozen) + ); + }); +} + +#[test] +fn freeze_behavior_when_consumer_limit_fully_exhausted() { + ExtBuilder::default() + .existential_deposit(1) + .monied(true) + .build() + .execute_with(|| { + // Account 1 starts with 100 balance + Balances::make_free_balance_be(&1, 100); + + // Fill up all consumer refs. + let max_consumers: u32 = ::MaxConsumers::get(); + for _ in 0..max_consumers { + assert_ok!(System::inc_consumers(&1)); + } + assert_eq!(System::consumers(&1), max_consumers); + + // Now try to set a freeze - this will FAIL because freezes don't force consumer + // increment and we've exhausted the consumer limit. + assert_noop!( + Balances::set_freeze(&TestId::Foo, &1, 20), + DispatchError::TooManyConsumers + ); + + // Verify no freeze was created + assert_eq!(Balances::balance_frozen(&TestId::Foo, &1), 0); + // frozen amount is not updated + assert_eq!(get_test_account_data(1).frozen, 0); + }); +} + +#[test] +fn hold_behavior_when_consumer_limit_fully_exhausted() { + ExtBuilder::default() + .existential_deposit(1) + .monied(true) + .build() + .execute_with(|| { + // Account 1 starts with 100 balance + Balances::make_free_balance_be(&1, 100); + + // Fill up all consumer refs. + let max_consumers: u32 = ::MaxConsumers::get(); + for _ in 0..max_consumers { + assert_ok!(System::inc_consumers(&1)); + } + assert_eq!(System::consumers(&1), max_consumers); + + // Hold is similar to freeze -- it will successfully fail and report an error. + // Note: we use `assert_err` instead of `assert_noop` as this is not a dispatchable -- + // when this is executed as a part of any transaction, it will revert. + assert_err!(Balances::hold(&TestId::Foo, &1, 50), DispatchError::TooManyConsumers); + }); +} + +#[test] +fn reserve_behavior_when_consumer_limit_fully_exhausted() { + ExtBuilder::default() + .existential_deposit(1) + .monied(true) + .build() + .execute_with(|| { + // Account 1 starts with 100 balance + Balances::make_free_balance_be(&1, 100); + + // Fill up all 16 consumer refs. + let max_consumers: u32 = ::MaxConsumers::get(); + for _ in 0..max_consumers { + assert_ok!(System::inc_consumers(&1)); + } + assert_eq!(System::consumers(&1), max_consumers); + + // Reserve is similar to freeze -- it will successfully fail and report an error. + assert_noop!(Balances::reserve(&1, 20), DispatchError::TooManyConsumers); + }); +} diff --git a/substrate/frame/balances/src/tests/currency_tests.rs b/substrate/frame/balances/src/tests/currency_tests.rs index 0e5d7ccb46dee..018a47ad56134 100644 --- a/substrate/frame/balances/src/tests/currency_tests.rs +++ b/substrate/frame/balances/src/tests/currency_tests.rs @@ -463,7 +463,7 @@ fn deducting_balance_should_work() { fn refunding_balance_should_work() { ExtBuilder::default().build_and_execute_with(|| { let _ = Balances::deposit_creating(&1, 42); - assert_ok!(Balances::mutate_account(&1, |a| a.reserved = 69)); + assert_ok!(Balances::mutate_account(&1, false, |a| a.reserved = 69)); Balances::unreserve(&1, 69); assert_eq!(Balances::free_balance(1), 111); assert_eq!(Balances::reserved_balance(1), 0); @@ -1383,22 +1383,22 @@ fn freezing_and_locking_should_work() { assert_eq!(System::consumers(&1), 1); // Frozen and locked balances update correctly. - assert_eq!(Balances::account(&1).frozen, 5); + assert_eq!(get_test_account_data(1).frozen, 5); assert_ok!(>::set_freeze(&TestId::Foo, &1, 6)); - assert_eq!(Balances::account(&1).frozen, 6); + assert_eq!(get_test_account_data(1).frozen, 6); assert_ok!(>::set_freeze(&TestId::Foo, &1, 4)); - assert_eq!(Balances::account(&1).frozen, 5); + assert_eq!(get_test_account_data(1).frozen, 5); Balances::set_lock(ID_1, &1, 3, WithdrawReasons::all()); - assert_eq!(Balances::account(&1).frozen, 4); + assert_eq!(get_test_account_data(1).frozen, 4); Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all()); - assert_eq!(Balances::account(&1).frozen, 5); + assert_eq!(get_test_account_data(1).frozen, 5); // Locks update correctly. Balances::remove_lock(ID_1, &1); - assert_eq!(Balances::account(&1).frozen, 4); + assert_eq!(get_test_account_data(1).frozen, 4); assert_eq!(System::consumers(&1), 1); assert_ok!(>::set_freeze(&TestId::Foo, &1, 0)); - assert_eq!(Balances::account(&1).frozen, 0); + assert_eq!(get_test_account_data(1).frozen, 0); assert_eq!(System::consumers(&1), 0); }); } diff --git a/substrate/frame/balances/src/tests/dispatchable_tests.rs b/substrate/frame/balances/src/tests/dispatchable_tests.rs index 77e44202e9822..a42fd6b13ea97 100644 --- a/substrate/frame/balances/src/tests/dispatchable_tests.rs +++ b/substrate/frame/balances/src/tests/dispatchable_tests.rs @@ -214,11 +214,11 @@ fn upgrade_accounts_should_work() { Ok(()) } )); - assert!(!Balances::account(&7).flags.is_new_logic()); + assert!(!get_test_account_data(7).flags.is_new_logic()); assert_eq!(System::providers(&7), 1); assert_eq!(System::consumers(&7), 0); assert_ok!(Balances::upgrade_accounts(Some(1).into(), vec![7])); - assert!(Balances::account(&7).flags.is_new_logic()); + assert!(get_test_account_data(7).flags.is_new_logic()); assert_eq!(System::providers(&7), 1); assert_eq!(System::consumers(&7), 1); diff --git a/substrate/frame/balances/src/tests/fungible_tests.rs b/substrate/frame/balances/src/tests/fungible_tests.rs index 4a8b213c645e0..4726448a10aa1 100644 --- a/substrate/frame/balances/src/tests/fungible_tests.rs +++ b/substrate/frame/balances/src/tests/fungible_tests.rs @@ -227,11 +227,11 @@ fn freezing_and_holds_should_overlap() { .build_and_execute_with(|| { assert_ok!(Balances::set_freeze(&TestId::Foo, &1, 10)); assert_ok!(Balances::hold(&TestId::Foo, &1, 9)); - assert_eq!(Balances::account(&1).free, 1); + assert_eq!(get_test_account_data(1).free, 1); assert_eq!(System::consumers(&1), 1); - assert_eq!(Balances::account(&1).free, 1); - assert_eq!(Balances::account(&1).frozen, 10); - assert_eq!(Balances::account(&1).reserved, 9); + assert_eq!(get_test_account_data(1).free, 1); + assert_eq!(get_test_account_data(1).frozen, 10); + assert_eq!(get_test_account_data(1).reserved, 9); assert_eq!(Balances::total_balance_on_hold(&1), 9); }); } @@ -305,7 +305,7 @@ fn thaw_should_work() { assert_ok!(Balances::thaw(&TestId::Foo, &1)); assert_eq!(System::consumers(&1), 0); assert_eq!(Balances::balance_frozen(&TestId::Foo, &1), 0); - assert_eq!(Balances::account(&1).frozen, 0); + assert_eq!(get_test_account_data(1).frozen, 0); assert_ok!(>::transfer(&1, &2, 10, Expendable)); }); } @@ -320,7 +320,7 @@ fn set_freeze_zero_should_work() { assert_ok!(Balances::set_freeze(&TestId::Foo, &1, 0)); assert_eq!(System::consumers(&1), 0); assert_eq!(Balances::balance_frozen(&TestId::Foo, &1), 0); - assert_eq!(Balances::account(&1).frozen, 0); + assert_eq!(get_test_account_data(1).frozen, 0); assert_ok!(>::transfer(&1, &2, 10, Expendable)); }); } @@ -349,7 +349,7 @@ fn extend_freeze_should_work() { .build_and_execute_with(|| { assert_ok!(Balances::set_freeze(&TestId::Foo, &1, 5)); assert_ok!(Balances::extend_freeze(&TestId::Foo, &1, 10)); - assert_eq!(Balances::account(&1).frozen, 10); + assert_eq!(get_test_account_data(1).frozen, 10); assert_eq!(Balances::balance_frozen(&TestId::Foo, &1), 10); assert_noop!( >::transfer(&1, &2, 1, Expendable), @@ -483,16 +483,16 @@ fn withdraw_precision_exact_works() { .monied(true) .build_and_execute_with(|| { assert_ok!(Balances::set_freeze(&TestId::Foo, &1, 10)); - assert_eq!(Balances::account(&1).free, 10); - assert_eq!(Balances::account(&1).frozen, 10); + assert_eq!(get_test_account_data(1).free, 10); + assert_eq!(get_test_account_data(1).frozen, 10); // `BestEffort` will not reduce anything assert_ok!(>::withdraw( &1, 5, BestEffort, Preserve, Polite )); - assert_eq!(Balances::account(&1).free, 10); - assert_eq!(Balances::account(&1).frozen, 10); + assert_eq!(get_test_account_data(1).free, 10); + assert_eq!(get_test_account_data(1).frozen, 10); assert_noop!( >::withdraw(&1, 5, Exact, Preserve, Polite), diff --git a/substrate/frame/balances/src/tests/general_tests.rs b/substrate/frame/balances/src/tests/general_tests.rs index a855fae5616af..606feedb95332 100644 --- a/substrate/frame/balances/src/tests/general_tests.rs +++ b/substrate/frame/balances/src/tests/general_tests.rs @@ -19,7 +19,9 @@ use crate::{ system::AccountInfo, - tests::{ensure_ti_valid, Balances, ExtBuilder, System, Test, TestId, UseSystem}, + tests::{ + ensure_ti_valid, get_test_account, Balances, ExtBuilder, System, Test, TestId, UseSystem, + }, AccountData, ExtraFlags, TotalIssuance, }; use frame_support::{ @@ -50,7 +52,7 @@ fn regression_historic_acc_does_not_evaporate_reserve() { System::dec_consumers(&alice); assert_eq!( - System::account(&alice), + get_test_account(alice), AccountInfo { data: AccountData { free: 90, @@ -119,7 +121,7 @@ fn try_state_works() { traits::{Get, Hooks, VariantCount}, }; - ExtBuilder::default().build_and_execute_with(|| { + ExtBuilder::default().auto_try_state(false).build_and_execute_with(|| { storage::unhashed::put( &Holds::::hashed_key_for(1), &vec![0u8; ::RuntimeHoldReason::VARIANT_COUNT as usize + 1], @@ -129,7 +131,7 @@ fn try_state_works() { .contains("Found `Hold` with too many elements")); }); - ExtBuilder::default().build_and_execute_with(|| { + ExtBuilder::default().auto_try_state(false).build_and_execute_with(|| { let max_freezes: u32 = ::MaxFreezes::get(); storage::unhashed::put( diff --git a/substrate/frame/balances/src/tests/mod.rs b/substrate/frame/balances/src/tests/mod.rs index d27d997582cf5..a46caf6e1e791 100644 --- a/substrate/frame/balances/src/tests/mod.rs +++ b/substrate/frame/balances/src/tests/mod.rs @@ -46,6 +46,7 @@ use sp_runtime::{ }; use std::collections::BTreeSet; +mod consumer_limit_tests; mod currency_tests; mod dispatchable_tests; mod fungible_conformance_tests; @@ -79,6 +80,9 @@ impl VariantCount for TestId { const VARIANT_COUNT: u32 = 3; } +pub(crate) type AccountId = ::AccountId; +pub(crate) type Balance = ::Balance; + frame_support::construct_runtime!( pub enum Test { System: frame_system, @@ -154,6 +158,11 @@ impl ExtBuilder { self.dust_trap = Some(account); self } + #[cfg(feature = "try-runtime")] + pub fn auto_try_state(self, auto_try_state: bool) -> Self { + AutoTryState::set(auto_try_state); + self + } pub fn set_associated_consts(&self) { DUST_TRAP_TARGET.with(|v| v.replace(self.dust_trap)); EXISTENTIAL_DEPOSIT.with(|v| v.replace(self.existential_deposit)); @@ -189,9 +198,19 @@ impl ExtBuilder { pub fn build_and_execute_with(self, f: impl Fn()) { let other = self.clone(); UseSystem::set(false); - other.build().execute_with(|| f()); + other.build().execute_with(|| { + f(); + if AutoTryState::get() { + Balances::do_try_state(System::block_number()).unwrap(); + } + }); UseSystem::set(true); - self.build().execute_with(|| f()); + self.build().execute_with(|| { + f(); + if AutoTryState::get() { + Balances::do_try_state(System::block_number()).unwrap(); + } + }); } } @@ -215,6 +234,7 @@ impl OnUnbalanced> for DustTrap { parameter_types! { pub static UseSystem: bool = false; + pub static AutoTryState: bool = true; } type BalancesAccountStore = StorageMapShim, u64, super::AccountData>; @@ -349,3 +369,24 @@ fn check_whitelist() { // Total Issuance assert!(whitelist.contains("c2261276cc9d1f8598ea4b6a74b15c2f57c875e4cff74148e4628f264b974c80")); } + +/// This pallet runs tests twice, once with system as `type AccountStore` and once this pallet. This +/// function will return the right value based on the `UseSystem` flag. +pub(crate) fn get_test_account_data(who: AccountId) -> AccountData { + if UseSystem::get() { + >::get(&who) + } else { + >::get(&who) + } +} + +/// Same as `get_test_account_data`, but returns a `frame_system::AccountInfo` with the data filled +/// in. +pub(crate) fn get_test_account( + who: AccountId, +) -> frame_system::AccountInfo> { + let mut system_account = frame_system::Account::::get(&who); + let account_data = get_test_account_data(who); + system_account.data = account_data; + system_account +} diff --git a/substrate/frame/support/src/traits/tokens/currency/lockable.rs b/substrate/frame/support/src/traits/tokens/currency/lockable.rs index 4ec45c908e688..7fe86b28193fe 100644 --- a/substrate/frame/support/src/traits/tokens/currency/lockable.rs +++ b/substrate/frame/support/src/traits/tokens/currency/lockable.rs @@ -25,6 +25,9 @@ use crate::{dispatch::DispatchResult, traits::misc::Get}; pub type LockIdentifier = [u8; 8]; /// A currency whose accounts can have liquidity restrictions. +/// +/// Note: Consider using [`crate::traits::fungible::MutateFreeze`] (and family) as it returns a +/// `Result`, and is therefore much safer to use. pub trait LockableCurrency: Currency { /// The quantity used to denote time; usually just a `BlockNumber`. type Moment;