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

[pallet-xcm] fix transport fees for remote reserve transfers #3792

Conversation

girazoki
Copy link
Contributor

Currently transfer_assets from pallet-xcm covers 4 main different transfer types:

  • localReserve
  • DestinationReserve
  • Teleport
  • RemoteReserve

For the first three, the local execution and the remote message sending are separated, and fees are deducted in pallet-xcm itself:

Self::charge_fees(origin.clone(), price).map_err(|error| {
.

For the 4th case RemoteReserve, pallet-xcm is still relying on the xcm-executor itself to send the message (through the initiateReserveWithdraw instruction). In this case, if delivery fees need to be charged, it is not possible to do so because the jit_withdraw mode has not being set.

This PR proposes to still use the initiateReserveWithdraw but prepending a setFeesMode { jit_withdraw: true } to make sure delivery fees can be paid.

A test-case is also added to present the aforementioned case

@girazoki girazoki requested a review from a team as a code owner March 22, 2024 09:57
@acatangiu acatangiu added the T6-XCM This PR/Issue is related to XCM. label Mar 22, 2024
@acatangiu
Copy link
Contributor

Good catch! Seems this was missed in emulated tests because Penpal still has free message delivery 🤦

Let me add an emulated transfer regression test as well 👍

@acatangiu
Copy link
Contributor

In the meantime, can you please add a prdoc for documenting the fix in the changelog?
Example(s) here: https://github.com/paritytech/polkadot-sdk/blob/master/prdoc/pr_3740.prdoc

acatangiu added a commit that referenced this pull request Mar 22, 2024
Add `SetFeesMode { jit_withdraw: true }` before
`InitiateReserveWithdraw` instruction to allow paying JIT fees in the
executor.

This is backport of #3792
@girazoki
Copy link
Contributor Author

In the meantime, can you please add a prdoc for documenting the fix in the changelog? Example(s) here: https://github.com/paritytech/polkadot-sdk/blob/master/prdoc/pr_3740.prdoc

Absolutely!

@acatangiu acatangiu changed the title Add set fees mode initiate reserve withdraw pallet-xcm: fix transport fees for remote reserve transfers Mar 22, 2024
@acatangiu
Copy link
Contributor

I added regression tests PR (to merge with this PR): girazoki#1

@girazoki PTAL and let's merge those to this PR so this PR is complete with fix and regression tests.

@acatangiu acatangiu requested a review from a team March 22, 2024 14:01
@girazoki
Copy link
Contributor Author

Merged @acatangiu

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5621324

@girazoki girazoki changed the title pallet-xcm: fix transport fees for remote reserve transfers [pallet-xcm] fix transport fees for remote reserve transfers Mar 22, 2024
@girazoki
Copy link
Contributor Author

@acatangiu I dont know how to fix the prdoc failure now :(, do I need to put the title in between quotes?

@acatangiu acatangiu added this pull request to the merge queue Mar 22, 2024
Merged via the queue into paritytech:master with commit 9a04ebb Mar 22, 2024
128 of 133 checks passed
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Mar 24, 2024
…ech#3792)

Currently `transfer_assets` from pallet-xcm covers 4 main different
transfer types:
- `localReserve`
- `DestinationReserve`
- `Teleport`
- `RemoteReserve`

For the first three, the local execution and the remote message sending
are separated, and fees are deducted in pallet-xcm itself:
https://github.com/paritytech/polkadot-sdk/blob/3410dfb3929462da88be2da813f121d8b1cf46b3/polkadot/xcm/pallet-xcm/src/lib.rs#L1758.

For the 4th case `RemoteReserve`, pallet-xcm is still relying on the
xcm-executor itself to send the message (through the
`initiateReserveWithdraw` instruction). In this case, if delivery fees
need to be charged, it is not possible to do so because the
`jit_withdraw` mode has not being set.

This PR proposes to still use the `initiateReserveWithdraw` but
prepending a `setFeesMode { jit_withdraw: true }` to make sure delivery
fees can be paid.

A test-case is also added to present the aforementioned case

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
girazoki added a commit to moondance-labs/polkadot-sdk that referenced this pull request Mar 26, 2024
bkontur pushed a commit that referenced this pull request Mar 26, 2024
Add `SetFeesMode { jit_withdraw: true }` before
`InitiateReserveWithdraw` instruction to allow paying JIT fees in the
executor.

This is backport of #3792
bkontur pushed a commit that referenced this pull request Mar 26, 2024
Add `SetFeesMode { jit_withdraw: true }` before
`InitiateReserveWithdraw` instruction to allow paying JIT fees in the
executor.

This is backport of #3792
acatangiu added a commit that referenced this pull request Mar 26, 2024
… backport for 1.8 (#3839)

This is backport of #3792

Expected patches for:
- pallet-xcm

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
acatangiu added a commit that referenced this pull request Mar 26, 2024
… backport for 1.9 (#3840)

This is backport of #3792

Expected patches for:
- pallet-xcm

Co-authored-by: Adrian Catangiu <adrian@parity.io>
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants