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: add new flexible transfer_assets() call/extrinsic #2388

Conversation

acatangiu
Copy link
Contributor

@acatangiu acatangiu commented Nov 17, 2023

Motivation (+testing)

Enable easy ForeignAssets transfers using pallet-xcm

We had just previously added capabilities to teleport fees during reserve-based transfers, but what about reserve-transferring fees when needing to teleport some non-fee asset?

This PR aligns everything under either explicit reserve-transfer, explicit teleport, or this new flexible transfer_assets() which can mix and match as needed with fewer artificial constraints imposed to the user.

This will enable, for example, a (non-system) parachain to teleport their ForeignAssets assets to AssetHub while using DOT to pay fees.
(the assets are teleported - as foreign assets should from their owner chain - while DOT used for fees can only be reserve-based transferred between said parachain and AssetHub).

Added xcm-emulator tests for this scenario ^.

Description

Reverts (limited_)reserve_transfer_assets to only allow reserve-based transfers for all assets including fees.

Similarly (limited_)teleport_assets only allows teleports for all assets including fees.

For complex combinations of asset transfers where assets and fees may have different reserves or different reserve/teleport trust configurations, users can use the newly added transfer_assets() extrinsic which is more flexible in allowing more complex scenarios.

assets (excluding fees) must have same reserve location or otherwise be teleportable to dest.
No limitations imposed on fees.

  • for local reserve: transfer assets to sovereign account of destination chain and forward a notification XCM to dest to mint and deposit reserve-based assets to beneficiary.
  • for destination reserve: burn local assets and forward a notification to dest chain to withdraw the reserve assets from this chain's sovereign account and deposit them to beneficiary.
  • for remote reserve: burn local assets, forward XCM to reserve chain to move reserves from this chain's SA to dest chain's SA, and forward another XCM to dest to mint and deposit reserve-based assets to beneficiary.
  • for teleports: burn local assets and forward XCM to dest chain to mint/teleport assets and deposit them to beneficiary.

Review notes

Only around 500 lines are prod code (see pallet_xcm/src/lib.rs), the rest of the PR is new tests and improving existing tests.

@acatangiu acatangiu added T6-XCM This PR/Issue is related to XCM. T2-pallets This PR/Issue is related to a particular pallet. labels Nov 17, 2023
@acatangiu acatangiu self-assigned this Nov 17, 2023
@acatangiu acatangiu requested a review from a team as a code owner November 17, 2023 17:26
@acatangiu
Copy link
Contributor Author

acatangiu commented Nov 21, 2023

@KiChjang @franciscoaguirre PTAL and let me know if there's arguments against this - if we are aligned, I will then continue with adding the missing pieces to the PR.

@franciscoaguirre
Copy link
Contributor

I like not requiring the user to think about teleports or reserve backed transfers, while still keeping the other extrinsics for more specific use-cases and for devs that have to think about this stuff

@acatangiu acatangiu force-pushed the xcm-emulator-foreign-assets-transfer-tests branch from 499f5ce to 4b8fad6 Compare November 28, 2023 11:58
@acatangiu acatangiu requested review from a team November 28, 2023 11:58
@acatangiu acatangiu marked this pull request as draft November 28, 2023 12:23
@paritytech paritytech deleted a comment from paritytech-cicd-pr Nov 29, 2023
Reverts `(limited_)reserve_transfer_assets` to only allow reserve-based transfers
for all `assets` including fees.
Similarly `(limited_)teleport_assets` only allows teleports for all `assets`
including feed.

For complex combinations of asset transfers where assets and fees may have different
reserves or different reserve/teleport trust configurations, users can use the newly
added `transfer_assets()` extrinsic which is more flexible in allowing more complex
scenarios.

`assets` (excluding `fees`) must have same reserve location or otherwise be teleportable
to `dest`.
No limitations imposed on `fees`.

- for local reserve: transfer assets to sovereign account of destination chain and
  forward a notification XCM to `dest` to mint and deposit reserve-based assets to
  `beneficiary`.
- for destination reserve: burn local assets and forward a notification to `dest` chain
  to withdraw the reserve assets from this chain's sovereign account and deposit them
  to `beneficiary`.
- for remote reserve: burn local assets, forward XCM to reserve chain to move reserves
  from this chain's SA to `dest` chain's SA, and forward another XCM to `dest` to mint
  and deposit reserve-based assets to `beneficiary`.
- for teleports: burn local assets and forward XCM to `dest` chain to mint/teleport
  assets and deposit them to `beneficiary`.
…enpal assets and Asset Hub as foreign assets
@acatangiu acatangiu marked this pull request as ready for review November 30, 2023 06:12
@acatangiu acatangiu force-pushed the xcm-emulator-foreign-assets-transfer-tests branch from 4e2a22c to fbc22b1 Compare November 30, 2023 06:12
@paritytech paritytech deleted a comment from paritytech-cicd-pr Nov 30, 2023
@acatangiu acatangiu merged commit e7651cf into paritytech:master Dec 6, 2023
116 of 118 checks passed
@acatangiu acatangiu added the R1-breaking_change This PR introduces a breaking change and should be highlighted in the upcoming release. label Dec 6, 2023
ordian added a commit that referenced this pull request Dec 11, 2023
* tsv-disabling: (155 commits)
  Fix failing rc-automation GHA (#2648)
  [ci] Return CI_IMAGE variable (#2647)
  Support querying peer reputation (#2392)
  [ci] Update rust to 1.74 (#2545)
  Relax approval requirements on CI files (#2564)
  Added AllSiblingSystemParachains matcher to be used at a parachain level (#2422)
  Improve polkadot sdk docs (#2631)
  Bridges subtree update (#2602)
  pallet-xcm: add new flexible `transfer_assets()` call/extrinsic (#2388)
  [ci] Move rust-features.sh script to .gitlab folder (#2630)
  Bump parity-db from 0.4.10 to 0.4.12 (#2635)
  sp-core: Rename VrfOutput to VrfPreOutput (#2534)
  chore: fix typo (#2596)
  Bump tracing-core from 0.1.31 to 0.1.32 (#2618)
  chore: fixed std wasm build of xcm (#2535)
  Fix PRdoc that have been previously drafted with older schema (#2623)
  Github Workflow migrations (#1574)
  Bridges update subtree (#2625)
  PVF: Add Secure Validator Mode (#2486)
  statement-distribution: Add tests for incoming acknowledgements (#2498)
  ...
ordian added a commit that referenced this pull request Dec 15, 2023
* tsv-disabling: (155 commits)
  Fix failing rc-automation GHA (#2648)
  [ci] Return CI_IMAGE variable (#2647)
  Support querying peer reputation (#2392)
  [ci] Update rust to 1.74 (#2545)
  Relax approval requirements on CI files (#2564)
  Added AllSiblingSystemParachains matcher to be used at a parachain level (#2422)
  Improve polkadot sdk docs (#2631)
  Bridges subtree update (#2602)
  pallet-xcm: add new flexible `transfer_assets()` call/extrinsic (#2388)
  [ci] Move rust-features.sh script to .gitlab folder (#2630)
  Bump parity-db from 0.4.10 to 0.4.12 (#2635)
  sp-core: Rename VrfOutput to VrfPreOutput (#2534)
  chore: fix typo (#2596)
  Bump tracing-core from 0.1.31 to 0.1.32 (#2618)
  chore: fixed std wasm build of xcm (#2535)
  Fix PRdoc that have been previously drafted with older schema (#2623)
  Github Workflow migrations (#1574)
  Bridges update subtree (#2625)
  PVF: Add Secure Validator Mode (#2486)
  statement-distribution: Add tests for incoming acknowledgements (#2498)
  ...
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-5-0/5358/1

bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 10, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 10, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 10, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 11, 2024
Agusrodri pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Jan 11, 2024
…tytech#2388)

We had just previously added capabilities to teleport fees during
reserve-based transfers, but what about reserve-transferring fees when
needing to teleport some non-fee asset?

This PR aligns everything under either explicit reserve-transfer,
explicit teleport, or this new flexible `transfer_assets()` which can
mix and match as needed with fewer artificial constraints imposed to the
user.

This will enable, for example, a (non-system) parachain to teleport
their `ForeignAssets` assets to AssetHub while using DOT to pay fees.
(the assets are teleported - as foreign assets should from their owner
chain - while DOT used for fees can only be reserve-based transferred
between said parachain and AssetHub).

Added `xcm-emulator` tests for this scenario ^.

Reverts `(limited_)reserve_transfer_assets` to only allow reserve-based
transfers for all `assets` including fees.

Similarly `(limited_)teleport_assets` only allows teleports for all
`assets` including fees.

For complex combinations of asset transfers where assets and fees may
have different reserves or different reserve/teleport trust
configurations, users can use the newly added `transfer_assets()`
extrinsic which is more flexible in allowing more complex scenarios.

`assets` (excluding `fees`) must have same reserve location or otherwise
be teleportable to `dest`.
No limitations imposed on `fees`.

- for local reserve: transfer assets to sovereign account of destination
chain and forward a notification XCM to `dest` to mint and deposit
reserve-based assets to `beneficiary`.
- for destination reserve: burn local assets and forward a notification
to `dest` chain to withdraw the reserve assets from this chain's
sovereign account and deposit them to `beneficiary`.
- for remote reserve: burn local assets, forward XCM to reserve chain to
move reserves from this chain's SA to `dest` chain's SA, and forward
another XCM to `dest` to mint and deposit reserve-based assets to
`beneficiary`.
- for teleports: burn local assets and forward XCM to `dest` chain to
mint/teleport assets and deposit them to `beneficiary`.

Only around 500 lines are prod code (see `pallet_xcm/src/lib.rs`), the
rest of the PR is new tests and improving existing tests.

---------

Co-authored-by: command-bot <>
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 12, 2024
bkontur added a commit to bkontur/runtimes that referenced this pull request Jan 12, 2024
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/8

@brenzi
Copy link
Contributor

brenzi commented Feb 14, 2024

This works for our parachain 👍
However: can it be that teleporting our native token to asset hub only works if the order of assets in the transferAsset call is (TEER, ROC, feeAsset = 1)?
I first tried (ROC, TEER, feeAsset=0) but that didn't work

@joepetrowski
Copy link
Contributor

I will leave it to @acatangiu to answer, but that does seem strange to me.

@acatangiu
Copy link
Contributor Author

acatangiu commented Feb 15, 2024

This works for our parachain 👍 However: can it be that teleporting our native token to asset hub only works if the order of assets in the transferAsset call is (TEER, ROC, feeAsset = 1)? I first tried (ROC, TEER, feeAsset=0) but that didn't work

Like the other extrinsics, transferAsset() takes the assets to transfer through a Box<VersionedMultiAssets> parameter.

MultiAssets object internally sorts the list of assets (I'm not sure where this requirement to be sorted comes from, maybe required for encoding/decoding?).

So my guess is you're doing something like:

let assets: MultiAssets = vec![TEER, ROC].into();    // <-- this works
let fee_idx = 1;                                     // ok

let assets: MultiAssets = vec![ROC, TEER].into();    // <-- still works, but `fee_idx` should still be `1` because of sorting :(
let fee_idx = 0; // <-- reading this looks like it's pointing to ROC, but because of the sorting done by `.into()`, TEER is actually idx 0 and ROC is idx 1

(local assets will always go first because of parents=0 vs ROC which has parents=1)

and I know... this is annoying and bad API design - we could change extrinsic to take actual fee asset instead of index, but would be a breaking change..

Also here is an example helper function I used in tests to get the right index for transfers.

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…tytech#2388)

# Motivation (+testing)

### Enable easy `ForeignAssets` transfers using `pallet-xcm` 

We had just previously added capabilities to teleport fees during
reserve-based transfers, but what about reserve-transferring fees when
needing to teleport some non-fee asset?

This PR aligns everything under either explicit reserve-transfer,
explicit teleport, or this new flexible `transfer_assets()` which can
mix and match as needed with fewer artificial constraints imposed to the
user.

This will enable, for example, a (non-system) parachain to teleport
their `ForeignAssets` assets to AssetHub while using DOT to pay fees.
(the assets are teleported - as foreign assets should from their owner
chain - while DOT used for fees can only be reserve-based transferred
between said parachain and AssetHub).

Added `xcm-emulator` tests for this scenario ^.

# Description

Reverts `(limited_)reserve_transfer_assets` to only allow reserve-based
transfers for all `assets` including fees.

Similarly `(limited_)teleport_assets` only allows teleports for all
`assets` including fees.
    
For complex combinations of asset transfers where assets and fees may
have different reserves or different reserve/teleport trust
configurations, users can use the newly added `transfer_assets()`
extrinsic which is more flexible in allowing more complex scenarios.

`assets` (excluding `fees`) must have same reserve location or otherwise
be teleportable to `dest`.
No limitations imposed on `fees`.

- for local reserve: transfer assets to sovereign account of destination
chain and forward a notification XCM to `dest` to mint and deposit
reserve-based assets to `beneficiary`.
- for destination reserve: burn local assets and forward a notification
to `dest` chain to withdraw the reserve assets from this chain's
sovereign account and deposit them to `beneficiary`.
- for remote reserve: burn local assets, forward XCM to reserve chain to
move reserves from this chain's SA to `dest` chain's SA, and forward
another XCM to `dest` to mint and deposit reserve-based assets to
`beneficiary`.
- for teleports: burn local assets and forward XCM to `dest` chain to
mint/teleport assets and deposit them to `beneficiary`.

## Review notes

Only around 500 lines are prod code (see `pallet_xcm/src/lib.rs`), the
rest of the PR is new tests and improving existing tests.

---------

Co-authored-by: command-bot <>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-transfer-reserve-non-reserve-assets-in-a-single-xcm/2808/8

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-user-and-developer-experience-improvements/4511/21

@acatangiu acatangiu deleted the xcm-emulator-foreign-assets-transfer-tests branch June 25, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R1-breaking_change This PR introduces a breaking change and should be highlighted in the upcoming release. T2-pallets This PR/Issue is related to a particular pallet. T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants