From 48b0c1f15c44b32550b4757e7df41b9e756e82d7 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 26 Aug 2021 12:45:02 +0200 Subject: [PATCH 01/16] XCM: Introduce AssetTrap --- xcm/pallet-xcm/src/lib.rs | 32 +++++++++++++++++-- xcm/src/v2/mod.rs | 6 ++-- xcm/xcm-builder/src/barriers.rs | 11 +++---- xcm/xcm-builder/src/mock.rs | 1 + xcm/xcm-builder/src/tests.rs | 25 --------------- xcm/xcm-executor/src/assets.rs | 5 +++ xcm/xcm-executor/src/config.rs | 6 +++- xcm/xcm-executor/src/lib.rs | 13 ++++---- xcm/xcm-executor/src/traits/mod.rs | 2 ++ xcm/xcm-executor/src/traits/should_execute.rs | 4 +-- xcm/xcm-simulator/example/src/parachain.rs | 1 + xcm/xcm-simulator/example/src/relay_chain.rs | 1 + 12 files changed, 59 insertions(+), 48 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index acc5a6269f23..b8f3a34fa5f3 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -51,7 +51,7 @@ pub mod pallet { }; use frame_system::{pallet_prelude::*, Config as SysConfig}; use sp_runtime::traits::{AccountIdConversion, BlockNumberProvider}; - use xcm_executor::traits::{InvertLocation, OnResponse, WeightBounds}; + use xcm_executor::{Assets, traits::{InvertLocation, OnResponse, WeightBounds, DropAssets}}; #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] @@ -236,6 +236,12 @@ pub mod pallet { pub(super) type Queries = StorageMap<_, Blake2_128Concat, QueryId, QueryStatus, OptionQuery>; + /// The latest available query index. + #[pallet::storage] + #[pallet::getter(fn asset_trap)] + pub(super) type AssetTraps = + StorageMap<_, Blake2_128Concat, MultiLocation, Vec, ValueQuery>; + #[pallet::hooks] impl Hooks> for Pallet {} @@ -553,8 +559,29 @@ pub mod pallet { } } + impl DropAssets for Pallet { + fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { + let versioned = VersionedMultiAssets::from(MultiAssets::from(assets)); + AssetTraps::::append(origin, versioned); + 0 + } + + fn collect_assets(origin: &MultiLocation, _max_weight: Weight) -> (Assets, Weight) { + // TODO: Check max_weight and figure out weight of this operation. + let versioned_assetss: Vec = AssetTraps::::take(origin); + let mut all_assets = Assets::new(); + for versioned_assets in versioned_assetss.into_iter() { + if let Ok(assets) = MultiAssets::try_from(versioned_assets) { + for asset in assets.drain().into_iter() { + all_assets.subsume(asset); + } + } + } + (all_assets, 0) + } + } + impl OnResponse for Pallet { - /// Returns `true` if we are expecting a response from `origin` for query `query_id`. fn expecting_response(origin: &MultiLocation, query_id: QueryId) -> bool { if let Some(QueryStatus::Pending { responder, .. }) = Queries::::get(query_id) { return MultiLocation::try_from(responder).map_or(false, |r| origin == &r) @@ -562,7 +589,6 @@ pub mod pallet { false } - /// Handler for receiving a `response` from `origin` relating to `query_id`. fn on_response( origin: &MultiLocation, query_id: QueryId, diff --git a/xcm/src/v2/mod.rs b/xcm/src/v2/mod.rs index 42d0283652d8..bddc28a7f8ae 100644 --- a/xcm/src/v2/mod.rs +++ b/xcm/src/v2/mod.rs @@ -226,11 +226,11 @@ pub enum Instruction { /// Errors: ReceiveTeleportedAsset(MultiAssets), - /// Respond with information that the local system is expecting. + /// Indication of the contents of the holding register corresponding to the `QueryHolding` + /// order of `query_id`. /// /// - `query_id`: The identifier of the query that resulted in this message being sent. - /// - `response`: The message content. - /// - `max_weight`: The maximum weight that handling this response should take. + /// - `assets`: The message content. /// /// Safety: No concerns. /// diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index 4d82380a03c1..c9a0fa30598e 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -30,7 +30,7 @@ use xcm_executor::traits::{OnResponse, ShouldExecute}; pub struct TakeWeightCredit; impl ShouldExecute for TakeWeightCredit { fn should_execute( - _origin: &Option, + _origin: &MultiLocation, _top_level: bool, _message: &mut Xcm, max_weight: Weight, @@ -49,13 +49,12 @@ impl ShouldExecute for TakeWeightCredit { pub struct AllowTopLevelPaidExecutionFrom(PhantomData); impl> ShouldExecute for AllowTopLevelPaidExecutionFrom { fn should_execute( - origin: &Option, + origin: &MultiLocation, top_level: bool, message: &mut Xcm, max_weight: Weight, _weight_credit: &mut Weight, ) -> Result<(), ()> { - let origin = origin.as_ref().ok_or(())?; ensure!(T::contains(origin), ()); ensure!(top_level, ()); let mut iter = message.0.iter_mut(); @@ -87,13 +86,12 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFro pub struct AllowUnpaidExecutionFrom(PhantomData); impl> ShouldExecute for AllowUnpaidExecutionFrom { fn should_execute( - origin: &Option, + origin: &MultiLocation, _top_level: bool, _message: &mut Xcm, _max_weight: Weight, _weight_credit: &mut Weight, ) -> Result<(), ()> { - let origin = origin.as_ref().ok_or(())?; ensure!(T::contains(origin), ()); Ok(()) } @@ -115,13 +113,12 @@ impl> Contains for IsChildSystemPara pub struct AllowKnownQueryResponses(PhantomData); impl ShouldExecute for AllowKnownQueryResponses { fn should_execute( - origin: &Option, + origin: &MultiLocation, _top_level: bool, message: &mut Xcm, _max_weight: Weight, _weight_credit: &mut Weight, ) -> Result<(), ()> { - let origin = origin.as_ref().ok_or(())?; match message.0.first() { Some(QueryResponse { query_id, .. }) if ResponseHandler::expecting_response(origin, *query_id) => diff --git a/xcm/xcm-builder/src/mock.rs b/xcm/xcm-builder/src/mock.rs index 21cb0c8650c4..b32082cccd5e 100644 --- a/xcm/xcm-builder/src/mock.rs +++ b/xcm/xcm-builder/src/mock.rs @@ -282,4 +282,5 @@ impl Config for TestConfig { type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; type ResponseHandler = TestResponseHandler; + type AssetTrap = (); // TODO: TestAssetTrap } diff --git a/xcm/xcm-builder/src/tests.rs b/xcm/xcm-builder/src/tests.rs index 22f72041c364..9319707b03df 100644 --- a/xcm/xcm-builder/src/tests.rs +++ b/xcm/xcm-builder/src/tests.rs @@ -254,31 +254,6 @@ fn errors_should_return_unused_weight() { assert_eq!(sent_xcm(), vec![]); } -#[test] -fn weight_bounds_should_respect_instructions_limit() { - MaxInstructions::set(3); - let mut message = Xcm(vec![ClearOrigin; 4]); - // 4 instructions are too many. - assert_eq!(::Weigher::weight(&mut message), Err(())); - - let mut message = - Xcm(vec![SetErrorHandler(Xcm(vec![ClearOrigin])), SetAppendix(Xcm(vec![ClearOrigin]))]); - // 4 instructions are too many, even when hidden within 2. - assert_eq!(::Weigher::weight(&mut message), Err(())); - - let mut message = - Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm( - vec![ClearOrigin], - ))]))]))]); - // 4 instructions are too many, even when it's just one that's 3 levels deep. - assert_eq!(::Weigher::weight(&mut message), Err(())); - - let mut message = - Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(vec![ClearOrigin]))]))]); - // 3 instructions are OK. - assert_eq!(::Weigher::weight(&mut message), Ok(30)); -} - #[test] fn code_registers_should_work() { // we'll let them have message execution for free. diff --git a/xcm/xcm-executor/src/assets.rs b/xcm/xcm-executor/src/assets.rs index 8b87cc7ca5c4..c7049a3a6412 100644 --- a/xcm/xcm-executor/src/assets.rs +++ b/xcm/xcm-executor/src/assets.rs @@ -94,6 +94,11 @@ impl Assets { self.fungible.len() + self.non_fungible.len() } + /// Returns `true` if `self` contains no assets. + pub fn is_empty(&self) -> bool { + self.fungible.is_empty() && self.non_fungible.is_empty() + } + /// A borrowing iterator over the fungible assets. pub fn fungible_assets_iter<'a>(&'a self) -> impl Iterator + 'a { self.fungible diff --git a/xcm/xcm-executor/src/config.rs b/xcm/xcm-executor/src/config.rs index abb241c49274..68027b0e5388 100644 --- a/xcm/xcm-executor/src/config.rs +++ b/xcm/xcm-executor/src/config.rs @@ -16,7 +16,7 @@ use crate::traits::{ ConvertOrigin, FilterAssetLocation, InvertLocation, OnResponse, ShouldExecute, TransactAsset, - WeightBounds, WeightTrader, + WeightBounds, WeightTrader, DropAssets, }; use frame_support::{ dispatch::{Dispatchable, Parameter}, @@ -58,4 +58,8 @@ pub trait Config { /// What to do when a response of a query is found. type ResponseHandler: OnResponse; + + /// The general asset trap - handler for when assets are left in the Holding Register at the + /// end of execution. + type AssetTrap: DropAssets; } diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 5484121d5ebc..5d2e059ec1b1 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -32,7 +32,7 @@ use xcm::latest::{ pub mod traits; use traits::{ ConvertOrigin, FilterAssetLocation, InvertLocation, OnResponse, ShouldExecute, TransactAsset, - WeightBounds, WeightTrader, + WeightBounds, WeightTrader, DropAssets, }; mod assets; @@ -86,7 +86,6 @@ impl ExecuteXcm for XcmExecutor { if xcm_weight > weight_limit { return Outcome::Error(XcmError::WeightLimitReached(xcm_weight)) } - let origin = Some(origin); if let Err(_) = Config::Barrier::should_execute( &origin, @@ -98,7 +97,7 @@ impl ExecuteXcm for XcmExecutor { return Outcome::Error(XcmError::Barrier) } - let mut vm = Self::new(origin); + let mut vm = Self::new(origin.clone()); while !message.0.is_empty() { let result = vm.execute(message); @@ -116,9 +115,9 @@ impl ExecuteXcm for XcmExecutor { vm.refund_surplus(); drop(vm.trader); - // TODO #2841: Do something with holding? (Fail-safe AssetTrap?) + let drop_weight = Config::AssetTrap::drop_assets(&origin, vm.holding); - let weight_used = xcm_weight.saturating_sub(vm.total_surplus); + let weight_used = xcm_weight.saturating_add(drop_weight).saturating_sub(vm.total_surplus); match vm.error { None => Outcome::Complete(weight_used), // TODO: #2841 #REALWEIGHT We should deduct the cost of any instructions following @@ -129,10 +128,10 @@ impl ExecuteXcm for XcmExecutor { } impl XcmExecutor { - fn new(origin: Option) -> Self { + fn new(origin: MultiLocation) -> Self { Self { holding: Assets::new(), - origin, + origin: Some(origin), trader: Config::Trader::new(), error: None, total_surplus: 0, diff --git a/xcm/xcm-executor/src/traits/mod.rs b/xcm/xcm-executor/src/traits/mod.rs index 0a304590b638..206b47f04452 100644 --- a/xcm/xcm-executor/src/traits/mod.rs +++ b/xcm/xcm-executor/src/traits/mod.rs @@ -18,6 +18,8 @@ mod conversion; pub use conversion::{Convert, ConvertOrigin, Decoded, Encoded, Identity, InvertLocation, JustTry}; +mod drop_assets; +pub use drop_assets::DropAssets; mod filter_asset_location; pub use filter_asset_location::FilterAssetLocation; mod matches_fungible; diff --git a/xcm/xcm-executor/src/traits/should_execute.rs b/xcm/xcm-executor/src/traits/should_execute.rs index 53600779fc54..08c74334517b 100644 --- a/xcm/xcm-executor/src/traits/should_execute.rs +++ b/xcm/xcm-executor/src/traits/should_execute.rs @@ -34,7 +34,7 @@ pub trait ShouldExecute { /// message may utilize in its execution. Typically non-zero only because of prior fee /// payment, but could in principle be due to other factors. fn should_execute( - origin: &Option, + origin: &MultiLocation, top_level: bool, message: &mut Xcm, max_weight: Weight, @@ -45,7 +45,7 @@ pub trait ShouldExecute { #[impl_trait_for_tuples::impl_for_tuples(30)] impl ShouldExecute for Tuple { fn should_execute( - origin: &Option, + origin: &MultiLocation, top_level: bool, message: &mut Xcm, max_weight: Weight, diff --git a/xcm/xcm-simulator/example/src/parachain.rs b/xcm/xcm-simulator/example/src/parachain.rs index 8f714a30cb52..1cffcbd86492 100644 --- a/xcm/xcm-simulator/example/src/parachain.rs +++ b/xcm/xcm-simulator/example/src/parachain.rs @@ -143,6 +143,7 @@ impl Config for XcmConfig { type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; type ResponseHandler = (); + type AssetTrap = (); } #[frame_support::pallet] diff --git a/xcm/xcm-simulator/example/src/relay_chain.rs b/xcm/xcm-simulator/example/src/relay_chain.rs index fa2218786d2c..9d10d417ad79 100644 --- a/xcm/xcm-simulator/example/src/relay_chain.rs +++ b/xcm/xcm-simulator/example/src/relay_chain.rs @@ -133,6 +133,7 @@ impl Config for XcmConfig { type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; type ResponseHandler = (); + type AssetTrap = (); } pub type LocalOriginToLocation = SignedToAccountId32; From 7863a9ce06097cf92bf1bc881e2ddaa898ebca10 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 26 Aug 2021 12:57:14 +0200 Subject: [PATCH 02/16] Revert reversions --- xcm/src/v2/mod.rs | 6 +++--- xcm/xcm-builder/src/tests.rs | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/xcm/src/v2/mod.rs b/xcm/src/v2/mod.rs index bddc28a7f8ae..42d0283652d8 100644 --- a/xcm/src/v2/mod.rs +++ b/xcm/src/v2/mod.rs @@ -226,11 +226,11 @@ pub enum Instruction { /// Errors: ReceiveTeleportedAsset(MultiAssets), - /// Indication of the contents of the holding register corresponding to the `QueryHolding` - /// order of `query_id`. + /// Respond with information that the local system is expecting. /// /// - `query_id`: The identifier of the query that resulted in this message being sent. - /// - `assets`: The message content. + /// - `response`: The message content. + /// - `max_weight`: The maximum weight that handling this response should take. /// /// Safety: No concerns. /// diff --git a/xcm/xcm-builder/src/tests.rs b/xcm/xcm-builder/src/tests.rs index 9319707b03df..22f72041c364 100644 --- a/xcm/xcm-builder/src/tests.rs +++ b/xcm/xcm-builder/src/tests.rs @@ -254,6 +254,31 @@ fn errors_should_return_unused_weight() { assert_eq!(sent_xcm(), vec![]); } +#[test] +fn weight_bounds_should_respect_instructions_limit() { + MaxInstructions::set(3); + let mut message = Xcm(vec![ClearOrigin; 4]); + // 4 instructions are too many. + assert_eq!(::Weigher::weight(&mut message), Err(())); + + let mut message = + Xcm(vec![SetErrorHandler(Xcm(vec![ClearOrigin])), SetAppendix(Xcm(vec![ClearOrigin]))]); + // 4 instructions are too many, even when hidden within 2. + assert_eq!(::Weigher::weight(&mut message), Err(())); + + let mut message = + Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm( + vec![ClearOrigin], + ))]))]))]); + // 4 instructions are too many, even when it's just one that's 3 levels deep. + assert_eq!(::Weigher::weight(&mut message), Err(())); + + let mut message = + Xcm(vec![SetErrorHandler(Xcm(vec![SetErrorHandler(Xcm(vec![ClearOrigin]))]))]); + // 3 instructions are OK. + assert_eq!(::Weigher::weight(&mut message), Ok(30)); +} + #[test] fn code_registers_should_work() { // we'll let them have message execution for free. From 1dd0cdd31f9089ccb465ec62570ee69811094665 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 26 Aug 2021 16:36:13 -0500 Subject: [PATCH 03/16] Remove attempts at weighing and add test --- xcm/pallet-xcm/src/lib.rs | 49 +++++++++----- xcm/pallet-xcm/src/mock.rs | 2 + xcm/pallet-xcm/src/tests.rs | 70 +++++++++++++++++++- xcm/src/v2/mod.rs | 64 ++++++++++++++++-- xcm/src/v2/traits.rs | 2 + xcm/xcm-builder/src/barriers.rs | 2 +- xcm/xcm-builder/src/mock.rs | 1 + xcm/xcm-executor/src/config.rs | 6 +- xcm/xcm-executor/src/lib.rs | 31 ++++++--- xcm/xcm-executor/src/traits/mod.rs | 2 +- xcm/xcm-simulator/example/src/parachain.rs | 1 + xcm/xcm-simulator/example/src/relay_chain.rs | 1 + 12 files changed, 200 insertions(+), 31 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index b8f3a34fa5f3..ff0193f3b7a8 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -51,7 +51,7 @@ pub mod pallet { }; use frame_system::{pallet_prelude::*, Config as SysConfig}; use sp_runtime::traits::{AccountIdConversion, BlockNumberProvider}; - use xcm_executor::{Assets, traits::{InvertLocation, OnResponse, WeightBounds, DropAssets}}; + use xcm_executor::{Assets, traits::{ClaimAssets, DropAssets, InvertLocation, OnResponse, WeightBounds}}; #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] @@ -226,21 +226,32 @@ pub mod pallet { /// Value of a query, must be unique for each query. pub type QueryId = u64; + /// Identifier for an asset trap. + pub type TrapId = u64; + /// The latest available query index. #[pallet::storage] pub(super) type QueryCount = StorageValue<_, QueryId, ValueQuery>; - + /// The ongoing queries. #[pallet::storage] #[pallet::getter(fn query)] pub(super) type Queries = - StorageMap<_, Blake2_128Concat, QueryId, QueryStatus, OptionQuery>; + StorageMap<_, Blake2_128Concat, QueryId, QueryStatus, OptionQuery>; - /// The latest available query index. + /// The latest available trap index. + #[pallet::storage] + pub(super) type TrapCount = StorageValue<_, TrapId, ValueQuery>; + + /// The existing asset traps - i.e. those assets which have been placed with us through our + /// `DropAsset` impl. + /// + /// There are two fields to the value; the origin which dropped the assets and the assets + /// themselves. #[pallet::storage] #[pallet::getter(fn asset_trap)] pub(super) type AssetTraps = - StorageMap<_, Blake2_128Concat, MultiLocation, Vec, ValueQuery>; + StorageMap<_, Blake2_128Concat, TrapId, (MultiLocation, VersionedMultiAssets), OptionQuery>; #[pallet::hooks] impl Hooks> for Pallet {} @@ -561,23 +572,31 @@ pub mod pallet { impl DropAssets for Pallet { fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { + if assets.is_empty() { return 0 } + let trap_id = TrapCount::::mutate(|c| { *c += 1; *c - 1 }); let versioned = VersionedMultiAssets::from(MultiAssets::from(assets)); - AssetTraps::::append(origin, versioned); + AssetTraps::::insert(trap_id, (origin, versioned)); + // TODO: Put the real weight in there. 0 } + } - fn collect_assets(origin: &MultiLocation, _max_weight: Weight) -> (Assets, Weight) { - // TODO: Check max_weight and figure out weight of this operation. - let versioned_assetss: Vec = AssetTraps::::take(origin); - let mut all_assets = Assets::new(); - for versioned_assets in versioned_assetss.into_iter() { - if let Ok(assets) = MultiAssets::try_from(versioned_assets) { - for asset in assets.drain().into_iter() { - all_assets.subsume(asset); + impl ClaimAssets for Pallet { + fn claim_assets(origin: &MultiLocation, id: &MultiLocation, assets: &MultiAssets) -> bool { + if let MultiLocation { parents: 0, interior: X1(GeneralIndex(trap_id)) } = id { + if *trap_id <= u64::max_value() as u128 { + let trap_id = *trap_id as u64; + if let Some((o, a)) = AssetTraps::::get(trap_id) { + if &o == origin && Some(assets) == MultiAssets::try_from(a).ok().as_ref() { + AssetTraps::::remove(trap_id); + // TODO: figure out weight of this operation (state map read and asset + // conversion). + return true + } } } } - (all_assets, 0) + false } } diff --git a/xcm/pallet-xcm/src/mock.rs b/xcm/pallet-xcm/src/mock.rs index 66c427b9ff3c..804a39646a16 100644 --- a/xcm/pallet-xcm/src/mock.rs +++ b/xcm/pallet-xcm/src/mock.rs @@ -251,6 +251,8 @@ impl xcm_executor::Config for XcmConfig { type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; type ResponseHandler = XcmPallet; + type AssetTrap = XcmPallet; + type AssetClaims = XcmPallet; } pub type LocalOriginToLocation = SignedToAccountId32; diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs index 540ee153a64e..444fc5fa3afb 100644 --- a/xcm/pallet-xcm/src/tests.rs +++ b/xcm/pallet-xcm/src/tests.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use crate::{mock::*, QueryStatus}; +use crate::{mock::*, QueryStatus, AssetTraps}; use frame_support::{assert_noop, assert_ok, traits::Currency}; use polkadot_parachain::primitives::{AccountIdConversion, Id as ParaId}; use std::convert::TryInto; @@ -295,3 +295,71 @@ fn execute_withdraw_to_deposit_works() { ); }); } + +/// Test drop/claim assets. +#[test] +fn trapped_assets_can_be_claimed() { + let balances = + vec![(ALICE, INITIAL_BALANCE), (BOB, INITIAL_BALANCE)]; + new_test_ext_with_balances(balances).execute_with(|| { + let weight = 6 * BaseXcmWeight::get(); + let dest: MultiLocation = + Junction::AccountId32 { network: NetworkId::Any, id: BOB.into() }.into(); + + assert_ok!(XcmPallet::execute( + Origin::signed(ALICE), + Box::new(VersionedXcm::from(Xcm(vec![ + WithdrawAsset((Here, SEND_AMOUNT).into()), + buy_execution((Here, SEND_AMOUNT)), + // Don't propagated the error into the result. + SetErrorHandler(Xcm(vec![ClearError])), + // This will make an error. + Trap(0), + // This would succeed, but we never get to it. + DepositAsset { assets: All.into(), max_assets: 1, beneficiary: dest.clone() }, + ]))), + weight + )); + + assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE - SEND_AMOUNT); + assert_eq!(Balances::total_balance(&BOB), INITIAL_BALANCE); + + let source: MultiLocation = + Junction::AccountId32 { network: NetworkId::Any, id: ALICE.into() }.into(); + let trapped = AssetTraps::::iter().collect::>(); + let expected = vec![(0u64, (source, MultiAssets::from((Here, SEND_AMOUNT)).into()))]; + assert_eq!(trapped, expected); + + let weight = 3 * BaseXcmWeight::get(); + assert_ok!(XcmPallet::execute( + Origin::signed(ALICE), + Box::new(VersionedXcm::from(Xcm(vec![ + ClaimAsset { assets: (Here, SEND_AMOUNT).into(), ticket: GeneralIndex(0).into() }, + buy_execution((Here, SEND_AMOUNT)), + DepositAsset { assets: All.into(), max_assets: 1, beneficiary: dest.clone() }, + ]))), + weight + )); + + assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE - SEND_AMOUNT); + assert_eq!(Balances::total_balance(&BOB), INITIAL_BALANCE + SEND_AMOUNT); + assert_eq!(AssetTraps::::iter().collect::>(), vec![]); + + let weight = 3 * BaseXcmWeight::get(); + assert_ok!(XcmPallet::execute( + Origin::signed(ALICE), + Box::new(VersionedXcm::from(Xcm(vec![ + ClaimAsset { assets: (Here, SEND_AMOUNT).into(), ticket: GeneralIndex(0).into() }, + buy_execution((Here, SEND_AMOUNT)), + DepositAsset { assets: All.into(), max_assets: 1, beneficiary: dest }, + ]))), + weight + )); + assert_eq!( + last_event(), + Event::XcmPallet(crate::Event::Attempted( + Outcome::Incomplete(BaseXcmWeight::get(), XcmError::AssetNotFound) + )) + ); + }); +} diff --git a/xcm/src/v2/mod.rs b/xcm/src/v2/mod.rs index 42d0283652d8..68660e2c4ab0 100644 --- a/xcm/src/v2/mod.rs +++ b/xcm/src/v2/mod.rs @@ -37,6 +37,9 @@ pub use super::v1::{ MultiLocation, NetworkId, OriginKind, Parent, ParentThen, WildFungibility, WildMultiAsset, }; +/// Identifier for assets that were trapped. +pub type TrapId = u64; + #[derive(Derivative, Default, Encode, Decode)] #[derivative(Clone(bound = ""), Eq(bound = ""), PartialEq(bound = ""), Debug(bound = ""))] #[codec(encode_bound())] @@ -392,6 +395,8 @@ pub enum Instruction { /// prioritized under standard asset ordering. Any others will remain in holding. /// - `beneficiary`: The new owner for the assets. /// + /// Kind: *Instruction* + /// /// Errors: DepositAsset { assets: MultiAssetFilter, max_assets: u32, beneficiary: MultiLocation }, @@ -411,6 +416,8 @@ pub enum Instruction { /// - `xcm`: The orders that should follow the `ReserveAssetDeposited` instruction /// which is sent onwards to `dest`. /// + /// Kind: *Instruction* + /// /// Errors: DepositReserveAsset { assets: MultiAssetFilter, @@ -428,6 +435,8 @@ pub enum Instruction { /// - `give`: The asset(s) to remove from holding. /// - `receive`: The minimum amount of assets(s) which `give` should be exchanged for. /// + /// Kind: *Instruction* + /// /// Errors: ExchangeAsset { give: MultiAssetFilter, receive: MultiAssets }, @@ -442,6 +451,8 @@ pub enum Instruction { /// - `xcm`: The instructions to execute on the assets once withdrawn *on the reserve /// location*. /// + /// Kind: *Instruction* + /// /// Errors: InitiateReserveWithdraw { assets: MultiAssetFilter, reserve: MultiLocation, xcm: Xcm<()> }, @@ -456,6 +467,8 @@ pub enum Instruction { /// NOTE: The `dest` location *MUST* respect this origin as a valid teleportation origin for all /// `assets`. If it does not, then the assets may be lost. /// + /// Kind: *Instruction* + /// /// Errors: InitiateTeleport { assets: MultiAssetFilter, dest: MultiLocation, xcm: Xcm<()> }, @@ -472,6 +485,8 @@ pub enum Instruction { /// is sent as a reply may take to execute. NOTE: If this is unexpectedly large then the /// response may not execute at all. /// + /// Kind: *Instruction* + /// /// Errors: QueryHolding { #[codec(compact)] @@ -490,13 +505,20 @@ pub enum Instruction { /// expected maximum weight of the total XCM to be executed for the /// `AllowTopLevelPaidExecutionFrom` barrier to allow the XCM be executed. /// + /// Kind: *Instruction* + /// /// Errors: BuyExecution { fees: MultiAsset, weight_limit: WeightLimit }, /// Refund any surplus weight previously bought with `BuyExecution`. + /// + /// Kind: *Instruction* + /// + /// Errors: None. RefundSurplus, - /// Set code that should be called in the case of an error happening. + /// Set the Error Handler Register. This is code that should be called in the case of an error + /// happening. /// /// An error occurring within execution of this code will _NOT_ result in the error register /// being set, nor will an error handler be called due to it. The error handler and appendix @@ -505,10 +527,15 @@ pub enum Instruction { /// The apparent weight of this instruction is inclusive of the inner `Xcm`; the executing /// weight however includes only the difference between the previous handler and the new /// handler, which can reasonably be negative, which would result in a surplus. + /// + /// Kind: *Instruction* + /// + /// Errors: None. SetErrorHandler(Xcm), - /// Set code that should be called after code execution (including the error handler if any) - /// is finished. This will be called regardless of whether an error occurred. + /// Set the Appendix Register. This is code that should be called after code execution + /// (including the error handler if any) is finished. This will be called regardless of whether + /// an error occurred. /// /// Any error occurring due to execution of this code will result in the error register being /// set, and the error handler (if set) firing. @@ -516,10 +543,37 @@ pub enum Instruction { /// The apparent weight of this instruction is inclusive of the inner `Xcm`; the executing /// weight however includes only the difference between the previous appendix and the new /// appendix, which can reasonably be negative, which would result in a surplus. + /// + /// Kind: *Instruction* + /// + /// Errors: None. SetAppendix(Xcm), - /// Clear the error register. + /// Clear the Error Register. + /// + /// Kind: *Instruction* + /// + /// Errors: None. ClearError, + + /// Create some assets which are being held on behalf of the origin. + /// + /// - `ticket`: An identifier for the assets to be claimed. + /// - `assets`: The assets which are to be claimed. This must match exactly with the assets + /// claimable by the origin of the ticket. + /// + /// Kind: *Instruction* + /// + /// Errors: + ClaimAsset { assets: MultiAssets, ticket: MultiLocation }, + + /// Always throws an error of type `Trap`. + /// + /// Kind: *Instruction* + /// + /// Errors: + /// - `Trap`: All circumstances, whose inner value is the same as this item's inner value. + Trap(u64), } impl Xcm { @@ -572,6 +626,8 @@ impl Instruction { SetErrorHandler(xcm) => SetErrorHandler(xcm.into()), SetAppendix(xcm) => SetAppendix(xcm.into()), ClearError => ClearError, + ClaimAsset { assets, ticket } => ClaimAsset { assets, ticket }, + Trap(code) => Trap(code), } } } diff --git a/xcm/src/v2/traits.rs b/xcm/src/v2/traits.rs index facbdd089263..1cf8e4f29f88 100644 --- a/xcm/src/v2/traits.rs +++ b/xcm/src/v2/traits.rs @@ -94,6 +94,8 @@ pub enum Error { Unroutable, /// The weight required was not specified when it should have been. UnknownWeightRequired, + /// An error was intentionally forced. A code is included. + Trap(u64), } impl From<()> for Error { diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index c9a0fa30598e..dceb0f76bb19 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -60,7 +60,7 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFro let mut iter = message.0.iter_mut(); let i = iter.next().ok_or(())?; match i { - ReceiveTeleportedAsset(..) | WithdrawAsset(..) | ReserveAssetDeposited(..) => (), + ReceiveTeleportedAsset(..) | WithdrawAsset(..) | ReserveAssetDeposited(..) | ClaimAsset{..} => (), _ => return Err(()), } let mut i = iter.next().ok_or(())?; diff --git a/xcm/xcm-builder/src/mock.rs b/xcm/xcm-builder/src/mock.rs index b32082cccd5e..0661b179e026 100644 --- a/xcm/xcm-builder/src/mock.rs +++ b/xcm/xcm-builder/src/mock.rs @@ -283,4 +283,5 @@ impl Config for TestConfig { type Trader = FixedRateOfFungible; type ResponseHandler = TestResponseHandler; type AssetTrap = (); // TODO: TestAssetTrap + type AssetClaims = (); // TODO: TestAssetClaims } diff --git a/xcm/xcm-executor/src/config.rs b/xcm/xcm-executor/src/config.rs index 68027b0e5388..7eaf254a3028 100644 --- a/xcm/xcm-executor/src/config.rs +++ b/xcm/xcm-executor/src/config.rs @@ -16,7 +16,7 @@ use crate::traits::{ ConvertOrigin, FilterAssetLocation, InvertLocation, OnResponse, ShouldExecute, TransactAsset, - WeightBounds, WeightTrader, DropAssets, + WeightBounds, WeightTrader, DropAssets, ClaimAssets, }; use frame_support::{ dispatch::{Dispatchable, Parameter}, @@ -62,4 +62,8 @@ pub trait Config { /// The general asset trap - handler for when assets are left in the Holding Register at the /// end of execution. type AssetTrap: DropAssets; + + /// The general asset trap - handler for when assets are left in the Holding Register at the + /// end of execution. + type AssetClaims: ClaimAssets; } diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 5d2e059ec1b1..fccfeaef6e46 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -32,7 +32,7 @@ use xcm::latest::{ pub mod traits; use traits::{ ConvertOrigin, FilterAssetLocation, InvertLocation, OnResponse, ShouldExecute, TransactAsset, - WeightBounds, WeightTrader, DropAssets, + WeightBounds, WeightTrader, DropAssets, ClaimAssets, }; mod assets; @@ -115,9 +115,12 @@ impl ExecuteXcm for XcmExecutor { vm.refund_surplus(); drop(vm.trader); - let drop_weight = Config::AssetTrap::drop_assets(&origin, vm.holding); + let mut weight_used = xcm_weight.saturating_sub(vm.total_surplus); + + if !vm.holding.is_empty() { + weight_used.saturating_accrue(Config::AssetTrap::drop_assets(&origin, vm.holding)); + }; - let weight_used = xcm_weight.saturating_add(drop_weight).saturating_sub(vm.total_surplus); match vm.error { None => Outcome::Complete(weight_used), // TODO: #2841 #REALWEIGHT We should deduct the cost of any instructions following @@ -182,7 +185,7 @@ impl XcmExecutor { /// Drop the registered error handler and refund its weight. fn drop_error_handler(&mut self) { self.error_handler = Xcm::(vec![]); - self.total_surplus = self.total_surplus.saturating_add(self.error_handler_weight); + self.total_surplus.saturating_accrue(self.error_handler_weight); self.error_handler_weight = 0; } @@ -198,7 +201,7 @@ impl XcmExecutor { fn refund_surplus(&mut self) { let current_surplus = self.total_surplus.saturating_sub(self.total_refunded); if current_surplus > 0 { - self.total_refunded = self.total_refunded.saturating_add(current_surplus); + self.total_refunded.saturating_accrue(current_surplus); if let Some(w) = self.trader.refund_weight(current_surplus) { self.holding.subsume(w); } @@ -299,7 +302,7 @@ impl XcmExecutor { // reported back to the caller and this ensures that they account for the total // weight consumed correctly (potentially allowing them to do more operations in a // block than they otherwise would). - self.total_surplus = self.total_surplus.saturating_add(surplus); + self.total_surplus.saturating_accrue(surplus); Ok(()) }, QueryResponse { query_id, response, max_weight } => { @@ -389,14 +392,14 @@ impl XcmExecutor { }, SetErrorHandler(mut handler) => { let handler_weight = Config::Weigher::weight(&mut handler)?; - self.total_surplus = self.total_surplus.saturating_add(self.error_handler_weight); + self.total_surplus.saturating_accrue(self.error_handler_weight); self.error_handler = handler; self.error_handler_weight = handler_weight; Ok(()) }, SetAppendix(mut appendix) => { let appendix_weight = Config::Weigher::weight(&mut appendix)?; - self.total_surplus = self.total_surplus.saturating_add(self.appendix_weight); + self.total_surplus.saturating_accrue(self.appendix_weight); self.appendix = appendix; self.appendix_weight = appendix_weight; Ok(()) @@ -405,6 +408,18 @@ impl XcmExecutor { self.error = None; Ok(()) }, + ClaimAsset { assets, ticket } => { + let origin = self.origin.as_ref().ok_or(XcmError::BadOrigin)?; + let ok = Config::AssetClaims::claim_assets(origin, &ticket, &assets); + ensure!(ok, XcmError::AssetNotFound); + for asset in assets.drain().into_iter() { + self.holding.subsume(asset); + } + Ok(()) + } + Trap(code) => { + Err(XcmError::Trap(code)) + } ExchangeAsset { .. } => Err(XcmError::Unimplemented), HrmpNewChannelOpenRequest { .. } => Err(XcmError::Unimplemented), HrmpChannelAccepted { .. } => Err(XcmError::Unimplemented), diff --git a/xcm/xcm-executor/src/traits/mod.rs b/xcm/xcm-executor/src/traits/mod.rs index 206b47f04452..687a16a38a04 100644 --- a/xcm/xcm-executor/src/traits/mod.rs +++ b/xcm/xcm-executor/src/traits/mod.rs @@ -19,7 +19,7 @@ mod conversion; pub use conversion::{Convert, ConvertOrigin, Decoded, Encoded, Identity, InvertLocation, JustTry}; mod drop_assets; -pub use drop_assets::DropAssets; +pub use drop_assets::{DropAssets, ClaimAssets}; mod filter_asset_location; pub use filter_asset_location::FilterAssetLocation; mod matches_fungible; diff --git a/xcm/xcm-simulator/example/src/parachain.rs b/xcm/xcm-simulator/example/src/parachain.rs index 1cffcbd86492..faa2d8b3fc0a 100644 --- a/xcm/xcm-simulator/example/src/parachain.rs +++ b/xcm/xcm-simulator/example/src/parachain.rs @@ -144,6 +144,7 @@ impl Config for XcmConfig { type Trader = FixedRateOfFungible; type ResponseHandler = (); type AssetTrap = (); + type AssetClaims = (); } #[frame_support::pallet] diff --git a/xcm/xcm-simulator/example/src/relay_chain.rs b/xcm/xcm-simulator/example/src/relay_chain.rs index 9d10d417ad79..f3173b25e075 100644 --- a/xcm/xcm-simulator/example/src/relay_chain.rs +++ b/xcm/xcm-simulator/example/src/relay_chain.rs @@ -134,6 +134,7 @@ impl Config for XcmConfig { type Trader = FixedRateOfFungible; type ResponseHandler = (); type AssetTrap = (); + type AssetClaims = (); } pub type LocalOriginToLocation = SignedToAccountId32; From 9617be068a7c443733843658bfc9bf4a2758a3e1 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 26 Aug 2021 18:55:12 -0500 Subject: [PATCH 04/16] Less storage use for asset trapping --- xcm/pallet-xcm/Cargo.toml | 3 +- xcm/pallet-xcm/src/lib.rs | 56 +++++++++++++++++++------------------ xcm/pallet-xcm/src/tests.rs | 29 +++++++++++++------ xcm/src/lib.rs | 10 +++++++ xcm/src/v2/mod.rs | 6 ++-- xcm/src/v2/traits.rs | 2 ++ xcm/xcm-executor/src/lib.rs | 2 +- 7 files changed, 66 insertions(+), 42 deletions(-) diff --git a/xcm/pallet-xcm/Cargo.toml b/xcm/pallet-xcm/Cargo.toml index 821608e62e72..49f3de61b182 100644 --- a/xcm/pallet-xcm/Cargo.toml +++ b/xcm/pallet-xcm/Cargo.toml @@ -11,6 +11,7 @@ log = { version = "0.4.14", default-features = false } sp-std = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } +sp-core = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } frame-support = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } frame-system = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } @@ -20,7 +21,6 @@ xcm-executor = { path = "../xcm-executor", default-features = false } [dev-dependencies] pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "master" } polkadot-runtime-parachains = { path = "../../runtime/parachains" } -sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" } sp-io = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false } xcm-builder = { path = "../xcm-builder" } polkadot-parachain = { path = "../../parachain" } @@ -31,6 +31,7 @@ std = [ "codec/std", "serde", "sp-std/std", + "sp-core/std", "sp-runtime/std", "frame-support/std", "frame-system/std", diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index ff0193f3b7a8..c1437ba9e5b2 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -50,7 +50,8 @@ pub mod pallet { pallet_prelude::*, }; use frame_system::{pallet_prelude::*, Config as SysConfig}; - use sp_runtime::traits::{AccountIdConversion, BlockNumberProvider}; + use sp_core::H256; + use sp_runtime::traits::{AccountIdConversion, BlakeTwo256, BlockNumberProvider, Hash}; use xcm_executor::{Assets, traits::{ClaimAssets, DropAssets, InvertLocation, OnResponse, WeightBounds}}; #[pallet::pallet] @@ -170,6 +171,10 @@ pub mod pallet { /// /// \[ id \] ResponseTaken(QueryId), + /// Some assets have been placed in an asset trap. + /// + /// \[ hash, origin, assets \] + AssetsTrapped(H256, MultiLocation, VersionedMultiAssets), } #[pallet::origin] @@ -239,19 +244,13 @@ pub mod pallet { pub(super) type Queries = StorageMap<_, Blake2_128Concat, QueryId, QueryStatus, OptionQuery>; - /// The latest available trap index. - #[pallet::storage] - pub(super) type TrapCount = StorageValue<_, TrapId, ValueQuery>; - - /// The existing asset traps - i.e. those assets which have been placed with us through our - /// `DropAsset` impl. + /// The existing asset traps. /// - /// There are two fields to the value; the origin which dropped the assets and the assets - /// themselves. + /// Key is the blake2 256 hash of (origin, versioned multiassets) pair. Value is the number of + /// times this pair has been trapped (usually just 1 if it exists at all). #[pallet::storage] #[pallet::getter(fn asset_trap)] - pub(super) type AssetTraps = - StorageMap<_, Blake2_128Concat, TrapId, (MultiLocation, VersionedMultiAssets), OptionQuery>; + pub(super) type AssetTraps = StorageMap<_, Identity, H256, u32, ValueQuery>; #[pallet::hooks] impl Hooks> for Pallet {} @@ -573,30 +572,33 @@ pub mod pallet { impl DropAssets for Pallet { fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { if assets.is_empty() { return 0 } - let trap_id = TrapCount::::mutate(|c| { *c += 1; *c - 1 }); let versioned = VersionedMultiAssets::from(MultiAssets::from(assets)); - AssetTraps::::insert(trap_id, (origin, versioned)); + let hash = BlakeTwo256::hash_of(&(&origin, &versioned)); + AssetTraps::::mutate(hash, |n| *n += 1); + Self::deposit_event(Event::AssetsTrapped(hash, origin.clone(), versioned)); // TODO: Put the real weight in there. 0 } } impl ClaimAssets for Pallet { - fn claim_assets(origin: &MultiLocation, id: &MultiLocation, assets: &MultiAssets) -> bool { - if let MultiLocation { parents: 0, interior: X1(GeneralIndex(trap_id)) } = id { - if *trap_id <= u64::max_value() as u128 { - let trap_id = *trap_id as u64; - if let Some((o, a)) = AssetTraps::::get(trap_id) { - if &o == origin && Some(assets) == MultiAssets::try_from(a).ok().as_ref() { - AssetTraps::::remove(trap_id); - // TODO: figure out weight of this operation (state map read and asset - // conversion). - return true - } - } - } + fn claim_assets(origin: &MultiLocation, ticket: &MultiLocation, assets: &MultiAssets) -> bool { + let mut versioned = VersionedMultiAssets::from(assets.clone()); + match (ticket.parents, &ticket.interior) { + (0, X1(GeneralIndex(i))) => versioned = match versioned.into_version(*i as u32) { + Ok(v) => v, + Err(()) => return false, + }, + (0, Here) => (), + _ => return false, + }; + let hash = BlakeTwo256::hash_of(&(origin, versioned)); + match AssetTraps::::get(hash) { + 0 => return false, + 1 => AssetTraps::::remove(hash), + n => AssetTraps::::insert(hash, n - 1), } - false + return true } } diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs index 444fc5fa3afb..9b087320cf73 100644 --- a/xcm/pallet-xcm/src/tests.rs +++ b/xcm/pallet-xcm/src/tests.rs @@ -17,8 +17,9 @@ use crate::{mock::*, QueryStatus, AssetTraps}; use frame_support::{assert_noop, assert_ok, traits::Currency}; use polkadot_parachain::primitives::{AccountIdConversion, Id as ParaId}; +use sp_runtime::traits::{BlakeTwo256, Hash}; use std::convert::TryInto; -use xcm::{latest::prelude::*, VersionedXcm}; +use xcm::{VersionedMultiAssets, VersionedXcm, latest::prelude::*}; use xcm_executor::XcmExecutor; const ALICE: AccountId = AccountId::new([0u8; 32]); @@ -320,21 +321,31 @@ fn trapped_assets_can_be_claimed() { ]))), weight )); - - assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE - SEND_AMOUNT); - assert_eq!(Balances::total_balance(&BOB), INITIAL_BALANCE); - let source: MultiLocation = Junction::AccountId32 { network: NetworkId::Any, id: ALICE.into() }.into(); let trapped = AssetTraps::::iter().collect::>(); - let expected = vec![(0u64, (source, MultiAssets::from((Here, SEND_AMOUNT)).into()))]; + let vma = VersionedMultiAssets::from(MultiAssets::from((Here, SEND_AMOUNT))); + let hash = BlakeTwo256::hash_of(&(source.clone(), vma.clone())); + assert_eq!( + last_events(2), + vec![ + Event::XcmPallet(crate::Event::AssetsTrapped(hash.clone(), source, vma)), + Event::XcmPallet(crate::Event::Attempted( + Outcome::Complete(5 * BaseXcmWeight::get()) + )) + ] + ); + assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE - SEND_AMOUNT); + assert_eq!(Balances::total_balance(&BOB), INITIAL_BALANCE); + + let expected = vec![(hash, 1u32)]; assert_eq!(trapped, expected); let weight = 3 * BaseXcmWeight::get(); assert_ok!(XcmPallet::execute( Origin::signed(ALICE), Box::new(VersionedXcm::from(Xcm(vec![ - ClaimAsset { assets: (Here, SEND_AMOUNT).into(), ticket: GeneralIndex(0).into() }, + ClaimAsset { assets: (Here, SEND_AMOUNT).into(), ticket: Here.into() }, buy_execution((Here, SEND_AMOUNT)), DepositAsset { assets: All.into(), max_assets: 1, beneficiary: dest.clone() }, ]))), @@ -349,7 +360,7 @@ fn trapped_assets_can_be_claimed() { assert_ok!(XcmPallet::execute( Origin::signed(ALICE), Box::new(VersionedXcm::from(Xcm(vec![ - ClaimAsset { assets: (Here, SEND_AMOUNT).into(), ticket: GeneralIndex(0).into() }, + ClaimAsset { assets: (Here, SEND_AMOUNT).into(), ticket: Here.into() }, buy_execution((Here, SEND_AMOUNT)), DepositAsset { assets: All.into(), max_assets: 1, beneficiary: dest }, ]))), @@ -358,7 +369,7 @@ fn trapped_assets_can_be_claimed() { assert_eq!( last_event(), Event::XcmPallet(crate::Event::Attempted( - Outcome::Incomplete(BaseXcmWeight::get(), XcmError::AssetNotFound) + Outcome::Incomplete(BaseXcmWeight::get(), XcmError::UnknownClaim) )) ); }); diff --git a/xcm/src/lib.rs b/xcm/src/lib.rs index af53b7bddcf2..33404935a6cf 100644 --- a/xcm/src/lib.rs +++ b/xcm/src/lib.rs @@ -217,6 +217,16 @@ pub enum VersionedMultiAssets { V1(v1::MultiAssets), } +impl VersionedMultiAssets { + pub fn into_version(self, n: u32) -> Result { + Ok(match n { + 0 => Self::V0(self.try_into()?), + 1 | 2 => Self::V1(self.try_into()?), + _ => return Err(()) + }) + } +} + impl From> for VersionedMultiAssets { fn from(x: Vec) -> Self { VersionedMultiAssets::V0(x) diff --git a/xcm/src/v2/mod.rs b/xcm/src/v2/mod.rs index 68660e2c4ab0..a6d75930cb63 100644 --- a/xcm/src/v2/mod.rs +++ b/xcm/src/v2/mod.rs @@ -37,9 +37,6 @@ pub use super::v1::{ MultiLocation, NetworkId, OriginKind, Parent, ParentThen, WildFungibility, WildMultiAsset, }; -/// Identifier for assets that were trapped. -pub type TrapId = u64; - #[derive(Derivative, Default, Encode, Decode)] #[derivative(Clone(bound = ""), Eq(bound = ""), PartialEq(bound = ""), Debug(bound = ""))] #[codec(encode_bound())] @@ -558,9 +555,10 @@ pub enum Instruction { /// Create some assets which are being held on behalf of the origin. /// - /// - `ticket`: An identifier for the assets to be claimed. /// - `assets`: The assets which are to be claimed. This must match exactly with the assets /// claimable by the origin of the ticket. + /// - `ticket`: The ticket of the asset; this is an abstract identifier to help locate the + /// asset. /// /// Kind: *Instruction* /// diff --git a/xcm/src/v2/traits.rs b/xcm/src/v2/traits.rs index 1cf8e4f29f88..c9f84451ecfe 100644 --- a/xcm/src/v2/traits.rs +++ b/xcm/src/v2/traits.rs @@ -96,6 +96,8 @@ pub enum Error { UnknownWeightRequired, /// An error was intentionally forced. A code is included. Trap(u64), + /// The given claim could not be recognised/found. + UnknownClaim, } impl From<()> for Error { diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index fccfeaef6e46..7c9bfe05f86f 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -411,7 +411,7 @@ impl XcmExecutor { ClaimAsset { assets, ticket } => { let origin = self.origin.as_ref().ok_or(XcmError::BadOrigin)?; let ok = Config::AssetClaims::claim_assets(origin, &ticket, &assets); - ensure!(ok, XcmError::AssetNotFound); + ensure!(ok, XcmError::UnknownClaim); for asset in assets.drain().into_iter() { self.holding.subsume(asset); } From b75f495e10edbf6757996a274dcb15d10192d3a5 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 08:22:16 -0500 Subject: [PATCH 05/16] Add missing file --- xcm/xcm-executor/src/traits/drop_assets.rs | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 xcm/xcm-executor/src/traits/drop_assets.rs diff --git a/xcm/xcm-executor/src/traits/drop_assets.rs b/xcm/xcm-executor/src/traits/drop_assets.rs new file mode 100644 index 000000000000..6bec530e2419 --- /dev/null +++ b/xcm/xcm-executor/src/traits/drop_assets.rs @@ -0,0 +1,55 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Polkadot is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Polkadot. If not, see . + +use crate::Assets; +use frame_support::weights::Weight; +use xcm::latest::{MultiAssets, MultiLocation}; + +/// Define a handler for when some non-empty `Assets` value should be dropped. +pub trait DropAssets { + /// Handler for receiving dropped assets. Returns the weight consumed by this operation. + fn drop_assets( + origin: &MultiLocation, + assets: Assets, + ) -> Weight; +} +impl DropAssets for () { + fn drop_assets( + _origin: &MultiLocation, + _assets: Assets, + ) -> Weight { + 0 + } +} + +/// Define any handlers for the `AssetClaim` instruction. +pub trait ClaimAssets { + /// Claim any assets available to `origin` and return them in a single `Assets` value, together + /// with the weight used by this operation. + fn claim_assets(origin: &MultiLocation, ticket: &MultiLocation, what: &MultiAssets) -> bool; +} + +#[impl_trait_for_tuples::impl_for_tuples(30)] +impl ClaimAssets for Tuple { + fn claim_assets(origin: &MultiLocation, ticket: &MultiLocation, what: &MultiAssets) -> bool { + for_tuples!( #( + if Tuple::claim_assets(origin, ticket, what) { + return true; + } + )* ); + false + } +} From 7c3355f4b6c6d897d0a299966416178980f6a25d Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 15:14:42 -0500 Subject: [PATCH 06/16] Fixes --- xcm/xcm-builder/src/tests.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/xcm/xcm-builder/src/tests.rs b/xcm/xcm-builder/src/tests.rs index 22f72041c364..869595a90a22 100644 --- a/xcm/xcm-builder/src/tests.rs +++ b/xcm/xcm-builder/src/tests.rs @@ -58,7 +58,7 @@ fn take_weight_credit_barrier_should_work() { Xcm::<()>(vec![TransferAsset { assets: (Parent, 100).into(), beneficiary: Here.into() }]); let mut weight_credit = 10; let r = TakeWeightCredit::should_execute( - &Some(Parent.into()), + &Parent.into(), true, &mut message, 10, @@ -68,7 +68,7 @@ fn take_weight_credit_barrier_should_work() { assert_eq!(weight_credit, 0); let r = TakeWeightCredit::should_execute( - &Some(Parent.into()), + &Parent.into(), true, &mut message, 10, @@ -86,7 +86,7 @@ fn allow_unpaid_should_work() { AllowUnpaidFrom::set(vec![Parent.into()]); let r = AllowUnpaidExecutionFrom::>::should_execute( - &Some(Parachain(1).into()), + &Parachain(1).into(), true, &mut message, 10, @@ -95,7 +95,7 @@ fn allow_unpaid_should_work() { assert_eq!(r, Err(())); let r = AllowUnpaidExecutionFrom::>::should_execute( - &Some(Parent.into()), + &Parent.into(), true, &mut message, 10, @@ -112,7 +112,7 @@ fn allow_paid_should_work() { Xcm::<()>(vec![TransferAsset { assets: (Parent, 100).into(), beneficiary: Here.into() }]); let r = AllowTopLevelPaidExecutionFrom::>::should_execute( - &Some(Parachain(1).into()), + &Parachain(1).into(), true, &mut message, 10, @@ -128,7 +128,7 @@ fn allow_paid_should_work() { ]); let r = AllowTopLevelPaidExecutionFrom::>::should_execute( - &Some(Parent.into()), + &Parent.into(), true, &mut underpaying_message, 30, @@ -144,7 +144,7 @@ fn allow_paid_should_work() { ]); let r = AllowTopLevelPaidExecutionFrom::>::should_execute( - &Some(Parachain(1).into()), + &Parachain(1).into(), true, &mut paying_message, 30, @@ -153,7 +153,7 @@ fn allow_paid_should_work() { assert_eq!(r, Err(())); let r = AllowTopLevelPaidExecutionFrom::>::should_execute( - &Some(Parent.into()), + &Parent.into(), true, &mut paying_message, 30, From 8e25edc0f55dd40fc2a92339dbe3c8ea773b2453 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 15:26:04 -0500 Subject: [PATCH 07/16] Fixes --- xcm/xcm-builder/tests/mock/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/xcm/xcm-builder/tests/mock/mod.rs b/xcm/xcm-builder/tests/mock/mod.rs index ed64ce9470da..6c3285d838b9 100644 --- a/xcm/xcm-builder/tests/mock/mod.rs +++ b/xcm/xcm-builder/tests/mock/mod.rs @@ -167,6 +167,8 @@ impl xcm_executor::Config for XcmConfig { type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; type ResponseHandler = (); + type AssetTrap = XcmPallet; + type AssetClaims = XcmPallet; } pub type LocalOriginToLocation = SignedToAccountId32; From 42e7ab344239fc4b03da8b64c9e5e52e9cf25c56 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 15:27:14 -0500 Subject: [PATCH 08/16] Formatting --- xcm/pallet-xcm/src/lib.rs | 32 ++++++++++++++-------- xcm/pallet-xcm/src/tests.rs | 20 +++++++------- xcm/src/lib.rs | 2 +- xcm/src/v2/mod.rs | 10 +++---- xcm/xcm-builder/src/barriers.rs | 5 +++- xcm/xcm-builder/src/mock.rs | 4 +-- xcm/xcm-executor/src/config.rs | 4 +-- xcm/xcm-executor/src/lib.rs | 10 +++---- xcm/xcm-executor/src/traits/drop_assets.rs | 10 ++----- xcm/xcm-executor/src/traits/mod.rs | 2 +- 10 files changed, 52 insertions(+), 47 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index c1437ba9e5b2..c75b9984828d 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -52,7 +52,10 @@ pub mod pallet { use frame_system::{pallet_prelude::*, Config as SysConfig}; use sp_core::H256; use sp_runtime::traits::{AccountIdConversion, BlakeTwo256, BlockNumberProvider, Hash}; - use xcm_executor::{Assets, traits::{ClaimAssets, DropAssets, InvertLocation, OnResponse, WeightBounds}}; + use xcm_executor::{ + traits::{ClaimAssets, DropAssets, InvertLocation, OnResponse, WeightBounds}, + Assets, + }; #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] @@ -172,7 +175,7 @@ pub mod pallet { /// \[ id \] ResponseTaken(QueryId), /// Some assets have been placed in an asset trap. - /// + /// /// \[ hash, origin, assets \] AssetsTrapped(H256, MultiLocation, VersionedMultiAssets), } @@ -237,15 +240,15 @@ pub mod pallet { /// The latest available query index. #[pallet::storage] pub(super) type QueryCount = StorageValue<_, QueryId, ValueQuery>; - + /// The ongoing queries. #[pallet::storage] #[pallet::getter(fn query)] pub(super) type Queries = - StorageMap<_, Blake2_128Concat, QueryId, QueryStatus, OptionQuery>; + StorageMap<_, Blake2_128Concat, QueryId, QueryStatus, OptionQuery>; /// The existing asset traps. - /// + /// /// Key is the blake2 256 hash of (origin, versioned multiassets) pair. Value is the number of /// times this pair has been trapped (usually just 1 if it exists at all). #[pallet::storage] @@ -571,7 +574,9 @@ pub mod pallet { impl DropAssets for Pallet { fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { - if assets.is_empty() { return 0 } + if assets.is_empty() { + return 0 + } let versioned = VersionedMultiAssets::from(MultiAssets::from(assets)); let hash = BlakeTwo256::hash_of(&(&origin, &versioned)); AssetTraps::::mutate(hash, |n| *n += 1); @@ -582,13 +587,18 @@ pub mod pallet { } impl ClaimAssets for Pallet { - fn claim_assets(origin: &MultiLocation, ticket: &MultiLocation, assets: &MultiAssets) -> bool { + fn claim_assets( + origin: &MultiLocation, + ticket: &MultiLocation, + assets: &MultiAssets, + ) -> bool { let mut versioned = VersionedMultiAssets::from(assets.clone()); match (ticket.parents, &ticket.interior) { - (0, X1(GeneralIndex(i))) => versioned = match versioned.into_version(*i as u32) { - Ok(v) => v, - Err(()) => return false, - }, + (0, X1(GeneralIndex(i))) => + versioned = match versioned.into_version(*i as u32) { + Ok(v) => v, + Err(()) => return false, + }, (0, Here) => (), _ => return false, }; diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs index 9b087320cf73..c58e04261072 100644 --- a/xcm/pallet-xcm/src/tests.rs +++ b/xcm/pallet-xcm/src/tests.rs @@ -14,12 +14,12 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use crate::{mock::*, QueryStatus, AssetTraps}; +use crate::{mock::*, AssetTraps, QueryStatus}; use frame_support::{assert_noop, assert_ok, traits::Currency}; use polkadot_parachain::primitives::{AccountIdConversion, Id as ParaId}; use sp_runtime::traits::{BlakeTwo256, Hash}; use std::convert::TryInto; -use xcm::{VersionedMultiAssets, VersionedXcm, latest::prelude::*}; +use xcm::{latest::prelude::*, VersionedMultiAssets, VersionedXcm}; use xcm_executor::XcmExecutor; const ALICE: AccountId = AccountId::new([0u8; 32]); @@ -300,8 +300,7 @@ fn execute_withdraw_to_deposit_works() { /// Test drop/claim assets. #[test] fn trapped_assets_can_be_claimed() { - let balances = - vec![(ALICE, INITIAL_BALANCE), (BOB, INITIAL_BALANCE)]; + let balances = vec![(ALICE, INITIAL_BALANCE), (BOB, INITIAL_BALANCE)]; new_test_ext_with_balances(balances).execute_with(|| { let weight = 6 * BaseXcmWeight::get(); let dest: MultiLocation = @@ -330,9 +329,9 @@ fn trapped_assets_can_be_claimed() { last_events(2), vec![ Event::XcmPallet(crate::Event::AssetsTrapped(hash.clone(), source, vma)), - Event::XcmPallet(crate::Event::Attempted( - Outcome::Complete(5 * BaseXcmWeight::get()) - )) + Event::XcmPallet(crate::Event::Attempted(Outcome::Complete( + 5 * BaseXcmWeight::get() + ))) ] ); assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE - SEND_AMOUNT); @@ -368,9 +367,10 @@ fn trapped_assets_can_be_claimed() { )); assert_eq!( last_event(), - Event::XcmPallet(crate::Event::Attempted( - Outcome::Incomplete(BaseXcmWeight::get(), XcmError::UnknownClaim) - )) + Event::XcmPallet(crate::Event::Attempted(Outcome::Incomplete( + BaseXcmWeight::get(), + XcmError::UnknownClaim + ))) ); }); } diff --git a/xcm/src/lib.rs b/xcm/src/lib.rs index 33404935a6cf..b9aad191ef0d 100644 --- a/xcm/src/lib.rs +++ b/xcm/src/lib.rs @@ -222,7 +222,7 @@ impl VersionedMultiAssets { Ok(match n { 0 => Self::V0(self.try_into()?), 1 | 2 => Self::V1(self.try_into()?), - _ => return Err(()) + _ => return Err(()), }) } } diff --git a/xcm/src/v2/mod.rs b/xcm/src/v2/mod.rs index a6d75930cb63..c89c20df747d 100644 --- a/xcm/src/v2/mod.rs +++ b/xcm/src/v2/mod.rs @@ -510,7 +510,7 @@ pub enum Instruction { /// Refund any surplus weight previously bought with `BuyExecution`. /// /// Kind: *Instruction* - /// + /// /// Errors: None. RefundSurplus, @@ -526,7 +526,7 @@ pub enum Instruction { /// handler, which can reasonably be negative, which would result in a surplus. /// /// Kind: *Instruction* - /// + /// /// Errors: None. SetErrorHandler(Xcm), @@ -554,7 +554,7 @@ pub enum Instruction { ClearError, /// Create some assets which are being held on behalf of the origin. - /// + /// /// - `assets`: The assets which are to be claimed. This must match exactly with the assets /// claimable by the origin of the ticket. /// - `ticket`: The ticket of the asset; this is an abstract identifier to help locate the @@ -566,9 +566,9 @@ pub enum Instruction { ClaimAsset { assets: MultiAssets, ticket: MultiLocation }, /// Always throws an error of type `Trap`. - /// + /// /// Kind: *Instruction* - /// + /// /// Errors: /// - `Trap`: All circumstances, whose inner value is the same as this item's inner value. Trap(u64), diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index dceb0f76bb19..d0261d5d09ef 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -60,7 +60,10 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFro let mut iter = message.0.iter_mut(); let i = iter.next().ok_or(())?; match i { - ReceiveTeleportedAsset(..) | WithdrawAsset(..) | ReserveAssetDeposited(..) | ClaimAsset{..} => (), + ReceiveTeleportedAsset(..) | + WithdrawAsset(..) | + ReserveAssetDeposited(..) | + ClaimAsset { .. } => (), _ => return Err(()), } let mut i = iter.next().ok_or(())?; diff --git a/xcm/xcm-builder/src/mock.rs b/xcm/xcm-builder/src/mock.rs index 0661b179e026..e13bb6b4dc64 100644 --- a/xcm/xcm-builder/src/mock.rs +++ b/xcm/xcm-builder/src/mock.rs @@ -282,6 +282,6 @@ impl Config for TestConfig { type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; type ResponseHandler = TestResponseHandler; - type AssetTrap = (); // TODO: TestAssetTrap - type AssetClaims = (); // TODO: TestAssetClaims + type AssetTrap = (); // TODO: TestAssetTrap + type AssetClaims = (); // TODO: TestAssetClaims } diff --git a/xcm/xcm-executor/src/config.rs b/xcm/xcm-executor/src/config.rs index 7eaf254a3028..91daa4a581d9 100644 --- a/xcm/xcm-executor/src/config.rs +++ b/xcm/xcm-executor/src/config.rs @@ -15,8 +15,8 @@ // along with Polkadot. If not, see . use crate::traits::{ - ConvertOrigin, FilterAssetLocation, InvertLocation, OnResponse, ShouldExecute, TransactAsset, - WeightBounds, WeightTrader, DropAssets, ClaimAssets, + ClaimAssets, ConvertOrigin, DropAssets, FilterAssetLocation, InvertLocation, OnResponse, + ShouldExecute, TransactAsset, WeightBounds, WeightTrader, }; use frame_support::{ dispatch::{Dispatchable, Parameter}, diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 7c9bfe05f86f..01a8e96b516b 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -31,8 +31,8 @@ use xcm::latest::{ pub mod traits; use traits::{ - ConvertOrigin, FilterAssetLocation, InvertLocation, OnResponse, ShouldExecute, TransactAsset, - WeightBounds, WeightTrader, DropAssets, ClaimAssets, + ClaimAssets, ConvertOrigin, DropAssets, FilterAssetLocation, InvertLocation, OnResponse, + ShouldExecute, TransactAsset, WeightBounds, WeightTrader, }; mod assets; @@ -416,10 +416,8 @@ impl XcmExecutor { self.holding.subsume(asset); } Ok(()) - } - Trap(code) => { - Err(XcmError::Trap(code)) - } + }, + Trap(code) => Err(XcmError::Trap(code)), ExchangeAsset { .. } => Err(XcmError::Unimplemented), HrmpNewChannelOpenRequest { .. } => Err(XcmError::Unimplemented), HrmpChannelAccepted { .. } => Err(XcmError::Unimplemented), diff --git a/xcm/xcm-executor/src/traits/drop_assets.rs b/xcm/xcm-executor/src/traits/drop_assets.rs index 6bec530e2419..7415c652c43f 100644 --- a/xcm/xcm-executor/src/traits/drop_assets.rs +++ b/xcm/xcm-executor/src/traits/drop_assets.rs @@ -21,16 +21,10 @@ use xcm::latest::{MultiAssets, MultiLocation}; /// Define a handler for when some non-empty `Assets` value should be dropped. pub trait DropAssets { /// Handler for receiving dropped assets. Returns the weight consumed by this operation. - fn drop_assets( - origin: &MultiLocation, - assets: Assets, - ) -> Weight; + fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight; } impl DropAssets for () { - fn drop_assets( - _origin: &MultiLocation, - _assets: Assets, - ) -> Weight { + fn drop_assets(_origin: &MultiLocation, _assets: Assets) -> Weight { 0 } } diff --git a/xcm/xcm-executor/src/traits/mod.rs b/xcm/xcm-executor/src/traits/mod.rs index 687a16a38a04..94ef8bd4bd0f 100644 --- a/xcm/xcm-executor/src/traits/mod.rs +++ b/xcm/xcm-executor/src/traits/mod.rs @@ -19,7 +19,7 @@ mod conversion; pub use conversion::{Convert, ConvertOrigin, Decoded, Encoded, Identity, InvertLocation, JustTry}; mod drop_assets; -pub use drop_assets::{DropAssets, ClaimAssets}; +pub use drop_assets::{ClaimAssets, DropAssets}; mod filter_asset_location; pub use filter_asset_location::FilterAssetLocation; mod matches_fungible; From 88c52ba251c8bf7541be27e0cdfd32e2b59a48ca Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 15:29:48 -0500 Subject: [PATCH 09/16] Fixes --- runtime/kusama/src/lib.rs | 4 +++- runtime/rococo/src/lib.rs | 4 +++- runtime/test-runtime/src/xcm_config.rs | 2 ++ runtime/westend/src/lib.rs | 4 +++- xcm/xcm-builder/tests/mock/mod.rs | 2 +- 5 files changed, 12 insertions(+), 4 deletions(-) diff --git a/runtime/kusama/src/lib.rs b/runtime/kusama/src/lib.rs index 3d7eac08e775..036b8ab392fb 100644 --- a/runtime/kusama/src/lib.rs +++ b/runtime/kusama/src/lib.rs @@ -1296,7 +1296,9 @@ impl xcm_executor::Config for XcmConfig { type Weigher = FixedWeightBounds; // The weight trader piggybacks on the existing transaction-fee conversion logic. type Trader = UsingComponents>; - type ResponseHandler = (); + type ResponseHandler = XcmPallet; + type AssetTrap = XcmPallet; + type AssetClaims = XcmPallet; } parameter_types! { diff --git a/runtime/rococo/src/lib.rs b/runtime/rococo/src/lib.rs index 1ae220330b1c..c46bca567506 100644 --- a/runtime/rococo/src/lib.rs +++ b/runtime/rococo/src/lib.rs @@ -669,7 +669,9 @@ impl xcm_executor::Config for XcmConfig { type Barrier = Barrier; type Weigher = FixedWeightBounds; type Trader = UsingComponents>; - type ResponseHandler = (); + type ResponseHandler = XcmPallet; + type AssetTrap = XcmPallet; + type AssetClaims = XcmPallet; } parameter_types! { diff --git a/runtime/test-runtime/src/xcm_config.rs b/runtime/test-runtime/src/xcm_config.rs index 3af079e92f43..3a25b8c9ff2b 100644 --- a/runtime/test-runtime/src/xcm_config.rs +++ b/runtime/test-runtime/src/xcm_config.rs @@ -86,4 +86,6 @@ impl xcm_executor::Config for XcmConfig { type Weigher = FixedWeightBounds; type Trader = DummyWeightTrader; type ResponseHandler = super::Xcm; + type AssetTrap = super::Xcm; + type AssetClaims = super::Xcm; } diff --git a/runtime/westend/src/lib.rs b/runtime/westend/src/lib.rs index 0be1568f0fc3..52c8a72c134e 100644 --- a/runtime/westend/src/lib.rs +++ b/runtime/westend/src/lib.rs @@ -938,7 +938,9 @@ impl xcm_executor::Config for XcmConfig { type Barrier = Barrier; type Weigher = FixedWeightBounds; type Trader = UsingComponents>; - type ResponseHandler = (); + type ResponseHandler = XcmPallet; + type AssetTrap = XcmPallet; + type AssetClaims = XcmPallet; } /// Type to convert an `Origin` type value into a `MultiLocation` value which represents an interior location diff --git a/xcm/xcm-builder/tests/mock/mod.rs b/xcm/xcm-builder/tests/mock/mod.rs index 6c3285d838b9..e28352485fb5 100644 --- a/xcm/xcm-builder/tests/mock/mod.rs +++ b/xcm/xcm-builder/tests/mock/mod.rs @@ -166,7 +166,7 @@ impl xcm_executor::Config for XcmConfig { type Barrier = Barrier; type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; - type ResponseHandler = (); + type ResponseHandler = XcmPallet; type AssetTrap = XcmPallet; type AssetClaims = XcmPallet; } From 94bf490968f24cd66a6c0d985963a0cef114bda6 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 15:31:07 -0500 Subject: [PATCH 10/16] Docs --- runtime/common/src/claims.rs | 2 +- xcm/pallet-xcm/src/lib.rs | 2 +- xcm/src/v2/traits.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/runtime/common/src/claims.rs b/runtime/common/src/claims.rs index 60d9a0aa9e85..f2be5a954b3c 100644 --- a/runtime/common/src/claims.rs +++ b/runtime/common/src/claims.rs @@ -1044,7 +1044,7 @@ mod tests { } #[test] - fn invalid_attest_transactions_are_recognised() { + fn invalid_attest_transactions_are_recognized() { new_test_ext().execute_with(|| { let p = PrevalidateAttests::::new(); let c = Call::Claims(ClaimsCall::attest(StatementKind::Regular.to_text().to_vec())); diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index c75b9984828d..8c50f52b5c68 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -249,7 +249,7 @@ pub mod pallet { /// The existing asset traps. /// - /// Key is the blake2 256 hash of (origin, versioned multiassets) pair. Value is the number of + /// Key is the blake2 256 hash of (origin, versioned `MultiAssets`) pair. Value is the number of /// times this pair has been trapped (usually just 1 if it exists at all). #[pallet::storage] #[pallet::getter(fn asset_trap)] diff --git a/xcm/src/v2/traits.rs b/xcm/src/v2/traits.rs index c9f84451ecfe..d5d96a5d0c46 100644 --- a/xcm/src/v2/traits.rs +++ b/xcm/src/v2/traits.rs @@ -96,7 +96,7 @@ pub enum Error { UnknownWeightRequired, /// An error was intentionally forced. A code is included. Trap(u64), - /// The given claim could not be recognised/found. + /// The given claim could not be recognized/found. UnknownClaim, } From 1186685b1136068bffa84d6de3644bc197f67cc4 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 16:51:51 -0500 Subject: [PATCH 11/16] Filter types to allow runtimes to dictate which assets/origins should be trapped --- xcm/xcm-builder/src/mock.rs | 4 +-- xcm/xcm-executor/src/traits/drop_assets.rs | 39 +++++++++++++++++++++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/xcm/xcm-builder/src/mock.rs b/xcm/xcm-builder/src/mock.rs index e13bb6b4dc64..b51e035f9f71 100644 --- a/xcm/xcm-builder/src/mock.rs +++ b/xcm/xcm-builder/src/mock.rs @@ -282,6 +282,6 @@ impl Config for TestConfig { type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; type ResponseHandler = TestResponseHandler; - type AssetTrap = (); // TODO: TestAssetTrap - type AssetClaims = (); // TODO: TestAssetClaims + type AssetTrap = (); + type AssetClaims = (); } diff --git a/xcm/xcm-executor/src/traits/drop_assets.rs b/xcm/xcm-executor/src/traits/drop_assets.rs index 7415c652c43f..83e319f74c69 100644 --- a/xcm/xcm-executor/src/traits/drop_assets.rs +++ b/xcm/xcm-executor/src/traits/drop_assets.rs @@ -14,8 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +use std::marker::PhantomData; + use crate::Assets; -use frame_support::weights::Weight; +use frame_support::{traits::Contains, weights::Weight}; use xcm::latest::{MultiAssets, MultiLocation}; /// Define a handler for when some non-empty `Assets` value should be dropped. @@ -29,6 +31,41 @@ impl DropAssets for () { } } +/// Morph a given `DropAssets` implementation into one which can filter based on assets. This can +/// be used to ensure that `Assets` values which hold no value are ignores. +pub struct FilterAssets(PhantomData<(D, A)>); + +impl< + D: DropAssets, + A: Contains, +> DropAssets for FilterAssets { + fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { + if A::contains(&assets) { + D::drop_assets(origin, assets) + } else { + 0 + } + } +} + +/// Morph a given `DropAssets` implementation into one which can filter based on origin. This can +/// be used to ban origins which don't have proper protections/policies against misuse of the +/// asset trap facility don't get to use it. +pub struct FilterOrigin(PhantomData<(D, O)>); + +impl< + D: DropAssets, + O: Contains, +> DropAssets for FilterOrigin { + fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { + if O::contains(origin) { + D::drop_assets(origin, assets) + } else { + 0 + } + } +} + /// Define any handlers for the `AssetClaim` instruction. pub trait ClaimAssets { /// Claim any assets available to `origin` and return them in a single `Assets` value, together From 2b23b24246c94d27cdb25ccd3656628c56dd7a36 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 16:53:16 -0500 Subject: [PATCH 12/16] Formatting --- xcm/xcm-executor/src/traits/drop_assets.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/xcm/xcm-executor/src/traits/drop_assets.rs b/xcm/xcm-executor/src/traits/drop_assets.rs index 83e319f74c69..8d5cb5aa0d93 100644 --- a/xcm/xcm-executor/src/traits/drop_assets.rs +++ b/xcm/xcm-executor/src/traits/drop_assets.rs @@ -35,10 +35,7 @@ impl DropAssets for () { /// be used to ensure that `Assets` values which hold no value are ignores. pub struct FilterAssets(PhantomData<(D, A)>); -impl< - D: DropAssets, - A: Contains, -> DropAssets for FilterAssets { +impl> DropAssets for FilterAssets { fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { if A::contains(&assets) { D::drop_assets(origin, assets) @@ -53,10 +50,7 @@ impl< /// asset trap facility don't get to use it. pub struct FilterOrigin(PhantomData<(D, O)>); -impl< - D: DropAssets, - O: Contains, -> DropAssets for FilterOrigin { +impl> DropAssets for FilterOrigin { fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { if O::contains(origin) { D::drop_assets(origin, assets) From 3ba05fb2ca1bb47f0e19eb3a69095b6a8cf94de0 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 17:27:27 -0500 Subject: [PATCH 13/16] Tests --- xcm/xcm-builder/src/mock.rs | 36 ++++++++++- xcm/xcm-builder/src/tests.rs | 113 +++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+), 2 deletions(-) diff --git a/xcm/xcm-builder/src/mock.rs b/xcm/xcm-builder/src/mock.rs index b51e035f9f71..7eb277bae44c 100644 --- a/xcm/xcm-builder/src/mock.rs +++ b/xcm/xcm-builder/src/mock.rs @@ -35,6 +35,7 @@ pub use sp_std::{ marker::PhantomData, }; pub use xcm::latest::prelude::*; +use xcm_executor::traits::{ClaimAssets, DropAssets}; pub use xcm_executor::{ traits::{ConvertOrigin, FilterAssetLocation, InvertLocation, OnResponse, TransactAsset}, Assets, Config, @@ -269,6 +270,37 @@ pub type TestBarrier = ( AllowUnpaidExecutionFrom>, ); +parameter_types! { + pub static TrappedAssets: Vec<(MultiLocation, MultiAssets)> = vec![]; +} + +pub struct TestAssetTrap; + +impl DropAssets for TestAssetTrap { + fn drop_assets(origin: &MultiLocation, assets: Assets) -> Weight { + let mut t: Vec<(MultiLocation, MultiAssets)> = TrappedAssets::get(); + t.push((origin.clone(), assets.into())); + TrappedAssets::set(t); + 5 + } +} + +impl ClaimAssets for TestAssetTrap { + fn claim_assets(origin: &MultiLocation, ticket: &MultiLocation, what: &MultiAssets) -> bool { + let mut t: Vec<(MultiLocation, MultiAssets)> = TrappedAssets::get(); + if let (0, X1(GeneralIndex(i))) = (ticket.parents, &ticket.interior) { + if let Some((l, a)) = t.get(*i as usize) { + if l == origin && a == what { + t.swap_remove(*i as usize); + TrappedAssets::set(t); + return true; + } + } + } + false + } +} + pub struct TestConfig; impl Config for TestConfig { type Call = TestCall; @@ -282,6 +314,6 @@ impl Config for TestConfig { type Weigher = FixedWeightBounds; type Trader = FixedRateOfFungible; type ResponseHandler = TestResponseHandler; - type AssetTrap = (); - type AssetClaims = (); + type AssetTrap = TestAssetTrap; + type AssetClaims = TestAssetTrap; } diff --git a/xcm/xcm-builder/src/tests.rs b/xcm/xcm-builder/src/tests.rs index 869595a90a22..038acf85c711 100644 --- a/xcm/xcm-builder/src/tests.rs +++ b/xcm/xcm-builder/src/tests.rs @@ -202,6 +202,119 @@ fn transfer_should_work() { assert_eq!(sent_xcm(), vec![]); } +#[test] +fn basic_asset_trap_should_work() { + // we'll let them have message execution for free. + AllowUnpaidFrom::set(vec![X1(Parachain(1)).into(), X1(Parachain(2)).into()]); + + // Child parachain #1 owns 1000 tokens held by us in reserve. + add_asset(1001, (Here, 1000)); + // They want to transfer 100 of them to their sibling parachain #2 but have a problem + let r = XcmExecutor::::execute_xcm( + Parachain(1).into(), + Xcm(vec![ + WithdrawAsset((Here, 100).into()), + DepositAsset { + assets: Wild(All), + max_assets: 0, //< Whoops! + beneficiary: AccountIndex64 { index: 3, network: Any }.into(), + }, + ]), + 20, + ); + assert_eq!(r, Outcome::Complete(25)); + assert_eq!(assets(1001), vec![(Here, 900).into()]); + assert_eq!(assets(3), vec![]); + + // Incorrect ticket doesn't work. + let old_trapped_assets = TrappedAssets::get(); + let r = XcmExecutor::::execute_xcm( + Parachain(1).into(), + Xcm(vec![ + ClaimAsset { assets: (Here, 100).into(), ticket: GeneralIndex(1).into() }, + DepositAsset { + assets: Wild(All), + max_assets: 1, + beneficiary: AccountIndex64 { index: 3, network: Any }.into(), + }, + ]), + 20, + ); + assert_eq!(r, Outcome::Incomplete(10, XcmError::UnknownClaim)); + assert_eq!(assets(1001), vec![(Here, 900).into()]); + assert_eq!(assets(3), vec![]); + assert_eq!(old_trapped_assets, TrappedAssets::get()); + + // Incorrect origin doesn't work. + let old_trapped_assets = TrappedAssets::get(); + let r = XcmExecutor::::execute_xcm( + Parachain(2).into(), + Xcm(vec![ + ClaimAsset { assets: (Here, 100).into(), ticket: GeneralIndex(0).into() }, + DepositAsset { + assets: Wild(All), + max_assets: 1, + beneficiary: AccountIndex64 { index: 3, network: Any }.into(), + }, + ]), + 20, + ); + assert_eq!(r, Outcome::Incomplete(10, XcmError::UnknownClaim)); + assert_eq!(assets(1001), vec![(Here, 900).into()]); + assert_eq!(assets(3), vec![]); + assert_eq!(old_trapped_assets, TrappedAssets::get()); + + // Incorrect assets doesn't work. + let old_trapped_assets = TrappedAssets::get(); + let r = XcmExecutor::::execute_xcm( + Parachain(1).into(), + Xcm(vec![ + ClaimAsset { assets: (Here, 101).into(), ticket: GeneralIndex(0).into() }, + DepositAsset { + assets: Wild(All), + max_assets: 1, + beneficiary: AccountIndex64 { index: 3, network: Any }.into(), + }, + ]), + 20, + ); + assert_eq!(r, Outcome::Incomplete(10, XcmError::UnknownClaim)); + assert_eq!(assets(1001), vec![(Here, 900).into()]); + assert_eq!(assets(3), vec![]); + assert_eq!(old_trapped_assets, TrappedAssets::get()); + + let r = XcmExecutor::::execute_xcm( + Parachain(1).into(), + Xcm(vec![ + ClaimAsset { assets: (Here, 100).into(), ticket: GeneralIndex(0).into() }, + DepositAsset { + assets: Wild(All), + max_assets: 1, + beneficiary: AccountIndex64 { index: 3, network: Any }.into(), + }, + ]), + 20, + ); + assert_eq!(r, Outcome::Complete(20)); + assert_eq!(assets(1001), vec![(Here, 900).into()]); + assert_eq!(assets(3), vec![(Here, 100).into()]); + + // Same again doesn't work :-) + let r = XcmExecutor::::execute_xcm( + Parachain(1).into(), + Xcm(vec![ + ClaimAsset { assets: (Here, 100).into(), ticket: GeneralIndex(0).into() }, + DepositAsset { + assets: Wild(All), + max_assets: 1, + beneficiary: AccountIndex64 { index: 3, network: Any }.into(), + }, + ]), + 20, + ); + assert_eq!(r, Outcome::Incomplete(10, XcmError::UnknownClaim)); +} + #[test] fn errors_should_return_unused_weight() { // we'll let them have message execution for free. From 87f14172931909f3bd015755e29944a2a8855ca2 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 17:31:38 -0500 Subject: [PATCH 14/16] Formatting --- xcm/xcm-builder/src/mock.rs | 2 +- xcm/xcm-builder/src/tests.rs | 6 +++--- xcm/xcm-executor/src/traits/drop_assets.rs | 3 +-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/xcm/xcm-builder/src/mock.rs b/xcm/xcm-builder/src/mock.rs index 7eb277bae44c..3f00b5019558 100644 --- a/xcm/xcm-builder/src/mock.rs +++ b/xcm/xcm-builder/src/mock.rs @@ -293,7 +293,7 @@ impl ClaimAssets for TestAssetTrap { if l == origin && a == what { t.swap_remove(*i as usize); TrappedAssets::set(t); - return true; + return true } } } diff --git a/xcm/xcm-builder/src/tests.rs b/xcm/xcm-builder/src/tests.rs index 038acf85c711..c7913c1281b0 100644 --- a/xcm/xcm-builder/src/tests.rs +++ b/xcm/xcm-builder/src/tests.rs @@ -216,7 +216,7 @@ fn basic_asset_trap_should_work() { WithdrawAsset((Here, 100).into()), DepositAsset { assets: Wild(All), - max_assets: 0, //< Whoops! + max_assets: 0, //< Whoops! beneficiary: AccountIndex64 { index: 3, network: Any }.into(), }, ]), @@ -263,7 +263,7 @@ fn basic_asset_trap_should_work() { assert_eq!(assets(1001), vec![(Here, 900).into()]); assert_eq!(assets(3), vec![]); assert_eq!(old_trapped_assets, TrappedAssets::get()); - + // Incorrect assets doesn't work. let old_trapped_assets = TrappedAssets::get(); let r = XcmExecutor::::execute_xcm( @@ -282,7 +282,7 @@ fn basic_asset_trap_should_work() { assert_eq!(assets(1001), vec![(Here, 900).into()]); assert_eq!(assets(3), vec![]); assert_eq!(old_trapped_assets, TrappedAssets::get()); - + let r = XcmExecutor::::execute_xcm( Parachain(1).into(), Xcm(vec![ diff --git a/xcm/xcm-executor/src/traits/drop_assets.rs b/xcm/xcm-executor/src/traits/drop_assets.rs index 8d5cb5aa0d93..9552a1971e19 100644 --- a/xcm/xcm-executor/src/traits/drop_assets.rs +++ b/xcm/xcm-executor/src/traits/drop_assets.rs @@ -14,9 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::marker::PhantomData; - use crate::Assets; +use core::marker::PhantomData; use frame_support::{traits::Contains, weights::Weight}; use xcm::latest::{MultiAssets, MultiLocation}; From ee98056c4d48a77b6a8fa14e1dfa2590c6737e93 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 17:45:35 -0500 Subject: [PATCH 15/16] Fixes --- xcm/pallet-xcm/src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index 8c50f52b5c68..83f8b65524ed 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -234,9 +234,6 @@ pub mod pallet { /// Value of a query, must be unique for each query. pub type QueryId = u64; - /// Identifier for an asset trap. - pub type TrapId = u64; - /// The latest available query index. #[pallet::storage] pub(super) type QueryCount = StorageValue<_, QueryId, ValueQuery>; From f138680a1253c3487193adb163ceff691c1d594b Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Fri, 27 Aug 2021 19:09:20 -0500 Subject: [PATCH 16/16] Docs --- xcm/xcm-executor/src/config.rs | 3 +-- xcm/xcm-executor/src/traits/drop_assets.rs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/xcm/xcm-executor/src/config.rs b/xcm/xcm-executor/src/config.rs index 91daa4a581d9..153a9de9a794 100644 --- a/xcm/xcm-executor/src/config.rs +++ b/xcm/xcm-executor/src/config.rs @@ -63,7 +63,6 @@ pub trait Config { /// end of execution. type AssetTrap: DropAssets; - /// The general asset trap - handler for when assets are left in the Holding Register at the - /// end of execution. + /// The handler for when there is an instruction to claim assets. type AssetClaims: ClaimAssets; } diff --git a/xcm/xcm-executor/src/traits/drop_assets.rs b/xcm/xcm-executor/src/traits/drop_assets.rs index 9552a1971e19..5f82c5feb74b 100644 --- a/xcm/xcm-executor/src/traits/drop_assets.rs +++ b/xcm/xcm-executor/src/traits/drop_assets.rs @@ -31,7 +31,7 @@ impl DropAssets for () { } /// Morph a given `DropAssets` implementation into one which can filter based on assets. This can -/// be used to ensure that `Assets` values which hold no value are ignores. +/// be used to ensure that `Assets` values which hold no value are ignored. pub struct FilterAssets(PhantomData<(D, A)>); impl> DropAssets for FilterAssets {