Predicate route table by Xcm #6074
Conversation
|
|
||
| impl Contains<Xcm<()>> for XcmForSnowbridgeV1 { | ||
| fn contains(_: &Xcm<()>) -> bool { | ||
| true |
There was a problem hiding this comment.
Shouldn't this be better like "does not contain AliasOrigin" instead of just "true"?
Or maybe something like:
fn contains(xcm: &Xcm<()>) -> bool {
!XcmForSnowbridgeV2::contains(xcm)
}
or even easier, you dont need XcmForSnowbridgeV1 at all, maybe just use EverythingBut:
pub type EthereumNetworkExportTable =
xcm_builder::NetworkWithXcmExportTable<EthereumBridgeTable, EverythingBut<XcmForSnowbridgeV2>>;
Anyway, neither XcmForSnowbridgeV2 nor XcmForSnowbridgeV1 is aligned with description:
The predicate for V1 XCM will look for
WithdrawAsset(...) or ReserveAssetDeposited # For erc20 and PNA respectively
ClearOrigin
BuyExecution
The predicate for V2 XCM will look for
WithdrawAsset or ReserveAssetDeposited or TeleportedAssetRevceived or *
PayFees(fees)
ALiasOrigin
| /// `(bridge_location, payment)` for the requested `network` and `remote_location` and `xcm` | ||
| /// in the provided `T` table containing various exporters. | ||
| pub struct NetworkWithXcmExportTable<T, M>(core::marker::PhantomData<(T, M)>); | ||
| impl<T: Get<Vec<NetworkExportTableItem>>, M: Contains<Xcm<()>>> ExporterFor |
There was a problem hiding this comment.
I’m not convinced we need both NetworkExportTable and NetworkWithXcmExportTable. Why not just modify the existing NetworkExportTable to include this M / M::contains logic? Wherever NetworkExportTable is used, we can simply add NetworkExportTable<.., Everything>.
There was a problem hiding this comment.
It's a low level primitive and the change will impact current codes/tests a lot, I'd prefer to minimize the change required.
There was a problem hiding this comment.
It's a low level primitive and the change will impact current codes/tests a lot, I'd prefer to minimize the change required.
I assume that at some point you will remove support for v1, correct? If so, then NetworkWithXcmExportTable will no longer be needed. Therefore, I suggest moving NetworkWithXcmExportTable to the bridges/snowbridge submodule for now.
If it needs to stay here, I would suggest modifying NetworkExportTable, yes, it is a breaking change (just like XCMv5, which this feature is waiting for, right?), but it is clearer than having both NetworkWithXcmExportTable and NetworkExportTable.
There was a problem hiding this comment.
Moving NetworkWithXcmExportTable to the bridges/snowbridge submodule with 44dbb6c
| T::get() | ||
| .into_iter() | ||
| .find(|item| { | ||
| &item.remote_network == network && | ||
| M::contains(xcm) && | ||
| item.remote_location_filter | ||
| .as_ref() | ||
| .map(|filters| filters.iter().any(|filter| filter == remote_location)) | ||
| .unwrap_or(true) | ||
| }) | ||
| .map(|item| (item.bridge, item.payment)) |
There was a problem hiding this comment.
I would suggest a small optimization by reusing NetworkExportTable, so you don't need to duplicate code. Currently, your M::contains(xcm) (implemented by xcm.clone().0.matcher()) is called for every item in the list. However, the XCM instructions do not depend on the table, network, or remote location, so you are repeatedly matching the same XCM, which could be checked just once.
| T::get() | |
| .into_iter() | |
| .find(|item| { | |
| &item.remote_network == network && | |
| M::contains(xcm) && | |
| item.remote_location_filter | |
| .as_ref() | |
| .map(|filters| filters.iter().any(|filter| filter == remote_location)) | |
| .unwrap_or(true) | |
| }) | |
| .map(|item| (item.bridge, item.payment)) | |
| // check `network` and `remote_location` | |
| let Some(bridge_with_fees) = NetworkExportTable::<T>::exporter_for(network, remote_location, xcm) else { | |
| return None, | |
| }; | |
| // check the XCM | |
| if M::contains(xcm) { | |
| Some(bridge_with_fees) | |
| } else { | |
| None | |
| } |
And perhaps it makes sense to check the XCM beforehand and then network, remote_location with iterating T table.
Context
The ExporterFor trait returns the fee to be charged to the user when exporting to Ethereum on Asset Hub. This trait exposes the XCM as message parameter that can be used to select an exporter. On Asset Hub this is configured with NetworkExportTable struct implementation. However this implementation ignores the message parameter. We need to modify this implementation so that it contains another field, a predicate which accepts Xcm<()> -> bool which acts as the filter. We can then modify the existing record in the BridgeTable to have a predicate which matches V1 XCM. And add a new record in the bridge table that matches V2 XCM. Each record can then be configured with a different fee.
The predicate for V1 XCM will look for
The predicate for V2 XCM will look for