-
Notifications
You must be signed in to change notification settings - Fork 145
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
Fix threshold for no for non v1 side contracts #220
Conversation
Nice, the main issue is it checked if no passed threshold, when it should check if no passes This is a good fix. (The buggy version would refuse to mark as rejected some proposals that would never pass, but it wouldn't let them pass, so this is an enhancement and not security critical) |
Just need to add the logic around the case where the passing threshold is 100% as discussed in Discord. #221 |
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.
Looks great! Thanks for all help iterating on this.
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.
LGTM
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.
Did we reach 100% test coverage for the does_vote_count_reach_threshold()
for both multi-sig and cw3-dao. we probably have but I'm just curious here
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.
Besides those comments, overall looks good and given we're passing the old tests this means this is working as expected.
Need to get some eyes on this to check my logic all works clear.
New logic is no votes can reject proposals when they are greater than 1 - threshold. (Or for absolute count total_weight - count).
Going to need some eyes on this to sanity check.