ERC721 Voting power based on some property

Support Contracts
1

Hello everyone,

I see that Governance voting is currently compatible with ERC721 Tokens, and there is even this awesome example: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/extensions/draft-ERC721Votes.sol

However, one notable thing is that the voting power is "1 Vote per ERC721 Token". This seems to undermine most of the reason for using ERC721 in the first place (non-fungibility). In most cases, I imagine certain tokens would have more or less voting power based on some specific aspect about the token. In some example in the NFT space, there might be a "common item" and a "rare item", and since the rare one is harder to obtain, it should potentially get more voting power.

So I think adjusting voting power based on some property of the ERC721 token would be a fairly common use-case. But I am not sure how and am looking for advice. Could this be done by modifying draft-ERC721Votes.sol and looking for some metadata on the token?

Is all metadata stored in the URI (so some kind of lookup-Oracle would have to be used possibly), or is there some more native property of ERC721 that could hold this kind of votingPower value.

Would love to hear suggestions, thanks!

1 Like
2

Thanks for the suggestion @kiko.

We've thought about this, initially we wanted an internal function _votingPower(tokenId) that could be overriden by the user. But it turns out there are a few challenges to implement this.

The first is that the delegation mechanism is intended to make trading undelegated tokens efficient, by opting out of (or rather, never opting in to) the tracking of history of balance changes (i.e. voting power changes). If we allow different token ids to have different voting power, the moment an account goes from not delegating their tokens to delegating them, we need to know how much voting power has to be added to the delegate. The only way to know this amount appears to be to keep track of it as tokens are traded, which is the cost that delegation was trying to avoid. It wouldn't be exactly the same cost, it's a little less, but it's still non-zero overhead.

A second challenge is the question of whether voting power for one token id stays the same throughout its lifetime, or if it can change dynamically. If it can change dynamically that makes tracking voting power more complicated, and it wouldn't be as simple as overriding the _votingPower function. The fact that people may do this accidentally and we can't prevent it in the design of the contract was also a strong reason why we decided against implementing this feature.

It would definitely be valuable but we decided it was better to ship the feature with 1 vote per NFT.

It would be great to see someone experiment with votes where it can change. You can even build on the Votes abstraction which would make things a lot simpler.

2 Likes
3

I'm not sure if I fully understand the challenges of the first problem, but for this second, I think that the expectation for most people would be that voting power would be immutable. Mainly since NFTs are already valued for their immutability.

I'm glad you guys have considered it, and maybe if others are interested you could give it another attempt!

4

I don't think NFTs are inherently immutable. I'm sure I've seen some NFTs that have the ability to be "evolved" and change their attributes. The contract logic may be immutable, but attributes aren't, and voting power that is derived from attributes may be dynamic as well.

5

What I'm mainly trying to get across is that, if these tokens did not have a dynamic voting power, it would be pretty intuitive. I'm sure if you threw the concept of a ERC721 token with voting power and asked random people if they expected this property, most would say they expect it to be immutable.

Just suggesting that this might not as much of a hurdle that is worth blocking this feature as expected.

6

@frangio I'm still super interested in this feature, and would even like to try implementing an ERC721 Votes where each token has a different, arbitrary amount of voting power myself if nobody else is on it. Let me try and figure out the core engineering challenges here.

I've discussed why I don't think static vs. dynamic token problem is a big deal (static is simply much more intuitive, so that is the solution). It should also make engineering easier to reason about. I would like to follow up with this issue:

The first is that the delegation mechanism is intended to make trading undelegated tokens efficient, by opting out of (or rather, never opting in to) the tracking of history of balance changes (i.e. voting power changes). If we allow different token ids to have different voting power, the moment an account goes from not delegating their tokens to delegating them, we need to know how much voting power has to be added to the delegate. The only way to know this amount appears to be to keep track of it as tokens are traded, which is the cost that delegation was trying to avoid. It wouldn't be exactly the same cost, it's a little less, but it's still non-zero overhead.

This just manifests itself as higher gas cost when the tokens are traded, right? How much % higher would that be than not tracking?

Also, I've discussed previously why I think delegation counter-intuitive for users. Could we maybe scrap that whole system and build an ERC721 Votes system from the ground up?

Very interested in ideas here, and would appreciate your input!

7

As @Amxx mentioned here, the delegation problem seems to stem from when Alice votes with her token(s), then trade them, then Bob tries to vote with those same tokens.

Wondering, could this just be avoided by not allowing tokens that have been voted with to be traded (until the respective proposal is over)?

8

Just finding @JulissaDantes's awesome change here: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2944

Which has the 1 token = 1 vote principal by default, but PR description notes that this could be possibly adjusted?

So far this implementation account for 1 Token = 1 vote, if a _getVotingPower internal virtual function is created, taking the token Id and returning the voting power of such token, that would allow for differently weighted token(you could call it token rarity or token value) to be used as voting power.

I'm having some trouble understanding how a _getVotingPower function to implement this functionality would work exactly. If any of you guys have a better understanding, a small example would be really helpful!

9

Hi Kiko, thanks for showing interest in this feature. 

To explain a little what I stated in the PR, and merging what I said in the PR and Frangio´s first comment, _getVotingPower would just return what the address voting power is, right now we have the _getVotingUnits function that returns the balanceOf since each token represents one vote, but for the case you are proposing you would need to, as Fran said:

When an account goes from not delegating their tokens to delegating them, we need to know how much voting power has to be added to the delegate. The only way to know this amount appears to be to keep track of it as tokens are traded

And then return this tracking on the _getVotingUnits function. But remember we are trying to avoid this tracking cost when delegating so if you could think of a different way that would be great

2 Likes
10

You should carefully read ERC721Votes.sol and the documentation for Votes.sol which explains _getVotingUnits:

The challenge is in defining your implementation of _getVotingUnits. You will need to know how much weighted voting power an account holds.

2 Likes
11

Thanks for the feedback @JulissaDantes and @frangio! It seems we will need to store additional state for tracking which NFT has x amount of voting power, and since not everyone will want this is probably makes the most sense to do this on a per-project bases (by overriding these functions).

Still, I think going through a trivial example of using arbitrary NFT voting power would help a lot of developers such as myself, so your guidance here would be very valuable. Maybe we can go through a design process of this with static voting power, where each NFT's voting power is assumed to be immutable.

Brainstorming:

For additional state, we need (at the very least) a mapping of tokenId's to the arbitrary voting power that each one has:

mapping(uint256 => uint256) tokenIdToPower

We will assume that this state is already filled in properly by previous other processes, such as when the token was minted. So every token in this map will have a value by the time our voting module reads it.

then we can use this state to override how many votes are adjusted when token power changes:

function _afterTokenTransfer(
    address from,
    address to,
    uint256 tokenId
) internal override {
        _transferVotingUnits(from, to, tokenIdToPower[tokenId]);
        super._afterTokenTransfer(from, to, tokenId);
}

Overriding _getVotingUnits(address account) may be a bit more complicated. The current implementation of balanceOf(account) just returns the total number of NFTs under an account, which achieves the 1 NFT = 1 Vote mechanism.

I'm looking through https://eips.ethereum.org/EIPS/eip-721 to see if there are any convince functions that would help us distinguish between which NFTs an address owns. This seems like it would require some enumeration, so we could potentially use
RC721Enumerable's tokenOfOwnerByIndex(address _owner, uint256 _index) to acquire this information.

Let's assume that by some method we can achieve this, then an override function might look like:

function _getVotingUnits(address account) internal view override returns (uint256) {
    // TODO: Figure out a way to find all tokenIds an account has
    uint256[] ownedTokenIds = // ...;

    uint256 balance;
    for (uint256 i = 0; i < ownedTokenIds.length; ++i) {
        balance += tokenIdToPower[tokenId];
    }
    return balance;
}

So as I understand it, the main blocker here is an efficient means for distinguishing which tokenId's exist under an account. Please let me know if I have misunderstood any of the above or if there is anything that can be improved in this design.

Other Thoughts:

If ways to make this easier by limiting delegation functionality (such as post-vote transfers not allowed or only self-delegation allowed), that would be great to know.

Thanks again for your time and attention, looking forward to what you guys have to say!

12

Your reasoning is correct. One piece you're missing is that listing the token id's for an account is already available in ERC721Enumerable. But keep in mind that ERC721Enumerable adds a lot of cost to transfers, so we do not recommend it generally.

That said, your code loops over an array of unbounded length, and this is bad practice for smart contracts because it can result in out of gas errors. If you accept that risk (which I would not recommend) and the extra cost, the code would be as simple as your code combined with ERC721Enumerable.

Alternatively, you would need to keep a mapping mapping(address => uint256) accountToPower that you update incrementally on every _afterTokenTransfer. This is additionally to transferring voting units.

1 Like
13

Thanks for the explanation, I think I see the core problem now.

I am wondering, could this not all be avoided if we just have a rule: you can not transfer any tokens that have been voted with until that proposal has ended. In this way, it seems like we can avoid the tracking completely, and have a more efficient system of simply add a hasVoted property to the ERC721 token itself that can be checked when a transfer event tried to occur (conveniently using _beforeTokenTransfer).

Thoughts?

14

You can do this but it probably ties the NFT contract to a single governance contract, which is not always desirable.

15

Makes sense, I will try to stick with the OZ standard functionality mainly for interoperability and ease-of-understanding purposes. I think that method of maintaining a custom accountToPower or something like it will work well.

More Brainstorming

I have an alternative idea which may even less overhead, but I am not sure if it would break any interoperability: instead of a new mapping, we just re-use mapping(address => uint256) _balances but instead of the # of tokens, it represents the sum'd voting power.

In this way, something close to the default ERC721Votes behavior could be used: _getVotingUnits returns the balanceOf still.

_afterTokenTransfer would be almost the same, instead replacing _transferVotingUnits(from, to, 1); with _transferVotingUnits(from, to, tokenIdToVotingPower(tokenId)); (which still a separate mapping but only one mapping (uint256 => uint256) tokenIdToVotingPower;.

In this way, it shifts the power calculation to _mint, _transfer, and _burn to adjust the balance properly, so we will override those. For example, this is the old mint:

    function _mint(address to, uint256 tokenId) internal virtual {
        require(to != address(0), "ERC721: mint to the zero address");
        require(!_exists(tokenId), "ERC721: token already minted");

        _beforeTokenTransfer(address(0), to, tokenId);

        _balances[to] += 1;
        _owners[tokenId] = to;

        emit Transfer(address(0), to, tokenId);

        _afterTokenTransfer(address(0), to, tokenId);
    }

For our new mint to work, we will track the total number of tokens (tokenCount) and increment it for a generated tokenId. This allows our mint to take in a tokenPower, and assign it to the next avalliable tokenCount. This gives us:

    function _mint(address to, uint256 tokenPower) internal virtual override {
        require(to != address(0), "ERC721: mint to the zero address");
        require(tokenPower != 0, "ERC721: power must be > 0");

        tokenCount++;
        uint256 tokenId = tokenCount;

        _beforeTokenTransfer(address(0), to, tokenId);

        tokenIdToVotingPower[tokenId] = tokenPower;

        _balances[to] += tokenPower;
        _owners[tokenId] = to;

        emit Transfer(address(0), to, tokenId);

        _afterTokenTransfer(address(0), to, tokenId);
    }

(I realize I cannot override the function directly due to the difference in naming tokenId => tokenPower, in a real example I will pass in tokenId to internally overriden mint that will have some of the storage removed, it will be equivalent to the above though).

The same kind of method should work fine for _transfer and _burn, referencing tokenIdToVotingPower when we add or subtract from the balances map.

I believe this is minimal overhead (only new storage is mapping (uint256 => uint256) tokenIdToVotingPower and uint256 tokenCount). But there may be some fundamental flaws that I am not seeing. I'm also not sure how bad it would be to shift how balanceOf is interpreted. Would the break the kinds of Web3 UIs that try to display total tokens (e.g. Tally)?, or would this actually help to make that instantly interoperable since balanceOf would normally show voting power in these UIs?

Would absolutely love to get your thoughts @frangio and @JulissaDantes!

1 Like
16

Here is a more complete contract example with full _mint, _transfer, and _burn overrides:

contract NFTVotesToken is ERC721, ERC721Votes {
    uint256 tokenCount;
    mapping(uint256 => uint256) tokenIdToVotingPower;

    function initialize() public payable initializer {
        __ERC721_init("NFT Votes Token", "NVT");
    }

    function _afterTokenTransfer(
        address from,
        address to,
        uint256 tokenId
    ) internal override(ERC721, ERC721Votes) {
        _transferVotingUnits(from, to, LibVotesPower.votesStorage().tokenIdToVotingPower[tokenId]);
        super._afterTokenTransfer(from, to, tokenId);
    }

    function mint(address to, uint256 tokenPower) public {
        require(to != address(0), "ERC721: mint to the zero address");
        require(tokenPower != 0, "ERC721: power must be > 0");

        tokenCount++;
        uint256 tokenId = tokenCount;

        _beforeTokenTransfer(address(0), to, tokenId);

        tokenIdToVotingPower[tokenId] = tokenPower;
        _balances[to] += tokenPower;
        _owners[tokenId] = to;

        _mint(to, tokenId);
    }

    function _mint(address to, uint256 tokenId) internal override(ERC721) {
        emit Transfer(address(0), to, tokenId);
        _afterTokenTransfer(address(0), to, tokenId);
    }

    function transfer(
        address from,
        address to,
        uint256 tokenId
    ) public {
        _transfer(from, to, tokenId);
    }

    function _transfer(
        address from,
        address to,
        uint256 tokenId
    ) internal override(ERC721) {
        require(ERC721.ownerOf(tokenId) == from, "ERC721: transfer from incorrect owner");
        require(to != address(0), "ERC721: transfer to the zero address");

        _beforeTokenTransfer(from, to, tokenId);

        // Clear approvals from the previous owner
        _approve(address(0), tokenId);

        uint256 tokenPower = tokenIdToVotingPower[tokenId];
        _balances[from] -= tokenPower;
        _balances[to] += tokenPower;
        _owners[tokenId] = to;

        emit Transfer(from, to, tokenId);

        _afterTokenTransfer(from, to, tokenId);
    }

    function burn(uint256 tokenId) public {
        _burn(tokenId);
    }

    function _burn(uint256 tokenId) internal override(ERC721) {
        address owner = ERC721.ownerOf(tokenId);

        _beforeTokenTransfer(owner, address(0), tokenId);

        // Clear approvals
        _approve(address(0), tokenId);

        ERC721Storage.layout()._balances[owner] -= tokenIdToVotingPower[tokenId];
        delete tokenIdToVotingPower[tokenId];
        delete _owners[tokenId];

        emit Transfer(owner, address(0), tokenId);

        _afterTokenTransfer(owner, address(0), tokenId);
    }
}

Would be great to get some feedback, thanks again folks!

1 Like
17

@frangio or @JulissaDantes or anyone else that's interested in this, I would love to get your feedback on this proposed solution!

18

The code looks like it's based on modified OpenZeppelin contracts. These variables are actually private so they couldn't be modified:

        _balances[to] += tokenPower;
        _owners[tokenId] = to;
19

@frangio Thank you for this amazing answer!

I do see why people might want this, but I completely agree this should be out of scope for the first 721Votes version.

@kiko I also understand your interest in this. I personally see more uses for ERC1155_Votes with different power levels for different tokens. I wonder if you can maybe share what kinds of uses you're thinking about here.

I do think that now that gas is less of a concern there's room for making a more complex voting system without an option to opt-out. I do agree that would be more intuitive, and, on L2s at lest, the cost to the user would be just about negligible.

1 Like
20

That's a really interesting point that I think is worth exploring!