Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Pay tx fee with assets by using the asset conversion pallet #14340

Merged
merged 66 commits into from
Jun 23, 2023

Conversation

jsidorenko
Copy link
Contributor

@jsidorenko jsidorenko commented Jun 9, 2023

Allows to pay for the tx in any assets by swapping it into the native, given the liquidity pool exists

cumulus companion: paritytech/cumulus#2148

@jsidorenko jsidorenko requested a review from gilescope June 9, 2023 21:15
@jsidorenko jsidorenko added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 12, 2023
@jsidorenko jsidorenko added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 16, 2023
@paritytech-ci paritytech-ci requested a review from a team June 23, 2023 13:15
@paritytech-ci paritytech-ci requested a review from a team June 23, 2023 13:16
@paritytech-ci paritytech-ci requested a review from a team June 23, 2023 13:20
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

I think this is OK to go on Westend. Would like to see a few more things addressed before Kusama though. Will let @muharem and/or @kianenigma have another look before approving.

@paritytech-ci paritytech-ci requested a review from a team June 23, 2023 13:21
…ent.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@joepetrowski
Copy link
Contributor

  • How do you benchmark this? How does this effect the base tx weight? How do you ensure the weight is correctly consumed for tx that required conversion vs the one does not require conversion?

This needs to be addressed.

  • How do you ensure someone doesn't accidentally spend all the funds to perform a swap on a pool with no liquidity?

We don't, this is something that wallets and UIs can (and should) handle.

  • How do you handle weight refund?

Look at the PR.

  • Would you handle multi path swap? (because the direct path is not available or low on liquidity or unbalanced)

On Asset Hub we won't need to since everything can only pair with DOT, but this would be a nice addition (and should at least be documented in the README).

@paritytech-ci paritytech-ci requested a review from a team June 23, 2023 15:47
// If successful returns the amount in native tokens.
/// Trait for providing methods to swap between the chain's native token and other asset classes.
pub trait SwapNative<Origin, AccountId, Balance, AssetBalance, AssetId> {
/// Take an `asset_id` and swap some amount for `amount_out` of the chain's native asset. If an
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you actually need all of this to be generics, instead of associated types? You can probably save a bunch of boilerplate if you revise.

From what I can seem we want the asset-related stuff to be generic, but everything else to be associated types, because a chain might have multiple asset classes, but almost always has just one native currency. So something like

impl SawpNative<AssetType1> for PalletA {...}
impl SawpNative<AssetType2> for PalletB {...}

is reasonable but I don't think we ever want to customize the origin per implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would invite @gilescope to answer your question :) It was his idea to create this generic

@paritytech-ci paritytech-ci requested a review from a team June 23, 2023 15:52
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Some suggestions for follow-ups, looks good otherwise.

gilescope and others added 2 commits June 23, 2023 17:29
* Dedup raising swap event

* use expect rather than unwrap

* Additional checks for future defence.

* cargo fmt

* Update frame/asset-conversion/src/lib.rs

Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>

---------

Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>
@jsidorenko
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 81d125f into master Jun 23, 2023
44 checks passed
@paritytech-processbot paritytech-processbot bot deleted the js/pay-by-swap branch June 23, 2023 19:17
+ From<ChargeAssetLiquidityOf<T>>,
ChargeAssetIdOf<T>: Send + Sync,
{
const IDENTIFIER: &'static str = "ChargeAssetTxPayment";
Copy link
Member

Choose a reason for hiding this comment

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

This name should be unique per signed extension. But as I already said on element, we should just make asset-tx-payment generic to support the use case here.

@redzsina redzsina added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jul 14, 2023
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ch#14340)

* Pay tx by swapping the assets

* Change liquidity structure

* Uncomment the event

* Update frame/transaction-payment/asset-tx-payment/src/payment.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* New approach

* Fix bounds

* Clearer version

* Change IsType with Into and From

* Enable event

* Check ED + fix the logic

* Add temp comments

* Rework the refund

* Clean up

* Improve readability

* Getting closer

* fix

* Use fungible instead of Currency

* Test account without ed

* Final push

* Fixed

* Rename to pallet-asset-conversion-tx-payment

* Bring back the old pallet

* Update versions

* Update docs

* Update readme

* Wrong readme updated

* Revert back doc change

* Fix import

* Fix kitchensink

* Fix

* One more time..

* Wait pls

* Update frame/asset-conversion/src/lib.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* Update frame/support/src/traits/tokens/fungibles/regular.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Update docs/comments

* Docs improvement

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

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

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

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

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

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

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

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

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

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Payed -> paid

* Docs

* Update frame/transaction-payment/asset-conversion-tx-payment/README.md

Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com>

* Rewrite docs

* Try to clean the deps

* Add debug assert

* Return back frame-benchmarking

* Update cargo

* Update frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Rename

* clearer error message

* Docs for Pay by Swap (paritytech#14445)

* docs

* better error name

* more comments

* more docs on swap trait

* Fix compile errors

* Another fix

* Refactoring

* Update frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Emit an error if we fail to swap the refund back

* Add integrity_test

* Update frame/asset-conversion/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Fmt

* Use defensive_ok_or

* child PR: Tidy swap event (paritytech#14441)

* Dedup raising swap event

* use expect rather than unwrap

* Additional checks for future defence.

* cargo fmt

* Update frame/asset-conversion/src/lib.rs

Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>

---------

Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com>

---------

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants