Prevents EthereumBlobExporter from consuming dest when returning NotA…

…pplicable (paritytech#5789)

# Description

The EthereumBlobExporter consumes the `dest` parameter when the
destination is not `Here`. Subsequent exporters will receive a `None`
value for the destination instead of the original destination value,
which is incorrect.
 
Closes paritytech#5788

## Integration

Minor fix related to the exporter behaviour.

## Review Notes

Verified that tests
`exporter_validate_with_invalid_dest_does_not_alter_destination` and
`exporter_validate_with_invalid_universal_source_does_not_alter_universal_source`
fail without the fix in the exporter.

---------

Co-authored-by: Adrian Catangiu <[email protected]>

bridges/snowbridge/primitives/router/src/outbound/mod.rs

@@ -69,13 +69,15 @@ where
 			return Err(SendError::NotApplicable)
 		}
 
-		let dest = destination.take().ok_or(SendError::MissingArgument)?;
+		// Cloning destination to avoid modifying the value so subsequent exporters can use it.
+		let dest = destination.clone().take().ok_or(SendError::MissingArgument)?;
 		if dest != Here {
 			log::trace!(target: "xcm::ethereum_blob_exporter", "skipped due to unmatched remote destination {dest:?}.");
 			return Err(SendError::NotApplicable)
 		}
 
-		let (local_net, local_sub) = universal_source
+		// Cloning universal_source to avoid modifying the value so subsequent exporters can use it.
+		let (local_net, local_sub) = universal_source.clone()
 			.take()
 			.ok_or_else(|| {
 				log::error!(target: "xcm::ethereum_blob_exporter", "universal source not provided.");
@@ -84,7 +86,7 @@ where
 			.split_global()
 			.map_err(|()| {
 				log::error!(target: "xcm::ethereum_blob_exporter", "could not get global consensus from universal source '{universal_source:?}'.");
-				SendError::Unroutable
+				SendError::NotApplicable
 			})?;
 
 		if Ok(local_net) != universal_location.global_consensus() {
@@ -96,25 +98,25 @@ where
 			[Parachain(para_id)] => *para_id,
 			_ => {
 				log::error!(target: "xcm::ethereum_blob_exporter", "could not get parachain id from universal source '{local_sub:?}'.");
-				return Err(SendError::MissingArgument)
+				return Err(SendError::NotApplicable)
 			},
 		};
 
-		let message = message.take().ok_or_else(|| {
-			log::error!(target: "xcm::ethereum_blob_exporter", "xcm message not provided.");
-			SendError::MissingArgument
-		})?;
-
 		let source_location = Location::new(1, local_sub.clone());
 
 		let agent_id = match AgentHashedDescription::convert_location(&source_location) {
 			Some(id) => id,
 			None => {
 				log::error!(target: "xcm::ethereum_blob_exporter", "unroutable due to not being able to create agent id. '{source_location:?}'");
-				return Err(SendError::Unroutable)
+				return Err(SendError::NotApplicable)
 			},
 		};
 
+		let message = message.take().ok_or_else(|| {
+			log::error!(target: "xcm::ethereum_blob_exporter", "xcm message not provided.");
+			SendError::MissingArgument
+		})?;
+
 		let mut converter =
 			XcmConverter::<ConvertAssetId, ()>::new(&message, expected_network, agent_id);
 		let (command, message_id) = converter.convert().map_err(|err|{

bridges/snowbridge/primitives/router/src/outbound/tests.rs

@@ -148,7 +148,7 @@ fn exporter_validate_without_universal_source_yields_missing_argument() {
 }
 
 #[test]
-fn exporter_validate_without_global_universal_location_yields_unroutable() {
+fn exporter_validate_without_global_universal_location_yields_not_applicable() {
 	let network = BridgedNetwork::get();
 	let channel: u32 = 0;
 	let mut universal_source: Option<InteriorLocation> = Here.into();
@@ -163,7 +163,7 @@ fn exporter_validate_without_global_universal_location_yields_unroutable() {
 			AgentIdOf,
 			MockTokenIdConvert,
 		>::validate(network, channel, &mut universal_source, &mut destination, &mut message);
-	assert_eq!(result, Err(XcmSendError::Unroutable));
+	assert_eq!(result, Err(XcmSendError::NotApplicable));
 }
 
 #[test]
@@ -206,7 +206,7 @@ fn exporter_validate_with_remote_universal_source_yields_not_applicable() {
 }
 
 #[test]
-fn exporter_validate_without_para_id_in_source_yields_missing_argument() {
+fn exporter_validate_without_para_id_in_source_yields_not_applicable() {
 	let network = BridgedNetwork::get();
 	let channel: u32 = 0;
 	let mut universal_source: Option<InteriorLocation> = Some(GlobalConsensus(Polkadot).into());
@@ -221,11 +221,11 @@ fn exporter_validate_without_para_id_in_source_yields_missing_argument() {
 			AgentIdOf,
 			MockTokenIdConvert,
 		>::validate(network, channel, &mut universal_source, &mut destination, &mut message);
-	assert_eq!(result, Err(XcmSendError::MissingArgument));
+	assert_eq!(result, Err(XcmSendError::NotApplicable));
 }
 
 #[test]
-fn exporter_validate_complex_para_id_in_source_yields_missing_argument() {
+fn exporter_validate_complex_para_id_in_source_yields_not_applicable() {
 	let network = BridgedNetwork::get();
 	let channel: u32 = 0;
 	let mut universal_source: Option<InteriorLocation> =
@@ -241,7 +241,7 @@ fn exporter_validate_complex_para_id_in_source_yields_missing_argument() {
 			AgentIdOf,
 			MockTokenIdConvert,
 		>::validate(network, channel, &mut universal_source, &mut destination, &mut message);
-	assert_eq!(result, Err(XcmSendError::MissingArgument));
+	assert_eq!(result, Err(XcmSendError::NotApplicable));
 }
 
 #[test]
@@ -1163,3 +1163,107 @@ fn xcm_converter_transfer_native_token_with_invalid_location_will_fail() {
 	let result = converter.convert();
 	assert_eq!(result.err(), Some(XcmConverterError::InvalidAsset));
 }
+
+#[test]
+fn exporter_validate_with_invalid_dest_does_not_alter_destination() {
+	let network = BridgedNetwork::get();
+	let destination: InteriorLocation = Parachain(1000).into();
+
+	let universal_source: InteriorLocation = [GlobalConsensus(Polkadot), Parachain(1000)].into();
+
+	let token_address: [u8; 20] = hex!("1000000000000000000000000000000000000000");
+	let beneficiary_address: [u8; 20] = hex!("2000000000000000000000000000000000000000");
+
+	let channel: u32 = 0;
+	let assets: Assets = vec![Asset {
+		id: AssetId([AccountKey20 { network: None, key: token_address }].into()),
+		fun: Fungible(1000),
+	}]
+	.into();
+	let fee = assets.clone().get(0).unwrap().clone();
+	let filter: AssetFilter = assets.clone().into();
+	let msg: Xcm<()> = vec![
+		WithdrawAsset(assets.clone()),
+		ClearOrigin,
+		BuyExecution { fees: fee, weight_limit: Unlimited },
+		DepositAsset {
+			assets: filter,
+			beneficiary: AccountKey20 { network: None, key: beneficiary_address }.into(),
+		},
+		SetTopic([0; 32]),
+	]
+	.into();
+	let mut msg_wrapper: Option<Xcm<()>> = Some(msg.clone());
+	let mut dest_wrapper = Some(destination.clone());
+	let mut universal_source_wrapper = Some(universal_source.clone());
+
+	let result =
+		EthereumBlobExporter::<
+			UniversalLocation,
+			BridgedNetwork,
+			MockOkOutboundQueue,
+			AgentIdOf,
+			MockTokenIdConvert,
+		>::validate(
+			network, channel, &mut universal_source_wrapper, &mut dest_wrapper, &mut msg_wrapper
+		);
+
+	assert_eq!(result, Err(XcmSendError::NotApplicable));
+
+	// ensure mutable variables are not changed
+	assert_eq!(Some(destination), dest_wrapper);
+	assert_eq!(Some(msg), msg_wrapper);
+	assert_eq!(Some(universal_source), universal_source_wrapper);
+}
+
+#[test]
+fn exporter_validate_with_invalid_universal_source_does_not_alter_universal_source() {
+	let network = BridgedNetwork::get();
+	let destination: InteriorLocation = Here.into();
+
+	let universal_source: InteriorLocation = [GlobalConsensus(Westend), Parachain(1000)].into();
+
+	let token_address: [u8; 20] = hex!("1000000000000000000000000000000000000000");
+	let beneficiary_address: [u8; 20] = hex!("2000000000000000000000000000000000000000");
+
+	let channel: u32 = 0;
+	let assets: Assets = vec![Asset {
+		id: AssetId([AccountKey20 { network: None, key: token_address }].into()),
+		fun: Fungible(1000),
+	}]
+	.into();
+	let fee = assets.clone().get(0).unwrap().clone();
+	let filter: AssetFilter = assets.clone().into();
+	let msg: Xcm<()> = vec![
+		WithdrawAsset(assets.clone()),
+		ClearOrigin,
+		BuyExecution { fees: fee, weight_limit: Unlimited },
+		DepositAsset {
+			assets: filter,
+			beneficiary: AccountKey20 { network: None, key: beneficiary_address }.into(),
+		},
+		SetTopic([0; 32]),
+	]
+	.into();
+	let mut msg_wrapper: Option<Xcm<()>> = Some(msg.clone());
+	let mut dest_wrapper = Some(destination.clone());
+	let mut universal_source_wrapper = Some(universal_source.clone());
+
+	let result =
+		EthereumBlobExporter::<
+			UniversalLocation,
+			BridgedNetwork,
+			MockOkOutboundQueue,
+			AgentIdOf,
+			MockTokenIdConvert,
+		>::validate(
+			network, channel, &mut universal_source_wrapper, &mut dest_wrapper, &mut msg_wrapper
+		);
+
+	assert_eq!(result, Err(XcmSendError::NotApplicable));
+
+	// ensure mutable variables are not changed
+	assert_eq!(Some(destination), dest_wrapper);
+	assert_eq!(Some(msg), msg_wrapper);
+	assert_eq!(Some(universal_source), universal_source_wrapper);
+}

prdoc/pr_5789.prdoc

@@ -0,0 +1,19 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Prevents EthereumBlobExporter from consuming parameters when returning NotApplicable
+
+doc:
+  - audience: Node Dev
+    description: |
+     When the EthereumBlobExporter returned a NotApplicable error, it consumed parameters `universal_source`,
+     `destination` and `message`. As a result, subsequent exporters could not use these values. This PR corrects
+     this incorrect behaviour. It also changes error type from `Unroutable` to `NotApplicable` when the global consensus
+     system cannot be extracted from the `universal_source`, or when the source location cannot be converted to an agent
+     ID. Lastly, it changes the error type from `MissingArgument` to `NotApplicable` when the parachain ID cannot be
+     extracted from the location. These changes should have no effect - it is purely to correct behvaiour should
+     multiple exporters be used.
+
+crates:
+  - name: snowbridge-router-primitives
+    bump: patch