Make RelayedFrom typesafe (#3617)

* Corrent type

* Fix build

* Fixes

* Fixes

* Formatting

xcm/pallet-xcm/src/lib.rs

@@ -106,6 +106,8 @@ pub mod pallet {
 		CannotReanchor,
 		/// Too many assets have been attempted for transfer.
 		TooManyAssets,
+		/// Origin is invalid for sending.
+		InvalidOrigin,
 	}
 
 	#[pallet::hooks]
@@ -120,12 +122,12 @@ pub mod pallet {
 			message: Box<Xcm<()>>,
 		) -> DispatchResult {
 			let origin_location = T::SendXcmOrigin::ensure_origin(origin)?;
-			Self::send_xcm(origin_location.clone(), *dest.clone(), *message.clone()).map_err(
-				|e| match e {
-					XcmError::CannotReachDestination(..) => Error::<T>::Unreachable,
-					_ => Error::<T>::SendFailure,
-				},
-			)?;
+			let interior =
+				origin_location.clone().try_into().map_err(|_| Error::<T>::InvalidOrigin)?;
+			Self::send_xcm(interior, *dest.clone(), *message.clone()).map_err(|e| match e {
+				XcmError::CannotReachDestination(..) => Error::<T>::Unreachable,
+				_ => Error::<T>::SendFailure,
+			})?;
 			Self::deposit_event(Event::Sent(origin_location, *dest, *message));
 			Ok(())
 		}
@@ -302,11 +304,11 @@ pub mod pallet {
 		/// Relay an XCM `message` from a given `interior` location in this context to a given `dest`
 		/// location. A null `dest` is not handled.
 		pub fn send_xcm(
-			interior: MultiLocation,
+			interior: Junctions,
 			dest: MultiLocation,
 			message: Xcm<()>,
 		) -> Result<(), XcmError> {
-			let message = if interior.is_here() {
+			let message = if let Junctions::Here = interior {
 				message
 			} else {
 				Xcm::<()>::RelayedFrom { who: interior, message: Box::new(message) }

xcm/pallet-xcm/src/tests.rs

@@ -17,10 +17,8 @@
 use crate::mock::*;
 use frame_support::{assert_noop, assert_ok, traits::Currency};
 use polkadot_parachain::primitives::{AccountIdConversion, Id as ParaId};
-use xcm::{
-	opaque::v1::prelude::*,
-	v1::{Junction, Xcm},
-};
+use std::convert::TryInto;
+use xcm::v1::prelude::*;
 
 const ALICE: AccountId = AccountId::new([0u8; 32]);
 const BOB: AccountId = AccountId::new([1u8; 32]);
@@ -55,7 +53,10 @@ fn send_works() {
 			sent_xcm(),
 			vec![(
 				Here.into(),
-				RelayedFrom { who: sender.clone(), message: Box::new(message.clone()) }
+				RelayedFrom {
+					who: sender.clone().try_into().unwrap(),
+					message: Box::new(message.clone()),
+				}
 			)]
 		);
 		assert_eq!(

xcm/src/v0/mod.rs

@@ -30,7 +30,7 @@ mod multi_asset;
 mod multi_location;
 mod order;
 mod traits;
-use super::v1::{Response as Response1, Xcm as Xcm1};
+use super::v1::{MultiLocation as MultiLocation1, Response as Response1, Xcm as Xcm1};
 pub use junction::{BodyId, BodyPart, Junction, NetworkId};
 pub use multi_asset::{AssetInstance, MultiAsset};
 pub use multi_location::MultiLocation::{self, *};
@@ -376,7 +376,7 @@ impl<Call> TryFrom<Xcm1<Call>> for Xcm<Call> {
 			Xcm1::Transact { origin_type, require_weight_at_most, call } =>
 				Transact { origin_type, require_weight_at_most, call: call.into() },
 			Xcm1::RelayedFrom { who, message } => RelayedFrom {
-				who: who.try_into()?,
+				who: MultiLocation1 { interior: who, parents: 0 }.try_into()?,
 				message: alloc::boxed::Box::new((*message).try_into()?),
 			},
 		})

xcm/src/v1/mod.rs

@@ -267,14 +267,11 @@ pub enum Xcm<Call> {
 	/// A message to indicate that the embedded XCM is actually arriving on behalf of some consensus
 	/// location within the origin.
 	///
-	/// Safety: `who` must be an interior location of the context. This basically means that no `Parent`
-	/// junctions are allowed in it. This should be verified at the time of XCM execution.
-	///
 	/// Kind: *Instruction*
 	///
 	/// Errors:
 	#[codec(index = 10)]
-	RelayedFrom { who: MultiLocation, message: alloc::boxed::Box<Xcm<Call>> },
+	RelayedFrom { who: Junctions, message: alloc::boxed::Box<Xcm<Call>> },
 }
 
 impl<Call> Xcm<Call> {
@@ -375,7 +372,7 @@ impl<Call> TryFrom<Xcm0<Call>> for Xcm<Call> {
 			Xcm0::Transact { origin_type, require_weight_at_most, call } =>
 				Transact { origin_type, require_weight_at_most, call: call.into() },
 			Xcm0::RelayedFrom { who, message } => RelayedFrom {
-				who: who.try_into()?,
+				who: MultiLocation::try_from(who)?.try_into()?,
 				message: alloc::boxed::Box::new((*message).try_into()?),
 			},
 		})

xcm/src/v1/multilocation.rs

@@ -149,53 +149,36 @@ impl MultiLocation {
 		(multilocation, last)
 	}
 
-	/// Mutates `self`, suffixing its interior junctions with `new`. Returns `Err` in case of overflow.
-	pub fn push_interior(&mut self, new: Junction) -> result::Result<(), ()> {
-		let mut n = Junctions::Here;
-		mem::swap(&mut self.interior, &mut n);
-		match n.pushed_with(new) {
-			Ok(result) => {
-				self.interior = result;
-				Ok(())
-			},
-			Err(old) => {
-				self.interior = old;
-				Err(())
-			},
-		}
+	/// Mutates `self`, suffixing its interior junctions with `new`. Returns `Err` with `new` in
+	/// case of overflow.
+	pub fn push_interior(&mut self, new: Junction) -> result::Result<(), Junction> {
+		self.interior.push(new)
 	}
 
-	/// Mutates `self`, prefixing its interior junctions with `new`. Returns `Err` in case of overflow.
-	pub fn push_front_interior(&mut self, new: Junction) -> result::Result<(), ()> {
-		let mut n = Junctions::Here;
-		mem::swap(&mut self.interior, &mut n);
-		match n.pushed_front_with(new) {
-			Ok(result) => {
-				self.interior = result;
-				Ok(())
-			},
-			Err(old) => {
-				self.interior = old;
-				Err(())
-			},
-		}
+	/// Mutates `self`, prefixing its interior junctions with `new`. Returns `Err` with `new` in
+	/// case of overflow.
+	pub fn push_front_interior(&mut self, new: Junction) -> result::Result<(), Junction> {
+		self.interior.push_front(new)
 	}
 
-	/// Consumes `self` and returns a `MultiLocation` suffixed with `new`, or an `Err` with the original value of
+	/// Consumes `self` and returns a `MultiLocation` suffixed with `new`, or an `Err` with theoriginal value of
 	/// `self` in case of overflow.
-	pub fn pushed_with_interior(self, new: Junction) -> result::Result<Self, Self> {
+	pub fn pushed_with_interior(self, new: Junction) -> result::Result<Self, (Self, Junction)> {
 		match self.interior.pushed_with(new) {
 			Ok(i) => Ok(MultiLocation { interior: i, parents: self.parents }),
-			Err(i) => Err(MultiLocation { interior: i, parents: self.parents }),
+			Err((i, j)) => Err((MultiLocation { interior: i, parents: self.parents }, j)),
 		}
 	}
 
 	/// Consumes `self` and returns a `MultiLocation` prefixed with `new`, or an `Err` with the original value of
 	/// `self` in case of overflow.
-	pub fn pushed_front_with_interior(self, new: Junction) -> result::Result<Self, Self> {
+	pub fn pushed_front_with_interior(
+		self,
+		new: Junction,
+	) -> result::Result<Self, (Self, Junction)> {
 		match self.interior.pushed_front_with(new) {
 			Ok(i) => Ok(MultiLocation { interior: i, parents: self.parents }),
-			Err(i) => Err(MultiLocation { interior: i, parents: self.parents }),
+			Err((i, j)) => Err((MultiLocation { interior: i, parents: self.parents }, j)),
 		}
 	}
 
@@ -259,8 +242,7 @@ impl MultiLocation {
 		self.interior.match_and_split(&prefix.interior)
 	}
 
-	/// Mutate `self` so that it is suffixed with `suffix`. The correct normalized form is returned,
-	/// removing any internal [Non-Parent, `Parent`]  combinations.
+	/// Mutate `self` so that it is suffixed with `suffix`.
 	///
 	/// Does not modify `self` and returns `Err` with `suffix` in case of overflow.
 	///
@@ -268,22 +250,19 @@ impl MultiLocation {
 	/// ```rust
 	/// # use xcm::v1::{Junctions::*, Junction::*, MultiLocation};
 	/// # fn main() {
-	/// let mut m = MultiLocation::new(1, X2(Parachain(21), OnlyChild));
-	/// assert_eq!(m.append_with(MultiLocation::new(1, X1(PalletInstance(3)))), Ok(()));
+	/// let mut m = MultiLocation::new(1, X1(Parachain(21)));
+	/// assert_eq!(m.append_with(X1(PalletInstance(3))), Ok(()));
 	/// assert_eq!(m, MultiLocation::new(1, X2(Parachain(21), PalletInstance(3))));
 	/// # }
 	/// ```
-	pub fn append_with(&mut self, suffix: MultiLocation) -> Result<(), MultiLocation> {
-		let mut prefix = suffix;
-		core::mem::swap(self, &mut prefix);
-		match self.prepend_with(prefix) {
-			Ok(()) => Ok(()),
-			Err(prefix) => {
-				let mut suffix = prefix;
-				core::mem::swap(self, &mut suffix);
-				Err(suffix)
-			},
+	pub fn append_with(&mut self, suffix: Junctions) -> Result<(), Junctions> {
+		if self.interior.len().saturating_add(suffix.len()) > MAX_JUNCTIONS {
+			return Err(suffix)
 		}
+		for j in suffix.into_iter() {
+			self.interior.push(j).expect("Already checked the sum of the len()s; qed")
+		}
+		Ok(())
 	}
 
 	/// Mutate `self` so that it is prefixed with `prefix`.
@@ -767,9 +746,41 @@ impl Junctions {
 		tail
 	}
 
-	/// Consumes `self` and returns a `Junctions` suffixed with `new`, or an `Err` with the original value of
-	/// `self` in case of overflow.
-	pub fn pushed_with(self, new: Junction) -> result::Result<Self, Self> {
+	/// Mutates `self` to be appended with `new` or returns an `Err` with `new` if would overflow.
+	pub fn push(&mut self, new: Junction) -> result::Result<(), Junction> {
+		let mut dummy = Junctions::Here;
+		mem::swap(self, &mut dummy);
+		match dummy.pushed_with(new) {
+			Ok(s) => {
+				*self = s;
+				Ok(())
+			},
+			Err((s, j)) => {
+				*self = s;
+				Err(j)
+			},
+		}
+	}
+
+	/// Mutates `self` to be prepended with `new` or returns an `Err` with `new` if would overflow.
+	pub fn push_front(&mut self, new: Junction) -> result::Result<(), Junction> {
+		let mut dummy = Junctions::Here;
+		mem::swap(self, &mut dummy);
+		match dummy.pushed_front_with(new) {
+			Ok(s) => {
+				*self = s;
+				Ok(())
+			},
+			Err((s, j)) => {
+				*self = s;
+				Err(j)
+			},
+		}
+	}
+
+	/// Consumes `self` and returns a `Junctions` suffixed with `new`, or an `Err` with the
+	/// original value of `self` and `new` in case of overflow.
+	pub fn pushed_with(self, new: Junction) -> result::Result<Self, (Self, Junction)> {
 		Ok(match self {
 			Junctions::Here => Junctions::X1(new),
 			Junctions::X1(a) => Junctions::X2(a, new),
@@ -779,13 +790,13 @@ impl Junctions {
 			Junctions::X5(a, b, c, d, e) => Junctions::X6(a, b, c, d, e, new),
 			Junctions::X6(a, b, c, d, e, f) => Junctions::X7(a, b, c, d, e, f, new),
 			Junctions::X7(a, b, c, d, e, f, g) => Junctions::X8(a, b, c, d, e, f, g, new),
-			s => Err(s)?,
+			s => Err((s, new))?,
 		})
 	}
 
-	/// Consumes `self` and returns a `Junctions` prefixed with `new`, or an `Err` with the original value of
-	/// `self` in case of overflow.
-	pub fn pushed_front_with(self, new: Junction) -> result::Result<Self, Self> {
+	/// Consumes `self` and returns a `Junctions` prefixed with `new`, or an `Err` with the
+	/// original value of `self` and `new` in case of overflow.
+	pub fn pushed_front_with(self, new: Junction) -> result::Result<Self, (Self, Junction)> {
 		Ok(match self {
 			Junctions::Here => Junctions::X1(new),
 			Junctions::X1(a) => Junctions::X2(new, a),
@@ -795,7 +806,7 @@ impl Junctions {
 			Junctions::X5(a, b, c, d, e) => Junctions::X6(new, a, b, c, d, e),
 			Junctions::X6(a, b, c, d, e, f) => Junctions::X7(new, a, b, c, d, e, f),
 			Junctions::X7(a, b, c, d, e, f, g) => Junctions::X8(new, a, b, c, d, e, f, g),
-			s => Err(s)?,
+			s => Err((s, new))?,
 		})
 	}
 
@@ -1245,7 +1256,7 @@ mod tests {
 	fn append_with_works() {
 		let acc = AccountIndex64 { network: Any, index: 23 };
 		let mut m = MultiLocation { parents: 1, interior: X1(Parachain(42)) };
-		assert_eq!(m.append_with(MultiLocation::from(X2(PalletInstance(3), acc.clone()))), Ok(()));
+		assert_eq!(m.append_with(X2(PalletInstance(3), acc.clone())), Ok(()));
 		assert_eq!(
 			m,
 			MultiLocation {
@@ -1256,12 +1267,12 @@ mod tests {
 
 		// cannot append to create overly long multilocation
 		let acc = AccountIndex64 { network: Any, index: 23 };
-		let mut m = MultiLocation {
+		let m = MultiLocation {
 			parents: 254,
 			interior: X5(Parachain(42), OnlyChild, OnlyChild, OnlyChild, OnlyChild),
 		};
-		let suffix = MultiLocation::from(X4(PalletInstance(3), acc.clone(), OnlyChild, OnlyChild));
-		assert_eq!(m.append_with(suffix.clone()), Err(suffix));
+		let suffix = X4(PalletInstance(3), acc.clone(), OnlyChild, OnlyChild);
+		assert_eq!(m.clone().append_with(suffix.clone()), Err(suffix));
 	}
 
 	#[test]

xcm/xcm-builder/src/fungibles_adapter.rs

@@ -55,7 +55,7 @@ impl<Prefix: Get<MultiLocation>, AssetId: Clone, ConvertAssetId: Convert<u128, A
 	fn reverse_ref(what: impl Borrow<AssetId>) -> result::Result<MultiLocation, ()> {
 		let mut location = Prefix::get();
 		let id = ConvertAssetId::reverse_ref(what)?;
-		location.push_interior(Junction::GeneralIndex(id))?;
+		location.push_interior(Junction::GeneralIndex(id)).map_err(|_| ())?;
 		Ok(location)
 	}
 }

xcm/xcm-executor/src/lib.rs

@@ -244,7 +244,6 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				None
 			},
 			(origin, Xcm::RelayedFrom { who, message }) => {
-				ensure!(who.parent_count() == 0, XcmError::EscalationOfPrivilege);
 				let mut origin = origin;
 				origin.append_with(who).map_err(|_| XcmError::MultiLocationFull)?;
 				let surplus = Self::do_execute_xcm(

xcm/xcm-simulator/example/src/lib.rs

@@ -111,7 +111,7 @@ mod tests {
 		);
 		Relay::execute_with(|| {
 			assert_ok!(RelayChainPalletXcm::send_xcm(
-				Here.into(),
+				Here,
 				Parachain(1).into(),
 				Transact {
 					origin_type: OriginKind::SovereignAccount,
@@ -138,7 +138,7 @@ mod tests {
 		);
 		ParaA::execute_with(|| {
 			assert_ok!(ParachainPalletXcm::send_xcm(
-				Here.into(),
+				Here,
 				Parent.into(),
 				Transact {
 					origin_type: OriginKind::SovereignAccount,
@@ -165,7 +165,7 @@ mod tests {
 		);
 		ParaA::execute_with(|| {
 			assert_ok!(ParachainPalletXcm::send_xcm(
-				Here.into(),
+				Here,
 				MultiLocation::new(1, X1(Parachain(2))),
 				Transact {
 					origin_type: OriginKind::SovereignAccount,