Some modifiers could use internal functions to reduce code size impact

[nicolasgarcia214]

The following modifier could be added to many methods and its implementation is large enough to consider using internal functions:

• onlyGovernance in Governor.sol

Furthermore, other modifiers with fewer lines within each code block may benefit from also using internal functions:

• onlyProxy and notDelegated in UUPSUpgradeable.sol.
• onlyInitializing in Initializable.sol.

[ernestognw]

I didn't know internal functions improve code size. So, is the proposal to replace these modifiers with internal functions or use the internal functions inside the modifiers?

We can implement such changes without issues if the proposal is to keep the modifiers, however, replacing them with internal functions would require more discussion as it arguably undermines DX.

I think it'd be helpful to have a benchmark with a rough estimate of how much it would save for both creation and runtime. A foundry test may do the job for this.

[nicolasgarcia214]

Use the internal function inside the modifiers. This is something we commonly use in most of our modifiers but for some reason not on the ones I mentioned.

[ernestognw]

Ok, that makes sense as it doesn't change the interface. I'm still not sure about the gas savings.

My conclusion is that the code size reduction is significant but the runtime costs added could be even worse if used multiple times. Here you have the Foundry tests I made:

PoC Code

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test} from "forge-std/Test.sol";

/// === Base ===

contract AccessControlStorage {
    bytes32 public constant NOT_HELD_ROLE = 0x00;
    bytes32 public constant HELD_ROLE = bytes32(uint256(0x01));

    struct RoleData {
        mapping(address => bool) members;
        bytes32 adminRole;
    }

    mapping(bytes32 => RoleData) internal _roles;

    constructor() {
        _roles[HELD_ROLE].members[msg.sender] = true;
    }
}

/// === Inlined Setup ===

contract Inlined is AccessControlStorage {
    modifier onlyRole(bytes32 role) {
        require(_roles[role].members[msg.sender]);
        _;
    }
}

contract ModifiedInlined is Inlined {
    function fail() public onlyRole(NOT_HELD_ROLE) {}

    function succeed() public onlyRole(HELD_ROLE) {}
}

/// === Internal Setup ===

contract Internal is AccessControlStorage {
    modifier onlyRole(bytes32 role) {
        _checkRole(role, msg.sender);
        _;
    }

    function _checkRole(bytes32 role, address account) internal view virtual {
        require(_roles[role].members[account]);
    }
}

contract ModifiedInternal is Internal {
    function fail() public onlyRole(NOT_HELD_ROLE) {}

    function succeed() public onlyRole(HELD_ROLE) {}
}

/// === Tests ===

contract ModifiedInlinedTest is Test {
    ModifiedInlined private _modified;

    function setUp() public {
        _modified = new ModifiedInlined();
    }

    function testReverts() public {
        vm.expectRevert();
        _modified.fail();
    }

    function testPasses() public {
        _modified.succeed();
    }
}

contract ModifiedInternalTest is Test {
    ModifiedInternal private _modified;

    function setUp() public {
        _modified = new ModifiedInternal();
    }

    function testReverts() public {
        vm.expectRevert();
        _modified.fail();
    }

    function testPasses() public {
        _modified.succeed();
    }
}

When running forge test --match-contract Modified --gas-report, it gives the following output:

test/Modifiers.t.sol:ModifiedInlined contract
Deployment Cost	Deployment Size
81703	386
Function Name	min	avg	median	max	# calls
fail	2309	2309	2309	2309	1
succeed	2343	2343	2343	2343	1

test/Modifiers.t.sol:ModifiedInternal contract
Deployment Cost	Deployment Size
70891	331
Function Name	min	avg	median	max	# calls
fail	2417	2417	2417	2417	1
succeed	2447	2447	2447	2447	1

So far, we're saving 10,812 units of gas by using internal functions and we're saving 104 and 108 units of gas per function call. That means we'll need around 100 calls to waste more gas than saved.

Interestingly, the functions you shared are not expected to be used frequently imo so it should be fine.

Also note Governor is already a big contract so we'll be interested in a code size reduction if it doesn't affect runtime costs.

[ernestognw]

For anyone wanting to take the issue, the implementation should be as follows:

• Replace the onlyGovernance code with an internal function called _checkGovernance (alternatively _checkExecutor) doing the same check.
• Replace the onlyProxy code with an internal function called _checkProxy with the same check and the notDelegated code with a _checkNotDelegated internal function
• Replace the onlyInitializing code with an internal function called _checkInitializing

Labeling as a good first issue but bear in mind a PR might be delayed since this is not a priority at the moment and other members in the team might want to take a look.

@frangio @Amxx

[allwin199]

I would like to work on this issue. Meanwhile, If there is any update let me know.

[asujan205]

I would Like to take this issue

[pratik-31]

Can I take on this issue?

[Amxx]

There is already a PR for that issue. Please don't create multiple ones.