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

[PVM, DAC] Bugfixes, tests, early-exit by fail #284

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Conversation

evlekht
Copy link
Member

@evlekht evlekht commented Dec 5, 2023

Why this should be merged

Bugfixes

Fixes following bugs:

  • addMember proposal could add not KYC-verified address
  • excludeMember proposal can be created by not consortium members or without having active validator
  • baseFee proposal could contain not-unique options

Tests

PR also adds unit tests for proposal Verify, IsSuccessful, CanBeFinished implementations and slightly improves camino service GetBalance test.

AddProposalTx changes

PR adds new field to tx:

	// Contains arbitrary bytes, up to maxProposalDescriptionSize
	ProposalDescription types.JSONByteSlice `serialize:"true" json:"proposalDescription"`

This field is limited to 2048 bytes, corresponding checks in syntactic verify, tests updated

Early-exit by fail

PR adds new early exit condition for base fee proposal:

mostVotedWeight + totalAllowedVoters - voted < mostVotedTheshold
mostVotedThreshold = voted / 2 + 1

Basically, this means, that if proposal most voted option can't get enough votes even if all not-yet casted votes would be casted for this option, than proposal is already failed and can be finished.

This is irrelevant for add/exclude member proposals, cause they have just 2 options and its impossible to not reach mostVotedTheshold without at least 3 options.

How this works

Regarding add/exclude member proposals bugs, PR adds corresponding checks to proposal verifier methods:

  • checks that addMemberProposal applicant address is kyc-verified
  • checks that excludeMemberProposal proposer address is consortium member and has active validator, but only if its not admin proposal - admin can bypass this requirements

Regarding baseFeeProposal, corresponding checks were in simpleVoteOptions - generic struct that was part of ProposalState. ProposalState is created by node on addProposalTx execution and should always be valid, so proposalState.Verify was never called. PR removes this function and moves its logic to baseFeeProposal.Verify (other existing proposal types doesn't have options, e.g. add/exclude member), which is implementation of proposal.Verify and is called on addProposalTx execution before proposalState is created.

How this was tested

Unit tests, integration tests

@evlekht evlekht force-pushed the evlekht/dac-fixes branch 9 times, most recently from 9324bbe to 099cf77 Compare December 8, 2023 14:01
@evlekht evlekht changed the title [PVM, DAC] Minor bugfixes [PVM, DAC] Bugfixes, tests, early-exit by fail Dec 11, 2023
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.

LGTM

@evlekht evlekht merged commit 6b4ec58 into dev Dec 11, 2023
5 checks passed
@evlekht evlekht deleted the evlekht/dac-fixes branch December 11, 2023 13:39
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