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 Clones assembly #3640

Merged
merged 17 commits into from
Aug 24, 2022
Merged

Optimize Clones assembly #3640

merged 17 commits into from
Aug 24, 2022

Conversation

Vectorized
Copy link
Contributor

@Vectorized Vectorized commented Aug 19, 2022

Changes:

  • Deployments use the scratch space.
  • Optimize predictDeterministicAddress to use lesser operations, while maintaining the same memory expansion costs.

Gas Before:

·------------------------------------------------------------|---------------------------|-------------|-----------------------------·
|                    Solc version: 0.8.13                    ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 10000000 gas  │
·····························································|···························|·············|······························
|  Methods                                                                                                                           │
···············|·············································|·············|·············|·············|···············|··············
|  Contract    ·  Method                                     ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
···············|·············································|·············|·············|·············|···············|··············
|  ClonesMock  ·  clone(address,bytes)                       ·      90724  ·      97783  ·      93136  ·           12  ·          -  │
···············|·············································|·············|·············|·············|···············|··············
|  ClonesMock  ·  cloneDeterministic(address,bytes32,bytes)  ·      65269  ·      98378  ·      89662  ·           14  ·          -  │
···············|·············································|·············|·············|·············|···············|··············
|  Deployments                                               ·                                         ·  % of limit   ·             │
·····························································|·············|·············|·············|···············|··············
|  ClonesMock                                                ·          -  ·          -  ·     435385  ·        4.4 %  ·          -  │
·····························································|·············|·············|·············|···············|··············
|  DummyImplementation                                       ·          -  ·          -  ·     398946  ·          4 %  ·          -  │
·------------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·

Gas After:

·------------------------------------------------------------|---------------------------|-------------|-----------------------------·
|                    Solc version: 0.8.13                    ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 10000000 gas  │
·····························································|···························|·············|······························
|  Methods                                                                                                                           │
···············|·············································|·············|·············|·············|···············|··············
|  Contract    ·  Method                                     ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
···············|·············································|·············|·············|·············|···············|··············
|  ClonesMock  ·  clone(address,bytes)                       ·      90707  ·      97766  ·      93119  ·           12  ·          -  │
···············|·············································|·············|·············|·············|···············|··············
|  ClonesMock  ·  cloneDeterministic(address,bytes32,bytes)  ·      65237  ·      98361  ·      89645  ·           14  ·          -  │
···············|·············································|·············|·············|·············|···············|··············
|  Deployments                                               ·                                         ·  % of limit   ·             │
·····························································|·············|·············|·············|···············|··············
|  ClonesMock                                                ·          -  ·          -  ·     432690  ·        4.3 %  ·          -  │
·····························································|·············|·············|·············|···············|··············
|  DummyImplementation                                       ·          -  ·          -  ·     398946  ·          4 %  ·          -  │
·------------------------------------------------------------|-------------|-------------|-------------|---------------|-------------·

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Aug 19, 2022

Can you quantify the improvement to the clone functions from using the scratch space?

The suggested method for predictDeterministicAddress is being discussed in #3600 (comment). I don't think this is something we want to merge. It's a use of memory outside the rules for memory safety as described in the Solidity documentation.

@Vectorized Vectorized changed the title Optimize clones to avoid using free memory Optimize clones to avoid using free memory during deployment Aug 19, 2022
@Vectorized
Copy link
Contributor Author

Vectorized commented Aug 19, 2022

@frangio Ok, understand.

Changed the implementation of predictDeterministicAddress to use free memory instead.

Saves a tiny bit of gas during deployment, and makes the deployer bytecode a little smaller.

@Vectorized Vectorized changed the title Optimize clones to avoid using free memory during deployment Optimize clones to use only the scratch space during deployment Aug 19, 2022
@Vectorized Vectorized changed the title Optimize clones to use only the scratch space during deployment Optimize clones assembly Aug 19, 2022
@frangio
Copy link
Contributor

frangio commented Aug 20, 2022

For the record, I looked at defining a helper like below that we could reuse in all places. Unfortunately Solidity doesn't inline the call so I don't think it's worth it.

    function _getInitCode(address implementation) internal pure returns (bytes32 w0, bytes32 w1) {
        /// @solidity memory-safe-assembly
        assembly {
            w0 := or(0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000, shr(0xe8, shl(0x60, implementation)))
            // Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
            w1 := or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3)
        }
    }

@frangio frangio changed the title Optimize clones assembly Optimize Clones assembly Aug 23, 2022
frangio
frangio previously approved these changes Aug 23, 2022
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.

LGTM

@Vectorized
Copy link
Contributor Author

Resolved the CHANGELOG.md conflict.

@frangio
Copy link
Contributor

frangio commented Aug 24, 2022

For reference here's the memory layout in predictDeterministicAddress. Rows are written onto memory from top to bottom.

0                               1                               2                               3                               4                               5                               6                               7                               8                               9
0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 8 9 a b c d e f 0 1 2 3 4 5 6 7 
                                                                                                                ~~~~~~~~~~~~~~~~~~~~~~~~dddddddddddddddddddddddddddddddddddddddd
                                                                        000000000000000000000000000000005af43d82803e903d91602b57fd5bf3ff
                                        ~~~~~~~~~~~~~~~~~~~~~~~~iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii
0000000000000000000000003d602d80600a3d3981f3363d3d373d3d3d363d73
                                                                                                                                                                                ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss
                                                                                                                                                                                                                                                hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh

                        [                                                                                                            ][                                                                                                                                                                        ]

frangio
frangio previously approved these changes Aug 24, 2022
@frangio frangio requested a review from Amxx August 24, 2022 00:29
contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
contracts/proxy/Clones.sol Outdated Show resolved Hide resolved
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
@Vectorized
Copy link
Contributor Author

Brb, i’m outside right now.

@Amxx Amxx enabled auto-merge (squash) August 24, 2022 09:16
@Amxx Amxx merged commit 6b9bda8 into OpenZeppelin:master Aug 24, 2022
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 3, 2022
@LuKks
Copy link

LuKks commented Feb 23, 2023

@Vectorized Any hints on how to create the equivalent of predictDeterministicAddress but on JavaScript? For example, having all the data needed pre-loaded locally (deployer addr, implementation addr, contract bytecode, etc) to later being able to calculate addresses on pure JS?

I tried a lot but nothing seems to result in the correct address, maybe the optimizations and using the proxy prefix + suffix bytes makes it not straightforward to replicate it on JavaScript?

References:
https://stackoverflow.com/questions/71573083/im-trying-to-implement-openzeppelins-minimal-proxy-clone-contract-on-tron-bloc/71581522#71581522
https://github.com/thegostep/solidity-create2-deployer

Thank you in advance! :)

@Amxx
Copy link
Collaborator

Amxx commented Feb 23, 2023

@Vectorized Any hints on how to create the equivalent of predictDeterministicAddress but on JavaScript? For example, having all the data needed pre-loaded locally (deployer addr, implementation addr, contract bytecode, etc) to later being able to calculate addresses on pure JS?

I tried a lot but nothing seems to result in the correct address, maybe the optimizations and using the proxy prefix + suffix bytes makes it not straightforward to replicate it on JavaScript?

References: https://stackoverflow.com/questions/71573083/im-trying-to-implement-openzeppelins-minimal-proxy-clone-contract-on-tron-bloc/71581522#71581522 https://github.com/thegostep/solidity-create2-deployer

Thank you in advance! :)

Here

@LuKks
Copy link

LuKks commented Feb 25, 2023

@Amxx Thanks, I ended up doing a tiny JS library to solve the issue so I don't have to deal with it again! Haha
https://github.com/LuKks/predict-deterministic-address

So that allowed me to finish this other one:
https://github.com/LuKks/wallet-accounts (wallet with sub-accounts as a smart contract)

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.

4 participants