Add router to split XCM fee and assets #2394
Conversation
|
The CI pipeline was cancelled due to failure one of the required jobs. |
There was a problem hiding this comment.
I find this approach of mutating in-flight XCMs to "fix" original XCMs unnatural and obfuscating.. Long term this will just add even more issues and corner-cases.
It adds (to) problems like fees/weights, debugability, control over exact XCM being sent and its intended effects, and general obfuscation of what is happening under the hood.
Instead of this approach, I am very much in favor of biting the bullet and fixing the original miss of these corner-cases in the pallets building XCMs and in the xcm-executor.
With a clear limit of max 1 asset allowed for buying execution, it then becomes clear to XCM builders that XCMs sent to be executed on remote chains they need to do explicit:
Xcm(vec![
ReceiveTeleportedAsset(fee)/ReserveAssetDeposited(fee)/WithdrawAsset(fee)/ClaimAsset(fee),
BuyExecution(fee),
<rest of the program here>,
])
This also means we need to do breaking changes to TransferReserveAsset, DepositReserveAsset, InitiateReserveWithdraw, InitiateTeleport to build fully correct onward XCMs in the first place.
Ideas for this ^ :
- take extra
Option<fees>parameter and make the same split your current router does and addBuyExecutionbefore addingClearOrigin - instead of single
xcmthey takewith_origin_xcmandwithout_origin_xcmspecifying which custom instructions to run beforeClearOriginand which to run afterClearOrigin
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
should move this under tests/ - maybe tests/assets.rs or some new tests/routing.rs
| @@ -104,3 +104,139 @@ impl<Inner: SendXcm, TopicSource: SourceTopic> SendXcm for WithTopicSource<Inner | |||
| Ok(unique_id) | |||
| } | |||
| } | |||
|
|
|||
| fn split_message(message: &mut Option<Xcm<()>>) { | |||
There was a problem hiding this comment.
| fn split_message(message: &mut Option<Xcm<()>>) { | |
| fn split_assets_instructions_for_explicit_fees(message: &mut Option<Xcm<()>>) { |
or something more explicit
| (n, ClearOrigin) if n > 0 && n <= 4 => { | ||
| clear_origin_instructions += 1; | ||
| } | ||
| (n, BuyExecution { fees, .. }) if n > 0 && n <= 5 && initial_fund => { |
There was a problem hiding this comment.
| (n, ClearOrigin) if n > 0 && n <= 4 => { | |
| clear_origin_instructions += 1; | |
| } | |
| (n, BuyExecution { fees, .. }) if n > 0 && n <= 5 && initial_fund => { | |
| (n, ClearOrigin) if n > 0 => { | |
| clear_origin_instructions += 1; | |
| } | |
| (n, BuyExecution { fees, .. }) if n > 0 && initial_fund => { |
I would remove this opinionated n limit here - it's not clear where it comes from (I'm guessing from AllowTopLevelPaidExecutionFrom barrier)
| if clear_origin_instructions > 0 { | ||
| instructions.insert(n + 2, ClearOrigin); | ||
| for index in 1..1 + clear_origin_instructions { | ||
| instructions.remove(index); | ||
| } | ||
| } |
There was a problem hiding this comment.
I would dedup the ClearOrigin instructions in a dedicated router (or at least dedicated function), that gets called before this one, instead of overloading this one.
|
|
||
| for item in instructions.iter().enumerate() { | ||
| match item { | ||
| (0, WithdrawAsset(assets, ..)) if assets.len() > 1 => { |
There was a problem hiding this comment.
This should also handle ReceiveTeleportedAsset(assets), ReserveAssetDeposited(assets) and ClaimAsset { assets, .. } not just WithdrawAsset(assets)
|
@acatangiu What you're proposing seems to require an XCM version upgrade. I think I agree with the general approach of solving the issue from the roots, however we'd also want to have a solution while we prepare a long-term fix in the next version. |
Yes, it'd require a new XCM version.
I'm not against this, but I'm wondering if current I do see how some router solution like this would be useful/necessary during transition to the new XCM version (aka the window between when we change |
|
Closing since |
This router is meant to be used for being able to send more assets before BuyExecution than the limit specified in the AllowTopLevelPaidExecutionFrom barrier. It works by splitting those assets into the fee first and the other assets afterward.
TODO: