Fix TransactAsset Implementation (#3345)

* convert local AssetNotFound errors into XcmError::AssetNotFound

aims allow the tuple implementation of TransactAsset to iterate properly

* add more XcmError descriptions

* adjust the rest of the TransactAsset tuple implementation to the behavior of can_check_in

* fix copy paste errors

* keep iterating tuple on Unimplemented error in TransactAsset

* add tests for tuple implementation of TransactAsset

* Update xcm/src/v0/traits.rs

Co-authored-by: Andronik Ordian <write@reusable.software>

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Andronik Ordian <write@reusable.software>

polkadot/xcm/src/v0/traits.rs

@@ -24,11 +24,14 @@ use super::{MultiLocation, Xcm};
 #[derive(Clone, Encode, Decode, Eq, PartialEq, Debug)]
 pub enum Error {
 	Undefined,
+	/// An arithmetic overflow happened.
 	Overflow,
 	/// The operation is intentionally unsupported.
 	Unimplemented,
 	UnhandledXcmVersion,
+	/// The implementation does not handle a given XCM.
 	UnhandledXcmMessage,
+	/// The implementation does not handle an effect present in an XCM.
 	UnhandledEffect,
 	EscalationOfPrivilege,
 	UntrustedReserveLocation,
@@ -43,10 +46,15 @@ pub enum Error {
 	FailedToDecode,
 	BadOrigin,
 	ExceedsMaxMessageSize,
+	/// An asset transaction (like withdraw or deposit) failed.
+	/// See implementers of the `TransactAsset` trait for sources.
+	/// Causes can include type conversion failures between id or balance types.
 	FailedToTransactAsset(#[codec(skip)] &'static str),
 	/// Execution of the XCM would potentially result in a greater weight used than the pre-specified
 	/// weight limit. The amount that is potentially required is the parameter.
 	WeightLimitReached(Weight),
+	/// An asset wildcard was passed where it was not expected (e.g. as the asset to withdraw in a
+	/// `WithdrawAsset` XCM).
 	Wildcard,
 	/// The case where an XCM message has specified a optional weight limit and the weight required for
 	/// processing is too great.

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

@@ -37,7 +37,7 @@ impl From<Error> for XcmError {
 	fn from(e: Error) -> Self {
 		use XcmError::FailedToTransactAsset;
 		match e {
-			Error::AssetNotFound => FailedToTransactAsset("AssetNotFound"),
+			Error::AssetNotFound => XcmError::AssetNotFound,
 			Error::AccountIdConversionFailed => FailedToTransactAsset("AccountIdConversionFailed"),
 			Error::AmountToBalanceConversionFailed => FailedToTransactAsset("AmountToBalanceConversionFailed"),
 		}

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

@@ -33,7 +33,7 @@ impl From<Error> for XcmError {
 	fn from(e: Error) -> Self {
 		use XcmError::FailedToTransactAsset;
 		match e {
-			Error::AssetNotFound => FailedToTransactAsset("AssetNotFound"),
+			Error::AssetNotFound => XcmError::AssetNotFound,
 			Error::AccountIdConversionFailed => FailedToTransactAsset("AccountIdConversionFailed"),
 			Error::AmountToBalanceConversionFailed => FailedToTransactAsset("AmountToBalanceConversionFailed"),
 			Error::AssetIdConversionFailed => FailedToTransactAsset("AssetIdConversionFailed"),

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

@@ -103,7 +103,7 @@ impl TransactAsset for Tuple {
 	fn can_check_in(origin: &MultiLocation, what: &MultiAsset) -> XcmResult {
 		for_tuples!( #(
 			match Tuple::can_check_in(origin, what) {
-				Err(XcmError::AssetNotFound) => (),
+				Err(XcmError::AssetNotFound | XcmError::Unimplemented) => (),
 				r => return r,
 			}
 		)* );
@@ -130,7 +130,10 @@ impl TransactAsset for Tuple {
 
 	fn deposit_asset(what: &MultiAsset, who: &MultiLocation) -> XcmResult {
 		for_tuples!( #(
-			match Tuple::deposit_asset(what, who) { o @ Ok(_) => return o, _ => () }
+			match Tuple::deposit_asset(what, who) {
+				Err(XcmError::AssetNotFound | XcmError::Unimplemented) => (),
+				r => return r,
+			}
 		)* );
 		log::trace!(
 			target: "xcm::TransactAsset::deposit_asset",
@@ -138,25 +141,31 @@ impl TransactAsset for Tuple {
 			what,
 			who,
 		);
-		Err(XcmError::Unimplemented)
+		Err(XcmError::AssetNotFound)
 	}
 
 	fn withdraw_asset(what: &MultiAsset, who: &MultiLocation) -> Result<Assets, XcmError> {
 		for_tuples!( #(
-			match Tuple::withdraw_asset(what, who) { o @ Ok(_) => return o, _ => () }
+			match Tuple::withdraw_asset(what, who) {
+				Err(XcmError::AssetNotFound | XcmError::Unimplemented) => (),
+				r => return r,
+			}
 		)* );
 		log::trace!(
 			target: "xcm::TransactAsset::withdraw_asset",
 			"did not withdraw asset: what: {:?}, who: {:?}",
-			what, 
+			what,
 			who,
 		);
-		Err(XcmError::Unimplemented)
+		Err(XcmError::AssetNotFound)
 	}
 
 	fn transfer_asset(what: &MultiAsset, from: &MultiLocation, to: &MultiLocation) -> Result<Assets, XcmError> {
 		for_tuples!( #(
-			match Tuple::transfer_asset(what, from, to) { o @ Ok(_) => return o, _ => () }
+			match Tuple::transfer_asset(what, from, to) {
+				Err(XcmError::AssetNotFound | XcmError::Unimplemented) => (),
+				r => return r,
+			}
 		)* );
 		log::trace!(
 			target: "xcm::TransactAsset::transfer_asset",
@@ -165,6 +174,99 @@ impl TransactAsset for Tuple {
 			from,
 			to,
 		);
-		Err(XcmError::Unimplemented)
+		Err(XcmError::AssetNotFound)
+	}
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	pub struct UnimplementedTransactor;
+	impl TransactAsset for UnimplementedTransactor {}
+
+	pub struct NotFoundTransactor;
+	impl TransactAsset for NotFoundTransactor {
+		fn can_check_in(_origin: &MultiLocation, _what: &MultiAsset) -> XcmResult {
+			Err(XcmError::AssetNotFound)
+		}
+
+		fn deposit_asset(_what: &MultiAsset, _who: &MultiLocation) -> XcmResult {
+			Err(XcmError::AssetNotFound)
+		}
+
+		fn withdraw_asset(_what: &MultiAsset, _who: &MultiLocation) -> Result<Assets, XcmError> {
+			Err(XcmError::AssetNotFound)
+		}
+
+		fn transfer_asset(_what: &MultiAsset, _from: &MultiLocation, _to: &MultiLocation) -> Result<Assets, XcmError> {
+			Err(XcmError::AssetNotFound)
+		}
+	}
+
+	pub struct OverflowTransactor;
+	impl TransactAsset for OverflowTransactor {
+		fn can_check_in(_origin: &MultiLocation, _what: &MultiAsset) -> XcmResult {
+			Err(XcmError::Overflow)
+		}
+
+		fn deposit_asset(_what: &MultiAsset, _who: &MultiLocation) -> XcmResult {
+			Err(XcmError::Overflow)
+		}
+
+		fn withdraw_asset(_what: &MultiAsset, _who: &MultiLocation) -> Result<Assets, XcmError> {
+			Err(XcmError::Overflow)
+		}
+
+		fn transfer_asset(_what: &MultiAsset, _from: &MultiLocation, _to: &MultiLocation) -> Result<Assets, XcmError> {
+			Err(XcmError::Overflow)
+		}
+	}
+
+	pub struct SuccessfulTransactor;
+	impl TransactAsset for SuccessfulTransactor {
+		fn can_check_in(_origin: &MultiLocation, _what: &MultiAsset) -> XcmResult {
+			Ok(())
+		}
+
+		fn deposit_asset(_what: &MultiAsset, _who: &MultiLocation) -> XcmResult {
+			Ok(())
+		}
+
+		fn withdraw_asset(_what: &MultiAsset, _who: &MultiLocation) -> Result<Assets, XcmError> {
+			Ok(Assets::default())
+		}
+
+		fn transfer_asset(_what: &MultiAsset, _from: &MultiLocation, _to: &MultiLocation) -> Result<Assets, XcmError> {
+			Ok(Assets::default())
+		}
+	}
+
+	#[test]
+	fn defaults_to_asset_not_found() {
+		type MultiTransactor = (UnimplementedTransactor, NotFoundTransactor, UnimplementedTransactor);
+
+		assert_eq!(MultiTransactor::deposit_asset(&MultiAsset::All, &MultiLocation::Null), Err(XcmError::AssetNotFound));
+	}
+
+	#[test]
+	fn unimplemented_and_not_found_continue_iteration() {
+		type MultiTransactor = (UnimplementedTransactor, NotFoundTransactor, SuccessfulTransactor);
+
+		assert_eq!(MultiTransactor::deposit_asset(&MultiAsset::All, &MultiLocation::Null), Ok(()));
+	}
+
+	#[test]
+	fn unexpected_error_stops_iteration() {
+		type MultiTransactor = (OverflowTransactor, SuccessfulTransactor);
+
+		assert_eq!(MultiTransactor::deposit_asset(&MultiAsset::All, &MultiLocation::Null), Err(XcmError::Overflow));
+	}
+
+	#[test]
+	fn success_stops_iteration() {
+		type MultiTransactor = (SuccessfulTransactor, OverflowTransactor);
+
+		assert_eq!(MultiTransactor::deposit_asset(&MultiAsset::All, &MultiLocation::Null), Ok(()));
 	}
 }