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

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

Conversation

Agusrodri
Copy link
Contributor

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: paritytech/polkadot-sdk#3792

@xlc
Copy link
Member

xlc commented Jun 27, 2024

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
Copy link
Contributor Author

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
Copy link
Member

xlc commented Jun 29, 2024

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

@xlc xlc merged commit 23c8363 into open-web3-stack:master Jun 29, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants