Why does OpenZeppelin's governor not utilize calldata for castVoteWithReason?

Support Contracts
1

Compound's original Governor Bravo module used the following function for casting votes with a reason:

 function castVoteWithReason(uint proposalId, uint8 support, string calldata reason) external {
        emit VoteCast(msg.sender, proposalId, support, castVoteInternal(msg.sender, proposalId, support), reason);
    }

My assumption is that the VoteCast event was chosen not to be emitted inside castVoteInternal to save on gas, as that would require an extra memory copy (please correct me if I'm wrong). The down-side of this approach is that code is less modularized, as the event has to be explicitly emitted across all vote casting method variants.

OpenZeppelin, however, chose to go with the modular approach, embedding the reason string into the internal function:

function _castVote(
        uint256 proposalId,
        address account,
        uint8 support,
        string memory reason
    ) internal virtual returns (uint256) {
         ...
         emit VoteCast(account, proposalId, support, weight, reason);
}

My question, then, is given that the calldata type is now allowed inside internal functions, why not use string calldata reason instead of string memory reason? Or would an extra memory copy be required in either case?

1 Like
2

My guess is that as of at least Solidity version 0.8.3, calldata was still limited to external function calls. So in order to maintain compatibility for ^0.8.0 they stuck with memory for the internal function implementation. Maybe one of the OZ devs can confirm.

3

calldata is allowed in internal functions but it limits the use that one can give to that internal function. Namely, it would only be possibly to use it with a reason that was originally passed in calldata, and not with a reason that was constructed in memory.

We don't follow this convention to the letter, but this is something we've run into before and a good reason to keep internal function arguments as memory-located.

Note: it's possible to convert calldata to memory, but not the other way around.

1 Like