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

Extend Governor with parameterized votes #3043

Merged
merged 19 commits into from Mar 1, 2022

Conversation

apbendi
Copy link
Contributor

@apbendi apbendi commented Dec 21, 2021

This PR lays the groundwork for a future implementation of #2929, along with other potentially useful Governance extensions, by making the _castVote interface more flexible. In particular, this PR follows @frangio's suggestion and adds a data parameter to the internal, virtual _castVote method signature, along with associated countVote methods.

The idea is this data field can be used to encode arbitrary data about the vote in question, enabling many interesting voting mechanisms via Governor extension. It can be coupled with a reporting of which kind of standardized voting mechanisms are supported in the COUNTING_MODE function.

I'm opening this PR as draft for two reasons:

  1. To get feedback on the general approach, as suggested by @frangio and implemented here. It seems reasonable to me but I'm very open to feedback!
  2. To get suggestions on better naming. data as a parameter name and WithData for method extensions seems very generic. I think we can do better but I don't have any great ideas. One possibility would be countingContext, countingData, or something like this. Any ideas?

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Dec 21, 2021

Thank you @apbendi! I agree that "data" is a bad name, I am thinking what we're adding here are "parameters" for the vote. So I'd go with "params", or if we want to tie it into the "counting" concept it could be "counting parameters". What do you think?

Have you tried to implement something like #2929 on top of these changes? It would be good to see if it actually works well for that purpose.

What I think is missing is a virtual function _defaultParams() with a default return value of empty string, and we can use in place of the hardcoded empty string there is in the PR currently. A module like fractional voting would override _defaultParams() to return "100%" or something like that... not sure if I remember exactly how that was meant to work for fractional voting.

@apbendi
Copy link
Contributor Author

apbendi commented Dec 21, 2021

Thanks for the quick reply @frangio!

I am thinking what we're adding here are "parameters" for the vote. So I'd go with "params"

This seems reasonable, and certainly an improvement over data. I'll make this change.

Have you tried to implement something like #2929 on top of these changes? It would be good to see if it actually works well for that purpose.

I'm actively working on this, yes. Can give you some impressions as I get a little further.

What I think is missing is a virtual function _defaultParams() with a default return value of empty string

Makes sense. I will add this?

Also, any opinion on this.

Thanks!

@frangio
Copy link
Contributor

frangio commented Dec 22, 2021

Can give you some impressions as I get a little further.

Please do! We want to make sure it serves the intended purpose.

One more thing I wanted to suggest is just adding a single new function with both reason and params, so as not to explode the number of functions. We've also found that memory parameters in external/public functions contribute to bytecode size quite a bit, so adding the one function rather than two will be better in that sense.

@apbendi
Copy link
Contributor Author

apbendi commented Dec 22, 2021

hey @frangio, I believe I've addressed all feedback. I'm thinking I can take this PR out of Draft if that works for you. Would you like me to squash everything back to one commit? Also can you help me understand the codecov CI failure I'm seeing— looks like maybe the code coverage of the diff is not high enough, is that right?

@frangio
Copy link
Contributor

frangio commented Dec 22, 2021

Sorry @apbendi I was actually waiting to hear back about your experience implementing the fractional votes before moving forward with this. Just want to make sure we're on the right track with this PR.

@apbendi
Copy link
Contributor Author

apbendi commented Jan 3, 2022

Sorry @apbendi I was actually waiting to hear back about your experience implementing the fractional votes before moving forward with this. Just want to make sure we're on the right track with this PR.

Ahh ok, good to know. We have a bit of a chicken/egg problem because I didn't want to go too far with my implementation before knowing the approach would be accepted 😅

I'll try to break us out of this infinite loop by pushing mine forward a bit soon so I have more concrete feedback to give!

This was referenced Jan 5, 2022
@Amxx
Copy link
Collaborator

Amxx commented Jan 13, 2022

Hello @apbendi, I'm joining the conversation to discuss possible additions to this PR.

When a vote is cast there are basically 3 things happening:

  1. getting the weight associated with the vote
  2. making sure that weight is not re-used to perform another vote latter
  3. aggregate that weight to the voting result.

Operation 1 is done in the "Votes" modules, usually, be checking the snapshotted balance of a Comp-like token
Operation 2 and 3 are performed by the "Counting" module.

While your PR proposes to pass an additional "params" to the Counting module, I personally believe it should also be passed to the Votes module. I created this commit showing how I would do that, and how I think it could be used for voting with non-snapshotted NFT contracts.

What do you think?

@Amxx
Copy link
Collaborator

Amxx commented Jan 13, 2022

I also think we may want a castVoteWithReasonAndParamsBySig. Not having it would kill the "vote with signature" (metatx) option when params are required.

@apbendi
Copy link
Contributor Author

apbendi commented Jan 13, 2022

Hi @Amxx 👋

Great idea! I was going to ask for an example of when this data my needed by Votes module, but your ERC721 implementation provided a perfect example :)

I see no issue with this approach. How would you like to go about handling the merge dependencies here? Shall I rebase this branch to master and apply just the getVotes related changes onto this branch?

I also think we may want a castVoteWithReasonAndParamsBySig. Not having it would kill the "vote with signature" (metatx) option when params are required.

I have no issue with this, and believe I may have included originally but removed it based on this comment. Only one follow up question: are there any concerns about contract size constraints?

@Amxx
Copy link
Collaborator

Amxx commented Jan 13, 2022

You can fast forward your branch to my commit, or just apply it. Whatever works best for you.

For the size constraints, it could become an issue ... but I'd argue that as long as the mock can be deployed, that is good. In the worst case, we should probably cut revert strings before we remove features.

@apbendi
Copy link
Contributor Author

apbendi commented Jan 13, 2022

@Amxx

You can fast forward your branch to my commit, or just apply it. Whatever works best for you.

Cool, so do we want to include the ERC721 voting implementation in this PR?

For the size constraints, it could become an issue ... but I'd argue that as long as the mock can be deployed, that is good. In the worst case, we should probably cut revert strings before we remove features.

Fair enough, I guess one consideration I was thinking about is how much space implementers of custom extensions might need.

@Amxx
Copy link
Collaborator

Amxx commented Jan 13, 2022

Cool, so do we want to include the ERC721 voting implementation in this PR?

@frangio what do you think?

@frangio
Copy link
Contributor

frangio commented Jan 13, 2022

No I wouldn't include that yet. We should have a small mock for testing the params though.

@apbendi
Copy link
Contributor Author

apbendi commented Jan 17, 2022

@frangio I modified @Amxx's commit to remove the 721 implementation and also rebased this whole branch to master. I'll work on getting a mock and some proper tests set up. Also, to confirm, I should (re)add the castVoteWithReasonAndParamsBySig method?

@Amxx
Copy link
Collaborator

Amxx commented Jan 17, 2022

Thank you a lot @apbendi

IMO yes, we should re add castVoteWithReasonAndParamsBySig

@Amxx
Copy link
Collaborator

Amxx commented Jan 17, 2022

When we get to it, we'll have to add a breaking change notice (internal module interface + override change)

@apbendi
Copy link
Contributor Author

apbendi commented Jan 17, 2022

Hey @frangio and @Amxx, I've created a Mock and used it to test usage of the Governance extension parameters. I've also implemented castVoteWithReasonAndParamsBySig and included in the tests. Could you take a quick look and tell me if everything looks good to you?

A couple of additional questions:

  • Should we consider adding params to the VoteCast event emitted by the Governor?
  • What do I need to do to update the changelog/breaking change notice? Just an entry in the "Unreleased" section at the top of the CHANGELOG.md?

@apbendi
Copy link
Contributor Author

apbendi commented Jan 19, 2022

Hey @frangio and @Amxx, friendly bump here :)

@frangio
Copy link
Contributor

frangio commented Jan 24, 2022

Ohh the event. That's annoying. Adding params to VoteCast is a breaking change. If we want to log params in an event (and we probably should), we need to emit a separate event, but how should a consumer of the events link it to the corresponding VoteCast? And do we always emit it or for example only if params.length > 0?

For the changelog just a description in Unreleased, yes.

Mock and tests look good.

@frangio
Copy link
Contributor

frangio commented Jan 25, 2022

Another idea to avoid having to merge data from two separate events: emit the current VoteCast only if params.length == 0, and emit a new event with params if params.length > 0.

I think this is better. It drops compatibility with GovernorBravo events but that doesn't make any sense when you introduce params because the vote can't be interpreted unless you know them.

@frangio
Copy link
Contributor

frangio commented Feb 16, 2022

Can you add a mention of params in the docs for COUNTING_MODE? If a counting module makes use of encoded params, it should include this in the COUNTING_MODE string under a new params key with a unique name that describes the behavior.


This is looking good to me. I'd like @Amxx to give this a review as well, but I don't really have further comments. I will want to get some external feedback before releasing this but I'm ready to merge it after these last few comments are resolved.

@frangio frangio changed the title Add a data parameter to the _countVote interface Extend Governor with parameterized votes Feb 21, 2022
@apbendi apbendi requested review from frangio and Amxx February 26, 2022 04:34
@apbendi
Copy link
Contributor Author

apbendi commented Feb 26, 2022

hey @frangio and @Amxx, thanks again for your patience. I'm back from ETHDenver, recovered from Covid, and finally had a chance to address your latest comments. I believe everything has been resolved. Please let me know if anything else is needed before this can be merged. Can't say it enough: really appreciate your feedback through this process!

@apbendi
Copy link
Contributor Author

apbendi commented Feb 28, 2022

hey @frangio friendly bump on this PR :)

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the patience and working with us!

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.

None yet

4 participants