-
Notifications
You must be signed in to change notification settings - Fork 41
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
--- | ||
Title: Fee handling generalization and simplification | ||
Number: 53 | ||
Status: Draft | ||
Version: 5 | ||
Authors: | ||
- Francisco Aguirre | ||
Created: 2024-03-01 | ||
Impact: Low | ||
Requires: | ||
Replaces: | ||
--- | ||
|
||
## Summary | ||
|
||
XCM already handles execution fees in an effective and efficient manner using the `BuyExecution` instruction. | ||
However, other types of fees are not handled as effectively -- for example, delivery fees. | ||
Fees exist that can't be measured using `Weight` -- as execution fees can -- so a new method should be thought up for those cases. | ||
This RFC proposes making the fee handling system simpler and more general, by doing two things: | ||
- Adding a `fees` register | ||
- Deprecating `BuyExecution` and adding a new instruction `PayFees` with new semantics to ultimately replace it. | ||
|
||
This new instruction only takes the amount of fees that the XCVM can use from the holding register. | ||
The XCVM implementation is free to use these fees to pay for execution fees, transport fees, or any other type of fee that might be necessary. | ||
This moves the specifics of fees further away from the XCM standard, and more into the actual underlying XCVM implementation, which is a good thing. | ||
|
||
## Motivation | ||
|
||
Execution fees are handled correctly by XCM right now. | ||
However, the addition of extra fees, like for message delivery, result in awkward ways of integrating them into the XCVM implementation. | ||
The standard should have a way to correctly deal with these implementation specific fees, that might not exist in every system that uses XCM. | ||
|
||
## Specification | ||
|
||
The new instruction that will replace `BuyExecution` is a much simpler and general version: `PayFees`. | ||
This instruction takes some `Assets` and takes them from the holding register, putting them into a new `fees` register. | ||
The XCVM implementation can now use these `Assets` to make sure every necessary fee is paid for, this includes execution fees, delivery fees, or any other fee. | ||
|
||
```rust | ||
PayFees { assets: Assets } | ||
``` | ||
|
||
This new instruction will reserve **the entirety** of `assets` for fee payment. | ||
The assets can't be used for anything else during the entirety of the program. | ||
This is different from the current semantics of `BuyExecution`. | ||
|
||
If not all `Assets` in the `fees` register are used when the execution ends, then we trap them. | ||
|
||
### Examples | ||
|
||
Most XCM programs that pay for execution are written like so: | ||
|
||
```rust | ||
// Instruction that loads the holding register | ||
BuyExecution { assets, weight_limit } | ||
// ...rest | ||
``` | ||
|
||
With this RFC, the structure would be the same, but using the new instruction, that has different semantics: | ||
|
||
```rust | ||
// Instruction that loads the holding register | ||
PayFees { assets } | ||
// ...rest | ||
``` | ||
|
||
## Security considerations | ||
|
||
Overpaying for fees might have an increase in trapped assets. | ||
Depending on the method used for trapping assets, this might result in too much storage used. | ||
Comment on lines
+69
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Instead of trapping the asset, can you automatically There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
## Impact | ||
|
||
For systems with no fees other than execution, this change would be just a rename of the instruction. | ||
For systems with other types of fees, migration to this new structure would be advised, as they currently can't express the new fees. | ||
|
||
## Alternatives | ||
|
||
Alternatives include: | ||
1. Simply changing the semantics of `BuyExecution` | ||
2. Create a new instruction for each conceivable type of fees | ||
|
||
Alternative number 1, while possible, would result in a lot of existing programs breaking because of changed assumptions. | ||
The change must happen either in a new instruction, or in a new version, to ensure those programs are not broken. | ||
While the instruction could be called the same in a new version of the format, changing the name makes the changes clearer. | ||
|
||
Alternative number 2 is not only unfeasible, but also brings a lot of implementation-specific details into the format. | ||
|
||
The impact of not doing this is suffering from bugs born from failing to handle the different type of fees. | ||
|
||
## Questions and open Discussions (optional) | ||
|
||
The name of the instruction is not set in stone. | ||
There might be a better way of handling fee overpayment. |
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.