vochain: refactor vote transaction check #864
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
1. Reuse code and improve readability. Remove duplicates. 2. Add comments. 3. Fix issue with maxCensusSize and maxVoteOverwrite If a process already reached maxCensusSize, it should not discard votes that are doing an overwrite. 4. Use VoterID type instead of pubkey/address directly. So the pubkey/address primitives are now abstracted. Signed-off-by: p4u <pau@dabax.net>
p4u
force-pushed
the
f/refactor_vote_tx_check
branch
from
d3ea4e7
to
aa0b0ef
Compare
p4u
commented
Mar 24, 2023
| @@ -14,25 +14,35 @@ import ( | |||
|
|
|||
| // VoteTxCheck performs basic checks on a vote transaction. | |||
| func (t *TransactionHandler) VoteTxCheck(vtx *vochaintx.VochainTx, forCommit bool) (*vstate.Vote, error) { | |||
There was a problem hiding this comment.
This is the method where I'd need review. It is a very crucial part of the system and the logic should be double-checked. @mvdan @altergui @lucasmenendez @jordipainan
jordipainan
reviewed
Mar 27, 2023
jordipainan
reviewed
Mar 27, 2023
jordipainan
approved these changes
Mar 27, 2023
Most of the functions we use from ethereum cryptography, are
defined in the crypt/ethereum package.
For coherency and better abstraction, we might use always the new
type ethereum.Address{} instead of common.Address{} (from go-ethereum).
Ideally the only package that would import the ethereum common and other
go-ethereum packages would be crypto/ethereum, while the others relay on
the types and primitives defined within crypto/ethereum.
This approach would also bring better flexibility for changing the address
schema to a new one.
This commit is only part of the work.
Signed-off-by: p4u <pau@dabax.net>
p4u
force-pushed
the
f/refactor_vote_tx_check
branch
from
884099c
to
511cd31
Compare
|
LGTM, i didn't do my review in time, sorry about that, but here's a small (late) contribution: what testsuite covers of VoteTxCheck (i.e. tested on each CI run). the only significant omission is "// if vote exists, check if it has reached the max overwrite count" this is not covered. but it's precisely what Maria is working on.
|
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.
see commit messages