Unverified Commit 54d1cb91 authored by Keith Yeung's avatar Keith Yeung Committed by GitHub
Browse files

Return a Result in invert_location (#3730)

* Return a Result in invert_location

* Add invertible to spellcheck dictionary

* Fix Some -> Ok
parent 8ea76695
Pipeline #154634 passed with stages
in 40 minutes and 58 seconds
...@@ -68,8 +68,8 @@ impl WeightTrader for DummyWeightTrader { ...@@ -68,8 +68,8 @@ impl WeightTrader for DummyWeightTrader {
pub struct InvertNothing; pub struct InvertNothing;
impl InvertLocation for InvertNothing { impl InvertLocation for InvertNothing {
fn invert_location(_: &MultiLocation) -> MultiLocation { fn invert_location(_: &MultiLocation) -> sp_std::result::Result<MultiLocation, ()> {
Here.into() Ok(Here.into())
} }
} }
......
...@@ -105,6 +105,7 @@ intrinsics ...@@ -105,6 +105,7 @@ intrinsics
invariant/MS invariant/MS
invariants invariants
inverter/MS inverter/MS
invertible
io io
IP/S IP/S
isn isn
......
...@@ -31,6 +31,7 @@ use sp_std::{ ...@@ -31,6 +31,7 @@ use sp_std::{
convert::{TryFrom, TryInto}, convert::{TryFrom, TryInto},
marker::PhantomData, marker::PhantomData,
prelude::*, prelude::*,
result::Result,
vec, vec,
}; };
use xcm::{ use xcm::{
...@@ -206,6 +207,8 @@ pub mod pallet { ...@@ -206,6 +207,8 @@ pub mod pallet {
Filtered, Filtered,
/// The message's weight could not be determined. /// The message's weight could not be determined.
UnweighableMessage, UnweighableMessage,
/// The destination `MultiLocation` provided cannot be inverted.
DestinationNotInvertible,
/// The assets to be sent are empty. /// The assets to be sent are empty.
Empty, Empty,
/// Could not re-anchor the assets to declare the fees for the destination chain. /// Could not re-anchor the assets to declare the fees for the destination chain.
...@@ -322,7 +325,8 @@ pub mod pallet { ...@@ -322,7 +325,8 @@ pub mod pallet {
let value = (origin_location, assets.drain()); let value = (origin_location, assets.drain());
ensure!(T::XcmTeleportFilter::contains(&value), Error::<T>::Filtered); ensure!(T::XcmTeleportFilter::contains(&value), Error::<T>::Filtered);
let (origin_location, assets) = value; let (origin_location, assets) = value;
let inv_dest = T::LocationInverter::invert_location(&dest); let inv_dest = T::LocationInverter::invert_location(&dest)
.map_err(|()| Error::<T>::DestinationNotInvertible)?;
let fees = assets let fees = assets
.get(fee_asset_item as usize) .get(fee_asset_item as usize)
.ok_or(Error::<T>::Empty)? .ok_or(Error::<T>::Empty)?
...@@ -392,7 +396,8 @@ pub mod pallet { ...@@ -392,7 +396,8 @@ pub mod pallet {
let value = (origin_location, assets.drain()); let value = (origin_location, assets.drain());
ensure!(T::XcmReserveTransferFilter::contains(&value), Error::<T>::Filtered); ensure!(T::XcmReserveTransferFilter::contains(&value), Error::<T>::Filtered);
let (origin_location, assets) = value; let (origin_location, assets) = value;
let inv_dest = T::LocationInverter::invert_location(&dest); let inv_dest = T::LocationInverter::invert_location(&dest)
.map_err(|()| Error::<T>::DestinationNotInvertible)?;
let fees = assets let fees = assets
.get(fee_asset_item as usize) .get(fee_asset_item as usize)
.ok_or(Error::<T>::Empty)? .ok_or(Error::<T>::Empty)?
...@@ -489,18 +494,21 @@ pub mod pallet { ...@@ -489,18 +494,21 @@ pub mod pallet {
/// - `timeout`: The block number after which it is permissible for `notify` not to be /// - `timeout`: The block number after which it is permissible for `notify` not to be
/// called even if a response is received. /// 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` /// To check the status of the query, use `fn query()` passing the resultant `QueryId`
/// value. /// value.
pub fn report_outcome( pub fn report_outcome(
message: &mut Xcm<()>, message: &mut Xcm<()>,
responder: MultiLocation, responder: MultiLocation,
timeout: T::BlockNumber, timeout: T::BlockNumber,
) -> QueryId { ) -> Result<QueryId, XcmError> {
let dest = T::LocationInverter::invert_location(&responder); let dest = T::LocationInverter::invert_location(&responder)
.map_err(|()| XcmError::MultiLocationNotInvertible)?;
let query_id = Self::new_query(responder, timeout); let query_id = Self::new_query(responder, timeout);
let report_error = Xcm(vec![ReportError { dest, query_id, max_response_weight: 0 }]); let report_error = Xcm(vec![ReportError { dest, query_id, max_response_weight: 0 }]);
message.0.insert(0, SetAppendix(report_error)); 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 /// Consume `message` and return another which is equivalent to it except that it reports
...@@ -516,6 +524,8 @@ pub mod pallet { ...@@ -516,6 +524,8 @@ pub mod pallet {
/// - `timeout`: The block number after which it is permissible for `notify` not to be /// - `timeout`: The block number after which it is permissible for `notify` not to be
/// called even if a response is received. /// 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 /// 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 /// 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 /// weighing `ReportOutcome` on the way back. If it turns out to be heavier once it returns
...@@ -526,13 +536,15 @@ pub mod pallet { ...@@ -526,13 +536,15 @@ pub mod pallet {
responder: MultiLocation, responder: MultiLocation,
notify: impl Into<<T as Config>::Call>, notify: impl Into<<T as Config>::Call>,
timeout: T::BlockNumber, timeout: T::BlockNumber,
) { ) -> Result<(), XcmError> {
let dest = T::LocationInverter::invert_location(&responder); let dest = T::LocationInverter::invert_location(&responder)
.map_err(|()| XcmError::MultiLocationNotInvertible)?;
let notify: <T as Config>::Call = notify.into(); let notify: <T as Config>::Call = notify.into();
let max_response_weight = notify.get_dispatch_info().weight; let max_response_weight = notify.get_dispatch_info().weight;
let query_id = Self::new_notify_query(responder, notify, timeout); let query_id = Self::new_notify_query(responder, notify, timeout);
let report_error = Xcm(vec![ReportError { dest, query_id, max_response_weight }]); let report_error = Xcm(vec![ReportError { dest, query_id, max_response_weight }]);
message.0.insert(0, SetAppendix(report_error)); 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. /// Attempt to create a new query ID and register it as a query that is yet to respond.
......
...@@ -40,7 +40,8 @@ fn report_outcome_notify_works() { ...@@ -40,7 +40,8 @@ fn report_outcome_notify_works() {
let call = pallet_test_notifier::Call::notification_received(0, Default::default()); let call = pallet_test_notifier::Call::notification_received(0, Default::default());
let notify = Call::TestNotifier(call); let notify = Call::TestNotifier(call);
new_test_ext_with_balances(balances).execute_with(|| { 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!( assert_eq!(
message, message,
Xcm(vec![ Xcm(vec![
...@@ -94,7 +95,7 @@ fn report_outcome_works() { ...@@ -94,7 +95,7 @@ fn report_outcome_works() {
beneficiary: sender.clone(), beneficiary: sender.clone(),
}]); }]);
new_test_ext_with_balances(balances).execute_with(|| { 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!( assert_eq!(
message, message,
Xcm(vec![ Xcm(vec![
......
...@@ -38,6 +38,7 @@ pub enum Error { ...@@ -38,6 +38,7 @@ pub enum Error {
UntrustedTeleportLocation, UntrustedTeleportLocation,
DestinationBufferOverflow, DestinationBufferOverflow,
MultiLocationFull, MultiLocationFull,
MultiLocationNotInvertible,
FailedToDecode, FailedToDecode,
BadOrigin, BadOrigin,
ExceedsMaxMessageSize, ExceedsMaxMessageSize,
......
...@@ -174,24 +174,24 @@ impl<Network: Get<NetworkId>, AccountId: From<[u8; 20]> + Into<[u8; 20]> + Clone ...@@ -174,24 +174,24 @@ impl<Network: Get<NetworkId>, AccountId: From<[u8; 20]> + Into<[u8; 20]> + Clone
/// ///
/// let input = MultiLocation::new(2, X2(Parachain(2), AccountId32 { network: Any, id: Default::default() })); /// let input = MultiLocation::new(2, X2(Parachain(2), AccountId32 { network: Any, id: Default::default() }));
/// let inverted = LocationInverter::<Ancestry>::invert_location(&input); /// let inverted = LocationInverter::<Ancestry>::invert_location(&input);
/// assert_eq!(inverted, MultiLocation::new( /// assert_eq!(inverted, Ok(MultiLocation::new(
/// 2, /// 2,
/// X2(Parachain(1), AccountKey20 { network: Any, key: Default::default() }), /// X2(Parachain(1), AccountKey20 { network: Any, key: Default::default() }),
/// )); /// )));
/// # } /// # }
/// ``` /// ```
pub struct LocationInverter<Ancestry>(PhantomData<Ancestry>); pub struct LocationInverter<Ancestry>(PhantomData<Ancestry>);
impl<Ancestry: Get<MultiLocation>> InvertLocation for LocationInverter<Ancestry> { impl<Ancestry: Get<MultiLocation>> InvertLocation for LocationInverter<Ancestry> {
fn invert_location(location: &MultiLocation) -> MultiLocation { fn invert_location(location: &MultiLocation) -> Result<MultiLocation, ()> {
let mut ancestry = Ancestry::get(); let mut ancestry = Ancestry::get();
let mut junctions = Here; let mut junctions = Here;
for _ in 0..location.parent_count() { for _ in 0..location.parent_count() {
junctions = junctions junctions = junctions
.pushed_with(ancestry.take_first_interior().unwrap_or(OnlyChild)) .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; let parents = location.interior().len() as u8;
MultiLocation::new(parents, junctions) Ok(MultiLocation::new(parents, junctions))
} }
} }
...@@ -229,7 +229,7 @@ mod tests { ...@@ -229,7 +229,7 @@ mod tests {
} }
let input = MultiLocation::new(3, X2(Parachain(2), account32())); let input = MultiLocation::new(3, X2(Parachain(2), account32()));
let inverted = LocationInverter::<Ancestry>::invert_location(&input); let inverted = LocationInverter::<Ancestry>::invert_location(&input).unwrap();
assert_eq!(inverted, MultiLocation::new(2, X3(Parachain(1), account20(), account20()))); assert_eq!(inverted, MultiLocation::new(2, X3(Parachain(1), account20(), account20())));
} }
...@@ -244,7 +244,7 @@ mod tests { ...@@ -244,7 +244,7 @@ mod tests {
} }
let input = MultiLocation::grandparent(); let input = MultiLocation::grandparent();
let inverted = LocationInverter::<Ancestry>::invert_location(&input); let inverted = LocationInverter::<Ancestry>::invert_location(&input).unwrap();
assert_eq!(inverted, X2(account20(), account20()).into()); assert_eq!(inverted, X2(account20(), account20()).into());
} }
...@@ -259,7 +259,18 @@ mod tests { ...@@ -259,7 +259,18 @@ mod tests {
} }
let input = MultiLocation::grandparent(); let input = MultiLocation::grandparent();
let inverted = LocationInverter::<Ancestry>::invert_location(&input); let inverted = LocationInverter::<Ancestry>::invert_location(&input).unwrap();
assert_eq!(inverted, X2(PalletInstance(5), OnlyChild).into()); 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::<Ancestry>::invert_location(&input);
assert_eq!(inverted, Err(()));
}
} }
...@@ -244,7 +244,8 @@ impl<Config: config::Config> XcmExecutor<Config> { ...@@ -244,7 +244,8 @@ impl<Config: config::Config> XcmExecutor<Config> {
TransferReserveAsset { mut assets, dest, xcm } => { TransferReserveAsset { mut assets, dest, xcm } => {
let origin = self.origin.as_ref().ok_or(XcmError::BadOrigin)?; let origin = self.origin.as_ref().ok_or(XcmError::BadOrigin)?;
// Take `assets` from the origin account (on-chain) and place into dest account. // 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() { for asset in assets.inner() {
Config::AssetTransactor::beam_asset(asset, origin, &dest)?; Config::AssetTransactor::beam_asset(asset, origin, &dest)?;
} }
...@@ -343,13 +344,13 @@ impl<Config: config::Config> XcmExecutor<Config> { ...@@ -343,13 +344,13 @@ impl<Config: config::Config> XcmExecutor<Config> {
for asset in deposited.assets_iter() { for asset in deposited.assets_iter() {
Config::AssetTransactor::deposit_asset(&asset, &dest)?; 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]; let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin];
message.extend(xcm.0.into_iter()); message.extend(xcm.0.into_iter());
Config::XcmSender::send_xcm(dest, Xcm(message)).map_err(Into::into) Config::XcmSender::send_xcm(dest, Xcm(message)).map_err(Into::into)
}, },
InitiateReserveWithdraw { assets, reserve, xcm } => { 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]; let mut message = vec![WithdrawAsset(assets), ClearOrigin];
message.extend(xcm.0.into_iter()); message.extend(xcm.0.into_iter());
Config::XcmSender::send_xcm(reserve, Xcm(message)).map_err(Into::into) Config::XcmSender::send_xcm(reserve, Xcm(message)).map_err(Into::into)
...@@ -360,13 +361,13 @@ impl<Config: config::Config> XcmExecutor<Config> { ...@@ -360,13 +361,13 @@ impl<Config: config::Config> XcmExecutor<Config> {
for asset in assets.assets_iter() { for asset in assets.assets_iter() {
Config::AssetTransactor::check_out(&dest, &asset); 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]; let mut message = vec![ReceiveTeleportedAsset(assets), ClearOrigin];
message.extend(xcm.0.into_iter()); message.extend(xcm.0.into_iter());
Config::XcmSender::send_xcm(dest, Xcm(message)).map_err(Into::into) Config::XcmSender::send_xcm(dest, Xcm(message)).map_err(Into::into)
}, },
QueryHolding { query_id, dest, assets, max_response_weight } => { 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 max_weight = max_response_weight;
let response = Response::Assets(assets); let response = Response::Assets(assets);
let instruction = QueryResponse { query_id, response, max_weight }; let instruction = QueryResponse { query_id, response, max_weight };
...@@ -425,9 +426,10 @@ impl<Config: config::Config> XcmExecutor<Config> { ...@@ -425,9 +426,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
} }
} }
fn reanchored(mut assets: Assets, dest: &MultiLocation) -> MultiAssets { fn reanchored(mut assets: Assets, dest: &MultiLocation) -> Result<MultiAssets, XcmError> {
let inv_dest = Config::LocationInverter::invert_location(&dest); let inv_dest = Config::LocationInverter::invert_location(&dest)
.map_err(|()| XcmError::MultiLocationNotInvertible)?;
assets.prepend_location(&inv_dest); assets.prepend_location(&inv_dest);
assets.into_assets_iter().collect::<Vec<_>>().into() Ok(assets.into_assets_iter().collect::<Vec<_>>().into())
} }
} }
...@@ -200,5 +200,5 @@ impl<O> ConvertOrigin<O> for Tuple { ...@@ -200,5 +200,5 @@ impl<O> ConvertOrigin<O> for Tuple {
/// Means of inverting a location: given a location which describes a `target` interpreted from the /// 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`. /// `source`, this will provide the corresponding location which describes the `source`.
pub trait InvertLocation { pub trait InvertLocation {
fn invert_location(l: &MultiLocation) -> MultiLocation; fn invert_location(l: &MultiLocation) -> Result<MultiLocation, ()>;
} }
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment