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

Add hrmp general admin track manipulator #2010

Merged
merged 60 commits into from Jan 17, 2023

Conversation

girazoki
Copy link
Collaborator

@girazoki girazoki commented Dec 19, 2022

What does it do?

Adds the hrmp-manage extrinsic to xcm-transactor pallet that is able to filter on distinct origin to any other extrinsic in the pallet. The idea is that thanks to OpenGov we can have an origin capable of opening HRMP channels.

The extrinsic works as follows: We have the HRMP pallet relay encoding hardcoded in the runtime. Through the extrinsic one can choose to perform three main actions:

  • InitChannel
  • Accept
  • Close

Based on the option selected the extrinsic will be able to encode differently. The idea is that we send 3 instrucions to the relay:

  • WithdrawAsset
  • BuyExecution
  • SetAppendix(RefundSurplus, DepositAsset(Sov account))
  • Transact

The setAppendix instruction is critical: it basically determines that no matter whether the xcm execution goes right or wong, any remaining tokens are deposited back to the sovereign account. This is important because a malicious referenda might try to withdraw more assets than what are needed to open/accept/close the HRMP channel. If we do not return them to the sovereign account, this might be short on assets for future token transfers.

However, the setAppendix is only placed after the BuyExecution instruction (otherwise it wouldnt pass relay barriers). An error in BuyExecution would not trigger the appendix. For instance, an error when trying to buy a high amount of weight would not with not-enough assets would not return the assets back. Luckily, we know the relay barriers just try to buy the amount of weight necessary to execute the message (https://github.com/paritytech/polkadot/blob/6a5d31fcbdd3ac5dd880e0bf3616e303b5bb480f/xcm/xcm-builder/src/barriers.rs#L83).

To further prevent situations in which tokens are not returned to the sovereign account, we also limit the amount that can be used as fee.

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@girazoki girazoki added D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Jan 2, 2023
@girazoki
Copy link
Collaborator Author

girazoki commented Jan 2, 2023

@crystalin I added a general admin track but we will probably need to modify parameters. Let me know about those

@girazoki girazoki marked this pull request as ready for review January 3, 2023 16:44
@crystalin
Copy link
Collaborator

@girazoki is the fee/second value manageable for with the admin track too ?

@girazoki
Copy link
Collaborator Author

girazoki commented Jan 16, 2023

@girazoki is the fee/second value manageable for with the admin track too ?

You mean if I can make the generalAdmin be responsible for changing the feePerSecond? Sure, there should be no problem with that

Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

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

I left some minor comments, but overall it's very good

@girazoki girazoki merged commit 876b5b0 into master Jan 17, 2023
@girazoki girazoki deleted the girazoki-hrmp-general-admin-track-manipulator branch January 17, 2023 11:24
@crystalin crystalin changed the title Girazoki hrmp general admin track manipulator Add hrmp general admin track manipulator Jan 20, 2023
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 26, 2023
imstar15 pushed a commit to OAK-Foundation/moonbeam that referenced this pull request May 16, 2023
* Add hrmp calls to relay encoders

* Keep advancing, use result

* add different traits

* Modify xcm-transactor pallet to be able to use appendix

* Better organization of traits

* first unitests

* Start adapting tests

* add general admin track and origin to manipulate hrmp

* build

* fix build for other runtimes

* integration tests for the new extrinsic

* toml sort

* format and prettier

* clippy happy

* comments

* editorconfig

* solve mock

* fix tests

* fix tests

* more timeout

* add max fee to pallet

* wip

* tests with hrmp

* Integration tests with mocked hrmp finished

* Add refundsurplus

* editorconfig

* fmt

* Remove non-necessary debugging file

* Cargo lock

* more tests on the encoding

* wip xcm transactor

* Keep working on separating traits

* Separate traits in relay-encoder

* Re-adapt precompile to separate traits

* Adapt tests to new separate traits

* Last changes to integration tests

* Make tests pass

* FMT

* toml sort

* Fix manange typo

* Add benchmark func for hrmp-manage

* Add benchmarking weight to hrmp_manage

* Add new functions to relay encoder instance

* Official track numbers

* Proper curves

* fix bug

* Change tests, in referenda test we check tracks, in xcm-transactor we check origin permissions

* prettier

* prettier

* prettier again

* pr suggestions

* Moonbeam unitests fixed

* fix benchmarks

* prettier and fmt

* referenda typescript

* referenda prettier

* Add generalAdminOrRoot to moonriver
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants