Tx Payment: drop ED requirements for tx payments with exchangeable asset #4488

muharem · 2024-05-16T15:30:42Z

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

This achieved by using SwapCredit implementation of swap, which works with imbalances and does not require a temporary balance account within the transaction payment.

Problem

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.

…310) 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](https://github.com/paritytech/polkadot-sdk/blob/115c2477eb287df55107cd95594100ba395ed239/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs#L115). 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 --------- Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

paritytech-cicd-pr · 2024-05-24T09:26:15Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6280279

muharem · 2024-05-24T10:02:37Z

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs

+		OUF::on_unbalanced(fee);
+		OUT::on_unbalanced(tip);


we can make this as in transaction payment pallet (OU::on_unbalanceds(vec![fee, tip])), if #4564 pr get merged

ggwpez

Am not that deep into the code, but looks good.

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/lib.rs

ggwpez · 2024-05-24T17:36:20Z

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs

+			F::total_balance(asset_id.clone(), who).is_zero()
+		{
+			// Nothing to refund or the account was removed be the dispatched function.
+			(initial_asset_consumed, fee_paid)


It could also be that the account is still alive but just does not have any balances of that asset?
But its probably fine - otherwise it would need some more complicated logic to check that the refunded amount is above ED.

right. also current implementation for only a native asset (pallet-transaction-payment) also follows this logic and asserts it with a test. there might be other reasons. for example if you transfer all and wanna your account to be removed, you do not wanna any refund back.
may be @kianenigma can add more to this.

Do we want to keep the same assumption in the case where the asset is sufficient?

@georgepisaltu which assumption? the transfer all and reap account is real use case.

The assumption that the account was removed. I understand the reap account is a real use case, I was talking about @ggwpez 's situation where you maybe consumed all the asset. It would get messy with the ED calculation, but I think we don't care about that if the asset is sufficient and we have some of it to refund, right? I'm asking if the intended behavior is to ignore this "sufficient" case as with the current impl.

Yes, system account or any other can be still alive.
The intended behavior is to have no refund if a dispatched call results the asset account to be removed, regardless of the asset type.

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

georgepisaltu

Looks good overall, just a couple of comments

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs

georgepisaltu · 2024-05-28T14:07:14Z

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs

+			F::total_balance(asset_id.clone(), who).is_zero()
+		{
+			// Nothing to refund or the account was removed be the dispatched function.
+			(initial_asset_consumed, fee_paid)


Do we want to keep the same assumption in the case where the asset is sufficient?

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/tests.rs

georgepisaltu

LGTM

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/tests.rs

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/lib.rs

Make `on_unbalanceds` work with `fungibles` `imbalances`. The `fungibles` `imbalances` cannot be handled by the default implementation of `on_unbalanceds` from the `OnUnbalanced` trait. This is because the `fungibles` `imbalances` types do not implement the `Imbalance` trait (and cannot with its current semantics). The `on_unbalanceds` function requires only the `merge` function for the imbalance type. In this PR, we provide the `TryMerge` trait, which can be implemented by all imbalance types and make `OnUnbalanced` require it instead `Imbalance`. ### Migration for `OnUnbalanced` trait implementations: In case if you have a custom implementation of `on_unbalanceds` trait function, remove it's `<B>` type argument. ### Migration for custom imbalance types: If you have your own imbalance types implementations, implement the `TryMerge` trait for it introduced with this update. The applicability of the `on_unbalanceds` function to fungibles imbalances is useful in cases like - [link](https://github.com/paritytech/polkadot-sdk/blob/3a8e675e9f6f283514c00c14d3d1d90ed5bf59c0/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs#L267) from #4488. --------- Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

…et-payment

…4564) Make `on_unbalanceds` work with `fungibles` `imbalances`. The `fungibles` `imbalances` cannot be handled by the default implementation of `on_unbalanceds` from the `OnUnbalanced` trait. This is because the `fungibles` `imbalances` types do not implement the `Imbalance` trait (and cannot with its current semantics). The `on_unbalanceds` function requires only the `merge` function for the imbalance type. In this PR, we provide the `TryMerge` trait, which can be implemented by all imbalance types and make `OnUnbalanced` require it instead `Imbalance`. ### Migration for `OnUnbalanced` trait implementations: In case if you have a custom implementation of `on_unbalanceds` trait function, remove it's `<B>` type argument. ### Migration for custom imbalance types: If you have your own imbalance types implementations, implement the `TryMerge` trait for it introduced with this update. The applicability of the `on_unbalanceds` function to fungibles imbalances is useful in cases like - [link](https://github.com/paritytech/polkadot-sdk/blob/3a8e675e9f6f283514c00c14d3d1d90ed5bf59c0/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs#L267) from paritytech#4488. --------- Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

…set (paritytech#4488) Drop the Existential Deposit (ED) requirement for the asset amount exchangeable for the fee asset (eg. DOT/KSM) during transaction payments. This achieved by using `SwapCredit` implementation of swap, which works with imbalances and does not require a temporary balance account within the transaction payment. ### Problem 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](https://github.com/paritytech/polkadot-sdk/blob/115c2477eb287df55107cd95594100ba395ed239/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs#L115). 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. --------- Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* master: (27 commits) Bridges improved tests and nits (#5128) Fix misleading comment about RewardHandler in epm config (#3095) Introduce a workflow updating the wishlist leaderboards (#5085) membership: Restructure pallet into separate files (#4536) Fix after ring-proof api change (#5126) Bump paritytech/review-bot from 2.4.0 to 2.5.0 (#5057) Bump docker/login-action from 3.0.0 to 3.3.0 (#5109) Bump docker/build-push-action from 5.1.0 to 6.5.0 (#5108) Bump peter-evans/create-pull-request from 5.0.0 to 6.1.0 (#5093) Tx Payment: drop ED requirements for tx payments with exchangeable asset (#4488) Remove `pallet-getter` usage from pallet-transaction-payment (#4970) pallet macro: do not generate try-runtime related code when frame-support doesn't have try-runtime. (#5099) fix(chain-spec): ChainSpecBuilder with object as default genesis (#4345) Migrate BEEFY BLS crypto to bls12-381 curve (#4931) Bump clap from 4.5.9 to 4.5.10 in the known_good_semver group (#5120) Use jobserver in wasm-builder to limit concurrency of spawned cargo processes (#4946) include events for voting (#4613) [subsystem-bench] Add mocks for own assignments triggering (#5042) Remove not-audited warning (#5114) hotfix: blockchain/backend: Skip genesis leaf to unblock syncing (#5103) ...

…polkadot-sdk#4488] (SDK #4935)[paritytech/polkadot-sdk#4935] (SDK #4949)[paritytech/polkadot-sdk#4949] (SDK #5649)[paritytech/polkadot-sdk#5649] - Configure Polkadot Asset Hub - Configure Bridge Hubs

muharem added 2 commits May 16, 2024 17:18

refactoring and swap credit

55f2072

integration of new api

dcfa93e

muharem requested a review from a team as a code owner May 16, 2024 15:30

This was referenced May 16, 2024

Tx Payment: (tests check) drop ED requirements for tx payments with exchangeable asset #4455

Closed

Drop ED Requirement for Transaction Payment with Exchangeable Asset polkadot-fellows/runtimes#310

Merged

style chages and logs

a4dbc71

muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label May 23, 2024

muharem added 3 commits May 23, 2024 16:30

pr doc

62f474f

pr doc style fix

27bb400

handle: asset id is a native asset

16ef515

fix integrations

3a8e675

muharem mentioned this pull request May 24, 2024

Make on_unbalanceds work with fungibles imbalances #4564

Merged

muharem commented May 24, 2024

View reviewed changes

update pr doc

e4f762b

ggwpez approved these changes May 24, 2024

View reviewed changes

muharem and others added 3 commits May 27, 2024 10:17

Apply suggestions from code review

6a31c79

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

do not deposit zero amount

ed57577

style fix

165084d

muharem requested a review from liamaharon May 27, 2024 15:41

georgepisaltu reviewed May 28, 2024

View reviewed changes

test

f305f09

georgepisaltu approved these changes May 30, 2024

View reviewed changes

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/tests.rs Outdated Show resolved Hide resolved

test assert fix

9583ebb

muharem enabled auto-merge June 25, 2024 10:52

muharem disabled auto-merge June 25, 2024 10:52

muharem mentioned this pull request Jul 15, 2024

Support different assets for delivery fees #4375

Closed

franciscoaguirre approved these changes Jul 18, 2024

View reviewed changes

substrate/frame/transaction-payment/asset-conversion-tx-payment/src/lib.rs Show resolved Hide resolved

muharem added 4 commits July 23, 2024 17:25

Merge remote-tracking branch 'origin/master' into muharem-tx-swap-ass…

6e4c4af

…et-payment

Merge remote-tracking branch 'origin/master' into muharem-tx-swap-ass…

dc6aae3

…et-payment

use on_unbalanceds

7b026fd

fmt

2b04035

muharem added this pull request to the merge queue Jul 24, 2024

muharem removed this pull request from the merge queue due to a manual request Jul 24, 2024

update prdoc

a3964fd

muharem enabled auto-merge July 24, 2024 13:15

muharem added this pull request to the merge queue Jul 24, 2024

Merged via the queue into master with commit 5878ea2 Jul 24, 2024
158 of 161 checks passed

muharem deleted the muharem-tx-swap-asset-payment branch July 24, 2024 14:26

ggwpez mentioned this pull request Sep 9, 2024

Parent issue for stable2409 LTS release #5583

Closed

github-actions bot mentioned this pull request Oct 8, 2024

Update polkadot-sdk from stable2407 to stable2409 moonbeam-foundation/moonbeam#2994

Open

TDemeco mentioned this pull request Oct 14, 2024

feat: ⏫ upgrade to Polkadot SDK stable2409 Moonsong-Labs/storage-hub#228

Merged

Aideepakchaudhary mentioned this pull request Oct 23, 2024

Feat: Polkadot stable2407 to stable2409 upgrade Cerebellum-Network/blockchain-node#456

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tx Payment: drop ED requirements for tx payments with exchangeable asset #4488

Tx Payment: drop ED requirements for tx payments with exchangeable asset #4488

muharem commented May 16, 2024 •

edited

Loading

paritytech-cicd-pr commented May 24, 2024

muharem May 24, 2024

ggwpez left a comment

ggwpez May 24, 2024 •

edited

Loading

muharem May 27, 2024

georgepisaltu May 28, 2024

muharem May 30, 2024

georgepisaltu May 30, 2024

muharem May 30, 2024

georgepisaltu left a comment

georgepisaltu May 28, 2024

georgepisaltu left a comment

Tx Payment: drop ED requirements for tx payments with exchangeable asset #4488

Tx Payment: drop ED requirements for tx payments with exchangeable asset #4488

Conversation

muharem commented May 16, 2024 • edited Loading

Problem

paritytech-cicd-pr commented May 24, 2024

muharem May 24, 2024

Choose a reason for hiding this comment

ggwpez left a comment

Choose a reason for hiding this comment

ggwpez May 24, 2024 • edited Loading

Choose a reason for hiding this comment

muharem May 27, 2024

Choose a reason for hiding this comment

georgepisaltu May 28, 2024

Choose a reason for hiding this comment

muharem May 30, 2024

Choose a reason for hiding this comment

georgepisaltu May 30, 2024

Choose a reason for hiding this comment

muharem May 30, 2024

Choose a reason for hiding this comment

georgepisaltu left a comment

Choose a reason for hiding this comment

georgepisaltu May 28, 2024

Choose a reason for hiding this comment

georgepisaltu left a comment

Choose a reason for hiding this comment

muharem commented May 16, 2024 •

edited

Loading

ggwpez May 24, 2024 •

edited

Loading