Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-xcm: XCM assets reserve withdraw feature #5975

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

akru
Copy link

@akru akru commented Sep 7, 2022

Motivation

Historically pallet-xcm is basic Polkadot pallet for sending XCM, including assets manipulation functions like reserve_transfer_assets call. Unfortunately pallet-xcm miss function for getting reserved / transferred assets back.

For example, reserve_transfer_assets send multi-asset using TransferReserveAssets instruction, which process lock asset and notify destination parachain about.

let mut message = Xcm(vec![TransferReserveAsset { assets, dest, xcm }]);

But in pair to it, I believe, should exist a way for get these assets back using InitiateReserveWithdraw instruction, which burn synthetic asset on chain and send notification to destination for releasing locked asset.

Solution

This PR implements reserve_withdraw_asset in pair to reserve_transfer_assets, it uses InitiateReserveWithdraw XCVM instruction and can be used for sending reserved assets back. It release ecosystem projects of using multiple XCM pallets like xtokens / pallet-xcm: one for assets deposing and other for assets withdraw.

Applying this PR makes possible ecosystem projects to use pallet-xcm only for full XCM assets support.

@bkchr bkchr requested a review from KiChjang September 7, 2022 14:45
@KiChjang
Copy link
Contributor

KiChjang commented Sep 8, 2022

Adding an extrinsic to execute an XCM that includes InitiateReserveWithdraw doesn't simply allow the destination chain to send synthetic/derivative/bookkeeping assets back to the reserve, it also allows any two chains to send/receive said assets via an intermediary reserve chain.

We'll need to think thoroughly about the implications of such an extrinsic first to see whether it's a fit for the purposes of the XCM pallet.

@akru
Copy link
Author

akru commented Sep 9, 2022

Thank you for feedback @KiChjang, I believe if pallet-xcm includes reserve transfer (deposit) function then it should have reserve withdraw function.

Looking forward to your final decision.

/// - `dest`: Destination context for the assets. Will typically be `X2(Parent, Parachain(..))` to send
/// from parachain to parachain, or `X1(Parachain(..))` to send from relay to parachain.
Copy link
Member

@gavofyork gavofyork Sep 12, 2022

Choose a reason for hiding this comment

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

Suggested change
/// - `dest`: Destination context for the assets. Will typically be `X2(Parent, Parachain(..))` to send
/// from parachain to parachain, or `X1(Parachain(..))` to send from relay to parachain.
/// - `dest`: Destination context for the assets. Will typically be `(Parent, Parachain(..))` to claim
/// reserve assets from a sibling parachain, or `(Parent,)` to claim the reserve assets from the Relay chain.

@gavofyork
Copy link
Member

I would just expose do_reserve_withdraw_assets as reserve_withdraw_assets, rather than having two separate extrinsics.

@akru
Copy link
Author

akru commented Sep 12, 2022

@gavofyork thank you for fixes.

I just made do_* and call separately in manner of reserve_transfer_assets/limited_reserve_transfer_assets, it makes possible to provide 'limited' version of same call.

xcm/pallet-xcm/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +984 to +989
let mut message = Xcm(vec![
WithdrawAsset(assets),
InitiateReserveWithdraw {
assets: Wild(All),
reserve: dest,
xcm,
},
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This XCM here doesn't actually include any instruction that switches the derivative asset back to the reserve asset. In fact, it looks to me that you're in fact sending the derivative asset back to the reserve.

Copy link
Author

Choose a reason for hiding this comment

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

Of course, that’s the point, it should withdraw asset from reserve. Shortly user story is:

  1. Alice send some DOT from relay to parachain A using reserve_transfer call on relay side, call lock DOT on parachain account.
  2. Parachain A got notification and mint synthetic xDOT.
  3. Few moments later Alice decide to get DOT back and uses reserve_withdraw call on parachain side, burn xDOT.
  4. Relay got notification and release DOT to Alice account.

Copy link
Contributor

@KiChjang KiChjang Sep 14, 2022

Choose a reason for hiding this comment

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

Right, and this XCM here is step 3, and there isn't any instruction here that burns xDOT. Neither WithdrawAsset nor InitiateReserveWithdraw explicitly does so.

In fact, what it actually does is that it sends xDOT onwards to the reserve chain, and expecting it to withdraw xDOT locally.

Copy link
Author

Choose a reason for hiding this comment

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

In fact it burns assets as you can see here, for example: https://astar.subscan.io/extrinsic/1683493-9

But yes, it's not clear how InitiateReserveWithdraw instruction should works. Do you mind better command sentence here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The asset you sent over isn't a derivative; it's the native token of the parent chain. It does not prove to me that any of the instructions here had exchanged a derivative asset with its corresponding reserve asset.

Copy link
Author

Choose a reason for hiding this comment

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

The aim of this PR is to make possible return native token of parent chain back. Currently it's impossible using pallet-xcm, you known.

What's about is it derivative or not, I believe it becomes derivative when comes to parachain, technically parachain runtime mints synthetic asset according to locked amount of native asset on relay chain side.

Copy link
Contributor

@KiChjang KiChjang Sep 18, 2022

Choose a reason for hiding this comment

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

Okay, I just dug through Astar's code and found out where the miscommunication is happening -- Astar is using the same MultiLocation to represent both the reserve asset (DOT) and the derivative asset (xDOT), but when the configured AssetTransactor checks out an asset with a MultiLocation of ../Here, it would always understand it as the derivative asset. This may work locally on the parachain, but the major drawback here is that the parachain does not know how to distinguish between a reserve asset and its derivative asset using MultiLocation.

This is very unexpected, as my understanding of MultiLocations is that it should refer to one and only one location in consensus at any given time. If we were to push the file path system analogy even further, this is akin to being brought to two different files via the same path.

Your PR exploits this "location punning" in order to work, and honestly the "location punning" issue is a much bigger issue that we need to discuss as a community. I'll discuss internally about the issue we found here, and will come back with a proper action plan later.

Copy link
Author

Choose a reason for hiding this comment

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

I see, thank you very much for highlight this. Honestly that's the problem we had, for now is no way to understand just by MultiLocation is this asset derivative or not. We just expect from user that withdraw_transfer method will be used for derivatives only while reserve_transfer is used for reserve assets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, after some thought, I think this is what we should do:

Your code in this PR absolutely cannot contain the assumption that both the reserve and derivative asset shares the same MultiLocation. This is what the community had decided, and does not always hold true for every chain.

Thus, a proper implementation should then require the extrinsic to also take in a reserve_asset_location, so that when the XCM gets sent to the reserve chain, the WithdrawAsset step takes out the reserve asset instead of the derivative asset, opposite of what you have currently done in this PR.

In the meantime, I'm going to figure out just how and when exactly a derivative asset should be converted to its corresponding reserve asset. I had always assumed that InitiateReserveWithdraw would contain such a step, but it apparently does not. But assuming that it does work, your PR would still need to make the changes I've described above.

Does that make sense?

Copy link
Author

Choose a reason for hiding this comment

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

Got it, but I prefer to have clear design before starting to change code that works well for now. I feel that proposed changes will require to change XCM message arguments and come with XCMv{N} standard later. So, probably we should suspend this PR until XCM support will come.

@Polkadot-Forum
Copy link

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

https://forum.polkadot.network/t/asset-multilocation-punning/423/1

@akru
Copy link
Author

akru commented Sep 23, 2022

@KiChjang just to make summary of this PR high-level workflow discussion.

How reserve transfer should works:

  1. The Alice wan't to transfer asset(T) to Bob from parachain A to parachain B (T has MultiLocation = X);
  2. The Alice send extrinsic reserve_transfer_assets which precisely send XCM in form
let xcm = Xcm(vec![
    BuyExecution { fees, weight_limit },
    DepositAsset { assets: Wild(All), max_assets, beneficiary },
]);
let mut message = Xcm(vec![TransferReserveAsset { assets, dest, xcm }]);

where parachain B receives message:

Xcm(vec![
    ReserveAssetDeposited(assets),
    ClearOrigin,
    BuyExecution { fees, weight_limit: Limited(0) },
    DepositAsset { assets: Wild(All), max_assets, beneficiary },
]);
  1. This step is important, because the parachain B dont receives precisely same multi-asset as parachain A sent. I believe that anchor/reanchor wrappers in XCM executor is used for making different multi-asset location before send:

https://github.com/paritytech/polkadot/blob/master/xcm/xcm-executor/src/lib.rs#L314

  1. Reverse way is the same but uses InitiateReserveWithdraw opcode. XCM executor reanchor multi-location of asset before sending it back to parachain A.
let xcm = Xcm(vec![
                BuyExecution { fees, weight_limit },
                DepositAsset {
                    assets: Wild(All),
                    max_assets,
                    beneficiary,
                },
            ]);
            let message = Xcm(vec![
                WithdrawAsset(assets),
                InitiateReserveWithdraw {
                    assets: Wild(All),
                    reserve: dest,
                    xcm,
                },
            ]);

XCM message don't directly use user sent multi-asset values, XCM executor converts it before sent:

https://github.com/paritytech/polkadot/blob/master/xcm/xcm-executor/src/lib.rs#L415

So, I guess your concern should be applied not for this PR because it uses XCM executor anchor/reanchor logic.

Why it looks like parachains uses same asset id for local and remote assets, I guess it is because of AssetId conversion used inside AssetTransactor that should definitely have 1-1 mapping between numeric asset_id and asset multi-location.

@KiChjang
Copy link
Contributor

@akru From your description, it would seem to me that it isn't clear how re-anchoring works to you. All re-anchoring does is rewrite the MultiLocation from the perspective of the destination, i.e. if your asset was originally written as ../Here, re-anchoring this to the relay chain would simply change it to Here, or if the destination is Statemint, it would still stay the same as ../Here.

If we have a derivative asset that has a MultiLocation of PalletIndex(17)/GeneralIndex(42) instead, then reanchoring this MultiLocation to the relay chain would yield Parachain(your_para_id)/PalletIndex(17)/GeneralIndex(42), and if the destination is Statemint, it would be re-anchored to ../Parachain(your_para_id)/PalletIndex(17)/GeneralIndex(42).

I hope you can see that the MultiLocation still points to the derivative asset, and not the reserve asset. In short, re-anchoring does not magically convert the MultiLocation of the derivative asset to that of the reserve asset.

@akru
Copy link
Author

akru commented Sep 24, 2022

@KiChjang thank you for explanation. I see, here you meant derivative asset in your opinion should have totally different multi-location without any reference to original, right?
So, in this case we should have some kind of mapping between derivative and reserved asset, that makes things more and more complex, didn’t?

(Ok(assets), Ok(dest)) => {
use sp_std::vec;
let mut message = Xcm(vec![
TransferReserveAsset { assets, dest, xcm: Xcm(vec![]) }
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong instruction here, should be the same as for reserve_withdraw_assets:

                    let mut message = Xcm(vec![
						WithdrawAsset(assets),
						InitiateReserveWithdraw { assets: Wild(All), reserve: dest, xcm: Xcm(vec![]) }
					]);

);
let value = (origin_location, assets.drain());
ensure!(
T::XcmReserveTransferFilter::contains(&value),
Copy link
Contributor

@bkontur bkontur Jul 11, 2023

Choose a reason for hiding this comment

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

I think that this is not a good filter, T::XcmReserveTransferFilter is used for checking assets for "reserve based transfer out", which means "origin can do reserve(assets) here and send ReserveAssetDeposited(assets) to destination",

/// Our XCM filter which messages to be reserve-transferred using the dedicated extrinsic must pass.
type XcmReserveTransferFilter: Contains<(MultiLocation, Vec<MultiAsset>)>;

for opposite "reserve withdraw assets", there should be another dedicated filter like T::XcmReserveWithdrawFilter which should check almost the same as xcm_executor::Config::IsReserve,

IsReserve - checks (assets, origin) if runtime trusts to origin that holds reserves for some assets (origin is here e.g. sibling parachain)
T::XcmReserveWithdrawFilter - should check (dest, assets), if runtime trusts to dest that holds reserves, that we want to withdraw

@franciscoaguirre franciscoaguirre changed the title pallet-xcm: XCM assets reserve withdraw feature pallet-xcm: XCM assets reserve withdraw feature dawdaw Aug 8, 2023
@franciscoaguirre franciscoaguirre changed the title pallet-xcm: XCM assets reserve withdraw feature dawdaw pallet-xcm: XCM assets reserve withdraw feature Aug 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants