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

Reduce contract size and deployment gas for onlyOwner modifier #3223

Closed
wants to merge 3 commits into from

Conversation

uglyrobot
Copy link

Fixes #3222

For inheriting contracts that use the onlyOwner modifier significant gas savings for deployment can be made by not inlining the require statement in every modified function.

When you add a function modifier, the code of that function is picked up and put in the function modifier in place of the _ symbol. If the same code is inlined multiple times, it adds up in size. Internal functions, on the other hand, are not inlined but called as separate functions. This means they are very slightly more expensive in run time but save a lot of redundant bytecode in deployment.

From my testing with a sample contract this simple PR saves substantial gas per modifier call in contract size. This really adds up when for example in my contract I have 18 calls to onlyOwner!:

Savings per call at 10 Runs Optimization:
1 modifier call: 3883700-3857256 = 26,444 Gwei
2 modifier calls: 3892096-3857268 = 17,414 Gwei
5 modifier calls: 3917411-3857268 = 12,028 Gwei

Savings per call at 1000 Runs Optimization:
1 modifier call: 4311775-4258622 = 53,153 Gwei
2 modifier calls: 4329572-4258622 = 35,475 Gwei
5 modifier calls: 4382740-4258622 = 24,823 Gwei

PR Checklist

  • Tests
  • [ N/A] Documentation
  • Changelog entry

For inheriting contracts that use the onlyOwner modifier significant gas savings for deployment can be made by not inlining the require statement in every modified function.

When you add a function modifier, the code of that function is picked up and put in the function modifier in place of the _ symbol. If the same code is inlined multiple times, it adds up in size. Internal functions, on the other hand, are not inlined but called as separate functions. This means they are very slightly more expensive in run time but save a lot of redundant bytecode in deployment.

From my testing with a sample contract this simple PR saves substantial gas per modifier call in contract size. This really adds up when for example in my contract I have 18 calls to onlyOwner!:

Savings per call at 10 Runs Optimization:
1 modifier call: 3883700-3857256 = 26,444 Gwei
2 modifier calls: 3892096-3857268 = 17,414 Gwei
5 modifier calls: 3917411-3857268 = 12,028 Gwei

Savings per call at 1000 Runs Optimization:
1 modifier call: 4311775-4258622 = 53,153 Gwei
2 modifier calls: 4329572-4258622 = 35,475 Gwei
5 modifier calls: 4382740-4258622 = 24,823 Gwei
@Amxx
Copy link
Collaborator

Amxx commented Feb 26, 2022

Thank you @uglyrobot for this PR.

Could you please fix the lintting? Running npm run lint:fix should do the trick.

@uglyrobot
Copy link
Author

Fixed

@Amxx
Copy link
Collaborator

Amxx commented Feb 28, 2022

@frangio do we need a changelog entry for that ?

Comment on lines +48 to +49
* @dev Putting the require in an internal function decreases contract size
* when onlyOwner modifier is used multiple times.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @dev Putting the require in an internal function decreases contract size
* when onlyOwner modifier is used multiple times.
* @dev Throws if the sender is not the owner.

* @dev Putting the require in an internal function decreases contract size
* when onlyOwner modifier is used multiple times.
*/
function _onlyOwner() internal view {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function should be virtual (we make all our functions virtual).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could make it internal virtual, but we could also make it private

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal virtual IMO.

@frangio
Copy link
Contributor

frangio commented Mar 1, 2022

Yes I'd say we should add a changelog entry.

@Amxx
Copy link
Collaborator

Amxx commented Jun 7, 2022

Closed by #3347

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce contract size and deployment gas for onlyOwner modifier
3 participants