Pay XCM execution with local and foreign assets

[PatricioNapoli]

Attempts to solve paritytech/polkadot-sdk#105 by changing the XCM Trader to one that uses swap_tokens_for_exact_native for acquiring native asset to pay for XCM execution and swap_exact_native_for_tokens for refunds.

Achieved through the usage of the default Swap impl present in the asset conversion pallet.

Should handle both local and foreign assets.

[command-bot]

@paritytech-cicd-pr Requester could not be detected as a member of an allowed organization.

[joepetrowski]

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.

[bkontur]

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

[joepetrowski]

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.

[PatricioNapoli]

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

[bkontur]

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

[PatricioNapoli]

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?

[bkontur]

@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

[PatricioNapoli]

@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

[bkontur]

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

[muharem]

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.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/3412075