Return a Result in invert_location (#3730)

* Return a Result in invert_location

* Add invertible to spellcheck dictionary

* Fix Some -> Ok

runtime/test-runtime/src/xcm_config.rs

@@ -68,8 +68,8 @@ impl WeightTrader for DummyWeightTrader {
 
 pub struct InvertNothing;
 impl InvertLocation for InvertNothing {
-	fn invert_location(_: &MultiLocation) -> MultiLocation {
-		Here.into()
+	fn invert_location(_: &MultiLocation) -> sp_std::result::Result<MultiLocation, ()> {
+		Ok(Here.into())
 	}
 }
 

scripts/gitlab/lingua.dic

@@ -105,6 +105,7 @@ intrinsics
 invariant/MS
 invariants
 inverter/MS
+invertible
 io
 IP/S
 isn

xcm/pallet-xcm/src/lib.rs

@@ -31,6 +31,7 @@ use sp_std::{
 	convert::{TryFrom, TryInto},
 	marker::PhantomData,
 	prelude::*,
+	result::Result,
 	vec,
 };
 use xcm::{
@@ -206,6 +207,8 @@ pub mod pallet {
 		Filtered,
 		/// The message's weight could not be determined.
 		UnweighableMessage,
+		/// The destination `MultiLocation` provided cannot be inverted.
+		DestinationNotInvertible,
 		/// The assets to be sent are empty.
 		Empty,
 		/// Could not re-anchor the assets to declare the fees for the destination chain.
@@ -322,7 +325,8 @@ pub mod pallet {
 			let value = (origin_location, assets.drain());
 			ensure!(T::XcmTeleportFilter::contains(&value), Error::<T>::Filtered);
 			let (origin_location, assets) = value;
-			let inv_dest = T::LocationInverter::invert_location(&dest);
+			let inv_dest = T::LocationInverter::invert_location(&dest)
+				.map_err(|()| Error::<T>::DestinationNotInvertible)?;
 			let fees = assets
 				.get(fee_asset_item as usize)
 				.ok_or(Error::<T>::Empty)?
@@ -392,7 +396,8 @@ pub mod pallet {
 			let value = (origin_location, assets.drain());
 			ensure!(T::XcmReserveTransferFilter::contains(&value), Error::<T>::Filtered);
 			let (origin_location, assets) = value;
-			let inv_dest = T::LocationInverter::invert_location(&dest);
+			let inv_dest = T::LocationInverter::invert_location(&dest)
+				.map_err(|()| Error::<T>::DestinationNotInvertible)?;
 			let fees = assets
 				.get(fee_asset_item as usize)
 				.ok_or(Error::<T>::Empty)?
@@ -489,18 +494,21 @@ pub mod pallet {
 		/// - `timeout`: The block number after which it is permissible for `notify` not to be
 		///   called even if a response is received.
 		///
+		/// `report_outcome` may return an error if the `responder` is not invertible.
+		///
 		/// To check the status of the query, use `fn query()` passing the resultant `QueryId`
 		/// value.
 		pub fn report_outcome(
 			message: &mut Xcm<()>,
 			responder: MultiLocation,
 			timeout: T::BlockNumber,
-		) -> QueryId {
-			let dest = T::LocationInverter::invert_location(&responder);
+		) -> Result<QueryId, XcmError> {
+			let dest = T::LocationInverter::invert_location(&responder)
+				.map_err(|()| XcmError::MultiLocationNotInvertible)?;
 			let query_id = Self::new_query(responder, timeout);
 			let report_error = Xcm(vec![ReportError { dest, query_id, max_response_weight: 0 }]);
 			message.0.insert(0, SetAppendix(report_error));
-			query_id
+			Ok(query_id)
 		}
 
 		/// Consume `message` and return another which is equivalent to it except that it reports
@@ -516,6 +524,8 @@ pub mod pallet {
 		/// - `timeout`: The block number after which it is permissible for `notify` not to be
 		///   called even if a response is received.
 		///
+		/// `report_outcome_notify` may return an error if the `responder` is not invertible.
+		///
 		/// NOTE: `notify` gets called as part of handling an incoming message, so it should be
 		/// lightweight. Its weight is estimated during this function and stored ready for
 		/// weighing `ReportOutcome` on the way back. If it turns out to be heavier once it returns
@@ -526,13 +536,15 @@ pub mod pallet {
 			responder: MultiLocation,
 			notify: impl Into<<T as Config>::Call>,
 			timeout: T::BlockNumber,
-		) {
-			let dest = T::LocationInverter::invert_location(&responder);
+		) -> Result<(), XcmError> {
+			let dest = T::LocationInverter::invert_location(&responder)
+				.map_err(|()| XcmError::MultiLocationNotInvertible)?;
 			let notify: <T as Config>::Call = notify.into();
 			let max_response_weight = notify.get_dispatch_info().weight;
 			let query_id = Self::new_notify_query(responder, notify, timeout);
 			let report_error = Xcm(vec![ReportError { dest, query_id, max_response_weight }]);
 			message.0.insert(0, SetAppendix(report_error));
+			Ok(())
 		}
 
 		/// Attempt to create a new query ID and register it as a query that is yet to respond.

xcm/pallet-xcm/src/tests.rs

@@ -40,7 +40,8 @@ fn report_outcome_notify_works() {
 	let call = pallet_test_notifier::Call::notification_received(0, Default::default());
 	let notify = Call::TestNotifier(call);
 	new_test_ext_with_balances(balances).execute_with(|| {
-		XcmPallet::report_outcome_notify(&mut message, Parachain(PARA_ID).into(), notify, 100);
+		XcmPallet::report_outcome_notify(&mut message, Parachain(PARA_ID).into(), notify, 100)
+			.unwrap();
 		assert_eq!(
 			message,
 			Xcm(vec![
@@ -94,7 +95,7 @@ fn report_outcome_works() {
 		beneficiary: sender.clone(),
 	}]);
 	new_test_ext_with_balances(balances).execute_with(|| {
-		XcmPallet::report_outcome(&mut message, Parachain(PARA_ID).into(), 100);
+		XcmPallet::report_outcome(&mut message, Parachain(PARA_ID).into(), 100).unwrap();
 		assert_eq!(
 			message,
 			Xcm(vec![

xcm/src/v2/traits.rs

@@ -38,6 +38,7 @@ pub enum Error {
 	UntrustedTeleportLocation,
 	DestinationBufferOverflow,
 	MultiLocationFull,
+	MultiLocationNotInvertible,
 	FailedToDecode,
 	BadOrigin,
 	ExceedsMaxMessageSize,

xcm/xcm-builder/src/location_conversion.rs

@@ -174,24 +174,24 @@ impl<Network: Get<NetworkId>, AccountId: From<[u8; 20]> + Into<[u8; 20]> + Clone
 ///
 /// let input = MultiLocation::new(2, X2(Parachain(2), AccountId32 { network: Any, id: Default::default() }));
 /// let inverted = LocationInverter::<Ancestry>::invert_location(&input);
-/// assert_eq!(inverted, MultiLocation::new(
+/// assert_eq!(inverted, Ok(MultiLocation::new(
 ///     2,
 ///     X2(Parachain(1), AccountKey20 { network: Any, key: Default::default() }),
-/// ));
+/// )));
 /// # }
 /// ```
 pub struct LocationInverter<Ancestry>(PhantomData<Ancestry>);
 impl<Ancestry: Get<MultiLocation>> InvertLocation for LocationInverter<Ancestry> {
-	fn invert_location(location: &MultiLocation) -> MultiLocation {
+	fn invert_location(location: &MultiLocation) -> Result<MultiLocation, ()> {
 		let mut ancestry = Ancestry::get();
 		let mut junctions = Here;
 		for _ in 0..location.parent_count() {
 			junctions = junctions
 				.pushed_with(ancestry.take_first_interior().unwrap_or(OnlyChild))
-				.expect("ancestry is well-formed and has less than 8 non-parent junctions; qed");
+				.map_err(|_| ())?;
 		}
 		let parents = location.interior().len() as u8;
-		MultiLocation::new(parents, junctions)
+		Ok(MultiLocation::new(parents, junctions))
 	}
 }
 
@@ -229,7 +229,7 @@ mod tests {
 		}
 
 		let input = MultiLocation::new(3, X2(Parachain(2), account32()));
-		let inverted = LocationInverter::<Ancestry>::invert_location(&input);
+		let inverted = LocationInverter::<Ancestry>::invert_location(&input).unwrap();
 		assert_eq!(inverted, MultiLocation::new(2, X3(Parachain(1), account20(), account20())));
 	}
 
@@ -244,7 +244,7 @@ mod tests {
 		}
 
 		let input = MultiLocation::grandparent();
-		let inverted = LocationInverter::<Ancestry>::invert_location(&input);
+		let inverted = LocationInverter::<Ancestry>::invert_location(&input).unwrap();
 		assert_eq!(inverted, X2(account20(), account20()).into());
 	}
 
@@ -259,7 +259,18 @@ mod tests {
 		}
 
 		let input = MultiLocation::grandparent();
-		let inverted = LocationInverter::<Ancestry>::invert_location(&input);
+		let inverted = LocationInverter::<Ancestry>::invert_location(&input).unwrap();
 		assert_eq!(inverted, X2(PalletInstance(5), OnlyChild).into());
 	}
+
+	#[test]
+	fn inverter_errors_when_location_is_too_large() {
+		parameter_types! {
+			pub Ancestry: MultiLocation = Here.into();
+		}
+
+		let input = MultiLocation { parents: 99, interior: X1(Parachain(88)) };
+		let inverted = LocationInverter::<Ancestry>::invert_location(&input);
+		assert_eq!(inverted, Err(()));
+	}
 }

xcm/xcm-executor/src/lib.rs

@@ -244,7 +244,8 @@ impl<Config: config::Config> XcmExecutor<Config> {
 			TransferReserveAsset { mut assets, dest, xcm } => {
 				let origin = self.origin.as_ref().ok_or(XcmError::BadOrigin)?;
 				// Take `assets` from the origin account (on-chain) and place into dest account.
-				let inv_dest = Config::LocationInverter::invert_location(&dest);
+				let inv_dest = Config::LocationInverter::invert_location(&dest)
+					.map_err(|()| XcmError::MultiLocationNotInvertible)?;
 				for asset in assets.inner() {
 					Config::AssetTransactor::beam_asset(asset, origin, &dest)?;
 				}
@@ -343,13 +344,13 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				for asset in deposited.assets_iter() {
 					Config::AssetTransactor::deposit_asset(&asset, &dest)?;
 				}
-				let assets = Self::reanchored(deposited, &dest);
+				let assets = Self::reanchored(deposited, &dest)?;
 				let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin];
 				message.extend(xcm.0.into_iter());
 				Config::XcmSender::send_xcm(dest, Xcm(message)).map_err(Into::into)
 			},
 			InitiateReserveWithdraw { assets, reserve, xcm } => {
-				let assets = Self::reanchored(self.holding.saturating_take(assets), &reserve);
+				let assets = Self::reanchored(self.holding.saturating_take(assets), &reserve)?;
 				let mut message = vec![WithdrawAsset(assets), ClearOrigin];
 				message.extend(xcm.0.into_iter());
 				Config::XcmSender::send_xcm(reserve, Xcm(message)).map_err(Into::into)
@@ -360,13 +361,13 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				for asset in assets.assets_iter() {
 					Config::AssetTransactor::check_out(&dest, &asset);
 				}
-				let assets = Self::reanchored(assets, &dest);
+				let assets = Self::reanchored(assets, &dest)?;
 				let mut message = vec![ReceiveTeleportedAsset(assets), ClearOrigin];
 				message.extend(xcm.0.into_iter());
 				Config::XcmSender::send_xcm(dest, Xcm(message)).map_err(Into::into)
 			},
 			QueryHolding { query_id, dest, assets, max_response_weight } => {
-				let assets = Self::reanchored(self.holding.min(&assets), &dest);
+				let assets = Self::reanchored(self.holding.min(&assets), &dest)?;
 				let max_weight = max_response_weight;
 				let response = Response::Assets(assets);
 				let instruction = QueryResponse { query_id, response, max_weight };
@@ -425,9 +426,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		}
 	}
 
-	fn reanchored(mut assets: Assets, dest: &MultiLocation) -> MultiAssets {
-		let inv_dest = Config::LocationInverter::invert_location(&dest);
+	fn reanchored(mut assets: Assets, dest: &MultiLocation) -> Result<MultiAssets, XcmError> {
+		let inv_dest = Config::LocationInverter::invert_location(&dest)
+			.map_err(|()| XcmError::MultiLocationNotInvertible)?;
 		assets.prepend_location(&inv_dest);
-		assets.into_assets_iter().collect::<Vec<_>>().into()
+		Ok(assets.into_assets_iter().collect::<Vec<_>>().into())
 	}
 }

xcm/xcm-executor/src/traits/conversion.rs

@@ -200,5 +200,5 @@ impl<O> ConvertOrigin<O> for Tuple {
 /// Means of inverting a location: given a location which describes a `target` interpreted from the
 /// `source`, this will provide the corresponding location which describes the `source`.
 pub trait InvertLocation {
-	fn invert_location(l: &MultiLocation) -> MultiLocation;
+	fn invert_location(l: &MultiLocation) -> Result<MultiLocation, ()>;
 }