-
Notifications
You must be signed in to change notification settings - Fork 142
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
Allow voting up to expiration time #477
Allow voting up to expiration time #477
Conversation
hey! thanks for putting this together! skimmed this and it looks good. we're really busy trying to get v2 released here so might be a couple days before i can get to a proper review. <3 |
apologies for the delay in reviewing here -- lets push this through! Also, for the integration test failure, rebasing on main should fix it. |
52c2166
to
758588e
Compare
thanks for the review! I rebased over main, hope it fixes the failure |
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.
thanks for the PR! i think this would be a really nice change to do some fuzzing on. specifically, the assertion we're making here that:
if a proposal's outcome has been determined, no sequence of additional votes will cause that outcome to change.
you could probably fuzz this pretty easily by manually selecting a number of interesting "passed" vote counts, generating a sequence of random votes, casting those votes, and then making sure the proposal's outcome has not changed.
if you'd like some example code to build this on top of, i did something pretty similar recently to fuzz a rate limiter here.
any time we change vote casting code, it is pretty high risk. testing this really well would make me feel good about these changes.
closing this for now. @NinoLipartiia if you have plans for picking this up feel free to just push to this branch and we'll reopen and do more review :) just want to clear the pr queue. |
I'll pick this up |
758588e
to
44739a5
Compare
Deleted NotOpen error, added tests Changed Expired error
unit & adversarial tests
3e3e889
to
3feb0bd
Compare
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.
Great work! Thanks for helping this get across the finish line for v2.
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.
amazing work. thank you @bekauz !
Closes #463.