Reject proposals early #668

Callum-A · 2022-02-22T15:09:11Z

Addresses Issue #665

Implemented what I implemented for DAO DAO here for the multisigs. If I have missed any functionality or coverage do say.

Proposals are now marked as rejected when no votes reach their threshold.
Added tests for proposal struct and unit tests for functionality.

Feedback appreciated.

CLAassistant · 2022-02-22T15:35:23Z

All committers have signed the CLA.

CLAassistant · 2022-02-22T15:37:31Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

ueco-jb

I'll be back to review actual implementation, commented only obvious stuff so far.

ueco-jb · 2022-02-22T15:46:24Z

contracts/cw3-fixed-multisig/src/state.rs

        if status == Status::Open && self.expires.is_expired(block) {
            status = Status::Rejected;
        }
+        if status == Status::Open && self.is_rejected(block) {
+            status = Status::Rejected;
+        }


Suggested change

if status == Status::Open && self.expires.is_expired(block) {

status = Status::Rejected;

}

if status == Status::Open && self.is_rejected(block) {

status = Status::Rejected;

}

if status == Status::Open && (self.expires.is_expired(block) || self.is_rejected(block)) {

status = Status::Rejected;

}

ueco-jb · 2022-02-22T15:50:05Z

contracts/cw3-fixed-multisig/src/state.rs

+    /// returns true iff this proposal is sure to pass (even before expiration if no future
+    /// sequence of possible votes can cause it to fail)


Suggested change

/// returns true iff this proposal is sure to pass (even before expiration if no future

/// sequence of possible votes can cause it to fail)

/// returns true if this proposal is sure to pass (even before expiration, if no future

/// sequence of possible votes could cause it to fail)

ueco-jb · 2022-02-22T15:51:09Z

contracts/cw3-fixed-multisig/src/state.rs

+    /// As above for the rejected check, used to check if a proposal is
+    /// already rejected.


IMO doc comments shouldn't state "as above`, since "above" part won't be shown by IDE, rust compiled docs etc.

ueco-jb · 2022-02-22T15:57:16Z

contracts/cw3-fixed-multisig/src/state.rs

+    /// This function returns true if and only if vote_count is greater than the threshold which
+    /// is calculated.
+    /// In the case where we use yes votes, this function will return true if and only if the
+    /// proposal will pass.
+    /// In the case where we use no votes, this function will return true if and only if the
+    /// proposal will be rejected regardless of other votes.


Suggested change

/// This function returns true if and only if vote_count is greater than the threshold which

/// is calculated.

/// In the case where we use yes votes, this function will return true if and only if the

/// proposal will pass.

/// In the case where we use no votes, this function will return true if and only if the

/// proposal will be rejected regardless of other votes.

/// This function returns true if vote_count is greater than the threshold.

/// In the case where we use `yes` votes, this function will return true if the proposal will pass.

/// In the case where we use `no` votes, this function will return true if the proposal will be

/// rejected, regardless of other votes.

Those "if and only if" are redundant, as well as some other stuff..

maurolacy

LGTM. Thanks for the contribution.

contracts/cw3-flex-multisig/src/contract.rs

contracts/cw3-fixed-multisig/src/state.rs

Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>

ueco-jb

lgtm

ethanfrey

Is rejected doesn't work if the threshold deviates from 50%.

Added suggestions how to fix code and tests

ethanfrey · 2022-03-24T12:41:22Z

contracts/cw3-fixed-multisig/src/state.rs

        match self.threshold {
            Threshold::AbsoluteCount {
                weight: weight_needed,
-            } => self.votes.yes >= weight_needed,
+            } => vote_count >= weight_needed,


This check doesn't work properly of no votes. I would implement it sepately rather than combining.

Assume a threshold of 67%.

If we have 40% yes and 40% no, it would return false for both.
However, this could be safely marked as rejected.

Your logic works fine when the threshold is 50% (same for both).

I would propose not renaming the function and using it for both cases (as done here), but rather copying is_passed to is_rejected and make the various logic changes there needed (often 1-threshold or such). This is different for each branch and hard to generalise. Better to have two separate functions than combine them

You're completely right, will try and address this today

ethanfrey · 2022-03-24T12:45:16Z

contracts/cw3-fixed-multisig/src/contract.rs

+        let info = mock_info(VOTER4, &[]);
+        let res = execute(deps.as_mut(), mock_env(), info, no_vote).unwrap();
+
+        // Verify it is now rejected due to reaching threshold


This is actually a test showing the code is broken.

setup_test_case adds 6 voters with a total weight of 16.
This test cases constructs an example with the odd configuration of 3 votes to reach threshold.

4 no votes should not mark it as rejected. You should require 14 no votes to ensure it is rejected.

I would propose a separate test function to cover this.

ethanfrey · 2022-03-24T12:45:50Z

contracts/cw3-fixed-multisig/src/state.rs

+    #[test]
+    fn proposal_rejected_quorum() {
+        let quorum = Threshold::ThresholdQuorum {
+            threshold: Decimal::percent(50),


Let's use threshold = Decimal::percent(60) so we can test the real case. Then fix the logic to match

ethanfrey

Nice fix.
Just request two minor changes, which don't affect correctness

ethanfrey · 2022-03-24T17:11:01Z

contracts/cw3-fixed-multisig/src/contract.rs

+                .add_attribute("status", "Open")
+        );
+
+        // Voter 2 votes no, weight 2, total weight for no so far 15, need 14.


ethanfrey · 2022-03-24T17:11:50Z

contracts/cw3-fixed-multisig/src/state.rs

+            Threshold::AbsoluteCount {
+                weight: weight_needed,
+            } => {
+                let weight = self.total_weight - weight_needed;


This is good

ethanfrey · 2022-03-24T17:12:46Z

contracts/cw3-fixed-multisig/src/state.rs

+                self.votes.no
+                    > votes_needed(
+                        self.total_weight - self.votes.abstain,
+                        Decimal::one() - percentage_needed,


Yes, this is proper place. Nice eye

ethanfrey · 2022-03-24T17:13:50Z

contracts/cw3-fixed-multisig/src/state.rs

+                    )
+            }
+            Threshold::ThresholdQuorum { threshold, quorum } => {
+                // we always require the quorum


We don't need this check. This is just needed for is_passed

If threshold is 67% and quorum is 40%, then 35% no votes should guarantee rejected.

contracts/cw3-fixed-multisig/src/state.rs

ethanfrey

great work. merging

ethanfrey · 2022-03-24T17:34:29Z

contracts/cw3-fixed-multisig/src/state.rs

+            true
+        ));
+
+        // Check edgecase where quorum is not met but we can reject


maurolacy · 2022-03-24T17:54:03Z

Is rejected doesn't work if the threshold deviates from 50%.

Good catch!

Callum-A added 3 commits February 22, 2022 15:06

Reject proposals when they cannot pass.

f48dbce

lint

a46886e

Add flex tests

f770502

ueco-jb reviewed Feb 22, 2022

View reviewed changes

Address feedback

8cb9e80

maurolacy approved these changes Feb 23, 2022

View reviewed changes

contracts/cw3-flex-multisig/src/contract.rs Outdated Show resolved Hide resolved

contracts/cw3-fixed-multisig/src/state.rs Outdated Show resolved Hide resolved

Callum-A and others added 2 commits February 23, 2022 11:35

Update contracts/cw3-flex-multisig/src/contract.rs

a231a2e

Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>

Update contracts/cw3-fixed-multisig/src/state.rs

4cc1159

Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>

ueco-jb approved these changes Feb 23, 2022

View reviewed changes

Callum-A and others added 2 commits February 24, 2022 11:49

Merge branch 'main' into main

4a12462

Merge branch 'main' into main

8d0cf8d

ethanfrey requested changes Mar 24, 2022

View reviewed changes

Callum-A added 4 commits March 24, 2022 15:15

Address feedback

1b43acc

Merge branch 'main' of https://github.com/Callum-A/cw-plus into main

07d8530

Correct small error

3b326ee

Typo again

7689360

ethanfrey approved these changes Mar 24, 2022

View reviewed changes

Address feedback

068d33f

ethanfrey approved these changes Mar 24, 2022

View reviewed changes

contracts/cw3-fixed-multisig/src/state.rs

true

));

// Check edgecase where quorum is not met but we can reject

Copy link

Member

ethanfrey Mar 24, 2022

Choose a reason for hiding this comment

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

💯

Update comment. Circle CI

92e01a4

ethanfrey merged commit c924232 into CosmWasm:main Mar 24, 2022

ethanfrey mentioned this pull request Mar 24, 2022

Prepare 0.13.1 release #686

Merged

maurolacy mentioned this pull request Aug 8, 2022

[cw3-flex/fixed-multisig] Reject proposals early #665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject proposals early #668

Reject proposals early #668

Callum-A commented Feb 22, 2022

CLAassistant commented Feb 22, 2022 •

edited

CLAassistant commented Feb 22, 2022

ueco-jb left a comment

ueco-jb Feb 22, 2022

ueco-jb Feb 22, 2022

ueco-jb Feb 22, 2022

ueco-jb Feb 22, 2022 •

edited

maurolacy left a comment

ueco-jb left a comment

ethanfrey left a comment

ethanfrey Mar 24, 2022

Callum-A Mar 24, 2022

ethanfrey Mar 24, 2022

ethanfrey Mar 24, 2022

ethanfrey left a comment

ethanfrey Mar 24, 2022

ethanfrey Mar 24, 2022

ethanfrey Mar 24, 2022

ethanfrey Mar 24, 2022

ethanfrey left a comment

ethanfrey Mar 24, 2022

maurolacy commented Mar 24, 2022

		/// returns true iff this proposal is sure to pass (even before expiration if no future
		/// sequence of possible votes can cause it to fail)

		/// As above for the rejected check, used to check if a proposal is
		/// already rejected.

Reject proposals early #668

Reject proposals early #668

Conversation

Callum-A commented Feb 22, 2022

CLAassistant commented Feb 22, 2022 • edited

CLAassistant commented Feb 22, 2022

ueco-jb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueco-jb Feb 22, 2022 • edited

Choose a reason for hiding this comment

maurolacy left a comment

Choose a reason for hiding this comment

ueco-jb left a comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy commented Mar 24, 2022

CLAassistant commented Feb 22, 2022 •

edited

ueco-jb Feb 22, 2022 •

edited