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

ensure account of native token keeps alive after charge fee #1204

Merged
merged 1 commit into from
Jul 14, 2021

Conversation

wangjj9219
Copy link
Member

charge fee maybe fail even if the account has sufficient tokens for these reasons:

  1. match <T as Config>::Currency::withdraw(who, fee, reason, ExistenceRequirement::KeepAlive) {
    the account must keep alive after charge fee.

  2. if T::DEX::swap_with_exact_target(
    The balances::transfer will executed when swap native token and transfer requires the receiver is alive, otherwise the swap will fail.

So fix ensure_can_charge_fee to guarantee account will keep alive.

@wangjj9219 wangjj9219 requested a review from xlc July 13, 2021 11:13
Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

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

better to have some tests

let native_is_enough = <T as Config>::Currency::free_balance(who)
.checked_sub(&fee_and_alive_gap)
.map_or(false, |new_free_balance| {
<T as Config>::Currency::ensure_can_withdraw(who, fee, reason, new_free_balance).is_ok()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<T as Config>::Currency::ensure_can_withdraw(who, fee, reason, new_free_balance).is_ok()
<T as Config>::Currency::ensure_can_withdraw(who, fee, reason, fee_and_alive_gap).is_ok()

Copy link
Member

Choose a reason for hiding this comment

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

@xlc The old code seems to be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes we removed the last parameter

@xlc xlc merged commit 861d75d into master Jul 14, 2021
@xlc xlc deleted the fix-charge-fee branch July 14, 2021 01:33
syan095 pushed a commit that referenced this pull request Jul 14, 2021
* origin/master:
  update orml (#1206)
  ensure account of native token keeps alive after charge fee (#1204)
  bump version (#1205)
  Update event name mapping (#1203)
  update call filter to disable sudo and enable transfer & dex (#1202)
  refactor calculation of initial lp token (#1198)
  enable unsigned transaction for parachain-system (#1197)
  Configurable BaseXcmWeight. (#1200)
  Update release.md
  Bring back CurrencyIdConvert test. (#1199)
  refactor some calculation from saturating to checked (#1195)

# Conflicts:
#	orml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants