allow treasury to do reserve asset transfers

[samelamin]

This pr resolves #1428.

Added only to Kusama for now

I did raise it here and we discussed creating a chopsticks test to run an end-to-end test

however, to do that I will need a build agent/custom runner that is powerful enough to run the build

I will be doing that separately as I still think having chopsticks test your runtime with each commit will be very powerful and extremely useful for the ecosystem

For now I have used XCM simulator and replicated what the other reserve tests do

[bkchr]

I don't get why you opened this here. The runtimes will be released from the fellowship repo. You also wanted to create a chopsticks test?

TLDR: Please open against the fellowship repo.

[samelamin]

I don't get why you opened this here. The runtimes will be released from the fellowship repo. You also wanted to create a chopsticks test?

TLDR: Please open against the fellowship repo.

Sorry, I was under the impression the fellowship repo is read-only till it gets migrated over. My mistake.

I'll make the changes in that repo and close this pr

[samelamin]

reopening after discussion with Basti because this pr requires xcm changes that are not part of the fellowship repo currently

The test currently ensures we can do a reserve asset transfer from the relay chain to a parachain (Penpal)

We can remove all the changes to the runtime itself(and make the runtime changes in the fellowship repo) and only keep the xcm changes

But that would mean we have to remove the test as we need the Treasury Origin enabled so we can do a reserve asset transfer. Otherwise, the test will fail with BadOrigin

Let me know what is best to do :)

[franciscoaguirre]

If I understand correctly, you'll leave the change to xcm-builder here, and change the runtimes in the runtimes repo?
And with that, you say you have to remove the test from here?
You could create a test for this with a mock runtime, no need to use the actual repos.

[gavofyork]

Looks like you're mainly adding a means of converting Treasurer Frame-Origin to a local Plurality XCM-Origin and a means of converting a local Plurality XCM-Origin to a Frame-Origin of the Signed variant of the Treasury account ID.

Presumably you'll be wanting to communicate this XCM between two non-local XCM endpoints, in which case the local Plurality on the source endpoint will be a remote Plurality on the remote endpoint. In this case, your conversion to Treasury account ID won't work.

[samelamin]

Looks like you're mainly adding a means of converting Treasurer Frame-Origin to a local Plurality XCM-Origin and a means of converting a local Plurality XCM-Origin to a Frame-Origin of the Signed variant of the Treasury account ID.

Presumably you'll be wanting to communicate this XCM between two non-local XCM endpoints, in which case the local Plurality on the source endpoint will be a remote Plurality on the remote endpoint. In this case, your conversion to Treasury account ID won't work.

Hey Gav, if I understand your concern, you are saying that this will only work from relay to para.

We can resolve that by adding another match on parent:1. i.e.

		let id: [u8; 32] = match *location {
			MultiLocation { parents: 0, interior: X1(Plurality { id: BodyId::Treasury, .. }) } |
			MultiLocation { parents: 1, interior: X1(Plurality { id: BodyId::Treasury, .. }) } =>
				PalletId(*b"py/trsry").into_account_truncating(),
			_ => return None,
		};

Alternatively, if the remote plurality looks different we can rename the struct to LocalTreasuryVoiceConvertsVia, and it might make sense given its part of the LocalOriginConverter

Would that be acceptable?

[samelamin]

If I understand correctly, you'll leave the change to xcm-builder here, and change the runtimes in the runtimes repo? And with that, you say you have to remove the test from here? You could create a test for this with a mock runtime, no need to use the actual repos.

Hey Cisco, thanks for the tip. Is there an existing test that uses MockRuntime that I should use to keep things consistent

Also I am assuming that if we want to isolate a test for only the xcm changes then all we need to do is test the location conversion right? because the "reserve_asset_transfer" is actually part of the runtime and can be included in a test there?

[samelamin]

is there anything else that needs changing in this pr?

[samelamin]

Looks like you're mainly adding a means of converting Treasurer Frame-Origin to a local Plurality XCM-Origin and a means of converting a local Plurality XCM-Origin to a Frame-Origin of the Signed variant of the Treasury account ID.

Presumably you'll be wanting to communicate this XCM between two non-local XCM endpoints, in which case the local Plurality on the source endpoint will be a remote Plurality on the remote endpoint. In this case, your conversion to Treasury account ID won't work.

Hi Gav, sorry I think I misunderstood you but _think) I understand you now.

Let's assume Chain A is sending some funds from its treasury to Chain B.

With the current conversion, Chain B will incorrectly assume the funds are coming from its own treasury. The same issue occurs if going from Relay to Chain B

Speaking to @franciscoaguirre, he kindly advised me to use HashedDesciption to match on all remote endpoints, so ill be changing this shortly

[bkontur]

probably I am missing some context here, I can understand remote_account_convert_on_para - that this converter will be used as a part of LocationToAccountId converter on parachain,
but what does exactly mean sending_relay_treasury?

[samelamin]

its a badly named test i think, but its meant to be remote treasury on a parachain to another parachain

[bkontur]

hmm, because from the parachain's point of view this:

        MultiLocation {
			parents: 1,
			interior: X1(Plurality { id: BodyId::Treasury, part: BodyPart::Voice }),
		};

represents Plurality-Treasury-Voice location on relay chain (because parents: 1).

From the parachain's point-of-view a relay chain plurality looks like:

MultiLocation { parents: 1, interior: X1(Plurality { .. }

From the parachain's point-of-view an other parachain plurality looks like:

MultiLocation { parents: 1, interior: X2(Parachain(other_parachain_para_id), Plurality { .. }

So I would suggest to cover at least these two case in this test, to be sure that it works as expected.
Ideally, it should maybe cover more cases:
DescribeTreasuryVoiceTerminal is deployed on relay chain and local treasury location.
DescribeTreasuryVoiceTerminal is deployed on relay chain and parachain treasury location? (not sure about this case)
DescribeTreasuryVoiceTerminal is deployed on parachain and local treasury location.
DescribeTreasuryVoiceTerminal is deployed on parachain and relay chain treasury location.
DescribeTreasuryVoiceTerminal is deployed on parachain and other parachain treasury location.

(but as I said, I dont have full context here)

[samelamin]

Hi @bkontur thanks for this, I've renamed the tests and added another test to cover the para to para treasury conversion

I think the DescribeFamily covers all the other cases but please correct me if I am mistaken.

[bkontur]

please, revert also those imports from cumulus/parachains/integration-tests/emulated/common/src/constants.rs

[samelamin]

please, revert also those imports from cumulus/parachains/integration-tests/emulated/common/src/constants.rs

reverted