Use `total balance (free + reserved)` when performing liquidity checks for a new reserve #8108

RomarQ · 2025-03-31T13:33:57Z

Description

Solves: #8099

Based on the documentation and existing code, the usable balance is computed with the following formula:

// 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:

|__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.

gui1117 · 2025-04-09T01:04:38Z

code looks good, but it is difficult to say if this is not breaking some other assumptions in the code elsewhere in the currency implementation.

Maybe @muharem you could have some opinion

RomarQ · 2025-04-09T07:41:31Z

code looks good, but it is difficult to say if this is not breaking some other assumptions in the code elsewhere in the currency implementation.

Maybe @muharem you could have some opinion

Thanks for having a look 👍

bkontur · 2025-04-09T08:26:27Z

code looks good, but it is difficult to say if this is not breaking some other assumptions in the code elsewhere in the currency implementation.

Maybe, we could catch some assumptions with fn try_state for pallet_balances.

This PR fixes Currency::ensure_can_withdraw (we are trying to deprecated Currency in favor of fungible traits), there is also fn can_withdraw( in the impl_fungible.rs - I don't know if it make sense to align behavior for both?

RomarQ · 2025-04-09T10:25:11Z

code looks good, but it is difficult to say if this is not breaking some other assumptions in the code elsewhere in the currency implementation.

Maybe, we could catch some assumptions with fn try_state for pallet_balances.

This PR fixes Currency::ensure_can_withdraw (we are trying to deprecated Currency in favor of fungible traits), there is also fn can_withdraw( in the impl_fungible.rs - I don't know if it make sense to align behavior for both?

The can_withdraw from impl_fungible.rs is only applicable for balance withdraws, not used for Holds. The deprecated Currency::ensure_can_withdraw is called for Reserves, where the operation can be distinguished by checking the WithdrawReasons. The method try_mutate_account also performs sanity checks that make sure we do not break account balance assumptions.

I think we should include this change, since there are multiple pallets (eg. pallet_proxy) still using impl_currency.rs traits.

substrate/frame/balances/src/impl_currency.rs

…umentation

This reverts commit e3a2c0d.

This reverts commit fa912c8.

prdoc/pr_8108.prdoc

sigurpol

That's a very neat and elegant solution imo 🚀

RomarQ · 2025-09-18T07:46:02Z

@kianenigma adapted your tests in e0b80f7 to validate that both traits now have the same behaviour against the same conditions/state.

substrate/frame/balances/src/tests/fungible_and_currency.rs

kianenigma

LGTM esp seeing the final fungible_and_currency tests.

sigurpol · 2025-09-18T08:32:25Z

I'll take care of backporting and bumping crates for KAHM. Before merging, @Gioyik can you please 🙏 ack this last version vs https://github.com/paritytech-secops/appsec_findings/issues/100 ? Thank you in advance!!!

ggwpez

okay now

…s for a new reserve (#8108) # Description Solves: #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 <[email protected]> (cherry picked from commit 7c2642d)

paritytech-release-backport-bot · 2025-09-19T13:03:14Z

Successfully created backport PR for unstable2507:

[unstable2507] Backport #8108 #9789

…s for a new reserve (#8108) # Description Solves: #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 <[email protected]> (cherry picked from commit 7c2642d)

paritytech-release-backport-bot · 2025-09-19T13:03:19Z

Successfully created backport PR for stable2509:

[stable2509] Backport #8108 #9790

Backport #8108 into `unstable2507` from RomarQ. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot.  Co-authored-by: Rodrigo Quelhas <[email protected]> Co-authored-by: Kian Paimani <[email protected]>

Backport #8108 into `stable2509` from RomarQ. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot.  Co-authored-by: Rodrigo Quelhas <[email protected]> Co-authored-by: Kian Paimani <[email protected]>

* Init balances e2e tests (wip) * Rework initial tests for clarity Most of them were written with recourse to Cursor, and were just not very useful. * Refactor `transfer_allow_death` test It scrutinizes the extrinsic's events and their inner data. * Recreate tests for all Polkadot/Kusama relay/SPs * Rework networks that run `transferAllowDeath` test Now, at the calling site of the test tree creation function, chains can signal whether their ED is too low to run the basic `transferAllowDeath` test. * Scrutinize more events in `transferAllowDeath` test * Test non-destructive `transfer_allow_death` * Test `force_transfer` with account reaping * Test to `force_transfer` reaping * Test `transfer_allow_death` below ED * Test `force_transfer` below ED * Test overtransferring account * Update comments and snapshots * Test `force_transfer` with insufficient funds * Test self `transfer_allow_death` of near-entire balance * Update snapshots * Update accounts tests after merge from `master` PR #384 required some additional fixes that auto-merge could not perform. * Update snapshots * Fund new test account on Bridge Hub * Revert change to Polkadot AH multisig suite name * Create test to liquidity restrictions bug See paritytech/polkadot-sdk#9560 and paritytech/polkadot-sdk#8108. TL;DR proxy and multisig pallets are using the old `Currency` trait and not the new fungible traits, it is possible for an operation that is permissible to an account at a given point in time and state to fail. See #401 * Test liquidity restrictions when creating proxy * Parametrize deposit-requiring action liquidity restriction test The `balances.LiquidityRestrictions` error can occur is different contexts, as multiple pallets still use the old `Currency::reserve` operation. This commit parametrizes the test to allow for different kinds of actions to be tested, not just proxy addition. * Extend liquidity restriction tests to cover more cases The test now combinatorially covers several cases: 1. Reserve action can vary between staking bond and nomination pool creation 2. Lock action is always vested transfer (for now) 3. Deposit action can vary between proxy creation, referendum submission and staking bond * Test manual reserve/lock in liquidity restriction tests This adds the possibility of manually set reserve/locks to the reserve/ lock actions used to combinatorially generate tests to liquidity restrictions. This is needed because some chains have no vesting or staking, but the behavior should still be tested. * Fix manual reserve/lock actions in liq. restriction tests Fees were not being accounted for correctly when performing checks on free/frozen balances. * Update snapshots and accounts E2E test tree call sites * Filter vesting as locking action on Asset Hubs ... while the AHM is pending - vesting operations are filtered, and thus cannot be used. * Add more comments to liquidity restriction test Especially the action interfaces. * Add more comments to liquidity restriction tests * Remove leftover debug code from self-transfer test * Fix Asset Hub test suites' use of relay chain An invalid argument was being passed to the `scheduleInlineCallWithOrigin` function: the base's chains block provider, instead of the relay chain's, which is always `Local`. * Snapshot skipped liquidity restriction tests ... instead of logging a message. Logs add some noise to CI test output. * Apply lint fixes * Update block numbers * Change lock identifier used * Remove unstable `Transfer` event from snapshots * Update snapshots * Simplify liquidity restriction tests The `Balance.locks` storage is no longer modified - Polkadot Relay is failing for an undetermined reason. * Use nonces when setting storage * Test `transfer_allow_death` with reserve * Add test to `transfer_all` with reserve * Add `force_transfer` with reserve test * Add origin check tests for gated extrinsics * Add tests to transfer functions with insufficient funds * Test `transfer_all` with `keepAlive = true` * Correct previous transfer all test snapshot * Test `transfer_all` with `keep_alive = false` * Correct snapshots of accounts tests * Test `transfer_keep_alive` with source left below ED * Split `transfer_keep_alive` tests ... into two separate tests: 1. one for chains with low ED (below a typical transfer fee) 2. one for chains with normal ED * Test `force_adjust_total_issuance` with zero delta * Test total issuance changes * Test `force_unreserve` no-op cases * Update accounts E2E test snapshots * Update liquidity restriction test snapshots for AHs * Test forceful unreservation of funds * Add more self-transfer tests * Add self-transfer test to `force_transfer` * Add tests to `force_set_balance` * Refactor `burn` tests into low/normal ED tests * Update snapshots with new `burn` tests * Test burning from account with consumer reference * Add test to burning of entire balance (or more than it) * Update snapshots to remove obsolete data * Improve documentation and `README`

RomarQ and others added 3 commits March 31, 2025 14:20

Account for reserved amount in ensure_can_withdraw function

6a0652b

Merge branch 'master' into rq/fix-reservable-balance-check

13575c1

add PRdoc

78a6bad

RomarQ marked this pull request as ready for review March 31, 2025 13:43

RomarQ requested a review from a team as a code owner March 31, 2025 13:43

RomarQ mentioned this pull request Mar 31, 2025

balances.LiquidityRestrictions on addProxy moonbeam-foundation/moonbeam#3242

Closed

RomarQ and others added 2 commits April 9, 2025 08:31

update error based on new heuristic

fa912c8

Merge branch 'master' into rq/fix-reservable-balance-check

81f4a3a

update error in test

e3a2c0d

gui1117 reviewed Apr 23, 2025

View reviewed changes

substrate/frame/balances/src/impl_currency.rs Outdated Show resolved Hide resolved

gui1117 mentioned this pull request Apr 23, 2025

Cannot reserve when (free balance) is lower than (frozen balance) #8099

Closed

RomarQ mentioned this pull request Sep 3, 2025

proxy: migrate from Currency to fungible traits #9560

Open

1 task

RomarQ and others added 6 commits September 7, 2025 09:48

Merge branch 'master' into rq/fix-reservable-balance-check

b7fb566

do not account existential deposit in ensure_can_withdraw improve doc…

ce6f6da

…umentation

Revert "update error in test"

99458a4

This reverts commit e3a2c0d.

Revert "update error based on new heuristic"

feb396f

This reverts commit fa912c8.

fix clippy warning

b5a50e6

fix test

075fd6b

seadanda approved these changes Sep 8, 2025

View reviewed changes

prdoc/pr_8108.prdoc Outdated Show resolved Hide resolved

improve prdoc

075cd99

sigurpol added the T2-pallets This PR/Issue is related to a particular pallet. label Sep 8, 2025

RomarQ and others added 3 commits September 8, 2025 14:53

apply suggestions

ae469cd

Merge branch 'master' into rq/fix-reservable-balance-check

f5756f9

Merge branch 'master' into rq/fix-reservable-balance-check

c0e1a82

sigurpol approved these changes Sep 8, 2025

View reviewed changes

improve code and comments

8968f60

RomarQ and others added 2 commits September 18, 2025 08:44

Test the behavior of a runtime when both Fungible and Currency traits

e0b80f7

Merge branch 'master' into rq/fix-reservable-balance-check

14d4932

kianenigma reviewed Sep 18, 2025

View reviewed changes

substrate/frame/balances/src/tests/fungible_and_currency.rs Show resolved Hide resolved

kianenigma approved these changes Sep 18, 2025

View reviewed changes

Improve checks in tests

69643c8

sigurpol requested a review from Gioyik September 18, 2025 08:34

ggwpez approved these changes Sep 18, 2025

View reviewed changes

RomarQ and others added 2 commits September 18, 2025 16:38

Merge branch 'master' into rq/fix-reservable-balance-check

24a1d95

Merge branch 'master' into rq/fix-reservable-balance-check

8fb3ac0

kianenigma enabled auto-merge September 19, 2025 08:22

kianenigma disabled auto-merge September 19, 2025 08:23

Gioyik approved these changes Sep 19, 2025

View reviewed changes

sigurpol added this pull request to the merge queue Sep 19, 2025

Merged via the queue into paritytech:master with commit 7c2642d Sep 19, 2025
264 of 267 checks passed

paritytech-release-backport-bot bot mentioned this pull request Sep 19, 2025

[unstable2507] Backport #8108 #9789

Merged

paritytech-release-backport-bot bot mentioned this pull request Sep 19, 2025

[stable2509] Backport #8108 #9790

Merged

rockbmb mentioned this pull request Sep 22, 2025

Create E2E test suite for accounts and balances open-web3-stack/polkadot-ecosystem-tests#377

Merged

17 tasks

sigurpol mentioned this pull request Sep 22, 2025

Bump pallet-balances to 43.0.1 polkadot-fellows/runtimes#920

Merged

This was referenced Sep 29, 2025

Refactor liquidity restriction tests to allow rolling updates open-web3-stack/polkadot-ecosystem-tests#417

Open

Refactor liquidity tests for future runtime upgrades open-web3-stack/polkadot-ecosystem-tests#418

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use `total balance (free + reserved)` when performing liquidity checks for a new reserve #8108

Use `total balance (free + reserved)` when performing liquidity checks for a new reserve #8108

RomarQ commented Mar 31, 2025 •

edited

Loading

Uh oh!

gui1117 commented Apr 9, 2025

Uh oh!

RomarQ commented Apr 9, 2025

Uh oh!

bkontur commented Apr 9, 2025

Uh oh!

RomarQ commented Apr 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

sigurpol left a comment

Uh oh!

RomarQ commented Sep 18, 2025

Uh oh!

Uh oh!

kianenigma left a comment

Uh oh!

sigurpol commented Sep 18, 2025

Uh oh!

ggwpez left a comment

Uh oh!

Uh oh!

paritytech-release-backport-bot bot commented Sep 19, 2025

Uh oh!

paritytech-release-backport-bot bot commented Sep 19, 2025

Uh oh!

Uh oh!

Use total balance (free + reserved) when performing liquidity checks for a new reserve #8108

Use total balance (free + reserved) when performing liquidity checks for a new reserve #8108

Conversation

RomarQ commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

The problem:

Visual example of how usable/spendable balance works:

Integration

Review Notes

Uh oh!

gui1117 commented Apr 9, 2025

Uh oh!

RomarQ commented Apr 9, 2025

Uh oh!

bkontur commented Apr 9, 2025

Uh oh!

RomarQ commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sigurpol left a comment

Choose a reason for hiding this comment

Uh oh!

RomarQ commented Sep 18, 2025

Uh oh!

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

sigurpol commented Sep 18, 2025

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paritytech-release-backport-bot bot commented Sep 19, 2025

Uh oh!

paritytech-release-backport-bot bot commented Sep 19, 2025

Uh oh!

Uh oh!

Use `total balance (free + reserved)` when performing liquidity checks for a new reserve #8108

Use `total balance (free + reserved)` when performing liquidity checks for a new reserve #8108

RomarQ commented Mar 31, 2025 •

edited

Loading

Visual example of how `usable/spendable` balance works:

RomarQ commented Apr 9, 2025 •

edited

Loading