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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 173 additions & 0 deletions xcm/pallet-xcm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,51 @@ pub mod pallet {
)
}

/// Transfer some assets from sovereign account to reserve holder chain.
///
/// Fee payment on the destination side is made from the asset in the `assets` vector of
/// index `fee_asset_item`. The weight limit for fees is not provided and thus is unlimited,
/// with all fees taken as needed from the asset.
///
/// - `origin`: Must be capable of withdrawing the `assets` and executing XCM.
/// - `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.
Comment on lines +560 to +561
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.

/// - `beneficiary`: A beneficiary location for the assets in the context of `dest`. Will generally be
/// an `AccountId32` value.
/// - `assets`: The assets to be withdrawn. This should include the assets used to pay the fee on the
/// `dest` side.
/// - `fee_asset_item`: The index into `assets` of the item which should be used to pay
/// fees.
#[pallet::weight({
match ((*assets.clone()).try_into(), (*dest.clone()).try_into()) {
(Ok(assets), Ok(dest)) => {
use sp_std::vec;
let mut message = Xcm(vec![
WithdrawAsset(assets),
InitiateReserveWithdraw { assets: Wild(All), reserve: dest, xcm: Xcm(vec![]) }
]);
T::Weigher::weight(&mut message).map_or(Weight::MAX, |w| Weight::from_ref_time(100_000_000.saturating_add(w)))
},
_ => Weight::MAX,
}
})]
pub fn reserve_withdraw_assets(
origin: OriginFor<T>,
dest: Box<VersionedMultiLocation>,
beneficiary: Box<VersionedMultiLocation>,
assets: Box<VersionedMultiAssets>,
fee_asset_item: u32,
) -> DispatchResult {
Self::do_reserve_withdraw_assets(
origin,
dest,
beneficiary,
assets,
fee_asset_item,
None,
)
}

/// Execute an XCM message from a local, signed, origin.
///
/// An event is deposited indicating whether `msg` could be executed completely or only
Expand Down Expand Up @@ -762,6 +807,52 @@ pub mod pallet {
Some(weight_limit),
)
}

/// Transfer some assets from sovereign account to reserve holder chain.
///
/// Fee payment on the destination side is made from the asset in the `assets` vector of
/// index `fee_asset_item`. The weight limit for fees is not provided and thus is unlimited,
/// with all fees taken as needed from the asset.
///
/// - `origin`: Must be capable of withdrawing the `assets` and executing XCM.
/// - `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.
/// - `beneficiary`: A beneficiary location for the assets in the context of `dest`. Will generally be
/// an `AccountId32` value.
/// - `assets`: The assets to be withdrawn. This should include the assets used to pay the fee on the
/// `dest` side.
/// - `fee_asset_item`: The index into `assets` of the item which should be used to pay
/// fees.
/// - `weight_limit`: The remote-side weight limit, if any, for the XCM fee purchase.
#[pallet::weight({
match ((*assets.clone()).try_into(), (*dest.clone()).try_into()) {
(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![]) }
					]);

]);
T::Weigher::weight(&mut message).map_or(Weight::MAX, |w| Weight::from_ref_time(100_000_000.saturating_add(w)))
},
_ => Weight::MAX,
}
})]
pub fn limited_reserve_withdraw_assets(
origin: OriginFor<T>,
dest: Box<VersionedMultiLocation>,
beneficiary: Box<VersionedMultiLocation>,
assets: Box<VersionedMultiAssets>,
fee_asset_item: u32,
weight_limit: WeightLimit,
) -> DispatchResult {
Self::do_reserve_withdraw_assets(
origin,
dest,
beneficiary,
assets,
fee_asset_item,
Some(weight_limit),
)
}
}

impl<T: Config> Pallet<T> {
Expand Down Expand Up @@ -822,6 +913,88 @@ pub mod pallet {
Ok(())
}

fn do_reserve_withdraw_assets(
origin: OriginFor<T>,
dest: Box<VersionedMultiLocation>,
beneficiary: Box<VersionedMultiLocation>,
assets: Box<VersionedMultiAssets>,
fee_asset_item: u32,
maybe_weight_limit: Option<WeightLimit>,
) -> DispatchResult {
let origin_location = T::ExecuteXcmOrigin::ensure_origin(origin)?;
let dest = (*dest).try_into().map_err(|()| Error::<T>::BadVersion)?;
let beneficiary: MultiLocation = (*beneficiary)
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;
let assets: MultiAssets = (*assets).try_into().map_err(|()| Error::<T>::BadVersion)?;

ensure!(
assets.len() <= MAX_ASSETS_FOR_TRANSFER,
Error::<T>::TooManyAssets
);
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

Error::<T>::Filtered
);
let (origin_location, assets) = value;
let ancestry = T::LocationInverter::ancestry();
let fees = assets
.get(fee_asset_item as usize)
.ok_or(Error::<T>::Empty)?
.clone()
.reanchored(&dest, &ancestry)
.map_err(|_| Error::<T>::CannotReanchor)?;
let max_assets = assets.len() as u32;
let assets: MultiAssets = assets.into();
let weight_limit = match maybe_weight_limit {
Some(weight_limit) => weight_limit,
None => {
let beneficiary = beneficiary.clone();
let fees = fees.clone();
let mut remote_message = Xcm(vec![
WithdrawAsset(assets.clone()),
ClearOrigin,
BuyExecution {
fees,
weight_limit: Limited(0),
},
DepositAsset {
assets: Wild(All),
max_assets,
beneficiary,
},
]);
// use local weight for remote message and hope for the best.
let remote_weight = T::Weigher::weight(&mut remote_message)
.map_err(|()| Error::<T>::UnweighableMessage)?;
Limited(remote_weight)
}
};
let xcm = Xcm(vec![
BuyExecution { fees, weight_limit },
DepositAsset {
assets: Wild(All),
max_assets,
beneficiary,
},
]);
let mut message = Xcm(vec![
WithdrawAsset(assets),
InitiateReserveWithdraw {
assets: Wild(All),
reserve: dest,
xcm,
},
]);
Comment on lines +982 to +989
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.

let weight =
T::Weigher::weight(&mut message).map_err(|()| Error::<T>::UnweighableMessage)?;
let outcome =
T::XcmExecutor::execute_xcm_in_credit(origin_location, message, weight, weight);
Self::deposit_event(Event::Attempted(outcome));
Ok(())
}

fn do_teleport_assets(
origin: OriginFor<T>,
dest: Box<VersionedMultiLocation>,
Expand Down
56 changes: 56 additions & 0 deletions xcm/pallet-xcm/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,62 @@ fn unlimited_reserve_transfer_assets_works() {
});
}

/// Test `reserve_withdraw_assets`
///
/// Asserts that the sender's balance is decreased and the beneficiary's balance
/// is increased. Verifies the correct message is sent and event is emitted.
#[test]
fn reserve_withdraw_assets_works() {
let balances = vec![
(ALICE, INITIAL_BALANCE),
(
ParaId::from(PARA_ID).into_account_truncating(),
INITIAL_BALANCE,
),
];
new_test_ext_with_balances(balances).execute_with(|| {
let weight = BaseXcmWeight::get();
let dest: MultiLocation = Junction::AccountId32 {
network: NetworkId::Any,
id: ALICE.into(),
}
.into();
assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE);
assert_ok!(XcmPallet::reserve_withdraw_assets(
Origin::signed(ALICE),
Box::new(Parachain(PARA_ID).into().into()),
Box::new(dest.clone().into()),
Box::new((Here, SEND_AMOUNT).into()),
0,
));
// Alice spent amount
assert_eq!(Balances::free_balance(ALICE), INITIAL_BALANCE - SEND_AMOUNT);
// Check destination XCM program
assert_eq!(
sent_xcm(),
vec![(
Parachain(PARA_ID).into(),
Xcm(vec![
WithdrawAsset((Parent, SEND_AMOUNT).into()),
ClearOrigin,
buy_limited_execution((Parent, SEND_AMOUNT), 4000),
DepositAsset {
assets: All.into(),
max_assets: 1,
beneficiary: dest
},
]),
)]
);
let versioned_sent = VersionedXcm::from(sent_xcm().into_iter().next().unwrap().1);
let _check_v2_ok: xcm::v2::Xcm<()> = versioned_sent.try_into().unwrap();
assert_eq!(
last_event(),
Event::XcmPallet(crate::Event::Attempted(Outcome::Complete(2 * weight)))
);
});
}

/// Test local execution of XCM
///
/// Asserts that the sender's balance is decreased and the beneficiary's balance
Expand Down