Avoid consuming XCM message for NotApplicable scenario (#1787)

* Avoid consuming message for NotApplicable scenario

* Avoid consuming message for NotApplicable scenario tests

pallets/xcmp-queue/src/lib.rs

@@ -1138,19 +1138,20 @@ impl<T: Config> SendXcm for Pallet<T> {
 		msg: &mut Option<Xcm<()>>,
 	) -> 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)
 			},

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<MultiLocation>,
+		message: &mut Option<Xcm<()>>,
+	) -> SendResult<Self::Ticket> {
+		assert!(destination.is_some());
+		assert!(message.is_some());
+		Ok(((), OkFixedXcmHashWithAssertingRequiredInputsSender::fixed_delivery_asset()))
+	}
+
+	fn deliver(_: Self::Ticket) -> Result<XcmHash, SendError> {
+		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),
+		<XcmpQueue 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::<(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!(<XcmpQueue 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());
+
+	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
+			)
+		);
+	});
+}

primitives/utility/src/lib.rs

@@ -75,10 +75,10 @@ where
 		msg: &mut Option<Xcm<()>>,
 	) -> SendResult<Vec<u8>> {
 		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)
 		}
 	}
@@ -317,3 +318,104 @@ pub trait ChargeWeightInFungibles<AccountId, Assets: fungibles::Inspect<AccountI
 		weight: Weight,
 	) -> Result<<Assets as Inspect<AccountId>>::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<MultiLocation>,
+			message: &mut Option<Xcm<()>>,
+		) -> SendResult<Self::Ticket> {
+			assert!(destination.is_some());
+			assert!(message.is_some());
+			Ok(((), OkFixedXcmHashWithAssertingRequiredInputsSender::fixed_delivery_asset()))
+		}
+
+		fn deliver(_: Self::Ticket) -> Result<XcmHash, SendError> {
+			Ok(Self::FIXED_XCM_HASH)
+		}
+	}
+
+	/// Impl [`UpwardMessageSender`] that return `Other` error
+	struct OtherErrorUpwardMessageSender;
+	impl UpwardMessageSender for OtherErrorUpwardMessageSender {
+		fn send_upward_message(_: UpwardMessage) -> Result<u32, MessageSendError> {
+			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),
+			<ParentAsUmp<(), (), ()> 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!(<ParentAsUmp<(), (), ()> 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<OtherErrorUpwardMessageSender, (), ()>,
+				OkFixedXcmHashWithAssertingRequiredInputsSender
+			)>(dest.into(), message)
+		);
+	}
+}