-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fixes #249 #250
Fixes #249 #250
Conversation
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.
Instead of the two checks on getRevealWinner
compare with votingSubject.getRevealWinner() == Boolean.FALSE
To be honest I have no clue what this is validating. What is a VotingSubjectRevealWinnerValidator actually validating? Your code is changing it's behavior, but it is untested and undocumented. I demand unit tests to verify the desired behavior.
I got 2 integration tests on this but bukajsytlos changed the logic in a way that was similar but not the same |
9c7fbc0
to
40ede7f
Compare
But Brutus is right. I changed/broke the logic but tests still passed. So there is something missing on test side, right? |
40ede7f
to
6a4b024
Compare
Well yes I did not test all cases, I tested the security relevant ones |
I will add another test |
6a4b024
to
97c3b0b
Compare
97c3b0b
to
fef9aac
Compare
Btw if you merge this could you deploy it as well? |
Probably not. Depends on required database migrations. |
oh ok. hmm |
👍 only needs to be deployed some when then ftx can do his ladder vote |
@axel1200 it's on prod now |
Cool only missing the modor PR and I can tell ftx his ladder voting stuff is done |
No description provided.