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

Optimize ERC1167 proxy creation code by 1 opcode #3329

Merged
merged 5 commits into from
Apr 8, 2022

Conversation

Philogy
Copy link
Contributor

@Philogy Philogy commented Apr 8, 2022

Fixes: None (No issue created beforehand)

Description

The existing proxy creation bytecode was a total of 55 bytes:

  • 3d602d80600a3d3981f3 10 bytes, responsible for returning the remaining 45 byte ERC1167 proxy code, equivalent to the constructor in solidity
  • 363d3d373d3d3d363d73????????????????????????????????????????5af43d82803e903pd91602b57fd5bf3 45 bytes, standard ERC1167 proxy bytecode

Looking at the execution of the constructor code one notices that one value always remains, unused on the stack at the end of execution (stack after op depicted in the square brackets; left -> right, top -> bottom):

  1. (0) RETURNDATASIZE [ 0, ]
  2. (2) PUSH1 0x2d [ 45, 0, ]
  3. (3) DUP1 [ 45, 45, 0, ]
  4. (5) PUSH1 0x0a [ 10, 45, 45, 0, ]
  5. (6) RETURNDATASIZE [ 0, 10, 45, 45, 0, ]
  6. (7) CODECOPY [ 45, 0, ]
  7. (8) DUP2 [ 0, 45, 0, ]
  8. (9) RETURN [ 0, ]

By rearranging the opcodes slightly, one opcode can be removed, resulting in a smaller, 9-byte constructor 602d8060093d393df3:

  1. (0) PUSH1 0x2d [ 45, ]
  2. (2) DUP1 [ 45, 45, ]
  3. (4) PUSH1 0x09 [ 9, 45, 45, ]
  4. (5) RETURNDATASIZE [0, 9, 45, 45, ]
  5. (6) CODECOPY [ 45, ]
  6. (7) RETURNDATASIZE [ 0, 45, ]
  7. (8) RETURN [ ]

The proxy/Clones.sol library's clone, cloneDeterministic and predictDeterministicAddress methods are then adjusted for the smaller code. The total creation bytecode is then 54 bytes instead of the current 55 bytes.

PR Checklist

  • Tests (functionality should remain unchanged, therefore tests should remain unchanged)
  • Documentation (functionality should remain unchanged, therefore docs should remain unchanged)
  • Changelog entry (unsure how to change)

@frangio
Copy link
Contributor

frangio commented Apr 8, 2022

This is a tiny optimization so it has low priority for us, but I don't see a reason not to merge it.

Can you confirm if minimal proxies deployed with this new code are still detected as minimal proxies by Etherscan?

@Philogy
Copy link
Contributor Author

Philogy commented Apr 8, 2022

It's completely understandable that a 3 gas optimization is not your priority.

I've verified that etherscan recognizes proxies deployed with the adjusted bytecode as minimal proxies: https://ropsten.etherscan.io/address/0x6d91057e495b2fe6ba421ed7117bde6453c61516#code (creator address is a contract which has the adjusted internal clone method)

@frangio
Copy link
Contributor

frangio commented Apr 8, 2022

Nice. Thanks.

  • Changelog entry (unsure how to change)

You can add an entry in the topmost "Unreleased" section in CHANGELOG.md. Just a short description, follow a similar format than the rest.

@Philogy
Copy link
Contributor Author

Philogy commented Apr 8, 2022

Changelog entry added and merged

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

@frangio frangio merged commit 28dd490 into OpenZeppelin:master Apr 8, 2022
Amxx added a commit to Amxx/openzeppelin-contracts that referenced this pull request Jun 6, 2022
@frangio
Copy link
Contributor

frangio commented Jun 6, 2022

@Philogy We ended up reverting this change, we realized it was a breaking change of the way deterministic addresses are computed and this has the risk of breaking upgradeable contracts. The risk is not worth taking for such a small improvement.

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.

2 participants