Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow treasury to send XCM calls #19

Closed
wants to merge 5 commits into from

Conversation

samelamin
Copy link
Contributor

@samelamin samelamin commented Aug 26, 2023

Closes #18

This PR would make it possible for treasury spends via xcm but its currently blocked awaiting an audit so this is an interim solution that should achieve a similar result

How I envisage this working with treasury spends:

Treasurer track to ensure we avoid spend limits

batch { 
spend(ParachainSovereignAccount), 
// Notify destination of funds sent
send(dest: para, xcm: [ReserveAssetDeposited, ..]) 
}

@xlc
Copy link
Contributor

xlc commented Aug 26, 2023

need to ensure this can't be used to bypass the spender limit, or only support treasury origin which doesn't have the limit

@samelamin
Copy link
Contributor Author

need to ensure this can't be used to bypass the spender limit, or only support treasury origin which doesn't have the limit

That's a good point. I have changed it so it only uses the treasurer origin

But speaking to Bryan it might make sense to add xcm spends on the treasury pallet to allow spend limits there and dispatch the call from the pallet itself

@samelamin samelamin changed the title add spender track Allow treasury to send XCM calls Aug 26, 2023
Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The XCM config looks fine to me, but I am unsure about the implications of allowing the treasury to use the send extrinsic. Like what @xlc says, we want to ensure that this cannot be used to bypass existing limitations.

@xlc
Copy link
Contributor

xlc commented Aug 30, 2023

The new code only allow Treasurer origin, which does not have any spend limits so should be fine. However I do think some e2e tests will be helpful to ensure this indeed working as expected.

@samelamin
Copy link
Contributor Author

samelamin commented Aug 30, 2023

@xlc i don't mind building an integration test for this, but the current codebase doesn't have any integration tests

I could add some and follow what Cumulus are/was using, i.e. xcm-emulator

But again I am unsure if that's what's required here because most of the tests seem to be reusing macros from test-utils -> https://github.com/polkadot-fellows/runtimes/blob/main/system-parachains/asset-hubs/asset-hub-kusama/tests/tests.rs#L497

So I dont know if xcm emulator should be used in here. TBH, I do not see why not but it's always best to check

@bkchr @ggwpez what would make the most sense here?

@bkchr
Copy link
Contributor

bkchr commented Aug 30, 2023

I already discussed this briefly with @xlc and we should support chopstick tests inside this repo here.

@samelamin
Copy link
Contributor Author

Fair enough. In that case this is what I propose

  1. Commit a dummy github actions file into main to run the test. We need the initial github actions file merged before we can run it as part of this pr

  2. Use chopsticks to spin up a local network of both assethub and kusama and validate that we can send an xcm message using a treasury origin to assethub

Does the above sound reasonable?

@bkchr
Copy link
Contributor

bkchr commented Sep 4, 2023

Sounds reasonable :)

@samelamin
Copy link
Contributor Author

Closing this as raised a separate pr on the polkadot-sdk repo

@samelamin samelamin closed this Sep 7, 2023
@samelamin samelamin reopened this Sep 7, 2023
@samelamin
Copy link
Contributor Author

Reopening PR based on @bkchr comments

I can build a chopsticks test to run this but unfortunately, I will need a build runner that is powerful enough to run the actual test

In the meantime what I can do is build it via the free version of github actions until the build runner is built

That's essentially what I suggested here

Commit a dummy github actions file into main to run the test. We need the initial github actions file merged before we can run it as part of this pr

However, one thing to note is that reserve transfers are disabled from the relaychain to assethub and Penpal

So we can either

  1. Allow reserve transfers from the relay to Penpal
  2. Assert that the kusama XCM event is sent

The current xcm simulator tests on polkadot-sdk does 2.

However, not all chains will want to do this and I noticed that the parachain template also does this, is this intentional? Given that historically teams start off using the template would we want to define the barrier as so?

TLDR; What should the test assert on?

XCM event sent + balance reduction of sender

or

allowing a particular runtime to accept reserve transfers and doing the above + balance of receiver?

@samelamin samelamin marked this pull request as draft September 7, 2023 12:56
@samelamin
Copy link
Contributor Author

samelamin commented Sep 7, 2023

Also, this pr will need a companion pr to XCM to convert the xcm location of the treasury

The XCM crate is currently pointing to the archived repo https://github.com/paritytech/polkadot.git

https://github.com/polkadot-fellows/runtimes/blob/main/relay/kusama/Cargo.toml#L103

Should I submit the companion PR to the archived Polkadot repo or the new XCM repo? https://github.com/paritytech/xcm

@samelamin samelamin mentioned this pull request Sep 7, 2023
bkchr added a commit to paritytech/polkadot-sdk that referenced this pull request Oct 12, 2023
This pr resolves #1428.

*Added only to Kusama for now*

I did raise it
[here](polkadot-fellows/runtimes#19) 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

---------

Co-authored-by: Gavin Wood <github@gavwood.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
franciscoaguirre added a commit to paritytech/xcm that referenced this pull request Jan 25, 2024
This pr resolves paritytech/polkadot-sdk#1428.

*Added only to Kusama for now*

I did raise it
[here](polkadot-fellows/runtimes#19) 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

---------

Co-authored-by: Gavin Wood <github@gavwood.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
@bkchr
Copy link
Contributor

bkchr commented May 15, 2024

Stale

@bkchr bkchr closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Treasury/Spender Origins to do XCM transfers
4 participants