Optimize Ownable and Pausable modifiers' size impact

[jnishiyama]

Fixes #3222

Affects:

• contracts/access/Ownable.sol
• contracts/security/Pausable.sol

Changes:

• In both contracts, the modifiers require statements are moved into an internal virtual function. This reduces the size of compiled contracts that use the modifiers.
• In both contracts, the modifier definition is moved above the definitions of functions to better adhere to the solidity style guide.

PR Checklist

• Tests
• Documentation
• Changelog entry

[Amxx]

EDIT: solidity version updated to 0.8.14

For the Ownable, I created two contracts that uses the old and the new versions. Both compiled with 0.8.14 and optimizations on (200)

contract Test1 is Ownable {
    function f1() public onlyOwner() {}
    function f2() public onlyOwner() {}
    function f3() public onlyOwner() {}
    function f4() public onlyOwner() {}
    function f5() public onlyOwner() {}
    function f6() public onlyOwner() {}
}

contract Test2 is OwnableV2 {
    function f1() public onlyOwner() {}
    function f2() public onlyOwner() {}
    function f3() public onlyOwner() {}
    function f4() public onlyOwner() {}
    function f5() public onlyOwner() {}
    function f6() public onlyOwner() {}
}

Here are the deployment gas cost:

Number of functions	0	1	2	3	4	5	6
Test1	212836	217510 (+4674)	219886 (+2376)	222238 (+2352)	228088 (+5850)	230452 (+2364)	232871 (+2419)
Test2	196840	202636 (+5796)	205006 (+2370)	207370 (+2364)	213172 (+5802)	215536 (+2364)	217894 (+2358)

Oddly enough, each additional function is not really cheaper with the new version, but the base cost is cheaper

[Amxx]

With solidity 0.8.14 and optimisations on (200)

contract Ownable is Context {
    address public owner = _msgSender();

    modifier onlyOwner() {
        require(owner == _msgSender(), "Ownable: caller is not the owner");
        _;
    }
}

contract Ownable2 is Context {
    address public owner = _msgSender();

    modifier onlyOwner() {
        _checkOwner();
        _;
    }

    function _checkOwner() internal view virtual {
        require(owner == _msgSender(), "Ownable: caller is not the owner");
    }
}

// 0: 107172
// 1: 131600
// 2: 133760
// 3: 135920
// 4: 138080
// 5: 140246
contract T1 is Ownable {
    function f1() external onlyOwner() {}
    function f2() external onlyOwner() {}
    function f3() external onlyOwner() {}
    function f4() external onlyOwner() {}
    function f5() external onlyOwner() {}
}

// 0: 107172
// 1: 132896
// 2: 135056
// 3: 137216
// 4: 139382
// 5: 141542
contract T2 is Ownable2 {
    function f1() external onlyOwner() {}
    function f2() external onlyOwner() {}
    function f3() external onlyOwner() {}
    function f4() external onlyOwner() {}
    function f5() external onlyOwner() {}
}

Its actually costing more gas to have the internal function!

[Amxx]

Note that changing visibility between internal and private does NOT change the deployment cost.

[jnishiyama]

I'm afk all day, so I'll take another look when I am at a computer. That being said, regarding the first set of tests, objectively, Ownable has less code than OwnableV2, so it doesn't make much sense to me that the base contract would be larger for Ownable than it is for OwnableV2. Definitely surprising to me that the behavior we are seeing in those tests is literally the exact opposite of what we should expect from the code. Is it possible the tests are not representing the correct contracts?

If not, maybe there is a nuance between gas cost and contract size I'm not understanding.

[Amxx]

There is a nuance between gas cost and contract size. IMO what the devs really care about is deployment gas costs.

objectively, Ownable has less code than OwnableV2

In terms of number of lines yes, but not in terms of functionality. If functionalities are equal, and if the gas cost is the same (or very close) I would go for the more readable version, which is the current one.

Adding code, that reduces readability, hoping to save gas, not saving gas, and arguing it's normal because "there is more code" is not the way we want to go.

[jnishiyama]

To be clear, the argument is not "there is more code" the argument is that the new version can reduce contract size significantly (depending on usage of modifier). If deployment gas cost is what we are optimizing for then it would seem from your tests that it would make sense to not introduce the change.

[Amxx]

the new version can reduce contract size significantly (depending on usage of modifier)

That is what I'm trying to show ... and so far I'm failing. What test would you run to verify that claim?

[Amxx]

When calling f5 on both T1 and T2 I get the following costs:

• T1: 23326
• T2: 23350

[jnishiyama]

To verify the claim I would just look at the size of a compiled contract that uses the modifier, we should expect the compiled size to increase by more for each function in a contract that uses the original modifier and the increase should be less for a contract that uses the new modifier.

The idea here being that because the modifier literally has less code, when it is inlined in compilation, it will increase the contract size by less than it did originally.

Regarding the gas cost at run time, we should expect the new modifier to be slightly more expensive.

I'm sorry I'm afk, but will be happy to help when I'm at a computer.

[Amxx]

To verify the claim I would just look at the size of a compiled contract that uses the modifier, we should expect the compiled size to increase by more for each function in a contract that uses the original modifier and the increase should be less for a contract that uses the new modifier.

That is also what I would expect ... but if you take my T1 and T2 example from above (here), T1 object is 348 bytes, and T2 is 354 bytes.

[Amxx]

My results have been biased by the function being empty, and the compiler possibly merging them. If I put some code in the function's body, then I observe the expected behavior.

// 0: 107172
// 1: 145772
// 2: 181610
// 3: 198170
// 4: 214532
// 5: 241059
contract T1 is Ownable {
    event Call(bytes4 selector);
    function f0() external onlyOwner() { emit Call(this.f0.selector); }
    function f1() external onlyOwner() { emit Call(this.f1.selector); }
    function f2() external onlyOwner() { emit Call(this.f2.selector); }
    function f3() external onlyOwner() { emit Call(this.f3.selector); }
    function f4() external onlyOwner() { emit Call(this.f4.selector); }
}

// 0: 107172
// 1: 147908
// 2: 165818
// 3: 183506
// 4: 192500
// 5: 211682
contract T2 is Ownable2 {
    event Call(bytes4 selector);
    function f0() external onlyOwner() { emit Call(this.f0.selector); }
    function f1() external onlyOwner() { emit Call(this.f1.selector); }
    function f2() external onlyOwner() { emit Call(this.f2.selector); }
    function f3() external onlyOwner() { emit Call(this.f3.selector); }
    function f4() external onlyOwner() { emit Call(this.f4.selector); }
}

[Amxx]

[Amxx]

LGTM,

@frangio?

[frangio]

Let's go!