diff --git a/bridges/modules/xcm-bridge-hub-router/src/lib.rs b/bridges/modules/xcm-bridge-hub-router/src/lib.rs
index c6008802ae9a50c02ec54d981e5bd503ff659816..ece72ac8494b763035b7ebfead71e3e280bf4b5c 100644
--- a/bridges/modules/xcm-bridge-hub-router/src/lib.rs
+++ b/bridges/modules/xcm-bridge-hub-router/src/lib.rs
@@ -328,40 +328,58 @@ impl<T: Config<I>, I: 'static> SendXcm for Pallet<T, I> {
xcm: &mut Option<Xcm<()>>,
) -> SendResult<Self::Ticket> {
log::trace!(target: LOG_TARGET, "validate - msg: {xcm:?}, destination: {dest:?}");
- // `dest` and `xcm` are required here
- let dest_ref = dest.as_ref().ok_or(SendError::MissingArgument)?;
- let xcm_ref = xcm.as_ref().ok_or(SendError::MissingArgument)?;
-
- // we won't have an access to `dest` and `xcm` in the `deliver` method, so precompute
- // everything required here
- let message_size = xcm_ref.encoded_size() as _;
-
- // bridge doesn't support oversized/overweight messages now. So it is better to drop such
- // messages here than at the bridge hub. Let's check the message size.
- if message_size > HARD_MESSAGE_SIZE_LIMIT {
- return Err(SendError::ExceedsMaxMessageSize)
- }
- // We need to ensure that the known `dest`'s XCM version can comprehend the current `xcm`
- // program. This may seem like an additional, unnecessary check, but it is not. A similar
- // check is probably performed by the `ViaBridgeHubExporter`, which attempts to send a
- // versioned message to the sibling bridge hub. However, the local bridge hub may have a
- // higher XCM version than the remote `dest`. Once again, it is better to discard such
- // messages here than at the bridge hub (e.g., to avoid losing funds).
- let destination_version = T::DestinationVersion::get_version_for(dest_ref)
- .ok_or(SendError::DestinationUnsupported)?;
- let _ = VersionedXcm::from(xcm_ref.clone())
- .into_version(destination_version)
- .map_err(|()| SendError::DestinationUnsupported)?;
-
- // just use exporter to validate destination and insert instructions to pay message fee
- // at the sibling/child bridge hub
- //
- // the cost will include both cost of: (1) to-sibling bridge hub delivery (returned by
- // the `Config::ToBridgeHubSender`) and (2) to-bridged bridge hub delivery (returned by
- // `Self::exporter_for`)
- ViaBridgeHubExporter::<T, I>::validate(dest, xcm)
- .map(|(ticket, cost)| ((message_size, ticket), cost))
+ // In case of success, the `ViaBridgeHubExporter` can modify XCM instructions and consume
+ // `dest` / `xcm`, so we retain the clone of original message and the destination for later
+ // `DestinationVersion` validation.
+ let xcm_to_dest_clone = xcm.clone();
+ let dest_clone = dest.clone();
+
+ // First, use the inner exporter to validate the destination to determine if it is even
+ // routable. If it is not, return an error. If it is, then the XCM is extended with
+ // instructions to pay the message fee at the sibling/child bridge hub. The cost will
+ // include both the cost of (1) delivery to the sibling bridge hub (returned by
+ // `Config::ToBridgeHubSender`) and (2) delivery to the bridged bridge hub (returned by
+ // `Self::exporter_for`).
+ match ViaBridgeHubExporter::<T, I>::validate(dest, xcm) {
+ Ok((ticket, cost)) => {
+ // If the ticket is ok, it means we are routing with this router, so we need to
+ // apply more validations to the cloned `dest` and `xcm`, which are required here.
+ let xcm_to_dest_clone = xcm_to_dest_clone.ok_or(SendError::MissingArgument)?;
+ let dest_clone = dest_clone.ok_or(SendError::MissingArgument)?;
+
+ // We won't have access to `dest` and `xcm` in the `deliver` method, so we need to
+ // precompute everything required here. However, `dest` and `xcm` were consumed by
+ // `ViaBridgeHubExporter`, so we need to use their clones.
+ let message_size = xcm_to_dest_clone.encoded_size() as _;
+
+ // The bridge doesn't support oversized or overweight messages. Therefore, it's
+ // better to drop such messages here rather than at the bridge hub. Let's check the
+ // message size."
+ if message_size > HARD_MESSAGE_SIZE_LIMIT {
+ return Err(SendError::ExceedsMaxMessageSize)
+ }
+
+ // We need to ensure that the known `dest`'s XCM version can comprehend the current
+ // `xcm` program. This may seem like an additional, unnecessary check, but it is
+ // not. A similar check is probably performed by the `ViaBridgeHubExporter`, which
+ // attempts to send a versioned message to the sibling bridge hub. However, the
+ // local bridge hub may have a higher XCM version than the remote `dest`. Once
+ // again, it is better to discard such messages here than at the bridge hub (e.g.,
+ // to avoid losing funds).
+ let destination_version = T::DestinationVersion::get_version_for(&dest_clone)
+ .ok_or(SendError::DestinationUnsupported)?;
+ let _ = VersionedXcm::from(xcm_to_dest_clone)
+ .into_version(destination_version)
+ .map_err(|()| SendError::DestinationUnsupported)?;
+
+ Ok(((message_size, ticket), cost))
+ },
+ Err(e) => {
+ log::trace!(target: LOG_TARGET, "validate - ViaBridgeHubExporter - error: {e:?}");
+ Err(e)
+ },
+ }
}
fn deliver(ticket: Self::Ticket) -> Result<XcmHash, SendError> {
@@ -452,24 +470,51 @@ mod tests {
#[test]
fn not_applicable_if_destination_is_within_other_network() {
run_test(|| {
+ // unroutable dest
+ let dest = Location::new(2, [GlobalConsensus(ByGenesis([0; 32])), Parachain(1000)]);
+ let xcm: Xcm<()> = vec![ClearOrigin].into();
+
+ // check that router does not consume when `NotApplicable`
+ let mut xcm_wrapper = Some(xcm.clone());
assert_eq!(
- send_xcm::<XcmBridgeHubRouter>(
- Location::new(2, [GlobalConsensus(Rococo), Parachain(1000)]),
- vec![].into(),
- ),
+ XcmBridgeHubRouter::validate(&mut Some(dest.clone()), &mut xcm_wrapper),
Err(SendError::NotApplicable),
);
+ // XCM is NOT consumed and untouched
+ assert_eq!(Some(xcm.clone()), xcm_wrapper);
+
+ // check the full `send_xcm`
+ assert_eq!(send_xcm::<XcmBridgeHubRouter>(dest, xcm,), Err(SendError::NotApplicable),);
});
}
#[test]
fn exceeds_max_message_size_if_size_is_above_hard_limit() {
run_test(|| {
+ // routable dest with XCM version
+ let dest =
+ Location::new(2, [GlobalConsensus(BridgedNetworkId::get()), Parachain(1000)]);
+ // oversized XCM
+ let xcm: Xcm<()> = vec![ClearOrigin; HARD_MESSAGE_SIZE_LIMIT as usize].into();
+
+ // dest is routable with the inner router
+ assert_ok!(ViaBridgeHubExporter::<TestRuntime, ()>::validate(
+ &mut Some(dest.clone()),
+ &mut Some(xcm.clone())
+ ));
+
+ // check for oversized message
+ let mut xcm_wrapper = Some(xcm.clone());
+ assert_eq!(
+ XcmBridgeHubRouter::validate(&mut Some(dest.clone()), &mut xcm_wrapper),
+ Err(SendError::ExceedsMaxMessageSize),
+ );
+ // XCM is consumed by the inner router
+ assert!(xcm_wrapper.is_none());
+
+ // check the full `send_xcm`
assert_eq!(
- send_xcm::<XcmBridgeHubRouter>(
- Location::new(2, [GlobalConsensus(Rococo), Parachain(1000)]),
- vec![ClearOrigin; HARD_MESSAGE_SIZE_LIMIT as usize].into(),
- ),
+ send_xcm::<XcmBridgeHubRouter>(dest, xcm,),
Err(SendError::ExceedsMaxMessageSize),
);
});
@@ -478,11 +523,28 @@ mod tests {
#[test]
fn destination_unsupported_if_wrap_version_fails() {
run_test(|| {
+ // routable dest but we don't know XCM version
+ let dest = UnknownXcmVersionForRoutableLocation::get();
+ let xcm: Xcm<()> = vec![ClearOrigin].into();
+
+ // dest is routable with the inner router
+ assert_ok!(ViaBridgeHubExporter::<TestRuntime, ()>::validate(
+ &mut Some(dest.clone()),
+ &mut Some(xcm.clone())
+ ));
+
+ // check that it does not pass XCM version check
+ let mut xcm_wrapper = Some(xcm.clone());
+ assert_eq!(
+ XcmBridgeHubRouter::validate(&mut Some(dest.clone()), &mut xcm_wrapper),
+ Err(SendError::DestinationUnsupported),
+ );
+ // XCM is consumed by the inner router
+ assert!(xcm_wrapper.is_none());
+
+ // check the full `send_xcm`
assert_eq!(
- send_xcm::<XcmBridgeHubRouter>(
- UnknownXcmVersionLocation::get(),
- vec![ClearOrigin].into(),
- ),
+ send_xcm::<XcmBridgeHubRouter>(dest, xcm,),
Err(SendError::DestinationUnsupported),
);
});
diff --git a/bridges/modules/xcm-bridge-hub-router/src/mock.rs b/bridges/modules/xcm-bridge-hub-router/src/mock.rs
index 54e10966d51b23e7be5010b39cb9cb7d6a3b0118..20c86d1da9a2e315c9465b7e8fe0633401a3a3d4 100644
--- a/bridges/modules/xcm-bridge-hub-router/src/mock.rs
+++ b/bridges/modules/xcm-bridge-hub-router/src/mock.rs
@@ -61,7 +61,7 @@ parameter_types! {
Some((BridgeFeeAsset::get(), BASE_FEE).into())
)
];
- pub UnknownXcmVersionLocation: Location = Location::new(2, [GlobalConsensus(BridgedNetworkId::get()), Parachain(9999)]);
+ pub UnknownXcmVersionForRoutableLocation: Location = Location::new(2, [GlobalConsensus(BridgedNetworkId::get()), Parachain(9999)]);
}
#[derive_impl(frame_system::config_preludes::TestDefaultConfig)]
@@ -76,7 +76,7 @@ impl pallet_xcm_bridge_hub_router::Config<()> for TestRuntime {
type BridgedNetworkId = BridgedNetworkId;
type Bridges = NetworkExportTable<BridgeTable>;
type DestinationVersion =
- LatestOrNoneForLocationVersionChecker<Equals<UnknownXcmVersionLocation>>;
+ LatestOrNoneForLocationVersionChecker<Equals<UnknownXcmVersionForRoutableLocation>>;
type BridgeHubOrigin = EnsureRoot<AccountId>;
type ToBridgeHubSender = TestToBridgeHubSender;