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

cw3-flex-multisig uses voting power from a snapshot of the block the proposal opened #160

Merged
merged 10 commits into from
Dec 9, 2020

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Dec 8, 2020

Closes #141

Uses the preferred "complex" solution. We no longer close open proposals when the group changes, but handle it gracefully

Count all votes with state at beginning of voting period

  • The total weight needed to pass is fixed at the beginning and never changes
  • If a member is updated and they haven't voted yet, record the previous weight (the value at the beginning of the proposal) in some local cache inside cw3-flex-multisig. When the member votes, the weight there overrides the current weight in the group.

We do this with max 1 write per updated weight, only if this was the first time it has changed since the most recent proposal. -> O(diffs), regardless of how many proposals there are.

Future improvement: We currently iterate over all open proposals to find the latest start height. With a clever compound secondary index, we could do this with one read.

TODO:

  • Rename MemberDiff fields (old, new)
  • Test voting on second proposal doesn't use snapshot
  • Test complex case
  • Improve performance

@maurolacy
Copy link
Contributor

Hi, I'll review it now, so this may be asked in the code, but anyway, I have a couple questions:

* If a member is updated and they haven't voted yet, record the previous weight (the value at the beginning of the proposal) in some local cache inside `cw3-flex-multisig`. When the member votes, the weight there overrides the current weight in the group.

What about addition of new members? Can they vote?
What about deletion of members that existed when the proposal was created?

@ethanfrey
Copy link
Member Author

I should probably add more code comments. The test covers both the new and deleted cases.

In any case, use the voting power (or lack thereof) at the block height when the proposal was created.

It doesn't snapshot per tx, but at the beginning of the block in which the proposal was created, which should be fine for any reasonable case

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.

Seeing and thinking about the concept of snapshots, it occurred to me that all that logic could be implemented at the level of the group contract directly.
It would be more efficient (implemented once, and available for all clients of the group).
It would be enough to extend the query API to allow for a (optional) date / height parameter. If that parameter is not supplied, the most recent / updated value is returned.

What do you think?

@maurolacy maurolacy self-requested a review December 9, 2020 08:49
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 to me.

Probably my biggest concern would be for this functionality to be moved to the group contract entirely.

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

Probably my biggest concern would be for this functionality to be moved to the group contract entirely.

This is a good question. Below is my reasoning (I thought it was more efficient)

I want to pull out the snapshot function in a simple module that can be imported and reused by various contracts. The question is implement in the receiver or in the producer

If producer (group), the receiver (multisig) needs to inform the producer every time a proposal opens or closes, so it tracks needed changes. And the queries could not be raw queries (as it uses iterator to find relevant update). Thus making queries much more expensive that before this pr (raw query) or now (local state + raw query).

It would however, remove the need for the external hook pattern, which would save gas on updates.

With the hooks, you add N messages to every state change, but the queries are cheap. I guess it depends if we assume there are more state changes or more proposals/votes.

@ethanfrey
Copy link
Member Author

Let's discuss this design.

BTW, I wanted to make diff+snapshot design an easily importable module to reuse it. Whether that is using hooks, or stored in the producer is an open question.

Let's discuss it a bit more, but my thought is.

  1. Merge this pr, do following as separate steps.
  2. Refactor this out so core diff / snapshot logic is in a common library
  3. Make a pr not using hooks, but rather snapshots in the producer (ideally minimal code changes needed), then we can compare the two approaches. And also reference that pr and discussion in any design doc - to show the two patterns

@ethanfrey
Copy link
Member Author

Also, I think there will be more different cw4 implementations than composable cw3 ones (based on external groups), but I may be wrong.

@maurolacy
Copy link
Contributor

maurolacy commented Dec 9, 2020

I want to pull out the snapshot function in a simple module that can be imported and reused by various contracts. The question is implement in the receiver or in the producer

That can be done, and is independent of this question.

If producer (group), the receiver (multisig) needs to inform the producer every time a proposal opens or closes, so it tracks needed changes. And the queries could not be raw queries (as it uses iterator to find relevant update). Thus making queries much more expensive that before this pr (raw query) or now (local state + raw query).

If implemented in producer, I'll do it independently of any receiver. The group contract can have a flag on creation (or even after creation, with an API call); enable_snapshots or similar.

It would however, remove the need for the external hook pattern, which would save gas on updates.

Yes. That, and centralization of impl would be the biggest advantages, as I see it.

With the hooks, you add N messages to every state change, but the queries are cheap. I guess it depends if we assume there are more state changes or more proposals/votes.

I was supposing updates are relatively rare, and we would in general have more votes than voters's changes.

@maurolacy
Copy link
Contributor

Let's discuss this design.

BTW, I wanted to make diff+snapshot design an easily importable module to reuse it. Whether that is using hooks, or stored in the producer is an open question.

Yes, exactly.

Let's discuss it a bit more, but my thought is.

1. Merge this pr, do following as separate steps.

2. Refactor this out so core diff / snapshot logic is in a common library

3. Make a pr not using hooks, but rather snapshots in the producer (ideally minimal code changes needed), then we can compare the two approaches. And also reference that pr and discussion in any design doc - to show the two patterns

Sounds great. Let's do it.

@ethanfrey
Copy link
Member Author

Thanks for the review and feedback on the architecture.

You propose keeping a snapshot of all data, not just just those needed for open proposals?

@ethanfrey
Copy link
Member Author

I like the idea of the enable snapshot flag on init

@maurolacy
Copy link
Contributor

maurolacy commented Dec 9, 2020

You propose keeping a snapshot of all data, not just just those needed for open proposals?

Yes. Mostly for simplicity. Contract can even support setting an expiration height / time, i. e. "generate snapshots up until this block / date".

@ethanfrey ethanfrey merged commit 7361654 into master Dec 9, 2020
@ethanfrey ethanfrey deleted the snapshot-of-voters-begin branch December 9, 2020 20:04
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.

Multisig handles changes to group membership
2 participants