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] ExcludeMemberProposal #278

Merged
merged 3 commits into from
Nov 16, 2023
Merged

[DAC] ExcludeMemberProposal #278

merged 3 commits into from
Nov 16, 2023

Conversation

evlekht
Copy link
Member

@evlekht evlekht commented Oct 5, 2023

Why this should be merged

  • Adds exclude member proposal.
  • Extends proposals and finishProposalsTx logic to allow successful proposals unbond additional utxos.
  • Extracts proposalState types from txs codec to their own dac codec
  • Some general renaming and cleanup over dac

How this works

Exclude member proposal

PR adds new proposal type and corresponding proposals executor and verifier implementations.

If successful, exclude member proposal will do:

  • unset consortium member address state bit.
  • remove registered node short-link, if any
  • suspend active validator
  • remove pending validator and return its bond

BondTxIDsGetter

Because exclude member proposal is capable of unbonding bond other than proposals, finishProposalsTx was extended to use another proposals visitor: BondTxIDsGetter. This visitor is called for successful proposals and will return any additional bond tx ids that should be unlocked along with proposal bonds.

And because finishProposalsTx is system tx that is built by node, txs/builder pkg has to import dac BondTxIDsGetter implementation. In order to avoid cycle dependencies, dac visitors implementations was extracted from txs/executor to their own txs/dac pkg.

Codec

The reason for creating dac codec for proposalState type is that this types actually not used in transaction bodies, so there is no need for this types to pollute txs codec.

Commits

PR consists of 3 commits:

  • ExcludeMemberProposal and all corresponding implementations including BondTxIDsGetter
  • General DAC cleanup
  • Codec changes

How this was tested

unit-tests, integration tests

Co-authors:

@knikos great help with bondTxIDsGetter improvement

@evlekht evlekht changed the title Evlekht/dac exclude c member [DAC] ExcludeMemberProposal Oct 5, 2023
@evlekht evlekht force-pushed the evlekht/dac-exclude-c-member branch 2 times, most recently from 5de4447 to def0301 Compare October 5, 2023 16:39
@evlekht evlekht force-pushed the evlekht/dac-exclude-c-member branch 2 times, most recently from fcff5c7 to 59d951b Compare November 2, 2023 08:36
@evlekht evlekht force-pushed the evlekht/dac-exclude-c-member branch 5 times, most recently from ea1c855 to c1422b7 Compare November 14, 2023 13:46
@evlekht evlekht marked this pull request as ready for review November 14, 2023 13:54
@evlekht evlekht force-pushed the evlekht/dac-exclude-c-member branch 2 times, most recently from 36ca617 to fd7ed0e Compare November 14, 2023 14:17
@knikos knikos self-requested a review November 15, 2023 12:44
Copy link
Member

@knikos knikos left a comment

Choose a reason for hiding this comment

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

awesome test coverage and test case explanations. thank you!!

vms/platformvm/txs/dac/camino_dac.go Outdated Show resolved Hide resolved
vms/platformvm/txs/dac/camino_dac.go Outdated Show resolved Hide resolved
vms/platformvm/camino_helpers_test.go Show resolved Hide resolved
vms/platformvm/dac/camino_proposal.go Outdated Show resolved Hide resolved
vms/platformvm/dac/camino_exclude_member_proposal_test.go Outdated Show resolved Hide resolved
knikos
knikos previously approved these changes Nov 16, 2023
@evlekht evlekht merged commit 1dbf8ea into dev Nov 16, 2023
5 checks passed
@evlekht evlekht deleted the evlekht/dac-exclude-c-member branch November 16, 2023 17:30
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