From 111b2447a0ce9122a31ad67c4a8ad1479db159a0 Mon Sep 17 00:00:00 2001 From: Clara van Staden Date: Fri, 4 Oct 2024 11:55:18 +0200 Subject: [PATCH] Prevents EthereumBlobExporter from consuming dest when returning NotApplicable (#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 #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 --- .../primitives/router/src/outbound/mod.rs | 22 ++-- .../primitives/router/src/outbound/tests.rs | 116 +++++++++++++++++- prdoc/pr_5789.prdoc | 19 +++ 3 files changed, 141 insertions(+), 16 deletions(-) create mode 100644 prdoc/pr_5789.prdoc diff --git a/bridges/snowbridge/primitives/router/src/outbound/mod.rs b/bridges/snowbridge/primitives/router/src/outbound/mod.rs index b23bce0e2185..efc1ef56f304 100644 --- a/bridges/snowbridge/primitives/router/src/outbound/mod.rs +++ b/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::::new(&message, expected_network, agent_id); let (command, message_id) = converter.convert().map_err(|err|{ diff --git a/bridges/snowbridge/primitives/router/src/outbound/tests.rs b/bridges/snowbridge/primitives/router/src/outbound/tests.rs index 6e4fd5946340..8bd3fa24df5b 100644 --- a/bridges/snowbridge/primitives/router/src/outbound/tests.rs +++ b/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 = 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 = 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 = @@ -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> = 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> = 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); +} diff --git a/prdoc/pr_5789.prdoc b/prdoc/pr_5789.prdoc new file mode 100644 index 000000000000..9a808fc89d59 --- /dev/null +++ b/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