From e311a94a6bbcfdf9d42bb9f50151017a8989b180 Mon Sep 17 00:00:00 2001 From: Rodrigo Quelhas <22591718+RomarQ@users.noreply.github.com> Date: Fri, 19 Sep 2025 13:17:42 +0100 Subject: [PATCH] Use `total balance (free + reserved)` when performing liquidity checks for a new reserve (#8108) # Description Solves: https://github.com/paritytech/polkadot-sdk/issues/8099 Based on the documentation and existing code, the usable balance is computed with the following formula: ```rs // If Fortitude == Polite let usable_balance = free - max(frozen - reserved, existential balance) ``` ### The problem: If an account's `free balance` is lower than `frozen balance`, no reserves will be allowed even though the `usable balance` is enough to cover the reserve, resulting in a `LiquidityRestrictions` error, which should not happen. ### Visual example of how `usable/spendable` balance works: ```bash |__total__________________________________| |__on_hold__|_____________free____________| |__________frozen___________| |__on_hold__|__ed__| |__untouchable__|__spendable__| ``` ## Integration No action is required, the changes only change existing code, it does not add or change any API. ## Review Notes From my understanding, the function `ensure_can_withdraw` is incorrect, and instead of checking that the new `free` balance is higher or equal to the `frozen` balance, it should make sure the `new free` balance is higher or equal to the `usable` balance. --------- Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> (cherry picked from commit 7c2642df6079b4e73d51fc62c41269cb5c288af2) --- prdoc/pr_8108.prdoc | 16 + substrate/frame/balances/src/impl_currency.rs | 33 +- .../balances/src/tests/currency_tests.rs | 55 ++- .../src/tests/fungible_and_currency.rs | 419 ++++++++++++++++++ substrate/frame/balances/src/tests/mod.rs | 1 + 5 files changed, 516 insertions(+), 8 deletions(-) create mode 100644 prdoc/pr_8108.prdoc create mode 100644 substrate/frame/balances/src/tests/fungible_and_currency.rs diff --git a/prdoc/pr_8108.prdoc b/prdoc/pr_8108.prdoc new file mode 100644 index 0000000000000..798c42c43bae4 --- /dev/null +++ b/prdoc/pr_8108.prdoc @@ -0,0 +1,16 @@ +title: "Fix `reserve` and `can_reserve` methods in `pallet-balances` to properly handle reserved balances" + +doc: +- audience: Runtime Dev + description: |- + Fixes a bug in `pallet-balances` where the `reserve` and `can_reserve` methods were incorrectly + preventing new reserves when an account's free balance was lower than its frozen balance, + even if the usable balance was sufficient after accounting for the reserved balance. +- audience: Runtime User + description: |- + Fixes a bug that was causing unexpected `LiquidityRestrictions` errors when attempting to + reserve funds. + +crates: +- name: pallet-balances + bump: patch \ No newline at end of file diff --git a/substrate/frame/balances/src/impl_currency.rs b/substrate/frame/balances/src/impl_currency.rs index c5c5b0a3d2dd2..f0558d26f94a5 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/substrate/frame/balances/src/impl_currency.rs @@ -535,6 +535,29 @@ where } } +/// Validates whether an account can create a reserve without violating +/// liquidity constraints. +/// +/// This method performs liquidity checks without modifying the account state. +fn ensure_can_reserve, I: 'static>( + who: &T::AccountId, + value: T::Balance, + check_existential_deposit: bool, +) -> DispatchResult { + let AccountData { free, .. } = Pallet::::account(who); + + // Early validation: Check sufficient free balance + let new_free_balance = free.checked_sub(&value).ok_or(Error::::InsufficientBalance)?; + + // Conditionally validate existential deposit preservation + if check_existential_deposit { + let existential_deposit = T::ExistentialDeposit::get(); + ensure!(new_free_balance >= existential_deposit, Error::::Expendability); + } + + Ok(()) +} + impl, I: 'static> ReservableCurrency for Pallet where T::Balance: MaybeSerializeDeserialize + Debug, @@ -546,11 +569,7 @@ where if value.is_zero() { return true } - Self::account(who).free.checked_sub(&value).map_or(false, |new_balance| { - new_balance >= T::ExistentialDeposit::get() && - Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance) - .is_ok() - }) + ensure_can_reserve::(who, value, true).is_ok() } fn reserved_balance(who: &T::AccountId) -> Self::Balance { @@ -570,7 +589,9 @@ where account.free.checked_sub(&value).ok_or(Error::::InsufficientBalance)?; account.reserved = account.reserved.checked_add(&value).ok_or(ArithmeticError::Overflow)?; - Self::ensure_can_withdraw(&who, value, WithdrawReasons::RESERVE, account.free) + + // Check if it is possible to reserve before trying to mutate the account + ensure_can_reserve::(who, value, false) })?; Self::deposit_event(Event::Reserved { who: who.clone(), amount: value }); diff --git a/substrate/frame/balances/src/tests/currency_tests.rs b/substrate/frame/balances/src/tests/currency_tests.rs index e4c1de7c02fa8..18c36b63b457f 100644 --- a/substrate/frame/balances/src/tests/currency_tests.rs +++ b/substrate/frame/balances/src/tests/currency_tests.rs @@ -258,7 +258,9 @@ fn lock_should_work_reserve() { >::transfer(&1, &2, 1, AllowDeath), TokenError::Frozen ); - assert_noop!(Balances::reserve(&1, 1), Error::::LiquidityRestrictions,); + // We can use frozen balance for reserves + assert_ok!(Balances::reserve(&1, 9)); + assert_noop!(Balances::reserve(&1, 1), DispatchError::ConsumerRemaining,); assert!(ChargeTransactionPayment::::validate_and_prepare( ChargeTransactionPayment::from(1), Some(1).into(), @@ -280,6 +282,47 @@ fn lock_should_work_reserve() { }); } +#[test] +fn reserve_should_work_for_frozen_balance() { + ExtBuilder::default() + .existential_deposit(1) + .monied(true) + .build_and_execute_with(|| { + // Check balances + let account = Balances::account(&1); + assert_eq!(account.free, 10); + assert_eq!(account.frozen, 0); + assert_eq!(account.reserved, 0); + + Balances::set_lock(ID_1, &1, 9, WithdrawReasons::RESERVE); + + let account = Balances::account(&1); + assert_eq!(account.free, 10); + assert_eq!(account.frozen, 9); + assert_eq!(account.reserved, 0); + + assert_ok!(Balances::reserve(&1, 5)); + + let account = Balances::account(&1); + assert_eq!(account.free, 5); + assert_eq!(account.frozen, 9); + assert_eq!(account.reserved, 5); + + assert_ok!(Balances::reserve(&1, 4)); + + let account = Balances::account(&1); + assert_eq!(account.free, 1); + assert_eq!(account.frozen, 9); + assert_eq!(account.reserved, 9); + + // Check usable balance + // usable_balance = free - max(frozen - reserved, ExistentialDeposit) + // 0 = 1 - max(9 - 9, 1) + let usable_balance = Balances::usable_balance(&1); + assert_eq!(usable_balance, 0); + }); +} + #[test] fn lock_should_work_tx_fee() { ExtBuilder::default() @@ -291,7 +334,9 @@ fn lock_should_work_tx_fee() { >::transfer(&1, &2, 1, AllowDeath), TokenError::Frozen ); - assert_noop!(Balances::reserve(&1, 1), Error::::LiquidityRestrictions,); + // We can use frozen balance for reserves + assert_ok!(Balances::reserve(&1, 9)); + assert_noop!(Balances::reserve(&1, 1), DispatchError::ConsumerRemaining,); assert!(ChargeTransactionPayment::::validate_and_prepare( ChargeTransactionPayment::from(1), Some(1).into(), @@ -1273,8 +1318,14 @@ 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); + let _ = Balances::deposit_creating(&3, 3); assert!(Balances::can_reserve(&1, 1) == Balances::reserve(&1, 1).is_ok()); assert!(Balances::can_reserve(&2, 1) == Balances::reserve(&2, 1).is_ok()); + + // Ensure that we can reserve as long (free + reserved) remains above + // the maximum of frozen balance. + Balances::set_lock(ID_1, &3, 2, WithdrawReasons::RESERVE); + assert_eq!(Balances::can_reserve(&3, 2), Balances::reserve(&3, 2).is_ok()); }); } diff --git a/substrate/frame/balances/src/tests/fungible_and_currency.rs b/substrate/frame/balances/src/tests/fungible_and_currency.rs new file mode 100644 index 0000000000000..e4666dc08f8ad --- /dev/null +++ b/substrate/frame/balances/src/tests/fungible_and_currency.rs @@ -0,0 +1,419 @@ +// 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. + +//! Test the behavior of a runtime when both `Fungible` and `Currency` traits are in use and are +//! being mixed. +//! +//! The primitives that we have and can mix are: +//! +//! * locks +//! * reserves +//! * holds +//! * freezes +//! +//! All permutations of which are: +//! +//! * Two primitives combined +//! * locks + reserves +//! * locks + holds +//! * locks + freezes +//! * reserves + holds +//! * reserves + freezes +//! * holds + freezes +//! +//! * Three primitives combined +//! * locks + reserves + holds +//! * locks + reserves + freezes +//! * locks + holds + freezes +//! * reserves + holds + freezes +//! +//! * All four primitives combined +//! * locks + reserves + holds + freezes +//! +//! For each test, after creating the primitive, we check: +//! +//! * The account data triplet. +//! * What `can_reserve` returns and where is the boundary. +//! * What `can_hold` returns and where is the boundary. + +use super::*; +use frame_support::traits::{ + fungible::{InspectHold, MutateFreeze, MutateHold}, + Currency, LockIdentifier, LockableCurrency, ReservableCurrency, WithdrawReasons, +}; + +fn subject() -> AccountId { + let subject = 1; + Balances::make_free_balance_be(&subject, 100); + subject +} + +const ID: LockIdentifier = *b"1 "; + +fn b(x: AccountId) -> (Balance, Balance, Balance) { + let a = get_test_account_data(x); + (a.free, a.reserved, a.frozen) +} + +fn ensure_max_reserve(who: AccountId, amount: Balance) { + assert!(!>::can_reserve(&who, amount.max(1) * 2)); + assert!(!>::can_reserve(&who, amount + 1)); + assert!(>::can_reserve(&who, amount)); + assert!(>::can_reserve(&who, amount.saturating_sub(1))); + assert!(>::can_reserve(&who, amount / 2)); +} + +fn ensure_max_hold(who: AccountId, amount: Balance) { + assert!(>::ensure_can_hold(&TestId::Foo, &who, amount.max(1) * 2) + .is_err()); + assert!(>::ensure_can_hold(&TestId::Foo, &who, amount + 1).is_err()); + assert!(>::ensure_can_hold(&TestId::Foo, &who, amount).is_ok()); + assert!(>::ensure_can_hold( + &TestId::Foo, + &who, + amount.saturating_sub(1) + ) + .is_ok()); + assert!(>::ensure_can_hold(&TestId::Foo, &who, amount / 2).is_ok()); +} + +// Two primitives combined + +#[test] +fn locks_and_reserves() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + >::set_lock(ID, &who, 50, WithdrawReasons::all()); + assert_eq!(b(who), (100, 0, 50)); + + // Can reserve up to 99 (leaving 1 for ED) + ensure_max_reserve(who, 99); + // Can hold up to 99 (leaving 1 for ED) + ensure_max_hold(who, 99); + + assert_ok!(>::reserve(&who, 30)); + assert_eq!(b(who), (70, 30, 50)); + + // Can hold or reserve up to 69 more (leaving 1 for ED) + let expected = 69; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + }); +} + +#[test] +fn locks_and_holds() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Lock 60 tokens + >::set_lock(ID, &who, 60, WithdrawReasons::all()); + assert_eq!(b(who), (100, 0, 60)); + + ensure_max_hold(who, 99); + ensure_max_reserve(who, 99); + + // Hold 40 tokens + assert_ok!(>::hold(&TestId::Foo, &who, 40)); + assert_eq!(b(who), (60, 40, 60)); + + // Can hold or reserve up to 59 more (leaving 1 for ED) + let expected = 59; + ensure_max_hold(who, expected); + ensure_max_reserve(who, expected); + }); +} + +#[test] +fn locks_and_freezes() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Lock 40 tokens + >::set_lock(ID, &who, 40, WithdrawReasons::all()); + assert_eq!(b(who), (100, 0, 40)); + + // Freeze 70 tokens + assert_ok!(>::set_freeze(&TestId::Foo, &who, 70)); + // Frozen takes the max of lock (40) and freeze (70) + assert_eq!(b(who), (100, 0, 70)); + + // Can hold or reserve up to 64 more (leaving 1 for ED) + let expected = 99; + ensure_max_hold(who, expected); + ensure_max_reserve(who, expected); + }); +} + +#[test] +fn reserves_and_holds() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Reserve 30 tokens + assert_ok!(>::reserve(&who, 30)); + assert_eq!(b(who), (70, 30, 0)); + ensure_max_reserve(who, 69); + ensure_max_hold(who, 69); + + // Hold 25 tokens (accumulates with reserve) + assert_ok!(>::hold(&TestId::Foo, &who, 25)); + assert_eq!(b(who), (45, 55, 0)); // reserved = 30 + 25 = 55 + + // Can hold or reserve up to 44 more (leaving 1 for ED) + let expected = 44; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + }); +} + +#[test] +fn reserves_and_freezes() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Reserve 25 tokens + assert_ok!(>::reserve(&who, 25)); + assert_eq!(b(who), (75, 25, 0)); + + // Freeze 80 tokens + assert_ok!(>::set_freeze(&TestId::Foo, &who, 80)); + assert_eq!(b(who), (75, 25, 80)); + + // Can hold or reserve up to 74 more (leaving 1 for ED) + let expected = 74; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + }); +} + +#[test] +fn holds_and_freezes() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Hold 35 tokens + assert_ok!(>::hold(&TestId::Foo, &who, 35)); + assert_eq!(b(who), (65, 35, 0)); + + // Can hold or reserve up to 64 more (leaving 1 for ED) + let expected = 64; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + + // Freeze 90 tokens + assert_ok!(>::set_freeze(&TestId::Foo, &who, 90)); + assert_eq!(b(who), (65, 35, 90)); + + // Can hold or reserve up to 64 more (leaving 1 for ED) + let expected = 64; + ensure_max_hold(who, expected); + ensure_max_reserve(who, expected); + }); +} + +// Three primitives combined + +#[test] +fn locks_reserves_and_holds() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Lock 60 tokens + >::set_lock(ID, &who, 60, WithdrawReasons::all()); + assert_eq!(b(who), (100, 0, 60)); + + // Reserve 20 tokens + assert_ok!(>::reserve(&who, 20)); + assert_eq!(b(who), (80, 20, 60)); + + // Can hold or reserve up to 79 more (leaving 1 for ED) + let expected = 79; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + + // Hold 15 tokens (accumulates with reserve) + assert_ok!(>::hold(&TestId::Foo, &who, 15)); + assert_eq!(b(who), (65, 35, 60)); // reserved = 20 + 15 = 35 + + // Can hold or reserve up to 64 more (leaving 1 for ED) + let expected = 64; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + }); +} + +#[test] +fn locks_reserves_and_freezes() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Lock 40 tokens + >::set_lock(ID, &who, 40, WithdrawReasons::all()); + assert_eq!(b(who), (100, 0, 40)); + + // Reserve 25 tokens + assert_ok!(>::reserve(&who, 25)); + assert_eq!(b(who), (75, 25, 40)); + + // Freeze 80 tokens (max of lock 40 and freeze 80) + assert_ok!(>::set_freeze(&TestId::Foo, &who, 80)); + assert_eq!(b(who), (75, 25, 80)); + + // Can hold or reserve up to 74 more (leaving 1 for ED) + let expected = 74; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + }); +} + +#[test] +fn locks_holds_and_freezes() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Lock 50 tokens + >::set_lock(ID, &who, 50, WithdrawReasons::all()); + assert_eq!(b(who), (100, 0, 50)); + + // Hold 30 tokens + assert_ok!(>::hold(&TestId::Foo, &who, 30)); + assert_eq!(b(who), (70, 30, 50)); + + // Freeze 75 tokens (max of lock 50 and freeze 75) + assert_ok!(>::set_freeze(&TestId::Foo, &who, 75)); + assert_eq!(b(who), (70, 30, 75)); + + // Can hold or reserve up to 69 more (leaving 1 for ED) + let expected = 69; + ensure_max_hold(who, expected); + ensure_max_reserve(who, expected); + }); +} + +#[test] +fn reserves_holds_and_freezes() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Reserve 20 tokens + assert_ok!(>::reserve(&who, 20)); + assert_eq!(b(who), (80, 20, 0)); + + // Can hold or reserve up to 79 more (leaving 1 for ED) + let expected = 79; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + + // Hold 25 tokens (accumulates with reserve) + assert_ok!(>::hold(&TestId::Foo, &who, 25)); + assert_eq!(b(who), (55, 45, 0)); // reserved = 20 + 25 = 45 + + // Can hold or reserve up to 54 more (leaving 1 for ED) + let expected = 54; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + + // Freeze 90 tokens + assert_ok!(>::set_freeze(&TestId::Foo, &who, 90)); + assert_eq!(b(who), (55, 45, 90)); + + // Can hold or reserve up to 54 more (leaving 1 for ED) + let expected = 54; + ensure_max_hold(who, expected); + ensure_max_reserve(who, expected); + }); +} + +// All four primitives combined + +#[test] +fn locks_reserves_holds_and_freezes() { + ExtBuilder::default() + .monied(false) + .existential_deposit(1) + .build_and_execute_with(|| { + let who = subject(); + + // Lock 40 tokens + >::set_lock(ID, &who, 40, WithdrawReasons::all()); + assert_eq!(b(who), (100, 0, 40)); + + // Can hold or reserve up to 99 more (leaving 1 for ED) + let expected = 99; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + + // Reserve 20 tokens + assert_ok!(>::reserve(&who, 20)); + assert_eq!(b(who), (80, 20, 40)); + + // Can hold or reserve up to 79 more (leaving 1 for ED) + let expected = 79; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + + // Hold 15 tokens (accumulates with reserve) + assert_ok!(>::hold(&TestId::Foo, &who, 15)); + assert_eq!(b(who), (65, 35, 40)); // reserved = 20 + 15 = 35 + + // Can hold or reserve up to 64 more (leaving 1 for ED) + let expected = 64; + ensure_max_reserve(who, expected); + ensure_max_hold(who, expected); + + // Freeze 85 tokens (max of lock 40 and freeze 85) + assert_ok!(>::set_freeze(&TestId::Foo, &who, 85)); + assert_eq!(b(who), (65, 35, 85)); + + // Can hold or reserve up to 64 more (leaving 1 for ED) + let expected = 64; + ensure_max_hold(who, expected); + ensure_max_reserve(who, expected); + }); +} diff --git a/substrate/frame/balances/src/tests/mod.rs b/substrate/frame/balances/src/tests/mod.rs index a46caf6e1e791..155f78884d122 100644 --- a/substrate/frame/balances/src/tests/mod.rs +++ b/substrate/frame/balances/src/tests/mod.rs @@ -49,6 +49,7 @@ use std::collections::BTreeSet; mod consumer_limit_tests; mod currency_tests; mod dispatchable_tests; +mod fungible_and_currency; mod fungible_conformance_tests; mod fungible_tests; mod general_tests;