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-executor: take transport fee from transferred assets if necessary #4834

Merged

Conversation

acatangiu
Copy link
Contributor

@acatangiu acatangiu commented Jun 19, 2024

Description

Sending XCM messages to other chains requires paying a "transport fee". This can be paid either:

  • from origin local account if jit_withdraw = true,
  • taken from Holding register otherwise.

This currently works for following hops/scenarios:

  1. On destination no transport fee needed (only sending costs, not receiving),
  2. Local/originating chain: just set JIT=true and fee will be paid from signed account,
  3. Intermediary hops - only if intermediary is acting as reserve between two untrusted chains (aka only for DepositReserveAsset instruction) - this was fixed in xcm-executor: DepositReserveAsset charges delivery fees from inner assets #3142

But now we're seeing more complex asset transfers that are mixing reserve transfers with teleports depending on the involved chains.

Example

E.g. transferring DOT between Relay and parachain, but through AH (using AH instead of the Relay chain as parachain's DOT reserve).

In the Parachain --1--> AssetHub --2--> Relay scenario, DOT has to be reserve-withdrawn in leg 1, then teleported in leg 2.
On the intermediary hop (AssetHub), InitiateTeleport fails to send onward message because of missing transport fees. We also can't rely on jit_withdraw because the original origin is lost on the way, and even if it weren't we can't rely on the user having funded accounts on each hop along the way.

Solution/Changes

  • Charge the transport fee in the executor from the transferred assets (if available),
  • Only charge from transferred assets if JIT_WITHDRAW was not set,
  • Only charge from transferred assets if unless using XCMv5 PayFees where we do not have this problem.

Testing

Added regression tests in emulated transfers.

Fixes #4832
Fixes #6637

@acatangiu acatangiu added the T6-XCM This PR/Issue is related to XCM. label Jun 19, 2024
@acatangiu acatangiu self-assigned this Jun 19, 2024
@acatangiu acatangiu requested a review from a team as a code owner June 19, 2024 12:29
@xlc
Copy link
Contributor

xlc commented Jun 19, 2024

we shouldn't touch xcm-executor without open discussions (i.e. RFC process)
we are going to remote JIT mode so we shouldn't touch it
there needs a way for user to limit max fee so we can't do this

@acatangiu
Copy link
Contributor Author

we shouldn't touch xcm-executor without open discussions (i.e. RFC process)

I agree we can go through RFC process for this (I assumed it is ok even without RFC since it enables new scenarios without breaking any existing ones).

there needs a way for user to limit max fee so we can't do this

I don't agree, the proposed change is only used in scenarios not possible before (nobody is doing this yet) - when they'll try to do it, there are only two options (in current XCM) or three with XCMv5:

  1. (without this PR) transfers will fail somewhere along the way - leaving all funds stuck/trapped in some intermediary hop
  2. (with this PR) transfers work, fees (on intermediary hop only) are taken from transferred assets, there is no way to limit max fee in the XCM program, but with the dry-run API, the fee can be inspected beforehand (shown in Transfer UI) and either accepted by the user or not.
  3. wait for XCMv5, use new PayFees - requires all chains involved to be upgraded to XCMv5 (so realistically another year from now 😢 )

From a practical point of view, option 2 will enable needed scenarios in the Community while still being able to control fee UX from the higher levels (UI).

@xlc
Copy link
Contributor

xlc commented Jun 21, 2024

The issue of adding new features without RFC is that we can't easily change/remove it in future without a breaking change. So we need to get it right. We have observed such case in past and I want to make sure we don't repeat the same mistake.

I get we want to get features out and don't want to wait forever (everything it is taking too long and we need to address it somehow). However, I need to point it out with the option 2, there is a non zero chance people will lose funds in corner cases. e.g. if the fee payment mechanism is powered by a swap action and there could be someone doing market manipulation to take nearly all the transferred amount as fee and steal funds. This is a security risk and much worse than just have people funds stuck in limbo, which can be rescued via governance action.

Such risk maybe tolerable (e.g. the hop/dest chain are not using swap for fee payment) but I need to make sure all the concerns are discussed and addressed.

@acatangiu
Copy link
Contributor Author

closing this - will be cleanly supported using #5876 and #5420 in XCMv5

@acatangiu
Copy link
Contributor Author

@xlc @seadanda @franciscoaguirre

Using existing XCMv3 and XCMv4, we currently do not support DOT (or KSM for Kusama) transfers from a Parachain to the Relay Chain while using AH as reserve. We need this PR to make it possible. See #6637

XCMv5 supports it nicely.

I want your input considering the XCMv5 timeline (released in SDK this year, integrated in Relay and Sys Chains Q1 2025), and the Minimal Relay migration.

We have three options:

  1. require everyone to upgrade to XCMv5 before migration (so that they can switch to AH as DOT/KSM reserve before migration),
  2. have everyone keep DOT/KSM reserve on Relay, but coordinate that they switch their reserve (chain, tooling and everything) atomically with migration,
  3. merge this PR, get it integrated in fellowship runtimes and parachains can switch to AH as DOT/KSM reserve without needing to also upgrade to XCMv5.

Note: option 2 is practically impossible, bound to fail in practice, so either 1 or 3.

WDYT?

@xlc
Copy link
Contributor

xlc commented Nov 28, 2024

Upgrade to XCM v5 requires upgrade to latest polkadot-sdk, which could be a lot of work for some teams that are many versions behind. So I can feel there will be a lot of push-back from teams about option 1.
I guess option 3 it is.

@acatangiu acatangiu force-pushed the executor-take-fee-from-transfer branch from 0bc841c to 3a6650f Compare December 2, 2024 19:44
This can be paid either:
- from `origin` local account if `jit_withdraw = true`,
- taken from Holding register otherwise.

This currently works for following hops/scenarios:
1. On destination no transport fee needed (only sending costs, not receiving),
2. Local/originating chain: just set JIT=true and fee will be paid from signed account,
3. Intermediary hops - only if intermediary is acting as reserve between two untrusted
chains (aka only for `DepositReserveAsset` instruction) - this was fixed in
paritytech#3142

But now we're seeing more complex asset transfers that are mixing reserve transfers
with teleports depending on the involved chains.

E.g. transferring DOT between Relay and parachain, but through AH (using AH instead
of the Relay chain as parachain's DOT reserve).

In the `Parachain --1--> AssetHub --2--> Relay` scenario, DOT has to be reserve-withdrawn
in leg `1`, then teleported in leg `2`.
On the intermediary hop (AssetHub), `InitiateTeleport` fails to send onward message because
of missing transport fees. We also can't rely on `jit_withdraw` because the original origin
is lost on the way, and even if it weren't we can't rely on the user having funded accounts
on each hop along the way.

- Charge the transport fee in the executor from the transferred assets (if available),
- Only charge from transferred assets if JIT_WITHDRAW was not set,
- Only charge from transferred assets if Holding doesn't already contain enough (other)
assets to pay for the transport fee.

Added regression tests in emulated transfers.

Fixes paritytech#4832

Signed-off-by: Adrian Catangiu <[email protected]>
@acatangiu acatangiu force-pushed the executor-take-fee-from-transfer branch from 3a6650f to 2ea6aba Compare December 2, 2024 19:47
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

Looks good, I would just rename everywhere to delivery_fees since it seems to be the winning term and we shouldn't use multiple terms

polkadot/xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
polkadot/xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
prdoc/pr_4834.prdoc Outdated Show resolved Hide resolved
prdoc/pr_4834.prdoc Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team December 4, 2024 22:53
Comment on lines +360 to +362
// Send over assets and unspent fees, XCM delivery fee will be charged from
// here.
assets: Wild(AllCounted(2)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, should work for us!

@acatangiu acatangiu added this pull request to the merge queue Dec 9, 2024
@acatangiu acatangiu added the A4-needs-backport Pull request must be backported to all maintained releases. label Dec 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 9, 2024
@acatangiu acatangiu enabled auto-merge December 9, 2024 11:52
@acatangiu acatangiu added this pull request to the merge queue Dec 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 9, 2024
@acatangiu acatangiu enabled auto-merge December 9, 2024 12:49
@acatangiu acatangiu added this pull request to the merge queue Dec 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 9, 2024
@acatangiu acatangiu added this pull request to the merge queue Dec 9, 2024
Merged via the queue into paritytech:master with commit e79fd2b Dec 9, 2024
197 of 199 checks passed
@acatangiu acatangiu deleted the executor-take-fee-from-transfer branch December 9, 2024 20:56
@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2407:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-4834-to-stable2407
git worktree add --checkout .worktree/backport-4834-to-stable2407 backport-4834-to-stable2407
cd .worktree/backport-4834-to-stable2407
git reset --hard HEAD^
git cherry-pick -x e79fd2bb9926be7d94be0c002676fca57b6116bf
git push --force-with-lease

@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2409:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-4834-to-stable2409
git worktree add --checkout .worktree/backport-4834-to-stable2409 backport-4834-to-stable2409
cd .worktree/backport-4834-to-stable2409
git reset --hard HEAD^
git cherry-pick -x e79fd2bb9926be7d94be0c002676fca57b6116bf
git push --force-with-lease

github-actions bot pushed a commit that referenced this pull request Dec 9, 2024
…#4834)

# Description

Sending XCM messages to other chains requires paying a "transport fee".
This can be paid either:
- from `origin` local account if `jit_withdraw = true`,
- taken from Holding register otherwise.

This currently works for following hops/scenarios:
1. On destination no transport fee needed (only sending costs, not
receiving),
2. Local/originating chain: just set JIT=true and fee will be paid from
signed account,
3. Intermediary hops - only if intermediary is acting as reserve between
two untrusted chains (aka only for `DepositReserveAsset` instruction) -
this was fixed in #3142

But now we're seeing more complex asset transfers that are mixing
reserve transfers with teleports depending on the involved chains.

# Example

E.g. transferring DOT between Relay and parachain, but through AH (using
AH instead of the Relay chain as parachain's DOT reserve).

In the `Parachain --1--> AssetHub --2--> Relay` scenario, DOT has to be
reserve-withdrawn in leg `1`, then teleported in leg `2`.
On the intermediary hop (AssetHub), `InitiateTeleport` fails to send
onward message because of missing transport fees. We also can't rely on
`jit_withdraw` because the original origin is lost on the way, and even
if it weren't we can't rely on the user having funded accounts on each
hop along the way.

# Solution/Changes

- Charge the transport fee in the executor from the transferred assets
(if available),
- Only charge from transferred assets if JIT_WITHDRAW was not set,
- Only charge from transferred assets if unless using XCMv5 `PayFees`
where we do not have this problem.

# Testing

Added regression tests in emulated transfers.

Fixes #4832
Fixes #6637

---------

Signed-off-by: Adrian Catangiu <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
(cherry picked from commit e79fd2b)
@paritytech-cmd-bot-polkadot-sdk

Successfully created backport PR for stable2412:

EgorPopelyaev pushed a commit that referenced this pull request Dec 11, 2024
Backport #4834 into `stable2412` from acatangiu.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

---------

Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Ank4n pushed a commit that referenced this pull request Dec 15, 2024
…#4834)

# Description

Sending XCM messages to other chains requires paying a "transport fee".
This can be paid either:
- from `origin` local account if `jit_withdraw = true`,
- taken from Holding register otherwise.

This currently works for following hops/scenarios:
1. On destination no transport fee needed (only sending costs, not
receiving),
2. Local/originating chain: just set JIT=true and fee will be paid from
signed account,
3. Intermediary hops - only if intermediary is acting as reserve between
two untrusted chains (aka only for `DepositReserveAsset` instruction) -
this was fixed in #3142

But now we're seeing more complex asset transfers that are mixing
reserve transfers with teleports depending on the involved chains.

# Example

E.g. transferring DOT between Relay and parachain, but through AH (using
AH instead of the Relay chain as parachain's DOT reserve).

In the `Parachain --1--> AssetHub --2--> Relay` scenario, DOT has to be
reserve-withdrawn in leg `1`, then teleported in leg `2`.
On the intermediary hop (AssetHub), `InitiateTeleport` fails to send
onward message because of missing transport fees. We also can't rely on
`jit_withdraw` because the original origin is lost on the way, and even
if it weren't we can't rely on the user having funded accounts on each
hop along the way.

# Solution/Changes

- Charge the transport fee in the executor from the transferred assets
(if available),
- Only charge from transferred assets if JIT_WITHDRAW was not set,
- Only charge from transferred assets if unless using XCMv5 `PayFees`
where we do not have this problem.

# Testing

Added regression tests in emulated transfers.

Fixes #4832
Fixes #6637

---------

Signed-off-by: Adrian Catangiu <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A4-needs-backport Pull request must be backported to all maintained releases. T6-XCM This PR/Issue is related to XCM.
Projects
Status: Done
7 participants