-
Notifications
You must be signed in to change notification settings - Fork 83
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
feat: allow payment in non-native currencies by swapping with dex-general #1054
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sander2
force-pushed
the
feat/bring-your-own-fee
branch
from
May 12, 2023 10:36
14af546
to
de98856
Compare
sander2
commented
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
reviewed
May 25, 2023
daniel-savu
previously approved these changes
May 26, 2023
sander2
force-pushed
the
feat/bring-your-own-fee
branch
from
May 30, 2023 12:50
de98856
to
0fd54e7
Compare
sander2
force-pushed
the
feat/bring-your-own-fee
branch
from
May 30, 2023 12:51
0fd54e7
to
cc9c80e
Compare
This was referenced May 31, 2023
nud3l
approved these changes
Jun 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Existing solutions
I had a look at how other projects implement bring-your-own-fee. Specifically, I looked at Acala, HydraDX and substrate.
Existing implementations
Substrate native: pallet-asset-tx-payment
Substrate contains the
pallet-asset-tx-payment
pallet. This pallet changes the extrinsic header to include a CurrencyId. When a transaction is processed, a price source is used to convert the fee amount to the given currency. However, it does not do any swaps - it just pays the block author the equivalent amount in the given currency. This pallet is used by e.g. statemine and composable. The support in polkadot.js explorer is not good. Signing will work but it doesn't allow you to select different curriences. I think we could select the currency in our our front-end code, though.HydraDX
Hydra uses a custom pallet. Like the substrate one, it does not do any swaps, and just pays the block author in a different currency. The documentation states otherwise, but it is incorrect - I checked on-chain behavior: see e.g. block 2363593. Unlike the substrate pallet, they configure the fee currency not on a per-extrinsic level, but instead they configure the used currency in a dedicated extrinsic (and as a consequence, it's persistent). Implementation detail: they use the same
pallet_transaction_payment
that we do, but withOnChargeTransaction
that they implement in theirtransaction-multi-payment
pallet.Acala
Acala uses their own pallet, but unlike the previous two solutions, Acala actually performs a swap into their native currency. Similarly to Hydra, they have extrinsics to set the used currency (and swap path), but they also have a single-shot
with_fee_path
function. Their implementation is overkill for our purposes - they have this thing called a fee pool (in addition to the dex) and they allow fees to be paid by other accounts. They also reimplement a lot of code provided by substrate.Our implementation
One of the requirements given is to use actual swaps, which means that Acala's solution is closest to our requirements. However, for our purposes it is needlessly complex, so what I chose to do was to use their approach of
with_fee_path
, but with our own implementation. I did my best to keep our implementation as simple as possible, since this feature requires code execution prior to dispatch, which if you're not careful can easily lead to bugs that allow DoS attacks. It took a couple of iterations but I'm pretty happy with the simplicity of the current result. It reuses as much of the substrate implementation as possible.The usage of this pallet is as follows. If a user want to pay fees in a different currency, rather than calling
tx
, they callmultiTransactionPayment.withFeeSwapPath(path, ..., tx)
. UI will need to wrap all calls like this - the swap path is not committed to storage for future transactions (see discord discussion).How it works
First, a short summary of how transactions are processed. In our runtimes, we provide the SignedExtras, which is a tuple of
SignedExtension
s. In this case, the significant on ispallet-transaction-payment
: https://github.com/paritytech/substrate/blob/fd4c7b38c952cbac826e2da0a61ea40965f4296a/frame/transaction-payment/src/lib.rs#L780-L843 . Extensions have three functions:validate
,pre_dispatch
andpost_dispatch
. When a node receives a transaction, it first callsvalidate
. If this function returns an error, the transaction does not get included in the transaction queue, so it will never get included in a block. Typically thevalidate
andpre_dispatch
have very similar implementation; the first has to check if the second will be succesful. It also assigns a priority based on the tip left by the user. Anyway, if a transaction passes the all thevalidate
checks, and it gets included into a block, thepre_dispatch
gets executed, in which pallet-transaction-payment callsOnChargeTransaction::withdraw_fee
. After the dispatch,post_dispatch
gets called, which callsOnChargeTransaction::correct_and_deposit_fee
to refund unused fees (i.e. if weight is less than expected, or if the function returnsPays::No
).I implemented
OnChargeTransaction
on our newly added pallet. In thewithdraw_fee
, which is called fromvalidate
andpre_dispatch
, we inspect the call, and if the call iswith_fee_swap_path
, it will do the swap. Since the swap is done before the actual dispatch,with_fee_swap_path
itself is not doing any swaps. The only thing it does, is to dispatch the wrapped call, and to return the correct weight returned by it.The weight of
with_fee_swap_path
is set to the weight of the wrapped call, plus the weight of the swap. This ignores the logic for calculating the fee, and for checking the swap path. I think it's probably insignificant, but we could benchmark that part if needed.Note that if the fees are lower than expected due to the wrapped call returning a weight override, or it returning
Pays::No
, then the surplus is returned in the native currency; no additional swap gets done to swap it back to the fee currency.For now, I only added it to the kintsugi runtime. If this passes review I can add to the others as well.
Chopsticks transaction from an account that only has KSM: