Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #236

Open
code423n4 opened this issue Sep 12, 2022 · 1 comment
Open

Gas Optimizations #236

code423n4 opened this issue Sep 12, 2022 · 1 comment
Labels
bug Something isn't working G (Gas Optimization)

Comments

@code423n4
Copy link
Contributor

code423n4 commented Sep 12, 2022

Governor

Governor.propose

This method calls twice the method proposalThreshold(). It is not necessary because the its value was stored in a variable at the beggining. So you have to replace the second call in line 128 for the variable set before:
https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L128

Doing that you save ~1772 gas.

├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ proposeOld ┆ 9272 ┆ 40259 ┆ 40259 ┆ 71246 ┆ 2 │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ propose ┆ 7492 ┆ 38487 ┆ 38487 ┆ 69482 ┆ 2 │

On the other hand, all the following validation must be placed before proposalThreshold() method is called. Doing this, you could save gas when provided data is wrong:

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L132-L139

There you are the final optimized method is case you are agree to apply those changes:

/// @notice Creates a proposal
/// @param _targets The target addresses to call
/// @param _values The ETH values of each call
/// @param _calldatas The calldata of each call
/// @param _description The proposal description
function propose(
    address[] memory _targets,
    uint256[] memory _values,
    bytes[] memory _calldatas,
    string memory _description
) external returns (bytes32) {
    // Cache the number of targets
    uint256 numTargets = _targets.length;

    // Ensure at least one target exists
    if (numTargets == 0) revert PROPOSAL_TARGET_MISSING();

    // Ensure the number of targets matches the number of values and calldata
    if (numTargets != _values.length) revert PROPOSAL_LENGTH_MISMATCH();
    if (numTargets != _calldatas.length) revert PROPOSAL_LENGTH_MISMATCH();


    // Get the current proposal threshold
    uint256 currentProposalThreshold = proposalThreshold();

    // Cannot realistically underflow and `getVotes` would revert
    unchecked {
        // Ensure the caller's voting weight is greater than or equal to the threshold
        if (getVotes(msg.sender, block.timestamp - 1) < currentProposalThreshold) revert BELOW_PROPOSAL_THRESHOLD();
    }

    // Compute the description hash
    bytes32 descriptionHash = keccak256(bytes(_description));

    // Compute the proposal id
    bytes32 proposalId = hashProposal(_targets, _values, _calldatas, descriptionHash);

    // Get the pointer to store the proposal
    Proposal storage proposal = proposals[proposalId];

    // Ensure the proposal doesn't already exist
    if (proposal.voteStart != 0) revert PROPOSAL_EXISTS(proposalId);

    // Used to store the snapshot and deadline
    uint256 snapshot;
    uint256 deadline;

    // Cannot realistically overflow
    unchecked {
        // Compute the snapshot and deadline
        snapshot = block.timestamp + settings.votingDelay;
        deadline = snapshot + settings.votingPeriod;
    }

    // Store the proposal data
    proposal.voteStart = uint32(snapshot);
    proposal.voteEnd = uint32(deadline);
    proposal.proposalThreshold = uint32(currentProposalThreshold);
    proposal.quorumVotes = uint32(quorum());
    proposal.proposer = msg.sender;
    proposal.timeCreated = uint32(block.timestamp);

    emit ProposalCreated(proposalId, _targets, _values, _calldatas, _description, descriptionHash, proposal);

    return proposalId;
}



Governor

Governor.proposalVotes

I have added a POC for this method and realized that if you avoid to declare the memory variable and get the values from storage is cheaper (~412 gas) than the already coded approach. I assume that it is a sort of compiler's optimization.

├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name ┆ min ┆ avg ┆ median ┆ max ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ proposalVotes ┆ 759 ┆ 759 ┆ 759 ┆ 759 ┆ 3 │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ proposalVotesOld ┆ 1171 ┆ 1171 ┆ 1171 ┆ 1171 ┆ 1 │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

There you are the optimized method to use in case you consider properly:

/// @notice The vote counts for a proposal
/// @param _proposalId The proposal id
function proposalVotes(bytes32 _proposalId)
    external
    view
    returns (
        uint256,
        uint256,
        uint256
    )
{
    return (proposals[_proposalId].againstVotes, proposals[_proposalId].forVotes, proposals[_proposalId].abstainVotes);
}



Token.mint()

Token._isForFounder(uint256)

_isForFounder(uint256) method could be optimized storing in memory the value of tokenRecipient[baseTokenId]. It is read from storage three times.
Doing this optimization you will save ~260 gas during mint process:

├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ mintOld ┆ 298756
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ mint ┆ 298496
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤

You could use the following code in case think the optimization is valid:

/// @dev Checks if a given token is for a founder and mints accordingly
/// @param _tokenId The ERC-721 token id
function _isForFounder(uint256 _tokenId) private returns (bool) {
    // Get the base token id
    uint256 baseTokenId = _tokenId % 100;

    // If there is no scheduled recipient:
    Founder memory _founder = tokenRecipient[baseTokenId];
    if (_founder.wallet == address(0)) {
        return false;

        // Else if the founder is still vesting:
    } else if (block.timestamp < _founder.vestExpiry) {
        // Mint the token to the founder
        _mint(_founder.wallet, _tokenId);

        return true;

        // Else the founder has finished vesting:
    } else {
        // Remove them from future lookups
        delete tokenRecipient[baseTokenId];

        return false;
    }
}
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Sep 12, 2022
code423n4 added a commit that referenced this issue Sep 12, 2022
code423n4 added a commit that referenced this issue Sep 12, 2022
@GalloDaSballo
Copy link
Collaborator

2.5k (bonus)

Formatting can be improved, but benchmarks are always a better submission than most, good start!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization)
Projects
None yet
Development

No branches or pull requests

2 participants