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

Introduce XcmFeesToAccount fee manager #1234

Merged
merged 153 commits into from
Oct 18, 2023
Merged

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Aug 29, 2023

Combination of paritytech/polkadot#7005, its addon PR paritytech/polkadot#7585 and its companion paritytech/cumulus#2433.

This PR introduces a new XcmFeesToAccount struct which implements the FeeManager trait, and assigns this struct as the FeeManager in the XCM config for all runtimes.

The struct simply deposits all fees handled by the XCM executor to a specified account. In all runtimes, the specified account is configured as the treasury account.

XCM delivery fees are now being introduced (unless the root origin is sending a message to a system parachain on behalf of the originating chain).

Note for reviewers

Most file changes are tests that had to be modified to account for the new fees.
Main changes are in:

  • cumulus/pallets/xcmp-queue/src/lib.rs <- To make it track the delivery fees exponential factor
  • polkadot/xcm/xcm-builder/src/fee_handling.rs <- Added. Has the FeeManager implementation
  • All runtime xcm_config files <- To add the FeeManager to the XCM configuration

Important note

After this change, instructions that create and send a new XCM (Query*, Report*, ExportMessage, InitiateReserveWithdraw, InitiateTeleport, DepositReserveAsset, TransferReserveAsset, LockAsset and RequestUnlock) will require the corresponding origin account in the origin register to pay for transport delivery fees, and the onward message will fail to be sent if the origin account does not have the required amount. This delivery fee is in addition to what we already collect as tx fees in pallet-xcm and XCM BuyExecution fees!

Wallet UIs that want to expose the new delivery fee can do so using the formula:

delivery_fee_factor * (base_fee + encoded_msg_len * per_byte_fee)

where the delivery fee factor can be obtained from the corresponding pallet based on which transport you are using (UMP, HRMP or bridges), the base fee is a constant, the encoded message length from the message itself and the per byte fee is the same as the configured per byte fee for txs (i.e. TransactionByteFee).

@paritytech-ci paritytech-ci requested review from a team August 29, 2023 03:25
@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/frame-monthly-update/4628/1

@albertov19
Copy link

albertov19 commented Jan 25, 2024

My two cents here.

Although I understand why XCM delivery fees are needed, telling ecosystem developers to calculate the fee with the following formula:

delivery_fee_factor * (base_fee + encoded_msg_len * per_byte_fee)

It is probably the WORST developer experience ever. All chains (through an SDK) should have one standard way of requesting the chain: "How many tokens do I need to be able to send this call successfully?" and the answer should be "XYY amount of tokens."

This new delivery fee is obscured to the paymentInfo endpoint of the Polkadot.js SDK, meaning that dApps should now go and figure it out for themselves to calculate this delivery fee, which is just a terrible developer experience. Moreover, in certain edge cases, this can result in assets being locked in Parachain's sovereign account because the source wallet had enough tokens for the transaction fee but not enough for the delivery fee. For example:

https://assethub-kusama.subscan.io/extrinsic/6284005-2
https://assethub-kusama.subscan.io/extrinsic/6289790-2

This happens because the XCM delivery fee exceeds the account's balance, but this is not checked before executing the call.

@KiChjang
Copy link
Contributor Author

@albertov19 Appreciate your comment here, and I completely hear you. Certainly a single API that users can query to get delivery fees in the currency you specify is a really nice feature to have, and the XCM team is going to be putting it on the roadmap for this half of the year. I surmise that we would only be able to get the delivery fees part of it right in the beginning; we're going to have to need a bit more time to look further into the feasibility of listing just every single fee you need to pay for sending XCMs.

I do value the community's opinion on XCM related matters, and I do understand that there's currently no single source of information dissemination nor a hub to collect community feedback. We do have the Polkadot forums, but XCM news can easily get drowned with all other happenings in the ecosystem. Thus, on the first half of this year, we will be pushing for creating a sort of XCM info page where we can update you all on the stuff that the XCM team is working on, such as new features or important fee updates like this one. That way, we'll be able to hear from you much sooner and we'd be able to be more responsive to your concerns.

Finally, I'd like to take this brief moment to explain my position on DevEx. While I do see it as an important factor for adoption, we are unfortunately in a resource-constrained context in terms of engineering hours. As such, between better DevEx, optimized solutions and security concerns, I have to always prioritize security and functionality of the blockchain. I always follow the mantra of "first make it work; then make it fast; then make it pretty".

I hope I didn't come across as giving excuses for poor DevEx, but rather help you understand my circumstances.

@xlc
Copy link
Contributor

xlc commented Jan 25, 2024

we implement security feature to prevent people from losing money, not the other way around 💁‍♂️
and speaking about responsive, I raised this issue 10 months ago #690

@albertov19
Copy link

@KiChjang Thanks for your response.

I agree security must come first, there is no doubt about that. But to quote you on something: "We are unfortunately in a resource-constrained context in terms of engineering hours."

Parachain teams are in the same position, and current and ongoing problems with XCM and developer experience around it are creating an engineering nightmare in terms of overhead! We are dedicating our engineering hours to ongoing fixes and adaptation to non-documented changes.

https://kusama.subscan.io/extrinsic/21573927-7
#3070

@simonsso simonsso mentioned this pull request Feb 22, 2024
19 tasks
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Combination of paritytech/polkadot#7005, its addon PR
paritytech/polkadot#7585 and its companion paritytech/cumulus#2433.

This PR introduces a new XcmFeesToAccount struct which implements the
`FeeManager` trait, and assigns this struct as the `FeeManager` in the
XCM config for all runtimes.

The struct simply deposits all fees handled by the XCM executor to a
specified account. In all runtimes, the specified account is configured
as the treasury account.

XCM __delivery__ fees are now being introduced (unless the root origin
is sending a message to a system parachain on behalf of the originating
chain).

# Note for reviewers

Most file changes are tests that had to be modified to account for the
new fees.
Main changes are in:
- cumulus/pallets/xcmp-queue/src/lib.rs <- To make it track the delivery
fees exponential factor
- polkadot/xcm/xcm-builder/src/fee_handling.rs <- Added. Has the
FeeManager implementation
- All runtime xcm_config files <- To add the FeeManager to the XCM
configuration

# Important note

After this change, instructions that create and send a new XCM (Query*,
Report*, ExportMessage, InitiateReserveWithdraw, InitiateTeleport,
DepositReserveAsset, TransferReserveAsset, LockAsset and RequestUnlock)
will require the corresponding origin account in the origin register to
pay for transport delivery fees, and the onward message will fail to be
sent if the origin account does not have the required amount. This
delivery fee is on top of what we already collect as tx fees in
pallet-xcm and XCM BuyExecution fees!

Wallet UIs that want to expose the new delivery fee can do so using the
formula:

```
delivery_fee_factor * (base_fee + encoded_msg_len * per_byte_fee)
```

where the delivery fee factor can be obtained from the corresponding
pallet based on which transport you are using (UMP, HRMP or bridges),
the base fee is a constant, the encoded message length from the message
itself and the per byte fee is the same as the configured per byte fee
for txs (i.e. `TransactionByteFee`).

---------

Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: Giles Cope <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Liam Aharon <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* refactor message relay helpers

* single standalone_metrics function

* fixed tests

* clippy + fmt

* removed commented code

* add calls tracing

* fix spelling

* cargo fmt

* -commented code

* fix build again

* post-merge build fix

* clippy + fmt
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jun 25, 2024
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jun 28, 2024
* Setup deps

* Remove Koi from account migration test

* paritytech/polkadot-sdk#1495

* Bump

* paritytech/polkadot-sdk#1524

* !! paritytech/polkadot-sdk#1363

* paritytech/polkadot-sdk#1492

* paritytech/polkadot-sdk#1911

* paritytech/polkadot-sdk#1900

Signed-off-by: Xavier Lau <[email protected]>

* paritytech/polkadot-sdk#1661

* paritytech/polkadot-sdk#2144

* paritytech/polkadot-sdk#2048

* paritytech/polkadot-sdk#1672

* paritytech/polkadot-sdk#2303

* paritytech/polkadot-sdk#1256

* Remove identity and vesting

* Fixes

* paritytech/polkadot-sdk#2657

* paritytech/polkadot-sdk#1313

* paritytech/polkadot-sdk#2331

* paritytech/polkadot-sdk#2409 part.1

* paritytech/polkadot-sdk#2767

* paritytech/polkadot-sdk#2521

Signed-off-by: Xavier Lau <[email protected]>

* paritytech/polkadot-sdk#1222

* paritytech/polkadot-sdk#1234 part.1

* Satisfy compiler

* XCM V4 part.1

* paritytech/polkadot-sdk#1246

* Remove pallet-democracy part.1

* paritytech/polkadot-sdk#2142

* paritytech/polkadot-sdk#2428

* paritytech/polkadot-sdk#3228

* XCM V4 part.2

* Bump

* Build all runtimes

* Build node

* Remove pallet-democracy

Signed-off-by: Xavier Lau <[email protected]>

* Format

* Fix pallet tests

* Fix precompile tests

* Format

* Fixes

* Async, remove council, common pallet config

* Fix `ethtx-forward` test case (#1519)

* Fix ethtx-forward tests

* Format

* Fix following the review

* Fixes

* Fixes

* Use default impl

* Benchmark helper

* Bench part.1

* Bench part.2

* Bench part.3

* Fix all tests

* Typo

* Feat

* Fix EVM tracing build

* Reuse upstream `proof_size_base_cost()` (#1521)

* Format issue

* Fixes

* Fix CI

---------

Signed-off-by: Xavier Lau <[email protected]>
Co-authored-by: Bear Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
Status: Audited
Development

Successfully merging this pull request may close these issues.