Cast Votes Readability#

Informational

castVoteInternal is one of the most important governance functions in the codebase. It is worth ensuring the code is not only functional, secure, and bug-free, but also highly readable, even self documenting.

Currently, the function spans over 70 lines of logic and has mirrored conditionals on lines 481 and 524:

481 if (voterCommunityId < 209) {

...

524 if (voterCommunityId < 209) {

Recommendation#

The readability of this critical governance function might be improved by breaking the function logic into smaller discrete components. Specifically, the code might benefit from implementing helpers for packing hasVoted in lowerVotes vs. upperVotes.

See the below alternative with smaller, discrete functions

function setHasVotedLower(
    ProposalVote storage proposalVote,
    uint256 voterCommunityId
) internal {
    // bring lowerVotes into the stack
    uint208 lowerVotesWord = proposalVote.lowerVotes;
    // Bit flag at index voterCommunityId - 1 (since id starts at 1)
    // indicates if the voter has voted on this proposal.
    bool voted = lowerVotesWord & (1 << (voterCommunityId - 1)) ==
        (1 << (voterCommunityId - 1));

    require(!voted, "Governor::castVoteInternal: voter already voted");

    // Using lower votes word.
    proposalVote.lowerVotes =
        lowerVotesWord |
        uint208(1 << (voterCommunityId - 1));
}

function setHasVotedUpper(
    ProposalVote storage proposalVote,
    uint256 voterCommunityId
) internal {
    // Community id is too large to fit in lowerVotesWord. Therefore,
    // pull from the upperWords. Upper words indecies start at 209.
    uint256 wordIndex = (voterCommunityId - 209) / 256;
    // Get bit index within the word.
    uint256 bitIndex = (voterCommunityId - 209) % 256;
    uint256 upperVotesWord = proposalVote.upperVotes[wordIndex];

    // Bit flag at index bitIndex indicates if the voter has voted on this proposal.
    bool voted = upperVotesWord & (1 << bitIndex) == (1 << bitIndex);

    require(!voted, "Governor::castVoteInternal: voter already voted");

    // Using up votes word.
    proposalVote.upperVotes[wordIndex] = upperVotesWord | (1 << bitIndex);
}

function castVoteInternal(
    address voter,
    uint256 proposalId,
    VoteType support
) internal returns (uint16) {
    ProposalVote storage proposalVote = proposalVotes[proposalId];
    uint256 voterCommunityId = token.getCommunityId(voter);

    require(
        voterCommunityId != 0,
        "Governor::castVoteInternal: voter invalid"
    );

    if (voterCommunityId < 209) {
        setHasVotedLower(proposalVote, voterCommunityId);
    } else {
        setHasVotedUpper(proposalVote, voterCommunityId);
    }

    Proposal storage proposal = proposals[proposalId];

    require(
        state(proposal, proposalVote) == ProposalState.Active,
        "Governor::castVoteInternal: voting is closed"
    );
    uint16 votes = token.getPriorVotes(voter, uint40(proposal.startTime));

    if (support == VoteType.Against) {
        proposalVote.againstVotes += votes;
    } else if (support == VoteType.For) {
        proposalVote.forVotes += votes;
    } else {
        // must be abstain
        proposalVote.abstainVotes += votes;
    }

    return votes;
}

Note: The above alternative does add some gas overhead due to additional jump ops, but ends up being cheaper in the happy case of the communityId being < 209 because it doesn't declare unnecessary variables such as wordIndex.

A similar approach might be used for hasVoted function which has similar logic.