Support different assets for delivery fees #4375

franciscoaguirre · 2024-05-03T16:07:53Z

Context

Fees can already be paid in other assets locally thanks to the Trader implementations we have.
This doesn't work when sending messages because delivery fees can only be paid in one asset.
The idea is to fix this by providing an AssetConverter that's able to turn the asset the user wants to pay fees in into the asset the router expects for delivery fees.

Paying delivery fees in USDT instead of WND on Westend Asset Hub (uses min_balance ratio since it's sufficient):

Paying delivery fees in a custom asset in a pool with WND (notice how the custom asset is withdrawn but WND is deposited):

Main addition

A new xcm executor config item is introduced: AssetConverter, that can convert balances from one asset to another.
It's used for paying delivery fees in different assets other than the native one (the one specified in the XcmRouter).

How it works

In order to pay delivery fees with different assets, you need to add some type as the AssetConverter in your XCM executor.
Right now, the AssetConverter approach will only work for delivery fees, for execution fees, you need to copy the same logic onto a Trader.
The logic is mostly the same, only the interface changes. The Trader has to do a little bit more work.
This duplicated logic is planned to be removed in v5, when we have the same mechanism for both execution and delivery fees.

If there is no AssetConverter then nothing will break as long as you pay fees in the intended asset (the one in XcmRouter).
If there is, then you can define which assets can be used for fees.

Tests

The following asset hub westend integration tests show scenarios where this is used:

Sufficient trust backed assets
- Reserve asset transfer from system para to para
- Reserve asset transfer from para to para through system para
Sufficient foreign assets
- Reserve asset transfer from system para to para
- Reserve asset transfer from para to para through system para
Pool assets
- Reserve asset transfer from system para to para
- Reserve asset transfer from para to para through system para

Breaking changes

This PR breaks the following things:

New AssetConverter needs to be set in the executor config, even if it's just ().
XcmExecutor::charge_fees signature changed.
Pending changes to the executor to allow setting the desired asset to pay for fees.

Dear reviewer

TODO

…e fees

…erent assets

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

Polkadot-Forum · 2024-05-10T14:54:33Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/xcm-user-and-developer-experience-improvements/4511/21

…with different assets

command-bot · 2024-05-22T12:51:18Z

Here's a link to docs

muharem

What if we disable the FeeManager for our runtime setups and try to account a transport weights in Weigher. Even if we cannot do it properly, may be some reasonable constants would be enough.
For local execution the transport fee will be included in extrinsics' weights, and for the remote execution a user will pay with BuyExecution for transport fee as well.
To avoid a double payment with jit_withdraw fee mode, we will need to disable it via config (here we also can provide some info for dependent teams).

After reading the weights payment model in the xcm pallet and the executor, I do not see a good way to make it work with various assets through the FeeManager without breaking the APIs. With the new xcm version we can have a better solution.

muharem · 2024-05-21T09:34:11Z

polkadot/xcm/xcm-executor/src/traits/fee_manager.rs

+				},
+				Err(error) => {
+					// TODO: log::error is too noisy and this might get called a lot.
+					log::error!(


it's not really an error since we still trying

muharem · 2024-05-21T09:59:20Z

polkadot/xcm/xcm-executor/src/lib.rs

+					let actual_asset_to_use_for_fees = match Config::AssetConverter::convert_asset(&first, asset_id) {
+						Ok(new_asset) => new_asset,
+						Err(error) => {
+							log::error!(target: "xcm::DepositReserveAsset", "What happened?");


this will error every time when we had no asset_for_fees in registry

Yeah this case hasn't been done yet

muharem · 2024-05-21T10:29:39Z

polkadot/xcm/xcm-executor/src/lib.rs

+			Config::AssetTransactor::withdraw_asset(&actual_asset_to_use, &origin, None)?;
 			Config::FeeManager::handle_fee(fees, None, FeeReason::ChargeFees);


this looks dangerous, to withdraw only one and handle all

muharem · 2024-05-21T13:26:40Z

polkadot/xcm/pallet-xcm/src/lib.rs

@@ -2422,7 +2422,7 @@ impl<T: Config> Pallet<T> {
 		log::debug!(target: "xcm::send_xcm", "dest: {:?}, message: {:?}", &dest, &message);
 		let (ticket, price) = validate_send::<T::XcmRouter>(dest, message)?;
 		if let Some(fee_payer) = maybe_fee_payer {
-			Self::charge_fees(fee_payer, price).map_err(|_| SendError::Fees)?;
+			Self::charge_fees(fee_payer, price, &AssetId(Here.into())).map_err(|_| SendError::Fees)?;


why is this always here? even if we want it to be native always, it wont be here for system parachains

muharem · 2024-05-22T13:40:58Z

polkadot/xcm/pallet-xcm/src/lib.rs

 			weight_limit,
 		)?;
-		Self::execute_xcm_transfer(origin, dest, local_xcm, remote_xcm)
+		Self::execute_xcm_transfer(origin, dest, local_xcm, remote_xcm, fees.id)


the assumption is that a user always wanna pay local fees with the same asset as on a remote chain. this wont work for example when a user does a transfer all for fees.id asset.

muharem · 2024-05-22T13:44:52Z

polkadot/xcm/pallet-xcm/src/lib.rs

-	fn charge_fees(location: Location, assets: Assets) -> DispatchResult {
-		T::XcmExecutor::charge_fees(location.clone(), assets.clone())
+	fn charge_fees(location: Location, assets: Assets, asset_for_fees: &AssetId) -> DispatchResult {
+		T::XcmExecutor::charge_fees(location.clone(), assets.clone(), asset_for_fees)


we could convert an asset from assets into asset_for_fees here, and avoid breaking the trait api at least for this case.

here I mean on the pallet

muharem · 2024-05-22T13:51:05Z

polkadot/xcm/xcm-executor/src/lib.rs

@@ -945,6 +1070,9 @@ impl<Config: config::Config> XcmExecutor<Config> {
 				// should be executed.
 				let Some(weight) = Option::<Weight>::from(weight_limit) else { return Ok(()) };
 				let old_holding = self.holding.clone();
+				// Store the asset being used for fees, so delivery fees can access it.
+				self.asset_for_fees = Some(fees.id.clone());


I think this is reasonable assumption that a user is fine to pay a transport fee with the same asset used for execution fee. The only problem is that local executions wont have the BuyExecution instruction.

That's my main concern, and the reason why I did the "looping through all assets in holding" trick, which I know is bad.
If local executions had a BuyExecution instruction then this would be great.
My current thought is to create a new entrypoint prepare_and_execute_with_fee_asset.

muharem · 2024-05-22T13:55:19Z

polkadot/xcm/xcm-executor/src/lib.rs

 			self.origin_ref(),
 			self.fees_mode,
 			reason,
 		);
+		let first = fees.get(0).ok_or(XcmError::AssetNotFound)?;


valid for our implementations, but if someone have more than one here, there is a problem

muharem · 2024-05-22T14:15:19Z

polkadot/xcm/xcm-executor/src/lib.rs

+			// It's not empty because of jit withdrawal, it's empty because it's taken from holding before sending.
+			// That's the problem jit withdrawal was solving, we need to "park" delivery fees in the same way we did
+			// for `DepositReserveAsset`.
+			for asset in self.holding.fungible_assets_iter() {


This way we basically deciding for a user which asset to use for the payment, this can be a surprise.

Yeah, but some API needs to change or I have to add a new one to not have to do this.
I was thinking of creating a new entrypoint prepare_and_execute_with_fee_asset that pallet-xcm can use.
That'd keep the API the same for all other teams.
What do you think?

paritytech-cicd-pr · 2024-05-22T14:40:38Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6263079

franciscoaguirre added 5 commits May 8, 2024 02:16

feat(xcm-executor): add AssetConverter config item to support multipl…

eea690b

…e fees

fix: implement AssetConverter in all runtimes

5d74482

feat: add AssetConverters copying the traders

8e9df56

test(asset-hub-westend): add tests for paying delivery fees with diff…

3ed0430

…erent assets

fix: update lock

da83c68

franciscoaguirre force-pushed the different-assets-delivery-fees branch from 98c7ded to da83c68 Compare May 8, 2024 00:23

franciscoaguirre mentioned this pull request May 8, 2024

Asset Transactor on Asset Hubs not Working With Sufficient Assets #3958

Open

joepetrowski reviewed May 8, 2024

View reviewed changes

cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs Outdated Show resolved Hide resolved

muharem self-requested a review May 8, 2024 11:34

franciscoaguirre added 4 commits May 11, 2024 10:39

fix(pallet-xcm): remove jit withdrawal to allow paying delivery fees …

efab825

…with different assets

doc: add TODOs I'm considering

da7b054

feat: start with SwapAssetConverter

bdfbeaf

Merge branch 'master' into different-assets-delivery-fees

a81ba18

4meta5 mentioned this pull request May 20, 2024

🏗️ [Core Feature]: Pay EVM Gas Fees with any* tokens OpenZeppelin/polkadot-runtime-templates#189

Open

franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label May 21, 2024

franciscoaguirre self-assigned this May 21, 2024

franciscoaguirre added 2 commits May 22, 2024 12:02

feat: SwapAssetConverter. Some tests failing

c69095f

fix(asset-hub-rococo-integration-tests): delivery fee taken from holding

0193e60

muharem reviewed May 22, 2024

View reviewed changes

fix(integration-tests): stick to jit withdrawal for now

0b0437c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support different assets for delivery fees #4375

Support different assets for delivery fees #4375

franciscoaguirre commented May 3, 2024 •

edited

Polkadot-Forum commented May 10, 2024

command-bot bot commented May 22, 2024

muharem left a comment

muharem May 21, 2024

muharem May 21, 2024

franciscoaguirre May 22, 2024

muharem May 21, 2024

muharem May 21, 2024

muharem May 22, 2024

muharem May 22, 2024

muharem May 22, 2024

muharem May 22, 2024

franciscoaguirre May 22, 2024

muharem May 22, 2024

muharem May 22, 2024

franciscoaguirre May 22, 2024

paritytech-cicd-pr commented May 22, 2024

		Config::AssetTransactor::withdraw_asset(&actual_asset_to_use, &origin, None)?;
		Config::FeeManager::handle_fee(fees, None, FeeReason::ChargeFees);

Support different assets for delivery fees #4375

Are you sure you want to change the base?

Support different assets for delivery fees #4375

Conversation

franciscoaguirre commented May 3, 2024 • edited

Context

Main addition

How it works

Tests

Breaking changes

Dear reviewer

Polkadot-Forum commented May 10, 2024

command-bot bot commented May 22, 2024

muharem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented May 22, 2024

franciscoaguirre commented May 3, 2024 •

edited