ERC721 voting with dynamic voting power

[RitzyDevBox]

🧐 Motivation
I'm working on an NFT based DAO, currently I don't see a way to override the voting power for an individual NFT.

📝 Details
Expose an Interface on in the ERC721Votes.sol

  setVotingPower(uint256 tokenId) {
  }

Update the GetVotes and GetPastVotes to support dynamic voting power

function getVotes(address account) public view virtual override returns (uint256) {
    
    uint256[] memory ownedNFTs =_nftOwnerCheckpoints[account];
     //Pseudo code
    return ownedNFTs.sum(n =>_delegateNFTCheckpoints[n].latest())

}

[Amxx]

Hello @RitzyDevBox

function getVotes(address account) public view virtual override returns (uint256) {

uint256[] memory ownedNFTs =_nftOwnerCheckpoints[account];
 //Pseudo code
return ownedNFTs.sum(n =>_delegateNFTCheckpoints[n].latest())

}

The ownedNFTs is an unbounded array, that might be arbitrarily expensive to fetch
The sum part is also arbitrarily expensive to read/compute.

While this approach is technically possible, it would result in very expensive voting, and could cause DOS issues when the cost of voting becomes greater then the size of a block.

[RitzyDevBox]

The DDOS would be user specific, so only that particular user wouldn't be able to vote. Potentially this can be solved by setting a hard cap on the number of NFTs owned by the user, and creating it as a configuration parameter, and breaking it out into a separate contract. I need to think on this more. I may have to write this anyway so I'll probably create a PR for this after I do more research

…

On Wed, Jan 25, 2023, 8:42 AM Hadrien Croubois ***@***.***> wrote: Hello @RitzyDevBox <https://github.com/RitzyDevBox> function getVotes(address account) public view virtual override returns (uint256) { uint256[] memory ownedNFTs =_nftOwnerCheckpoints[account]; //Pseudo code return ownedNFTs.sum(n =>_delegateNFTCheckpoints[n].latest()) } The ownedNFTs is an unbounded array, that might be arbitrarily expensive to fetch The sum part is also arbitrarily expensive to read/compute. While this approach is technically possible, it would result in very expensive voting, and could cause DOS issues when the cost of voting becomes greater then the size of a block. — Reply to this email directly, view it on GitHub <#3996 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYR4EL2I7YXY7I5W4BSS5ODWUEUWDANCNFSM6AAAAAAUF225VM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[frangio]

The right way to do this would be to use the params system. The user would call voteWithParams where the parameters are their owned token IDs they want to vote with. The array of token ids is iterated to check the ownership, compute the voting power, and validate that it hasn't been used to vote for the particular proposal yet.

This works because the params array doesn't need to include all owned token IDs, and a token owner can vote multiple times with different batches of tokens if the array is so large that voting would run out of gas.

This Governor extension would also be valuable because it allows voting with a standard non vote-enabled ERC721 token, which are cheaper to transfer, and doesn't require an upgrade to pre-existing tokens.

[RitzyDevBox]

params would work however since voting would potentially be transition to tokenId, the interface for has voted should be updated with a bytes calldata params as well

`function hasVoted(uint256 proposalId, address account, bytes calldata params) public view virtual returns (bool);`

[frangio]

I see. Changing that breaks compatibility. In my opinion hasVoted should be left as is. It's not really used to determine if an account can vote again so it's merely informational.

[RitzyDevBox]

There are a few more issues related to dynamic voting power.

The weight resolution is wrong, since params requires to option of partial votes per NFT

    uint256 weight = _getVotes(account, proposal.voteStart.getDeadline(), params);
    _countVote(proposalId, account, support, weight, params);

Also this will run into issues for:

    function _getVotingUnits(address) internal view virtual returns (uint256);

Since we're required to calculate the voting by the address. This can lead to Denial of Service, when attempting to delegate votes.

I think Ideally this might be suited for a better module. There are a few related issues.

1. User's cannot Delegate a single NFT. (NFT delegation is common for the gaming industry people lend their nfts for a share of the profits)
2. User voting is currently all or none.

I've solved this problem by having an external position manager contract. However that the way I'm doing position management is unique to my case as I do not require the checkpoint history all voting is dependent on the live environment.

I've also modified _countVote to return the weight instead, (so this emits voting weights properly), internally this is calculated from tokens position instead of the accounts full position.

[frangio]

The weight resolution is wrong, since params requires to option of partial votes per NFT

Can you explain this further? I didn't understand.

Also this will run into issues for: _getVotingUnits

_getVotingUnits is used for vote-tracking in the token itself. The suggestion here is to make a Governor module, not an ERC721 extension.

Feel free to share links to the code you're talking about.

[RitzyDevBox]

Hmm, I was trying to follow the existing interface too literally. I was treating GetVotes(account) as a way to get the total voting power for a user. However with the current architecture this would lead to DOS

Some architecture changes may improve gas though: shift count vote to return the weight will this way the params don't have to get parsed twice

function _castVote(
    uint256 proposalId,
    address account,
    uint8 support,
    string memory reason,
    bytes memory params
) internal virtual returns (uint256) {
    ProposalCore storage proposal = _proposals[proposalId];
    require(state(proposalId) == ProposalState.Active, "Governor: vote not currently active");

    //move the logic for obtaining the weight into _count vote
    //uint256 weight = _getVotes(account, proposal.voteStart.getDeadline(), params);
    uint256 weight = _countVote(proposalId, account, support, weight, params);

    if (params.length == 0) {
        emit VoteCast(account, proposalId, support, weight , reason);
    } else {
        emit VoteCastWithParams(account, proposalId, support, weight , reason, params);
    }

    return weight;
}

The vote seems like it might be better to be determined by the vote count instead, since NFT voting is partial voting. and

The way I have things working currently is. Voting Power is loosly coupled from the NFT. It's

1. An external position manager which handles voting power.
2. the position manager handles tracking via the User total votes and the NFT itself.

[Danielfisseha]

bold### title###