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

RFC: add complex asset transfers instructions #54

Closed
wants to merge 2 commits into from

Conversation

acatangiu
Copy link
Contributor

@acatangiu acatangiu commented Apr 17, 2024

Summary

This RFC proposes new instructions that provide a way to initiate asset transfers which transfer multiple types (teleports, local-reserve, destination-reserve) of assets, on remote chains using XCM alone.

The currently existing instructions are too opinionated and force each XCM asset transfer to a single transfer type (teleport, local-reserve, destination-reserve). This results in inability to combine different types of transfers in single transfer which results in overall poor UX when trying to move assets across chains.

Example usage

    // XCM to be executed locally
    let xcm = Xcm::<penpal_runtime::RuntimeCall>(vec![
        // Withdraw both ROCs and PENs from origin account
        WithdrawAsset(assets.clone().into()),
        // ROCs are reserve-withdrawn on AHR
        DestinationReserveWithdrawAssets(rocs.into()),
        // PENs are teleported to AHR
        TeleportTransferAssets(pens.into()),
        // Execute the transfers while paying remote fees with ROCs
        ExecuteAssetTransfers {
            dest: local_asset_hub,
            remote_fees: Some(AssetFilter::Wild(AllOf { id: rocs_id.into(), fun })),
            remote_xcm: xcm_on_ahr,
        },
    ]);

Proof-of-Concept

I also have a proof-of-concept implementation and a proof-of-concept asset transfer test, transferring 2 different types of assets across 3 chains.

@acatangiu acatangiu self-assigned this Apr 17, 2024
@acatangiu acatangiu changed the title RFC-XX: add complex asset transfers instructions RFC-54: add complex asset transfers instructions Apr 17, 2024
@acatangiu acatangiu changed the title RFC-54: add complex asset transfers instructions RFC: add complex asset transfers instructions Apr 17, 2024
@acatangiu
Copy link
Contributor Author

cc @mrshiposha @xlc

@acatangiu acatangiu requested a review from bkontur April 17, 2024 16:07
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Apr 24, 2024
…estination (#4260)

Change `transfer_assets_using_type()` to not assume `DepositAssets` as
the intended use of the assets on the destination.

Instead provides the caller with the ability to specify custom XCM that
be executed on `dest` chain as the last step of the transfer, thus
allowing custom usecases for the transferred assets. E.g. some are
used/swapped/etc there, while some are sent further to yet another
chain.

Note: this is a follow-up on
#3695, bringing in an API
change for `transfer_assets_using_type()`. This is ok as the previous
version has not been yet released. Thus, its first release will include
the new API proposed by this PR.

This allows usecases such as:
https://forum.polkadot.network/t/managing-sas-on-multiple-reserve-chains-for-same-asset/7538/4

BTW: all this pallet-xcm asset transfers code will be massively reduced
once we have polkadot-fellows/xcm-format#54

---------

Signed-off-by: Adrian Catangiu <adrian@parity.io>
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.

Great suggestion! My only comment is about naming and the number of instructions. I want to make sure these instructions are simple to reason about. Right now, because of the names, it's hard to figure out what they do if I don't look at the documentation.

There was the option of having only one huge instruction.
When you only care about a specific transfer type, like when doing a simple teleport, you would have to add a bunch of Nones.
I prefer the two-step approach you described, first configure the asset transfer and then execute it, because you don't want to add a bunch of Nones all the time.

That said, I'd have only one previous instruction PrepareAssetTransfer that receives a vector of a TransferType enum. This instruction sets asides assets for teleports, local reserve and destination reserve.

enum Instruction {
  ...
  PrepareAssetTransfer(Vec<AssetTransferType>),
  ...
}

enum AssetTransferType {
  Teleport(AssetFilter),
  LocalReserve(AssetFilter),
  DestinationReserve(AssetFilter),
}

I think this strikes a good balance between having just one instruction or a lot of them. It makes XCM programs easier to reason about.
What do you think?

PS: We could also probably do this in the one big instruction, further coupling concepts that work together. It would always require specifying a vector of AssetTransferType, even if it has only one element. That makes it good UX even when just doing a simple teleport.

enum Instruction {
  ...
  InitiateAssetTransfer {
    destination: Location,
    assets: Vec<AssetTransferType>,
    remote_fees: Option<AssetFilter>,
    remote_xcm: Xcm<()>,
  },
  ...
}

rest of the assets are handled by subsequent instructions, thus allowing
[single asset buy execution](https://github.com/paritytech/polkadot-sdk/issues/2423).

Open Q: for `remote_fees == None`, should we append `UnpaidExecution` or do nothing?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to add UnpaidExecution here. It would be very useful as right now system parachains have to teleport with unpaid execution and there are currently no instructions that handle this use-case correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can imagine unpaid asset transfers, but haven't seen any in practice. Do you know of any practical usecases so we can make sure we properly support them?

@acatangiu
Copy link
Contributor Author

PS: We could also probably do this in the one big instruction, further coupling concepts that work together. It would always require specifying a vector of AssetTransferType, even if it has only one element. That makes it good UX even when just doing a simple teleport.

enum Instruction {
  ...
  InitiateAssetTransfer {
    destination: Location,
    assets: Vec<AssetTransferType>,
    remote_fees: Option<AssetFilter>,
    remote_xcm: Xcm<()>,
  },
  ...
}

Yes, I like this ^


### What is the impact of not doing this?

Current multi-chain transfers are forced to happen in multiple programs per asset per "hop", resulting in very poor UX.
Copy link

Choose a reason for hiding this comment

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

For someone not having done a lot of XCM, one can only imagine why this is a very poor UX. What problems does a developer actually face with the current design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The developer simply can not write an XCM program that can, for example, transfer both DOT (local reserve deposit) and HDX (destination reserve withdraw) from AssetHub to HydraDX. It is simply not possible with the current instruction set.

The only XCM instructions available currently (XCMv4) for transferring assets are DepositReserveAsset, InitiateReserveWithdraw and InitiateTeleport instructions. They initiate asset transfers on execution, but they are opinionated in the type of transfer to use. Combining them is not possible, because as a result of their individual execution, a message containing a ClearOrigin instruction is sent to the destination chain (for security reasons - more details available in their docs), making subsequent transfers impossible.

@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

@franciscoaguirre franciscoaguirre mentioned this pull request May 24, 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/managing-sas-on-multiple-reserve-chains-for-same-asset/7538/7

@acatangiu
Copy link
Contributor Author

Unfortunately, following the repo migration to the new org, I lost write access to the PR (upstream) branch. I've opened the PR from a fork branch here #63

Closing this one.

@acatangiu acatangiu closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants