xcm-executor: allow deposit of multiple assets if at least one of them satisfies ED #4460
Conversation
There was a problem hiding this comment.
yes, the approach looks good, only have implementation details comments
also, you should move this to a helper function because it needs to happen for DepositReserveAsset instruction as well as DepositAsset (it too uses does Config::AssetTransactor::deposit_asset)
|
@acatangiu Sorry for the long delay with this one. One last thing, I'm not sure how to do the tests for this one. What do you guys usually do with the xcm executor test? |
There was a problem hiding this comment.
Functional changes look good! 👍
Regarding tests:
I was looking at xcm-executor unit tests, but unfortunately test-runtime only has pallet_balances so it doesn't support multiple asset types (so we can properly test this PR).
So I think it's best to add an actual Asset Hub regression test (testing the actual runtime, not just a mock). An example scenario that fails without the fix, and should work with it:
Initiate transfer from Penpal to Rococo Asset Hub, to a new beneficiary account on AH, transferring:
XCMTESTtoken ({ parents: 1, interior: X3(Parachain(1000), PalletInstance(50), GeneralIndex(6)) }) - this is not sufficient on AssetHub
andrTether USDtoken ({ parents: 1, interior: X3(Parachain(1000), PalletInstance(50), GeneralIndex(1984)) }) - this is sufficient on AssetHub
A somewhat similar test can be seen here: https://github.com/acatangiu/polkadot-sdk/blob/796890979e5d7d16a522c304376d78eec120f3cb/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs#L945
I am aware that these emulated tests require low level knowledge of the system, and are complex and hard to set up, so I am also happy to write it myself and push to your branch if you can't figure it out :)
acatangiu
changed the title
|
@jpserrat is it ok if we take over and take this over the finish line? or do you want to do it yourself? Either way, please add your Polkadot address to the PR description so we can propose a tip for the work done so far. |
|
Hi @acatangiu sorry for the long delay with this one, I did the fixes that you requested, I'm only missing the tests. Can you do this part? |
Sure. Can you check what happened with the latest push you did? it messed up the PR, it's showing a million changes now. |
jpserrat
force-pushed
the
jpserrat/depositing-multiple-assets
branch
from
4bc901a
to
1d26287
Compare
|
Fixed! |
…rrat/depositing-multiple-assets
…rrat/depositing-multiple-assets
acatangiu
force-pushed
the
jpserrat/depositing-multiple-assets
branch
from
268a3bc
to
d5c9d4b
Compare
|
bot clean |
|
We are migrating the command bot to be a GitHub Action |
…rrat/depositing-multiple-assets
|
The CI pipeline was cancelled due to failure one of the required jobs. |
ebcbca3
|
/tip small |
|
@kianenigma A referendum for a small (4 KSM) tip was successfully submitted for @jpserrat (FkB6QEo8VnV3oifugNj5NeVG3Mvq1zFbrUu4P5YwRoe5mQN on kusama). Referendum number: 440. |
|
The referendum has appeared on Polkassembly. |
Closes #4242
XCM programs that deposit assets to some new (empty) account will now succeed if at least one of the deposited assets satisfies ED. Before this change, the requirement was that the first asset had to satisfy ED, but assets order can be changed during reanchoring so it is not reliable.
With this PR, ordering doesn't matter, any one(s) of them can satisfy ED for the whole deposit to work.
Kusama address: FkB6QEo8VnV3oifugNj5NeVG3Mvq1zFbrUu4P5YwRoe5mQN