Account for reserved balance in ensure_can_withdraw function

[RomarQ]

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 was 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]

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]

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]

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]

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.

[gui1117]

I don't think this is a correct assumption of the withdraw reason API.

WithdrawReasons::RESERVE says:

In order to reserve some funds for a later return or repatriation.

But it doesn't say the reserved amount will be on the same account.

So I think in this function we shouldn't add the reserved amount.
Then in can_reserve and reserve we can be more precise so we can create a new function which does add the amount to the reserved amount.

Then after this I will approve the PR.