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

xcm-executor: take transport fee from transferred assets if necessary #4834

Open
wants to merge 1 commit 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
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ mod imports {
pub type ParaToParaThroughRelayTest = Test<PenpalA, PenpalB, Westend>;
pub type ParaToParaThroughAHTest = Test<PenpalA, PenpalB, AssetHubWestend>;
pub type RelayToParaThroughAHTest = Test<Westend, PenpalA, AssetHubWestend>;
pub type PenpalToRelayThroughAHTest = Test<PenpalA, Westend, AssetHubWestend>;
}

#[cfg(test)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -658,13 +658,13 @@ fn bidirectional_teleport_foreign_asset_between_para_and_asset_hub_using_explici
}

// ===============================================================
// ===== Transfer - Native Asset - Relay->AssetHub->Parachain ====
// ====== Transfer - Native Asset - Relay->AssetHub->Penpal ======
// ===============================================================
/// Transfers of native asset Relay to Parachain (using AssetHub reserve). Parachains want to avoid
/// Transfers of native asset Relay to Penpal (using AssetHub reserve). Parachains want to avoid
/// managing SAs on all system chains, thus want all their DOT-in-reserve to be held in their
/// Sovereign Account on Asset Hub.
#[test]
fn transfer_native_asset_from_relay_to_para_through_asset_hub() {
fn transfer_native_asset_from_relay_to_penpal_through_asset_hub() {
// Init values for Relay
let destination = Westend::child_location_of(PenpalA::para_id());
let sender = WestendSender::get();
Expand Down Expand Up @@ -820,6 +820,137 @@ fn transfer_native_asset_from_relay_to_para_through_asset_hub() {
assert!(receiver_assets_after < receiver_assets_before + amount_to_send);
}

// ===============================================================
// ===== Transfer - Native Asset - Penpal->AssetHub->Relay =======
// ===============================================================
/// Transfers of native asset Penpal to Relay (using AssetHub reserve). Parachains want to avoid
/// managing SAs on all system chains, thus want all their DOT-in-reserve to be held in their
/// Sovereign Account on Asset Hub.
#[test]
fn transfer_native_asset_from_penpal_to_relay_through_asset_hub() {
// Init values for Penpal
let destination = RelayLocation::get();
let sender = PenpalASender::get();
let amount_to_send: Balance = WESTEND_ED * 100;

// Init values for Penpal
let relay_native_asset_location = RelayLocation::get();
let receiver = WestendReceiver::get();

// Init Test
let test_args = TestContext {
sender: sender.clone(),
receiver: receiver.clone(),
args: TestArgs::new_para(
destination.clone(),
receiver.clone(),
amount_to_send,
(Parent, amount_to_send).into(),
None,
0,
),
};
let mut test = PenpalToRelayThroughAHTest::new(test_args);

let sov_penpal_on_ah = AssetHubWestend::sovereign_account_id_of(
AssetHubWestend::sibling_location_of(PenpalA::para_id()),
);
// fund Penpal's sender account
PenpalA::mint_foreign_asset(
<PenpalA as Chain>::RuntimeOrigin::signed(PenpalAssetOwner::get()),
relay_native_asset_location.clone(),
sender.clone(),
amount_to_send * 2,
);
// fund Penpal's SA on AssetHub with the assets held in reserve
AssetHubWestend::fund_accounts(vec![(sov_penpal_on_ah.clone().into(), amount_to_send * 2)]);

// prefund Relay checking account so we accept teleport "back" from AssetHub
let check_account =
Westend::execute_with(|| <Westend as WestendPallet>::XcmPallet::check_account());
Westend::fund_accounts(vec![(check_account, amount_to_send)]);

// Query initial balances
let sender_balance_before = PenpalA::execute_with(|| {
type ForeignAssets = <PenpalA as PenpalAPallet>::ForeignAssets;
<ForeignAssets as Inspect<_>>::balance(relay_native_asset_location.clone(), &sender)
});
let sov_penpal_on_ah_before = AssetHubWestend::execute_with(|| {
<AssetHubWestend as AssetHubWestendPallet>::Balances::free_balance(sov_penpal_on_ah.clone())
});
let receiver_balance_before = Westend::execute_with(|| {
<Westend as WestendPallet>::Balances::free_balance(receiver.clone())
});

fn transfer_assets_dispatchable(t: PenpalToRelayThroughAHTest) -> DispatchResult {
let fee_idx = t.args.fee_asset_item as usize;
let fee: Asset = t.args.assets.inner().get(fee_idx).cloned().unwrap();
let asset_hub_location = PenpalA::sibling_location_of(AssetHubWestend::para_id());
let context = PenpalUniversalLocation::get();

// reanchor fees to the view of destination (Westend Relay)
let mut remote_fees = fee.clone().reanchored(&t.args.dest, &context).unwrap();
if let Fungible(ref mut amount) = remote_fees.fun {
// we already spent some fees along the way, just use half of what we started with
*amount = *amount / 2;
}
let xcm_on_final_dest = Xcm::<()>(vec![
BuyExecution { fees: remote_fees, weight_limit: t.args.weight_limit.clone() },
DepositAsset {
assets: Wild(AllCounted(t.args.assets.len() as u32)),
beneficiary: t.args.beneficiary,
},
]);

// reanchor final dest (Westend Relay) to the view of hop (Asset Hub)
let mut dest = t.args.dest.clone();
dest.reanchor(&asset_hub_location, &context).unwrap();
// on Asset Hub
let xcm_on_hop = Xcm::<()>(vec![InitiateTeleport {
assets: Wild(AllCounted(t.args.assets.len() as u32)),
dest,
xcm: xcm_on_final_dest,
}]);

// First leg is a reserve-withdraw, from there a teleport to final dest
<PenpalA as PenpalAPallet>::PolkadotXcm::transfer_assets_using_type_and_then(
t.signed_origin,
bx!(asset_hub_location.into()),
bx!(t.args.assets.into()),
bx!(TransferType::DestinationReserve),
bx!(fee.id.into()),
bx!(TransferType::DestinationReserve),
bx!(VersionedXcm::from(xcm_on_hop)),
t.args.weight_limit,
)
}
test.set_dispatchable::<PenpalA>(transfer_assets_dispatchable);
test.assert();

// Query final balances
let sender_balance_after = PenpalA::execute_with(|| {
type ForeignAssets = <PenpalA as PenpalAPallet>::ForeignAssets;
<ForeignAssets as Inspect<_>>::balance(relay_native_asset_location.clone(), &sender)
});
let sov_penpal_on_ah_after = AssetHubWestend::execute_with(|| {
<AssetHubWestend as AssetHubWestendPallet>::Balances::free_balance(sov_penpal_on_ah.clone())
});
let receiver_balance_after = Westend::execute_with(|| {
<Westend as WestendPallet>::Balances::free_balance(receiver.clone())
});

// Sender's asset balance is reduced by amount sent plus delivery fees
assert!(sender_balance_after < sender_balance_before - amount_to_send);
// SA on AH balance is decreased by `amount_to_send`
assert_eq!(sov_penpal_on_ah_after, sov_penpal_on_ah_before - amount_to_send);
// Receiver's balance is increased
assert!(receiver_balance_after > receiver_balance_before);
// Receiver's balance increased by `amount_to_send - delivery_fees - bought_execution`;
// `delivery_fees` might be paid from transfer or JIT, also `bought_execution` is unknown but
// should be non-zero
assert!(receiver_balance_after < receiver_balance_before + amount_to_send);
}

// ==============================================================================================
// ==== Bidirectional Transfer - Native + Teleportable Foreign Assets - Parachain<->AssetHub ====
// ==============================================================================================
Expand Down
56 changes: 47 additions & 9 deletions polkadot/xcm/xcm-executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1086,12 +1086,13 @@ impl<Config: config::Config> XcmExecutor<Config> {
DepositReserveAsset { assets, dest, xcm } => {
let old_holding = self.holding.clone();
let result = Config::TransactionalProcessor::process(|| {
let maybe_delivery_fee_from_holding = if self.fees.is_empty() {
self.get_delivery_fee_from_holding(&assets, &dest, &xcm)?
// When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from
// transferred assets.
let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw {
self.get_delivery_fee_from_holding(&assets, &dest, FeeReason::DepositReserveAsset, &xcm)?
} else {
None
};

let mut message = Vec::with_capacity(xcm.len() + 2);
// now take assets to deposit (after having taken delivery fees)
let deposited = self.holding.saturating_take(assets);
Expand All @@ -1106,7 +1107,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
message.push(ClearOrigin);
// append custom instructions
message.extend(xcm.0.into_iter());
if let Some(delivery_fee) = maybe_delivery_fee_from_holding {
if let Some(delivery_fee) = maybe_transport_fee_from_holding {
// Put back delivery_fee in holding register to be charged by XcmSender.
self.holding.subsume_assets(delivery_fee);
}
Expand All @@ -1121,6 +1122,13 @@ impl<Config: config::Config> XcmExecutor<Config> {
InitiateReserveWithdraw { assets, reserve, xcm } => {
let old_holding = self.holding.clone();
let result = Config::TransactionalProcessor::process(|| {
// When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from
// transferred assets.
let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw {
self.get_delivery_fee_from_holding(&assets, &reserve, FeeReason::InitiateReserveWithdraw, &xcm)?
} else {
None
};
let assets = self.holding.saturating_take(assets);
let mut message = Vec::with_capacity(xcm.len() + 2);
Self::do_reserve_withdraw_assets(
Expand All @@ -1133,6 +1141,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
message.push(ClearOrigin);
// append custom instructions
message.extend(xcm.0.into_iter());
if let Some(delivery_fee) = maybe_transport_fee_from_holding {
// Put back delivery_fee in holding register to be charged by XcmSender.
self.holding.subsume_assets(delivery_fee);
}
self.send(reserve, Xcm(message), FeeReason::InitiateReserveWithdraw)?;
Ok(())
});
Expand All @@ -1144,13 +1156,24 @@ impl<Config: config::Config> XcmExecutor<Config> {
InitiateTeleport { assets, dest, xcm } => {
let old_holding = self.holding.clone();
let result = Config::TransactionalProcessor::process(|| {
// When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from
// transferred assets.
let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw {
self.get_delivery_fee_from_holding(&assets, &dest, FeeReason::InitiateTeleport, &xcm)?
} else {
None
};
let assets = self.holding.saturating_take(assets);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we also substract delivery fees here?, e.g.:

let assets = self.holding.saturating_take(assets - maybe_transport_fee_from_holding);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.. yes, good catch!

most times it doesn't matter since assets is most times a wildcard or some sum where we saturate when we take.

but there could be usecases where someone wants to send exactly N tokens while keeping other M for some subsequent instruction - so it makes sense to send N-transport and leave M for next instruction rather than how the code is now where we send N and leave M-transport.

I'll make the change.

let mut message = Vec::with_capacity(xcm.len() + 2);
Self::do_teleport_assets(assets, &dest, &mut message, &self.context)?;
// clear origin for subsequent custom instructions
message.push(ClearOrigin);
// append custom instructions
message.extend(xcm.0.into_iter());
if let Some(delivery_fee) = maybe_transport_fee_from_holding {
// Put back delivery_fee in holding register to be charged by XcmSender.
self.holding.subsume_assets(delivery_fee);
}
self.send(dest.clone(), Xcm(message), FeeReason::InitiateTeleport)?;
Ok(())
});
Expand Down Expand Up @@ -1707,36 +1730,51 @@ impl<Config: config::Config> XcmExecutor<Config> {
Ok(())
}

/// Gets the necessary delivery fee to send a reserve transfer message to `destination` from
/// holding.
/// Gets the necessary delivery fee from holding to send an onward transfer message to
/// `destination`.
///
/// Will be removed once the transition from `BuyExecution` to `PayFees` is complete.
fn get_delivery_fee_from_holding(
&mut self,
assets: &AssetFilter,
destination: &Location,
reason: FeeReason,
xcm: &Xcm<()>,
) -> Result<Option<AssetsInHolding>, XcmError> {
// we need to do this take/put cycle to solve wildcards and get exact assets to
// be weighed
let to_weigh = self.holding.saturating_take(assets.clone());
self.holding.subsume_assets(to_weigh.clone());
let to_weigh_reanchored = Self::reanchored(to_weigh, &destination, None);
let mut message_to_weigh = vec![ReserveAssetDeposited(to_weigh_reanchored), ClearOrigin];
let remote_instruction = match reason {
FeeReason::DepositReserveAsset => ReserveAssetDeposited(to_weigh_reanchored),
FeeReason::InitiateReserveWithdraw => WithdrawAsset(to_weigh_reanchored),
FeeReason::InitiateTeleport => ReceiveTeleportedAsset(to_weigh_reanchored),
_ => {
tracing::debug!(
target: "xcm::get_delivery_fee_from_holding",
"Unexpected transport fee reason",
);
return Err(XcmError::NotHoldingFees);
},
};
let mut message_to_weigh = Vec::with_capacity(xcm.len() + 2);
message_to_weigh.push(remote_instruction);
message_to_weigh.push(ClearOrigin);
message_to_weigh.extend(xcm.0.clone().into_iter());
let (_, fee) =
validate_send::<Config::XcmSender>(destination.clone(), Xcm(message_to_weigh))?;
let maybe_delivery_fee = fee.get(0).map(|asset_needed_for_fees| {
tracing::trace!(
target: "xcm::fees::DepositReserveAsset",
target: "xcm::fees::get_delivery_fee_from_holding",
"Asset provided to pay for fees {:?}, asset required for delivery fees: {:?}",
self.asset_used_in_buy_execution, asset_needed_for_fees,
);
let asset_to_pay_for_fees =
self.calculate_asset_for_delivery_fees(asset_needed_for_fees.clone());
// set aside fee to be charged by XcmSender
let delivery_fee = self.holding.saturating_take(asset_to_pay_for_fees.into());
tracing::trace!(target: "xcm::fees::DepositReserveAsset", ?delivery_fee);
tracing::trace!(target: "xcm::fees::get_delivery_fee_from_holding", ?delivery_fee);
delivery_fee
});
Ok(maybe_delivery_fee)
Expand Down
11 changes: 11 additions & 0 deletions prdoc/pr_4834.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
title: "xcm-executor: take transport fee from transferred assets if necessary"

doc:
- audience: Runtime Dev
description: |
In asset transfers, as a last resort, XCM transport fees are taken from
transferred assets rather than failing the transfer.

crates:
- name: staging-xcm-executor
bump: patch
Loading