Fix delivery fees issue in Kusama #3075
Conversation
franciscoaguirre
changed the base branch from
master
to
release-crates-io-v1.3.0
polkadot/xcm/xcm-executor/src/lib.rs
Outdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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?
franciscoaguirre
added
the
R0-silent
|
bot fmt |
|
@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5038910 was started for your command Comment |
|
@franciscoaguirre Command |
|
bot help |
|
Here's a link to docs |
|
bot bench polkadot-pallet --runtime westend --subcommand xcm --pallet pallet_xcm |
|
@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5040093 was started for your command Comment |
|
@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5040094 was started for your command Comment |
|
@franciscoaguirre Command |
|
@franciscoaguirre Command |
|
bot bench polkadot-pallet --runtime westend --subcommand xcm --pallet pallet_xcm |
|
@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5040237 was started for your command Comment |
|
@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5040238 was started for your command Comment |
…stend --target_dir=polkadot --pallet=pallet_xcm
|
@franciscoaguirre Command |
…coco --target_dir=polkadot --pallet=pallet_xcm
|
@acatangiu Users can accomplish the same thing with |
This reverts commit 4051d85.
polkadot/xcm/xcm-executor/src/lib.rs
Outdated
| let mut message_to_weigh = | ||
| vec![WithdrawAsset(to_weigh.into()), ClearOrigin]; | ||
| message_to_weigh.extend(xcm.0.clone().into_iter()); | ||
| let (_, fee) = validate_send::<Config::XcmSender>(reserve, Xcm(message_to_weigh))?; |
There was a problem hiding this comment.
As I understand fee is calculated in the native asset (for example, KSM). How will it work for the cases when, for example, RMRK or USDT is transferred from the Kusama Asset Hub and no KSM in the holding? Will the extrinsic itself fail on the Asset Hub and the state will be rolled back?
There was a problem hiding this comment.
Until fees can be paid in another asset, there should always be enough KSM to pay for fees, not only for delivery, also for execution.
There was a problem hiding this comment.
Does this mean that KSM must be included into the xcm together with the sending token (USDT)?
There was a problem hiding this comment.
Yes. It should've always been the case in order to pay for the regular execution fees.
franciscoaguirre
changed the base branch from
release-crates-io-v1.3.0
to
master
franciscoaguirre
requested review from
athei,
sorpaas,
andresilva,
jsidorenko and
cheme
as code owners
franciscoaguirre
changed the base branch from
master
to
release-crates-io-v1.3.0
Signed-off-by: Adrian Catangiu <adrian@parity.io>
| let (_, fee) = | ||
| validate_send::<Config::XcmSender>(reserve, Xcm(message_to_weigh))?; | ||
| // set aside fee to be charged by XcmSender | ||
| Some(self.holding.saturating_take(fee.into())) |
There was a problem hiding this comment.
Can trader be used to address cases for fee payment in asset available in holding as implemented in BuyExecution? Almost all dapps rely on logic that is if self sufficient token is sent to/from Asset Hub then the execution fee can be paid in this token. And as I understand that logic is changed now: there must be additional KSM asset in the holding to pay new fees.
There was a problem hiding this comment.
This PR is not changing that logic at all. If fee payment is allowed in USDT, which I think is the case in asset hub because it's a sufficient asset, then you can send only that, same with KSM.
|
Closing in favor of #3085 |
|
@michalisFr can you help here please. |
|
|
can you help with a stuck transaction?
пн, 29 янв. 2024 г. в 21:11, Bastian Köcher ***@***.***>:
… @michalisFr <https://github.com/michalisFr> can you help here please.
—
Reply to this email directly, view it on GitHub
<#3075 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWYFUHUCB6HFH5NVONK2YZ3YQ7X45AVCNFSM6AAAAABCL7C2TSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJVGM4DMNRSG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
@rageruun please do not spam this issue. The Moonbeam is working on indexing all the failed transactions and it will be submitting a governance proposal to help in the cases were assets are being held by the Moonriver owned account in other chains |
|
то есть можно ожидать успешного завершения транзакции?
пн, 29 янв. 2024 г. в 23:20, albertov19 ***@***.***>:
… @rageruun <https://github.com/rageruun> please do not spam this issue.
The Moonbeam is working on indexing all the failed transactions and it
will be submitting a governance proposal to help in the cases were assets
are being held by the Moonriver owned account in other chains
—
Reply to this email directly, view it on GitHub
<#3075 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWYFUHXDQSM7QGTVUZGFT7LYRAHDTAVCNFSM6AAAAABCL7C2TSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJVGU4TEMJYG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@gpestana What could I help with? |
This fix aims to solve an issue in Kusama that resulted in failed reserve asset transfers.
It will be ported later to master as well.
The issue is that during multi-hop XCMs, like reserve asset transfers where the reserve is not the sender nor the destination, there are no assets available to pay for delivery fees.
This fix makes sure to deduct delivery fees from the assets being transferred.