Fix DepositReserveAsset fees payment

[NachoPal]

The fee should be calculated with the reanchored asset, otherwise it could lead to a failure where the set aside fee ends up not being enough.

@acatangiu

[liamaharon]

Test?

[NachoPal]

Test?

What test would you like to see? It failing without the changes?

You can go to #3339 , undo the xcm-executor changes and run:

cargo test -p  asset-hub-rococo-integration-tests reserve_transfer_native_asset_from_para_to_system_para

It will fail due to the reason explained in this PR.

In any case, upon examining the code, it seems that the failure is potentially expected because the beforehand calculated fee is based on non-reanchored assets, while the actual calculation is performed for the reanchored assets

[liamaharon]

cargo test -p asset-hub-rococo-integration-tests reserve_transfer_native_asset_from_para_to_system_para passed for me on latest master.

I see there is a test for it in a different branch, but it is preferred I think (helps with review and ensures no regression can occur) to bundle the test with the fix when merging to master.

[acatangiu]

I agree you should add a test for this in this branch/PR, #3339 is more controversial.

Also for a regression test of this particular fix, instead of relying on a full xcm-emulator e2e test, you could add a (simpler) executor unit-test here.

[NachoPal]

@liamaharon @acatangiu I added a regression test. I'll merge the PR unless you request further changes.

[acatangiu]

this new test is failing in CI following merge from master

[NachoPal]

The previous PriceForMessageDelivery was flaky returning different prices.

Primarily, it was designed to return the same price for identical messages. However, reanchored messages weren't actually the same.

• First price calculation is done for a message with the full asset amount in ReserveAssetDeposited
• Actual price calculation during send() is done for a message where transport_fee has been deducted from the initial asset amount.

It does not explain by itself why the test was flaky. The reason for this flakiness is that the appended SetTopic(id) is calculated using the previous block as entropy. This is why any change in the runtime led to changes in the final message which might or might not generate enough transport fees to be paid.

All of this made me realise that the current method of setting aside the transport_fee for DepositReserveAsset will never be infallible for any possible PriceForMessageDelivery implementation. There is a circular dependency where it is impossible to know the actual DepositReserveAsset amount that will finally be included in the message from which the transport_fee is calculated. @acatangiu @franciscoaguirre

I followed a different approach to fix the test.

[acatangiu]

Can you expand a bit on the transport cost variance?

The actual amount (scalar value) of the asset within the message changes/impacts the message weight? I.e. An instruction containing asset: 2 DOT is more expensive than same instruction with asset: 3 DOT? Did I understand that correctly?
Or is it an issue with SetTopic(id) whose transport cost is not correctly calculated during execution of ReserveAssetDeposited? Looking at the code I'm missing where this cost changes between when we calculate and when it is charged..

[NachoPal]

An instruction containing asset: 2 DOT is more expensive than same instruction with asset: 3 DOT? Did I understand that correctly?

Not for our PriceForMessageDelivery implementation, because it only depends on the message size, that is why it was failing when the asset was not reanchored.

I am just saying that potentially, a third party could make use of the xcm-executor with a different PriceForMessageDelivery implementation, and we can not guarantee that the set aside transport_fee approach for DepositReserveAsset will be infalible. It is just something to be aware of in case the design can be improved, not a big deal, not even probable to ever happen, and something that shouldn't block this PR.

[NachoPal]

I'll proceed to merge it if the new test looks ok to you.

[Polkadot-Forum]

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

https://forum.polkadot.network/t/improve-xcm-development-and-release-process/6497/1