From e6837de0921d4f717cdab92f57d76d8fa077c982 Mon Sep 17 00:00:00 2001 From: claravanstaden Date: Fri, 20 Sep 2024 13:49:59 +0200 Subject: [PATCH 1/7] dest and message should be untouched when returning not applicable --- .../primitives/router/src/outbound/mod.rs | 2 + .../primitives/router/src/outbound/tests.rs | 49 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/bridges/snowbridge/primitives/router/src/outbound/mod.rs b/bridges/snowbridge/primitives/router/src/outbound/mod.rs index d3b6c116dd7a..5377b35b302d 100644 --- a/bridges/snowbridge/primitives/router/src/outbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/outbound/mod.rs @@ -71,6 +71,8 @@ impl = + Some([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 result = + EthereumBlobExporter::< + UniversalLocation, + BridgedNetwork, + MockOkOutboundQueue, + AgentIdOf, + MockTokenIdConvert, + >::validate(network, channel, &mut universal_source, &mut dest_wrapper, &mut msg_wrapper); + + assert_eq!(result, Err(XcmSendError::NotApplicable)); + + // ensure dest and msg are untouched + assert_eq!(Some(destination), dest_wrapper); + assert_eq!(Some(msg), msg_wrapper); +} From b96caa5a9632cab840cd12a1f9b68ef2763235fb Mon Sep 17 00:00:00 2001 From: claravanstaden Date: Fri, 20 Sep 2024 13:55:18 +0200 Subject: [PATCH 2/7] fix test name --- bridges/snowbridge/primitives/router/src/outbound/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bridges/snowbridge/primitives/router/src/outbound/tests.rs b/bridges/snowbridge/primitives/router/src/outbound/tests.rs index 41ae64b247e8..a66e226db73a 100644 --- a/bridges/snowbridge/primitives/router/src/outbound/tests.rs +++ b/bridges/snowbridge/primitives/router/src/outbound/tests.rs @@ -1165,7 +1165,7 @@ fn xcm_converter_transfer_native_token_with_invalid_location_will_fail() { } #[test] -fn exporter_validate_with_unknown_network_yields_not_applicable_does_not_alter_destination() { +fn exporter_validate_with_invalid_dest_does_not_alter_destination() { let network = BridgedNetwork::get(); let mut destination: InteriorLocation = Parachain(1000).into(); From bfc90ba5e9b8f55bf9e13210d06fe66f0e7b50dd Mon Sep 17 00:00:00 2001 From: claravanstaden Date: Fri, 20 Sep 2024 15:46:59 +0200 Subject: [PATCH 3/7] fix another case --- .../primitives/router/src/outbound/mod.rs | 8 ++- .../primitives/router/src/outbound/tests.rs | 65 +++++++++++++++++-- 2 files changed, 67 insertions(+), 6 deletions(-) diff --git a/bridges/snowbridge/primitives/router/src/outbound/mod.rs b/bridges/snowbridge/primitives/router/src/outbound/mod.rs index 5377b35b302d..03bf95bb737d 100644 --- a/bridges/snowbridge/primitives/router/src/outbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/outbound/mod.rs @@ -71,11 +71,13 @@ impl = - Some([GlobalConsensus(Polkadot), 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"); @@ -1196,6 +1247,7 @@ fn exporter_validate_with_invalid_dest_does_not_alter_destination() { .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::< @@ -1204,11 +1256,14 @@ fn exporter_validate_with_invalid_dest_does_not_alter_destination() { MockOkOutboundQueue, AgentIdOf, MockTokenIdConvert, - >::validate(network, channel, &mut universal_source, &mut dest_wrapper, &mut msg_wrapper); + >::validate( + network, channel, &mut universal_source_wrapper, &mut dest_wrapper, &mut msg_wrapper + ); assert_eq!(result, Err(XcmSendError::NotApplicable)); - // ensure dest and msg are untouched + // 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); } From 4fe408539e2ed56c4e10168059dc2318ac7abeb1 Mon Sep 17 00:00:00 2001 From: claravanstaden Date: Mon, 23 Sep 2024 15:28:28 +0200 Subject: [PATCH 4/7] clone values --- .../primitives/router/src/outbound/mod.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/bridges/snowbridge/primitives/router/src/outbound/mod.rs b/bridges/snowbridge/primitives/router/src/outbound/mod.rs index 03bf95bb737d..a818b4ea9514 100644 --- a/bridges/snowbridge/primitives/router/src/outbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/outbound/mod.rs @@ -68,17 +68,15 @@ impl Date: Thu, 3 Oct 2024 13:28:07 +0200 Subject: [PATCH 5/7] pr comments --- .../primitives/router/src/outbound/mod.rs | 16 ++++++++-------- .../primitives/router/src/outbound/tests.rs | 12 ++++++------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/bridges/snowbridge/primitives/router/src/outbound/mod.rs b/bridges/snowbridge/primitives/router/src/outbound/mod.rs index 4d98216330d8..efc1ef56f304 100644 --- a/bridges/snowbridge/primitives/router/src/outbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/outbound/mod.rs @@ -86,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() { @@ -98,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 5e4f476e08cc..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] From d34be33f7d73af305e1d0fca25fb6f8d3fd33bde Mon Sep 17 00:00:00 2001 From: claravanstaden Date: Fri, 4 Oct 2024 09:53:04 +0200 Subject: [PATCH 6/7] prdoc --- prdoc/pr_5789.prdoc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 prdoc/pr_5789.prdoc diff --git a/prdoc/pr_5789.prdoc b/prdoc/pr_5789.prdoc new file mode 100644 index 000000000000..8402c87fc9d8 --- /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 extrated 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 From 92a1847f54d52c9b85d916d4317b0cbbeb83fa63 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Fri, 4 Oct 2024 12:12:35 +0300 Subject: [PATCH 7/7] Update prdoc/pr_5789.prdoc --- prdoc/pr_5789.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_5789.prdoc b/prdoc/pr_5789.prdoc index 8402c87fc9d8..9a808fc89d59 100644 --- a/prdoc/pr_5789.prdoc +++ b/prdoc/pr_5789.prdoc @@ -9,7 +9,7 @@ doc: 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 extrated from the `universal_source`, or when the source location cannot be converted to an agent + 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.