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

Fix threshold for no for non v1 side contracts #220

Merged
merged 3 commits into from
Apr 4, 2022
Merged

Fix threshold for no for non v1 side contracts #220

merged 3 commits into from
Apr 4, 2022

Conversation

Callum-A
Copy link
Collaborator

@Callum-A Callum-A commented Mar 24, 2022

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.

@Callum-A Callum-A marked this pull request as ready for review March 24, 2022 17:39
@ethanfrey
Copy link

ethanfrey commented Mar 24, 2022

Nice, the main issue is it checked if no passed threshold, when it should check if no passes 1-threshold.

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)

@0xekez
Copy link
Contributor

0xekez commented Mar 25, 2022

Just need to add the logic around the case where the passing threshold is 100% as discussed in Discord. #221

Copy link
Contributor

@0xekez 0xekez left a 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.

Copy link
Contributor

@blue-note blue-note left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@0xjame5 0xjame5 left a 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

Copy link
Contributor

@0xjame5 0xjame5 left a 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.

@0xekez 0xekez merged commit edda6c2 into main Apr 4, 2022
@0xekez 0xekez deleted the 219-main branch April 4, 2022 20:01
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.

6 participants