Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Pay XCM execution with local and foreign assets #2854

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 27 additions & 9 deletions parachains/runtimes/assets/asset-hub-polkadot/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,10 @@ fn test_asset_xcm_trader() {
let mut trader = <XcmConfig as xcm_executor::Config>::Trader::new();

// Lets buy_weight and make sure buy_weight does not return an error
let unused_assets = trader.buy_weight(bought, asset.into()).expect("Expected Ok");
let unused_assets = trader
.buy_weight(&XcmContext::with_message_id([0; 32]), bought, asset.into())
.expect("Expected Ok");

// Check whether a correct amount of unused assets is returned
assert_ok!(
unused_assets.ensure_contains(&(asset_multilocation, asset_amount_extra).into())
Expand Down Expand Up @@ -184,12 +187,14 @@ fn test_asset_xcm_trader_with_refund() {

let asset: MultiAsset = (asset_multilocation, amount_bought).into();

let ctx = XcmContext::with_message_id([0; 32]);

// Make sure buy_weight does not return an error
assert_ok!(trader.buy_weight(bought, asset.clone().into()));
assert_ok!(trader.buy_weight(&ctx, bought, asset.clone().into()));

// Make sure again buy_weight does return an error
// This assert relies on the fact, that we use `TakeFirstAssetTrader` in `WeightTrader` tuple chain, which cannot be called twice
assert_noop!(trader.buy_weight(bought, asset.into()), XcmError::TooExpensive);
assert_noop!(trader.buy_weight(&ctx, bought, asset.into()), XcmError::TooExpensive);

// We actually use half of the weight
let weight_used = bought / 2;
Expand All @@ -198,7 +203,7 @@ fn test_asset_xcm_trader_with_refund() {
let amount_refunded = WeightToFee::weight_to_fee(&(bought - weight_used));

assert_eq!(
trader.refund_weight(bought - weight_used),
trader.refund_weight(&ctx, bought - weight_used),
Some((asset_multilocation, amount_refunded).into())
);

Expand Down Expand Up @@ -262,7 +267,10 @@ fn test_asset_xcm_trader_refund_not_possible_since_amount_less_than_ed() {
let asset: MultiAsset = (asset_multilocation, amount_bought).into();

// Buy weight should return an error
assert_noop!(trader.buy_weight(bought, asset.into()), XcmError::TooExpensive);
assert_noop!(
trader.buy_weight(&XcmContext::with_message_id([0; 32]), bought, asset.into()),
XcmError::TooExpensive
);

// not credited since the ED is higher than this value
assert_eq!(Assets::balance(1, AccountId::from(ALICE)), 0);
Expand Down Expand Up @@ -313,17 +321,24 @@ fn test_that_buying_ed_refund_does_not_refund() {
// We know we will have to buy at least ED, so lets make sure first it will
// fail with a payment of less than ED
let asset: MultiAsset = (asset_multilocation, amount_bought).into();
assert_noop!(trader.buy_weight(bought, asset.into()), XcmError::TooExpensive);
assert_noop!(
trader.buy_weight(&XcmContext::with_message_id([0; 32]), bought, asset.into()),
XcmError::TooExpensive
);

// Now lets buy ED at least
let asset: MultiAsset = (asset_multilocation, ExistentialDeposit::get()).into();

// Buy weight should work
assert_ok!(trader.buy_weight(bought, asset.into()));
assert_ok!(trader.buy_weight(
&XcmContext::with_message_id([0; 32]),
bought,
asset.into()
));

// Should return None. We have a specific check making sure we dont go below ED for
// drop payment
assert_eq!(trader.refund_weight(bought), None);
assert_eq!(trader.refund_weight(&XcmContext::with_message_id([0; 32]), bought), None);

// Drop trader
drop(trader);
Expand Down Expand Up @@ -384,7 +399,10 @@ fn test_asset_xcm_trader_not_possible_for_non_sufficient_assets() {
let asset: MultiAsset = (asset_multilocation, asset_amount_needed).into();

// Make sure again buy_weight does return an error
assert_noop!(trader.buy_weight(bought, asset.into()), XcmError::TooExpensive);
assert_noop!(
trader.buy_weight(&XcmContext::with_message_id([0; 32]), bought, asset.into()),
XcmError::TooExpensive
);

// Drop trader
drop(trader);
Expand Down
1 change: 1 addition & 0 deletions parachains/runtimes/assets/asset-hub-westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1224,6 +1224,7 @@ impl_runtime_apis! {
Ok(WestendLocation::get())
}
fn worst_case_holding(depositable_count: u32) -> MultiAssets {

// A mix of fungible, non-fungible, and concrete assets.
let holding_non_fungibles = MaxAssetsIntoHolding::get() / 2 - depositable_count;
let holding_fungibles = holding_non_fungibles - 1;
Expand Down
30 changes: 25 additions & 5 deletions parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use assets_common::{
matching::{
FromSiblingParachain, IsForeignConcreteAsset, StartsWith, StartsWithExplicitGlobalConsensus,
},
AssetIdForTrustBackedAssetsConvert,
};
use frame_support::{
match_types, parameter_types,
Expand All @@ -46,6 +47,7 @@ use xcm_builder::{
};
use xcm_executor::{traits::WithOriginFilter, XcmExecutor};

use assets_common::local_and_foreign_assets::LocalAndForeignAssets;
#[cfg(feature = "runtime-benchmarks")]
use {cumulus_primitives_core::ParaId, sp_core::Get};

Expand Down Expand Up @@ -125,6 +127,17 @@ pub type ForeignAssetsConvertedConcreteId = assets_common::ForeignAssetsConverte
Balance,
>;

/// `AssetId/Balance` converter for pay by swap
pub type LocalAndForeignAssetsConvertedConcreteId = assets_common::ForeignAssetsConvertedConcreteId<
Copy link
Contributor

@bkontur bkontur Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is good way and if it covers everything: Local and Foreign... it is kind of hacky.

what was wrong with two SwapFirstAssetTrader instance configured with Assets and ForeignAssets with reused verified converters?

SwapFirstAssetTrader<Assets, TrustBackedAssetsConvertedConcreteId>
SwapFirstAssetTrader<ForeignAssets, ForeignAssetsConvertedConcreteId>

xcm uses everywhere tuple implementation, e.g. check AssetTransactors, it is much clear approach,
maybe in future we would like to add another pallet_assets instance, then what? LocalAndForeignAndWhateverAssetsConvertedConcreteId + LocalAndForeignAndWhateverAssets ?
I guess easier/nicer is to add another SwapFirstAssetTrader
like we do for AssetTransactors - we just add new instance:

pub type AssetTransactors =
	(CurrencyTransactor, FungiblesTransactor, ForeignFungiblesTransactor, PoolFungiblesTransactor);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. I think a lot of the problem originates in the SignedExtension for fee payment. The LocalOrForeign struct basically serves as the router, to figure out which instance of the Assets pallet the input refers to. But I don't think we need to do that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try using two SwapFirstAssetTrader instances for now and if and when LocalAndForeignAssets gets conformance tests, we can switch to that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I would not switch back to LocalAndForeignAssets here :),
as Joe said LocalAndForeignAssets is ok/needed for SignedExtension, but here is much clear to have multiple SwapFirstAssetTrader

Copy link
Contributor Author

@PatricioNapoli PatricioNapoli Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the Assets one, how do I fulfill the bound in SwapFirstAssetTrader that is: MultiLocation: From<ConcreteAssets::AssetId> + Into<T::MultiAssetId>

Here's the constraint

The question is explicitly, what would be the right MatchedConvertedConcreteId (asset matcher) type to pass in here? Or else, what would the right bound be in SwapFirstAssetTrader for using the regular Asset's ID in the swap fn?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PatricioNapoli
for Assets -> xcm_config::TrustBackedAssetsConvertedConcreteId
for ForeignAssets -> xcm_config::ForeignAssetsConvertedConcreteId

and for that constraint, I tried it (does not compile because of XcmContext), but something like this should work:
#2955

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkontur

Does not currently compile for the Local assets trader, using TrustBackedAssetsConvertedConcreteId

error is:

required for `u32` to implement `Into<std::boxed::Box<cumulus_primitives_core::MultiLocation>>`

latest changes are pushed

(
// Ignore asset which starts explicitly with our `GlobalConsensus(NetworkId)`, means:
// - foreign assets from our consensus should be: `MultiLocation {parents: 1, X*(Parachain(xyz), ..)}
// - foreign assets outside our consensus with the same `GlobalConsensus(NetworkId)` wont be accepted here
StartsWithExplicitGlobalConsensus<UniversalLocationNetworkId>,
),
Balance,
>;

/// Means for transacting foreign assets from different global consensus.
pub type ForeignFungiblesTransactor = FungiblesAdapter<
// Use this fungibles implementation:
Expand Down Expand Up @@ -469,12 +482,19 @@ impl xcm_executor::Config for XcmConfig {
>;
type Trader = (
UsingComponents<WeightToFee, WestendLocation, AccountId, Balances, ToStakingPot<Runtime>>,
cumulus_primitives_utility::TakeFirstAssetTrader<
AccountId,
AssetFeeAsExistentialDepositMultiplierFeeCharger,
Copy link
Contributor

@muharem muharem Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understand is, that we can use the same TakeFirstAssetTrader and only provide a different BalanceToAssetBalance converter for AssetFeeAsExistentialDepositMultiplierFeeCharger type.
The converter which does the conversion based on the price provided by the pool, instead of min balances ration.
The rest can probably be reused.

One important thing is that the Trader is not supposed to perform any transfers/swaps, but only subtract the amount from the payment it receives as an argument, place it into its own register to be paid later on drop and return the unused, the executor will place unused back to its holding register.
The payment received as an argument is a credit from the executor's holding register, which was already withdrawn from the user's account.
I am not really an expert in this topic, but this how I understanding it after reading the executor codebase and it make sense to me, please validate.

TrustBackedAssetsConvertedConcreteId,
Assets,
cumulus_primitives_utility::SwapFirstAssetTrader<
PatricioNapoli marked this conversation as resolved.
Show resolved Hide resolved
Runtime,
LocationToAccountId,
pallet_asset_conversion::Pallet<Runtime>,
WeightToFee,
LocalAndForeignAssetsConvertedConcreteId,
LocalAndForeignAssets<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also LocalAndForeignAssets facade implementation for Assets / ForeignAssets is kind of questionable if it is 100% correct,
here I have two things:

  1. everywhere there is a IF if isLocal then Assets else ForeignAssets, but if we use some PoolAssets asset, it will fall to ForeignAssets branch and yes happily it fails, but it is not 100% bullet proof this if-else way -> another vote for tuple implementation

  2. it implements Unbalanced / Balanced traits, e.g. I had a bug with that, because it was missing overridden implementation for decrease_balance / increase_balance, which caused bug for benchmarks:

bparity@bkontur-ThinkPad-P14s-Gen-2i:~/parity/cumulus/cumulus$ ARTIFACTS_DIR=tmp2 ../../command-bot-scripts/commands/bench/lib/bench-all-cumulus.sh 
[+] Compiling benchmarks...
[+] Listing pallets for runtime asset-hub-westend for chain: asset-hub-westend-dev ...
[+] Benchmarking 17 pallets for runtime asset-hub-westend for chain: asset-hub-westend-dev, pallets:
   [+] cumulus_pallet_xcmp_queue
   [+] frame_system
   [+] pallet_asset_conversion
   [+] pallet_assets
   [+] pallet_balances
   [+] pallet_collator_selection
   [+] pallet_multisig
   [+] pallet_nft_fractionalization
   [+] pallet_nfts
   [+] pallet_proxy
   [+] pallet_session
   [+] pallet_timestamp
   [+] pallet_uniques
   [+] pallet_utility
   [+] pallet_xcm
   [+] pallet_xcm_benchmarks::fungible
   [+] pallet_xcm_benchmarks::generic
2023-07-12 10:20:15 a defensive failure has been triggered; please report the block number at https://github.com/paritytech/substrate/issues: "write_balance is not used if other functions are impl'd"    
2023-07-12 10:20:15 panicked at 'Expected Ok(_). Got Err(
    Unavailable,
)', /home/bparity/.cargo/git/checkouts/substrate-7e08433d4c370a21/edf58dc/frame/asset-conversion/src/benchmarking.rs:60:9   

it took me a while to find a issue with this missing overridden implementations b8f86cf

and I am afraid that there could be possible other hidden bugs (maybe not),
so if we wanna go with this LocalAndForeignAssets we have to add there at least substrate conformance tests or whatever to ensure that LocalAndForeignAssets is 100% compatible with Assets and ForeignAssets

Assets,
AssetIdForTrustBackedAssetsConvert<TrustBackedAssetsPalletLocation>,
ForeignAssets,
>,
cumulus_primitives_utility::XcmFeesTo32ByteAccount<
// Revenue could also be Foreign Fungible? Maybe with multi-asset treasury..?
Copy link
Contributor

@joepetrowski joepetrowski Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, although Treasury here is just a normal AccountId. But since the execution can be paid in local or foreign fungible assets, they'll need to be deposit-able in their destination account.

FungiblesTransactor,
AccountId,
XcmAssetFeesReceiver,
Expand Down
2 changes: 2 additions & 0 deletions primitives/utility/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ log = { version = "0.4.19", default-features = false }

# Substrate
frame-support = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
pallet-asset-conversion = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
sp-io = { 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-std = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
Expand All @@ -29,6 +30,7 @@ default = [ "std" ]
std = [
"codec/std",
"frame-support/std",
"pallet-asset-conversion/std",
"sp-runtime/std",
"sp-std/std",
"sp-io/std",
Expand Down
166 changes: 160 additions & 6 deletions primitives/utility/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,16 @@ use frame_support::{
},
weights::Weight,
};
use pallet_asset_conversion::{Config, MultiAssetIdConverter, Swap};
use polkadot_runtime_common::xcm_sender::ConstantPrice;
use sp_runtime::{traits::Saturating, SaturatedConversion};
use sp_runtime::{
traits::{CheckedSub, Saturating},
SaturatedConversion,
};
use sp_std::{marker::PhantomData, prelude::*};
use xcm::{latest::prelude::*, WrapVersion};
use xcm_builder::TakeRevenue;
use xcm_executor::traits::{MatchesFungibles, TransactAsset, WeightTrader};
use xcm_executor::traits::{ConvertLocation, MatchesFungibles, TransactAsset, WeightTrader};

pub trait PriceForParentDelivery {
fn price_for_parent_delivery(message: &Xcm<()>) -> MultiAssets;
Expand Down Expand Up @@ -144,6 +148,7 @@ impl<
// If everything goes well, we charge.
fn buy_weight(
&mut self,
_: &XcmContext,
weight: Weight,
payment: xcm_executor::Assets,
) -> Result<xcm_executor::Assets, XcmError> {
Expand Down Expand Up @@ -196,7 +201,7 @@ impl<
Ok(unused)
}

fn refund_weight(&mut self, weight: Weight) -> Option<MultiAsset> {
fn refund_weight(&mut self, _ctx: &XcmContext, weight: Weight) -> Option<MultiAsset> {
log::trace!(target: "xcm::weight", "TakeFirstAssetTrader::refund_weight weight: {:?}", weight);
if let Some(AssetTraderRefunder {
mut weight_outstanding,
Expand Down Expand Up @@ -269,8 +274,150 @@ impl<
}
}

/// Contains information to handle refund/payment for xcm-execution
#[derive(Clone, Eq, PartialEq, Debug)]
struct SwapAssetTraderRefunder {
// The concrete asset containing the asset location and any leftover balance
outstanding_concrete_asset: MultiAsset,
}

pub struct SwapFirstAssetTrader<
T: Config,
AccountIdConverter: ConvertLocation<T::AccountId>,
SWP: Swap<T::AccountId, T::HigherPrecisionBalance, T::MultiAssetId>,
WeightToFee: frame_support::weights::WeightToFee<Balance = T::Balance>,
Matcher: MatchesFungibles<ConcreteAssets::AssetId, ConcreteAssets::Balance>,
ConcreteAssets: fungibles::Mutate<T::AccountId> + fungibles::Balanced<T::AccountId>,
HandleRefund: TakeRevenue,
>(
Option<SwapAssetTraderRefunder>,
PhantomData<(T, AccountIdConverter, SWP, WeightToFee, Matcher, ConcreteAssets, HandleRefund)>,
);

impl<
T: Config,
AccountIdConverter: ConvertLocation<T::AccountId>,
SWP: Swap<T::AccountId, T::HigherPrecisionBalance, T::MultiAssetId>,
WeightToFee: frame_support::weights::WeightToFee<Balance = T::Balance>,
Matcher: MatchesFungibles<ConcreteAssets::AssetId, ConcreteAssets::Balance>,
ConcreteAssets: fungibles::Mutate<T::AccountId> + fungibles::Balanced<T::AccountId>,
HandleRefund: TakeRevenue,
> WeightTrader
for SwapFirstAssetTrader<
T,
AccountIdConverter,
SWP,
WeightToFee,
Matcher,
ConcreteAssets,
HandleRefund,
> where
T::HigherPrecisionBalance: From<ConcreteAssets::Balance> + TryInto<T::AssetBalance>,
<T::HigherPrecisionBalance as TryInto<T::AssetBalance>>::Error: core::fmt::Debug,
T::AssetBalance: Into<Fungibility>,
MultiLocation: From<ConcreteAssets::AssetId> + Into<T::MultiAssetId>,
{
fn new() -> Self {
Self(None, PhantomData)
}

// We take the first multiasset present in the payment
// Check whether we can swap the payment tokens using asset_conversion pallet
// If swap can be applied, we execute it
// On successful swap, we substract from the payment
fn buy_weight(
&mut self,
ctx: &XcmContext,
weight: Weight,
payment: xcm_executor::Assets,
) -> Result<xcm_executor::Assets, XcmError> {
log::trace!(target: "xcm::weight", "SwapFirstAssetTrader::buy_weight weight: {:?}, payment: {:?}", weight, payment);

// Make sure we dont enter twice
if self.0.is_some() {
return Err(XcmError::NotWithdrawable)
}

// We take the very first multiasset from payment
// (assets are sorted by fungibility/amount after this conversion)
let multiassets: MultiAssets = payment.clone().into();

// Take the first multiasset from the selected MultiAssets
let first = multiassets.get(0).ok_or(XcmError::AssetNotFound)?;

// Get the asset id in which we can pay for fees
let (asset_id, local_asset_balance) =
Matcher::matches_fungibles(first).map_err(|_| XcmError::AssetNotFound)?;

let asset_loc: MultiLocation = asset_id.into();

let fee = WeightToFee::weight_to_fee(&weight);

let origin = ctx.origin.ok_or(XcmError::BadOrigin)?;
let acc = AccountIdConverter::convert_location(&origin).ok_or(XcmError::InvalidLocation)?;

let amount_taken = SWP::swap_tokens_for_exact_tokens(
acc.clone(),
vec![asset_loc.into(), T::MultiAssetIdConverter::get_native()],
fee.into(),
None,
acc.clone(),
true,
)
.map_err(|_| XcmError::AssetNotFound)?;

// Substract amount_taken from payment_balance
let unused = T::HigherPrecisionBalance::from(local_asset_balance)
.checked_sub(&amount_taken)
.ok_or(XcmError::TooExpensive)?;

let unused_balance: T::AssetBalance = unused.try_into().map_err(|_| XcmError::Overflow)?;

// Record outstanding asset
self.0 = Some(SwapAssetTraderRefunder { outstanding_concrete_asset: first.clone() });

Ok(first.id.into_multiasset(unused_balance.into()).into())
}

fn refund_weight(&mut self, ctx: &XcmContext, weight: Weight) -> Option<MultiAsset> {
log::trace!(target: "xcm::weight", "SwapFirstAssetTrader::refund_weight weight: {:?}", weight);

// TODO: swap back the weight with swap_exact_tokens_for_tokens

None
}
}

impl<
T: Config,
AccountIdConverter: ConvertLocation<T::AccountId>,
SWP: Swap<T::AccountId, T::HigherPrecisionBalance, T::MultiAssetId>,
WeightToFee: frame_support::weights::WeightToFee<Balance = T::Balance>,
Matcher: MatchesFungibles<ConcreteAssets::AssetId, ConcreteAssets::Balance>,
ConcreteAssets: fungibles::Mutate<T::AccountId> + fungibles::Balanced<T::AccountId>,
HandleRefund: TakeRevenue,
> Drop
for SwapFirstAssetTrader<
T,
AccountIdConverter,
SWP,
WeightToFee,
Matcher,
ConcreteAssets,
HandleRefund,
>
{
fn drop(&mut self) {
if let Some(asset_trader) = self.0.clone() {
HandleRefund::take_revenue(asset_trader.outstanding_concrete_asset);
}
}
}

/// XCM fee depositor to which we implement the TakeRevenue trait
/// It receives a Transact implemented argument, a 32 byte convertible acocuntId, and the fee receiver account
/// It receives a Transact implemented argument, a 32 byte convertible accountId,
/// and the fee receiver account
///
/// FungiblesMutateAdapter should be identical to that implemented by WithdrawAsset
pub struct XcmFeesTo32ByteAccount<FungiblesMutateAdapter, AccountId, ReceiverAccount>(
PhantomData<(FungiblesMutateAdapter, AccountId, ReceiverAccount)>,
Expand Down Expand Up @@ -533,9 +680,16 @@ mod tests {
let weight_to_buy = Weight::from_parts(1_000, 1_000);

// lets do first call (success)
assert_ok!(trader.buy_weight(weight_to_buy, payment.clone()));
assert_ok!(trader.buy_weight(
&XcmContext::with_message_id([0; 32]),
weight_to_buy,
payment.clone()
));

// lets do second call (error)
assert_eq!(trader.buy_weight(weight_to_buy, payment), Err(XcmError::NotWithdrawable));
assert_eq!(
trader.buy_weight(&XcmContext::with_message_id([0; 32]), weight_to_buy, payment),
Err(XcmError::NotWithdrawable)
);
}
}
Loading