Deposit fee to beneficiary #137
Conversation
|
So where did the fees go before this PR? To the AH sovereign account? And now it is going to the transfer beneficiary? |
| // Deposit asset to beneficiary. | ||
| DepositAsset { assets: Definite(asset.into()), beneficiary }, | ||
| // Deposit both asset and fees to beneficiary. | ||
| DepositAsset { assets: Wild(AllCounted(2u32)), beneficiary }, |
Previously the remaining DOT was trapped in pallet-xcm. |
|
So basically there is no explicit calculation/configuration variable that we can set the ED to when transferring? We need to calculate the fee for sendToken with the ED taking into account? |
As I mentioned in notion page, since the ED is only 0.01 DOT on AssetHub, a tiny extra assetHubReserveTransferFee will just cover that. |
…t sending token to penpal
| println!("key:{:?}", hex::encode(&key)); | ||
| println!("value:{:?}", hex::encode(&value)); |
There was a problem hiding this comment.
Do we want to leave these in?
There was a problem hiding this comment.
| // Deposit asset to beneficiary. | ||
| DepositAsset { assets: Definite(asset.into()), beneficiary }, | ||
| // Deposit both asset and fees to beneficiary. | ||
| DepositAsset { assets: Wild(All), beneficiary }, |
There was a problem hiding this comment.
This introduces a new failure mode and extra burden on integrations. The failure it is trying to fix is that it will create the account on the destination parachain. However this now places a burden on the destination parachain to store excess DOT in one of their pallets, if it is not configured to than the deposit will fail and assets will be trapped.
So we probably shouldn't do this on the destination chain portion. Only on asset hub.
There was a problem hiding this comment.
Good point 👍
@yrong In general, I am very conservative with changes to our XCM messages, as they can have undefined behavior, depending on the destination parachain.
I think as a practice, before we write any code we should write a short RFC document explaining the change, and detailing any potential changes in behavior that could be problematic.
There was a problem hiding this comment.
Its also hard to review, without seeing the complete messages that are forwarded to AssetHub and the final dest para,
So the RFC doc should include pseudocode describing the full messages.
There was a problem hiding this comment.
Yeah, that make sense.
Usually I check the xcm message from log of emulated test with log level specified(e.g. RUST_LOG=xcm=trace) but it's not easy for others to review/understand.
Sorry for that, let me add the document with the xcm messages forwarded.
There was a problem hiding this comment.
There was a problem hiding this comment.
Btw, the initial intention is just for deposit the excess DOT to beneficiary on destination chain, because it's always not good to leave the fees trapped somewhere instead of send it to the user.
Also it seems better to encourage the end user to send enough destination_fee on Ethereum to ensure the xcm will be success on substrate, and we just let them know the extra fees will be finally refunded and no worry.
But consider the side effects above we only apply this change on asset hub and not do this on the destination chain portion.
Just the upper-stream for Snowfork#137 and more context there. --------- Co-authored-by: Clara van Staden <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>
|
Closed in favor of paritytech#4175 |
Just the upper-stream for Snowfork#137 and more context there. --------- Co-authored-by: Clara van Staden <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>
…#4175) Just the upper-stream for Snowfork#137 and more context there. --------- Co-authored-by: Clara van Staden <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>
No description provided.