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

DAC voting, base fee proposal #269

Merged
merged 3 commits into from
Oct 26, 2023
Merged

DAC voting, base fee proposal #269

merged 3 commits into from
Oct 26, 2023

Conversation

evlekht
Copy link
Member

@evlekht evlekht commented Jun 30, 2023

Why this should be merged

This PR adds feature of DAC proposals and voting which is executed with transactions.
Also, this PR renames all DAO entries to DAC.

How this works

There are 3 new transactions:

  • AddProposalTx, it contains proposal bytes and proposer address/auth. In addition to fee burning, this tx will bond some tokens. Also, this tx must be additionally signed by proposer.
  • AddVoteTx, it contains vote bytes, voter address/auth and proposalID (id of addProposalTx). This this must be additionally signed by voter.
  • FinishProposalsTx is system tx. It will finish all expired (both successful and failed) and early (again both successful and failed) proposals, tx body contains those proposal ids.

Vote and proposal fields in txs represented by bytes, not some structure or interface. This is done in order to be able to add new proposal types without needing to update codec.

Block builder will try to build block with FinishProposalsTx when there are early finished proposals or expired proposals.

Proposals execution and semantic verification logic resides in its own proposalExecutor type methods in txs/executor package. Proposals are calling this methods via visitor pattern.

Proposals and votes logic are encapsulated behind interfaces, which could be implemented by different types. This PR adds BaseFeeProposal and SimpleVote types.

SimpleVote is vote that simply points to specific proposal option index, e.g. baseFee proposal with options [ {newFee: 100}, {newFee: 200} ], and simpleVote {0}, where vote is for newFee 100 option.

BaseFeeProposal is proposal to change base fee, its options are new base fees. This proposal can only be proposed by caminoProposer address state role, which is introduced in this PR as well.

All proposals can be voted only by addresses that were active validators at the moment of addProposalTx block execution. While its not required for voters to still be active valaditors when they vote, it is required for them to still be consortium members.

Commits

PR consists of 3 commits:

  1. Commit with base logic except most of proposal-related tests, cause the need some actual proposal implementation.
  2. Commit with simpleVote and corresponding tests
  3. Commit with baseFeeProposal, corresponding tests and all tests that were excluded from (1)

How this was tested

Unit-tests, integration tests

@peak3d peak3d force-pushed the dev branch 3 times, most recently from fee8858 to c61241b Compare July 3, 2023 10:38
@evlekht evlekht force-pushed the evlekht/dao branch 4 times, most recently from 3e6cf54 to a4998e4 Compare July 19, 2023 17:19
@evlekht evlekht force-pushed the evlekht/dao branch 3 times, most recently from f9a89d2 to 4c1b2c3 Compare August 14, 2023 14:35
@evlekht evlekht marked this pull request as ready for review August 14, 2023 15:13
@evlekht evlekht force-pushed the evlekht/dao branch 4 times, most recently from 80a330d to 8a44b5d Compare August 18, 2023 12:11
@evlekht evlekht force-pushed the evlekht/dao branch 4 times, most recently from a555726 to 581065d Compare August 31, 2023 17:54
@evlekht evlekht mentioned this pull request Sep 29, 2023
@c4t-ag c4t-ag requested a review from peak3d September 29, 2023 10:21
Copy link

@c4t-ag c4t-ag left a comment

Choose a reason for hiding this comment

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

Nice work!
There are some minor things (comments), and some general things which should be at least discussed:
1.) If possible (not sure if it makes sense) the final PR should have at least 2 commits, the base implementation and another commit the basefee specialization. This would help to see what is required to implement a more complex one (thinking about a multi option proposal where the exit is not necessarily a simple 50% one.
2.) It should be evaluated (with @Noctunus because the idea was initially from him) to operate with a fee multiplier instead setting the basefee. This could then be applied to all spialized Fees.

version/constants.go Outdated Show resolved Hide resolved
vms/platformvm/dac/camino_vote.go Outdated Show resolved Hide resolved
vms/platformvm/locked/camino_lock.go Show resolved Hide resolved
@c4t-ag c4t-ag removed the request for review from peak3d September 29, 2023 12:55
Copy link

@c4t-ag c4t-ag left a comment

Choose a reason for hiding this comment

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

Some more comments for additional comments :-)

vms/platformvm/camino_service.go Outdated Show resolved Hide resolved
vms/platformvm/dac/camino_change_base_fee_proposal.go Outdated Show resolved Hide resolved
vms/platformvm/dac/camino_change_base_fee_proposal.go Outdated Show resolved Hide resolved
@evlekht evlekht changed the title Dao voting, base fee proposal DAC voting, base fee proposal Oct 2, 2023
@evlekht
Copy link
Member Author

evlekht commented Oct 2, 2023

@c4t-ag
I added "version" without base fee proposal, see last 2 commits. First commit removes all base fee related stuff, then second adds it back. As you can see from first commit, it will unfortunately remove a lot of transaction tests, cause we need at least one proposal type in order to create them. And I don't want to add dummy proposal type, cause then we'll need to modify proposals visitors to include this type.

@c4t-ag c4t-ag self-requested a review October 5, 2023 11:28
Copy link

@c4t-ag c4t-ag left a comment

Choose a reason for hiding this comment

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

Great work, go!

@evlekht evlekht merged commit 8419c47 into dev Oct 26, 2023
5 checks passed
@evlekht evlekht deleted the evlekht/dao branch October 26, 2023 12:51
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.

2 participants