Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fungible/s decrease balance preserves account #1683

Closed
wants to merge 4 commits into from

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Sep 23, 2023

Ensure preservation is respected when decreasing balance through fangible/s::Unbalanced::decrease_balance.

At present, an account might be removed even if preservation is set to Preserve or Protect. Given the default implementation of decrease_balance, we can't pinpoint the exact constraint that wasn't satisfied (frozen or ED). As a result, we return the FundsUnavailable error.

There's a more extensive PR that aims to address the same problem: #1296. Due to its broader scope, I'm maintaining this one separately, in the event it can be merged sooner. The new tests introduced here will eventually be superseded by the conformance tests from the referenced PR.

Closes: #1698

@muharem muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Sep 23, 2023
@liamaharon
Copy link
Contributor

I have a fix and tests for what appear to be the same thing you're working on here: https://github.com/paritytech/polkadot-sdk/pull/1296/files#diff-e04f7799b91b4225b1923b49d8484495f7868868ab32ed78919f310ffe268fd9R190-R193

@muharem
Copy link
Contributor Author

muharem commented Sep 25, 2023

I have a fix and tests for what appear to be the same thing you're working on here: https://github.com/paritytech/polkadot-sdk/pull/1296/files#diff-e04f7799b91b4225b1923b49d8484495f7868868ab32ed78919f310ffe268fd9R190-R193

sure, lets work on your PR

@muharem muharem marked this pull request as ready for review September 25, 2023 10:26
@muharem muharem requested review from a team September 25, 2023 10:26
amount = amount.min(free);
match precision {
BestEffort => amount = amount.min(free),
Exact => ensure!(free >= amount, TokenError::FundsUnavailable),
}
let new_balance = old_balance.checked_sub(&amount).ok_or(TokenError::FundsUnavailable)?;
Copy link
Contributor

@juangirini juangirini Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need this check anymore, as amount <= free <= old_balance (by definition reducible_balance must be always <= than balance)

/// Get the maximum amount that `who` can withdraw/transfer successfully based on whether the
/// account should be kept alive (`preservation`) or whether we are willing to force the
/// reduction and potentially go below user-level restrictions on the minimum amount of the
/// account.
///
/// Always less than or equal to `balance()`.
fn reducible_balance(
who: &AccountId,
preservation: Preservation,
force: Fortitude,
) -> Self::Balance;

Same applies to fungibles

@muharem
Copy link
Contributor Author

muharem commented Jan 16, 2024

closing this PR, since #1296 is merged
thanks to @liamaharon

@muharem muharem closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fungible/s decrease balance should respect preservation argument
4 participants