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: validate destinations for ReserveWithdraw and Teleport transfers #5660

Merged

Conversation

acatangiu
Copy link
Contributor

@acatangiu acatangiu commented Sep 10, 2024

This change adds the required validation for stronger UX guarantees when using InitiateReserveWithdraw or InitiateTeleport XCM instructions. Execution of the instructions will fail if the local chain is not configured to trust the "destination" or "reserve" chain as a reserve/trusted-teleporter for the provided "assets".

With this change, misuse of InitiateReserveWithdraw/InitiateTeleport fails on origin with no overall side-effects, rather than failing on destination (with side-effects to origin's assets issuance).

The commit also makes the same validations for pallet-xcm transfers, and adds regression tests.

@acatangiu acatangiu added the T6-XCM This PR/Issue is related to XCM. label Sep 10, 2024
@acatangiu acatangiu self-assigned this Sep 10, 2024
@acatangiu acatangiu requested a review from a team as a code owner September 10, 2024 11:20
@acatangiu acatangiu added the A4-needs-backport Pull request must be backported to all maintained releases. label Sep 10, 2024
@acatangiu acatangiu changed the title xcm-executor: validate target is configured as reserve when executing InitiateReserveWithdraw xcm-executor: InitiateReserveWithdraw only allowed to trusted reserves Sep 10, 2024
@acatangiu acatangiu force-pushed the validate-initiate-reserve-withdraw branch from 1e5b2ef to 3b92d33 Compare September 11, 2024 11:26
@acatangiu acatangiu changed the title xcm-executor: InitiateReserveWithdraw only allowed to trusted reserves xcm-executor: validate destinations for ReserveWithdraw and Teleport transfers Sep 11, 2024
…transfers

This change adds the required validation for stronger UX guarantees when using
`InitiateReserveWithdraw` or `InitiateTeleport` XCM instructions. Execution of
the instructions will fail if the local chain is not configured to trust the
"destination" or "reserve" chain as a reserve/trusted-teleporter for the provided
"assets".

With this change, misuse of `InitiateReserveWithdraw`/`InitiateTeleport` fails on
origin with no overall side-effects, rather than failing on destination (with
side-effects to origin's assets issuance).

The commit also makes the same validations for pallet-xcm transfers, and adds
regression tests.

Signed-off-by: Adrian Catangiu <[email protected]>
@acatangiu acatangiu force-pushed the validate-initiate-reserve-withdraw branch from 3b92d33 to 583fdd7 Compare September 11, 2024 11:29
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.

Good fix. This will break some messages if they're wrong right now, but it's better to break them than to allow them to lose funds :) so everything's good

@acatangiu acatangiu enabled auto-merge September 23, 2024 15:48
@acatangiu acatangiu requested a review from bkontur September 23, 2024 15:48
@acatangiu acatangiu added this pull request to the merge queue Sep 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 24, 2024
@bkontur bkontur enabled auto-merge September 24, 2024 22:17
@bkontur bkontur added this pull request to the merge queue Sep 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 25, 2024
@bkontur bkontur added this pull request to the merge queue Sep 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 25, 2024
@bkontur bkontur added this pull request to the merge queue Sep 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 25, 2024
@acatangiu acatangiu added this pull request to the merge queue Sep 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 25, 2024
@acatangiu acatangiu added this pull request to the merge queue Sep 25, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 25, 2024
…transfers (#5660)

This change adds the required validation for stronger UX guarantees when
using `InitiateReserveWithdraw` or `InitiateTeleport` XCM instructions.
Execution of the instructions will fail if the local chain is not
configured to trust the "destination" or "reserve" chain as a
reserve/trusted-teleporter for the provided "assets".

With this change, misuse of `InitiateReserveWithdraw`/`InitiateTeleport`
fails on origin with no overall side-effects, rather than failing on
destination (with side-effects to origin's assets issuance).

The commit also makes the same validations for pallet-xcm transfers, and
adds regression tests.

---------

Signed-off-by: Adrian Catangiu <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 25, 2024
@acatangiu acatangiu added this pull request to the merge queue Sep 25, 2024
Merged via the queue into paritytech:master with commit b5ac7a9 Sep 25, 2024
219 checks passed
@acatangiu acatangiu deleted the validate-initiate-reserve-withdraw branch September 25, 2024 16:54
@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2407:

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

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

github-actions bot pushed a commit that referenced this pull request Sep 25, 2024
…transfers (#5660)

This change adds the required validation for stronger UX guarantees when
using `InitiateReserveWithdraw` or `InitiateTeleport` XCM instructions.
Execution of the instructions will fail if the local chain is not
configured to trust the "destination" or "reserve" chain as a
reserve/trusted-teleporter for the provided "assets".

With this change, misuse of `InitiateReserveWithdraw`/`InitiateTeleport`
fails on origin with no overall side-effects, rather than failing on
destination (with side-effects to origin's assets issuance).

The commit also makes the same validations for pallet-xcm transfers, and
adds regression tests.

---------

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

Successfully created backport PR for stable2409:

EgorPopelyaev pushed a commit that referenced this pull request Sep 25, 2024
Backport #5660 into `stable2409` 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]>
@xlc
Copy link
Contributor

xlc commented Nov 10, 2024

This breaks existing test in ORML. The change is significant enough that it shouldn't be a patch version bump open-web3-stack/open-runtime-module-library#1011

also I can't find this PR in any release notes

@xlc xlc mentioned this pull request Nov 11, 2024
3 tasks
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants