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

Better delivery fees: PayFees #3434

Closed
franciscoaguirre opened this issue Feb 21, 2024 · 6 comments · Fixed by #4826
Closed

Better delivery fees: PayFees #3434

franciscoaguirre opened this issue Feb 21, 2024 · 6 comments · Fixed by #4826
Assignees
Labels
I5-enhancement An additional feature request. T6-XCM This PR/Issue is related to XCM.

Comments

@franciscoaguirre
Copy link
Contributor

franciscoaguirre commented Feb 21, 2024

As you all know, there have been a lot of XCM related issues in Kusama lately. This is because of the new delivery fees not being integrated in the best way to the executor.

There are three problems I can identify:

  1. Getting delivery fees from holding during execution
  2. Estimating delivery fees
  3. Fixing delivery fees for multi-hop messages

For 1. we need a system that's similar to BuyExecution but for delivery fees. We could have an instruction BuyTransport that works in a similar way. You pay once for all delivery fees and can be certain the rest of the execution will succeed.

Something like BuyTransport would work, but I'd like to take this opportunity to not add more must-use instructions that make XCM harder to use. It's already hard enough when people forget a BuyExecution instruction. I think it's worthwhile to pay for these fees transparently.

Different xcm senders can charge different prices, and we can only identify them via calling validate_send, which calls a tuple and each element either returns the price or fails and it tries the next one. These senders validate using the destination. So we need to know the destination of all messages that will be sent beforehand.

My proposal is to gather all destinations beforehand, and buy enough delivery fees for sending whenever we pay for them, be it on a custom instruction or on the first instruction that loads the holding register.

The size of the message is also important, so I suggest we use a worst-case message in case it doesn't increase costs too much.

I'll leave the other two for other issues.

L.E.: XCM RFC for this polkadot-fellows/xcm-format#53

@franciscoaguirre franciscoaguirre changed the title Better delivery fees Better delivery fees: BuyTransport Feb 21, 2024
@xlc
Copy link
Contributor

xlc commented Feb 21, 2024

Related #690

From user point of view, the only thing they care is the total fee. They don't care if the fee is used for buy execution or buy transport. It shouldn't matter.

So my suggestion is make a fee register, rename BuyExecution to DepositFee and simply charge fees from the fee register. In future we can introduce more ways to charge fee and it wouldn't make any impact.

@franciscoaguirre
Copy link
Contributor Author

I'm making a prototype in #3450, the idea is the following:

  • Reuse the existing BuyExecution instruction so as to not change how people write their XCMs. This instruction already lets you set a limit on how much you're willing to pay for fees, which satisfies the requirement.
  • Estimates the amount needed for delivery fees inside of BuyExecution, and then readjusts it when the actual self.send function is encountered during program execution. This "paying all at once" is more similar to how execution fees are handled.

My only concern is the use of a barrier to gather the destinations of where the messages will be sent. We need this for estimating.
In a way, this is similar to how the AllowTopLevelPaidExecutionFrom<T> barrier sets the actual weight of the message, to help BuyExecution further down the line.
I appreciate any feedback on this.

@acatangiu
Copy link
Contributor

acatangiu commented Feb 23, 2024

I propose to split this in two separate tracks that we can tackle cleanly and independently, and when both are completed, the whole fees problem will be solved:

  1. A clean mechanism to charge fees (all types: execution, transport) from assets included in XCM program (e.g. assets being transferred), user must be able to provide an upper limit.
  2. A mechanism for finding the complete fees (including local, intermediary and destination chains) required for the successful execution of an XCM program.

1. charging fees

This will not attempt to estimate fees.

So my suggestion is make a fee register, rename BuyExecution to DepositFee and simply charge fees from the fee register. In future we can introduce more ways to charge fee and it wouldn't make any impact.

This ^

In a new XCM version we can implement new DepositFee(fees: Asset) instruction (also deprecate/remove jit_withdraw mechanism).
Any unspent fees at the end of the execution are trapped for future claim back.

This means we can implement it and be useful for users even before/without having 2.
As long as users overestimate fees, their program will not fail, and excess/unspent can be claimed back.

We also need to fix assets trapping and claiming

  1. we need https://github.com/paritytech/xcm-format/blob/master/proposals/0038-execute-with-origin.md for actually trapping assets on remote chains (right now most programs do ClearOrigin which kills the asset trap as well)
  2. we need https://github.com/paritytech/xcm-format/blob/master/proposals/0037-custom-asset-claimer.md for providing better UX on claiming back trapped assets

2. Fees discovery

This is being discussed in #690

Current proposal is to provide runtime apis (callable from on-chain or off-chain) to effectively "dry-run" an XCM program, providing info on things like:

  • local side effects
  • forwarded messages: forwarded XCM content and its destination
  • locally spent fees

The same runtime API can then be called off-chain on [intermediary, +] destination chains recursively dry-running the forwarded XCMs from previous hop.

This way a wallet/dapp can dry-run the e2e path and get fees for each step.

This runtime API can be later genericized/replaced with a new XCQ mechanism to get the same info (and more) using generic chain-agnostic queries (rather than chain-specific runtime api calls).

Further, ideas were also discussed for some mechanism to actively lock-in the fee-price, to avoid synchronization issues between querying and executing.

1 & 2 together

Once we have both, users can find out how much fees are (in the future even lock in that price for a number of blocks), then adjust values used in the executed XCMs' DepositFee instructions to the right values.

@franciscoaguirre
Copy link
Contributor Author

franciscoaguirre commented Feb 23, 2024

I agree with the roadmap to fix all the fee issues. However, I'm hesitant to add a new version for XCM, since it would take some time. We still need to remove V2 and V4 was released very little ago.

I understand a lot of programs will be broken by this change. I can work on a V5 version but it'll take a little bit.
If not many people are using/have migrated to V4 yet, we could do it there. It's not even released in Kusama yet.

@acatangiu acatangiu changed the title Better delivery fees: BuyTransport Better delivery fees: DepositFee Feb 23, 2024
@franciscoaguirre
Copy link
Contributor Author

Nevermind, we should definitely do a new version for this

@acatangiu
Copy link
Contributor

Implemented here #5420

github-merge-queue bot pushed a commit that referenced this issue Nov 6, 2024
# Context

This PR aims to introduce XCMv5, for now it's in progress and will be
updated over time.
This branch will serve as a milestone branch for merging in all features
we want to add to XCM, roughly outlined
[here](polkadot-fellows/xcm-format#60).
More features could be added.

## TODO
- [x] Migrate foreign assets from v3 to v4
- [x] Setup v5 skeleton
- [x] Remove XCMv2
- [x] #5390
- [x] #5585
- [x] #5420
- [x] #5876
- [x] #5971
- [x] #6148
- [x] #6228

Fixes #3434 
Fixes #4190
Fixes #5209
Fixes #5241
Fixes #4284

---------

Signed-off-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Andrii <ndk@parity.io>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Joseph Zhao <65984904+programskillforverification@users.noreply.github.com>
Co-authored-by: Nazar Mokrynskyi <nazar@mokrynskyi.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Serban Iorga <serban@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T6-XCM This PR/Issue is related to XCM.
Projects
Status: Done
Status: In-Progress
Development

Successfully merging a pull request may close this issue.

3 participants