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

Add threshold to cw3 flex #180

Merged
merged 28 commits into from
Dec 16, 2020
Merged

Add threshold to cw3 flex #180

merged 28 commits into from
Dec 16, 2020

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Dec 14, 2020

Closes #139

This allows us to set any of the 3 defined threshold types for passing proposals.
Handler code is working and tested for the original case (AbsoluteCount).
Low level code (calculate passing, etc) is tested for all cases I could think of.

TODO:

  • unit test all helper functions in msg for the other threshold types
  • unit test all helper functions in state for the other threshold types
  • revisit rules for passing with quorum (hard to pass early)
  • provide high-level scenario tests with other threshold types (esp with changing groups)
  • test coverage for proposal lists, vote details

@ethanfrey
Copy link
Member Author

This should be ready to review. Covered with lots of unit tests. I will provide a few high-level scenario tests for the more dynamic variants, but the logic should all be covered with tests now.

@ethanfrey
Copy link
Member Author

Finished added all tests

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Incomplete review. Posting anyway for your consideration.

I propose we have a chat tomorrow to discuss some details I'm certainly missing.

contracts/cw3-flex-multisig/src/error.rs Outdated Show resolved Hide resolved
contracts/cw3-flex-multisig/src/msg.rs Outdated Show resolved Hide resolved
contracts/cw3-flex-multisig/src/msg.rs Outdated Show resolved Hide resolved
contracts/cw3-flex-multisig/src/msg.rs Outdated Show resolved Hide resolved
contracts/cw3-flex-multisig/src/msg.rs Show resolved Hide resolved
contracts/cw3-flex-multisig/src/state.rs Outdated Show resolved Hide resolved
contracts/cw3-flex-multisig/src/state.rs Outdated Show resolved Hide resolved
contracts/cw3-flex-multisig/src/state.rs Outdated Show resolved Hide resolved
contracts/cw3-flex-multisig/src/state.rs Outdated Show resolved Hide resolved
contracts/cw3-flex-multisig/src/state.rs Outdated Show resolved Hide resolved
@ethanfrey ethanfrey mentioned this pull request Dec 15, 2020
@ethanfrey
Copy link
Member Author

Please review #188 and give feedback there, then we can see what discussion points are still open.

@ethanfrey
Copy link
Member Author

Also I addressed the edge case on hitting absolute threshold, but not quorum that you pointed out. Good catch there.

@maurolacy maurolacy self-requested a review December 16, 2020 10:06
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Looks good.

Only remaining (minor) concern I have is about Abstain subtraction when compute passing weight in AbsoluteThreshold.

contracts/cw3-flex-multisig/src/state.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

I just added a few smaller commits for some of the last outstanding points.

If you can unify the docs a bit more, that would be great. Then feel free to merge when done with docs update.

@maurolacy
Copy link
Contributor

maurolacy commented Dec 16, 2020

I just added a few smaller commits for some of the last outstanding points.

This is looking great.

If you can unify the docs a bit more, that would be great. Then feel free to merge when done with docs update.

OK.

One more thing:
I think the different checks between expired and not expired voting must be applied to AbsolutePercentage too. I mean, after the voting has ended, you would want the passing percentage to be computed over the weights of the ones who effectively voted (minus Abstain), not over the total weight.

Or is it called Absolute because of that? Then we could have a fourth version: RelativePercentage (or plain Percentage).

Add reference to cw3 spec docs
Update schema
@maurolacy
Copy link
Contributor

I'll add an issue to consider implementing RelativePercentage in another PR.

Merging now.

@maurolacy maurolacy merged commit a3e414e into master Dec 16, 2020
@ethanfrey
Copy link
Member Author

I think the different checks between expired and not expired voting must be applied to AbsolutePercentage too. I mean, after the voting has ended, you would want the passing percentage to be computed over the weights of the ones who effectively voted (minus Abstain), not over the total weight.

Or is it called Absolute because of that? Then we could have a fourth version: RelativePercentage (or plain Percentage).

AbsolutePercentage is basically a way of a dynamic M of N multisig. 51% automatically shifts from "3 of 5" to "5 of 9" to "6 of 10". It doesn't matter show does or does not vote, just how many sigs they are. This, like AbsoluteCount, behave very much like off-chain signature aggregation, but allow the signing group to evolve.

Your idea of RelativePercentage is like ThresholdQuorum without a quorum. That means if only one person voted on a quick election, they could get it passed. I would prefer to use a ThresholdQuorum with a low (eg. 10-20% quorum) instead of one with no checks at all there.

Anyway, feel free to add an issue for RelativePercentage and we can discuss it there. I am also happy for other types that do something not covered by these three. (note these are just binary elections, so most of the cool findings, like "ranked choice" do not apply here... we can work on multiple choice elections in another spec)

@ethanfrey ethanfrey deleted the add-threshold-to-cw3-flex branch December 16, 2020 20:55
Copy link
Member Author

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

I'd love hyperlinks or other way to connect these two descriptions better

/// A proposal of this type can pass early as soon as the needed weight of yes votes has been cast.
/// Declares a percentage of the total weight that must cast Yes votes in order for
/// a proposal to pass.
/// See `ThresholdResponse.AbsolutePercentage` in the cw3 spec for details.
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be cool to see these as hyperlinks.

These seem to be possible in RustDoc, and even better supported since 1.48
Also see the RFC for more details

(Side-note, we really need to review the RustDoc output once we push 0.4.0)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they are possible, and are used extensively in the crates documentation. Here in particular, I first added and afterwards then removed the hyperlink formatting, because these comments go to the json files.

We definitely should start using these links, and confirm that they are working correctly in the crates docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Side-note, we really need to review the RustDoc output once we push 0.4.0)

I propose we create an issue in cosmwasm for that, and consider / mention all the projects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, there are issues... for quite some time:

CosmWasm/cosmwasm#436

CosmWasm/cosmwasm#196

Just seems that all of us prefer to code than tidy up RustDoc

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.

Extend threshold types for multisig
2 participants