XcmPaymentApi: take into account AH sufficient assets #6033

mrshiposha · 2024-10-13T16:38:19Z

Description

This PR provides:

The sufficient_assets function to the pallet-assets.
Report all sufficient assets as acceptable as XCM execution fees in addition to the Relay's token
Converting the native fees into the selected sufficient asset to be returned from the query_weight_to_asset_fee.

mrshiposha · 2024-10-13T16:38:28Z

acatangiu

Asset sufficiency does not mean asset can be directly used for paying fees.

For finding assets that can be (indirectly) used for paying fees, you need to check the list of pools. Specifying any asset in a pool paired with DOT/KSM/WND as the fee payment asset, will result in just-in-time auto swap for DOT just enough to pay for fees.

Maybe we actually need some other runtime API for exposing list of active pools if we don't have it already?
@franciscoaguirre has more info

acatangiu · 2024-10-14T07:53:29Z

cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs

+			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);


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.

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.

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

);

});

}

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)

acatangiu · 2024-10-15T14:42:31Z

I think this is superseeded by #5599?

cc @franciscoaguirre

franciscoaguirre · 2024-10-15T15:37:47Z

Yes, sufficient assets should only mean valid for ED but not necessarily for fees

acatangiu · 2024-10-15T15:39:00Z

@mrshiposha does #5599 cover your needs?

mrshiposha · 2024-10-15T15:39:07Z

Closing in favor of #5599

feat: use AH sufficient assets in xcm runtime api

32399b5

mrshiposha requested a review from a team as a code owner October 13, 2024 16:38

acatangiu requested changes Oct 14, 2024

View reviewed changes

mrshiposha closed this Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XcmPaymentApi: take into account AH sufficient assets #6033

XcmPaymentApi: take into account AH sufficient assets #6033

mrshiposha commented Oct 13, 2024

mrshiposha commented Oct 13, 2024

acatangiu left a comment

acatangiu Oct 14, 2024 •

edited

Loading

mrshiposha Oct 14, 2024

mrshiposha Oct 14, 2024

acatangiu Oct 14, 2024 •

edited

Loading

acatangiu commented Oct 15, 2024

franciscoaguirre commented Oct 15, 2024

acatangiu commented Oct 15, 2024

mrshiposha commented Oct 15, 2024

	// 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,
	>,
	>,

	#[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
	);
	});
	}

	#[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
	);
	});
	}

XcmPaymentApi: take into account AH sufficient assets #6033

XcmPaymentApi: take into account AH sufficient assets #6033

Conversation

mrshiposha commented Oct 13, 2024

Description

mrshiposha commented Oct 13, 2024

acatangiu left a comment

Choose a reason for hiding this comment

acatangiu Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

mrshiposha Oct 14, 2024

Choose a reason for hiding this comment

mrshiposha Oct 14, 2024

Choose a reason for hiding this comment

acatangiu Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

acatangiu commented Oct 15, 2024

franciscoaguirre commented Oct 15, 2024

acatangiu commented Oct 15, 2024

mrshiposha commented Oct 15, 2024

acatangiu Oct 14, 2024 •

edited

Loading

acatangiu Oct 14, 2024 •

edited

Loading