From 794589755232337394743898e5462a486f26c3c9 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Sat, 2 Apr 2022 16:44:02 +0200 Subject: [PATCH 01/15] Maximise chances that trapped assets can be reclaimed --- xcm/xcm-executor/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index b61bd0fea65b..15ef75489e58 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -331,8 +331,9 @@ impl XcmExecutor { "Trapping assets in holding register: {:?}, context: {:?} (original_origin: {:?})", self.holding, self.context, self.original_origin, ); + let effective_origin = self.context.origin.as_ref().unwrap_or(&self.original_origin); let trap_weight = - Config::AssetTrap::drop_assets(&self.original_origin, self.holding, &self.context); + Config::AssetTrap::drop_assets(effective_origin, self.holding, &self.context); weight_used.saturating_accrue(trap_weight); }; From 2bd0f8cdf1027f67e61b1427275746ec6ac42dac Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Sat, 2 Apr 2022 17:10:47 +0200 Subject: [PATCH 02/15] Do origin check as part of ExportMessage for security --- xcm/pallet-xcm/src/lib.rs | 4 ++-- xcm/src/v2/traits.rs | 4 ++-- xcm/src/v3/junctions.rs | 12 ++++++++++++ xcm/src/v3/traits.rs | 11 +++++++---- xcm/xcm-builder/src/universal_exports.rs | 15 +++------------ xcm/xcm-executor/src/lib.rs | 19 ++++++++++++++++--- 6 files changed, 42 insertions(+), 23 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index 457fe86672f4..2f6179d07dad 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -1340,7 +1340,7 @@ impl Pallet { let responder = responder.into(); let destination = T::UniversalLocation::get() .invert_target(&responder) - .map_err(|()| XcmError::MultiLocationNotInvertible)?; + .map_err(|()| XcmError::LocationNotInvertible)?; let query_id = Self::new_query(responder, timeout, Here); let response_info = QueryResponseInfo { destination, query_id, max_weight: 0 }; let report_error = Xcm(vec![ReportError(response_info)]); @@ -1379,7 +1379,7 @@ impl Pallet { let responder = responder.into(); let destination = T::UniversalLocation::get() .invert_target(&responder) - .map_err(|()| XcmError::MultiLocationNotInvertible)?; + .map_err(|()| XcmError::LocationNotInvertible)?; let notify: ::Call = notify.into(); let max_weight = notify.get_dispatch_info().weight; let query_id = Self::new_notify_query(responder, notify, timeout, Here); diff --git a/xcm/src/v2/traits.rs b/xcm/src/v2/traits.rs index af0240c1ad26..75aa94897993 100644 --- a/xcm/src/v2/traits.rs +++ b/xcm/src/v2/traits.rs @@ -120,8 +120,8 @@ impl TryFrom for Error { Unimplemented => Self::Unimplemented, UntrustedReserveLocation => Self::UntrustedReserveLocation, UntrustedTeleportLocation => Self::UntrustedTeleportLocation, - MultiLocationFull => Self::MultiLocationFull, - MultiLocationNotInvertible => Self::MultiLocationNotInvertible, + LocationFull => Self::MultiLocationFull, + LocationNotInvertible => Self::MultiLocationNotInvertible, BadOrigin => Self::BadOrigin, InvalidLocation => Self::InvalidLocation, AssetNotFound => Self::AssetNotFound, diff --git a/xcm/src/v3/junctions.rs b/xcm/src/v3/junctions.rs index 34815add7fb2..5fd6236e2b1d 100644 --- a/xcm/src/v3/junctions.rs +++ b/xcm/src/v3/junctions.rs @@ -221,6 +221,18 @@ impl Junctions { } } + /// Extract the network ID and the interior consensus location, treating this value as a + /// universal location. + /// + /// This will return an `Err` if the first item is not a `GlobalConsensus`, which would indicate + /// that this value is not a universal location. + pub fn split_global(self) -> Result<(NetworkId, Junctions), ()> { + match self.clone().split_first() { + (location, Some(Junction::GlobalConsensus(network))) => (network, location), + _ => return Err(()), + } + } + /// Consumes `self` and returns how `viewer` would address it locally. pub fn relative_to(mut self, viewer: &Junctions) -> MultiLocation { let mut i = 0; diff --git a/xcm/src/v3/traits.rs b/xcm/src/v3/traits.rs index 6f78cb19ef67..31d0e62d837b 100644 --- a/xcm/src/v3/traits.rs +++ b/xcm/src/v3/traits.rs @@ -41,10 +41,10 @@ pub enum Error { UntrustedTeleportLocation, /// `MultiLocation` value too large to descend further. #[codec(index = 4)] - MultiLocationFull, + LocationFull, /// `MultiLocation` value ascend more parents than known ancestors of local location. #[codec(index = 5)] - MultiLocationNotInvertible, + LocationNotInvertible, /// The Origin Register does not contain a valid value for instruction. #[codec(index = 6)] BadOrigin, @@ -126,6 +126,9 @@ pub enum Error { /// The state was not in a condition where the operation was valid to make. #[codec(index = 32)] NoPermission, + /// The universal location of the local consensus is improper. + #[codec(index = 33)] + Unanchored, // Errors that happen prior to instructions being executed. These fall outside of the XCM spec. /// XCM version not able to be handled. @@ -161,8 +164,8 @@ impl TryFrom for Error { Unimplemented => Self::Unimplemented, UntrustedReserveLocation => Self::UntrustedReserveLocation, UntrustedTeleportLocation => Self::UntrustedTeleportLocation, - MultiLocationFull => Self::MultiLocationFull, - MultiLocationNotInvertible => Self::MultiLocationNotInvertible, + MultiLocationFull => Self::LocationFull, + MultiLocationNotInvertible => Self::LocationNotInvertible, BadOrigin => Self::BadOrigin, InvalidLocation => Self::InvalidLocation, AssetNotFound => Self::AssetNotFound, diff --git a/xcm/xcm-builder/src/universal_exports.rs b/xcm/xcm-builder/src/universal_exports.rs index 5db871135db8..a4996d2b0262 100644 --- a/xcm/xcm-builder/src/universal_exports.rs +++ b/xcm/xcm-builder/src/universal_exports.rs @@ -221,24 +221,15 @@ impl = vec![UniversalOrigin(GlobalConsensus(local_network))].into(); - if local_location != Here { - exported.inner_mut().push(DescendOrigin(local_location)); - } - exported.inner_mut().extend(xcm.take().ok_or(MissingArgument)?.into_iter()); - + let xcm = xcm.take().ok_or(MissingArgument)?; let (bridge, maybe_payment) = - Bridges::exporter_for(&remote_network, &remote_location, &exported) + Bridges::exporter_for(&remote_network, &remote_location, &xcm) .ok_or(NotApplicable)?; let local_from_bridge = UniversalLocation::get().invert_target(&bridge).map_err(|_| Unroutable)?; let export_instruction = - ExportMessage { network: remote_network, destination: remote_location, xcm: exported }; + ExportMessage { network: remote_network, destination: remote_location, xcm }; let message = Xcm(if let Some(ref payment) = maybe_payment { let fees = payment diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 15ef75489e58..b0514b91a692 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -476,7 +476,7 @@ impl XcmExecutor { let reanchor_context = Config::UniversalLocation::get(); assets .reanchor(&dest, reanchor_context) - .map_err(|()| XcmError::MultiLocationFull)?; + .map_err(|()| XcmError::LocationFull)?; let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin]; message.extend(xcm.0.into_iter()); self.send(dest, Xcm(message), FeeReason::TransferReserveAsset)?; @@ -554,7 +554,7 @@ impl XcmExecutor { .as_mut() .ok_or(XcmError::BadOrigin)? .append_with(who) - .map_err(|_| XcmError::MultiLocationFull), + .map_err(|_| XcmError::LocationFull), ClearOrigin => { self.context.origin = None; Ok(()) @@ -768,13 +768,26 @@ impl XcmExecutor { Ok(()) }, ExportMessage { network, destination, xcm } => { + // Prepend the desired message with instructions which effectively rewrite the origin. + // + // This only works because the remote chain empowers the bridge + // to speak for the local network. + let (local_net, local_sub) = Config::UniversalLocation::get() + .split_global() + .ok_or(XcmError::Unanchored)?; + let mut exported: Xcm<()> = vec![UniversalOrigin(GlobalConsensus(local_net))].into(); + if local_sub != Here { + exported.inner_mut().push(DescendOrigin(local_sub)); + } + exported.inner_mut().extend(xcm.into_iter()); + let hash = (self.origin_ref(), &destination).using_encoded(blake2_128); let channel = u32::decode(&mut hash.as_ref()).unwrap_or(0); // Hash identifies the lane on the exporter which we use. We use the pairwise // combination of the origin and destination to ensure origin/destination pairs will // generally have their own lanes. let (ticket, fee) = - validate_export::(network, channel, destination, xcm)?; + validate_export::(network, channel, destination, exported)?; self.take_fee(fee, FeeReason::Export(network))?; Config::MessageExporter::deliver(ticket)?; Ok(()) From 2ce92528b947d98af5605f48b7f27c4542e9be0f Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Sat, 2 Apr 2022 17:15:59 +0200 Subject: [PATCH 03/15] Formatting --- xcm/xcm-builder/src/universal_exports.rs | 3 +-- xcm/xcm-executor/src/lib.rs | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/xcm/xcm-builder/src/universal_exports.rs b/xcm/xcm-builder/src/universal_exports.rs index a4996d2b0262..7a45af4686bf 100644 --- a/xcm/xcm-builder/src/universal_exports.rs +++ b/xcm/xcm-builder/src/universal_exports.rs @@ -223,8 +223,7 @@ impl XcmExecutor { Config::AssetTransactor::beam_asset(asset, origin, &dest, &self.context)?; } let reanchor_context = Config::UniversalLocation::get(); - assets - .reanchor(&dest, reanchor_context) - .map_err(|()| XcmError::LocationFull)?; + assets.reanchor(&dest, reanchor_context).map_err(|()| XcmError::LocationFull)?; let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin]; message.extend(xcm.0.into_iter()); self.send(dest, Xcm(message), FeeReason::TransferReserveAsset)?; @@ -772,10 +770,10 @@ impl XcmExecutor { // // This only works because the remote chain empowers the bridge // to speak for the local network. - let (local_net, local_sub) = Config::UniversalLocation::get() - .split_global() - .ok_or(XcmError::Unanchored)?; - let mut exported: Xcm<()> = vec![UniversalOrigin(GlobalConsensus(local_net))].into(); + let (local_net, local_sub) = + Config::UniversalLocation::get().split_global().ok_or(XcmError::Unanchored)?; + let mut exported: Xcm<()> = + vec![UniversalOrigin(GlobalConsensus(local_net))].into(); if local_sub != Here { exported.inner_mut().push(DescendOrigin(local_sub)); } @@ -786,8 +784,12 @@ impl XcmExecutor { // Hash identifies the lane on the exporter which we use. We use the pairwise // combination of the origin and destination to ensure origin/destination pairs will // generally have their own lanes. - let (ticket, fee) = - validate_export::(network, channel, destination, exported)?; + let (ticket, fee) = validate_export::( + network, + channel, + destination, + exported, + )?; self.take_fee(fee, FeeReason::Export(network))?; Config::MessageExporter::deliver(ticket)?; Ok(()) From 1441a670237326cec4acd589136965f09663462f Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Sat, 2 Apr 2022 17:17:08 +0200 Subject: [PATCH 04/15] Fixes --- xcm/src/v3/junctions.rs | 2 +- xcm/xcm-executor/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/xcm/src/v3/junctions.rs b/xcm/src/v3/junctions.rs index 5fd6236e2b1d..9b3fc2587fe1 100644 --- a/xcm/src/v3/junctions.rs +++ b/xcm/src/v3/junctions.rs @@ -228,7 +228,7 @@ impl Junctions { /// that this value is not a universal location. pub fn split_global(self) -> Result<(NetworkId, Junctions), ()> { match self.clone().split_first() { - (location, Some(Junction::GlobalConsensus(network))) => (network, location), + (location, Some(Junction::GlobalConsensus(network))) => Ok((network, location)), _ => return Err(()), } } diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index d43591618612..0750142bad4b 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -771,7 +771,7 @@ impl XcmExecutor { // This only works because the remote chain empowers the bridge // to speak for the local network. let (local_net, local_sub) = - Config::UniversalLocation::get().split_global().ok_or(XcmError::Unanchored)?; + Config::UniversalLocation::get().split_global().map_err(|()| XcmError::Unanchored)?; let mut exported: Xcm<()> = vec![UniversalOrigin(GlobalConsensus(local_net))].into(); if local_sub != Here { From 91b895ede78d03275d87609bb172b16797847c8f Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Sat, 2 Apr 2022 19:52:14 +0200 Subject: [PATCH 05/15] Cleanup export XCM APIs --- xcm/src/v3/junctions.rs | 18 ++++++ xcm/xcm-builder/src/tests/bridging/mod.rs | 6 +- xcm/xcm-builder/src/tests/mock.rs | 58 +++++++++++++----- xcm/xcm-builder/src/tests/origins.rs | 3 +- xcm/xcm-builder/src/universal_exports.rs | 71 +++++++++-------------- xcm/xcm-executor/src/lib.rs | 18 +++--- xcm/xcm-executor/src/traits/export.rs | 28 +++++++-- 7 files changed, 126 insertions(+), 76 deletions(-) diff --git a/xcm/src/v3/junctions.rs b/xcm/src/v3/junctions.rs index 9b3fc2587fe1..842f60eff04d 100644 --- a/xcm/src/v3/junctions.rs +++ b/xcm/src/v3/junctions.rs @@ -233,6 +233,24 @@ impl Junctions { } } + /// Treat `self` as a universal location and the context of `relative`, returning the universal + /// location of relative. + /// + /// This will return an error if `relative` has as many (or more) parents than there are + /// junctions in `self`, implying that relative refers into a different global consensus. + pub fn within_global(mut self, relative: MultiLocation) -> Result { + if self.len() <= relative.parents as usize { + return Err(()) + } + for _ in 0..relative.parents { + self.take_last(); + } + for j in relative.interior { + self.push(j).map_err(|_| ())?; + } + Ok(self) + } + /// Consumes `self` and returns how `viewer` would address it locally. pub fn relative_to(mut self, viewer: &Junctions) -> MultiLocation { let mut i = 0; diff --git a/xcm/xcm-builder/src/tests/bridging/mod.rs b/xcm/xcm-builder/src/tests/bridging/mod.rs index f145fc2d4d0c..aac51f228b0d 100644 --- a/xcm/xcm-builder/src/tests/bridging/mod.rs +++ b/xcm/xcm-builder/src/tests/bridging/mod.rs @@ -89,19 +89,21 @@ struct UnpaidExecutingRouter( fn price( n: NetworkId, c: u32, + s: &InteriorMultiLocation, d: &InteriorMultiLocation, m: &Xcm<()>, ) -> Result { - Ok(validate_export::(n, c, d.clone(), m.clone())?.1) + Ok(validate_export::(n, c, s.clone(), d.clone(), m.clone())?.1) } fn deliver( n: NetworkId, c: u32, + s: InteriorMultiLocation, d: InteriorMultiLocation, m: Xcm<()>, ) -> Result { - export_xcm::(n, c, d, m).map(|(hash, _)| hash) + export_xcm::(n, c, s, d, m).map(|(hash, _)| hash) } impl, Remote: Get, RemoteExporter: ExportXcm> SendXcm diff --git a/xcm/xcm-builder/src/tests/mock.rs b/xcm/xcm-builder/src/tests/mock.rs index dc5b427e9674..079a6183c5d2 100644 --- a/xcm/xcm-builder/src/tests/mock.rs +++ b/xcm/xcm-builder/src/tests/mock.rs @@ -107,10 +107,24 @@ impl GetDispatchInfo for TestCall { thread_local! { pub static SENT_XCM: RefCell, XcmHash)>> = RefCell::new(Vec::new()); - pub static EXPORTED_XCM: RefCell, XcmHash)>> = RefCell::new(Vec::new()); + pub static EXPORTED_XCM: RefCell< + Vec<(NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, Xcm<()>, XcmHash)> + > = RefCell::new(Vec::new()); pub static EXPORTER_OVERRIDE: RefCell) -> Result, - fn(NetworkId, u32, InteriorMultiLocation, Xcm<()>) -> Result, + fn( + NetworkId, + u32, + &InteriorMultiLocation, + &InteriorMultiLocation, + &Xcm<()>, + ) -> Result, + fn( + NetworkId, + u32, + InteriorMultiLocation, + InteriorMultiLocation, + Xcm<()>, + ) -> Result, )>> = RefCell::new(None); pub static SEND_PRICE: RefCell = RefCell::new(MultiAssets::new()); } @@ -120,12 +134,24 @@ pub fn sent_xcm() -> Vec<(MultiLocation, opaque::Xcm, XcmHash)> { pub fn set_send_price(p: impl Into) { SEND_PRICE.with(|l| l.replace(p.into().into())); } -pub fn exported_xcm() -> Vec<(NetworkId, u32, InteriorMultiLocation, opaque::Xcm, XcmHash)> { +pub fn exported_xcm() -> Vec<(NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, opaque::Xcm, XcmHash)> { EXPORTED_XCM.with(|q| (*q.borrow()).clone()) } pub fn set_exporter_override( - price: fn(NetworkId, u32, &InteriorMultiLocation, &Xcm<()>) -> Result, - deliver: fn(NetworkId, u32, InteriorMultiLocation, Xcm<()>) -> Result, + price: fn( + NetworkId, + u32, + &InteriorMultiLocation, + &InteriorMultiLocation, + &Xcm<()>, + ) -> Result, + deliver: fn( + NetworkId, + u32, + InteriorMultiLocation, + InteriorMultiLocation, + Xcm<()>, + ) -> Result, ) { EXPORTER_OVERRIDE.with(|x| x.replace(Some((price, deliver)))); } @@ -153,25 +179,27 @@ impl SendXcm for TestMessageSender { } pub struct TestMessageExporter; impl ExportXcm for TestMessageExporter { - type Ticket = (NetworkId, u32, InteriorMultiLocation, Xcm<()>, XcmHash); + type Ticket = (NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, Xcm<()>, XcmHash); fn validate( network: NetworkId, channel: u32, + uni_src: &mut Option, dest: &mut Option, msg: &mut Option>, - ) -> SendResult<(NetworkId, u32, InteriorMultiLocation, Xcm<()>, XcmHash)> { - let (d, m) = (dest.take().unwrap(), msg.take().unwrap()); + ) -> SendResult<(NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, Xcm<()>, XcmHash)> { + let (s, d, m) = (uni_src.take().unwrap(), dest.take().unwrap(), msg.take().unwrap()); let r: Result = EXPORTER_OVERRIDE.with(|e| { if let Some((ref f, _)) = &*e.borrow() { - f(network, channel, &d, &m) + f(network, channel, &s, &d, &m) } else { Ok(MultiAssets::new()) } }); let h = fake_message_hash(&m); match r { - Ok(price) => Ok(((network, channel, d, m, h), price)), + Ok(price) => Ok(((network, channel, s, d, m, h), price)), Err(e) => { + *uni_src = Some(s); *dest = Some(d); *msg = Some(m); Err(e) @@ -179,14 +207,14 @@ impl ExportXcm for TestMessageExporter { } } fn deliver( - tuple: (NetworkId, u32, InteriorMultiLocation, Xcm<()>, XcmHash), + tuple: (NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, Xcm<()>, XcmHash), ) -> Result { EXPORTER_OVERRIDE.with(|e| { if let Some((_, ref f)) = &*e.borrow() { - let (network, channel, dest, msg, _hash) = tuple; - f(network, channel, dest, msg) + let (network, channel, uni_src, dest, msg, _hash) = tuple; + f(network, channel, uni_src, dest, msg) } else { - let hash = tuple.4; + let hash = tuple.5; EXPORTED_XCM.with(|q| q.borrow_mut().push(tuple)); Ok(hash) } diff --git a/xcm/xcm-builder/src/tests/origins.rs b/xcm/xcm-builder/src/tests/origins.rs index 9b75c4f98650..d0dea4f49b40 100644 --- a/xcm/xcm-builder/src/tests/origins.rs +++ b/xcm/xcm-builder/src/tests/origins.rs @@ -71,5 +71,6 @@ fn export_message_should_work() { let hash = fake_message_hash(&message); let r = XcmExecutor::::execute_xcm(Parachain(1), message, hash, 50); assert_eq!(r, Outcome::Complete(10)); - assert_eq!(exported_xcm(), vec![(Polkadot, 403611790, Here, expected_message, expected_hash)]); + let uni_src = (ByGenesis([0; 32]), Parachain(42), Parachain(1)).into(); + assert_eq!(exported_xcm(), vec![(Polkadot, 403611790, uni_src, Here, expected_message, expected_hash)]); } diff --git a/xcm/xcm-builder/src/universal_exports.rs b/xcm/xcm-builder/src/universal_exports.rs index 7a45af4686bf..7406675c2a76 100644 --- a/xcm/xcm-builder/src/universal_exports.rs +++ b/xcm/xcm-builder/src/universal_exports.rs @@ -26,12 +26,12 @@ use SendError::*; fn ensure_is_remote( universal_local: impl Into, dest: impl Into, -) -> Result<(NetworkId, InteriorMultiLocation, NetworkId, InteriorMultiLocation), MultiLocation> { +) -> Result<(NetworkId, InteriorMultiLocation), MultiLocation> { let dest = dest.into(); let universal_local = universal_local.into(); - let (local_net, local_loc) = match universal_local.clone().split_first() { - (location, Some(GlobalConsensus(network))) => (network, location), - _ => return Err(dest), + let local_net = match universal_local.global_consensus() { + Ok(x) => x, + Err(_) => return Err(dest), }; let universal_destination: InteriorMultiLocation = universal_local .into_location() @@ -42,15 +42,12 @@ fn ensure_is_remote( (d, Some(GlobalConsensus(n))) if n != local_net => (d, n), _ => return Err(dest), }; - Ok((remote_net, remote_dest, local_net, local_loc)) + Ok((remote_net, remote_dest)) } /// Implementation of `SendXcm` which uses the given `ExportXcm` implementation in order to forward /// the message over a bridge. /// -/// The actual message forwarded over the bridge is prepended with `UniversalOrigin` and -/// `DescendOrigin` in order to ensure that the message is executed with this Origin. -/// /// No effort is made to charge for any bridge fees, so this can only be used when it is known /// that the message sending cannot be abused in any way. /// @@ -68,22 +65,17 @@ impl> SendXcm xcm: &mut Option>, ) -> SendResult { let d = dest.take().ok_or(MissingArgument)?; - let devolved = match ensure_is_remote(UniversalLocation::get(), d) { + let universal_source = UniversalLocation::get(); + let devolved = match ensure_is_remote(universal_source, d) { Ok(x) => x, Err(d) => { *dest = Some(d); return Err(NotApplicable) }, }; - let (network, destination, local_network, local_location) = devolved; - - let inner = xcm.take().ok_or(MissingArgument)?; - let mut message: Xcm<()> = vec![UniversalOrigin(GlobalConsensus(local_network))].into(); - if local_location != Here { - message.inner_mut().push(DescendOrigin(local_location)); - } - message.inner_mut().extend(inner.into_iter()); - validate_export::(network, 0, destination, message) + let (network, destination) = devolved; + let xcm = xcm.take().ok_or(SendError::MissingArgument)?; + validate_export::(network, 0, universal_source, destination, xcm) } fn deliver(ticket: Exporter::Ticket) -> Result { @@ -137,9 +129,6 @@ impl)>>> ExporterFor /// Implementation of `SendXcm` which wraps the message inside an `ExportMessage` instruction /// and sends it to a destination known to be able to handle it. /// -/// The actual message send to the bridge for forwarding is prepended with `UniversalOrigin` -/// and `DescendOrigin` in order to ensure that the message is executed with our Origin. -/// /// No effort is made to make payment to the bridge for its services, so the bridge location /// must have been configured with a barrier rule allowing unpaid execution for this message /// coming from our origin. @@ -160,20 +149,10 @@ impl SendResult { let d = dest.as_ref().ok_or(MissingArgument)?.clone(); let devolved = ensure_is_remote(UniversalLocation::get(), d).map_err(|_| NotApplicable)?; - let (remote_network, remote_location, local_network, local_location) = devolved; - - // Prepend the desired message with instructions which effectively rewrite the origin. - // - // This only works because the remote chain empowers the bridge - // to speak for the local network. - let mut exported: Xcm<()> = vec![UniversalOrigin(GlobalConsensus(local_network))].into(); - if local_location != Here { - exported.inner_mut().push(DescendOrigin(local_location)); - } - exported.inner_mut().extend(xcm.take().ok_or(MissingArgument)?.into_iter()); - + let (remote_network, remote_location) = devolved; + let xcm = xcm.take().ok_or(MissingArgument)?; let (bridge, maybe_payment) = - Bridges::exporter_for(&remote_network, &remote_location, &exported) + Bridges::exporter_for(&remote_network, &remote_location, &xcm) .ok_or(NotApplicable)?; ensure!(maybe_payment.is_none(), Unroutable); @@ -183,7 +162,7 @@ impl(bridge, message)?; if let Some(payment) = maybe_payment { @@ -200,9 +179,6 @@ impl( @@ -219,7 +195,7 @@ impl SendResult { let d = dest.as_ref().ok_or(MissingArgument)?.clone(); let devolved = ensure_is_remote(UniversalLocation::get(), d).map_err(|_| NotApplicable)?; - let (remote_network, remote_location, local_network, local_location) = devolved; + let (remote_network, remote_location) = devolved; let xcm = xcm.take().ok_or(MissingArgument)?; let (bridge, maybe_payment) = @@ -328,6 +304,7 @@ impl, Price: Get> fn validate( network: NetworkId, _channel: u32, + universal_source: &mut Option, destination: &mut Option, message: &mut Option>, ) -> Result<((Vec, XcmHash), MultiAssets), SendError> { @@ -342,7 +319,17 @@ impl, Price: Get> return Err(SendError::NotApplicable) }, }; - let message = VersionedXcm::from(message.take().ok_or(SendError::MissingArgument)?); + let (local_net, local_sub) = universal_source + .take() + .ok_or(SendError::MissingArgument)? + .split_global() + .map_err(|()| SendError::Unroutable)?; + let mut inner: Xcm<()> = vec![UniversalOrigin(GlobalConsensus(local_net))].into(); + if local_sub != Here { + inner.inner_mut().push(DescendOrigin(local_sub)); + } + inner.inner_mut().extend(message.take().ok_or(SendError::MissingArgument)?.into_iter()); + let message = VersionedXcm::from(inner); let hash = message.using_encoded(sp_io::hashing::blake2_256); let blob = BridgeMessage { universal_dest, message }.encode(); Ok(((blob, hash), Price::get())) @@ -362,11 +349,11 @@ mod tests { fn ensure_is_remote_works() { // A Kusama parachain is remote from the Polkadot Relay. let x = ensure_is_remote(Polkadot, (Parent, Kusama, Parachain(1000))); - assert_eq!(x, Ok((Kusama, Parachain(1000).into(), Polkadot, Here))); + assert_eq!(x, Ok((Kusama, Parachain(1000).into()))); // Polkadot Relay is remote from a Kusama parachain. let x = ensure_is_remote((Kusama, Parachain(1000)), (Parent, Parent, Polkadot)); - assert_eq!(x, Ok((Polkadot, Here, Kusama, Parachain(1000).into()))); + assert_eq!(x, Ok((Polkadot, Here))); // Our own parachain is local. let x = ensure_is_remote(Polkadot, Parachain(1000)); diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 0750142bad4b..2b3c8c4a4c40 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -766,19 +766,16 @@ impl XcmExecutor { Ok(()) }, ExportMessage { network, destination, xcm } => { + // The actual message send to the bridge for forwarding is prepended with `UniversalOrigin` + // and `DescendOrigin` in order to ensure that the message is executed with this Origin. + // // Prepend the desired message with instructions which effectively rewrite the origin. // // This only works because the remote chain empowers the bridge // to speak for the local network. - let (local_net, local_sub) = - Config::UniversalLocation::get().split_global().map_err(|()| XcmError::Unanchored)?; - let mut exported: Xcm<()> = - vec![UniversalOrigin(GlobalConsensus(local_net))].into(); - if local_sub != Here { - exported.inner_mut().push(DescendOrigin(local_sub)); - } - exported.inner_mut().extend(xcm.into_iter()); - + let origin = self.context.origin.as_ref().ok_or(XcmError::BadOrigin)?.clone(); + let universal_source = Config::UniversalLocation::get().within_global(origin) + .map_err(|()| XcmError::Unanchored)?; let hash = (self.origin_ref(), &destination).using_encoded(blake2_128); let channel = u32::decode(&mut hash.as_ref()).unwrap_or(0); // Hash identifies the lane on the exporter which we use. We use the pairwise @@ -787,8 +784,9 @@ impl XcmExecutor { let (ticket, fee) = validate_export::( network, channel, + universal_source, destination, - exported, + xcm, )?; self.take_fee(fee, FeeReason::Export(network))?; Config::MessageExporter::deliver(ticket)?; diff --git a/xcm/xcm-executor/src/traits/export.rs b/xcm/xcm-executor/src/traits/export.rs index 4b4169ddc16d..4122f30f7676 100644 --- a/xcm/xcm-executor/src/traits/export.rs +++ b/xcm/xcm-executor/src/traits/export.rs @@ -19,9 +19,9 @@ use xcm::latest::prelude::*; pub trait ExportXcm { type Ticket; - /// Check whether the given `_message` is deliverable to the given `_destination` and if so - /// determine the cost which will be paid by this chain to do so, returning a `Validated` token - /// which can be used to enact delivery. + /// Check whether the given `message` is deliverable to the given `destination` spoofing + /// its source as `universal_source` and if so determine the cost which will be paid by this + /// chain to do so, returning a `Validated` token which can be used to enact delivery. /// /// The `destination` and `message` must be `Some` (or else an error will be returned) and they /// may only be consumed if the `Err` is not `NotApplicable`. @@ -32,6 +32,7 @@ pub trait ExportXcm { fn validate( network: NetworkId, channel: u32, + universal_source: &mut Option, destination: &mut Option, message: &mut Option>, ) -> SendResult; @@ -51,6 +52,7 @@ impl ExportXcm for Tuple { fn validate( network: NetworkId, channel: u32, + universal_source: &mut Option, destination: &mut Option, message: &mut Option>, ) -> SendResult { @@ -59,7 +61,7 @@ impl ExportXcm for Tuple { if maybe_cost.is_some() { None } else { - match Tuple::validate(network, channel, destination, message) { + match Tuple::validate(network, channel, universal_source, destination, message) { Err(SendError::NotApplicable) => None, Err(e) => { return Err(e) }, Ok((v, c)) => { @@ -91,10 +93,17 @@ impl ExportXcm for Tuple { pub fn validate_export( network: NetworkId, channel: u32, + universal_source: InteriorMultiLocation, dest: InteriorMultiLocation, msg: Xcm<()>, ) -> SendResult { - T::validate(network, channel, &mut Some(dest), &mut Some(msg)) + T::validate( + network, + channel, + &mut Some(universal_source), + &mut Some(dest), + &mut Some(msg), + ) } /// Convenience function for using a `SendXcm` implementation. Just interprets the `dest` and wraps @@ -108,10 +117,17 @@ pub fn validate_export( pub fn export_xcm( network: NetworkId, channel: u32, + universal_source: InteriorMultiLocation, dest: InteriorMultiLocation, msg: Xcm<()>, ) -> Result<(XcmHash, MultiAssets), SendError> { - let (ticket, price) = T::validate(network, channel, &mut Some(dest), &mut Some(msg.clone()))?; + let (ticket, price) = T::validate( + network, + channel, + &mut Some(universal_source), + &mut Some(dest), + &mut Some(msg.clone()), + )?; let hash = T::deliver(ticket)?; Ok((hash, price)) } From f223d250784a863915b0ee5f652e930f09abfac5 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Sat, 2 Apr 2022 20:07:26 +0200 Subject: [PATCH 06/15] Formatting --- xcm/xcm-builder/src/tests/mock.rs | 6 ++++-- xcm/xcm-builder/src/tests/origins.rs | 5 ++++- xcm/xcm-builder/src/universal_exports.rs | 14 ++++++-------- xcm/xcm-executor/src/lib.rs | 3 ++- xcm/xcm-executor/src/traits/export.rs | 8 +------- 5 files changed, 17 insertions(+), 19 deletions(-) diff --git a/xcm/xcm-builder/src/tests/mock.rs b/xcm/xcm-builder/src/tests/mock.rs index 079a6183c5d2..044186a62ddc 100644 --- a/xcm/xcm-builder/src/tests/mock.rs +++ b/xcm/xcm-builder/src/tests/mock.rs @@ -134,7 +134,8 @@ pub fn sent_xcm() -> Vec<(MultiLocation, opaque::Xcm, XcmHash)> { pub fn set_send_price(p: impl Into) { SEND_PRICE.with(|l| l.replace(p.into().into())); } -pub fn exported_xcm() -> Vec<(NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, opaque::Xcm, XcmHash)> { +pub fn exported_xcm( +) -> Vec<(NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, opaque::Xcm, XcmHash)> { EXPORTED_XCM.with(|q| (*q.borrow()).clone()) } pub fn set_exporter_override( @@ -186,7 +187,8 @@ impl ExportXcm for TestMessageExporter { uni_src: &mut Option, dest: &mut Option, msg: &mut Option>, - ) -> SendResult<(NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, Xcm<()>, XcmHash)> { + ) -> SendResult<(NetworkId, u32, InteriorMultiLocation, InteriorMultiLocation, Xcm<()>, XcmHash)> + { let (s, d, m) = (uni_src.take().unwrap(), dest.take().unwrap(), msg.take().unwrap()); let r: Result = EXPORTER_OVERRIDE.with(|e| { if let Some((ref f, _)) = &*e.borrow() { diff --git a/xcm/xcm-builder/src/tests/origins.rs b/xcm/xcm-builder/src/tests/origins.rs index d0dea4f49b40..ea7acb4f3e60 100644 --- a/xcm/xcm-builder/src/tests/origins.rs +++ b/xcm/xcm-builder/src/tests/origins.rs @@ -72,5 +72,8 @@ fn export_message_should_work() { let r = XcmExecutor::::execute_xcm(Parachain(1), message, hash, 50); assert_eq!(r, Outcome::Complete(10)); let uni_src = (ByGenesis([0; 32]), Parachain(42), Parachain(1)).into(); - assert_eq!(exported_xcm(), vec![(Polkadot, 403611790, uni_src, Here, expected_message, expected_hash)]); + assert_eq!( + exported_xcm(), + vec![(Polkadot, 403611790, uni_src, Here, expected_message, expected_hash)] + ); } diff --git a/xcm/xcm-builder/src/universal_exports.rs b/xcm/xcm-builder/src/universal_exports.rs index 7406675c2a76..f16b683a12c7 100644 --- a/xcm/xcm-builder/src/universal_exports.rs +++ b/xcm/xcm-builder/src/universal_exports.rs @@ -152,18 +152,14 @@ impl(bridge, message)?; if let Some(payment) = maybe_payment { cost.push(payment); @@ -328,7 +324,9 @@ impl, Price: Get> if local_sub != Here { inner.inner_mut().push(DescendOrigin(local_sub)); } - inner.inner_mut().extend(message.take().ok_or(SendError::MissingArgument)?.into_iter()); + inner + .inner_mut() + .extend(message.take().ok_or(SendError::MissingArgument)?.into_iter()); let message = VersionedXcm::from(inner); let hash = message.using_encoded(sp_io::hashing::blake2_256); let blob = BridgeMessage { universal_dest, message }.encode(); diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 2b3c8c4a4c40..ba422bce99f1 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -774,7 +774,8 @@ impl XcmExecutor { // This only works because the remote chain empowers the bridge // to speak for the local network. let origin = self.context.origin.as_ref().ok_or(XcmError::BadOrigin)?.clone(); - let universal_source = Config::UniversalLocation::get().within_global(origin) + let universal_source = Config::UniversalLocation::get() + .within_global(origin) .map_err(|()| XcmError::Unanchored)?; let hash = (self.origin_ref(), &destination).using_encoded(blake2_128); let channel = u32::decode(&mut hash.as_ref()).unwrap_or(0); diff --git a/xcm/xcm-executor/src/traits/export.rs b/xcm/xcm-executor/src/traits/export.rs index 4122f30f7676..6a846fb0b75a 100644 --- a/xcm/xcm-executor/src/traits/export.rs +++ b/xcm/xcm-executor/src/traits/export.rs @@ -97,13 +97,7 @@ pub fn validate_export( dest: InteriorMultiLocation, msg: Xcm<()>, ) -> SendResult { - T::validate( - network, - channel, - &mut Some(universal_source), - &mut Some(dest), - &mut Some(msg), - ) + T::validate(network, channel, &mut Some(universal_source), &mut Some(dest), &mut Some(msg)) } /// Convenience function for using a `SendXcm` implementation. Just interprets the `dest` and wraps From 52090798e3856d7478eba7c2462be6c204ab1d0d Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Thu, 5 May 2022 20:33:04 +0100 Subject: [PATCH 07/15] Update xcm/src/v3/junctions.rs --- xcm/src/v3/junctions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xcm/src/v3/junctions.rs b/xcm/src/v3/junctions.rs index 842f60eff04d..9bf7905a86a3 100644 --- a/xcm/src/v3/junctions.rs +++ b/xcm/src/v3/junctions.rs @@ -227,7 +227,7 @@ impl Junctions { /// This will return an `Err` if the first item is not a `GlobalConsensus`, which would indicate /// that this value is not a universal location. pub fn split_global(self) -> Result<(NetworkId, Junctions), ()> { - match self.clone().split_first() { + match self.split_first() { (location, Some(Junction::GlobalConsensus(network))) => Ok((network, location)), _ => return Err(()), } From a79da58cdfdc3eec9397b1eb0aac30ff83d23582 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 26 May 2022 17:17:58 +0100 Subject: [PATCH 08/15] UnpaidExecution instruction and associated barrier. --- xcm/src/v3/mod.rs | 16 +++++++++ xcm/xcm-builder/src/barriers.rs | 33 ++++++++++++++++-- xcm/xcm-builder/src/lib.rs | 3 +- xcm/xcm-builder/src/tests/barriers.rs | 50 +++++++++++++++++++++++++++ xcm/xcm-builder/src/tests/mock.rs | 6 ++-- xcm/xcm-builder/src/tests/origins.rs | 24 +++++++++++++ xcm/xcm-executor/src/lib.rs | 4 +++ 7 files changed, 130 insertions(+), 6 deletions(-) diff --git a/xcm/src/v3/mod.rs b/xcm/src/v3/mod.rs index cd4060566aac..6231a647086d 100644 --- a/xcm/src/v3/mod.rs +++ b/xcm/src/v3/mod.rs @@ -986,6 +986,18 @@ pub enum Instruction { /// /// Errors: If the existing state would not allow such a change. AliasOrigin(MultiLocation), + + /// A directive to indicate that the origin expects free execution of the message. + /// + /// At execution time, this instruction just does a check on the Origin register. + /// However, at the barrier stage messages starting with this instruction can be disregarded if + /// the origin is not acceptable for free execution or the `weight_limit` is `Limited` and + /// insufficient. + /// + /// Kind: *Indication* + /// + /// Errors: If the given origin is `Some` and not equal to the current Origin register. + UnpaidExecution { weight_limit: WeightLimit, check_origin: Option }, } impl Xcm { @@ -1060,6 +1072,8 @@ impl Instruction { SetTopic(topic) => SetTopic(topic), ClearTopic => ClearTopic, AliasOrigin(location) => AliasOrigin(location), + UnpaidExecution { weight_limit, check_origin } => + UnpaidExecution { weight_limit, check_origin }, } } } @@ -1126,6 +1140,8 @@ impl> GetWeight for Instruction { SetTopic(topic) => W::set_topic(topic), ClearTopic => W::clear_topic(), AliasOrigin(location) => W::alias_origin(location), + UnpaidExecution { weight_limit, check_origin } => + W::unpaid_execution(weight_limit, check_origin), } } } diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index 8a29a3246617..1bce49f16dc6 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -197,9 +197,10 @@ impl< } } -/// Allows execution from any origin that is contained in `T` (i.e. `T::Contains(origin)`) without -/// any payments. -/// Use only for executions from trusted origin groups. +/// Allows execution from any origin that is contained in `T` (i.e. `T::Contains(origin)`). +/// +/// Use only for executions from completely trusted origins, from which no unpermissioned messages +/// can be sent. pub struct AllowUnpaidExecutionFrom(PhantomData); impl> ShouldExecute for AllowUnpaidExecutionFrom { fn should_execute( @@ -218,6 +219,32 @@ impl> ShouldExecute for AllowUnpaidExecutionFrom { } } +/// Allows execution from any origin that is contained in `T` (i.e. `T::Contains(origin)`) if the +/// message begins with the instruction `UnpaidExecution`. +/// +/// Use only for executions from trusted origin groups. +pub struct AllowExplicitUnpaidExecutionFrom(PhantomData); +impl> ShouldExecute for AllowExplicitUnpaidExecutionFrom { + fn should_execute( + origin: &MultiLocation, + instructions: &mut [Instruction], + max_weight: Weight, + _weight_credit: &mut Weight, + ) -> Result<(), ()> { + log::trace!( + target: "xcm::barriers", + "AllowUnpaidExecutionFrom origin: {:?}, instructions: {:?}, max_weight: {:?}, weight_credit: {:?}", + origin, instructions, max_weight, _weight_credit, + ); + ensure!(T::contains(origin), ()); + match instructions.first() { + Some(UnpaidExecution { weight_limit: Limited(m), .. }) if *m >= max_weight => Ok(()), + Some(UnpaidExecution { weight_limit: Unlimited, .. }) => Ok(()), + _ => Err(()), + } + } +} + /// Allows a message only if it is from a system-level child parachain. pub struct IsChildSystemParachain(PhantomData); impl> Contains for IsChildSystemParachain { diff --git a/xcm/xcm-builder/src/lib.rs b/xcm/xcm-builder/src/lib.rs index eb160fa12ecd..d17e14542324 100644 --- a/xcm/xcm-builder/src/lib.rs +++ b/xcm/xcm-builder/src/lib.rs @@ -48,7 +48,8 @@ pub use asset_conversion::{ConvertedAbstractAssetId, ConvertedConcreteAssetId}; mod barriers; pub use barriers::{ AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, IsChildSystemParachain, TakeWeightCredit, WithComputedOrigin, + AllowUnpaidExecutionFrom, AllowExplicitUnpaidExecutionFrom, IsChildSystemParachain, + TakeWeightCredit, WithComputedOrigin, }; mod currency_adapter; diff --git a/xcm/xcm-builder/src/tests/barriers.rs b/xcm/xcm-builder/src/tests/barriers.rs index b3af3b6c4d1d..b3cfaa48f0f5 100644 --- a/xcm/xcm-builder/src/tests/barriers.rs +++ b/xcm/xcm-builder/src/tests/barriers.rs @@ -107,6 +107,56 @@ fn allow_unpaid_should_work() { assert_eq!(r, Ok(())); } +#[test] +fn allow_explicit_unpaid_should_work() { + let mut bad_message1 = + Xcm::<()>(vec![TransferAsset { assets: (Parent, 100).into(), beneficiary: Here.into() }]); + + let mut bad_message2 = Xcm::<()>(vec![ + UnpaidExecution { weight_limit: Limited(10), check_origin: Some(Parent.into()) }, + TransferAsset { assets: (Parent, 100).into(), beneficiary: Here.into() }, + ]); + + let mut good_message = Xcm::<()>(vec![ + UnpaidExecution { weight_limit: Limited(20), check_origin: Some(Parent.into()) }, + TransferAsset { assets: (Parent, 100).into(), beneficiary: Here.into() }, + ]); + + AllowExplicitUnpaidFrom::set(vec![Parent.into()]); + + let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( + &Parachain(1).into(), + good_message.inner_mut(), + 20, + &mut 0, + ); + assert_eq!(r, Err(())); + + let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( + &Parent.into(), + bad_message1.inner_mut(), + 20, + &mut 0, + ); + assert_eq!(r, Err(())); + + let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( + &Parent.into(), + bad_message2.inner_mut(), + 20, + &mut 0, + ); + assert_eq!(r, Err(())); + + let r = AllowExplicitUnpaidExecutionFrom::>::should_execute( + &Parent.into(), + good_message.inner_mut(), + 20, + &mut 0, + ); + assert_eq!(r, Ok(())); +} + #[test] fn allow_paid_should_work() { AllowPaidFrom::set(vec![Parent.into()]); diff --git a/xcm/xcm-builder/src/tests/mock.rs b/xcm/xcm-builder/src/tests/mock.rs index 044186a62ddc..0cebe906ba8d 100644 --- a/xcm/xcm-builder/src/tests/mock.rs +++ b/xcm/xcm-builder/src/tests/mock.rs @@ -16,8 +16,8 @@ use crate::{barriers::AllowSubscriptionsFrom, test_utils::*}; pub use crate::{ - AllowKnownQueryResponses, AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, - FixedRateOfFungible, FixedWeightBounds, TakeWeightCredit, + AllowKnownQueryResponses, AllowTopLevelPaidExecutionFrom, AllowExplicitUnpaidExecutionFrom, + AllowUnpaidExecutionFrom, FixedRateOfFungible, FixedWeightBounds, TakeWeightCredit, }; use frame_support::traits::ContainsPair; pub use frame_support::{ @@ -407,6 +407,7 @@ parameter_types! { } parameter_types! { // Nothing is allowed to be paid/unpaid by default. + pub static AllowExplicitUnpaidFrom: Vec = vec![]; pub static AllowUnpaidFrom: Vec = vec![]; pub static AllowPaidFrom: Vec = vec![]; pub static AllowSubsFrom: Vec = vec![]; @@ -419,6 +420,7 @@ pub type TestBarrier = ( TakeWeightCredit, AllowKnownQueryResponses, AllowTopLevelPaidExecutionFrom>, + AllowExplicitUnpaidExecutionFrom>, AllowUnpaidExecutionFrom>, AllowSubscriptionsFrom>, ); diff --git a/xcm/xcm-builder/src/tests/origins.rs b/xcm/xcm-builder/src/tests/origins.rs index ea7acb4f3e60..aef55a54603e 100644 --- a/xcm/xcm-builder/src/tests/origins.rs +++ b/xcm/xcm-builder/src/tests/origins.rs @@ -77,3 +77,27 @@ fn export_message_should_work() { vec![(Polkadot, 403611790, uni_src, Here, expected_message, expected_hash)] ); } + +#[test] +fn unpaid_execution_should_work() { + // Bridge chain (assumed to be Relay) lets Parachain #1 have message execution for free. + AllowUnpaidFrom::set(vec![X1(Parachain(1)).into()]); + // Bridge chain (assumed to be Relay) lets Parachain #2 have message execution for free if it + // asks. + AllowExplicitUnpaidFrom::set(vec![X1(Parachain(2)).into()]); + // Asking for unpaid execution of up to 9 weight on the assumption it is origin of #2. + let message = Xcm(vec![ + UnpaidExecution { weight_limit: Limited(9), check_origin: Some(Parachain(2).into()) }, + ]); + let hash = fake_message_hash(&message); + let r = XcmExecutor::::execute_xcm(Parachain(1), message.clone(), hash, 50); + assert_eq!(r, Outcome::Incomplete(10, XcmError::BadOrigin)); + let r = XcmExecutor::::execute_xcm(Parachain(2), message.clone(), hash, 50); + assert_eq!(r, Outcome::Error(XcmError::Barrier)); + + let message = Xcm(vec![ + UnpaidExecution { weight_limit: Limited(10), check_origin: Some(Parachain(2).into()) }, + ]); + let r = XcmExecutor::::execute_xcm(Parachain(2), message.clone(), hash, 50); + assert_eq!(r, Outcome::Complete(10)); +} diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index ba422bce99f1..e753fb84c211 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -861,6 +861,10 @@ impl XcmExecutor { Ok(()) }, AliasOrigin(_) => Err(XcmError::NoPermission), + UnpaidExecution { check_origin, .. } => { + ensure!(check_origin.is_none() || self.context.origin == check_origin, XcmError::BadOrigin); + Ok(()) + }, HrmpNewChannelOpenRequest { .. } => Err(XcmError::Unimplemented), HrmpChannelAccepted { .. } => Err(XcmError::Unimplemented), HrmpChannelClosing { .. } => Err(XcmError::Unimplemented), From bd19b1fbd8287d22d8d2c987f61469219e79a3e2 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 26 May 2022 17:23:17 +0100 Subject: [PATCH 09/15] Tighten barriers (ClearOrigin/QueryResponse) --- xcm/xcm-builder/src/barriers.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index 1bce49f16dc6..9542644803d7 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -75,7 +75,10 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFro ); ensure!(T::contains(origin), ()); - let mut iter = instructions.iter_mut(); + // We will read up to 5 instructions. This allows up to 3 `ClearOrigin`s instructions. We + // allow for more than one since anything beyond the first is a no-op and it's conceivable + // that composition of operations might result in more than one being appended. + let mut iter = instructions.iter_mut().take(5); let i = iter.next().ok_or(())?; match i { ReceiveTeleportedAsset(..) | @@ -271,6 +274,7 @@ impl ShouldExecute for AllowKnownQueryResponses From 44e374db845925e48f6e8dc159ce4f0a51dc2b4c Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 27 May 2022 12:42:00 +0100 Subject: [PATCH 10/15] Allow only 1 ClearOrigin instruction in AllowTopLevelPaidExecutionFrom --- xcm/xcm-builder/src/barriers.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index 8a29a3246617..6727e511e044 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -84,11 +84,11 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFro ClaimAsset { .. } => (), _ => return Err(()), } - let mut i = iter.next().ok_or(())?; - while let ClearOrigin = i { - i = iter.next().ok_or(())?; + let i = iter.next().ok_or(())?; + if !matches!(i, ClearOrigin) { + return Err(()) } - match i { + match iter.next().ok_or(())? { BuyExecution { weight_limit: Limited(ref mut weight), .. } if *weight >= max_weight => { *weight = max_weight; Ok(()) From f2259fef107437873a9cab3d7863cf5cd5ee48de Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 May 2022 12:45:57 +0100 Subject: [PATCH 11/15] Bi-directional teleport accounting --- xcm/src/v3/traits.rs | 3 + xcm/xcm-builder/src/currency_adapter.rs | 100 ++++++++--- xcm/xcm-builder/src/fungibles_adapter.rs | 162 +++++++++++++++--- xcm/xcm-builder/src/lib.rs | 2 +- xcm/xcm-builder/src/nonfungibles_adapter.rs | 115 ++++++++++--- xcm/xcm-builder/tests/mock/mod.rs | 4 +- xcm/xcm-executor/src/lib.rs | 7 + xcm/xcm-executor/src/traits/transact_asset.rs | 54 +++++- 8 files changed, 375 insertions(+), 72 deletions(-) diff --git a/xcm/src/v3/traits.rs b/xcm/src/v3/traits.rs index 31d0e62d837b..028d8365c1e3 100644 --- a/xcm/src/v3/traits.rs +++ b/xcm/src/v3/traits.rs @@ -129,6 +129,9 @@ pub enum Error { /// The universal location of the local consensus is improper. #[codec(index = 33)] Unanchored, + /// An asset cannot be deposited, probably because (too much of) it already exists. + #[codec(index = 34)] + NotDepositable, // Errors that happen prior to instructions being executed. These fall outside of the XCM spec. /// XCM version not able to be handled. diff --git a/xcm/xcm-builder/src/currency_adapter.rs b/xcm/xcm-builder/src/currency_adapter.rs index d85f371bd346..71d05a4ba751 100644 --- a/xcm/xcm-builder/src/currency_adapter.rs +++ b/xcm/xcm-builder/src/currency_adapter.rs @@ -24,6 +24,7 @@ use xcm_executor::{ traits::{Convert, MatchesFungible, TransactAsset}, Assets, }; +use super::MintLocation; /// Asset transaction errors. enum Error { @@ -97,7 +98,48 @@ impl< Matcher: MatchesFungible, AccountIdConverter: Convert, AccountId: Clone, // can't get away without it since Currency is generic over it. - CheckedAccount: Get>, + CheckedAccount: Get>, + > CurrencyAdapter +{ + fn can_accrue_checked(_checked_account: AccountId, _amount: Currency::Balance) -> Result { + Ok(()) + } + fn can_reduce_checked(checked_account: AccountId, amount: Currency::Balance) -> Result { + let new_balance = Currency::free_balance(&checked_account) + .checked_sub(&amount) + .ok_or(XcmError::NotWithdrawable)?; + Currency::ensure_can_withdraw( + &checked_account, + amount, + WithdrawReasons::TRANSFER, + new_balance, + ) + .map_err(|_| XcmError::NotWithdrawable) + } + fn accrue_checked(checked_account: AccountId, amount: Currency::Balance) { + Currency::deposit_creating(&checked_account, amount); + } + fn reduce_checked(checked_account: AccountId, amount: Currency::Balance) { + let ok = Currency::withdraw( + &checked_account, + amount, + WithdrawReasons::TRANSFER, + AllowDeath, + ) + .is_ok(); + debug_assert!( + ok, + "`can_check_in` must have returned `true` immediately prior; qed" + ); +} +} + +impl< + Currency: frame_support::traits::Currency, + Matcher: MatchesFungible, + AccountIdConverter: Convert, + AccountId: Clone, // can't get away without it since Currency is generic over it. + CheckedAccount: Get>, > TransactAsset for CurrencyAdapter { @@ -106,45 +148,49 @@ impl< // Check we handle this asset. let amount: Currency::Balance = Matcher::matches_fungible(what).ok_or(Error::AssetNotFound)?; - if let Some(checked_account) = CheckedAccount::get() { - let new_balance = Currency::free_balance(&checked_account) - .checked_sub(&amount) - .ok_or(XcmError::NotWithdrawable)?; - Currency::ensure_can_withdraw( - &checked_account, - amount, - WithdrawReasons::TRANSFER, - new_balance, - ) - .map_err(|_| XcmError::NotWithdrawable)?; + match CheckedAccount::get() { + Some((checked_account, MintLocation::Local)) => + Self::can_reduce_checked(checked_account, amount), + Some((checked_account, MintLocation::NonLocal)) => + Self::can_accrue_checked(checked_account, amount), + None => Ok(()), } - Ok(()) } fn check_in(_origin: &MultiLocation, what: &MultiAsset, _context: &XcmContext) { log::trace!(target: "xcm::currency_adapter", "check_in origin: {:?}, what: {:?}", _origin, what); if let Some(amount) = Matcher::matches_fungible(what) { - if let Some(checked_account) = CheckedAccount::get() { - let ok = Currency::withdraw( - &checked_account, - amount, - WithdrawReasons::TRANSFER, - AllowDeath, - ) - .is_ok(); - debug_assert!( - ok, - "`can_check_in` must have returned `true` immediately prior; qed" - ); + match CheckedAccount::get() { + Some((checked_account, MintLocation::Local)) => + Self::reduce_checked(checked_account, amount), + Some((checked_account, MintLocation::NonLocal)) => + Self::accrue_checked(checked_account, amount), + None => (), } } } + fn can_check_out(_dest: &MultiLocation, what: &MultiAsset, _context: &XcmContext) -> Result { + log::trace!(target: "xcm::currency_adapter", "check_out dest: {:?}, what: {:?}", _dest, what); + let amount = Matcher::matches_fungible(what).ok_or(Error::AssetNotFound)?; + match CheckedAccount::get() { + Some((checked_account, MintLocation::Local)) => + Self::can_accrue_checked(checked_account, amount), + Some((checked_account, MintLocation::NonLocal)) => + Self::can_reduce_checked(checked_account, amount), + None => Ok(()), + } + } + fn check_out(_dest: &MultiLocation, what: &MultiAsset, _context: &XcmContext) { log::trace!(target: "xcm::currency_adapter", "check_out dest: {:?}, what: {:?}", _dest, what); if let Some(amount) = Matcher::matches_fungible(what) { - if let Some(checked_account) = CheckedAccount::get() { - Currency::deposit_creating(&checked_account, amount); + match CheckedAccount::get() { + Some((checked_account, MintLocation::Local)) => + Self::accrue_checked(checked_account, amount), + Some((checked_account, MintLocation::NonLocal)) => + Self::reduce_checked(checked_account, amount), + None => (), } } } diff --git a/xcm/xcm-builder/src/fungibles_adapter.rs b/xcm/xcm-builder/src/fungibles_adapter.rs index be8e9d27cfaa..a6e5f95dd854 100644 --- a/xcm/xcm-builder/src/fungibles_adapter.rs +++ b/xcm/xcm-builder/src/fungibles_adapter.rs @@ -54,6 +54,54 @@ impl< } } +/// The location which is allowed to mint a particular asset. +#[derive(Copy, Clone, Eq, PartialEq)] +pub enum MintLocation { + /// This chain is allowed to mint the asset. When we track teleports of the asset we ensure that + /// no more of the asset returns back to the chain than has been sent out. + Local, + /// This chain is not allowed to mint the asset. When we track teleports of the asset we ensure + /// that no more of the asset is sent out from the chain than has been previously received. + NonLocal, +} + +pub trait AssetChecking { + fn asset_checking(asset: &AssetId) -> Option; +} + +pub struct LocalMint(sp_std::marker::PhantomData); +impl> AssetChecking for LocalMint { + fn asset_checking(asset: &AssetId) -> Option { + match T::contains(asset) { + true => Some(MintLocation::Local), + false => None, + } + } +} + +pub struct NonLocalMint(sp_std::marker::PhantomData); +impl> AssetChecking for NonLocalMint { + fn asset_checking(asset: &AssetId) -> Option { + match T::contains(asset) { + true => Some(MintLocation::NonLocal), + false => None, + } + } +} + +pub struct DualMint(sp_std::marker::PhantomData<(L, R)>); +impl, R: Contains> AssetChecking for DualMint { + fn asset_checking(asset: &AssetId) -> Option { + if L::contains(asset) { + Some(MintLocation::Local) + } else if R::contains(asset) { + Some(MintLocation::NonLocal) + } else { + None + } + } +} + pub struct FungiblesMutateAdapter< Assets, Matcher, @@ -62,12 +110,54 @@ pub struct FungiblesMutateAdapter< CheckAsset, CheckingAccount, >(PhantomData<(Assets, Matcher, AccountIdConverter, AccountId, CheckAsset, CheckingAccount)>); + +impl< + Assets: fungibles::Mutate, + Matcher: MatchesFungibles, + AccountIdConverter: Convert, + AccountId: Clone, // can't get away without it since Currency is generic over it. + CheckAsset: AssetChecking, + CheckingAccount: Get, + > FungiblesMutateAdapter< + Assets, + Matcher, + AccountIdConverter, + AccountId, + CheckAsset, + CheckingAccount, + > +{ + fn can_accrue_checked(asset_id: Assets::AssetId, amount: Assets::Balance) -> XcmResult { + let checking_account = CheckingAccount::get(); + Assets::can_deposit(asset_id, &checking_account, amount) + .into_result() + .map_err(|_| XcmError::NotDepositable) + } + fn can_reduce_checked(asset_id: Assets::AssetId, amount: Assets::Balance) -> XcmResult { + let checking_account = CheckingAccount::get(); + Assets::can_withdraw(asset_id, &checking_account, amount) + .into_result() + .map_err(|_| XcmError::NotWithdrawable) + .map(|_| ()) + } + fn accrue_checked(asset_id: Assets::AssetId, amount: Assets::Balance) { + let checking_account = CheckingAccount::get(); + let ok = Assets::mint_into(asset_id, &checking_account, amount).is_ok(); + debug_assert!(ok, "`can_accrue_checked` must have returned `true` immediately prior; qed"); + } + fn reduce_checked(asset_id: Assets::AssetId, amount: Assets::Balance) { + let checking_account = CheckingAccount::get(); + let ok = Assets::burn_from(asset_id, &checking_account, amount).is_ok(); + debug_assert!(ok, "`can_reduce_checked` must have returned `true` immediately prior; qed"); + } +} + impl< Assets: fungibles::Mutate, Matcher: MatchesFungibles, AccountIdConverter: Convert, AccountId: Clone, // can't get away without it since Currency is generic over it. - CheckAsset: Contains, + CheckAsset: AssetChecking, CheckingAccount: Get, > TransactAsset for FungiblesMutateAdapter< @@ -91,14 +181,13 @@ impl< ); // Check we handle this asset. let (asset_id, amount) = Matcher::matches_fungibles(what)?; - if CheckAsset::contains(&asset_id) { - // This is an asset whose teleports we track. - let checking_account = CheckingAccount::get(); - Assets::can_withdraw(asset_id, &checking_account, amount) - .into_result() - .map_err(|_| XcmError::NotWithdrawable)?; + match CheckAsset::asset_checking(&asset_id) { + // We track this asset's teleports to ensure no more come in than have gone out. + Some(MintLocation::Local) => Self::can_reduce_checked(asset_id, amount), + // We track this asset's teleports to ensure no more go out than have come in. + Some(MintLocation::NonLocal) => Self::can_accrue_checked(asset_id, amount), + _ => Ok(()), } - Ok(()) } fn check_in(_origin: &MultiLocation, what: &MultiAsset, _context: &XcmContext) { @@ -108,17 +197,37 @@ impl< _origin, what ); if let Ok((asset_id, amount)) = Matcher::matches_fungibles(what) { - if CheckAsset::contains(&asset_id) { - let checking_account = CheckingAccount::get(); - let ok = Assets::burn_from(asset_id, &checking_account, amount).is_ok(); - debug_assert!( - ok, - "`can_check_in` must have returned `true` immediately prior; qed" - ); + match CheckAsset::asset_checking(&asset_id) { + // We track this asset's teleports to ensure no more come in than have gone out. + Some(MintLocation::Local) => Self::reduce_checked(asset_id, amount), + // We track this asset's teleports to ensure no more go out than have come in. + Some(MintLocation::NonLocal) => Self::accrue_checked(asset_id, amount), + _ => (), } } } + fn can_check_out( + _origin: &MultiLocation, + what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + log::trace!( + target: "xcm::fungibles_adapter", + "can_check_in origin: {:?}, what: {:?}", + _origin, what + ); + // Check we handle this asset. + let (asset_id, amount) = Matcher::matches_fungibles(what)?; + match CheckAsset::asset_checking(&asset_id) { + // We track this asset's teleports to ensure no more come in than have gone out. + Some(MintLocation::Local) => Self::can_accrue_checked(asset_id, amount), + // We track this asset's teleports to ensure no more go out than have come in. + Some(MintLocation::NonLocal) => Self::can_reduce_checked(asset_id, amount), + _ => Ok(()), + } + } + fn check_out(_dest: &MultiLocation, what: &MultiAsset, _context: &XcmContext) { log::trace!( target: "xcm::fungibles_adapter", @@ -126,10 +235,12 @@ impl< _dest, what ); if let Ok((asset_id, amount)) = Matcher::matches_fungibles(what) { - if CheckAsset::contains(&asset_id) { - let checking_account = CheckingAccount::get(); - let ok = Assets::mint_into(asset_id, &checking_account, amount).is_ok(); - debug_assert!(ok, "`mint_into` cannot generally fail; qed"); + match CheckAsset::asset_checking(&asset_id) { + // We track this asset's teleports to ensure no more come in than have gone out. + Some(MintLocation::Local) => Self::accrue_checked(asset_id, amount), + // We track this asset's teleports to ensure no more go out than have come in. + Some(MintLocation::NonLocal) => Self::reduce_checked(asset_id, amount), + _ => (), } } } @@ -181,7 +292,7 @@ impl< Matcher: MatchesFungibles, AccountIdConverter: Convert, AccountId: Clone, // can't get away without it since Currency is generic over it. - CheckAsset: Contains, + CheckAsset: AssetChecking, CheckingAccount: Get, > TransactAsset for FungiblesAdapter @@ -208,6 +319,17 @@ impl< >::check_in(origin, what, context) } + fn can_check_out(dest: &MultiLocation, what: &MultiAsset, context: &XcmContext) -> XcmResult { + FungiblesMutateAdapter::< + Assets, + Matcher, + AccountIdConverter, + AccountId, + CheckAsset, + CheckingAccount, + >::can_check_out(dest, what, context) + } + fn check_out(dest: &MultiLocation, what: &MultiAsset, context: &XcmContext) { FungiblesMutateAdapter::< Assets, diff --git a/xcm/xcm-builder/src/lib.rs b/xcm/xcm-builder/src/lib.rs index d17e14542324..4b2ad651dc1e 100644 --- a/xcm/xcm-builder/src/lib.rs +++ b/xcm/xcm-builder/src/lib.rs @@ -56,7 +56,7 @@ mod currency_adapter; pub use currency_adapter::CurrencyAdapter; mod fungibles_adapter; -pub use fungibles_adapter::{FungiblesAdapter, FungiblesMutateAdapter, FungiblesTransferAdapter}; +pub use fungibles_adapter::{FungiblesAdapter, FungiblesMutateAdapter, FungiblesTransferAdapter, MintLocation, LocalMint, NonLocalMint, DualMint, AssetChecking}; mod nonfungibles_adapter; pub use nonfungibles_adapter::{ diff --git a/xcm/xcm-builder/src/nonfungibles_adapter.rs b/xcm/xcm-builder/src/nonfungibles_adapter.rs index d717145cc4fc..c1a2ab119491 100644 --- a/xcm/xcm-builder/src/nonfungibles_adapter.rs +++ b/xcm/xcm-builder/src/nonfungibles_adapter.rs @@ -23,6 +23,7 @@ use frame_support::{ use sp_std::{marker::PhantomData, prelude::*, result}; use xcm::latest::prelude::*; use xcm_executor::traits::{Convert, Error as MatchError, MatchesNonFungibles, TransactAsset}; +use crate::{AssetChecking, MintLocation}; pub struct NonFungiblesTransferAdapter( PhantomData<(Assets, Matcher, AccountIdConverter, AccountId)>, @@ -63,12 +64,57 @@ pub struct NonFungiblesMutateAdapter< CheckAsset, CheckingAccount, >(PhantomData<(Assets, Matcher, AccountIdConverter, AccountId, CheckAsset, CheckingAccount)>); + +impl< + Assets: nonfungibles::Mutate, + Matcher: MatchesNonFungibles, + AccountIdConverter: Convert, + AccountId: Clone + Eq, // can't get away without it since Currency is generic over it. + CheckAsset: AssetChecking, + CheckingAccount: Get>, + > NonFungiblesMutateAdapter< + Assets, + Matcher, + AccountIdConverter, + AccountId, + CheckAsset, + CheckingAccount, + > +{ + fn can_accrue_checked(class: Assets::ClassId, instance: Assets::InstanceId) -> XcmResult { + ensure!(Assets::owner(&class, &instance).is_none(), XcmError::NotDepositable); + Ok(()) + } + fn can_reduce_checked(class: Assets::ClassId, instance: Assets::InstanceId) -> XcmResult { + if let Some(checking_account) = CheckingAccount::get() { + // This is an asset whose teleports we track. + let owner = Assets::owner(&class, &instance); + ensure!(owner == Some(checking_account), XcmError::NotWithdrawable); + ensure!(Assets::can_transfer(&class, &instance), XcmError::NotWithdrawable); + } + Ok(()) + } + fn accrue_checked(class: Assets::ClassId, instance: Assets::InstanceId) { + if let Some(checking_account) = CheckingAccount::get() { + let ok = Assets::mint_into(&class, &instance, &checking_account).is_ok(); + debug_assert!(ok, "`mint_into` cannot generally fail; qed"); + } +} + fn reduce_checked(class: Assets::ClassId, instance: Assets::InstanceId) { + let ok = Assets::burn(&class, &instance, None).is_ok(); + debug_assert!( + ok, + "`can_check_in` must have returned `true` immediately prior; qed" + ); + } +} + impl< Assets: nonfungibles::Mutate, Matcher: MatchesNonFungibles, AccountIdConverter: Convert, AccountId: Clone + Eq, // can't get away without it since Currency is generic over it. - CheckAsset: Contains, + CheckAsset: AssetChecking, CheckingAccount: Get>, > TransactAsset for NonFungiblesMutateAdapter< @@ -88,15 +134,13 @@ impl< ); // Check we handle this asset. let (class, instance) = Matcher::matches_nonfungibles(what)?; - if CheckAsset::contains(&class) { - if let Some(checking_account) = CheckingAccount::get() { - // This is an asset whose teleports we track. - let owner = Assets::owner(&class, &instance); - ensure!(owner == Some(checking_account), XcmError::NotWithdrawable); - ensure!(Assets::can_transfer(&class, &instance), XcmError::NotWithdrawable); - } + match CheckAsset::asset_checking(&class) { + // We track this asset's teleports to ensure no more come in than have gone out. + Some(MintLocation::Local) => Self::can_reduce_checked(class, instance), + // We track this asset's teleports to ensure no more go out than have come in. + Some(MintLocation::NonLocal) => Self::can_accrue_checked(class, instance), + _ => Ok(()), } - Ok(()) } fn check_in(_origin: &MultiLocation, what: &MultiAsset, context: &XcmContext) { @@ -106,16 +150,33 @@ impl< _origin, what, context, ); if let Ok((class, instance)) = Matcher::matches_nonfungibles(what) { - if CheckAsset::contains(&class) { - let ok = Assets::burn(&class, &instance, None).is_ok(); - debug_assert!( - ok, - "`can_check_in` must have returned `true` immediately prior; qed" - ); + match CheckAsset::asset_checking(&class) { + // We track this asset's teleports to ensure no more come in than have gone out. + Some(MintLocation::Local) => Self::reduce_checked(class, instance), + // We track this asset's teleports to ensure no more go out than have come in. + Some(MintLocation::NonLocal) => Self::accrue_checked(class, instance), + _ => (), } } } + fn can_check_out(_dest: &MultiLocation, what: &MultiAsset, context: &XcmContext) -> XcmResult { + log::trace!( + target: "xcm::fungibles_adapter", + "can_check_out dest: {:?}, what: {:?}, context: {:?}", + _dest, what, context, + ); + // Check we handle this asset. + let (class, instance) = Matcher::matches_nonfungibles(what)?; + match CheckAsset::asset_checking(&class) { + // We track this asset's teleports to ensure no more come in than have gone out. + Some(MintLocation::Local) => Self::can_accrue_checked(class, instance), + // We track this asset's teleports to ensure no more go out than have come in. + Some(MintLocation::NonLocal) => Self::can_reduce_checked(class, instance), + _ => Ok(()), + } + } + fn check_out(_dest: &MultiLocation, what: &MultiAsset, context: &XcmContext) { log::trace!( target: "xcm::fungibles_adapter", @@ -123,11 +184,12 @@ impl< _dest, what, context, ); if let Ok((class, instance)) = Matcher::matches_nonfungibles(what) { - if CheckAsset::contains(&class) { - if let Some(checking_account) = CheckingAccount::get() { - let ok = Assets::mint_into(&class, &instance, &checking_account).is_ok(); - debug_assert!(ok, "`mint_into` cannot generally fail; qed"); - } + match CheckAsset::asset_checking(&class) { + // We track this asset's teleports to ensure no more come in than have gone out. + Some(MintLocation::Local) => Self::accrue_checked(class, instance), + // We track this asset's teleports to ensure no more go out than have come in. + Some(MintLocation::NonLocal) => Self::reduce_checked(class, instance), + _ => (), } } } @@ -179,7 +241,7 @@ impl< Matcher: MatchesNonFungibles, AccountIdConverter: Convert, AccountId: Clone + Eq, // can't get away without it since Currency is generic over it. - CheckAsset: Contains, + CheckAsset: AssetChecking, CheckingAccount: Get>, > TransactAsset for NonFungiblesAdapter @@ -206,6 +268,17 @@ impl< >::check_in(origin, what, context) } + fn can_check_out(dest: &MultiLocation, what: &MultiAsset, context: &XcmContext) -> XcmResult { + NonFungiblesMutateAdapter::< + Assets, + Matcher, + AccountIdConverter, + AccountId, + CheckAsset, + CheckingAccount, + >::can_check_out(dest, what, context) + } + fn check_out(dest: &MultiLocation, what: &MultiAsset, context: &XcmContext) { NonFungiblesMutateAdapter::< Assets, diff --git a/xcm/xcm-builder/tests/mock/mod.rs b/xcm/xcm-builder/tests/mock/mod.rs index 4104e91b8776..3e2f1978b1a6 100644 --- a/xcm/xcm-builder/tests/mock/mod.rs +++ b/xcm/xcm-builder/tests/mock/mod.rs @@ -34,7 +34,7 @@ use xcm_builder::{ ChildParachainAsNative, ChildParachainConvertsVia, ChildSystemParachainAsSuperuser, CurrencyAdapter as XcmCurrencyAdapter, FixedRateOfFungible, FixedWeightBounds, IsChildSystemParachain, IsConcrete, SignedAccountId32AsNative, SignedToAccountId32, - SovereignSignedViaLocation, TakeWeightCredit, + SovereignSignedViaLocation, TakeWeightCredit, MintLocation, }; pub type AccountId = AccountId32; @@ -129,7 +129,7 @@ parameter_types! { pub const KsmLocation: MultiLocation = MultiLocation::here(); pub const KusamaNetwork: NetworkId = NetworkId::Kusama; pub UniversalLocation: InteriorMultiLocation = Here; - pub CheckAccount: AccountId = XcmPallet::check_account(); + pub CheckAccount: (AccountId, MintLocation) = (XcmPallet::check_account(), MintLocation::Local); } pub type SovereignAccountOf = diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index e753fb84c211..190b8d4e5086 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -604,6 +604,13 @@ impl XcmExecutor { InitiateTeleport { assets, dest, xcm } => { // We must do this first in order to resolve wildcards. let assets = self.holding.saturating_take(assets); + for asset in assets.assets_iter() { + // We should check that the asset can actually be teleported out (for this to + // be in error, there would need to be an accounting violation by ourselves, + // so it's unlikely, but we don't want to allow that kind of bug to leak into + // a trusted chain. + Config::AssetTransactor::can_check_out(&dest, &asset, &self.context)?; + } for asset in assets.assets_iter() { Config::AssetTransactor::check_out(&dest, &asset, &self.context); } diff --git a/xcm/xcm-executor/src/traits/transact_asset.rs b/xcm/xcm-executor/src/traits/transact_asset.rs index 49cf2f43fff8..2c9bf5b3cee7 100644 --- a/xcm/xcm-executor/src/traits/transact_asset.rs +++ b/xcm/xcm-executor/src/traits/transact_asset.rs @@ -26,7 +26,7 @@ use xcm::latest::{Error as XcmError, MultiAsset, MultiLocation, Result as XcmRes /// Can be amalgamated as a tuple of items that implement this trait. In such executions, if any of the transactors /// returns `Ok(())`, then it will short circuit. Else, execution is passed to the next transactor. pub trait TransactAsset { - /// Ensure that `check_in` will result in `Ok`. + /// Ensure that `check_in` will do as expected. /// /// When composed as a tuple, all type-items are called and at least one must result in `Ok`. fn can_check_in( @@ -52,6 +52,17 @@ pub trait TransactAsset { /// value for `_what` which can cause side-effects for more than one of the type-items. fn check_in(_origin: &MultiLocation, _what: &MultiAsset, _context: &XcmContext) {} + /// Ensure that `check_out` will do as expected. + /// + /// When composed as a tuple, all type-items are called and at least one must result in `Ok`. + fn can_check_out( + _dest: &MultiLocation, + _what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + Err(XcmError::Unimplemented) + } + /// An asset has been teleported out to the given destination. This should do whatever housekeeping is needed. /// /// Implementation note: In general this will do one of two things: On chains where the asset is native, @@ -145,6 +156,23 @@ impl TransactAsset for Tuple { )* ); } + fn can_check_out(dest: &MultiLocation, what: &MultiAsset, context: &XcmContext) -> XcmResult { + for_tuples!( #( + match Tuple::can_check_out(dest, what, context) { + Err(XcmError::AssetNotFound) | Err(XcmError::Unimplemented) => (), + r => return r, + } + )* ); + log::trace!( + target: "xcm::TransactAsset::can_check_out", + "asset not found: what: {:?}, dest: {:?}, context: {:?}", + what, + dest, + context, + ); + Err(XcmError::AssetNotFound) + } + fn check_out(dest: &MultiLocation, what: &MultiAsset, context: &XcmContext) { for_tuples!( #( Tuple::check_out(dest, what, context); @@ -231,6 +259,14 @@ mod tests { Err(XcmError::AssetNotFound) } + fn can_check_out( + _dest: &MultiLocation, + _what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + Err(XcmError::AssetNotFound) + } + fn deposit_asset( _what: &MultiAsset, _who: &MultiLocation, @@ -267,6 +303,14 @@ mod tests { Err(XcmError::Overflow) } + fn can_check_out( + _dest: &MultiLocation, + _what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + Err(XcmError::Overflow) + } + fn deposit_asset( _what: &MultiAsset, _who: &MultiLocation, @@ -303,6 +347,14 @@ mod tests { Ok(()) } + fn can_check_out( + _dest: &MultiLocation, + _what: &MultiAsset, + _context: &XcmContext, + ) -> XcmResult { + Ok(()) + } + fn deposit_asset( _what: &MultiAsset, _who: &MultiLocation, From 338eae732a5d30876c606219fd3e3ce3e569b370 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 May 2022 12:49:12 +0100 Subject: [PATCH 12/15] Revert other fix --- xcm/xcm-builder/src/barriers.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index 59190f8b3d74..9542644803d7 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -87,11 +87,11 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFro ClaimAsset { .. } => (), _ => return Err(()), } - let i = iter.next().ok_or(())?; - if !matches!(i, ClearOrigin) { - return Err(()) + let mut i = iter.next().ok_or(())?; + while let ClearOrigin = i { + i = iter.next().ok_or(())?; } - match iter.next().ok_or(())? { + match i { BuyExecution { weight_limit: Limited(ref mut weight), .. } if *weight >= max_weight => { *weight = max_weight; Ok(()) From c7e09d95df8085f50d31f1a5eaa76396058a01bf Mon Sep 17 00:00:00 2001 From: Gav Date: Fri, 7 Oct 2022 10:47:02 +0100 Subject: [PATCH 13/15] Build fixes] --- Cargo.lock | 6 ++--- runtime/kusama/src/weights/xcm/mod.rs | 3 +++ .../xcm/pallet_xcm_benchmarks_generic.rs | 3 +++ runtime/kusama/src/xcm_config.rs | 6 ++--- runtime/polkadot/src/xcm_config.rs | 6 ++--- runtime/rococo/src/weights/xcm/mod.rs | 3 +++ .../xcm/pallet_xcm_benchmarks_generic.rs | 3 +++ runtime/rococo/src/xcm_config.rs | 6 ++--- runtime/westend/src/weights/xcm/mod.rs | 3 +++ .../xcm/pallet_xcm_benchmarks_generic.rs | 3 +++ runtime/westend/src/xcm_config.rs | 4 ++-- xcm/pallet-xcm/src/lib.rs | 11 +++++---- xcm/xcm-builder/src/currency_adapter.rs | 19 +++++---------- xcm/xcm-builder/src/fungibles_adapter.rs | 21 +++++++++------- xcm/xcm-builder/src/lib.rs | 9 ++++--- xcm/xcm-builder/src/nonfungibles_adapter.rs | 24 +++++++++---------- xcm/xcm-builder/src/tests/mock.rs | 2 +- xcm/xcm-builder/src/tests/origins.rs | 14 ++++++----- xcm/xcm-builder/tests/mock/mod.rs | 4 ++-- xcm/xcm-executor/src/lib.rs | 5 +++- xcm/xcm-simulator/example/src/parachain.rs | 7 +++--- xcm/xcm-simulator/example/src/relay_chain.rs | 4 ++-- 22 files changed, 95 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d1c6206ead8d..432164a8120f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3395,7 +3395,7 @@ dependencies = [ [[package]] name = "libloading" -version = "0.7.2" +version = "0.7.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "efbc0f03f9a775e9f6aed295c6a1ba2253c5757a9e03d55c6caa46a681abcddd" dependencies = [ @@ -11862,8 +11862,8 @@ version = "1.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "97fee6b57c6a41524a810daee9286c02d7752c4253064d0b05472833a438f675" dependencies = [ - "cfg-if 1.0.0", - "digest 0.10.3", + "cfg-if 0.1.10", + "digest 0.10.5", "rand 0.8.5", "static_assertions", ] diff --git a/runtime/kusama/src/weights/xcm/mod.rs b/runtime/kusama/src/weights/xcm/mod.rs index a2996a0278de..64c19a79edfd 100644 --- a/runtime/kusama/src/weights/xcm/mod.rs +++ b/runtime/kusama/src/weights/xcm/mod.rs @@ -264,4 +264,7 @@ impl XcmWeightInfo for KusamaXcmWeight { // XCM Executor does not currently support alias origin operations Weight::MAX.ref_time() } + fn unpaid_execution(_: &WeightLimit, _: &Option) -> XCMWeight { + XcmGeneric::::unpaid_execution().ref_time() + } } diff --git a/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs b/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs index 5ad10df2f5a8..69421bdb414b 100644 --- a/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs +++ b/runtime/kusama/src/weights/xcm/pallet_xcm_benchmarks_generic.rs @@ -177,4 +177,7 @@ impl WeightInfo { pub(crate) fn set_fees_mode() -> Weight { Weight::from_ref_time(3_599_000 as u64) } + pub(crate) fn unpaid_execution() -> Weight { + Weight::from_ref_time(3_111_000 as u64) + } } diff --git a/runtime/kusama/src/xcm_config.rs b/runtime/kusama/src/xcm_config.rs index 443fb6f30058..b3b15a3443ce 100644 --- a/runtime/kusama/src/xcm_config.rs +++ b/runtime/kusama/src/xcm_config.rs @@ -31,8 +31,8 @@ use xcm_builder::{ AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, BackingToPlurality, ChildParachainAsNative, ChildParachainConvertsVia, ChildSystemParachainAsSuperuser, CurrencyAdapter as XcmCurrencyAdapter, FixedWeightBounds, IsChildSystemParachain, IsConcrete, - SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, - UsingComponents, WeightInfoBounds, + MintLocation, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, + TakeWeightCredit, UsingComponents, WeightInfoBounds, }; parameter_types! { @@ -47,7 +47,7 @@ parameter_types! { /// Since Kusama is a top-level relay-chain with its own consensus, it's just our network ID. pub UniversalLocation: InteriorMultiLocation = ThisNetwork::get().into(); /// The check account, which holds any native assets that have been teleported out and not back in (yet). - pub CheckAccount: AccountId = XcmPallet::check_account(); + pub CheckAccount: (AccountId, MintLocation) = (XcmPallet::check_account(), MintLocation::Local); } /// The canonical means of converting a `MultiLocation` into an `AccountId`, used when we want to determine diff --git a/runtime/polkadot/src/xcm_config.rs b/runtime/polkadot/src/xcm_config.rs index a9dc71d274a2..9e1eeaa67f41 100644 --- a/runtime/polkadot/src/xcm_config.rs +++ b/runtime/polkadot/src/xcm_config.rs @@ -30,8 +30,8 @@ use xcm_builder::{ AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, BackingToPlurality, ChildParachainAsNative, ChildParachainConvertsVia, CurrencyAdapter as XcmCurrencyAdapter, FixedWeightBounds, - IsConcrete, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, - TakeWeightCredit, UsingComponents, + IsConcrete, MintLocation, SignedAccountId32AsNative, SignedToAccountId32, + SovereignSignedViaLocation, TakeWeightCredit, UsingComponents, }; parameter_types! { @@ -44,7 +44,7 @@ parameter_types! { /// Our location in the universe of consensus systems. pub const UniversalLocation: InteriorMultiLocation = X1(GlobalConsensus(ThisNetwork::get())); /// The check account, which holds any native assets that have been teleported out and not back in (yet). - pub CheckAccount: AccountId = XcmPallet::check_account(); + pub CheckAccount: (AccountId, MintLocation) = (XcmPallet::check_account(), MintLocation::Local); } /// The canonical means of converting a `MultiLocation` into an `AccountId`, used when we want to determine diff --git a/runtime/rococo/src/weights/xcm/mod.rs b/runtime/rococo/src/weights/xcm/mod.rs index 84bc50dff739..e0221357d24a 100644 --- a/runtime/rococo/src/weights/xcm/mod.rs +++ b/runtime/rococo/src/weights/xcm/mod.rs @@ -264,4 +264,7 @@ impl XcmWeightInfo for RococoXcmWeight { // XCM Executor does not currently support alias origin operations Weight::MAX.ref_time() } + fn unpaid_execution(_: &WeightLimit, _: &Option) -> XCMWeight { + XcmGeneric::::unpaid_execution().ref_time() + } } diff --git a/runtime/rococo/src/weights/xcm/pallet_xcm_benchmarks_generic.rs b/runtime/rococo/src/weights/xcm/pallet_xcm_benchmarks_generic.rs index f9308d051ad6..f2d786e85a46 100644 --- a/runtime/rococo/src/weights/xcm/pallet_xcm_benchmarks_generic.rs +++ b/runtime/rococo/src/weights/xcm/pallet_xcm_benchmarks_generic.rs @@ -180,4 +180,7 @@ impl WeightInfo { pub(crate) fn set_fees_mode() -> Weight { Weight::from_ref_time(3_721_000 as u64) } + pub(crate) fn unpaid_execution() -> Weight { + Weight::from_ref_time(3_111_000 as u64) + } } diff --git a/runtime/rococo/src/xcm_config.rs b/runtime/rococo/src/xcm_config.rs index 937446d78c94..503af148e8ce 100644 --- a/runtime/rococo/src/xcm_config.rs +++ b/runtime/rococo/src/xcm_config.rs @@ -31,8 +31,8 @@ use xcm_builder::{ AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, BackingToPlurality, ChildParachainAsNative, ChildParachainConvertsVia, ChildSystemParachainAsSuperuser, CurrencyAdapter as XcmCurrencyAdapter, FixedWeightBounds, IsChildSystemParachain, IsConcrete, - SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, - UsingComponents, WeightInfoBounds, + MintLocation, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, + TakeWeightCredit, UsingComponents, WeightInfoBounds, }; use xcm_executor::XcmExecutor; @@ -40,7 +40,7 @@ parameter_types! { pub const TokenLocation: MultiLocation = Here.into_location(); pub const ThisNetwork: NetworkId = NetworkId::Rococo; pub UniversalLocation: InteriorMultiLocation = ThisNetwork::get().into(); - pub CheckAccount: AccountId = XcmPallet::check_account(); + pub CheckAccount: (AccountId, MintLocation) = (XcmPallet::check_account(), MintLocation::Local); } pub type LocationConverter = diff --git a/runtime/westend/src/weights/xcm/mod.rs b/runtime/westend/src/weights/xcm/mod.rs index 0c3ebe4b0cd7..4913ea15ae92 100644 --- a/runtime/westend/src/weights/xcm/mod.rs +++ b/runtime/westend/src/weights/xcm/mod.rs @@ -265,4 +265,7 @@ impl XcmWeightInfo for WestendXcmWeight { // XCM Executor does not currently support alias origin operations Weight::MAX.ref_time() } + fn unpaid_execution(_: &WeightLimit, _: &Option) -> XCMWeight { + XcmGeneric::::unpaid_execution().ref_time() + } } diff --git a/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs b/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs index e61ce93093c7..2a811ac3d2f0 100644 --- a/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs +++ b/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs @@ -177,4 +177,7 @@ impl WeightInfo { pub(crate) fn set_fees_mode() -> Weight { Weight::from_ref_time(3_721_000 as u64) } + pub(crate) fn unpaid_execution() -> Weight { + Weight::from_ref_time(3_111_000 as u64) + } } diff --git a/runtime/westend/src/xcm_config.rs b/runtime/westend/src/xcm_config.rs index 878842964b14..94098bbfc406 100644 --- a/runtime/westend/src/xcm_config.rs +++ b/runtime/westend/src/xcm_config.rs @@ -30,7 +30,7 @@ use xcm_builder::{ AccountId32Aliases, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, ChildParachainAsNative, ChildParachainConvertsVia, ChildSystemParachainAsSuperuser, - CurrencyAdapter as XcmCurrencyAdapter, IsChildSystemParachain, IsConcrete, + CurrencyAdapter as XcmCurrencyAdapter, IsChildSystemParachain, IsConcrete, MintLocation, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, UsingComponents, WeightInfoBounds, }; @@ -40,7 +40,7 @@ parameter_types! { pub const TokenLocation: MultiLocation = Here.into_location(); pub const ThisNetwork: NetworkId = Westend; pub UniversalLocation: InteriorMultiLocation = ThisNetwork::get().into(); - pub CheckAccount: AccountId = XcmPallet::check_account(); + pub CheckAccount: (AccountId, MintLocation) = (XcmPallet::check_account(), MintLocation::Local); } pub type LocationConverter = diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index 4ec4e9241aca..0073221d30fa 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -35,7 +35,10 @@ use sp_runtime::{ RuntimeDebug, }; use sp_std::{boxed::Box, marker::PhantomData, prelude::*, result::Result, vec}; -use xcm::{latest::{QueryResponseInfo, Weight as XcmWeight}, prelude::*}; +use xcm::{ + latest::{QueryResponseInfo, Weight as XcmWeight}, + prelude::*, +}; use xcm_executor::traits::{Convert, ConvertOrigin}; use frame_support::{ @@ -779,8 +782,8 @@ pub mod pallet { origin_location, message, hash, - max_weight.ref_time(), - max_weight.ref_time(), + max_weight, //.ref_time(), + max_weight, //.ref_time(), ); let result = Ok(Some(outcome.weight_used().saturating_add(100_000_000)).into()); Self::deposit_event(Event::Attempted(outcome)); @@ -1479,7 +1482,7 @@ impl xcm_executor::traits::Enact for LockTicket { None => { locks .try_push((self.amount, self.unlocker.clone().into())) - .map_err(|()| UnexpectedState)?; + .map_err(|(_balance, _location)| UnexpectedState)?; }, } LockedFungibles::::insert(&self.sovereign_account, locks); diff --git a/xcm/xcm-builder/src/currency_adapter.rs b/xcm/xcm-builder/src/currency_adapter.rs index ea3ab09b8558..09818248cddf 100644 --- a/xcm/xcm-builder/src/currency_adapter.rs +++ b/xcm/xcm-builder/src/currency_adapter.rs @@ -16,6 +16,7 @@ //! Adapters to work with `frame_support::traits::Currency` through XCM. +use super::MintLocation; use frame_support::traits::{ExistenceRequirement::AllowDeath, Get, WithdrawReasons}; use sp_runtime::traits::CheckedSub; use sp_std::{marker::PhantomData, result}; @@ -24,7 +25,6 @@ use xcm_executor::{ traits::{Convert, MatchesFungible, TransactAsset}, Assets, }; -use super::MintLocation; /// Asset transaction errors. enum Error { @@ -116,18 +116,11 @@ impl< Currency::deposit_creating(&checked_account, amount); } fn reduce_checked(checked_account: AccountId, amount: Currency::Balance) { - let ok = Currency::withdraw( - &checked_account, - amount, - WithdrawReasons::TRANSFER, - AllowDeath, - ) - .is_ok(); - debug_assert!( - ok, - "`can_check_in` must have returned `true` immediately prior; qed" - ); -} + let ok = + Currency::withdraw(&checked_account, amount, WithdrawReasons::TRANSFER, AllowDeath) + .is_ok(); + debug_assert!(ok, "`can_check_in` must have returned `true` immediately prior; qed"); + } } impl< diff --git a/xcm/xcm-builder/src/fungibles_adapter.rs b/xcm/xcm-builder/src/fungibles_adapter.rs index e121b9bf16e8..32cf6f014a5f 100644 --- a/xcm/xcm-builder/src/fungibles_adapter.rs +++ b/xcm/xcm-builder/src/fungibles_adapter.rs @@ -69,6 +69,13 @@ pub trait AssetChecking { fn asset_checking(asset: &AssetId) -> Option; } +pub struct NoChecking; +impl AssetChecking for NoChecking { + fn asset_checking(_: &AssetId) -> Option { + None + } +} + pub struct LocalMint(sp_std::marker::PhantomData); impl> AssetChecking for LocalMint { fn asset_checking(asset: &AssetId) -> Option { @@ -90,7 +97,9 @@ impl> AssetChecking for NonLocalMint { } pub struct DualMint(sp_std::marker::PhantomData<(L, R)>); -impl, R: Contains> AssetChecking for DualMint { +impl, R: Contains> AssetChecking + for DualMint +{ fn asset_checking(asset: &AssetId) -> Option { if L::contains(asset) { Some(MintLocation::Local) @@ -118,18 +127,12 @@ impl< AccountId: Clone, // can't get away without it since Currency is generic over it. CheckAsset: AssetChecking, CheckingAccount: Get, - > FungiblesMutateAdapter< - Assets, - Matcher, - AccountIdConverter, - AccountId, - CheckAsset, - CheckingAccount, > + FungiblesMutateAdapter { fn can_accrue_checked(asset_id: Assets::AssetId, amount: Assets::Balance) -> XcmResult { let checking_account = CheckingAccount::get(); - Assets::can_deposit(asset_id, &checking_account, amount) + Assets::can_deposit(asset_id, &checking_account, amount, true) .into_result() .map_err(|_| XcmError::NotDepositable) } diff --git a/xcm/xcm-builder/src/lib.rs b/xcm/xcm-builder/src/lib.rs index 4b2ad651dc1e..f48480616ac5 100644 --- a/xcm/xcm-builder/src/lib.rs +++ b/xcm/xcm-builder/src/lib.rs @@ -47,8 +47,8 @@ pub use asset_conversion::{ConvertedAbstractAssetId, ConvertedConcreteAssetId}; mod barriers; pub use barriers::{ - AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, AllowExplicitUnpaidExecutionFrom, IsChildSystemParachain, + AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses, AllowSubscriptionsFrom, + AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, IsChildSystemParachain, TakeWeightCredit, WithComputedOrigin, }; @@ -56,7 +56,10 @@ mod currency_adapter; pub use currency_adapter::CurrencyAdapter; mod fungibles_adapter; -pub use fungibles_adapter::{FungiblesAdapter, FungiblesMutateAdapter, FungiblesTransferAdapter, MintLocation, LocalMint, NonLocalMint, DualMint, AssetChecking}; +pub use fungibles_adapter::{ + AssetChecking, DualMint, FungiblesAdapter, FungiblesMutateAdapter, FungiblesTransferAdapter, + LocalMint, MintLocation, NoChecking, NonLocalMint, +}; mod nonfungibles_adapter; pub use nonfungibles_adapter::{ diff --git a/xcm/xcm-builder/src/nonfungibles_adapter.rs b/xcm/xcm-builder/src/nonfungibles_adapter.rs index 5fed83c2623e..2735a03ab6f3 100644 --- a/xcm/xcm-builder/src/nonfungibles_adapter.rs +++ b/xcm/xcm-builder/src/nonfungibles_adapter.rs @@ -16,14 +16,14 @@ //! Adapters to work with `frame_support::traits::tokens::fungibles` through XCM. +use crate::{AssetChecking, MintLocation}; use frame_support::{ ensure, - traits::{tokens::nonfungibles, Contains, Get}, + traits::{tokens::nonfungibles, Get}, }; use sp_std::{marker::PhantomData, prelude::*, result}; use xcm::latest::prelude::*; use xcm_executor::traits::{Convert, Error as MatchError, MatchesNonFungibles, TransactAsset}; -use crate::{AssetChecking, MintLocation}; pub struct NonFungiblesTransferAdapter( PhantomData<(Assets, Matcher, AccountIdConverter, AccountId)>, @@ -72,7 +72,8 @@ impl< AccountId: Clone + Eq, // can't get away without it since Currency is generic over it. CheckAsset: AssetChecking, CheckingAccount: Get>, - > NonFungiblesMutateAdapter< + > + NonFungiblesMutateAdapter< Assets, Matcher, AccountIdConverter, @@ -81,11 +82,11 @@ impl< CheckingAccount, > { - fn can_accrue_checked(class: Assets::CollectionId, instance: Assets::InstanceId) -> XcmResult { + fn can_accrue_checked(class: Assets::CollectionId, instance: Assets::ItemId) -> XcmResult { ensure!(Assets::owner(&class, &instance).is_none(), XcmError::NotDepositable); Ok(()) } - fn can_reduce_checked(class: Assets::CollectionId, instance: Assets::InstanceId) -> XcmResult { + fn can_reduce_checked(class: Assets::CollectionId, instance: Assets::ItemId) -> XcmResult { if let Some(checking_account) = CheckingAccount::get() { // This is an asset whose teleports we track. let owner = Assets::owner(&class, &instance); @@ -94,24 +95,21 @@ impl< } Ok(()) } - fn accrue_checked(class: Assets::CollectionId, instance: Assets::InstanceId) { + fn accrue_checked(class: Assets::CollectionId, instance: Assets::ItemId) { if let Some(checking_account) = CheckingAccount::get() { let ok = Assets::mint_into(&class, &instance, &checking_account).is_ok(); debug_assert!(ok, "`mint_into` cannot generally fail; qed"); } -} - fn reduce_checked(class: Assets::CollectionId, instance: Assets::InstanceId) { + } + fn reduce_checked(class: Assets::CollectionId, instance: Assets::ItemId) { let ok = Assets::burn(&class, &instance, None).is_ok(); - debug_assert!( - ok, - "`can_check_in` must have returned `true` immediately prior; qed" - ); + debug_assert!(ok, "`can_check_in` must have returned `true` immediately prior; qed"); } } impl< Assets: nonfungibles::Mutate, - Matcher: MatchesNonFungibles, + Matcher: MatchesNonFungibles, AccountIdConverter: Convert, AccountId: Clone + Eq, // can't get away without it since Currency is generic over it. CheckAsset: AssetChecking, diff --git a/xcm/xcm-builder/src/tests/mock.rs b/xcm/xcm-builder/src/tests/mock.rs index 58bf1a2fa0ca..46590b08f679 100644 --- a/xcm/xcm-builder/src/tests/mock.rs +++ b/xcm/xcm-builder/src/tests/mock.rs @@ -16,7 +16,7 @@ use crate::{barriers::AllowSubscriptionsFrom, test_utils::*}; pub use crate::{ - AllowKnownQueryResponses, AllowTopLevelPaidExecutionFrom, AllowExplicitUnpaidExecutionFrom, + AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses, AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, FixedRateOfFungible, FixedWeightBounds, TakeWeightCredit, }; use frame_support::traits::ContainsPair; diff --git a/xcm/xcm-builder/src/tests/origins.rs b/xcm/xcm-builder/src/tests/origins.rs index aef55a54603e..6ae3397492fe 100644 --- a/xcm/xcm-builder/src/tests/origins.rs +++ b/xcm/xcm-builder/src/tests/origins.rs @@ -86,18 +86,20 @@ fn unpaid_execution_should_work() { // asks. AllowExplicitUnpaidFrom::set(vec![X1(Parachain(2)).into()]); // Asking for unpaid execution of up to 9 weight on the assumption it is origin of #2. - let message = Xcm(vec![ - UnpaidExecution { weight_limit: Limited(9), check_origin: Some(Parachain(2).into()) }, - ]); + let message = Xcm(vec![UnpaidExecution { + weight_limit: Limited(9), + check_origin: Some(Parachain(2).into()), + }]); let hash = fake_message_hash(&message); let r = XcmExecutor::::execute_xcm(Parachain(1), message.clone(), hash, 50); assert_eq!(r, Outcome::Incomplete(10, XcmError::BadOrigin)); let r = XcmExecutor::::execute_xcm(Parachain(2), message.clone(), hash, 50); assert_eq!(r, Outcome::Error(XcmError::Barrier)); - let message = Xcm(vec![ - UnpaidExecution { weight_limit: Limited(10), check_origin: Some(Parachain(2).into()) }, - ]); + let message = Xcm(vec![UnpaidExecution { + weight_limit: Limited(10), + check_origin: Some(Parachain(2).into()), + }]); let r = XcmExecutor::::execute_xcm(Parachain(2), message.clone(), hash, 50); assert_eq!(r, Outcome::Complete(10)); } diff --git a/xcm/xcm-builder/tests/mock/mod.rs b/xcm/xcm-builder/tests/mock/mod.rs index 8795a4feed26..410aa19d6315 100644 --- a/xcm/xcm-builder/tests/mock/mod.rs +++ b/xcm/xcm-builder/tests/mock/mod.rs @@ -32,8 +32,8 @@ use xcm_builder::{ AccountId32Aliases, AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, ChildParachainAsNative, ChildParachainConvertsVia, ChildSystemParachainAsSuperuser, CurrencyAdapter as XcmCurrencyAdapter, FixedRateOfFungible, FixedWeightBounds, - IsChildSystemParachain, IsConcrete, SignedAccountId32AsNative, SignedToAccountId32, - SovereignSignedViaLocation, TakeWeightCredit, MintLocation, + IsChildSystemParachain, IsConcrete, MintLocation, SignedAccountId32AsNative, + SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, }; pub type AccountId = AccountId32; diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index ddf641583a60..df7d9061d5b9 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -874,7 +874,10 @@ impl XcmExecutor { }, AliasOrigin(_) => Err(XcmError::NoPermission), UnpaidExecution { check_origin, .. } => { - ensure!(check_origin.is_none() || self.context.origin == check_origin, XcmError::BadOrigin); + ensure!( + check_origin.is_none() || self.context.origin == check_origin, + XcmError::BadOrigin + ); Ok(()) }, HrmpNewChannelOpenRequest { .. } => Err(XcmError::Unimplemented), diff --git a/xcm/xcm-simulator/example/src/parachain.rs b/xcm/xcm-simulator/example/src/parachain.rs index abe9e0612689..6101b78bd9df 100644 --- a/xcm/xcm-simulator/example/src/parachain.rs +++ b/xcm/xcm-simulator/example/src/parachain.rs @@ -39,8 +39,9 @@ use xcm::{latest::prelude::*, VersionedXcm}; use xcm_builder::{ Account32Hash, AccountId32Aliases, AllowUnpaidExecutionFrom, ConvertedConcreteId, CurrencyAdapter as XcmCurrencyAdapter, EnsureXcmOrigin, FixedRateOfFungible, FixedWeightBounds, - IsConcrete, NativeAsset, NonFungiblesAdapter, ParentIsPreset, SiblingParachainConvertsVia, - SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, + IsConcrete, NativeAsset, NoChecking, NonFungiblesAdapter, ParentIsPreset, + SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, + SovereignSignedViaLocation, }; use xcm_executor::{ traits::{Convert, JustTry}, @@ -200,7 +201,7 @@ pub type LocalAssetTransactor = ( ConvertedConcreteId, SovereignAccountOf, AccountId, - Nothing, + NoChecking, (), >, ); diff --git a/xcm/xcm-simulator/example/src/relay_chain.rs b/xcm/xcm-simulator/example/src/relay_chain.rs index a13cdabb3ea4..f743b84ab280 100644 --- a/xcm/xcm-simulator/example/src/relay_chain.rs +++ b/xcm/xcm-simulator/example/src/relay_chain.rs @@ -30,7 +30,7 @@ use xcm_builder::{ Account32Hash, AccountId32Aliases, AllowUnpaidExecutionFrom, AsPrefixedGeneralIndex, ChildParachainAsNative, ChildParachainConvertsVia, ChildSystemParachainAsSuperuser, ConvertedConcreteId, CurrencyAdapter as XcmCurrencyAdapter, FixedRateOfFungible, - FixedWeightBounds, IsConcrete, NonFungiblesAdapter, SignedAccountId32AsNative, + FixedWeightBounds, IsConcrete, NoChecking, NonFungiblesAdapter, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, }; use xcm_executor::{traits::JustTry, Config, XcmExecutor}; @@ -135,7 +135,7 @@ pub type LocalAssetTransactor = ( ConvertedConcreteId, JustTry>, LocationToAccountId, AccountId, - Nothing, + NoChecking, (), >, ); From 7b38b4a488f752d1aae7cdd41c0eed3b8a2f93f5 Mon Sep 17 00:00:00 2001 From: Gav Date: Fri, 7 Oct 2022 10:53:58 +0100 Subject: [PATCH 14/15] Tests build --- xcm/pallet-xcm-benchmarks/src/fungible/mock.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs b/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs index 77cdd058e800..adef24f6b3df 100644 --- a/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs +++ b/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs @@ -30,7 +30,7 @@ use sp_runtime::{ BuildStorage, }; use xcm::latest::prelude::*; -use xcm_builder::AllowUnpaidExecutionFrom; +use xcm_builder::{AllowUnpaidExecutionFrom, MintLocation}; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; @@ -123,7 +123,7 @@ pub type AssetTransactor = xcm_builder::CurrencyAdapter< MatchAnyFungible, AccountIdConverter, u64, - CheckedAccount, + CheckingAccount, >; parameter_types! { @@ -179,6 +179,7 @@ impl crate::Config for Test { pub type TrustedTeleporters = (xcm_builder::Case,); parameter_types! { + pub const CheckingAccount: Option<(u64, MintLocation)> = Some((100, MintLocation::Local)); pub const CheckedAccount: Option = Some(100); pub const ChildTeleporter: MultiLocation = Parachain(1000).into_location(); pub const TrustedTeleporter: Option<(MultiLocation, MultiAsset)> = Some(( From 7bdc31d863cd8f624bfb616f4b252b18887b78d6 Mon Sep 17 00:00:00 2001 From: Gav Date: Fri, 7 Oct 2022 11:17:44 +0100 Subject: [PATCH 15/15] Benchmark fixes --- xcm/pallet-xcm-benchmarks/Cargo.toml | 4 +++- xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs | 6 +++--- xcm/pallet-xcm-benchmarks/src/fungible/mock.rs | 3 +-- xcm/pallet-xcm-benchmarks/src/fungible/mod.rs | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/xcm/pallet-xcm-benchmarks/Cargo.toml b/xcm/pallet-xcm-benchmarks/Cargo.toml index 91872fc2f3b7..c2b1de663813 100644 --- a/xcm/pallet-xcm-benchmarks/Cargo.toml +++ b/xcm/pallet-xcm-benchmarks/Cargo.toml @@ -18,6 +18,7 @@ sp-io = { default-features = false, branch = "master", git = "https://github.com xcm-executor = { path = "../xcm-executor", default-features = false } frame-benchmarking = { default-features = false, branch = "master", git = "https://github.com/paritytech/substrate" } xcm = { path = "..", default-features = false } +xcm-builder = { path = "../xcm-builder", default-features = false } log = "0.4.17" [dev-dependencies] @@ -25,7 +26,6 @@ pallet-balances = { branch = "master", git = "https://github.com/paritytech/subs pallet-assets = { branch = "master", git = "https://github.com/paritytech/substrate" } sp-core = { branch = "master", git = "https://github.com/paritytech/substrate" } sp-tracing = { branch = "master", git = "https://github.com/paritytech/substrate" } -xcm-builder = { path = "../xcm-builder" } xcm = { path = ".." } # temp pallet-xcm = { path = "../pallet-xcm" } @@ -43,10 +43,12 @@ std = [ "sp-io/std", "sp-runtime/std", "sp-std/std", + "xcm-builder/std", "xcm-executor/std" ] runtime-benchmarks = [ "xcm/runtime-benchmarks", + "xcm-builder/runtime-benchmarks", "xcm-executor/runtime-benchmarks", "frame-benchmarking/runtime-benchmarks", "frame-support/runtime-benchmarks", diff --git a/xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs b/xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs index 7a68e7ed939e..fade0d4869b3 100644 --- a/xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs +++ b/xcm/pallet-xcm-benchmarks/src/fungible/benchmarking.rs @@ -136,7 +136,7 @@ benchmarks_instance_pallet! { let (trusted_teleporter, teleportable_asset) = T::TrustedTeleporter::get() .ok_or(BenchmarkError::Skip)?; - if let Some(checked_account) = T::CheckedAccount::get() { + if let Some((checked_account, _)) = T::CheckedAccount::get() { T::TransactAsset::mint_into( &checked_account, < @@ -223,7 +223,7 @@ benchmarks_instance_pallet! { holding.push(asset.clone()); // Checked account starts at zero - assert!(T::CheckedAccount::get().map_or(true, |c| T::TransactAsset::balance(&c).is_zero())); + assert!(T::CheckedAccount::get().map_or(true, |(c, _)| T::TransactAsset::balance(&c).is_zero())); let mut executor = new_executor::(Default::default()); executor.set_holding(holding.into()); @@ -236,7 +236,7 @@ benchmarks_instance_pallet! { }: { executor.bench_process(xcm)?; } verify { - if let Some(checked_account) = T::CheckedAccount::get() { + if let Some((checked_account, _)) = T::CheckedAccount::get() { // teleport checked account should have received some asset. assert!(!T::TransactAsset::balance(&checked_account).is_zero()); } diff --git a/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs b/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs index adef24f6b3df..b1004818e685 100644 --- a/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs +++ b/xcm/pallet-xcm-benchmarks/src/fungible/mock.rs @@ -180,7 +180,6 @@ pub type TrustedTeleporters = (xcm_builder::Case,); parameter_types! { pub const CheckingAccount: Option<(u64, MintLocation)> = Some((100, MintLocation::Local)); - pub const CheckedAccount: Option = Some(100); pub const ChildTeleporter: MultiLocation = Parachain(1000).into_location(); pub const TrustedTeleporter: Option<(MultiLocation, MultiAsset)> = Some(( ChildTeleporter::get(), @@ -194,7 +193,7 @@ parameter_types! { impl xcm_balances_benchmark::Config for Test { type TransactAsset = Balances; - type CheckedAccount = CheckedAccount; + type CheckedAccount = CheckingAccount; type TrustedTeleporter = TrustedTeleporter; fn get_multi_asset() -> MultiAsset { diff --git a/xcm/pallet-xcm-benchmarks/src/fungible/mod.rs b/xcm/pallet-xcm-benchmarks/src/fungible/mod.rs index 0194c3b97a07..011e84a0e42e 100644 --- a/xcm/pallet-xcm-benchmarks/src/fungible/mod.rs +++ b/xcm/pallet-xcm-benchmarks/src/fungible/mod.rs @@ -34,7 +34,7 @@ pub mod pallet { type TransactAsset: frame_support::traits::fungible::Mutate; /// The account used to check assets being teleported. - type CheckedAccount: Get>; + type CheckedAccount: Get>; /// A trusted location which we allow teleports from, and the asset we allow to teleport. type TrustedTeleporter: Get>;