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: DepositReserveAsset charges delivery fees from inner assets #3142

Conversation

acatangiu
Copy link
Contributor

This fix aims to solve an issue in Kusama that resulted in failed reserve asset transfers.

During multi-hop XCMs, like reserve asset transfers where the reserve is not the sender nor the destination, but a third remote chain, the origin is not available to pay for delivery fees out of their account directly, so delivery fees should be paid out of transferred assets.

This commit also adds an xcm-emulator regression test that validates this scenario is now working.

@acatangiu acatangiu added R0-silent Changes should not be mentioned in any release notes T6-XCM This PR/Issue is related to XCM. labels Jan 30, 2024
@acatangiu acatangiu self-assigned this Jan 30, 2024
@acatangiu acatangiu requested a review from a team as a code owner January 30, 2024 18:07
@acatangiu acatangiu marked this pull request as draft January 30, 2024 18:08
…sets

This fix aims to solve an issue in Kusama that resulted in failed
reserve asset transfers.

During multi-hop XCMs, like reserve asset transfers where the reserve
is not the sender nor the destination, but a third remote chain, the
origin is not available to pay for delivery fees out of their account
directly, so delivery fees should be paid out of transferred assets.

This commit also adds an xcm-emulator regression test that validates
this scenario is now working.

Signed-off-by: Adrian Catangiu <[email protected]>
@acatangiu acatangiu marked this pull request as ready for review January 30, 2024 18:11
@acatangiu acatangiu force-pushed the xcm-executor-quick-fix-multihop-messages branch from 48ea3ce to 4d26f38 Compare January 30, 2024 18:11
@franciscoaguirre franciscoaguirre added this pull request to the merge queue Jan 31, 2024
Merged via the queue into paritytech:master with commit 5354097 Jan 31, 2024
123 checks passed
@acatangiu acatangiu deleted the xcm-executor-quick-fix-multihop-messages branch January 31, 2024 11:35
franciscoaguirre added a commit that referenced this pull request Feb 1, 2024
…sets (#3142)

This fix aims to solve an issue in Kusama that resulted in failed
reserve asset transfers.

During multi-hop XCMs, like reserve asset transfers where the reserve is
not the sender nor the destination, but a third remote chain, the origin
is not available to pay for delivery fees out of their account directly,
so delivery fees should be paid out of transferred assets.

This commit also adds an xcm-emulator regression test that validates
this scenario is now working.

Signed-off-by: Adrian Catangiu <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
acatangiu added a commit to acatangiu/polkadot-sdk that referenced this pull request Dec 2, 2024
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 added a commit to acatangiu/polkadot-sdk that referenced this pull request Dec 2, 2024
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]>
github-merge-queue 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]>
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)
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
R0-silent Changes should not be mentioned in any release notes T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants