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.
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
RFC: Better fee handling #53
RFC: Better fee handling #53
Changes from 2 commits
1808524
23dd93e
e8dac77
be522fe
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't like trapping asset. With the current model, we almost always needs a
RefundSurplus
+DepositAsset
at end of the XCM to avoid trapped asset. Can we do it better?I see a few options:
PayFees
also takes another optional refund address and automatically refund unused fees to it at end of the executionDepositAsset
that deposit assets from all the asset register including fees and holdingsSetTrapAssetDestination
instruction that automatically deposit trapped asset into an account, instead of actually trap itRefundSurplus
+DepositAsset
at end of the XCM to avoid trapped asset.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's not the best to always require
RefundSurplus
+DepositAsset
.I like the idea of depositing to some account instead of trapping the assets. At least letting the user specify that behaviour.
For example,
SetAssetClaimer
is already approved, which lets the user specify who can claim trapped assets.We could have something similar that allows the user to specify they want to deposit leftover assets to a particular location instead of trapping them.
However, this would just reduce the two instructions (
RefundSurplus
+DepositAsset
) to one (something likeSetLeftoverFeesDestination
).Not sure how much more useful that is.
We could make the default behaviour be to deposit the leftover fees to the Location specified by origin, given that we expect them to always be a little bit more than needed. If the user wants to specify the location for said leftover fees (say, an account they own in the local system that's not the sovereign account), they can specify it.
I think this would change the least assumptions about XCM programs. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't properly thought it through, so I won't offer an opinion on what is best option, but I want to call out early that we should design this (and further changes) to be easily usable in multi-hops XCM programs.
E.g. going through remote reserve where the user might not have an account, or going over a bridge where delivery fees need to be paid at both intermediary bridge-hubs, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One benefit of
SetXXX
is that we call it at the beginning of the execution and it will be executed no matter what. On the other hand, theRefundSurplus
at end may not be skipped because errors. We do have instructions to handle errors but then it maybe a bit more complicated? Actually we don't really have a best practice about how to write error-proof XCM and someone need to spend a bit of time to think about all the situations and come up some good recommendations. e.g. always haveSetAppendix
to executeRefundSurplus
+DepositAsset
to ensure no trapped asset no matter what.Some disadvantage of
SetXXX
instructions is that the implementation will be slightly more complicated as there are more state to remember. But I think that's not a real issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the
SetX
instruction would need a new register holding a Location to where you want to deposit leftover fees. I think it's a good idea to have it.Also, I agree best practices are definitely needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was never a fan of asset traps because it is not trivial to recover traps, etc. - I like having a field in
PayFees
where assets left in the holding register are sent to if program execution fails.Even by default, I think the address should be the sovereign account of the origin chain. Therefore, it is up to the governance system of such a chain to help recover the assets.
@xlc - you can always put the
RefundSurplus
+DepositAsset
within aSetAppendix
so that it is "ensured" that they get executed if program execution fails. That is what we do with the XCM-Transactor we use at Moonbeam to do remote transacts (although we make it optional because before the cost of adding those extra instructions was "high")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the upcoming SetAssetClaimer, you could achieve exactly that without having to "bake it in" the
PayFees
instruction.Redesigning the asset trapping mechanism is also something on the TODO list, with ideas like depositing directly to sovereign accounts (custom account if set by
SetAssetClaimer
or the sovereign account of the origin chain by default). But that should be discussed in a dedicated RFC imo.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When coupling this with a good fee discovery mechanism (being discussed in paritytech/polkadot-sdk#690), this concern is diminished. At least from users acting in good faith. We still need to consider if/how this can be exploited in bad faith.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah current trap asset implementation is flawed paritytech/polkadot-sdk#901
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make sense to not trap fees, or (fees + assets) if the amount is below a particular threshold. That way we prevent extra storage bloat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of trapping the asset, can you automatically
RefundSurplus
andDepositAsset
back to the account being executed from, to reduce the likelihood that someone would forget to execute those operations and slowly lose assets over time?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this once, but this assumes an explicit implementation more related to XCVM rather than XCM (for example, consider cases where more than one asset is being withdrawn, how is this automatically handled by the transaction format without enforcing an XCVM specific behaviour?).
I think that part of the design is okay as it is now.
That said, yeah, the assets trap design can be improved.