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.

Sign up for GitHub

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

Jump to bottom

Fix delivery fees issue in Kusama #3075

Closed
wants to merge 16 commits into from
Closed

Fix delivery fees issue in Kusama #3075

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d637826
Original fix for one instruction
franciscoaguirre Jan 26, 2024
d53dde2
More instructions
franciscoaguirre Jan 26, 2024
74a2d88
Version bump
franciscoaguirre Jan 26, 2024
398b333
Remove code from TransferReserveAsset
franciscoaguirre Jan 26, 2024
1c827db
Remove blank line
franciscoaguirre Jan 26, 2024
9368692
Fixes
franciscoaguirre Jan 26, 2024
74471d0
Remove jit withdrawal from asset transfer extrinsics
franciscoaguirre Jan 26, 2024
4051d85
Add updated Cargo.lock
franciscoaguirre Jan 26, 2024
bb15bed
".git/.scripts/commands/bench/bench.sh" --subcommand=xcm --runtime=we…
Jan 26, 2024
ba59a14
".git/.scripts/commands/bench/bench.sh" --subcommand=xcm --runtime=ro…
Jan 26, 2024
67c7390
Move weight files
franciscoaguirre Jan 26, 2024
b3ea78a
Remove unnecessary clones
franciscoaguirre Jan 26, 2024
b05a313
Revert "Add updated Cargo.lock"
franciscoaguirre Jan 26, 2024
1ce2488
Add checks for JIT fee mode
franciscoaguirre Jan 26, 2024
b88a5ca
build fixes + fmt
acatangiu Jan 26, 2024
6a85a54
add regression test
acatangiu Jan 26, 2024
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
2 changes: 1 addition & 1 deletion polkadot/xcm/xcm-executor/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ description = "An abstract and configurable XCM message executor."
authors.workspace = true
edition.workspace = true
license.workspace = true
version = "4.0.0"
version = "4.0.1"

[dependencies]
impl-trait-for-tuples = "0.2.2"
Expand Down
61 changes: 61 additions & 0 deletions polkadot/xcm/xcm-executor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,21 @@ impl<Config: config::Config> XcmExecutor<Config> {
},
TransferReserveAsset { mut assets, dest, xcm } => {
let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?;

// 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 mut message_to_weigh =
vec![ReserveAssetDeposited(to_weigh.into()), ClearOrigin];
message_to_weigh.extend(xcm.0.clone().into_iter());
let (_, fee) = validate_send::<Config::XcmSender>(dest, Xcm(message_to_weigh))?;
// set aside fee to be charged by XcmSender
let parked_fee = self.holding.saturating_take(fee.into());

// now take assets to deposit (excluding parked_fee)
let assets = self.holding.saturating_take(assets);
// Take `assets` from the origin account (on-chain) and place into dest account.
for asset in assets.inner() {
Config::AssetTransactor::transfer_asset(asset, origin, &dest, &self.context)?;
Expand Down Expand Up @@ -624,6 +639,19 @@ impl<Config: config::Config> XcmExecutor<Config> {
Ok(())
},
DepositReserveAsset { assets, dest, xcm } => {
// 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 mut message_to_weigh =
vec![ReserveAssetDeposited(to_weigh.into()), ClearOrigin];
message_to_weigh.extend(xcm.0.clone().into_iter());
let (_, fee) = validate_send::<Config::XcmSender>(dest, Xcm(message_to_weigh))?;
// set aside fee to be charged by XcmSender
let parked_fee = self.holding.saturating_take(fee.into());
Comment on lines +637 to +639
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

this can be extracted in some fn extract_transport_fee() or similar and reused


// now take assets to deposit (excluding parked_fee)
let deposited = self.holding.saturating_take(assets);
for asset in deposited.assets_iter() {
Config::AssetTransactor::deposit_asset(&asset, &dest, Some(&self.context))?;
Expand All @@ -633,10 +661,26 @@ impl<Config: config::Config> XcmExecutor<Config> {
let assets = Self::reanchored(deposited, &dest, None);
let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin];
message.extend(xcm.0.into_iter());

// put back parked_fee in holding register to be charged by XcmSender
self.holding.subsume_assets(parked_fee);
self.send(dest, Xcm(message), FeeReason::DepositReserveAsset)?;
Ok(())
},
InitiateReserveWithdraw { assets, reserve, xcm } => {
// we need to do this take/put cycle to solve wildcards and get exact assets to be
Copy link
Contributor

Choose a reason for hiding this comment

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

This is less of a problem since it happens on the origin chain, and not the intermediary chain. We could thus use JIT withdrawal to resolve this.

However, we may want a consistent UX, which would require us to not use JIT withdrawal and remove the SetFeesMode instruction when executing the initial teleport or reserve asset transfer.

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 thing with jit withdrawal is that it's not expected.
Users pay assets and expect execution fees to be deducted from that amount.
With jit withdrawal we are deducting delivery fees from the user's account itself, which might not have more assets after the transfer.
So user has to make sure they pay assets and also have enough for delivery fees.
This makes it so they only pay assets and both execution and delivery fees are deducted from that amount.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is definitely needed for InitiateReserveWithdraw too but not for the current scenario where the intermediary chain is the reserve - I say we keep it, but add a check if jit_withdraw is set - if it is, then we can skip it and let it withdraw from origin's account instead of holding

(maybe we do this jit check for all of them, but honestly I would just deprecate JIT_WITHDRAW altogether, it just introduces extra complexity and error cases)

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 removed the SetFeesMode { jit_withdraw: true } instruction from the teleport and reserve asset transfer extrinsics and re-run the benchmarks. It should only charge for the fees once now and in a more consistent way.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's true for pallet-xcm, but what about other xcm pallets (e.g. xtokens who surfaced this)?

we can't assume in executor that no one will use SetFeesMode { jit_withdraw: true } - as long as we have it, we have to assume it can/will be used

Copy link

Choose a reason for hiding this comment

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

@franciscoaguirre, just to confirm; this means that we can't just send now (as an example) USDT to a para chain from AssetHub. We also have to include KSM and AT LEAST the amount of the XCM delivery fee? And then whatever we put over the min XCM Delivery Fee amount will be delivered to the target account in the target chain?

// weighed
let to_weigh = self.holding.saturating_take(assets.clone());
self.holding.subsume_assets(to_weigh.clone());

let mut message_to_weigh =
vec![WithdrawAsset(to_weigh.clone()), ClearOrigin];
message_to_weigh.extend(xcm.0.clone().into_iter());
let (_, fee) = validate_send::<Config::XcmSender>(dest, Xcm(message_to_weigh))?;
// set aside fee to be charged by XcmSender
let parked_fee = self.holding.saturating_take(fee.into());

// now take assets to deposit (excluding parked_fee)
// Note that here we are able to place any assets which could not be reanchored
// back into Holding.
let assets = Self::reanchored(
Expand All @@ -646,11 +690,25 @@ impl<Config: config::Config> XcmExecutor<Config> {
);
let mut message = vec![WithdrawAsset(assets), ClearOrigin];
message.extend(xcm.0.into_iter());

// put back parked_fee in holding register to be charged by XcmSender
self.holding.subsume_assets(parked_fee);
self.send(reserve, Xcm(message), FeeReason::InitiateReserveWithdraw)?;
Ok(())
},
InitiateTeleport { assets, dest, xcm } => {
// We must do this first in order to resolve wildcards.
let to_weigh = self.holding.saturating_take(assets);
self.holding.subsume_assets(to_weigh.clone());

let mut message_to_weigh =
vec![ReceiveTeleportedAsset(to_weigh.clone()), ClearOrigin];
message_to_weigh.extend(xcm.0.clone().into_iter());
let (_, fee) = validate_send::<Config::XcmSender>(dest, Xcm(message_to_weigh))?;
// set aside fee to be charged by XcmSender
let parked_fee = self.holding.saturating_take(fee.into());

// now take assets to deposit (excluding parked_fee)
let assets = self.holding.saturating_take(assets);
for asset in assets.assets_iter() {
// We should check that the asset can actually be teleported out (for this to
Expand All @@ -667,6 +725,9 @@ impl<Config: config::Config> XcmExecutor<Config> {
let assets = Self::reanchored(assets, &dest, None);
let mut message = vec![ReceiveTeleportedAsset(assets), ClearOrigin];
message.extend(xcm.0.into_iter());

// put back parked_fee in holding register to be charged by XcmSender
self.holding.subsume_assets(parked_fee);
self.send(dest, Xcm(message), FeeReason::InitiateTeleport)?;
Ok(())
},
Expand Down
Loading