Add the possibility to include ERC721 token voting within OZ Governor module

[ivanminutillo]

🧐 Motivation
As stated in the governance introductory article:

OpenZeppelin Contracts contain the most common requirements out of the box. Beyond these, OpenZeppelin Governor could potentially be configured with ERC721 voting power so certain NFT owners can participate in governance when specified.

I was wondering if such possibility is already explored somewhere or not yet...
I imagine the customization and usecases could be infinite, but what would imply to have a basic version of the OZ Governor that uses ERC721 tokens to create proposals, delegate and vote?

This issue is a continuation of the #2868 one.
In this thread I explain a little bit more the usecase I'm working on.

📝 Details
Following @Amxx suggestions I drafted 3 new contracts:

• GovernorERC721Votes: A new governance flavor extension for voting weight extraction from an ERC721Votes token.
• ERC721Votes: A clone of ERC20Votes, with the same functions - included getPastVotes - but that inherits NFTPermit instead of draft-ERC20Permit
• NFTPermit: A draft implementation of Permit for the ERC721 token (taken from here)

As I am not really proficient with solidity, I expect there's lot of devil in the details when implementing even the most trivial things, so I was wondering if I'm totally out of track or not?

[Amxx]

Hello @ivanminutillo

Just for the records, the link you shared as a reference for NFTPermit is a prototype for this, which is not really the idea I would have of porting ERC2612 to ERC721.

Uniswap as an implementation of this concept ... but I think it would already be interesting to have a voting ERC721 WITHOUT any of the 712 signature. We can add that later, if there is some consensus around the "ERC721Permit" interface.

[frangio]

We want to implement this reusing as much code as possible with ERC20Votes. There is a prototype by @Amxx here.

[ivanminutillo]

thanks @frangio, is there any updates about it from your side? I'd likely take the time to test it out

[frangio]

@ivanminutillo We haven't made progress on the code as we're still finishing up the the 4.4 release. There's nothing to test yet but if you want to contribute we would welcome a PR.

[wighawag]

Hey, not familiar with the governor (just started to look) but just noticed that one path taken for erc721 votes was to adapt compound style checkpoints (which checkpoint balance at each transfer) (what Nouns DAO did)

While this work, this add a big overhead to transfer.

With NFT we can actually do better than ERC20: we can record the token used at the point of voting

From my initial look this would need a change of signature for catsVote where the frontend could inject the list of nft the user owns.

For NFT collection where it is common for individual to own many NFT, this method might not be practical but there are NFT collection that are small enough and there might be ways around for owner of many nft, like merkle trees

Maybe you already thought about this ?

[Amxx]

I can't say we thought about this.

We considered that the uint8 support, introduced by bravo, could be used in many different ways? It can be an enum, but it could also be a ranking. These "grades" would be used to compute the weighted average and decide the outcome of the vote.

For maximum bravo compatibility, we wanted our external function to share the same signature, so we couldn't add/remove or even change the types of the existing parameters. That being said, if you want to implement your approach, you can have the castVote, castVoteWithReason and castVoteBySig function disabled (by overriding _castVote with an empty function that reverts, and add functions of you own.

Interfaces that are commonly use to vote on compound like governors will not be able to have users vote, but the rest of the system (proposal / queueing/ execution) should be transparent.

you system would have to :

• create new public voting function
• extract the weight of each vote
• prevent double voting
• implement the _quorumReached and _voteSucceeded function

Note, the propose function requires getVotes to exist, so you may need to expose something like the NFT balance even though you don't use that for the actual voting

[frangio]

Interestingly, that proposal could be implemented by using the ERC721Enumerable interface. castVote can enumerate the tokens owned and mark them as already used for the proposal. This is a reasonable use case for enumerability on chain. It could still run out of gas if the number of tokens owned by a voter is too large. Not sure how that risk can be mitigated.

And it also doesn't implement vote delegation, which some people have pointed out as an important primitive to increase participation in governance.

[wighawag]

Enumerable can help but it has its limit as you say, and so providing the list in the castVote is I think perfectly fine. We can just allow multiple castVote call for higher number of tokens (since now we track vote not per address but per token)
Also with delegation, we would need to provide the list of ids anyway.

What I like about this system (using id) is that it does 2 things:

• do not cause overhead for transfer
• move the voting implementation outside of the token, much cleaner in my opinion and easier to experiment with different mechanism

One thing that @crazyrabbitLTC mentioned on twitter is that vote tracking at the point of vote casting do not prevent votes market (which checkpoints do prevent), but we can actually prevent that by saving the blockNumber on every transfer instead (and this is very cheap since we have space to store a uint96 along the owner address).

Need to look deeper in Governor but from what @Amxx is saying, this should be doable with minimum interface change, execpt maybe for delegation, which would now live outside of the token contract

[frangio]

Yes I think it's doable with a few changes but I'm not sure it can be cleanly built on top of our current Governor by just extending with inheritance. Seems like the _castVote and getVotes interface could use an extra argument for arbitrary data.

One way to implement it with the current interface is to have a separate function to input the list of NFTs that is then going to be consumed by the next cast vote by that account. The only problem is it needs to be put in storage temporarily. But it can all happen in the same transaction and be cleared up.

[cygnusv]

We want to implement this reusing as much code as possible with ERC20Votes.

What about the idea I originally mentioned in #2868 about extracting the checkpoint functionality to a separate abstract contract? That way, you would only have to extend from it for ERC20Votes, ERC721Votes, or whatever other voting power source you need. We already did this extraction in threshold-network/solidity-contracts#19 and used it not only for an ERC20 token, but also for a staking contract (threshold-network/solidity-contracts#25). I'd be very happy to contribute this here. BTW, this code has gone already through a security audit, if that's useful

[frangio]

@cygnusv Nice. That looks similar to @Amxx's prototype Voting.sol.

@JulissaDantes Is currently working on this in #2944, including extracting the checkpoint functionality to a library, so will be using the code as a reference.

[CrownOfBojji]

Hi I‘m trying to follow the threads here, is there any updates to the prototype of ERC721Votes? Our team also has a use case and really want to have something in place.

If we need to wait for 0.4.5, can I simply use this prototype this one to replace the draft-ERC721Votes?

[frangio]

@CrownOfBojji We're publishing a release candidate this week that includes ERC721Votes.

[omer2307]

Is draft-ERC721Votes.sol placed in the master directory is good to go?
Or is there more updated release(kind of afraid to inherit from file containing "draft" in its name)?

[Amxx]

@omer2307 It's marked as draft because it depends on EIP712, which is not final