XCM: Fix issue with RequestUnlock

xcm/src/v3/mod.rs

@@ -48,8 +48,8 @@ pub use multilocation::{
 	Ancestor, AncestorThen, InteriorMultiLocation, MultiLocation, Parent, ParentThen,
 };
 pub use traits::{
-	send_xcm, validate_send, Error, ExecuteXcm, Outcome, PreparedMessage, Result, SendError,
-	SendResult, SendXcm, Unwrappable, Weight, XcmHash,
+	send_xcm, validate_send, Error, ExecuteXcm, Outcome, PreparedMessage, Reanchorable, Result,
+	SendError, SendResult, SendXcm, Unwrappable, Weight, XcmHash,
 };
 // These parts of XCM v2 are unchanged in XCM v3, and are re-imported here.
 pub use super::v2::OriginKind;
@@ -183,8 +183,8 @@ pub mod prelude {
 			MultiAssets, MultiLocation,
 			NetworkId::{self, *},
 			OriginKind, Outcome, PalletInfo, Parent, ParentThen, PreparedMessage, QueryId,
-			QueryResponseInfo, Response, Result as XcmResult, SendError, SendResult, SendXcm,
-			Unwrappable,
+			QueryResponseInfo, Reanchorable, Response, Result as XcmResult, SendError, SendResult,
+			SendXcm, Unwrappable,
 			WeightLimit::{self, *},
 			WildFungibility::{self, Fungible as WildFungible, NonFungible as WildNonFungible},
 			WildMultiAsset::{self, *},

xcm/src/v3/multiasset.rs

@@ -24,11 +24,14 @@
 //!   account.
 
 use super::{InteriorMultiLocation, MultiLocation};
-use crate::v2::{
-	AssetId as OldAssetId, AssetInstance as OldAssetInstance, Fungibility as OldFungibility,
-	MultiAsset as OldMultiAsset, MultiAssetFilter as OldMultiAssetFilter,
-	MultiAssets as OldMultiAssets, WildFungibility as OldWildFungibility,
-	WildMultiAsset as OldWildMultiAsset,
+use crate::{
+	latest::Reanchorable,
+	v2::{
+		AssetId as OldAssetId, AssetInstance as OldAssetInstance, Fungibility as OldFungibility,
+		MultiAsset as OldMultiAsset, MultiAssetFilter as OldMultiAssetFilter,
+		MultiAssets as OldMultiAssets, WildFungibility as OldWildFungibility,
+		WildMultiAsset as OldWildMultiAsset,
+	},
 };
 use alloc::{vec, vec::Vec};
 use core::{
@@ -374,9 +377,22 @@ impl AssetId {
 		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 `context`.
-	pub fn reanchor(
+	/// 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 }
+	}
+
+	/// Use the value of `self` along with a `fun` fungibility specifier to create the corresponding `WildMultiAsset`
+	/// wildcard (`AllOf`) value.
+	pub fn into_wild(self, fun: WildFungibility) -> WildMultiAsset {
+		WildMultiAsset::AllOf { fun, id: self }
+	}
+}
+
+impl Reanchorable for AssetId {
+	type Error = ();
+
+	fn reanchor(
 		&mut self,
 		target: &MultiLocation,
 		context: InteriorMultiLocation,
@@ -387,15 +403,15 @@ impl AssetId {
 		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 }
-	}
-
-	/// Use the value of `self` along with a `fun` fungibility specifier to create the corresponding `WildMultiAsset`
-	/// wildcard (`AllOf`) value.
-	pub fn into_wild(self, fun: WildFungibility) -> WildMultiAsset {
-		WildMultiAsset::AllOf { fun, id: self }
+	fn reanchored(
+		mut self,
+		target: &MultiLocation,
+		context: InteriorMultiLocation,
+	) -> Result<Self, ()> {
+		match self.reanchor(target, context) {
+			Ok(()) => Ok(self),
+			Err(()) => Err(()),
+		}
 	}
 }
 
@@ -448,39 +464,39 @@ impl MultiAsset {
 		self.id.prepend_with(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 `context`.
-	pub fn reanchor(
+	/// Returns true if `self` is a super-set of the given `inner` asset.
+	pub fn contains(&self, inner: &MultiAsset) -> bool {
+		use Fungibility::*;
+		if self.id == inner.id {
+			match (&self.fun, &inner.fun) {
+				(Fungible(a), Fungible(i)) if a >= i => return true,
+				(NonFungible(a), NonFungible(i)) if a == i => return true,
+				_ => (),
+			}
+		}
+		false
+	}
+}
+
+impl Reanchorable for MultiAsset {
+	type Error = ();
+
+	fn reanchor(
 		&mut self,
 		target: &MultiLocation,
 		context: InteriorMultiLocation,
 	) -> Result<(), ()> {
 		self.id.reanchor(target, context)
 	}
 
-	/// 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 `context`.
-	pub fn reanchored(
+	fn reanchored(
 		mut self,
 		target: &MultiLocation,
 		context: InteriorMultiLocation,
 	) -> Result<Self, ()> {
 		self.id.reanchor(target, context)?;
 		Ok(self)
 	}
-
-	/// Returns true if `self` is a super-set of the given `inner` asset.
-	pub fn contains(&self, inner: &MultiAsset) -> bool {
-		use Fungibility::*;
-		if self.id == inner.id {
-			match (&self.fun, &inner.fun) {
-				(Fungible(a), Fungible(i)) if a >= i => return true,
-				(NonFungible(a), NonFungible(i)) if a == i => return true,
-				_ => (),
-			}
-		}
-		false
-	}
 }
 
 impl TryFrom<OldMultiAsset> for MultiAsset {

xcm/src/v3/multilocation.rs

@@ -16,7 +16,7 @@
 
 //! XCM `MultiLocation` datatype.
 
-use super::{Junction, Junctions};
+use super::{traits::Reanchorable, Junction, Junctions};
 use crate::{v2::MultiLocation as OldMultiLocation, VersionedMultiLocation};
 use core::{
 	convert::{TryFrom, TryInto},
@@ -385,11 +385,30 @@ impl MultiLocation {
 		}
 	}
 
-	/// Mutate `self` so that it represents the same location from the point of view of `target`.
-	/// The context of `self` is provided as `context`.
-	///
-	/// Does not modify `self` in case of overflow.
-	pub fn reanchor(
+	/// 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,
+			}
+		}
+	}
+}
+
+impl Reanchorable for MultiLocation {
+	type Error = Self;
+
+	fn reanchor(
 		&mut self,
 		target: &MultiLocation,
 		context: InteriorMultiLocation,
@@ -410,11 +429,7 @@ impl MultiLocation {
 		Ok(())
 	}
 
-	/// Consume `self` and return a new value representing the same location from the point of view
-	/// of `target`. The context of `self` is provided as `context`.
-	///
-	/// Returns the original `self` in case of overflow.
-	pub fn reanchored(
+	fn reanchored(
 		mut self,
 		target: &MultiLocation,
 		context: InteriorMultiLocation,
@@ -424,25 +439,6 @@ impl MultiLocation {
 			Err(()) => Err(self),
 		}
 	}
-
-	/// 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,
-			}
-		}
-	}
 }
 
 impl TryFrom<OldMultiLocation> for MultiLocation {

xcm/src/v3/traits.rs

@@ -382,6 +382,32 @@ impl<T> Unwrappable for Option<T> {
 	}
 }
 
+/// TODO: Add documentation
+pub trait Reanchorable: Sized {
+	/// Type to return in case of an error.
+	type Error;
+
+	/// Mutate `self` so that it represents the same location from the point of view of `target`.
+	/// The context of `self` is provided as `context`.
+	///
+	/// Does not modify `self` in case of overflow.
+	fn reanchor(
+		&mut self,
+		target: &MultiLocation,
+		context: InteriorMultiLocation,
+	) -> core::result::Result<(), ()>;
+
+	/// Consume `self` and return a new value representing the same location from the point of view
+	/// of `target`. The context of `self` is provided as `context`.
+	///
+	/// Returns the original `self` in case of overflow.
+	fn reanchored(
+		self,
+		target: &MultiLocation,
+		context: InteriorMultiLocation,
+	) -> core::result::Result<Self, Self::Error>;
+}
+
 /// Utility for sending an XCM message to a given location.
 ///
 /// These can be amalgamated in tuples to form sophisticated routing systems. In tuple format, each

xcm/xcm-executor/src/assets.rs

@@ -23,7 +23,7 @@ use sp_std::{
 use xcm::latest::{
 	AssetId, AssetInstance,
 	Fungibility::{Fungible, NonFungible},
-	InteriorMultiLocation, MultiAsset, MultiAssetFilter, MultiAssets, MultiLocation,
+	InteriorMultiLocation, MultiAsset, MultiAssetFilter, MultiAssets, MultiLocation, Reanchorable,
 	WildFungibility::{Fungible as WildFungible, NonFungible as WildNonFungible},
 	WildMultiAsset::{All, AllCounted, AllOf, AllOfCounted},
 };

xcm/xcm-executor/src/lib.rs

@@ -873,9 +873,11 @@ impl<Config: config::Config> XcmExecutor<Config> {
 			RequestUnlock { asset, locker } => {
 				let origin = *self.origin_ref().ok_or(XcmError::BadOrigin)?;
 				let remote_asset = Self::try_reanchor(asset.clone(), &locker)?.0;
+				let remote_target = Self::try_reanchor(origin.clone(), &locker)?.0;
 				let reduce_ticket =
 					Config::AssetLocker::prepare_reduce_unlockable(locker, asset, origin)?;
-				let msg = Xcm::<()>(vec![UnlockAsset { asset: remote_asset, target: origin }]);
+				let msg =
+					Xcm::<()>(vec![UnlockAsset { asset: remote_asset, target: remote_target }]);
 				let (ticket, price) = validate_send::<Config::XcmSender>(locker, msg)?;
 				self.take_fee(price, FeeReason::RequestUnlock)?;
 				reduce_ticket.enact()?;
@@ -976,15 +978,18 @@ impl<Config: config::Config> XcmExecutor<Config> {
 		Config::XcmSender::deliver(ticket).map_err(Into::into)
 	}
 
-	fn try_reanchor(
-		asset: MultiAsset,
+	fn try_reanchor<T>(
+		reanchorable: T,
 		destination: &MultiLocation,
-	) -> Result<(MultiAsset, InteriorMultiLocation), XcmError> {
+	) -> Result<(T, InteriorMultiLocation), XcmError>
+	where
+		T: Reanchorable,
+	{
 		let reanchor_context = Config::UniversalLocation::get();
-		let asset = asset
+		let reanchored = reanchorable
 			.reanchored(&destination, reanchor_context)
-			.map_err(|()| XcmError::ReanchorFailed)?;
-		Ok((asset, reanchor_context))
+			.map_err(|_| XcmError::ReanchorFailed)?;
+		Ok((reanchored, reanchor_context))
 	}
 
 	/// NOTE: Any assets which were unable to be reanchored are introduced into `failed_bin`.

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

@@ -272,7 +272,7 @@ mod tests {
 	}
 
 	#[test]
-	fn remote_locking() {
+	fn remote_locking_and_unlocking() {
 		MockNet::reset();
 
 		let locked_amount = 100;
@@ -306,6 +306,30 @@ mod tests {
 				}])]
 			);
 		});
+
+		ParaB::execute_with(|| {
+			// Request unlocking part of the funds on the relay chain
+			let message = Xcm(vec![RequestUnlock {
+				asset: (Parent, locked_amount - 50).into(),
+				locker: Parent.into(),
+			}]);
+			assert_ok!(ParachainPalletXcm::send_xcm(Here, (Parent, Parachain(1)), message));
+		});
+
+		Relay::execute_with(|| {
+			use pallet_balances::{BalanceLock, Reasons};
+			// Lock is reduced
+			assert_eq!(
+				relay_chain::Balances::locks(&child_account_id(2)),
+				vec![BalanceLock {
+					id: *b"py/xcmlk",
+					amount: locked_amount - 50,
+					reasons: Reasons::All
+				}]
+			);
+		});
+
+		// TODO: Test case where full lock is unlocked
 	}
 
 	/// Scenario: