From 6b354fd98e71bc6e0fba647ecf56223aa658a7d5 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Fri, 23 Jun 2023 13:25:22 +0200 Subject: [PATCH 01/12] [PoC] `pallet_xcm::reserve_transfer_assets` `BuyExecution` on destination by different asset, e.g. from SA of UniversalLocation on destination --- xcm/pallet-xcm/Cargo.toml | 3 +- xcm/pallet-xcm/src/lib.rs | 232 +++++++++++++++++++++++++++++++++--- xcm/pallet-xcm/src/mock.rs | 50 +++++++- xcm/pallet-xcm/src/tests.rs | 71 ++++++++++- 4 files changed, 334 insertions(+), 22 deletions(-) diff --git a/xcm/pallet-xcm/Cargo.toml b/xcm/pallet-xcm/Cargo.toml index 20bd5126861f..06ce5dc157de 100644 --- a/xcm/pallet-xcm/Cargo.toml +++ b/xcm/pallet-xcm/Cargo.toml @@ -21,13 +21,13 @@ sp-runtime = { git = "https://github.com/paritytech/substrate", default-features sp-std = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" } xcm = { path = "..", default-features = false } +xcm-builder = { path = "../xcm-builder", default-features = false } 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" } polkadot-parachain = { path = "../../parachain" } -xcm-builder = { path = "../xcm-builder" } [features] default = ["std"] @@ -44,6 +44,7 @@ std = [ "frame-support/std", "frame-system/std", "xcm/std", + "xcm-builder/std", "xcm-executor/std", ] runtime-benchmarks = [ diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index c53c9119bbd2..ed8c507395f8 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -50,6 +50,7 @@ use frame_support::{ }; use frame_system::pallet_prelude::*; pub use pallet::*; +use xcm_builder::ensure_is_remote; use xcm_executor::{ traits::{ CheckSuspension, ClaimAssets, ConvertLocation, DropAssets, MatchesFungible, OnResponse, @@ -145,6 +146,62 @@ impl WeightInfo for TestWeightInfo { } } +/// BuyExecutionSetup enum +pub enum BuyExecutionSetup { + /// In case origin's desired fees setup is used. + Origin, + /// In case `UniversalLocation` wants to do some conversion with origin's desired fees. + /// E.g. swap origin's fee or to `BuyExecution` in destination with different asset recalculated on weight. + UniversalLocation { + /// local account where we want to place additional withdrawn asset from origin (e.g. treasury, staking pot, BH...). + local_account: MultiLocation, + /// account on destination, where we want to place unspent fund from `BuyExecution`. + account_on_destination: MultiLocation, + }, +} + +/// Helper for handling `BuyExecution` setup. +pub trait DecideBuyExecutionSetup { + /// Decides how do we handle `BuyExecution` and fees stuff. + fn decide_for(destination: &MultiLocation, desired_fee_asset_id: &AssetId) + -> BuyExecutionSetup; + + /// Estimates to fees. + fn estimate_fee_for( + destination: &MultiLocation, + desired_fee_asset_id: &AssetId, + weight: &WeightLimit, + ) -> Option; +} + +/// Fees holder +pub struct FeeForBuyExecution { + /// Amount of how much of `desired_fee.AssetId` we need to buy for weight. + proportional_amount_to_withdraw: MultiAsset, + + /// Amount of real fee, which we want to use for `BuyExecution`. + proportional_amount_to_buy_execution: MultiAsset, +} + +/// Implementation handles setup as `BuyExecutionSetup::Origin` +impl DecideBuyExecutionSetup for () { + fn decide_for( + _destination: &MultiLocation, + _desired_fee_asset_id: &AssetId, + ) -> BuyExecutionSetup { + BuyExecutionSetup::Origin + } + + fn estimate_fee_for( + _destination: &MultiLocation, + _desired_fee_asset_id: &AssetId, + _weight: &WeightLimit, + ) -> Option { + // dont do any conversion or what ever, lets handle what origin wants on input + None + } +} + #[frame_support::pallet] pub mod pallet { use super::*; @@ -261,6 +318,9 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; + /// Runtime has ability to decide how we handle `BuyExecution` on destination (dedicated for asset transfer). + type BuyExecutionSetupResolver: DecideBuyExecutionSetup; + /// A `MultiLocation` that can be reached via `XcmRouter`. Used only in benchmarks. /// /// If `None`, the benchmarks that depend on a reachable destination will be skipped. @@ -1206,38 +1266,172 @@ impl Pallet { ensure!(T::XcmReserveTransferFilter::contains(&value), Error::::Filtered); let (origin_location, assets) = value; let context = T::UniversalLocation::get(); + // origin's desired fee asset, means origin wants to be charged in these assets (not reanchoring now) let fees = assets .get(fee_asset_item as usize) .ok_or(Error::::Empty)? - .clone() - .reanchored(&dest, context) - .map_err(|_| Error::::CannotReanchor)?; + .clone(); + + // resolve what setup we use for `BuyExecution` on destination + let buy_execution_setup = T::BuyExecutionSetupResolver::decide_for(&dest, &fees.id); + + // resolve weight_limit let max_assets = assets.len() as u32; let assets: MultiAssets = assets.into(); let weight_limit = match maybe_weight_limit { - Some(weight_limit) => weight_limit, + Some(weight_limit) => { + // lets use desired limit + weight_limit + }, None => { - let fees = fees.clone(); - let mut remote_message = Xcm(vec![ - ReserveAssetDeposited(assets.clone()), - ClearOrigin, - BuyExecution { fees, weight_limit: Limited(Weight::zero()) }, - DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, - ]); + // TODO: lets consider fees as worst case or add here some `T::BuyExecutionSetupResolver::max_limit_for(&dest, &fees.id)`? + let worst_case_fees = fees + .clone() + .reanchored(&dest, context) + .map_err(|_| Error::::CannotReanchor)?; + let worst_case_assets = assets.clone(); + let worst_case_weight_limit = Limited(Weight::zero()); + + // try to estimate weight_limit on destination by expected message executed on destination + let mut remote_message = match buy_execution_setup { + BuyExecutionSetup::Origin => Xcm(vec![ + ReserveAssetDeposited(worst_case_assets), + ClearOrigin, + BuyExecution { + fees: worst_case_fees, + weight_limit: worst_case_weight_limit, + }, + DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, + ]), + BuyExecutionSetup::UniversalLocation { account_on_destination, .. } => + Xcm(vec![ + ReserveAssetDeposited(worst_case_assets.clone()), + ClearOrigin, + // Withdraw fees (do not use those in `ReserveAssetDeposited`) + WithdrawAsset(MultiAssets::from(worst_case_fees.clone())), + BuyExecution { + fees: worst_case_fees.clone(), + weight_limit: worst_case_weight_limit, + }, + RefundSurplus, + // deposit unspent `fees` to some dedicated account + DepositAsset { + assets: MultiAssetFilter::from(MultiAssets::from(worst_case_fees)), + beneficiary: account_on_destination, + }, + // deposit `assets` to beneficiary + DepositAsset { + assets: MultiAssetFilter::from(worst_case_assets), + beneficiary, + }, + ]), + }; + + // if we are going to different consensus, we need to calculate also with `UniversalOrigin/DescendOrigin` + if ensure_is_remote(T::UniversalLocation::get(), dest.clone()).is_ok() { + let (local_net, local_sub) = T::UniversalLocation::get() + .split_global() + .map_err(|_| Error::::BadLocation)?; + remote_message.0.insert(0, UniversalOrigin(GlobalConsensus(local_net))); + if local_sub != Here { + remote_message.0.insert(1, DescendOrigin(local_sub)); + } + } + + // TODO: can we leave it here by default or adds according to some configuration? + // TODO: think about some `trait RemoteMessageEstimator` stuff, where runtime can customize this? + // remote_message.0.push(SetTopic([1; 32])); + // use local weight for remote message and hope for the best. let remote_weight = T::Weigher::weight(&mut remote_message) .map_err(|()| Error::::UnweighableMessage)?; Limited(remote_weight) }, }; - let xcm = Xcm(vec![ - BuyExecution { fees, weight_limit }, - DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, - ]); - let mut message = Xcm(vec![ - SetFeesMode { jit_withdraw: true }, - TransferReserveAsset { assets, dest, xcm }, - ]); + + // lets handle `BuyExecution` + let mut message = match buy_execution_setup { + BuyExecutionSetup::Origin => { + // we need to reanchor fees as dest will see it + let fees = fees + .reanchored(&dest, context) + .map_err(|_| Error::::CannotReanchor)?; + + // no, change, everything is like origin wanted before + let xcm = Xcm(vec![ + BuyExecution { fees, weight_limit }, + DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, + ]); + Xcm(vec![ + SetFeesMode { jit_withdraw: true }, + TransferReserveAsset { assets, dest, xcm }, + ]) + }, + BuyExecutionSetup::UniversalLocation { local_account, account_on_destination } => { + // estimate how much we want to use for `BuyExecution` and proportional amount to withdraw from origin's desired fees + let FeeForBuyExecution { + proportional_amount_to_withdraw, + proportional_amount_to_buy_execution, + } = T::BuyExecutionSetupResolver::estimate_fee_for(&dest, &fees.id, &weight_limit) + .ok_or(Error::::UnweighableMessage)?; + + // here we need to split origin's assets (lets do some math) + let original_assets = assets.clone(); + let assets_for_reserve: MultiAssets = xcm_executor::Assets::from(assets) + .checked_sub(proportional_amount_to_withdraw.clone()) + .map_err(|_| Error::::InvalidAsset)? + .into(); + let assets_to_withdraw = MultiAssets::from(proportional_amount_to_withdraw); + // ensure that origin's assets are still equal to assets_for_reserve + assets_to_withdraw + { + let mut collected_assets = + xcm_executor::Assets::from(assets_for_reserve.clone()); + collected_assets + .subsume_assets(xcm_executor::Assets::from(assets_to_withdraw.clone())); + ensure!( + original_assets == MultiAssets::from(collected_assets), + Error::::InvalidAsset + ); + } + drop(original_assets); + drop(fees); + + // we need to reanchor fees as dest will see it + let proportional_amount_to_buy_execution = proportional_amount_to_buy_execution + .reanchored(&dest, context) + .map_err(|_| Error::::CannotReanchor)?; + + let xcm = Xcm(vec![ + // Withdraw `fees` (do not use those in `ReserveAssetDeposited`) + WithdrawAsset(MultiAssets::from(proportional_amount_to_buy_execution.clone())), + // Use just those `fees` + BuyExecution { + fees: proportional_amount_to_buy_execution.clone(), + weight_limit, + }, + RefundSurplus, + // deposit unspent `fees` to some dedicated account + DepositAsset { + assets: MultiAssetFilter::from(MultiAssets::from( + proportional_amount_to_buy_execution, + )), + beneficiary: account_on_destination, + }, + // deposit `assets` to beneficiary + DepositAsset { + assets: Wild(AllCounted(assets_for_reserve.len() as u32)), + beneficiary, + }, + ]); + Xcm(vec![ + SetFeesMode { jit_withdraw: true }, + TransferAsset { assets: assets_to_withdraw, beneficiary: local_account }, + TransferReserveAsset { assets: assets_for_reserve, dest, xcm }, + ]) + }, + }; + + // execute XCM let weight = T::Weigher::weight(&mut message).map_err(|()| Error::::UnweighableMessage)?; let hash = message.using_encoded(sp_io::hashing::blake2_256); diff --git a/xcm/pallet-xcm/src/mock.rs b/xcm/pallet-xcm/src/mock.rs index 2d3cc385c230..2369dfeed471 100644 --- a/xcm/pallet-xcm/src/mock.rs +++ b/xcm/pallet-xcm/src/mock.rs @@ -36,7 +36,7 @@ use xcm_builder::{ }; use xcm_executor::XcmExecutor; -use crate::{self as pallet_xcm, TestWeightInfo}; +use crate::{self as pallet_xcm, TestWeightInfo, DecideBuyExecutionSetup, BuyExecutionSetup, FeeForBuyExecution}; pub type AccountId = AccountId32; pub type Balance = u128; @@ -253,6 +253,7 @@ impl pallet_balances::Config for Test { parameter_types! { pub const RelayLocation: MultiLocation = Here.into_location(); pub const AnyNetwork: Option = None; + // TODO: add here GlobalConsensus pub UniversalLocation: InteriorMultiLocation = Here; pub UnitWeightCost: u64 = 1_000; } @@ -324,6 +325,52 @@ parameter_types! { pub ReachableDest: Option = Some(Parachain(1000).into()); } +pub const DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID: u32 = 1234; +pub const SOME_LOCAL_ACCOUNT_AS_PARA_ID: u32 = 2222; +pub const SOME_ACCOUNT_ON_DESTINATION_AS_PARA_ID: u32 = 3333; +pub const PROPORTIONAL_SWAPPED_AMOUNT: u128 = 7; +pub const DIFFERENT_ASSET_AMOUNT: u128 = 13; + +parameter_types! { + pub DestinationWithBuyExecutionByDifferentAsset: MultiLocation = Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into(); + pub DifferentAsset: MultiAsset = (Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID), DIFFERENT_ASSET_AMOUNT).into(); + pub ProportionalAmountToWithdraw: MultiAsset = (Here, PROPORTIONAL_SWAPPED_AMOUNT).into(); + pub SomeLocalAccount: MultiLocation = Parachain(SOME_LOCAL_ACCOUNT_AS_PARA_ID).into(); + pub SomeAccountOnDestination: MultiLocation = Parachain(SOME_ACCOUNT_ON_DESTINATION_AS_PARA_ID).into(); +} + +pub struct TestBuyExecutionSetupResolver; +impl DecideBuyExecutionSetup for TestBuyExecutionSetupResolver { + fn decide_for( + destination: &MultiLocation, + _desired_fee_asset_id: &AssetId, + ) -> BuyExecutionSetup { + if destination.eq(&DestinationWithBuyExecutionByDifferentAsset::get()) { + BuyExecutionSetup::UniversalLocation { + local_account: SomeLocalAccount::get(), + account_on_destination: SomeAccountOnDestination::get(), + } + } else { + BuyExecutionSetup::Origin + } + } + + fn estimate_fee_for( + destination: &MultiLocation, + _desired_fee_asset_id: &AssetId, + _weight: &WeightLimit, + ) -> Option { + if destination.eq(&DestinationWithBuyExecutionByDifferentAsset::get()) { + Some(FeeForBuyExecution { + proportional_amount_to_withdraw: ProportionalAmountToWithdraw::get(), + proportional_amount_to_buy_execution: DifferentAsset::get(), + }) + } else { + None + } + } +} + impl pallet_xcm::Config for Test { type RuntimeEvent = RuntimeEvent; type SendXcmOrigin = xcm_builder::EnsureXcmOrigin; @@ -347,6 +394,7 @@ impl pallet_xcm::Config for Test { type MaxRemoteLockConsumers = frame_support::traits::ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = TestWeightInfo; + type BuyExecutionSetupResolver = TestBuyExecutionSetupResolver; #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs index 2ad13dced936..dc78c4f9f0bf 100644 --- a/xcm/pallet-xcm/src/tests.rs +++ b/xcm/pallet-xcm/src/tests.rs @@ -478,7 +478,7 @@ fn unlimited_teleport_assets_works() { }); } -/// Test `reserve_transfer_assets` +/// Test `reserve_transfer_assets` (`BuyExecutionSetup::Origin`) /// /// Asserts that the sender's balance is decreased and the beneficiary's balance /// is increased. Verifies the correct message is sent and event is emitted. @@ -525,6 +525,75 @@ fn reserve_transfer_assets_works() { }); } +/// Test `reserve_transfer_assets` (`BuyExecutionSetup::UniversalLocation`) +/// +/// Asserts that the sender's balance is decreased and the beneficiary's balance +/// is increased. Verifies the correct message is sent and event is emitted. +#[test] +fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() { + let balances = vec![ + (ALICE, INITIAL_BALANCE), + (ParaId::from(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into_account_truncating(), INITIAL_BALANCE), + (ParaId::from(SOME_LOCAL_ACCOUNT_AS_PARA_ID).into_account_truncating(), INITIAL_BALANCE), + ]; + new_test_ext_with_balances(balances).execute_with(|| { + // we expect execute 3 instructions + let expected_weight_for_instructions = BaseXcmWeight::get() * 3; + let dest_account: MultiLocation = Junction::AccountId32 { network: None, id: ALICE.into() }.into(); + + assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE); + assert_ok!(XcmPallet::reserve_transfer_assets( + RuntimeOrigin::signed(ALICE), + Box::new(Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into()), + Box::new(dest_account.clone().into()), + Box::new((Here, SEND_AMOUNT).into()), + 0, + )); + // Alice spent amount + assert_eq!(Balances::free_balance(ALICE), INITIAL_BALANCE - SEND_AMOUNT); + + // Configured local account received part of SEND_AMOUNT that was swapped for different asset + let some_local_account: AccountId = ParaId::from(SOME_LOCAL_ACCOUNT_AS_PARA_ID).into_account_truncating(); + assert_eq!(Balances::free_balance(some_local_account), INITIAL_BALANCE + PROPORTIONAL_SWAPPED_AMOUNT); + + // Destination account (parachain account) has as reserve (SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT) + let dest_para_account: AccountId = ParaId::from(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into_account_truncating(); + assert_eq!(Balances::free_balance(dest_para_account), INITIAL_BALANCE + SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT); + + // Check XCM + assert_eq!( + sent_xcm(), + vec![( + Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into(), + Xcm(vec![ + ReserveAssetDeposited((Parent, SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT).into()), + ClearOrigin, + // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID + WithdrawAsset((Here, DIFFERENT_ASSET_AMOUNT).into()), + // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID + buy_limited_execution((Here, DIFFERENT_ASSET_AMOUNT), Weight::from_parts(7000, 7000)), + RefundSurplus, + // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID + DepositAsset { + assets: Definite(MultiAssets::from(vec![(Here, DIFFERENT_ASSET_AMOUNT).into()])), + beneficiary: SomeAccountOnDestination::get(), + }, + DepositAsset { + assets: AllCounted(1).into(), + beneficiary: dest_account, + }, + ]), + )] + ); + let versioned_sent = VersionedXcm::from(sent_xcm().into_iter().next().unwrap().1); + let _check_v2_ok: xcm::v2::Xcm<()> = versioned_sent.try_into().unwrap(); + assert_eq!( + last_event(), + RuntimeEvent::XcmPallet(crate::Event::Attempted { outcome: Outcome::Complete(expected_weight_for_instructions) }) + ); + }); +} + /// Test `limited_reserve_transfer_assets` /// /// Asserts that the sender's balance is decreased and the beneficiary's balance From db23ce7ffa721a7d82442c09aebac25f1e51afeb Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Fri, 23 Jun 2023 15:09:25 +0200 Subject: [PATCH 02/12] Fields as pub --- xcm/pallet-xcm/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index ed8c507395f8..3ff5f03f38b9 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -177,10 +177,10 @@ pub trait DecideBuyExecutionSetup { /// Fees holder pub struct FeeForBuyExecution { /// Amount of how much of `desired_fee.AssetId` we need to buy for weight. - proportional_amount_to_withdraw: MultiAsset, + pub proportional_amount_to_withdraw: MultiAsset, /// Amount of real fee, which we want to use for `BuyExecution`. - proportional_amount_to_buy_execution: MultiAsset, + pub proportional_amount_to_buy_execution: MultiAsset, } /// Implementation handles setup as `BuyExecutionSetup::Origin` From f50991f0cdec27de76ded045fb203bce63f149bd Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Fri, 23 Jun 2023 16:08:09 +0200 Subject: [PATCH 03/12] Fmt + SetTopic --- xcm/pallet-xcm/src/lib.rs | 12 +++----- xcm/pallet-xcm/src/mock.rs | 5 +++- xcm/pallet-xcm/src/tests.rs | 55 +++++++++++++++++++++++++++---------- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index 3ff5f03f38b9..cf60c35ede25 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -1267,10 +1267,7 @@ impl Pallet { let (origin_location, assets) = value; let context = T::UniversalLocation::get(); // origin's desired fee asset, means origin wants to be charged in these assets (not reanchoring now) - let fees = assets - .get(fee_asset_item as usize) - .ok_or(Error::::Empty)? - .clone(); + let fees = assets.get(fee_asset_item as usize).ok_or(Error::::Empty)?.clone(); // resolve what setup we use for `BuyExecution` on destination let buy_execution_setup = T::BuyExecutionSetupResolver::decide_for(&dest, &fees.id); @@ -1340,7 +1337,7 @@ impl Pallet { // TODO: can we leave it here by default or adds according to some configuration? // TODO: think about some `trait RemoteMessageEstimator` stuff, where runtime can customize this? - // remote_message.0.push(SetTopic([1; 32])); + remote_message.0.push(SetTopic([1; 32])); // use local weight for remote message and hope for the best. let remote_weight = T::Weigher::weight(&mut remote_message) @@ -1353,9 +1350,8 @@ impl Pallet { let mut message = match buy_execution_setup { BuyExecutionSetup::Origin => { // we need to reanchor fees as dest will see it - let fees = fees - .reanchored(&dest, context) - .map_err(|_| Error::::CannotReanchor)?; + let fees = + fees.reanchored(&dest, context).map_err(|_| Error::::CannotReanchor)?; // no, change, everything is like origin wanted before let xcm = Xcm(vec![ diff --git a/xcm/pallet-xcm/src/mock.rs b/xcm/pallet-xcm/src/mock.rs index 2369dfeed471..c4df1d217394 100644 --- a/xcm/pallet-xcm/src/mock.rs +++ b/xcm/pallet-xcm/src/mock.rs @@ -36,7 +36,10 @@ use xcm_builder::{ }; use xcm_executor::XcmExecutor; -use crate::{self as pallet_xcm, TestWeightInfo, DecideBuyExecutionSetup, BuyExecutionSetup, FeeForBuyExecution}; +use crate::{ + self as pallet_xcm, BuyExecutionSetup, DecideBuyExecutionSetup, FeeForBuyExecution, + TestWeightInfo, +}; pub type AccountId = AccountId32; pub type Balance = u128; diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs index dc78c4f9f0bf..27013ab5167d 100644 --- a/xcm/pallet-xcm/src/tests.rs +++ b/xcm/pallet-xcm/src/tests.rs @@ -511,7 +511,11 @@ fn reserve_transfer_assets_works() { Xcm(vec![ ReserveAssetDeposited((Parent, SEND_AMOUNT).into()), ClearOrigin, - buy_limited_execution((Parent, SEND_AMOUNT), Weight::from_parts(4000, 4000)), + buy_limited_execution( + (Parent, SEND_AMOUNT), + // we have just 4 instructions, but 5. is ment to be SetTopic + Weight::from_parts(5000, 5000) + ), DepositAsset { assets: AllCounted(1).into(), beneficiary: dest }, ]), )] @@ -533,13 +537,18 @@ fn reserve_transfer_assets_works() { fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() { let balances = vec![ (ALICE, INITIAL_BALANCE), - (ParaId::from(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into_account_truncating(), INITIAL_BALANCE), + ( + ParaId::from(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID) + .into_account_truncating(), + INITIAL_BALANCE, + ), (ParaId::from(SOME_LOCAL_ACCOUNT_AS_PARA_ID).into_account_truncating(), INITIAL_BALANCE), ]; new_test_ext_with_balances(balances).execute_with(|| { // we expect execute 3 instructions let expected_weight_for_instructions = BaseXcmWeight::get() * 3; - let dest_account: MultiLocation = Junction::AccountId32 { network: None, id: ALICE.into() }.into(); + let dest_account: MultiLocation = + Junction::AccountId32 { network: None, id: ALICE.into() }.into(); assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE); assert_ok!(XcmPallet::reserve_transfer_assets( @@ -553,12 +562,21 @@ fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() assert_eq!(Balances::free_balance(ALICE), INITIAL_BALANCE - SEND_AMOUNT); // Configured local account received part of SEND_AMOUNT that was swapped for different asset - let some_local_account: AccountId = ParaId::from(SOME_LOCAL_ACCOUNT_AS_PARA_ID).into_account_truncating(); - assert_eq!(Balances::free_balance(some_local_account), INITIAL_BALANCE + PROPORTIONAL_SWAPPED_AMOUNT); + let some_local_account: AccountId = + ParaId::from(SOME_LOCAL_ACCOUNT_AS_PARA_ID).into_account_truncating(); + assert_eq!( + Balances::free_balance(some_local_account), + INITIAL_BALANCE + PROPORTIONAL_SWAPPED_AMOUNT + ); // Destination account (parachain account) has as reserve (SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT) - let dest_para_account: AccountId = ParaId::from(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into_account_truncating(); - assert_eq!(Balances::free_balance(dest_para_account), INITIAL_BALANCE + SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT); + let dest_para_account: AccountId = + ParaId::from(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID) + .into_account_truncating(); + assert_eq!( + Balances::free_balance(dest_para_account), + INITIAL_BALANCE + SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT + ); // Check XCM assert_eq!( @@ -566,22 +584,27 @@ fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() vec![( Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into(), Xcm(vec![ - ReserveAssetDeposited((Parent, SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT).into()), + ReserveAssetDeposited( + (Parent, SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT).into() + ), ClearOrigin, // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID WithdrawAsset((Here, DIFFERENT_ASSET_AMOUNT).into()), // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID - buy_limited_execution((Here, DIFFERENT_ASSET_AMOUNT), Weight::from_parts(7000, 7000)), + buy_limited_execution( + (Here, DIFFERENT_ASSET_AMOUNT), + // we have just 7 instructions, but 8. is ment to be SetTopic + Weight::from_parts(8000, 8000) + ), RefundSurplus, // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID DepositAsset { - assets: Definite(MultiAssets::from(vec![(Here, DIFFERENT_ASSET_AMOUNT).into()])), + assets: Definite(MultiAssets::from(vec![ + (Here, DIFFERENT_ASSET_AMOUNT).into() + ])), beneficiary: SomeAccountOnDestination::get(), }, - DepositAsset { - assets: AllCounted(1).into(), - beneficiary: dest_account, - }, + DepositAsset { assets: AllCounted(1).into(), beneficiary: dest_account }, ]), )] ); @@ -589,7 +612,9 @@ fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() let _check_v2_ok: xcm::v2::Xcm<()> = versioned_sent.try_into().unwrap(); assert_eq!( last_event(), - RuntimeEvent::XcmPallet(crate::Event::Attempted { outcome: Outcome::Complete(expected_weight_for_instructions) }) + RuntimeEvent::XcmPallet(crate::Event::Attempted { + outcome: Outcome::Complete(expected_weight_for_instructions) + }) ); }); } From a99aa0457d13300899b9380be2adf3b6daf1eb43 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Mon, 3 Jul 2023 16:43:26 +0200 Subject: [PATCH 04/12] Refactoring + AliasOrigin --- runtime/test-runtime/src/xcm_config.rs | 1 + xcm/pallet-xcm/src/destination_fees.rs | 75 +++++++++++++++++ xcm/pallet-xcm/src/lib.rs | 108 ++++++++----------------- xcm/pallet-xcm/src/mock.rs | 24 +++--- xcm/pallet-xcm/src/tests.rs | 20 +++-- xcm/xcm-builder/tests/mock/mod.rs | 1 + 6 files changed, 136 insertions(+), 93 deletions(-) create mode 100644 xcm/pallet-xcm/src/destination_fees.rs diff --git a/runtime/test-runtime/src/xcm_config.rs b/runtime/test-runtime/src/xcm_config.rs index 45e7956d45ba..7296aee8ad95 100644 --- a/runtime/test-runtime/src/xcm_config.rs +++ b/runtime/test-runtime/src/xcm_config.rs @@ -148,6 +148,7 @@ impl pallet_xcm::Config for crate::Runtime { type MaxRemoteLockConsumers = frame_support::traits::ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = pallet_xcm::TestWeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/xcm/pallet-xcm/src/destination_fees.rs b/xcm/pallet-xcm/src/destination_fees.rs new file mode 100644 index 000000000000..6e7873c27dcd --- /dev/null +++ b/xcm/pallet-xcm/src/destination_fees.rs @@ -0,0 +1,75 @@ +// Copyright (C) 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 . + +//! Utilities for handling fees and `BuyExecution` on destination. + +use xcm::prelude::*; + +/// BuyExecutionSetup enum +pub enum DestinationFeesSetup { + /// In case origin's desired fees setup is used. + ByOrigin, + /// In case `UniversalLocation` wants to do some conversion with origin's desired fees. + /// E.g. swap origin's fee or to `BuyExecution` in destination with different asset. + ByUniversalLocation { + /// Local account where we want to place additional withdrawn assets from origin (e.g. treasury, staking pot, BH...). + local_account: MultiLocation, + }, +} + +/// Holds two adequate amounts. +pub struct DestinationFees { + /// Amount which we want to withdraw on **source** chain. + pub proportional_amount_to_withdraw: MultiAsset, + + /// Amount which we want to use for `BuyExecution` on **destination** chain. + pub proportional_amount_to_buy_execution: MultiAsset, +} + +/// Manages fees handling. +pub trait DestinationFeesManager { + /// Decides how do we handle `BuyExecution` and fees stuff based on `destination` and `requested_fee_asset_id`. + fn decide_for( + destination: &MultiLocation, + requested_fee_asset_id: &AssetId, + ) -> DestinationFeesSetup; + + /// Estimates destination fees. + fn estimate_fee_for( + destination: &MultiLocation, + requested_fee_asset_id: &AssetId, + weight: &WeightLimit, + ) -> Option; +} + +/// Implementation handles setup as `DestinationFeesSetup::Origin` +impl DestinationFeesManager for () { + fn decide_for( + _destination: &MultiLocation, + _desired_fee_asset_id: &AssetId, + ) -> DestinationFeesSetup { + DestinationFeesSetup::ByOrigin + } + + fn estimate_fee_for( + _destination: &MultiLocation, + _desired_fee_asset_id: &AssetId, + _weight: &WeightLimit, + ) -> Option { + // dont do any conversion or what ever, lets handle what origin wants on input + None + } +} diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index cf60c35ede25..acfede5430cc 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -25,6 +25,7 @@ mod mock; #[cfg(test)] mod tests; +pub mod destination_fees; pub mod migration; use codec::{Decode, Encode, EncodeLike, MaxEncodedLen}; @@ -42,6 +43,7 @@ use sp_std::{boxed::Box, marker::PhantomData, prelude::*, result::Result, vec}; use xcm::{latest::QueryResponseInfo, prelude::*}; use xcm_executor::traits::{ConvertOrigin, Properties}; +use crate::destination_fees::{DestinationFees, DestinationFeesManager, DestinationFeesSetup}; use frame_support::{ dispatch::{Dispatchable, GetDispatchInfo}, pallet_prelude::*, @@ -146,65 +148,10 @@ impl WeightInfo for TestWeightInfo { } } -/// BuyExecutionSetup enum -pub enum BuyExecutionSetup { - /// In case origin's desired fees setup is used. - Origin, - /// In case `UniversalLocation` wants to do some conversion with origin's desired fees. - /// E.g. swap origin's fee or to `BuyExecution` in destination with different asset recalculated on weight. - UniversalLocation { - /// local account where we want to place additional withdrawn asset from origin (e.g. treasury, staking pot, BH...). - local_account: MultiLocation, - /// account on destination, where we want to place unspent fund from `BuyExecution`. - account_on_destination: MultiLocation, - }, -} - -/// Helper for handling `BuyExecution` setup. -pub trait DecideBuyExecutionSetup { - /// Decides how do we handle `BuyExecution` and fees stuff. - fn decide_for(destination: &MultiLocation, desired_fee_asset_id: &AssetId) - -> BuyExecutionSetup; - - /// Estimates to fees. - fn estimate_fee_for( - destination: &MultiLocation, - desired_fee_asset_id: &AssetId, - weight: &WeightLimit, - ) -> Option; -} - -/// Fees holder -pub struct FeeForBuyExecution { - /// Amount of how much of `desired_fee.AssetId` we need to buy for weight. - pub proportional_amount_to_withdraw: MultiAsset, - - /// Amount of real fee, which we want to use for `BuyExecution`. - pub proportional_amount_to_buy_execution: MultiAsset, -} - -/// Implementation handles setup as `BuyExecutionSetup::Origin` -impl DecideBuyExecutionSetup for () { - fn decide_for( - _destination: &MultiLocation, - _desired_fee_asset_id: &AssetId, - ) -> BuyExecutionSetup { - BuyExecutionSetup::Origin - } - - fn estimate_fee_for( - _destination: &MultiLocation, - _desired_fee_asset_id: &AssetId, - _weight: &WeightLimit, - ) -> Option { - // dont do any conversion or what ever, lets handle what origin wants on input - None - } -} - #[frame_support::pallet] pub mod pallet { use super::*; + use crate::destination_fees::DestinationFeesManager; use frame_support::{ dispatch::{Dispatchable, GetDispatchInfo, PostDispatchInfo}, parameter_types, @@ -319,7 +266,7 @@ pub mod pallet { type WeightInfo: WeightInfo; /// Runtime has ability to decide how we handle `BuyExecution` on destination (dedicated for asset transfer). - type BuyExecutionSetupResolver: DecideBuyExecutionSetup; + type DestinationFeesManager: DestinationFeesManager; /// A `MultiLocation` that can be reached via `XcmRouter`. Used only in benchmarks. /// @@ -1266,11 +1213,11 @@ impl Pallet { ensure!(T::XcmReserveTransferFilter::contains(&value), Error::::Filtered); let (origin_location, assets) = value; let context = T::UniversalLocation::get(); - // origin's desired fee asset, means origin wants to be charged in these assets (not reanchoring now) + // origin's desired fee asset, means origin wants to be charged in these assets (not re-anchoring now) let fees = assets.get(fee_asset_item as usize).ok_or(Error::::Empty)?.clone(); // resolve what setup we use for `BuyExecution` on destination - let buy_execution_setup = T::BuyExecutionSetupResolver::decide_for(&dest, &fees.id); + let destination_fees_setup = T::DestinationFeesManager::decide_for(&dest, &fees.id); // resolve weight_limit let max_assets = assets.len() as u32; @@ -1289,9 +1236,9 @@ impl Pallet { let worst_case_assets = assets.clone(); let worst_case_weight_limit = Limited(Weight::zero()); - // try to estimate weight_limit on destination by expected message executed on destination - let mut remote_message = match buy_execution_setup { - BuyExecutionSetup::Origin => Xcm(vec![ + // try to estimate weight_limit by expected message executed on destination + let mut remote_message = match destination_fees_setup { + DestinationFeesSetup::ByOrigin => Xcm(vec![ ReserveAssetDeposited(worst_case_assets), ClearOrigin, BuyExecution { @@ -1300,10 +1247,15 @@ impl Pallet { }, DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, ]), - BuyExecutionSetup::UniversalLocation { account_on_destination, .. } => + DestinationFeesSetup::ByUniversalLocation { .. } => { + let sovereign_account_on_destination = T::UniversalLocation::get() + .invert_target(&dest) + .map_err(|_| Error::::DestinationNotInvertible)?; Xcm(vec![ ReserveAssetDeposited(worst_case_assets.clone()), ClearOrigin, + // Change origin to sovereign account of `T::UniversalLocation` on destination. + AliasOrigin(sovereign_account_on_destination), // Withdraw fees (do not use those in `ReserveAssetDeposited`) WithdrawAsset(MultiAssets::from(worst_case_fees.clone())), BuyExecution { @@ -1311,17 +1263,19 @@ impl Pallet { weight_limit: worst_case_weight_limit, }, RefundSurplus, - // deposit unspent `fees` to some dedicated account + // deposit unspent `fees` back to `sovereign_account` DepositAsset { assets: MultiAssetFilter::from(MultiAssets::from(worst_case_fees)), - beneficiary: account_on_destination, + beneficiary: sovereign_account_on_destination, }, + ClearOrigin, // deposit `assets` to beneficiary DepositAsset { assets: MultiAssetFilter::from(worst_case_assets), beneficiary, }, - ]), + ]) + }, }; // if we are going to different consensus, we need to calculate also with `UniversalOrigin/DescendOrigin` @@ -1347,8 +1301,8 @@ impl Pallet { }; // lets handle `BuyExecution` - let mut message = match buy_execution_setup { - BuyExecutionSetup::Origin => { + let mut message = match destination_fees_setup { + DestinationFeesSetup::ByOrigin => { // we need to reanchor fees as dest will see it let fees = fees.reanchored(&dest, context).map_err(|_| Error::::CannotReanchor)?; @@ -1363,12 +1317,12 @@ impl Pallet { TransferReserveAsset { assets, dest, xcm }, ]) }, - BuyExecutionSetup::UniversalLocation { local_account, account_on_destination } => { + DestinationFeesSetup::ByUniversalLocation { local_account } => { // estimate how much we want to use for `BuyExecution` and proportional amount to withdraw from origin's desired fees - let FeeForBuyExecution { + let DestinationFees { proportional_amount_to_withdraw, proportional_amount_to_buy_execution, - } = T::BuyExecutionSetupResolver::estimate_fee_for(&dest, &fees.id, &weight_limit) + } = T::DestinationFeesManager::estimate_fee_for(&dest, &fees.id, &weight_limit) .ok_or(Error::::UnweighableMessage)?; // here we need to split origin's assets (lets do some math) @@ -1397,7 +1351,14 @@ impl Pallet { .reanchored(&dest, context) .map_err(|_| Error::::CannotReanchor)?; + // + let sovereign_account_on_destination = T::UniversalLocation::get() + .invert_target(&dest) + .map_err(|_| Error::::DestinationNotInvertible)?; + let xcm = Xcm(vec![ + // Change origin to sovereign account of `T::UniversalLocation` on destination. + AliasOrigin(sovereign_account_on_destination), // Withdraw `fees` (do not use those in `ReserveAssetDeposited`) WithdrawAsset(MultiAssets::from(proportional_amount_to_buy_execution.clone())), // Use just those `fees` @@ -1406,13 +1367,14 @@ impl Pallet { weight_limit, }, RefundSurplus, - // deposit unspent `fees` to some dedicated account + // deposit unspent `fees` back to `sovereign_account` DepositAsset { assets: MultiAssetFilter::from(MultiAssets::from( proportional_amount_to_buy_execution, )), - beneficiary: account_on_destination, + beneficiary: sovereign_account_on_destination, }, + ClearOrigin, // deposit `assets` to beneficiary DepositAsset { assets: Wild(AllCounted(assets_for_reserve.len() as u32)), diff --git a/xcm/pallet-xcm/src/mock.rs b/xcm/pallet-xcm/src/mock.rs index c4df1d217394..ff58bd816126 100644 --- a/xcm/pallet-xcm/src/mock.rs +++ b/xcm/pallet-xcm/src/mock.rs @@ -37,7 +37,8 @@ use xcm_builder::{ use xcm_executor::XcmExecutor; use crate::{ - self as pallet_xcm, BuyExecutionSetup, DecideBuyExecutionSetup, FeeForBuyExecution, + self as pallet_xcm, + destination_fees::{DestinationFees, DestinationFeesManager, DestinationFeesSetup}, TestWeightInfo, }; @@ -330,7 +331,6 @@ parameter_types! { pub const DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID: u32 = 1234; pub const SOME_LOCAL_ACCOUNT_AS_PARA_ID: u32 = 2222; -pub const SOME_ACCOUNT_ON_DESTINATION_AS_PARA_ID: u32 = 3333; pub const PROPORTIONAL_SWAPPED_AMOUNT: u128 = 7; pub const DIFFERENT_ASSET_AMOUNT: u128 = 13; @@ -339,22 +339,18 @@ parameter_types! { pub DifferentAsset: MultiAsset = (Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID), DIFFERENT_ASSET_AMOUNT).into(); pub ProportionalAmountToWithdraw: MultiAsset = (Here, PROPORTIONAL_SWAPPED_AMOUNT).into(); pub SomeLocalAccount: MultiLocation = Parachain(SOME_LOCAL_ACCOUNT_AS_PARA_ID).into(); - pub SomeAccountOnDestination: MultiLocation = Parachain(SOME_ACCOUNT_ON_DESTINATION_AS_PARA_ID).into(); } -pub struct TestBuyExecutionSetupResolver; -impl DecideBuyExecutionSetup for TestBuyExecutionSetupResolver { +pub struct TestDestinationFeesManager; +impl DestinationFeesManager for TestDestinationFeesManager { fn decide_for( destination: &MultiLocation, _desired_fee_asset_id: &AssetId, - ) -> BuyExecutionSetup { + ) -> DestinationFeesSetup { if destination.eq(&DestinationWithBuyExecutionByDifferentAsset::get()) { - BuyExecutionSetup::UniversalLocation { - local_account: SomeLocalAccount::get(), - account_on_destination: SomeAccountOnDestination::get(), - } + DestinationFeesSetup::ByUniversalLocation { local_account: SomeLocalAccount::get() } } else { - BuyExecutionSetup::Origin + DestinationFeesSetup::ByOrigin } } @@ -362,9 +358,9 @@ impl DecideBuyExecutionSetup for TestBuyExecutionSetupResolver { destination: &MultiLocation, _desired_fee_asset_id: &AssetId, _weight: &WeightLimit, - ) -> Option { + ) -> Option { if destination.eq(&DestinationWithBuyExecutionByDifferentAsset::get()) { - Some(FeeForBuyExecution { + Some(DestinationFees { proportional_amount_to_withdraw: ProportionalAmountToWithdraw::get(), proportional_amount_to_buy_execution: DifferentAsset::get(), }) @@ -397,7 +393,7 @@ impl pallet_xcm::Config for Test { type MaxRemoteLockConsumers = frame_support::traits::ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = TestWeightInfo; - type BuyExecutionSetupResolver = TestBuyExecutionSetupResolver; + type DestinationFeesManager = TestDestinationFeesManager; #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs index 27013ab5167d..fca3057ffc03 100644 --- a/xcm/pallet-xcm/src/tests.rs +++ b/xcm/pallet-xcm/src/tests.rs @@ -529,7 +529,7 @@ fn reserve_transfer_assets_works() { }); } -/// Test `reserve_transfer_assets` (`BuyExecutionSetup::UniversalLocation`) +/// Test `reserve_transfer_assets` (`DestinationFeesSetup::UniversalLocation`) /// /// Asserts that the sender's balance is decreased and the beneficiary's balance /// is increased. Verifies the correct message is sent and event is emitted. @@ -549,11 +549,13 @@ fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() let expected_weight_for_instructions = BaseXcmWeight::get() * 3; let dest_account: MultiLocation = Junction::AccountId32 { network: None, id: ALICE.into() }.into(); + let dest: MultiLocation = + Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into(); assert_eq!(Balances::total_balance(&ALICE), INITIAL_BALANCE); assert_ok!(XcmPallet::reserve_transfer_assets( RuntimeOrigin::signed(ALICE), - Box::new(Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into()), + Box::new(dest.clone().into()), Box::new(dest_account.clone().into()), Box::new((Here, SEND_AMOUNT).into()), 0, @@ -578,23 +580,28 @@ fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() INITIAL_BALANCE + SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT ); + let sovereign_account_on_destination = + UniversalLocation::get().invert_target(&dest).expect("valid location"); + // Check XCM assert_eq!( sent_xcm(), vec![( - Parachain(DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID).into(), + dest, Xcm(vec![ ReserveAssetDeposited( (Parent, SEND_AMOUNT - PROPORTIONAL_SWAPPED_AMOUNT).into() ), ClearOrigin, + // Here we change origin to sovereign account + AliasOrigin(sovereign_account_on_destination), // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID WithdrawAsset((Here, DIFFERENT_ASSET_AMOUNT).into()), // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID buy_limited_execution( (Here, DIFFERENT_ASSET_AMOUNT), // we have just 7 instructions, but 8. is ment to be SetTopic - Weight::from_parts(8000, 8000) + Weight::from_parts(10000, 10000) ), RefundSurplus, // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID @@ -602,14 +609,15 @@ fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() assets: Definite(MultiAssets::from(vec![ (Here, DIFFERENT_ASSET_AMOUNT).into() ])), - beneficiary: SomeAccountOnDestination::get(), + beneficiary: sovereign_account_on_destination, }, + ClearOrigin, DepositAsset { assets: AllCounted(1).into(), beneficiary: dest_account }, ]), )] ); let versioned_sent = VersionedXcm::from(sent_xcm().into_iter().next().unwrap().1); - let _check_v2_ok: xcm::v2::Xcm<()> = versioned_sent.try_into().unwrap(); + let _check_v3_ok: xcm::v3::Xcm<()> = versioned_sent.try_into().unwrap(); assert_eq!( last_event(), RuntimeEvent::XcmPallet(crate::Event::Attempted { diff --git a/xcm/xcm-builder/tests/mock/mod.rs b/xcm/xcm-builder/tests/mock/mod.rs index 6c2c8f222add..fe6f2367ccf2 100644 --- a/xcm/xcm-builder/tests/mock/mod.rs +++ b/xcm/xcm-builder/tests/mock/mod.rs @@ -237,6 +237,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = frame_support::traits::ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = pallet_xcm::TestWeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; From 69842208166926f650973a06263e135c39f67753 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 4 Jul 2023 11:26:07 +0200 Subject: [PATCH 05/12] AliasOrigin workaround + AllowAliasOriginWithdrawPaidExecutionFrom --- xcm/pallet-xcm/src/lib.rs | 25 ++++++------ xcm/xcm-builder/src/barriers.rs | 67 +++++++++++++++++++++++++++++++++ xcm/xcm-builder/src/lib.rs | 8 ++-- xcm/xcm-executor/src/lib.rs | 7 +++- 4 files changed, 90 insertions(+), 17 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index acfede5430cc..d8e85db4c362 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -1238,15 +1238,18 @@ impl Pallet { // try to estimate weight_limit by expected message executed on destination let mut remote_message = match destination_fees_setup { - DestinationFeesSetup::ByOrigin => Xcm(vec![ - ReserveAssetDeposited(worst_case_assets), - ClearOrigin, - BuyExecution { - fees: worst_case_fees, - weight_limit: worst_case_weight_limit, - }, - DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, - ]), + DestinationFeesSetup::ByOrigin => { + // no, change, everything is like origin wanted before + Xcm(vec![ + ReserveAssetDeposited(worst_case_assets), + ClearOrigin, + BuyExecution { + fees: worst_case_fees, + weight_limit: worst_case_weight_limit, + }, + DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, + ]) + }, DestinationFeesSetup::ByUniversalLocation { .. } => { let sovereign_account_on_destination = T::UniversalLocation::get() .invert_target(&dest) @@ -1258,6 +1261,7 @@ impl Pallet { AliasOrigin(sovereign_account_on_destination), // Withdraw fees (do not use those in `ReserveAssetDeposited`) WithdrawAsset(MultiAssets::from(worst_case_fees.clone())), + ClearOrigin, BuyExecution { fees: worst_case_fees.clone(), weight_limit: worst_case_weight_limit, @@ -1268,7 +1272,6 @@ impl Pallet { assets: MultiAssetFilter::from(MultiAssets::from(worst_case_fees)), beneficiary: sovereign_account_on_destination, }, - ClearOrigin, // deposit `assets` to beneficiary DepositAsset { assets: MultiAssetFilter::from(worst_case_assets), @@ -1361,6 +1364,7 @@ impl Pallet { AliasOrigin(sovereign_account_on_destination), // Withdraw `fees` (do not use those in `ReserveAssetDeposited`) WithdrawAsset(MultiAssets::from(proportional_amount_to_buy_execution.clone())), + ClearOrigin, // Use just those `fees` BuyExecution { fees: proportional_amount_to_buy_execution.clone(), @@ -1374,7 +1378,6 @@ impl Pallet { )), beneficiary: sovereign_account_on_destination, }, - ClearOrigin, // deposit `assets` to beneficiary DepositAsset { assets: Wild(AllCounted(assets_for_reserve.len() as u32)), diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index c29dd6c685ae..3ad5bbb1d13f 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -103,6 +103,73 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFro } } +// TODO:check-parameter - dummy copy of `AllowTopLevelPaidExecutionFrom` +/// Allows execution from `origin` if it is contained in `T` (i.e. `T::Contains(origin)`) taking +/// payments into account. +/// +/// Only allows for `TeleportAsset`, `WithdrawAsset`, `ClaimAsset` and `ReserveAssetDeposit` XCMs +/// because they are the only ones that place assets in the Holding Register to pay for execution. +pub struct AllowAliasOriginWithdrawPaidExecutionFrom(PhantomData); +impl> ShouldExecute for AllowAliasOriginWithdrawPaidExecutionFrom { + fn should_execute( + origin: &MultiLocation, + instructions: &mut [Instruction], + max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + log::trace!( + target: "xcm::barriers", + "AllowAliasOriginWithdrawPaidExecutionFrom origin: {:?}, instructions: {:?}, max_weight: {:?}, properties: {:?}", + origin, instructions, max_weight, _properties, + ); + + ensure!(T::contains(origin), ProcessMessageError::Unsupported); + // We will read up to 5 instructions. This allows up to 3 `ClearOrigin` 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 end = instructions.len().min(8); + instructions[..end] + .matcher() + .match_next_inst(|inst| match inst { + ReceiveTeleportedAsset(..) | + WithdrawAsset(..) | + ReserveAssetDeposited(..) | + ClaimAsset { .. } => Ok(()), + _ => Err(ProcessMessageError::BadFormat), + })? + .skip_inst_while(|inst| matches!(inst, ClearOrigin))? + // TODO:check-parameter - this is only difference with `AllowTopLevelPaidExecutionFrom` - check for AliasOrigin / WithdrawAsset / ClearOrigin + .match_next_inst(|inst| match inst { + AliasOrigin(_) => Ok(()), + _ => Err(ProcessMessageError::BadFormat), + })? + // TODO:check-parameter - this is only difference with `AllowTopLevelPaidExecutionFrom` - check for AliasOrigin / WithdrawAsset / ClearOrigin + .match_next_inst(|inst| match inst { + WithdrawAsset(..) => Ok(()), + _ => Err(ProcessMessageError::BadFormat), + })? + // TODO:check-parameter - this is only difference with `AllowTopLevelPaidExecutionFrom` - check for AliasOrigin / WithdrawAsset / ClearOrigin + .match_next_inst(|inst| match inst { + ClearOrigin => Ok(()), + _ => Err(ProcessMessageError::BadFormat), + })? + .match_next_inst(|inst| match inst { + BuyExecution { weight_limit: Limited(ref mut weight), .. } + if weight.all_gte(max_weight) => + { + *weight = max_weight; + Ok(()) + }, + BuyExecution { ref mut weight_limit, .. } if weight_limit == &Unlimited => { + *weight_limit = Limited(max_weight); + Ok(()) + }, + _ => Err(ProcessMessageError::Overweight(max_weight)), + })?; + Ok(()) + } +} + /// A derivative barrier, which scans the first `MaxPrefixes` instructions for origin-alterers and /// then evaluates `should_execute` of the `InnerBarrier` based on the remaining instructions and /// the newly computed origin. diff --git a/xcm/xcm-builder/src/lib.rs b/xcm/xcm-builder/src/lib.rs index 984ace84dc69..ac8881178c00 100644 --- a/xcm/xcm-builder/src/lib.rs +++ b/xcm/xcm-builder/src/lib.rs @@ -54,10 +54,10 @@ pub use asset_conversion::{ConvertedAbstractAssetId, ConvertedConcreteAssetId}; mod barriers; pub use barriers::{ - AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses, AllowSubscriptionsFrom, - AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, - DenyThenTry, IsChildSystemParachain, RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, - WithComputedOrigin, + AllowAliasOriginWithdrawPaidExecutionFrom, AllowExplicitUnpaidExecutionFrom, + AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, + AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, + RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, }; mod process_xcm_message; diff --git a/xcm/xcm-executor/src/lib.rs b/xcm/xcm-executor/src/lib.rs index 050d73837085..10a46f152801 100644 --- a/xcm/xcm-executor/src/lib.rs +++ b/xcm/xcm-executor/src/lib.rs @@ -915,8 +915,11 @@ impl XcmExecutor { Ok(()) }, AliasOrigin(target) => { - let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?; - if Config::Aliasers::contains(origin, &target) { + // TODO:check-parameter - is this correct? + let effective_origin = + self.context.origin.as_ref().unwrap_or(&self.original_origin); + // let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?; + if Config::Aliasers::contains(effective_origin, &target) { self.context.origin = Some(target); Ok(()) } else { From e00538b8e74a40e8334487a254a309bf7a02248d Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 4 Jul 2023 11:52:07 +0200 Subject: [PATCH 06/12] Compile fix --- runtime/kusama/src/xcm_config.rs | 1 + runtime/polkadot/src/xcm_config.rs | 1 + runtime/rococo/src/xcm_config.rs | 1 + runtime/westend/src/xcm_config.rs | 1 + xcm/xcm-simulator/example/src/parachain.rs | 1 + xcm/xcm-simulator/example/src/relay_chain.rs | 1 + xcm/xcm-simulator/fuzzer/src/parachain.rs | 1 + xcm/xcm-simulator/fuzzer/src/relay_chain.rs | 1 + 8 files changed, 8 insertions(+) diff --git a/runtime/kusama/src/xcm_config.rs b/runtime/kusama/src/xcm_config.rs index 0aa50a364a0c..0879e47e1c99 100644 --- a/runtime/kusama/src/xcm_config.rs +++ b/runtime/kusama/src/xcm_config.rs @@ -414,6 +414,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = crate::weights::pallet_xcm::WeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/runtime/polkadot/src/xcm_config.rs b/runtime/polkadot/src/xcm_config.rs index 54696f47ee6b..e78606b6ffeb 100644 --- a/runtime/polkadot/src/xcm_config.rs +++ b/runtime/polkadot/src/xcm_config.rs @@ -421,6 +421,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = crate::weights::pallet_xcm::WeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/runtime/rococo/src/xcm_config.rs b/runtime/rococo/src/xcm_config.rs index bc78ab7ab878..3bb8c6e667a6 100644 --- a/runtime/rococo/src/xcm_config.rs +++ b/runtime/rococo/src/xcm_config.rs @@ -377,6 +377,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = crate::weights::pallet_xcm::WeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/runtime/westend/src/xcm_config.rs b/runtime/westend/src/xcm_config.rs index d6a3feb3bc0f..3e714351a3ff 100644 --- a/runtime/westend/src/xcm_config.rs +++ b/runtime/westend/src/xcm_config.rs @@ -304,6 +304,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = crate::weights::pallet_xcm::WeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/xcm/xcm-simulator/example/src/parachain.rs b/xcm/xcm-simulator/example/src/parachain.rs index 875904ddecd9..52a8abd8cb4b 100644 --- a/xcm/xcm-simulator/example/src/parachain.rs +++ b/xcm/xcm-simulator/example/src/parachain.rs @@ -444,6 +444,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = pallet_xcm::TestWeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/xcm/xcm-simulator/example/src/relay_chain.rs b/xcm/xcm-simulator/example/src/relay_chain.rs index b82e2c9cc306..21e5ababa7bf 100644 --- a/xcm/xcm-simulator/example/src/relay_chain.rs +++ b/xcm/xcm-simulator/example/src/relay_chain.rs @@ -228,6 +228,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = pallet_xcm::TestWeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/xcm/xcm-simulator/fuzzer/src/parachain.rs b/xcm/xcm-simulator/fuzzer/src/parachain.rs index 26438f02f45f..c1c62177bf15 100644 --- a/xcm/xcm-simulator/fuzzer/src/parachain.rs +++ b/xcm/xcm-simulator/fuzzer/src/parachain.rs @@ -342,6 +342,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = frame_support::traits::ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = pallet_xcm::TestWeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; diff --git a/xcm/xcm-simulator/fuzzer/src/relay_chain.rs b/xcm/xcm-simulator/fuzzer/src/relay_chain.rs index ef1339097f5a..cc9a1505fc39 100644 --- a/xcm/xcm-simulator/fuzzer/src/relay_chain.rs +++ b/xcm/xcm-simulator/fuzzer/src/relay_chain.rs @@ -192,6 +192,7 @@ impl pallet_xcm::Config for Runtime { type MaxRemoteLockConsumers = ConstU32<0>; type RemoteLockConsumerIdentifier = (); type WeightInfo = pallet_xcm::TestWeightInfo; + type DestinationFeesManager = (); #[cfg(feature = "runtime-benchmarks")] type ReachableDest = ReachableDest; type AdminOrigin = EnsureRoot; From 856b8d8b95bb9f070cae0bafd73b1be3b2922c0d Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 4 Jul 2023 14:16:44 +0200 Subject: [PATCH 07/12] Fix test --- xcm/pallet-xcm/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xcm/pallet-xcm/src/tests.rs b/xcm/pallet-xcm/src/tests.rs index fca3057ffc03..a84f9da797d6 100644 --- a/xcm/pallet-xcm/src/tests.rs +++ b/xcm/pallet-xcm/src/tests.rs @@ -597,6 +597,7 @@ fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() AliasOrigin(sovereign_account_on_destination), // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID WithdrawAsset((Here, DIFFERENT_ASSET_AMOUNT).into()), + ClearOrigin, // Here - means we are paying with native asset of DEST_WITH_BUY_EXECUTION_BY_DIFFERENT_ASSET_PARA_ID buy_limited_execution( (Here, DIFFERENT_ASSET_AMOUNT), @@ -611,7 +612,6 @@ fn reserve_transfer_assets_for_buy_execution_setup_by_universal_location_works() ])), beneficiary: sovereign_account_on_destination, }, - ClearOrigin, DepositAsset { assets: AllCounted(1).into(), beneficiary: dest_account }, ]), )] From f11c2956df8f638858959d5aa965107f1ee726e4 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Tue, 4 Jul 2023 14:48:24 +0200 Subject: [PATCH 08/12] Clippy --- xcm/pallet-xcm/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index d8e85db4c362..8859f3eb68c6 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -1282,7 +1282,7 @@ impl Pallet { }; // if we are going to different consensus, we need to calculate also with `UniversalOrigin/DescendOrigin` - if ensure_is_remote(T::UniversalLocation::get(), dest.clone()).is_ok() { + if ensure_is_remote(T::UniversalLocation::get(), dest).is_ok() { let (local_net, local_sub) = T::UniversalLocation::get() .split_global() .map_err(|_| Error::::BadLocation)?; From 6cfdcb1b267e8050b64b91683e27c914a5b71c56 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 10 Jul 2023 13:08:25 +0300 Subject: [PATCH 09/12] pallet-xcm: minor nits --- xcm/pallet-xcm/src/destination_fees.rs | 24 +++++---- xcm/pallet-xcm/src/lib.rs | 75 +++++++++++--------------- 2 files changed, 44 insertions(+), 55 deletions(-) diff --git a/xcm/pallet-xcm/src/destination_fees.rs b/xcm/pallet-xcm/src/destination_fees.rs index 6e7873c27dcd..fe8107b4a658 100644 --- a/xcm/pallet-xcm/src/destination_fees.rs +++ b/xcm/pallet-xcm/src/destination_fees.rs @@ -18,36 +18,38 @@ use xcm::prelude::*; -/// BuyExecutionSetup enum +/// Describes how to handle `BuyExecution` on destination chains, +/// useful for handling foreign asset transfers. pub enum DestinationFeesSetup { - /// In case origin's desired fees setup is used. + /// Fees are directly paid using Origin's indicated asset. ByOrigin, - /// In case `UniversalLocation` wants to do some conversion with origin's desired fees. - /// E.g. swap origin's fee or to `BuyExecution` in destination with different asset. + /// `UniversalLocation` does some conversion from origin's indicated asset. + /// E.g. swap origin's fee and/or `BuyExecution` on destination using different asset. ByUniversalLocation { - /// Local account where we want to place additional withdrawn assets from origin (e.g. treasury, staking pot, BH...). + /// Local account where we want to place additional withdrawn assets from origin + /// (e.g. treasury, staking pot, BH...). local_account: MultiLocation, }, } -/// Holds two adequate amounts. +/// Pair of local and remote assets (equivalent in economic value). pub struct DestinationFees { - /// Amount which we want to withdraw on **source** chain. + /// Assets withdrawn on **source** chain. pub proportional_amount_to_withdraw: MultiAsset, - /// Amount which we want to use for `BuyExecution` on **destination** chain. + /// Assets used for `BuyExecution` on **destination** chain. pub proportional_amount_to_buy_execution: MultiAsset, } /// Manages fees handling. pub trait DestinationFeesManager { - /// Decides how do we handle `BuyExecution` and fees stuff based on `destination` and `requested_fee_asset_id`. + /// Decide how to handle `BuyExecution` based on `destination` and `requested_fee_asset_id`. fn decide_for( destination: &MultiLocation, requested_fee_asset_id: &AssetId, ) -> DestinationFeesSetup; - /// Estimates destination fees. + /// Estimate destination fees. fn estimate_fee_for( destination: &MultiLocation, requested_fee_asset_id: &AssetId, @@ -69,7 +71,7 @@ impl DestinationFeesManager for () { _desired_fee_asset_id: &AssetId, _weight: &WeightLimit, ) -> Option { - // dont do any conversion or what ever, lets handle what origin wants on input + // dont do any conversion or whatever, handle what origin wants on input None } } diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index 8859f3eb68c6..273ba38c3008 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -265,7 +265,7 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; - /// Runtime has ability to decide how we handle `BuyExecution` on destination (dedicated for asset transfer). + /// How to handle `BuyExecution` on destination chains, useful for handling foreign asset transfers. type DestinationFeesManager: DestinationFeesManager; /// A `MultiLocation` that can be reached via `XcmRouter`. Used only in benchmarks. @@ -1213,10 +1213,10 @@ impl Pallet { ensure!(T::XcmReserveTransferFilter::contains(&value), Error::::Filtered); let (origin_location, assets) = value; let context = T::UniversalLocation::get(); - // origin's desired fee asset, means origin wants to be charged in these assets (not re-anchoring now) + // origin's desired fee asset, means origin wants to be charged in these assets let fees = assets.get(fee_asset_item as usize).ok_or(Error::::Empty)?.clone(); - // resolve what setup we use for `BuyExecution` on destination + // resolve `BuyExecution`-on-destination strategy for `(dest, fees)` let destination_fees_setup = T::DestinationFeesManager::decide_for(&dest, &fees.id); // resolve weight_limit @@ -1224,64 +1224,53 @@ impl Pallet { let assets: MultiAssets = assets.into(); let weight_limit = match maybe_weight_limit { Some(weight_limit) => { - // lets use desired limit + // use desired limit weight_limit }, None => { - // TODO: lets consider fees as worst case or add here some `T::BuyExecutionSetupResolver::max_limit_for(&dest, &fees.id)`? - let worst_case_fees = fees + // TODO: estimate fees using local weigher, + // or add some `T::BuyExecutionSetupResolver::max_limit_for(&dest, &fees.id)`? + let fees = fees .clone() .reanchored(&dest, context) .map_err(|_| Error::::CannotReanchor)?; - let worst_case_assets = assets.clone(); - let worst_case_weight_limit = Limited(Weight::zero()); + let assets = assets.clone(); + let weight_limit = Limited(Weight::zero()); - // try to estimate weight_limit by expected message executed on destination + // estimate weight_limit by weighing message to be executed on destination let mut remote_message = match destination_fees_setup { - DestinationFeesSetup::ByOrigin => { - // no, change, everything is like origin wanted before - Xcm(vec![ - ReserveAssetDeposited(worst_case_assets), - ClearOrigin, - BuyExecution { - fees: worst_case_fees, - weight_limit: worst_case_weight_limit, - }, - DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, - ]) - }, + DestinationFeesSetup::ByOrigin => Xcm(vec![ + ReserveAssetDeposited(assets), + ClearOrigin, + BuyExecution { fees, weight_limit }, + DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, + ]), DestinationFeesSetup::ByUniversalLocation { .. } => { let sovereign_account_on_destination = T::UniversalLocation::get() .invert_target(&dest) .map_err(|_| Error::::DestinationNotInvertible)?; Xcm(vec![ - ReserveAssetDeposited(worst_case_assets.clone()), + ReserveAssetDeposited(assets.clone()), ClearOrigin, // Change origin to sovereign account of `T::UniversalLocation` on destination. AliasOrigin(sovereign_account_on_destination), // Withdraw fees (do not use those in `ReserveAssetDeposited`) - WithdrawAsset(MultiAssets::from(worst_case_fees.clone())), + WithdrawAsset(MultiAssets::from(fees.clone())), ClearOrigin, - BuyExecution { - fees: worst_case_fees.clone(), - weight_limit: worst_case_weight_limit, - }, + BuyExecution { fees: fees.clone(), weight_limit }, RefundSurplus, // deposit unspent `fees` back to `sovereign_account` DepositAsset { - assets: MultiAssetFilter::from(MultiAssets::from(worst_case_fees)), + assets: MultiAssetFilter::from(MultiAssets::from(fees)), beneficiary: sovereign_account_on_destination, }, // deposit `assets` to beneficiary - DepositAsset { - assets: MultiAssetFilter::from(worst_case_assets), - beneficiary, - }, + DepositAsset { assets: MultiAssetFilter::from(assets), beneficiary }, ]) }, }; - // if we are going to different consensus, we need to calculate also with `UniversalOrigin/DescendOrigin` + // if message is going to different consensus, also include `UniversalOrigin/DescendOrigin` if ensure_is_remote(T::UniversalLocation::get(), dest).is_ok() { let (local_net, local_sub) = T::UniversalLocation::get() .split_global() @@ -1292,7 +1281,7 @@ impl Pallet { } } - // TODO: can we leave it here by default or adds according to some configuration? + // TODO: can we leave it here by default or should we add according to some configuration? // TODO: think about some `trait RemoteMessageEstimator` stuff, where runtime can customize this? remote_message.0.push(SetTopic([1; 32])); @@ -1303,14 +1292,14 @@ impl Pallet { }, }; - // lets handle `BuyExecution` + // handle `BuyExecution` on target chain let mut message = match destination_fees_setup { DestinationFeesSetup::ByOrigin => { - // we need to reanchor fees as dest will see it + // reanchor fees to `dest` let fees = fees.reanchored(&dest, context).map_err(|_| Error::::CannotReanchor)?; - // no, change, everything is like origin wanted before + // no change, origin asset is used to `BuyExecution` let xcm = Xcm(vec![ BuyExecution { fees, weight_limit }, DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, @@ -1321,21 +1310,21 @@ impl Pallet { ]) }, DestinationFeesSetup::ByUniversalLocation { local_account } => { - // estimate how much we want to use for `BuyExecution` and proportional amount to withdraw from origin's desired fees + // estimate how much to use for `BuyExecution` and proportional amount to withdraw from origin's assets let DestinationFees { proportional_amount_to_withdraw, proportional_amount_to_buy_execution, } = T::DestinationFeesManager::estimate_fee_for(&dest, &fees.id, &weight_limit) .ok_or(Error::::UnweighableMessage)?; - // here we need to split origin's assets (lets do some math) + // split origin's assets let original_assets = assets.clone(); let assets_for_reserve: MultiAssets = xcm_executor::Assets::from(assets) .checked_sub(proportional_amount_to_withdraw.clone()) .map_err(|_| Error::::InvalidAsset)? .into(); let assets_to_withdraw = MultiAssets::from(proportional_amount_to_withdraw); - // ensure that origin's assets are still equal to assets_for_reserve + assets_to_withdraw + // ensure that origin's assets amount to assets_for_reserve + assets_to_withdraw { let mut collected_assets = xcm_executor::Assets::from(assets_for_reserve.clone()); @@ -1346,15 +1335,13 @@ impl Pallet { Error::::InvalidAsset ); } - drop(original_assets); - drop(fees); - // we need to reanchor fees as dest will see it + // reanchor fees to `dest` let proportional_amount_to_buy_execution = proportional_amount_to_buy_execution .reanchored(&dest, context) .map_err(|_| Error::::CannotReanchor)?; - // + // reanchor paying account to `dest` let sovereign_account_on_destination = T::UniversalLocation::get() .invert_target(&dest) .map_err(|_| Error::::DestinationNotInvertible)?; From 6f7818e20d3449a7d99d37572b7573a7e29611d2 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 10 Jul 2023 13:43:50 +0300 Subject: [PATCH 10/12] pallet-xcm: split off estimate_reserve_transfer_assets_remote_weight() --- xcm/pallet-xcm/src/lib.rs | 152 ++++++++++++++++++++------------------ 1 file changed, 82 insertions(+), 70 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index 273ba38c3008..1ee352311643 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -1194,6 +1194,78 @@ impl QueryHandler for Pallet { } impl Pallet { + fn estimate_reserve_transfer_assets_remote_weight( + dest: MultiLocation, + beneficiary: MultiLocation, + assets: &[MultiAsset], + fees: &MultiAsset, + destination_fees_setup: &DestinationFeesSetup, + ) -> Result { + // TODO: estimate fees using local weigher, + // or add some `T::BuyExecutionSetupResolver::max_limit_for(&dest, &fees.id)`? + let max_assets = assets.len() as u32; + let assets: MultiAssets = assets.to_vec().into(); + let context = T::UniversalLocation::get(); + let fees = fees + .clone() + .reanchored(&dest, context) + .map_err(|_| Error::::CannotReanchor)?; + let weight_limit = Limited(Weight::zero()); + + // estimate weight_limit by weighing message to be executed on destination + let mut remote_message = match destination_fees_setup { + DestinationFeesSetup::ByOrigin => Xcm(vec![ + ReserveAssetDeposited(assets), + ClearOrigin, + BuyExecution { fees, weight_limit }, + DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, + ]), + DestinationFeesSetup::ByUniversalLocation { .. } => { + let sovereign_account_on_destination = T::UniversalLocation::get() + .invert_target(&dest) + .map_err(|_| Error::::DestinationNotInvertible)?; + Xcm(vec![ + ReserveAssetDeposited(assets.clone()), + ClearOrigin, + // Change origin to sovereign account of `T::UniversalLocation` on destination. + AliasOrigin(sovereign_account_on_destination), + // Withdraw fees (do not use those in `ReserveAssetDeposited`) + WithdrawAsset(MultiAssets::from(fees.clone())), + ClearOrigin, + BuyExecution { fees: fees.clone(), weight_limit }, + RefundSurplus, + // deposit unspent `fees` back to `sovereign_account` + DepositAsset { + assets: MultiAssetFilter::from(MultiAssets::from(fees)), + beneficiary: sovereign_account_on_destination, + }, + // deposit `assets` to beneficiary + DepositAsset { assets: MultiAssetFilter::from(assets), beneficiary }, + ]) + }, + }; + + // if message is going to different consensus, also include `UniversalOrigin/DescendOrigin` + if ensure_is_remote(T::UniversalLocation::get(), dest).is_ok() { + let (local_net, local_sub) = T::UniversalLocation::get() + .split_global() + .map_err(|_| Error::::BadLocation)?; + remote_message.0.insert(0, UniversalOrigin(GlobalConsensus(local_net))); + if local_sub != Here { + remote_message.0.insert(1, DescendOrigin(local_sub)); + } + } + + // TODO: can we leave it here by default or should we add according to some configuration? + // TODO: think about some `trait RemoteMessageEstimator` stuff, where runtime can customize this? + remote_message.0.push(SetTopic([1; 32])); + + // use local weight for remote message and hope for the best. + let remote_weight = + T::Weigher::weight(&mut remote_message).map_err(|()| Error::::UnweighableMessage)?; + Ok(Limited(remote_weight)) + } + fn do_reserve_transfer_assets( origin: OriginFor, dest: Box, @@ -1220,78 +1292,18 @@ impl Pallet { let destination_fees_setup = T::DestinationFeesManager::decide_for(&dest, &fees.id); // resolve weight_limit + let weight_limit = maybe_weight_limit.ok_or(()).or_else(|()| { + Self::estimate_reserve_transfer_assets_remote_weight( + dest, + beneficiary, + &assets, + &fees, + &destination_fees_setup, + ) + })?; + let max_assets = assets.len() as u32; let assets: MultiAssets = assets.into(); - let weight_limit = match maybe_weight_limit { - Some(weight_limit) => { - // use desired limit - weight_limit - }, - None => { - // TODO: estimate fees using local weigher, - // or add some `T::BuyExecutionSetupResolver::max_limit_for(&dest, &fees.id)`? - let fees = fees - .clone() - .reanchored(&dest, context) - .map_err(|_| Error::::CannotReanchor)?; - let assets = assets.clone(); - let weight_limit = Limited(Weight::zero()); - - // estimate weight_limit by weighing message to be executed on destination - let mut remote_message = match destination_fees_setup { - DestinationFeesSetup::ByOrigin => Xcm(vec![ - ReserveAssetDeposited(assets), - ClearOrigin, - BuyExecution { fees, weight_limit }, - DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, - ]), - DestinationFeesSetup::ByUniversalLocation { .. } => { - let sovereign_account_on_destination = T::UniversalLocation::get() - .invert_target(&dest) - .map_err(|_| Error::::DestinationNotInvertible)?; - Xcm(vec![ - ReserveAssetDeposited(assets.clone()), - ClearOrigin, - // Change origin to sovereign account of `T::UniversalLocation` on destination. - AliasOrigin(sovereign_account_on_destination), - // Withdraw fees (do not use those in `ReserveAssetDeposited`) - WithdrawAsset(MultiAssets::from(fees.clone())), - ClearOrigin, - BuyExecution { fees: fees.clone(), weight_limit }, - RefundSurplus, - // deposit unspent `fees` back to `sovereign_account` - DepositAsset { - assets: MultiAssetFilter::from(MultiAssets::from(fees)), - beneficiary: sovereign_account_on_destination, - }, - // deposit `assets` to beneficiary - DepositAsset { assets: MultiAssetFilter::from(assets), beneficiary }, - ]) - }, - }; - - // if message is going to different consensus, also include `UniversalOrigin/DescendOrigin` - if ensure_is_remote(T::UniversalLocation::get(), dest).is_ok() { - let (local_net, local_sub) = T::UniversalLocation::get() - .split_global() - .map_err(|_| Error::::BadLocation)?; - remote_message.0.insert(0, UniversalOrigin(GlobalConsensus(local_net))); - if local_sub != Here { - remote_message.0.insert(1, DescendOrigin(local_sub)); - } - } - - // TODO: can we leave it here by default or should we add according to some configuration? - // TODO: think about some `trait RemoteMessageEstimator` stuff, where runtime can customize this? - remote_message.0.push(SetTopic([1; 32])); - - // use local weight for remote message and hope for the best. - let remote_weight = T::Weigher::weight(&mut remote_message) - .map_err(|()| Error::::UnweighableMessage)?; - Limited(remote_weight) - }, - }; - // handle `BuyExecution` on target chain let mut message = match destination_fees_setup { DestinationFeesSetup::ByOrigin => { From 4e7b8f5c929421ddda344483a8c2748169760e0c Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 10 Jul 2023 14:02:48 +0300 Subject: [PATCH 11/12] pallet-xcm: minor refactor --- xcm/pallet-xcm/src/lib.rs | 32 +++++++++++++++----------------- xcm/src/v3/multiasset.rs | 2 +- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/xcm/pallet-xcm/src/lib.rs b/xcm/pallet-xcm/src/lib.rs index 1ee352311643..73fe3bc2ade9 100644 --- a/xcm/pallet-xcm/src/lib.rs +++ b/xcm/pallet-xcm/src/lib.rs @@ -1198,7 +1198,7 @@ impl Pallet { dest: MultiLocation, beneficiary: MultiLocation, assets: &[MultiAsset], - fees: &MultiAsset, + origin_fees: &MultiAsset, destination_fees_setup: &DestinationFeesSetup, ) -> Result { // TODO: estimate fees using local weigher, @@ -1206,27 +1206,21 @@ impl Pallet { let max_assets = assets.len() as u32; let assets: MultiAssets = assets.to_vec().into(); let context = T::UniversalLocation::get(); - let fees = fees + let weight_limit = Limited(Weight::zero()); + let fees = origin_fees .clone() .reanchored(&dest, context) .map_err(|_| Error::::CannotReanchor)?; - let weight_limit = Limited(Weight::zero()); - // estimate weight_limit by weighing message to be executed on destination - let mut remote_message = match destination_fees_setup { - DestinationFeesSetup::ByOrigin => Xcm(vec![ - ReserveAssetDeposited(assets), - ClearOrigin, - BuyExecution { fees, weight_limit }, - DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }, - ]), + let buy_execution_on_dest = match destination_fees_setup { + DestinationFeesSetup::ByOrigin => { + vec![BuyExecution { fees, weight_limit }] + }, DestinationFeesSetup::ByUniversalLocation { .. } => { let sovereign_account_on_destination = T::UniversalLocation::get() .invert_target(&dest) .map_err(|_| Error::::DestinationNotInvertible)?; - Xcm(vec![ - ReserveAssetDeposited(assets.clone()), - ClearOrigin, + vec![ // Change origin to sovereign account of `T::UniversalLocation` on destination. AliasOrigin(sovereign_account_on_destination), // Withdraw fees (do not use those in `ReserveAssetDeposited`) @@ -1239,12 +1233,16 @@ impl Pallet { assets: MultiAssetFilter::from(MultiAssets::from(fees)), beneficiary: sovereign_account_on_destination, }, - // deposit `assets` to beneficiary - DepositAsset { assets: MultiAssetFilter::from(assets), beneficiary }, - ]) + ] }, }; + let mut remote_message = Xcm(vec![ReserveAssetDeposited(assets), ClearOrigin]); + remote_message.0.extend_from_slice(&buy_execution_on_dest); + remote_message + .0 + .push(DepositAsset { assets: Wild(AllCounted(max_assets)), beneficiary }); + // if message is going to different consensus, also include `UniversalOrigin/DescendOrigin` if ensure_is_remote(T::UniversalLocation::get(), dest).is_ok() { let (local_net, local_sub) = T::UniversalLocation::get() diff --git a/xcm/src/v3/multiasset.rs b/xcm/src/v3/multiasset.rs index 06cd2c8b5b82..eefb0c279099 100644 --- a/xcm/src/v3/multiasset.rs +++ b/xcm/src/v3/multiasset.rs @@ -406,7 +406,7 @@ pub struct MultiAsset { /// The overall asset identity (aka *class*, in the case of a non-fungible). pub id: AssetId, /// The fungibility of the asset, which contains either the amount (in the case of a fungible - /// asset) or the *insance ID`, the secondary asset identifier. + /// asset) or the `instance ID`, the secondary asset identifier. pub fun: Fungibility, } From b26da28d31d2903782d670ac6ed2248ebc591fca Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Sat, 15 Jul 2023 07:04:42 +0200 Subject: [PATCH 12/12] Ensure origin for AliasOrigin in barrier --- xcm/xcm-builder/src/barriers.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xcm/xcm-builder/src/barriers.rs b/xcm/xcm-builder/src/barriers.rs index 3ad5bbb1d13f..39610faa1f9a 100644 --- a/xcm/xcm-builder/src/barriers.rs +++ b/xcm/xcm-builder/src/barriers.rs @@ -140,7 +140,8 @@ impl> ShouldExecute for AllowAliasOriginWithdrawPaidE .skip_inst_while(|inst| matches!(inst, ClearOrigin))? // TODO:check-parameter - this is only difference with `AllowTopLevelPaidExecutionFrom` - check for AliasOrigin / WithdrawAsset / ClearOrigin .match_next_inst(|inst| match inst { - AliasOrigin(_) => Ok(()), + // we allow reset origin by AliasOrigin, only iff equals (computed) `origin` here. + AliasOrigin(alias_origin) if alias_origin == origin => Ok(()), _ => Err(ProcessMessageError::BadFormat), })? // TODO:check-parameter - this is only difference with `AllowTopLevelPaidExecutionFrom` - check for AliasOrigin / WithdrawAsset / ClearOrigin