forked from ava-labs/avalanchego
-
Notifications
You must be signed in to change notification settings - Fork 3
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
[PVM, DAC] Admin proposals #282
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8c99fa2
to
300e77b
Compare
knikos
reviewed
Nov 13, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job and very goot test coverage!!! just some comments related mostly to formatting issues.
only one thing to note: I would add a test that checks whether an Admin Proposal CanBeFinished() immediately after being created just to have that extra unit test.
300e77b
to
c3b0f60
Compare
c3b0f60
to
f367182
Compare
knikos
approved these changes
Nov 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
This PR does 2 things:
Admin proposals are proposals that can be created only by allowed role (on proposal-type basis) and will be executed immediately after creation without any voting. So, they're more like admin actions than proposals.
How this works
Admin proposals
PR adds new proposal type that wraps other proposal types.
If proposal of this type is inside of addProposalTx, than tx will try to create admin proposal.
On execution node will check if wrapped proposal type actually allows be used as admin proposal, than it will check that proposer has address state role that allows him to create admin proposals of this type.
If all checks successful, node will create proposal state with already existing vote for specified option index and then will its id as finished proposal. So, on next block-build, node will create finishProposalsTx with this proposal.
AddressStateTx
If BerlinPhase is active, node will reject addressStateTx that tries to edit consortium bit, even if its signed by admin role.
This PR also moves address state definitions from
txs
to their own package, cause its now used from dac pkg as well and this is causing cycle deps.How this was tested
Unit-tests, integration test