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

[RomarQ]

Description

I am creating this issue to confirm if the behaviour reported below is a bug or not. If it is not a bug, the documentation should be updated with an example explaining it.

Recently, for moonbeam, we received a bug report where the user could not add a proxy, even though he had enough usable balance to pay for the deposit.

Based on the documentation, 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 the following error LiquidityRestrictions.

Based on the documentation, this should not happen.

Visual example of how usable balance works:

|__total__________________________________|
|__on_hold__|_____________free____________|
|__________frozen___________|
|__on_hold__|__ed__|
            |__untouchable__|__spendable__|

Based on the account example below, the user should be able to reserve an amount of 5 tokens.

free: 5
frozen: 10
reserved: 10

In my understanding, the function below is incorrect, and instead of checking that the new free balance is higher or equal to the frozen balance, it should make sure that amount is less or equal to the usable balance.

polkadot-sdk/substrate/frame/balances/src/impl_currency.rs

Lines 315 to 326 in f9fac6c

	fn ensure_can_withdraw(
	who: &T::AccountId,
	amount: T::Balance,
	_reasons: WithdrawReasons,
	new_balance: T::Balance,
	) -> DispatchResult {
	if amount.is_zero() {
	return Ok(())
	}
	ensure!(new_balance >= Self::account(who).frozen, Error::<T, I>::LiquidityRestrictions);
	Ok(())
	}

If it is confirmed to be a bug, I can work on the fix and open a pull request.

Suggested fix for function ensure_can_withdraw:

	fn ensure_can_withdraw(
		who: &T::AccountId,
		amount: T::Balance,
		reasons: WithdrawReasons,
		new_balance: T::Balance,
	) -> DispatchResult {
		if amount.is_zero() {
			return Ok(())
		}
		let account = Self::account(who);

		// Calculate the new reserved amount only if withdrawing for reservation.
		let updated_reserved = if matches!(reasons, WithdrawReasons::RESERVE) {
			account.reserved.saturating_add(amount)
		} else {
			account.reserved
		};

		// Frozen balance applies to total. Anything on hold therefore gets discounted from the
		// limit given by the freezes minus reserves.
		let untouchable = account
			.frozen
			.saturating_sub(updated_reserved)
			.max(T::ExistentialDeposit::get());
		ensure!(new_balance >= untouchable, Error::<T, I>::LiquidityRestrictions);
		Ok(())
	}

Steps to reproduce

1. Have an account with 20 tokens
2. Set a lock with 10 tokens
3. Reserve 10 tokens
4. Try to reserve 1 token // This will fail with LiquidityRestrictions
   4.1. (But transferring the spendable balance works)

[RomarQ]

@bkchr @kianenigma Is there anyone from parity that can have a look at this issue?

[gui1117]

Is reserve same as on_hold ?

In this graph:

|__total__________________________________|
|__on_hold__|_____________free____________|
|__________frozen___________|
|__on_hold__|__ed__|
            |__untouchable__|__spendable__|

This is for on_hold which is the fungible traits.

But this issue is talking about the reserve API, part of the Currency API, that is deprecated in favor of fungible.
I am not sure we should fix the old reserve API, also I am not sure it was actually the specification of the reserve API.

That said the fungible API might suffer the same issue, but it looks easier to fix.

[RomarQ]

Is reserve same as on_hold ?

In this graph:

|__total__________________________________|
|__on_hold__|_____________free____________|
|__________frozen___________|
|__on_hold__|__ed__|
            |__untouchable__|__spendable__|

This is for on_hold which is the fungible traits.

But this issue is talking about the reserve API, part of the Currency API, that is deprecated in favor of fungible. I am not sure we should fix the old reserve API, also I am not sure it was actually the specification of the reserve API.

That said the fungible API might suffer the same issue, but it looks easier to fix.

The Currency implementation was supposedly deprecated in paritytech/substrate#12951, but in reality it is still used by many pallets (including the pallet_proxy), as can be seen below:

@gui1117, I think all Currency methods should have a proper deprecation warning #[deprecated(note = "...")] and all pallets properly migrated to the fungible implementation.

[RomarQ]

Going back to the problem, it does not make sense that I can transfer a given balance amount to other account and not be able to use it for a new reserve.

[gui1117]

I see, I confirm the issue

[albertov19]

@gui1117 any updates? Thanks

[gui1117]

@gui1117 any updates? Thanks

I took another look, ensure_can_withdraw is only used internally in 3 method, so the scope is less big than I imagined at first. I put a review, and I will approve after this. But this will still not other people to approve.

#8108 (comment)

[RomarQ]

@gui1117 any updates? Thanks

I took another look, ensure_can_withdraw is only used internally in 3 method, so the scope is less big than I imagined at first. I put a review, and I will approve after this. But this will still not other people to approve.

#8108 (comment)

Hi @gui1117,

I have pushed some new changes and also improved the documentation explaining the change. The PR should now be small enough that can be easily reviewed.

[RomarQ]

The label I10-unconfirmed can be removed. This will be addressed once #8108 is merged