Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Introduce XcmFeesToAccount fee manager #7005

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Apr 4, 2023

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).

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 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.

cumulus companion: paritytech/cumulus#2433

@KiChjang KiChjang added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Apr 4, 2023
@paritytech-ci paritytech-ci requested review from a team April 4, 2023 15:04
runtime/rococo/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/westend/src/xcm_config.rs Outdated Show resolved Hide resolved
xcm/xcm-builder/src/fee_handling.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/traits/transact_asset.rs Show resolved Hide resolved
Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top notch. There's a lot to like.
I think your right not to include the bridge hub in free xcm fees as bridge hub really should only be talking to statemine/t.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3322078

@acatangiu
Copy link
Contributor

@KiChjang what is the status of this PR? can we merge it?

@bkontur
Copy link
Contributor

bkontur commented Aug 7, 2023

@KiChjang @joepetrowski
I found one potential issue (with test and maybe with fix) with pallet_xcm::reserve_transfer_assets (probably this could affect all instructions in XcmExecutor which uses self.send),
please check this PR: #7585

@KiChjang
Copy link
Contributor Author

KiChjang commented Aug 7, 2023

@KiChjang what is the status of this PR? can we merge it?

It is ready whenever the cumulus companion is fixed.

@bkontur
Copy link
Contributor

bkontur commented Aug 8, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@acatangiu
Copy link
Contributor

@KiChjang what is the status of this PR? can we merge it?

It is ready whenever the cumulus companion is fixed.

Seems like this PR breaks pallet_xcm::reserve_transfer_assets() without #7585 - @KiChjang @franciscoaguirre please review fix in #7585 so we can add it here before we can merge this.

@bkontur
Copy link
Contributor

bkontur commented Aug 8, 2023

I made a cumulus companion to compile,
but when I run tests in Cumulus, they fail - see expected NotHoldingFees

test tests::reserve_transfer::reserve_transfer_native_asset_from_relay_to_assets ... FAILED
test tests::teleport::teleport_native_assets_from_relay_to_assets_para ... FAILED
test tests::transact::transact_sudo_from_relay_to_assets_para ... ok

failures:

---- tests::reserve_transfer::reserve_transfer_native_asset_from_relay_to_assets stdout ----
EVENTS: [RuntimeEvent::System(Event::NewAccount { account: 70617261e8030000000000000000000000000000000000000000000000000000 (5Ec4AhPZ...) }), RuntimeEvent::Balances(Event::Endowed { account: 70617261e8030000000000000000000000000000000000000000000000000000 (5Ec4AhPZ...), free_balance: 10000000000000 }), RuntimeEvent::Balances(Event::Transfer { from: d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d (5GrwvaEF...), to: 70617261e8030000000000000000000000000000000000000000000000000000 (5Ec4AhPZ...), amount: 10000000000000 }), RuntimeEvent::XcmPallet(Event::Attempted { outcome: Incomplete(Weight { ref_time: 686043000, proof_size: 6196 }, NotHoldingFees) })]
thread 'tests::reserve_transfer::reserve_transfer_native_asset_from_relay_to_assets' panicked at '

Event RuntimeEvent::XcmPallet(pallet_xcm::Event::Attempted {
outcome: Outcome::Complete(weight) }) was never received', parachains/integration-tests/emulated/assets/asset-hub-polkadot/src/tests/reserve_transfer.rs:49:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::teleport::teleport_native_assets_from_relay_to_assets_para stdout ----
EVENTS: [RuntimeEvent::Balances(Event::Withdraw { who: d43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d (5GrwvaEF...), amount: 10000000000000 }), RuntimeEvent::Balances(Event::Deposit { who: 6d6f646c70792f78636d63680000000000000000000000000000000000000000 (5EYCAe5i...), amount: 10000000000000 }), RuntimeEvent::System(Event::NewAccount { account: 6d6f646c70792f78636d63680000000000000000000000000000000000000000 (5EYCAe5i...) }), RuntimeEvent::Balances(Event::Endowed { account: 6d6f646c70792f78636d63680000000000000000000000000000000000000000 (5EYCAe5i...), free_balance: 10000000000000 }), RuntimeEvent::XcmPallet(Event::Attempted { outcome: Incomplete(Weight { ref_time: 792675000, proof_size: 7585 }, NotHoldingFees) })]
thread 'tests::teleport::teleport_native_assets_from_relay_to_assets_para' panicked at '

Event RuntimeEvent::XcmPallet(pallet_xcm::Event::Attempted {
outcome: Outcome::Complete { .. } }) was never received', parachains/integration-tests/emulated/assets/asset-hub-polkadot/src/tests/teleport.rs:49:9


failures:
    tests::reserve_transfer::reserve_transfer_native_asset_from_relay_to_assets
    tests::teleport::teleport_native_assets_from_relay_to_assets_para

so that fix #7585 is really needed

@bkontur
Copy link
Contributor

bkontur commented Aug 8, 2023

with fix those tests fails on Balance asserts which needs to be corrected, because of these new xcm fees are charged from user account:

test tests::reserve_transfer::reserve_transfer_native_asset_from_relay_to_assets ... FAILED
test tests::teleport::teleport_native_assets_from_relay_to_assets_para ... FAILED
test tests::transact::transact_sudo_from_relay_to_assets_para ... ok

failures:

---- tests::reserve_transfer::reserve_transfer_native_asset_from_relay_to_assets stdout ----
thread 'tests::reserve_transfer::reserve_transfer_native_asset_from_relay_to_assets' panicked at 'assertion failed: `(left == right)`
  left: `30960000000000`,
 right: `30959632000000`', parachains/integration-tests/emulated/assets/asset-hub-polkadot/src/tests/reserve_transfer.rs:79:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::teleport::teleport_native_assets_from_relay_to_assets_para stdout ----
thread 'tests::teleport::teleport_native_assets_from_relay_to_assets_para' panicked at 'assertion failed: `(left == right)`
  left: `30960000000000`,
 right: `30959632000000`', parachains/integration-tests/emulated/assets/asset-hub-polkadot/src/tests/teleport.rs:76:5


failures:
    tests::reserve_transfer::reserve_transfer_native_asset_from_relay_to_assets
    tests::teleport::teleport_native_assets_from_relay_to_assets_para

test result: FAILED. 1 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 18.42s

@bkontur
Copy link
Contributor

bkontur commented Aug 13, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkontur
Copy link
Contributor

bkontur commented Aug 15, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkontur
Copy link
Contributor

bkontur commented Aug 16, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants