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

Custom complaints voting contract #529

Merged
merged 9 commits into from
Oct 31, 2022
Merged

Custom complaints voting contract #529

merged 9 commits into from
Oct 31, 2022

Conversation

hashedone
Copy link
Contributor

Closes #44

Quick summary:

The new contract is based on fixed-multisig contract from cw-plus, but it is heavily modified. Instead of containing its own proposals, it is always created to vote on a single "phantom" proposal, always the complaint from ap-voting contract. Additionally executing the contract requires adding the summary and ipfs_link so it is possible to render decisions by the parent ap-voiting as the execution result.

The ap-voting contract had to be aligned to use the new contract (slight changes in the instantiation of the voting contract, also a bit of cleanup).

Lastly - one new MT is created to test the full complaint flow - from creating it up to rendering decisions by passed voting.

@maurolacy
Copy link
Contributor

The ap-voting contract had to be aligned to use the new contract (slight changes in the instantiation of the voting contract, also a bit of cleanup).

That's a problem, because that contract is already deployed in tgrade mainnet. Of course, we can always upgrade / migrate it, but it would be nice if we can avoid that step.

Will review the code asap. Thanks for working on this.

@daniellarita
Copy link

@abefernan just FYI that this issue is in review and almost complete so afterwards you should be able to continue with arbiter pool

#529

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.

First pass review.

Looks good. Will review the main contract logic and tests next.

.circleci/config.yml Outdated Show resolved Hide resolved
@@ -242,21 +243,22 @@ fn execute_propose_arbiters(
weight: pass_weight as u64,
},
max_voting_period: Duration::Time(config.waiting_period.seconds()),
complaint_id: case_id,
Copy link
Contributor

@maurolacy maurolacy Oct 21, 2022

Choose a reason for hiding this comment

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

The new contract could get this information from its own label, below. Not very robust, but if this is the only compatibility breaking change, worth considering IMO.

[package]
name = "tgrade-dispute-multisig"
version = "0.14.0"
authors = ["Ethan Frey <[email protected]>"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Credit author proper?

Also, it would have been nice to first copy the cw3-fixed-multisig code, commit that, and then change / adapt it. That way the commit log / diffs could be checked as a reference.

Copy link
Contributor

@maurolacy maurolacy Oct 21, 2022

Choose a reason for hiding this comment

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

Bonus point: It would have been nice to use the voting-contract package, and avoid having yet another similar but still slightly different copy of the voting logic around.

Update: See confio/poe-contracts#194. One more for the migration I guess.

contracts/tgrade-dispute-multisig/NOTICE Outdated Show resolved Hide resolved
@@ -0,0 +1,65 @@
# CW3 Fixed Multisig
Copy link
Contributor

Choose a reason for hiding this comment

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

Either remove or update this.

contracts/tgrade-dispute-multisig/src/msg.rs Outdated Show resolved Hide resolved
contracts/tgrade-dispute-multisig/src/msg.rs Show resolved Hide resolved
}
}

//#[cw_serde]
Copy link
Contributor

@maurolacy maurolacy Oct 21, 2022

Choose a reason for hiding this comment

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

Remove commented code.

It would have been nice that your changes were over this original code. And that the original version were part of the commit history for this contract.

pub status: Status,
}

impl State {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you renamed Proposal to State and are now passing Config as a parameter.

Other than that, I assume all this is standard voting state handling logic.

Copy link
Contributor

@maurolacy maurolacy Oct 21, 2022

Choose a reason for hiding this comment

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

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.

LGTM.

Approving, as requested changes / improvements (moving voting logic to voting-contract) can be done in the future.

@hashedone
Copy link
Contributor Author

As @maurolacy accepts to change stuff in future, merging this (all CI issues fixed)

@hashedone hashedone merged commit 3e79c71 into main Oct 31, 2022
@hashedone hashedone deleted the custom-voting-contract branch October 31, 2022 11: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.

3 participants