From 54d1cb9178fd41b535d8d908b021926d01d91f00 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 27 Aug 2021 17:17:51 -0700 Subject: [PATCH] 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 | 4 ++-- scripts/gitlab/lingua.dic | 1 + xcm/pallet-xcm/src/lib.rs | 26 +++++++++++++++------ xcm/pallet-xcm/src/tests.rs | 5 ++-- xcm/src/v2/traits.rs | 1 + xcm/xcm-builder/src/location_conversion.rs | 27 +++++++++++++++------- xcm/xcm-executor/src/lib.rs | 18 ++++++++------- xcm/xcm-executor/src/traits/conversion.rs | 2 +- 8 files changed, 56 insertions(+), 28 deletions(-) diff --git a/runtime/test-runtime/src/xcm_config.rs b/runtime/test-runtime/src/xcm_config.rs index 3a25b8c9ff2b..23c770f94a28 100644 --- a/runtime/test-runtime/src/xcm_config.rs +++ b/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 { + Ok(Here.into()) } } diff --git a/scripts/gitlab/lingua.dic b/scripts/gitlab/lingua.dic index e7b4348cbaec..c9fec231386d 100644 --- a/scripts/gitlab/lingua.dic +++ b/scripts/gitlab/lingua.dic @@ -105,6 +105,7 @@ intrinsics invariant/MS invariants inverter/MS +invertible io IP/S isn diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index 83f8b65524ed..bbb477971ec3 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/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::::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::::DestinationNotInvertible)?; let fees = assets .get(fee_asset_item as usize) .ok_or(Error::::Empty)? @@ -392,7 +396,8 @@ pub mod pallet { let value = (origin_location, assets.drain()); ensure!(T::XcmReserveTransferFilter::contains(&value), Error::::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::::DestinationNotInvertible)?; let fees = assets .get(fee_asset_item as usize) .ok_or(Error::::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 { + 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<::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: ::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. diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs index c58e04261072..8bafde3cbb94 100644 --- a/xcm/pallet-xcm/src/tests.rs +++ b/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![ diff --git a/xcm/src/v2/traits.rs b/xcm/src/v2/traits.rs index d5d96a5d0c46..0db5f7ef3ba7 100644 --- a/xcm/src/v2/traits.rs +++ b/xcm/src/v2/traits.rs @@ -38,6 +38,7 @@ pub enum Error { UntrustedTeleportLocation, DestinationBufferOverflow, MultiLocationFull, + MultiLocationNotInvertible, FailedToDecode, BadOrigin, ExceedsMaxMessageSize, diff --git a/xcm/xcm-builder/src/location_conversion.rs b/xcm/xcm-builder/src/location_conversion.rs index b067d98b8ede..be6e5aa629c0 100644 --- a/xcm/xcm-builder/src/location_conversion.rs +++ b/xcm/xcm-builder/src/location_conversion.rs @@ -174,24 +174,24 @@ impl, 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::::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(PhantomData); impl> InvertLocation for LocationInverter { - fn invert_location(location: &MultiLocation) -> MultiLocation { + fn invert_location(location: &MultiLocation) -> Result { 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::::invert_location(&input); + let inverted = LocationInverter::::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::::invert_location(&input); + let inverted = LocationInverter::::invert_location(&input).unwrap(); assert_eq!(inverted, X2(account20(), account20()).into()); } @@ -259,7 +259,18 @@ mod tests { } let input = MultiLocation::grandparent(); - let inverted = LocationInverter::::invert_location(&input); + let inverted = LocationInverter::::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::::invert_location(&input); + assert_eq!(inverted, Err(())); + } } diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 01a8e96b516b..85daaa4a5ebe 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -244,7 +244,8 @@ impl XcmExecutor { 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 XcmExecutor { 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 XcmExecutor { 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 XcmExecutor { } } - fn reanchored(mut assets: Assets, dest: &MultiLocation) -> MultiAssets { - let inv_dest = Config::LocationInverter::invert_location(&dest); + fn reanchored(mut assets: Assets, dest: &MultiLocation) -> Result { + let inv_dest = Config::LocationInverter::invert_location(&dest) + .map_err(|()| XcmError::MultiLocationNotInvertible)?; assets.prepend_location(&inv_dest); - assets.into_assets_iter().collect::>().into() + Ok(assets.into_assets_iter().collect::>().into()) } } diff --git a/xcm/xcm-executor/src/traits/conversion.rs b/xcm/xcm-executor/src/traits/conversion.rs index edfa1a96029f..7d2a2f50ba75 100644 --- a/xcm/xcm-executor/src/traits/conversion.rs +++ b/xcm/xcm-executor/src/traits/conversion.rs @@ -200,5 +200,5 @@ impl ConvertOrigin 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; }