Allow Sufficient Assets for XCM Fee Payment on Statemint #1884

[bkontur]

fixes: #1884

[bkontur]

@joepetrowski
I am just thinking, if it would be better here iterate and find "first.is_sufficient" maybe

[bkontur]

reverting payment.clone().into(); it does that sorting impl From<Vec<MultiAsset>> for MultiAssets {

[bkontur]

now it takes "first" from sorted (probably by highest amount),
but it does not consider is_sufficient and/or minimum_balance,

so, the first asset could have higher amount then second,
but first asset could have also higher minimum_balance than second asset, so it couldnt be charged from first asset, but the second could be charged

maybe "first selection" could be improved

[bkontur]

@joepetrowski
do we want to change this "first selection" or let it be or just create following issue? wdyt?

at least, I am going to add here unit-test with multiple assets in payment

[bkontur]

@girazoki
and please, wdyt about this "first selection" algo?

[joepetrowski]

Yeah, I like the iter approach. It would make it more user-friendly and make every effort to find an acceptable fee payment solution.

[girazoki]

multiassets object are always sorted I think (their creation involves sorting assets AFAIR). So we simply take the first.

There is no sufficiency indicator on a MultiAsset, so you probably will have to do db reads to know whether a multiasset is sufficient.

Besides, BuyExecution will always contain just one multiasset https://github.com/paritytech/polkadot/blob/53253de77f1649ecbb650df2ec3e29d0d1110a2d/xcm/src/v2/mod.rs#L564. The reason for buy_weight to receive MUltiAssets instead of MultiAsset as fee is because try_take from the holding register is defined in MultiAssets: https://github.com/paritytech/polkadot/blob/795b20c7150b04759ebddadd419e2f26fa382519/xcm/xcm-executor/src/lib.rs#L455

[bkontur]

@girazoki
ok, thank you for the information,

yes, there have to be database hit for checking MultiAsset (is_suffiecient,min_balance), but this is already done in AssetFeeAsExistentialDepositMultiplier -> BalanceConverter::to_asset_balance.

and according to what you say, then maybe let multiassets: MultiAssets = payment.clone().into(); the whole clone/join_iterators/resorting stuff is really not needed and payment.fungible_assets_iter().next() will do the same job with also less cloning of other stuff here, maybe without need to use Matcher here, so then maybe renaming TakeFirstAssetTrader to TakeFirstFungibleAssetTrader.

I understand, that TakeFirstAssetTrader is supposed to be as-much-as-possible abstract implementation, but I think, there is a little "space/tension" between API payment: xcm_executor::Assets (MutliAssets) vs. BuyExecution { fees: MultiAsset, and this "space/tension" could be possibly fixed by the improved "first selection" algo iterating multiassets. And based on the fact, that we are already doing database hit (is_suffiecient,min_balance) and "BuyExecution has only one asset", so there wont be any performance degradation, but opposite, without clone/join_iterators/resorting it could be just improved and on the other hand the algo will be more complete, maybe for future multiassets support for BuyExecution(MutliAssets)

we will come to this maybe later in the following issue: https://github.com/paritytech/cumulus/issues/1915

[joepetrowski]

@girazoki

[joepetrowski]

Just a few questions but looks good.

[dvdplm]

lgtm modulo some concerns about the naming (getting very long!).