RFC: Better fee handling

franciscoaguirre commented 9 months ago

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
Replacing BuyExecution with a new instruction PayFees with new semantics

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.

Polkadot-Forum commented 9 months ago

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-rfc-better-fee-handling/6547/1

franciscoaguirre commented 9 months ago

A PoC was done for Polkadot in https://github.com/paritytech/polkadot-sdk/pull/3450.

xlc commented 8 months ago

We had another XCM transfer issue on Acala related to Acala specific fee handling that could be addressed by this new fee mechanism by allowing the parachain to charge additional fees on top of the existing XCM fee types.

acatangiu commented 8 months ago

We had another XCM transfer issue on Acala related to Acala specific fee handling that could be addressed by this new fee mechanism by allowing the parachain to charge additional fees on top of the existing XCM fee types.

AFAICT the current proposal would allow that if you're implementing custom executor with custom PayFees execution, right?

What kind of "additional fees" should we be thinking about? Is it something we should just add support for in existing executor (some customizable extra fee types)? Or is handling them with custom executor better?

franciscoaguirre commented 8 months ago

The only config item we have for fees is the Trader, but that's tightly coupled to weight. We could provide a new config item to make it easier to define custom fees. That's regardless of the standard

xlc commented 8 months ago

For the Acala specific use case, we charge an additional storage deposit for EVM calls. In the case that the EVM call is triggered by XCM, we found we are in a poor situation that we don't have a good way to charge this deposit.

We cannot charge it from the sovereign account, because otherwise it will be an attack vector to drain those accounts. We cannot charge it using the XCM fee mechanism because there is no such mechanism. The only thing we can do is charge from the receiving account, and that's a bad idea as well.

franciscoaguirre commented 8 months ago

What I'm thinking we can do after this new fees register, is having a custom CallDispatcher that checks if the call inside Transact is an EVM call and uses fees from the fees register to pay for the storage deposit. That would only need access to the fees register from the CallDispatcher, and would allow everyone to create custom fees. This is very hacky and we could even do it if we give it access to the holding register right now.

The good approach I can think of is adding a config item that can be a tuple of structs that have access to the fees register. Then the implementation of PayFees will loop through them and charge all the fees. We could even add a log for each of them so the user knows why it was charged that. This would need to be accompanied by a good way of getting these fees so UIs can estimate them. We don't have anything that would fit this in the XCM fee estimation API. We could add a call for additional fees.

Just brainstorming here. The additional fees scenario is a different one from the main one this RFC is trying to solve though.

xlc commented 8 months ago

having a custom CallDispatcher that checks if the call inside Transact is an EVM call

that wouldn’t be enough. ERC20 tokens are first class citizens at Acala. ie we allow transfer them via XCM using deposit/withdraw mechanism. so it is possible to invoke EVM without transact

We currently have a global variable (implemented using a storage entry) to provide additional context to EVM so it knows where to charge the storage deposit. A fee register if implemented properly, will allow us to charge deposit from it. And we can simply make deposit/withdraw action to fail if unable to charge enough deposits.

mrshiposha commented 7 months ago

The interface of the PayFees instruction is great! It allows for accumulating all the fee assets to cover every fee type.

I'd like to discuss the usage of this instruction by clients/users. It would be great if users did not bother themselves with different "fee types" and did not have to worry about what currencies they should pay for each fee type.

And it would be awesome to be able to do something like this:

Suppose a user holds two currencies (CURRENCY_A and CURRENCY_B). Let the user have AMOUNT_A and AMOUNT_B of the corresponding currencies.
Imagine the user wants to execute an XCM program, but neither AMOUNT_A nor AMOUNT_B is enough to cover the fees.
But suppose, using the PayFees instruction, the user can supply both AMOUNT_A and AMOUNT_B, and the XCM executor will handle the rest. The user doesn't know for what "fee type" and in what proportion their funds will be used. The user only cares about whether it is enough to execute the program or not. The user isn't interested in the details.

To achieve this, we could introduce an abstract asset type that the XCM Executor could use internally (we can call them "XCM credits"). I'm not sure if it should be part of the spec; it seems more like an implementation detail. Still, I believe it is worth discussing here nonetheless.

The "XCM credits" may be used to cover:

The execution fees (with an appropriate conversion from Weight to these Credits)
The delivery fees where the amount of these Credits will depend on the congestion of the HRMP channel
Any other fee type

A concrete blockchain could define the appropriate two-way conversion between its acceptable payment assets and these Credits.

So, the PayFees instruction / fees register semantics could be defined in more detail:

This instruction populates the fees register
The fees register contains the list of (asset_id, amount_of_asset, corresponding_xcm_credits)
When the XCM implementation wants to deduct the Credits from the fees register, it will deduct them in order of this list (i.e., if the Credits obtained from one asset type are drained, the next one is used).
If there are any leftovers with non-zero corresponding_xcm_credits in the fees register, the remaining Credits should be converted back into their corresponding assets. These assets might be dealt with as discussed in the comment above: https://github.com/paritytech/xcm-format/pull/53#discussion_r1513073687.

The XCM Fee Payment Runtime API could also be modified accordingly so clients can:

convert programs into Credits
convert a list of assets into the total number of credits they represent

xlc commented 7 months ago

While there are some use cases of accepting multiple tokens for fee payments, I feel it is an edge case that can be easily avoided on the frontend side and the additional complicity does not worth the trade-off.

The frontend should just figure out what asset should be used for XCM fee and optionally provide a way for user to change that. If no single asset is enough, display not enough fee is a reasonable outcome.

mrshiposha commented 7 months ago

@xlc Even if we don't want to support payment in multiple user-supplied currencies, there is another reason to introduce abstract Credits (by the way, in the simplest case, they can be defined as equivalent to the chain's native token).

I am worried that the execution fee and the delivery fee (and any potential custom fee types) may currently require different tokens for payment on a single chain. With the BuyExecution instruction, users were able to select the token to cover the execution fees at least. With PayFees, the payment asset for any fee type is chain-defined.

The idea is to introduce an abstract asset to express any fee type so any acceptable user-supplied asset (singular) will cover all the fee types on a single chain (any acceptable asset can be converted to Credits). Thus, the user or a JS client won't need to think about different fee types and different required assets for each one within a single chain. They would be able to select a singular token to cover everything.

Payment in multiple currencies becomes possible with this approach (and the XCM executor can handle it without worrying the chain developers), but it is optional.

xlc commented 7 months ago

I don’t think that’s a real issue as the xcm on different chain are different and therefore it is possible to specify what assets for fee payment on each chain. It will be silly for a chain to require multiple assets for fee payments. Besides, the credit system will make refund fee lot more complicated.

mrshiposha commented 7 months ago

I'm worried because AssetHub does this. You can pay for the execution using USDT, but you also must have enough DOT to cover the delivery fees. It is a small fee, but nonetheless -- you pay for the execution in USDT and delivery in DOT.

xcmp-queue uses PriceForSiblingParachainDelivery
delivery fee asset id == DOT
When I send 1 USDT from Unique to Astar, then I must supply not only USDT for Asset Hub's execution fees but also some DOT (which is impossible since DOT's and USDT's reserve locations differ). See in the block, the assets were trapped. The error is NotHoldingFees, but 1 USDT should be enough to cover the execution fees. I suspect it failed here when it tries to deduct the delivery fees in DOT

acatangiu commented 7 months ago

I'm worried because AssetHub does this. You can pay for the execution using USDT, but you also must have enough DOT to cover the delivery fees. It is a small fee, but nonetheless -- you pay for the execution in USDT and delivery in DOT.

* [xcmp-queue uses PriceForSiblingParachainDelivery](https://github.com/polkadot-fellows/runtimes/blob/c0e66c3e121cc5da7d59415015019c3bb0ec6581/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L581-L586)

* [delivery fee asset id == DOT](https://github.com/polkadot-fellows/runtimes/blob/c0e66c3e121cc5da7d59415015019c3bb0ec6581/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L575)

* When I send 1 USDT from Unique to Astar, then I must supply not only USDT for Asset Hub's execution fees but also some DOT (which is impossible since DOT's and USDT's reserve locations differ). See [in the block](https://assethub-polkadot.subscan.io/block/0xe45496a3cc1e472282df659d254c17e31fc87ad76dac5bb424b7ef801f6f3a64?tab=event), the assets were trapped. The error is `NotHoldingFees`, but 1 USDT should be enough to cover the execution fees. I suspect it failed [here](https://github.com/paritytech/polkadot-sdk/blob/ecfcb42dfb419ebc7fee0793dbd1f9dbf0089883/polkadot/xcm/xcm-executor/src/lib.rs#L1094) when it tries to [deduct the delivery fees](https://github.com/paritytech/polkadot-sdk/blob/ecfcb42dfb419ebc7fee0793dbd1f9dbf0089883/polkadot/xcm/xcm-executor/src/lib.rs#L457-L458)

Going forward I hope to hear from the ecosystem about these kinds of problems and try and fix them instead of building complicated machinery either on core protocol or upper layers to handle them.

What you describe is a good example of this: a problem we need to fix (https://github.com/paritytech/polkadot-sdk/issues/3958) rather than build more complexity around it to work around.

mrshiposha commented 7 months ago

If taking several assets for different fee types on a single chain is always considered a problem that the chain's team should fix, I'm okay with that :)

I'd just be happy to see an interface that makes it easy "to do fee-related things right" and harder "to do wrong". However, the credit system would be an unpleasant migration for chains with big XCM configs (such as Acala). Also, an audit of the XCM executor would certainly be required, even if the actual implementation is relatively simple. So, yeah, the credit system requires much stronger reasons to be introduced.

Speaking of interface, if we agree that only one asset should be used to cover all the fees on a single chain, should the PayFees take multiple assets (as it currently does)? Is there a reason for that? Can we make it take just an Asset?

xlc commented 7 months ago

While the chain should only require a single asset for fee payment, the chain may also accept multiple assets for fee payment purpose. In the example of Acala, we accept multiple tokens for XCM fee payment including ACA, DOT, LDOT, etc. User only need to supply one of those accepted token and able to perform XCM execution on Acala.

mrshiposha commented 7 months ago

the chain may also accept multiple assets for fee payment purpose

Of course. It is not what I'm asking.

The Asset type definition is:

pub struct Asset {
    /// The overall asset identity (aka *class*, in the case of a non-fungible).
    pub id: AssetId,
    /// The fungibility of the asset, which contains either the amount (in the case of a fungible
    /// asset) or the *instance ID*, the secondary asset identifier.
    pub fun: Fungibility,
}

At the same time, Assets (the type the PayFee uses currently) has the following definition

pub struct Assets(Vec<Asset>);

The current BuyExecution instruction takes an Asset, not Assets. But PayFees takes Assets. Why?

If the chain should only require a single asset for fee payment, then why does PayFees allow a user to supply a vec of the different assets? For instance, vec![(ACA, 10), (DOT, 2), (LDOT, 3)]). PayFees instruction moves all the assets in the list (of different types) from the Holding Register to the fees register.

If we make PayFees accept the Asset type, the user is still capable of paying in different asset types, but only a single asset will be used in a single XCM program (just like it is with BuyExecution, but this time different fee types will be covered with this asset)

franciscoaguirre commented 7 months ago

But PayFees takes Assets. Why?**

I wanted to make it more generic, maybe you don't have enough of one asset and want to pay with a combination of assets. I agree though that this is too complicated for something that should be simple, like fees. I'll revert it to take only one asset.

Polkadot-Forum commented 7 months ago

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/2024-04-23-technical-fellowship-opendev-call/7592/1

Polkadot-Forum commented 6 months ago

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-user-and-developer-experience-improvements/4511/21

xlc commented 6 months ago

what's the next step?

franciscoaguirre commented 6 months ago

The next steps are finishing up the prototype, getting more RFCs for v5 and creating v5

franciscoaguirre commented 6 months ago

I've been busy with other tasks, but I can start with v5 in some capacity

franciscoaguirre commented 4 months ago

Closing in favor of https://github.com/polkadot-fellows/RFCs/pull/105

polkadot-fellows / xcm-format

RFC: Better fee handling #53