Reanchor should return canonical location (#4470)

* Reanchor should return canonical

* Formatting

* Formatting

* Update xcm/src/v1/multilocation.rs

* Formatting

* Fixes

* Don't discard unreanchorable assets

* Formatting

* Docs

* Fixes

* Fixes

* tidy

polkadot/runtime/test-runtime/src/xcm_config.rs

@@ -71,6 +71,9 @@ impl InvertLocation for InvertNothing {
 	fn invert_location(_: &MultiLocation) -> sp_std::result::Result<MultiLocation, ()> {
 		Ok(Here.into())
 	}
+	fn ancestry() -> MultiLocation {
+		Here.into()
+	}
 }
 
 pub struct XcmConfig;

polkadot/xcm/pallet-xcm/src/lib.rs

@@ -777,13 +777,12 @@ 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)
-				.map_err(|()| Error::<T>::DestinationNotInvertible)?;
+			let ancestry = T::LocationInverter::ancestry();
 			let fees = assets
 				.get(fee_asset_item as usize)
 				.ok_or(Error::<T>::Empty)?
 				.clone()
-				.reanchored(&inv_dest)
+				.reanchored(&dest, &ancestry)
 				.map_err(|_| Error::<T>::CannotReanchor)?;
 			let max_assets = assets.len() as u32;
 			let assets: MultiAssets = assets.into();
@@ -835,13 +834,12 @@ 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)
-				.map_err(|()| Error::<T>::DestinationNotInvertible)?;
+			let ancestry = T::LocationInverter::ancestry();
 			let fees = assets
 				.get(fee_asset_item as usize)
 				.ok_or(Error::<T>::Empty)?
 				.clone()
-				.reanchored(&inv_dest)
+				.reanchored(&dest, &ancestry)
 				.map_err(|_| Error::<T>::CannotReanchor)?;
 			let max_assets = assets.len() as u32;
 			let assets: MultiAssets = assets.into();

polkadot/xcm/src/v1/multiasset.rs

@@ -116,13 +116,22 @@ impl From<Vec<u8>> for AssetId {
 
 impl AssetId {
 	/// Prepend a `MultiLocation` to a concrete asset, giving it a new root location.
-	pub fn reanchor(&mut self, prepend: &MultiLocation) -> Result<(), ()> {
+	pub fn prepend_with(&mut self, prepend: &MultiLocation) -> Result<(), ()> {
 		if let AssetId::Concrete(ref mut l) = self {
 			l.prepend_with(prepend.clone()).map_err(|_| ())?;
 		}
 		Ok(())
 	}
 
+	/// Mutate the asset to represent the same value from the perspective of a new `target`
+	/// location. The local chain's location is provided in `ancestry`.
+	pub fn reanchor(&mut self, target: &MultiLocation, ancestry: &MultiLocation) -> Result<(), ()> {
+		if let AssetId::Concrete(ref mut l) = self {
+			l.reanchor(target, ancestry)?;
+		}
+		Ok(())
+	}
+
 	/// Use the value of `self` along with a `fun` fungibility specifier to create the corresponding `MultiAsset` value.
 	pub fn into_multiasset(self, fun: Fungibility) -> MultiAsset {
 		MultiAsset { fun, id: self }
@@ -203,13 +212,24 @@ impl MultiAsset {
 	}
 
 	/// Prepend a `MultiLocation` to a concrete asset, giving it a new root location.
-	pub fn reanchor(&mut self, prepend: &MultiLocation) -> Result<(), ()> {
-		self.id.reanchor(prepend)
+	pub fn prepend_with(&mut self, prepend: &MultiLocation) -> Result<(), ()> {
+		self.id.prepend_with(prepend)
 	}
 
-	/// Prepend a `MultiLocation` to a concrete asset, giving it a new root location.
-	pub fn reanchored(mut self, prepend: &MultiLocation) -> Result<Self, ()> {
-		self.reanchor(prepend)?;
+	/// Mutate the location of the asset identifier if concrete, giving it the same location
+	/// relative to a `target` context. The local context is provided as `ancestry`.
+	pub fn reanchor(&mut self, target: &MultiLocation, ancestry: &MultiLocation) -> Result<(), ()> {
+		self.id.reanchor(target, ancestry)
+	}
+
+	/// Mutate the location of the asset identifier if concrete, giving it the same location
+	/// relative to a `target` context. The local context is provided as `ancestry`.
+	pub fn reanchored(
+		mut self,
+		target: &MultiLocation,
+		ancestry: &MultiLocation,
+	) -> Result<Self, ()> {
+		self.id.reanchor(target, ancestry)?;
 		Ok(self)
 	}
 
@@ -415,8 +435,13 @@ impl MultiAssets {
 	}
 
 	/// Prepend a `MultiLocation` to any concrete asset items, giving it a new root location.
-	pub fn reanchor(&mut self, prepend: &MultiLocation) -> Result<(), ()> {
-		self.0.iter_mut().try_for_each(|i| i.reanchor(prepend))
+	pub fn prepend_with(&mut self, prefix: &MultiLocation) -> Result<(), ()> {
+		self.0.iter_mut().try_for_each(|i| i.prepend_with(prefix))
+	}
+
+	/// Prepend a `MultiLocation` to any concrete asset items, giving it a new root location.
+	pub fn reanchor(&mut self, target: &MultiLocation, ancestry: &MultiLocation) -> Result<(), ()> {
+		self.0.iter_mut().try_for_each(|i| i.reanchor(target, ancestry))
 	}
 
 	/// Return a reference to an item at a specific index or `None` if it doesn't exist.
@@ -485,10 +510,10 @@ impl WildMultiAsset {
 	}
 
 	/// Prepend a `MultiLocation` to any concrete asset components, giving it a new root location.
-	pub fn reanchor(&mut self, prepend: &MultiLocation) -> Result<(), ()> {
+	pub fn reanchor(&mut self, target: &MultiLocation, ancestry: &MultiLocation) -> Result<(), ()> {
 		use WildMultiAsset::*;
 		match self {
-			AllOf { ref mut id, .. } => id.reanchor(prepend).map_err(|_| ()),
+			AllOf { ref mut id, .. } => id.reanchor(target, ancestry).map_err(|_| ()),
 			All => Ok(()),
 		}
 	}
@@ -547,10 +572,10 @@ impl MultiAssetFilter {
 	}
 
 	/// Prepend a `MultiLocation` to any concrete asset components, giving it a new root location.
-	pub fn reanchor(&mut self, prepend: &MultiLocation) -> Result<(), ()> {
+	pub fn reanchor(&mut self, target: &MultiLocation, ancestry: &MultiLocation) -> Result<(), ()> {
 		match self {
-			MultiAssetFilter::Definite(ref mut assets) => assets.reanchor(prepend),
-			MultiAssetFilter::Wild(ref mut wild) => wild.reanchor(prepend),
+			MultiAssetFilter::Definite(ref mut assets) => assets.reanchor(target, ancestry),
+			MultiAssetFilter::Wild(ref mut wild) => wild.reanchor(target, ancestry),
 		}
 	}
 }

polkadot/xcm/src/v1/multilocation.rs

@@ -323,6 +323,60 @@ impl MultiLocation {
 		}
 		Ok(())
 	}
+
+	/// Mutate `self` so that it represents the same location from the point of view of `target`.
+	/// The context of `self` is provided as `ancestry`.
+	///
+	/// Does not modify `self` in case of overflow.
+	pub fn reanchor(&mut self, target: &MultiLocation, ancestry: &MultiLocation) -> Result<(), ()> {
+		// TODO: https://github.com/paritytech/polkadot/issues/4489 Optimize this.
+
+		// 1. Use our `ancestry` to figure out how the `target` would address us.
+		let inverted_target = ancestry.inverted(target)?;
+
+		// 2. Prepend `inverted_target` to `self` to get self's location from the perspective of
+		// `target`.
+		self.prepend_with(inverted_target).map_err(|_| ())?;
+
+		// 3. Given that we know some of `target` ancestry, ensure that any parents in `self` are
+		// strictly needed.
+		self.simplify(target.interior());
+
+		Ok(())
+	}
+
+	/// Treating `self` as a context, determine how it would be referenced by a `target` location.
+	pub fn inverted(&self, target: &MultiLocation) -> Result<MultiLocation, ()> {
+		use Junction::OnlyChild;
+		let mut ancestry = self.clone();
+		let mut junctions = Junctions::Here;
+		for _ in 0..target.parent_count() {
+			junctions = junctions
+				.pushed_front_with(ancestry.interior.take_last().unwrap_or(OnlyChild))
+				.map_err(|_| ())?;
+		}
+		let parents = target.interior().len() as u8;
+		Ok(MultiLocation::new(parents, junctions))
+	}
+
+	/// Remove any unneeded parents/junctions in `self` based on the given context it will be
+	/// interpreted in.
+	pub fn simplify(&mut self, context: &Junctions) {
+		if context.len() < self.parents as usize {
+			// Not enough context
+			return
+		}
+		while self.parents > 0 {
+			let maybe = context.at(context.len() - (self.parents as usize));
+			match (self.interior.first(), maybe) {
+				(Some(i), Some(j)) if i == j => {
+					self.interior.take_first();
+					self.parents -= 1;
+				},
+				_ => break,
+			}
+		}
+	}
 }
 
 /// A unit struct which can be converted into a `MultiLocation` of `parents` value 1.
@@ -773,9 +827,82 @@ impl TryFrom<MultiLocation> for Junctions {
 #[cfg(test)]
 mod tests {
 	use super::{Ancestor, AncestorThen, Junctions::*, MultiLocation, Parent, ParentThen};
-	use crate::opaque::v1::{Junction::*, NetworkId::Any};
+	use crate::opaque::v1::{Junction::*, NetworkId::*};
 	use parity_scale_codec::{Decode, Encode};
 
+	#[test]
+	fn inverted_works() {
+		let ancestry: MultiLocation = (Parachain(1000), PalletInstance(42)).into();
+		let target = (Parent, PalletInstance(69)).into();
+		let expected = (Parent, PalletInstance(42)).into();
+		let inverted = ancestry.inverted(&target).unwrap();
+		assert_eq!(inverted, expected);
+
+		let ancestry: MultiLocation = (Parachain(1000), PalletInstance(42), GeneralIndex(1)).into();
+		let target = (Parent, Parent, PalletInstance(69), GeneralIndex(2)).into();
+		let expected = (Parent, Parent, PalletInstance(42), GeneralIndex(1)).into();
+		let inverted = ancestry.inverted(&target).unwrap();
+		assert_eq!(inverted, expected);
+	}
+
+	#[test]
+	fn simplify_basic_works() {
+		let mut location: MultiLocation =
+			(Parent, Parent, Parachain(1000), PalletInstance(42), GeneralIndex(69)).into();
+		let context = X2(Parachain(1000), PalletInstance(42));
+		let expected = GeneralIndex(69).into();
+		location.simplify(&context);
+		assert_eq!(location, expected);
+
+		let mut location: MultiLocation = (Parent, PalletInstance(42), GeneralIndex(69)).into();
+		let context = X1(PalletInstance(42));
+		let expected = GeneralIndex(69).into();
+		location.simplify(&context);
+		assert_eq!(location, expected);
+
+		let mut location: MultiLocation = (Parent, PalletInstance(42), GeneralIndex(69)).into();
+		let context = X2(Parachain(1000), PalletInstance(42));
+		let expected = GeneralIndex(69).into();
+		location.simplify(&context);
+		assert_eq!(location, expected);
+
+		let mut location: MultiLocation =
+			(Parent, Parent, Parachain(1000), PalletInstance(42), GeneralIndex(69)).into();
+		let context = X3(OnlyChild, Parachain(1000), PalletInstance(42));
+		let expected = GeneralIndex(69).into();
+		location.simplify(&context);
+		assert_eq!(location, expected);
+	}
+
+	#[test]
+	fn simplify_incompatible_location_fails() {
+		let mut location: MultiLocation =
+			(Parent, Parent, Parachain(1000), PalletInstance(42), GeneralIndex(69)).into();
+		let context = X3(Parachain(1000), PalletInstance(42), GeneralIndex(42));
+		let expected =
+			(Parent, Parent, Parachain(1000), PalletInstance(42), GeneralIndex(69)).into();
+		location.simplify(&context);
+		assert_eq!(location, expected);
+
+		let mut location: MultiLocation =
+			(Parent, Parent, Parachain(1000), PalletInstance(42), GeneralIndex(69)).into();
+		let context = X1(Parachain(1000));
+		let expected =
+			(Parent, Parent, Parachain(1000), PalletInstance(42), GeneralIndex(69)).into();
+		location.simplify(&context);
+		assert_eq!(location, expected);
+	}
+
+	#[test]
+	fn reanchor_works() {
+		let mut id: MultiLocation = (Parent, Parachain(1000), GeneralIndex(42)).into();
+		let ancestry = Parachain(2000).into();
+		let target = (Parent, Parachain(1000)).into();
+		let expected = GeneralIndex(42).into();
+		id.reanchor(&target, &ancestry).unwrap();
+		assert_eq!(id, expected);
+	}
+
 	#[test]
 	fn encode_and_decode_works() {
 		let m = MultiLocation {

polkadot/xcm/xcm-builder/src/location_conversion.rs

@@ -182,6 +182,9 @@ impl<Network: Get<NetworkId>, AccountId: From<[u8; 20]> + Into<[u8; 20]> + Clone
 /// ```
 pub struct LocationInverter<Ancestry>(PhantomData<Ancestry>);
 impl<Ancestry: Get<MultiLocation>> InvertLocation for LocationInverter<Ancestry> {
+	fn ancestry() -> MultiLocation {
+		Ancestry::get()
+	}
 	fn invert_location(location: &MultiLocation) -> Result<MultiLocation, ()> {
 		let mut ancestry = Ancestry::get();
 		let mut junctions = Here;

polkadot/xcm/xcm-executor/src/assets.rs

@@ -194,7 +194,7 @@ impl Assets {
 		self.fungible = fungible
 			.into_iter()
 			.map(|(mut id, amount)| {
-				let _ = id.reanchor(prepend);
+				let _ = id.prepend_with(prepend);
 				(id, amount)
 			})
 			.collect();
@@ -203,12 +203,48 @@ impl Assets {
 		self.non_fungible = non_fungible
 			.into_iter()
 			.map(|(mut class, inst)| {
-				let _ = class.reanchor(prepend);
+				let _ = class.prepend_with(prepend);
 				(class, inst)
 			})
 			.collect();
 	}
 
+	/// Mutate the assets to be interpreted as the same assets from the perspective of a `target`
+	/// chain. The local chain's `ancestry` is provided.
+	///
+	/// Any assets which were unable to be reanchored are introduced into `failed_bin`.
+	pub fn reanchor(
+		&mut self,
+		target: &MultiLocation,
+		ancestry: &MultiLocation,
+		mut maybe_failed_bin: Option<&mut Self>,
+	) {
+		let mut fungible = Default::default();
+		mem::swap(&mut self.fungible, &mut fungible);
+		self.fungible = fungible
+			.into_iter()
+			.filter_map(|(mut id, amount)| match id.reanchor(target, ancestry) {
+				Ok(()) => Some((id, amount)),
+				Err(()) => {
+					maybe_failed_bin.as_mut().map(|f| f.fungible.insert(id, amount));
+					None
+				},
+			})
+			.collect();
+		let mut non_fungible = Default::default();
+		mem::swap(&mut self.non_fungible, &mut non_fungible);
+		self.non_fungible = non_fungible
+			.into_iter()
+			.filter_map(|(mut class, inst)| match class.reanchor(target, ancestry) {
+				Ok(()) => Some((class, inst)),
+				Err(()) => {
+					maybe_failed_bin.as_mut().map(|f| f.non_fungible.insert((class, inst)));
+					None
+				},
+			})
+			.collect();
+	}
+
 	/// Returns an error unless all `assets` are contained in `self`. In the case of an error, the first asset in
 	/// `assets` which is not wholly in `self` is returned.
 	pub fn ensure_contains(&self, assets: &MultiAssets) -> Result<(), TakeError> {

polkadot/xcm/xcm-executor/src/lib.rs

@@ -304,12 +304,11 @@ 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)
-					.map_err(|()| XcmError::MultiLocationNotInvertible)?;
 				for asset in assets.inner() {
 					Config::AssetTransactor::beam_asset(asset, origin, &dest)?;
 				}
-				assets.reanchor(&inv_dest).map_err(|()| XcmError::MultiLocationFull)?;
+				let ancestry = Config::LocationInverter::ancestry();
+				assets.reanchor(&dest, &ancestry).map_err(|()| XcmError::MultiLocationFull)?;
 				let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin];
 				message.extend(xcm.0.into_iter());
 				Config::XcmSender::send_xcm(dest, Xcm(message)).map_err(Into::into)
@@ -401,13 +400,21 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				for asset in deposited.assets_iter() {
 					Config::AssetTransactor::deposit_asset(&asset, &dest)?;
 				}
-				let assets = Self::reanchored(deposited, &dest)?;
+				// Note that we pass `None` as `maybe_failed_bin` and drop any assets which cannot
+				// be reanchored  because we have already called `deposit_asset` on all assets.
+				let assets = Self::reanchored(deposited, &dest, None);
 				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)?;
+				// Note that here we are able to place any assets which could not be reanchored
+				// back into Holding.
+				let assets = Self::reanchored(
+					self.holding.saturating_take(assets),
+					&reserve,
+					Some(&mut self.holding),
+				);
 				let mut message = vec![WithdrawAsset(assets), ClearOrigin];
 				message.extend(xcm.0.into_iter());
 				Config::XcmSender::send_xcm(reserve, Xcm(message)).map_err(Into::into)
@@ -418,13 +425,17 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				for asset in assets.assets_iter() {
 					Config::AssetTransactor::check_out(&dest, &asset);
 				}
-				let assets = Self::reanchored(assets, &dest)?;
+				// Note that we pass `None` as `maybe_failed_bin` and drop any assets which cannot
+				// be reanchored  because we have already checked all assets out.
+				let assets = Self::reanchored(assets, &dest, None);
 				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)?;
+				// Note that we pass `None` as `maybe_failed_bin` since no assets were ever removed
+				// from Holding.
+				let assets = Self::reanchored(self.holding.min(&assets), &dest, None);
 				let max_weight = max_response_weight;
 				let response = Response::Assets(assets);
 				let instruction = QueryResponse { query_id, response, max_weight };
@@ -497,10 +508,13 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		}
 	}
 
-	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);
-		Ok(assets.into_assets_iter().collect::<Vec<_>>().into())
+	/// NOTE: Any assets which were unable to be reanchored are introduced into `failed_bin`.
+	fn reanchored(
+		mut assets: Assets,
+		dest: &MultiLocation,
+		maybe_failed_bin: Option<&mut Assets>,
+	) -> MultiAssets {
+		assets.reanchor(dest, &Config::LocationInverter::ancestry(), maybe_failed_bin);
+		assets.into_assets_iter().collect::<Vec<_>>().into()
 	}
 }

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

@@ -207,5 +207,6 @@ 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 ancestry() -> MultiLocation;
 	fn invert_location(l: &MultiLocation) -> Result<MultiLocation, ()>;
 }