Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XcmPaymentApi: take into account AH sufficient assets #6033

Closed
wants to merge 1 commit into from
Closed

XcmPaymentApi: take into account AH sufficient assets #6033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 38 additions & 7 deletions cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use frame_support::{
ord_parameter_types, parameter_types,
traits::{
fungible, fungibles,
tokens::{imbalance::ResolveAssetTo, nonfungibles_v2::Inspect},
tokens::{imbalance::ResolveAssetTo, nonfungibles_v2::Inspect, ConversionToAssetBalance},
AsEnsureOriginWithArg, ConstBool, ConstU128, ConstU32, ConstU64, ConstU8, InstanceFilter,
TransformOrigin,
},
Expand All @@ -67,7 +67,10 @@ use sp_api::impl_runtime_apis;
use sp_core::{crypto::KeyTypeId, OpaqueMetadata};
use sp_runtime::{
create_runtime_str, generic, impl_opaque_keys,
traits::{AccountIdConversion, BlakeTwo256, Block as BlockT, Saturating, Verify},
traits::{
AccountIdConversion, BlakeTwo256, Block as BlockT, ConvertInto, MaybeEquivalence,
Saturating, Verify,
},
transaction_validity::{TransactionSource, TransactionValidity},
ApplyExtrinsicResult, Perbill, Permill, RuntimeDebug,
};
Expand Down Expand Up @@ -1178,6 +1181,12 @@ mod benches {
);
}

pub type NativeToAssets =
pallet_assets::BalanceToAssetBalance<Balances, Runtime, ConvertInto, TrustBackedAssetsInstance>;

pub type NativeToForeignAssets =
pallet_assets::BalanceToAssetBalance<Balances, Runtime, ConvertInto, ForeignAssetsInstance>;

impl_runtime_apis! {
impl sp_consensus_aura::AuraApi<Block, AuraId> for Runtime {
fn slot_duration() -> sp_consensus_aura::SlotDuration {
Expand Down Expand Up @@ -1367,19 +1376,41 @@ impl_runtime_apis! {

impl xcm_runtime_apis::fees::XcmPaymentApi<Block> for Runtime {
fn query_acceptable_payment_assets(xcm_version: xcm::Version) -> Result<Vec<VersionedAssetId>, XcmPaymentApiError> {
let acceptable_assets = vec![AssetId(xcm_config::WestendLocation::get())];
use xcm::prelude::Junction::*;

let assets = Assets::sufficient_assets()
.filter_map(|asset_id| TrustBackedAssetsPalletLocation::get().appended_with(GeneralIndex(asset_id.into())).ok().map(AssetId));
let foreign_assets = ForeignAssets::sufficient_assets()
.map(AssetId);
Comment on lines +1381 to +1384
Copy link
Contributor

@acatangiu acatangiu Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Asset sufficiency doesn't mean it can be used to buy weight, i.e. directly pay for fees.

Asset sufficiency only means the asset is sufficient for the account to exist, i.e. you doesn't require DOT ED on that account to hold a sufficient asset.

This code is incorrect as it will return assets as suitable for directly paying fees which is not the case. I suggest you always add a simple emulator test to PoC any XCM config change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Asset sufficiency doesn't mean it can be used to buy weight, i.e. directly pay for fees.

Hmm, there are comments in the code stating the opposite...

polkadot-sdk/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs

Lines 421 to 446 in d1c115b

// This trader allows to pay with `is_sufficient=true` "Trust Backed" assets from dedicated
// `pallet_assets` instance - `Assets`.
cumulus_primitives_utility::TakeFirstAssetTrader<
AccountId,
AssetFeeAsExistentialDepositMultiplierFeeCharger,
TrustBackedAssetsConvertedConcreteId,
Assets,
cumulus_primitives_utility::XcmFeesTo32ByteAccount<
FungiblesTransactor,
AccountId,
XcmAssetFeesReceiver,
>,
>,
// This trader allows to pay with `is_sufficient=true` "Foreign" assets from dedicated
// `pallet_assets` instance - `ForeignAssets`.
cumulus_primitives_utility::TakeFirstAssetTrader<
AccountId,
ForeignAssetFeeAsExistentialDepositMultiplierFeeCharger,
ForeignAssetsConvertedConcreteId,
ForeignAssets,
cumulus_primitives_utility::XcmFeesTo32ByteAccount<
ForeignFungiblesTransactor,
AccountId,
XcmAssetFeesReceiver,
>,
>,

Also, I ran Westend AssetHub locally. I haven't set up the pools, but sufficient assets (both from Assets and ForeignAssets) worked as XCM execution fees. The code under the linker above provides this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are even tests for that:

  • ForeignAssets:

    polkadot-sdk/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs

    Lines 489 to 568 in d1c115b

    #[test]
    fn test_foreign_asset_xcm_take_first_trader() {
    ExtBuilder::<Runtime>::default()
    .with_collators(vec![AccountId::from(ALICE)])
    .with_session_keys(vec![(
    AccountId::from(ALICE),
    AccountId::from(ALICE),
    SessionKeys { aura: AuraId::from(sp_core::sr25519::Public::from_raw(ALICE)) },
    )])
    .build()
    .execute_with(|| {
    // We need root origin to create a sufficient asset
    let minimum_asset_balance = 3333333_u128;
    let foreign_location = xcm::v4::Location {
    parents: 1,
    interior: (
    xcm::v4::Junction::Parachain(1234),
    xcm::v4::Junction::GeneralIndex(12345),
    )
    .into(),
    };
    assert_ok!(ForeignAssets::force_create(
    RuntimeHelper::root_origin(),
    foreign_location.clone().into(),
    AccountId::from(ALICE).into(),
    true,
    minimum_asset_balance
    ));
    // We first mint enough asset for the account to exist for assets
    assert_ok!(ForeignAssets::mint(
    RuntimeHelper::origin_of(AccountId::from(ALICE)),
    foreign_location.clone().into(),
    AccountId::from(ALICE).into(),
    minimum_asset_balance
    ));
    let asset_location_v4: Location = foreign_location.clone().try_into().unwrap();
    // Set Alice as block author, who will receive fees
    RuntimeHelper::run_to_block(2, AccountId::from(ALICE));
    // We are going to buy 4e9 weight
    let bought = Weight::from_parts(4_000_000_000u64, 0);
    // Lets calculate amount needed
    let asset_amount_needed = ForeignAssetFeeAsExistentialDepositMultiplierFeeCharger::charge_weight_in_fungibles(foreign_location.clone(), bought)
    .expect("failed to compute");
    // Lets pay with: asset_amount_needed + asset_amount_extra
    let asset_amount_extra = 100_u128;
    let asset: Asset =
    (asset_location_v4.clone(), asset_amount_needed + asset_amount_extra).into();
    let mut trader = <XcmConfig as xcm_executor::Config>::Trader::new();
    let ctx = XcmContext { origin: None, message_id: XcmHash::default(), topic: None };
    // Lets buy_weight and make sure buy_weight does not return an error
    let unused_assets = trader.buy_weight(bought, asset.into(), &ctx).expect("Expected Ok");
    // Check whether a correct amount of unused assets is returned
    assert_ok!(
    unused_assets.ensure_contains(&(asset_location_v4, asset_amount_extra).into())
    );
    // Drop trader
    drop(trader);
    // Make sure author(Alice) has received the amount
    assert_eq!(
    ForeignAssets::balance(foreign_location.clone(), AccountId::from(ALICE)),
    minimum_asset_balance + asset_amount_needed
    );
    // We also need to ensure the total supply increased
    assert_eq!(
    ForeignAssets::total_supply(foreign_location),
    minimum_asset_balance + asset_amount_needed
    );
    });
    }
  • Assets:

    polkadot-sdk/cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs

    Lines 411 to 487 in d1c115b

    #[test]
    fn test_asset_xcm_take_first_trader() {
    ExtBuilder::<Runtime>::default()
    .with_collators(vec![AccountId::from(ALICE)])
    .with_session_keys(vec![(
    AccountId::from(ALICE),
    AccountId::from(ALICE),
    SessionKeys { aura: AuraId::from(sp_core::sr25519::Public::from_raw(ALICE)) },
    )])
    .build()
    .execute_with(|| {
    // We need root origin to create a sufficient asset
    let minimum_asset_balance = 3333333_u128;
    let local_asset_id = 1;
    assert_ok!(Assets::force_create(
    RuntimeHelper::root_origin(),
    local_asset_id.into(),
    AccountId::from(ALICE).into(),
    true,
    minimum_asset_balance
    ));
    // We first mint enough asset for the account to exist for assets
    assert_ok!(Assets::mint(
    RuntimeHelper::origin_of(AccountId::from(ALICE)),
    local_asset_id.into(),
    AccountId::from(ALICE).into(),
    minimum_asset_balance
    ));
    // get asset id as location
    let asset_location =
    AssetIdForTrustBackedAssetsConvert::convert_back(&local_asset_id).unwrap();
    // Set Alice as block author, who will receive fees
    RuntimeHelper::run_to_block(2, AccountId::from(ALICE));
    // We are going to buy 4e9 weight
    let bought = Weight::from_parts(4_000_000_000u64, 0);
    // Lets calculate amount needed
    let asset_amount_needed =
    AssetFeeAsExistentialDepositMultiplierFeeCharger::charge_weight_in_fungibles(
    local_asset_id,
    bought,
    )
    .expect("failed to compute");
    // Lets pay with: asset_amount_needed + asset_amount_extra
    let asset_amount_extra = 100_u128;
    let asset: Asset =
    (asset_location.clone(), asset_amount_needed + asset_amount_extra).into();
    let mut trader = <XcmConfig as xcm_executor::Config>::Trader::new();
    let ctx = XcmContext { origin: None, message_id: XcmHash::default(), topic: None };
    // Lets buy_weight and make sure buy_weight does not return an error
    let unused_assets = trader.buy_weight(bought, asset.into(), &ctx).expect("Expected Ok");
    // Check whether a correct amount of unused assets is returned
    assert_ok!(unused_assets.ensure_contains(&(asset_location, asset_amount_extra).into()));
    // Drop trader
    drop(trader);
    // Make sure author(Alice) has received the amount
    assert_eq!(
    Assets::balance(local_asset_id, AccountId::from(ALICE)),
    minimum_asset_balance + asset_amount_needed
    );
    // We also need to ensure the total supply increased
    assert_eq!(
    Assets::total_supply(local_asset_id),
    minimum_asset_balance + asset_amount_needed
    );
    });
    }

Copy link
Contributor

@acatangiu acatangiu Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ah yes, it is unfortunate that the TakeFirstAssetTrader is still there.. afaik the plan was to remove it once we have some pools set up and can rely on the one above it SwapFirstAssetTrader.

The reason to remote TakeFirstAssetTrader is that it doesn't (and can't) correctly account the economic value of weight. Depending on market movements buying weight with sufficient assets can be abused to potentially severely undercut fees.

AFAIK the plan still is to remove TakeFirstAssetTrader from system chains.

(opened polkadot-fellows/runtimes#475 to track this)


let acceptable_assets = core::iter::once(AssetId(xcm_config::WestendLocation::get()))
.chain(assets)
.chain(foreign_assets)
.collect::<Vec<_>>();

PolkadotXcm::query_acceptable_payment_assets(xcm_version, acceptable_assets)
}

fn query_weight_to_asset_fee(weight: Weight, asset: VersionedAssetId) -> Result<u128, XcmPaymentApiError> {
let native_fee = WeightToFee::weight_to_fee(&weight);

match asset.try_as::<AssetId>() {
Ok(asset_id) if asset_id.0 == xcm_config::WestendLocation::get() => {
// for native token
Ok(WeightToFee::weight_to_fee(&weight))
Ok(native_fee)
},
Ok(asset_id) => {
log::trace!(target: "xcm::xcm_runtime_apis", "query_weight_to_asset_fee - unhandled asset_id: {asset_id:?}!");
Err(XcmPaymentApiError::AssetNotFound)
Ok(AssetId(location)) => {
if let Some(asset_id) = <AssetIdForTrustBackedAssetsConvert<TrustBackedAssetsPalletLocation>>::convert(location) {
let sufficient_asset_fee = NativeToAssets::to_asset_balance(native_fee, asset_id)
.map_err(|_| XcmPaymentApiError::WeightNotComputable)?;

return Ok(sufficient_asset_fee);
}

let foreign_asset_fee = NativeToForeignAssets::to_asset_balance(native_fee, location.clone())
.map_err(|_| XcmPaymentApiError::WeightNotComputable)?;

Ok(foreign_asset_fee)
},
Err(_) => {
log::trace!(target: "xcm::xcm_runtime_apis", "query_weight_to_asset_fee - failed to convert asset: {asset:?}!");
Expand Down
5 changes: 5 additions & 0 deletions substrate/frame/assets/src/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ use DeadConsequence::*;
impl<T: Config<I>, I: 'static> Pallet<T, I> {
// Public immutables

pub fn sufficient_assets() -> impl Iterator<Item = T::AssetId> {
Asset::<T, I>::iter()
.filter_map(|(asset_id, details)| details.is_sufficient.then_some(asset_id))
}

/// Return the extra "sid-car" data for `id`/`who`, or `None` if the account doesn't exist.
pub fn adjust_extra(
id: T::AssetId,
Expand Down
Loading