Skip to content
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

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions proposals/0053-better-fee-handling.md
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 one `Asset`, takes it from the holding register, and puts it into a new `fees` register.
The XCVM implementation can now use this `Asset` to make sure every necessary fee is paid for, this includes execution fees, delivery fees, or any other fee.

```rust
PayFees { asset: Asset }
```

This new instruction will reserve **the entirety** of `asset` for fee payment.
The asset 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 `Asset` in the `fees` register is used when the execution ends, then we trap them alongside any possible leftover assets from the holding register.

### Examples

Most XCM programs that pay for execution are written like so:

```rust
// Instruction that loads the holding register
BuyExecution { asset, 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 { asset }
// ...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
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

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 and DepositAsset 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?

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 and DepositAsset 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?

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.