pallet-xcm: add extrinsic to allow withdrawal of reserve-based assets

[acatangiu]

Description

Motivation

pallet-xcm is the main user-facing interface for XCM functionality, including assets manipulation functions like teleportAssets() and reserve_transfer_assets() calls.

While teleportAsset() works both ways, reserve_transfer_assets() works only for sending reserve-based assets to a remote destination and beneficiary, but there is no user-facing extrinsic for burning reserved-based assets and getting back the original reserved assets.

Solution

This PR adds (limited_)reserve_withdraw_assets() extrinsics that builds and locally executes XCM program containing InitiateReserveWithdraw XCVM instruction to burn synthetic asset on (local) chain and send notification to destination (reserve chain) for releasing locked asset.

Testing

Added synthetic pallet-xcm unit-test for testing extrinsic when used with native asset, and AssetHubs runtime unit-tests that test extrinsic when used with foreign asset.

Partially fixes paritytech/parity-bridges-common#2405

Checklist

• My PR includes a detailed description as outlined in the "Description" section above
• My PR follows the labeling requirements of this project (at minimum one label for T
  required)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)
• If this PR alters any external APIs or interfaces used by Polkadot, the corresponding Polkadot PR is ready as well
  as the corresponding Cumulus PR (optional)

[acatangiu]

bot bench polkadot-pallet --pallet=pallet_xcm

[command-bot]

@acatangiu Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=runtime --runtime=polkadot --target_dir=polkadot --pallet=pallet_xcm has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3593251 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3593251/artifacts/download.

[KiChjang]

So I'm not 100% sure that we should have a separate extrinsic for this. We actually found out that reserve_transfer_asset isn't doing what we expected it to be doing -- that is, it withdraws derivatives from chain A, tells the sovereign account of A on chain R to transfer reserve assets to the sovereign account of B, and then forwards a notification to chain B.

In other words, if the goal for introducing reserve_withdraw_asset is to work with the limitations of the current reserve_transfer_asset, we really should be changing reserve_transfer_asset first.

[acatangiu]

In other words, if the goal for introducing reserve_withdraw_asset is to work with the limitations of the current reserve_transfer_asset, we really should be changing reserve_transfer_asset first.

No, the goal is not to work around any limitations (none present afaik) of reserve_transfer_asset... I see the need for both extrinsics:

TLDR: reserve_transfer_asset() builds XCM program around TransferReserveAsset instruction, whereas reserve_withdraw_asset builds XCM program around InitiateReserveWithdraw instruction, each meant to be run on different chains.

Long-winded explanation of above

reserve_transfer_asset

My understanding of the purpose of reserve_transfer_asset is to provide user-facing (friendly) api/extrinsic to locally run an XCM program where "the main" instruction is TransferReserveAsset and send an onward XCM program to dest.

Where TransferReserveAsset has following documentation:

/// Withdraw asset(s) (`assets`) from the ownership of `origin` and place equivalent assets
/// under the ownership of `dest` within this consensus system (i.e. its sovereign account).

So (at least afaik) it can only be used for the original reserve asset, not for derivatives (assets based on original reserve). It clearly "reserves" an asset by placing in local SA of dest and send over ReserveAssetDeposited to dest to notify it that dest now has control of reserve assets on source chain.

So the point of it is to turn an/any asset into a reserve asset, reserve it locally in the ownership of dest, and let dest know about it (which usually means dest mints some dest-local derivative equivalent asset).

(Let chain A be the local use of reserve_transfer_asset and chain B be the dest used in the extrinsic, for further examples)

reserve_withdraw_asset

To do the reversed operation (reversed as in "undo", not as in mirrored), user would call reserve_withdraw_asset on chain B to build XCM program and locally run InitiateReserveWithdraw instruction:

• burn derivative asset local to chain B
• send over to chain A an XCM program with Origin of chain B to withdraw/release original reserve assets from SA-of-chainB-on-chainA
• transfer the original assets to some user-specified beneficiary local to chain A

A (poorish) analogy I can think of on the spot is a push/pop pair of stack operations:

• reserve_transfer_asset is like (oversimplification) pushing assets to the "reserves" stack
• reserve_withdraw_asset is like (oversimplification) poping assets from "reserves" stack

I guess we could build a more complex extrinsic that could do both "push" and "pop" and figure out the intention of its user based on parameters or built-in (opaque) logic, but ultimately it still has to run different XCM instructions (TransferReserveAsset vs InitiateReserveWithdraw) based on where it is called (reserve-chain or derivatives-chain). Conflating the two into single extrinsic doesn't look like good design to me.

Maybe I'm missing something, WDYT?

[KiChjang]

So the point of it is to turn an/any asset into a reserve asset, reserve it locally in the ownership of dest, and let dest know about it (which usually means dest mints some dest-local derivative equivalent asset).

That's what it isn't supposed to do -- we fully intended it to be a function that allows you to burn a derivative, send an onwards message to the reserve, transfer the reserve asset underlying the derivative to the SA of the destination, and finally notifying the destination chain. The extrinsic as it stands now doesn't burn anything via InitiateReserveWithdraw and so this is something that we need to rectify.

This would then solve your other issue as well, since there is no "reverse direction" anymore -- it's all just the same function that you call. What I also imagine it to be doing is to be smart and know that if e.g. the destination and the reserve location are the same, it'll just know not to forward a ReserveAssetDeposited instruction to the destination.

[acatangiu]

Sorry I'm a bit confused so I will answer to both interpretations to your comment:

The extrinsic as it stands now doesn't burn anything via InitiateReserveWithdraw and so this is something that we need to rectify.

"The extrinsic" you refer to is reserve_withdraw_asset

reserve_withdraw_asset extrinsic does actually burn the derivative asset via InitiateReserveWithdraw which does:

polkadot-sdk/polkadot/xcm/xcm-executor/src/lib.rs

Lines 641 to 646 in 342d720

	let assets = Self::reanchored(
	self.holding.saturating_take(assets),
	&reserve,
	Some(&mut self.holding),
	);
	let mut message = vec![WithdrawAsset(assets), ClearOrigin];

.
It saturating_takes it out of holding and does nothing else with it - by reanchoring its multilocation we end up referring to the reserve asset. The WithdrawAssets(assets) then happens on destination chain to release the reserve asset from the SA of this chain then later deposit it to final beneficiary.
It does the "burning" same as BurnAsset:

polkadot-sdk/polkadot/xcm/xcm-executor/src/lib.rs

Lines 747 to 750 in 342d720

	BurnAsset(assets) => {
	self.holding.saturating_take(assets.into());
	Ok(())
	},

"The extrinsic" you refer to is reserve_transfer_asset

Or by "The extrinsic" you mean reserve_transfer_asset and that it should do either ReserveAssetDeposited or InitiateReserveWithdraw (or both) based on inner logic?

If this is the case, can you please provide a scenario/user-story for a user with accounts on both chain A and chain B that wants to move a reserve-based asset between his/her accounts using reserve_transfer_asset in both ways?
What should the extrinsic do in each case? Also behavior would be different I think and should also be described when reserve is chain A (or B) or third chain C.

If reserve_transfer_asset extrinsic redesign is required, we can also move this discussion to a https://github.com/polkadot-fellows/RFCs to properly redesign before we attempt code changes?

[xlc]

I still think the way xtokens handles it is significantly better. i.e. a transfer that works for all the supported scenarios. User/Developer don't need to worry about if it is a reserve based transfer or teleport.

[KiChjang]

If reserve_transfer_asset extrinsic redesign is required, we can also move this discussion to a https://github.com/polkadot-fellows/RFCs to properly redesign before we attempt code changes?

This isn't necessary -- that was an oversight, and reserve_transfer_asset was intended to be exactly what I just described. None of what I said applies to reserve_withdraw_asset at all, and I'm actually arguing for it to not exist, since reserve_transfer_asset is supposed to contain such logic.

[Polkadot-Forum]

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

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

[acatangiu]

This isn't necessary -- that was an oversight, and reserve_transfer_asset was intended to be exactly what I just described. None of what I said applies to reserve_withdraw_asset at all, and I'm actually arguing for it to not exist, since reserve_transfer_asset is supposed to contain such logic.

Opened #1584 to detail the proposed solution - will close this PR on behalf of alternative described there.

[acatangiu]

I still think the way xtokens handles it is significantly better. i.e. a transfer that works for all the supported scenarios. User/Developer don't need to worry about if it is a reserve based transfer or teleport.

I'll look at the xtokens implementation 👍
Not sure if we can/should merge reserve based transfers and teleports into generic transfer in pallet-xcm - the XCM team if better suited to make that decision.

[acatangiu]

closing in favor of #1672