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

Drop ED Requirement for Transaction Payment with Exchangeable Asset #310

Merged

Conversation

muharem
Copy link
Contributor

@muharem muharem commented May 14, 2024

Drop the Existential Deposit (ED) requirement for the asset amount exchangeable for the fee asset (DOT/KSM) during transaction payments.

Currently, every swap during transaction payment, processed with asset A for native asset, must be for an amount greater than the ED of a native asset if the user lacks a native asset account. Since fees are typically smaller, the current implementation necessitates additional swaps to meet the ED during pre_dispatch, with refunds for excess ED swap occurring during post_dispatch. Further details can be found here.

This setup presents an issue where a user is unable to transfer their entire balance and close the account. Instead, the user must transfer slightly less of asset A to ensure there is enough not only for the fee payment but also some extra to meet the ED requirement for their native account during pre_dispatch. In some cases during post_dispatch, the user will have the excess ED swapped back to asset A, while in other cases, it may not be sufficient to meet the ED requirement for asset A, leading it being left in the user's 'temporary' native asset account.
Example: https://assethub-polkadot.subscan.io/extrinsic/6204018-9?event=6204018-79

Given the importance of this scenario for CEX, I propose a solution that does not entail breaking changes to the pallets' API and open PR to the runtimes without waiting for new polkadot-sdk version. Additionally, I have opened a draft PR with these types in their respective pallets in FRAME, where they have been tested against existing tests for types implementing the same contract. PR - paritytech/polkadot-sdk#4455

Target implementation with breaking changes: paritytech/polkadot-sdk#4488

@muharem muharem changed the title Drop ED Requirements for Transaction Payment with Exchangeable Asset Drop ED Requirement for Transaction Payment with Exchangeable Asset May 14, 2024
@joepetrowski joepetrowski mentioned this pull request May 15, 2024
10 tasks
@antonkhvorov
Copy link

YES PLEASE

@xlc
Copy link
Contributor

xlc commented May 17, 2024

tests?

@muharem
Copy link
Contributor Author

muharem commented May 17, 2024

tests?

tests for these types are passing here - paritytech/polkadot-sdk#4455
it will take some time for me to more those tests to this PR, but that would be reasonable request.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Slightly concerned about the transactionality of the functions.
Having to manually un-do every action in an error path is tiresome. We could wrap them in a transactional layer, but not sure if that is so nice.

system-parachains/asset-hubs/asset-hub-kusama/src/impls.rs Outdated Show resolved Hide resolved
match T::Assets::settle(who, debt, Preservation::Expendable) {
Ok(dust) => ensure!(
dust.peek() == Zero::zero(),
InvalidTransaction::Payment
Copy link
Member

Choose a reason for hiding this comment

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

Debugging this will be fund, as it always returns InvalidTransaction::Payment 🙃 maybe some logs?

@muharem
Copy link
Contributor Author

muharem commented May 17, 2024

Slightly concerned about the transactionality of the functions. Having to manually un-do every action in an error path is tiresome. We could wrap them in a transactional layer, but not sure if that is so nice.

I did address your comments. Also if these functions errors, the extrinsic is invalid, these methods used by validate, pre_dispatch, and post_dispatch.

I have also updated the PR with tests, to have the exact same types as in this PR with the last changes, the tests are green (paritytech/polkadot-sdk#4455).

CHANGELOG.md Outdated Show resolved Hide resolved
@joepetrowski
Copy link
Contributor

/merge

@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@fellowship-merge-bot fellowship-merge-bot bot merged commit b6f99f2 into polkadot-fellows:main May 19, 2024
36 checks passed
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.

5 participants