From ab2312c68f42c2ecaee52dcc2c5e9da975420ab7 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 20 Oct 2022 15:38:11 +0200 Subject: [PATCH 1/2] Avoid consuming message for NotApplicable scenario --- pallets/xcmp-queue/src/lib.rs | 5 +++-- primitives/utility/src/lib.rs | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pallets/xcmp-queue/src/lib.rs b/pallets/xcmp-queue/src/lib.rs index 72f1c12011b..2fdedd3e306 100644 --- a/pallets/xcmp-queue/src/lib.rs +++ b/pallets/xcmp-queue/src/lib.rs @@ -1138,19 +1138,20 @@ impl SendXcm for Pallet { msg: &mut Option>, ) -> SendResult<(ParaId, VersionedXcm<()>)> { let d = dest.take().ok_or(SendError::MissingArgument)?; - let xcm = msg.take().ok_or(SendError::MissingArgument)?; match &d { // An HRMP message for a sibling parachain. MultiLocation { parents: 1, interior: X1(Parachain(id)) } => { + let xcm = msg.take().ok_or(SendError::MissingArgument)?; let id = ParaId::from(*id); let price = T::PriceForSiblingDelivery::price_for_sibling_delivery(id, &xcm); let versioned_xcm = T::VersionWrapper::wrap_version(&d, xcm) .map_err(|()| SendError::DestinationUnsupported)?; Ok(((id, versioned_xcm), price)) }, - // Anything else is unhandled. This includes a message this is meant for us. _ => { + // Anything else is unhandled. This includes a message that is not meant for us. + // We need to make sure that dest/msg is not consumed here. *dest = Some(d); Err(SendError::NotApplicable) }, diff --git a/primitives/utility/src/lib.rs b/primitives/utility/src/lib.rs index 1e93aa7d792..91730339af0 100644 --- a/primitives/utility/src/lib.rs +++ b/primitives/utility/src/lib.rs @@ -75,10 +75,10 @@ where msg: &mut Option>, ) -> SendResult> { let d = dest.take().ok_or(SendError::MissingArgument)?; - let xcm = msg.take().ok_or(SendError::MissingArgument)?; if d.contains_parents_only(1) { // An upward message for the relay chain. + let xcm = msg.take().ok_or(SendError::MissingArgument)?; let price = P::price_for_parent_delivery(&xcm); let versioned_xcm = W::wrap_version(&d, xcm).map_err(|()| SendError::DestinationUnsupported)?; @@ -86,8 +86,9 @@ where Ok((data, price)) } else { + // Anything else is unhandled. This includes a message that is not meant for us. + // We need to make sure that dest/msg is not consumed here. *dest = Some(d); - // Anything else is unhandled. This includes a message this is meant for us. Err(SendError::NotApplicable) } } From c316212ba1c26d616cad5310701c803f53030522 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 25 Oct 2022 11:04:02 +0200 Subject: [PATCH 2/2] Avoid consuming message for NotApplicable scenario tests --- pallets/xcmp-queue/src/tests.rs | 85 +++++++++++++++++++++++++++ primitives/utility/src/lib.rs | 101 ++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+) diff --git a/pallets/xcmp-queue/src/tests.rs b/pallets/xcmp-queue/src/tests.rs index 1b6303ddaf1..595bd8ca33b 100644 --- a/pallets/xcmp-queue/src/tests.rs +++ b/pallets/xcmp-queue/src/tests.rs @@ -247,3 +247,88 @@ fn update_xcmp_max_individual_weight() { assert_eq!(data.xcmp_max_individual_weight, 30u64 * WEIGHT_PER_MILLIS); }); } + +/// Validates [`validate`] for required Some(destination) and Some(message) +struct OkFixedXcmHashWithAssertingRequiredInputsSender; +impl OkFixedXcmHashWithAssertingRequiredInputsSender { + const FIXED_XCM_HASH: [u8; 32] = [9; 32]; + + fn fixed_delivery_asset() -> MultiAssets { + MultiAssets::new() + } + + fn expected_delivery_result() -> Result<(XcmHash, MultiAssets), SendError> { + Ok((Self::FIXED_XCM_HASH, Self::fixed_delivery_asset())) + } +} +impl SendXcm for OkFixedXcmHashWithAssertingRequiredInputsSender { + type Ticket = (); + + fn validate( + destination: &mut Option, + message: &mut Option>, + ) -> SendResult { + assert!(destination.is_some()); + assert!(message.is_some()); + Ok(((), OkFixedXcmHashWithAssertingRequiredInputsSender::fixed_delivery_asset())) + } + + fn deliver(_: Self::Ticket) -> Result { + Ok(Self::FIXED_XCM_HASH) + } +} + +#[test] +fn xcmp_queue_does_not_consume_dest_or_msg_on_not_applicable() { + // dummy message + let message = Xcm(vec![Trap(5)]); + + // XcmpQueue - check dest is really not applicable + let dest = (Parent, Parent, Parent); + let mut dest_wrapper = Some(dest.clone().into()); + let mut msg_wrapper = Some(message.clone()); + assert_eq!( + Err(SendError::NotApplicable), + ::validate(&mut dest_wrapper, &mut msg_wrapper) + ); + + // check wrapper were not consumed + assert_eq!(Some(dest.clone().into()), dest_wrapper.take()); + assert_eq!(Some(message.clone()), msg_wrapper.take()); + + // another try with router chain with asserting sender + assert_eq!( + OkFixedXcmHashWithAssertingRequiredInputsSender::expected_delivery_result(), + send_xcm::<(XcmpQueue, OkFixedXcmHashWithAssertingRequiredInputsSender)>( + dest.into(), + message + ) + ); +} + +#[test] +fn xcmp_queue_consumes_dest_and_msg_on_ok_validate() { + // dummy message + let message = Xcm(vec![Trap(5)]); + + // XcmpQueue - check dest/msg is valid + let dest = (Parent, X1(Parachain(5555))); + let mut dest_wrapper = Some(dest.clone().into()); + let mut msg_wrapper = Some(message.clone()); + assert!(::validate(&mut dest_wrapper, &mut msg_wrapper).is_ok()); + + // check wrapper were consumed + assert_eq!(None, dest_wrapper.take()); + assert_eq!(None, msg_wrapper.take()); + + new_test_ext().execute_with(|| { + // another try with router chain with asserting sender + assert_eq!( + Err(SendError::Transport("NoChannel")), + send_xcm::<(XcmpQueue, OkFixedXcmHashWithAssertingRequiredInputsSender)>( + dest.into(), + message + ) + ); + }); +} diff --git a/primitives/utility/src/lib.rs b/primitives/utility/src/lib.rs index 91730339af0..5cededd7b46 100644 --- a/primitives/utility/src/lib.rs +++ b/primitives/utility/src/lib.rs @@ -318,3 +318,104 @@ pub trait ChargeWeightInFungibles Result<>::Balance, XcmError>; } + +#[cfg(test)] +mod tests { + use super::*; + use cumulus_primitives_core::UpwardMessage; + + /// Validates [`validate`] for required Some(destination) and Some(message) + struct OkFixedXcmHashWithAssertingRequiredInputsSender; + impl OkFixedXcmHashWithAssertingRequiredInputsSender { + const FIXED_XCM_HASH: [u8; 32] = [9; 32]; + + fn fixed_delivery_asset() -> MultiAssets { + MultiAssets::new() + } + + fn expected_delivery_result() -> Result<(XcmHash, MultiAssets), SendError> { + Ok((Self::FIXED_XCM_HASH, Self::fixed_delivery_asset())) + } + } + impl SendXcm for OkFixedXcmHashWithAssertingRequiredInputsSender { + type Ticket = (); + + fn validate( + destination: &mut Option, + message: &mut Option>, + ) -> SendResult { + assert!(destination.is_some()); + assert!(message.is_some()); + Ok(((), OkFixedXcmHashWithAssertingRequiredInputsSender::fixed_delivery_asset())) + } + + fn deliver(_: Self::Ticket) -> Result { + Ok(Self::FIXED_XCM_HASH) + } + } + + /// Impl [`UpwardMessageSender`] that return `Other` error + struct OtherErrorUpwardMessageSender; + impl UpwardMessageSender for OtherErrorUpwardMessageSender { + fn send_upward_message(_: UpwardMessage) -> Result { + Err(MessageSendError::Other) + } + } + + #[test] + fn parent_as_ump_does_not_consume_dest_or_msg_on_not_applicable() { + // dummy message + let message = Xcm(vec![Trap(5)]); + + // ParentAsUmp - check dest is really not applicable + let dest = (Parent, Parent, Parent); + let mut dest_wrapper = Some(dest.clone().into()); + let mut msg_wrapper = Some(message.clone()); + assert_eq!( + Err(SendError::NotApplicable), + as SendXcm>::validate(&mut dest_wrapper, &mut msg_wrapper) + ); + + // check wrapper were not consumed + assert_eq!(Some(dest.clone().into()), dest_wrapper.take()); + assert_eq!(Some(message.clone()), msg_wrapper.take()); + + // another try with router chain with asserting sender + assert_eq!( + OkFixedXcmHashWithAssertingRequiredInputsSender::expected_delivery_result(), + send_xcm::<(ParentAsUmp<(), (), ()>, OkFixedXcmHashWithAssertingRequiredInputsSender)>( + dest.into(), + message + ) + ); + } + + #[test] + fn parent_as_ump_consumes_dest_and_msg_on_ok_validate() { + // dummy message + let message = Xcm(vec![Trap(5)]); + + // ParentAsUmp - check dest/msg is valid + let dest = (Parent, Here); + let mut dest_wrapper = Some(dest.clone().into()); + let mut msg_wrapper = Some(message.clone()); + assert!( as SendXcm>::validate( + &mut dest_wrapper, + &mut msg_wrapper + ) + .is_ok()); + + // check wrapper were consumed + assert_eq!(None, dest_wrapper.take()); + assert_eq!(None, msg_wrapper.take()); + + // another try with router chain with asserting sender + assert_eq!( + Err(SendError::Transport("Other")), + send_xcm::<( + ParentAsUmp, + OkFixedXcmHashWithAssertingRequiredInputsSender + )>(dest.into(), message) + ); + } +}