search

open-web3-stack / open-runtime-module-library

Substrate Open Runtime Module Library
Apache License 2.0
455 stars 295 forks source link

[XCM delivery fees] Add missing SetFeesMode instruction in xtokens #1006

Closed Agusrodri closed 3 months ago

Agusrodri commented 3 months ago

Motivation

At Moonbeam, we are on process of enabling XCM delivery fees in our runtimes. While testing this functionality through xtokens pallet, some of our tests were failing.

The reason for this is that if XCM delivery fees are enabled on the origin chain (the chain that in this case is building the message though xtokens) and the SetFeesMode instruction is not appended to the final message, the local XCM execution prior to send the message will fail, as the XCM executor will try to deduct the delivery fees from the holding register (usually the origin chain's native asset) and they won't be there.

More context on: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-executor/src/lib.rs#L459-L467

Similar issue on polkadot-sdk

There was also a similar problem on polkadot-sdk. In that case, SetFeesMode was missing in some part of the code that transfer_assets function of pallet-xcm executes.

Related PR: https://github.com/paritytech/polkadot-sdk/pull/3792

xlc commented 3 months ago

SetFeesMode was a mistake and it will be removed in XCM v5 so I don't really want to touch it. However if it is a blocking issue for you, I am fine with this change.

Can you add some tests?

Agusrodri commented 3 months ago

SetFeesMode was a mistake and it will be removed in XCM v5 so I don't really want to touch it. However if it is a blocking issue for you, I am fine with this change.

Can you add some tests?

Thanks! Yes I could add some tests. Although, I don't see a good way to test this, as if we would like to test the addition of SetFeesMode specifically, we would need to enable delivery fees in the mocked runtime(s), and I think that's out of the scope for this PR as that change in mocks would break other existing tests.

The addition of SetFeesMode doesn't break any existing compatibility, as if XCM delivery fees are not enabled in the runtime, the behavior is the same as always, so I would say the best way to test SetFeesMode is by ensuring that none of the existing tests and scenarios are failing.

What do you think? Do you have any suggestions? Thanks in advance :)

xlc commented 3 months ago

I see. Ideally, we will want another mock runtime to test the code with different config. But I can merge this as it is fully backward compatible (i.e. all existing tests passes).