-
Notifications
You must be signed in to change notification settings - Fork 709
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
Handle existential deposit BelowMinimum
error
#3720
base: master
Are you sure you want to change the base?
Handle existential deposit BelowMinimum
error
#3720
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please write in the pr comment each way the method could fail, and the corresponding err you're returning in each case?
Would be nice to also have a conformance test covering each of these cases with a comment explaining the rationale.
@@ -146,7 +146,7 @@ where | |||
Preservation::Preserve, | |||
Fortitude::Polite, | |||
), | |||
Err(TokenError::FundsUnavailable.into()), | |||
Err(TokenError::BelowMinimum.into()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a conformance test for FundsUnavaliable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I will add one
if reducible < amount { | ||
if matches!(preservation, Preservation::Protect | Preservation::Preserve) { | ||
return Err(TokenError::BelowMinimum.into()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why to always return BelowMinimum here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see, preservation
is provided to ensure the balance does not fall below the existential deposit. Hence, if the reducible < amount
we want to throw Token::BelowMinimum
. There is a test case for this saying Decreasing the balance below the minimum when Precision::Exact should fail.
.
// Decreasing the balance below the minimum when Precision::Exact should fail.
let amount = 11.into();
assert_eq!(
T::decrease_balance(
&account_0,
amount,
Precision::Exact,
Preservation::Preserve,
Fortitude::Polite,
),
Err(TokenError::BelowMinimum.into()),
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Hold some funds in an account. If a hold for `reason` is already in place, then this
/// will increase it.
fn hold(reason: &Self::Reason, who: &AccountId, amount: Self::Balance) -> DispatchResult {
// NOTE: This doesn't change the total balance of the account so there's no need to
// check liquidity.
Self::ensure_can_hold(reason, who, amount)?;
// Should be infallible now, but we proceed softly anyway.
>>>> Self::decrease_balance(who, amount, Exact, Protect, Force)?;
Self::increase_balance_on_hold(reason, who, amount, BestEffort)?;
Self::done_hold(reason, who, amount);
Ok(())
}
Because method hold()
has the method decrease_balance
called with Preservation::Protect
, if it fails, it must throw TokenError::BelowMinimum
instead
Expand the error thrown on specific cases of
decrease_balance
methodTokeError::BelowMinimum
if existential deposit case happens. This happens whenPrecision::Exact
is provided andPreservation
isProtect
orProvided
. If the check fails,FundsUnavailable
should not be a right message as there might be a fund but it is below the existential deposit balance.Issue: #2240
polkadot address: 19nSqFQorfF2HxD3oBzWM3oCh4SaCRKWt1yvmgaPYGCo71J