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 Address.functionCall removing redundant isContract check #3469

Merged
merged 6 commits into from
Jun 27, 2022
Merged

Optimize Address.functionCall removing redundant isContract check #3469

merged 6 commits into from
Jun 27, 2022

Conversation

ZumZoom
Copy link
Contributor

@ZumZoom ZumZoom commented Jun 13, 2022

In Address.functionCall, Address.functionDelegateCall and Address.functionStaticCall it is possible to skip target's isContract check in cases where call fails or returns non-empty data. This can only happen when target has some code so there is no point in redundant isContract check.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx
Copy link
Collaborator

Amxx commented Jun 14, 2022

Thank you @ZumZoom for this interesting proposal.

Do you have any metrics on the gas savings this can produce?

@Amxx Amxx added this to the 4.8 milestone Jun 14, 2022
@ZumZoom
Copy link
Contributor Author

ZumZoom commented Jun 15, 2022

Here are gas reports from relevant tests.

TLDR: saves 108 gas when checks are not triggered (which is probably the most used case due to SafeERC20 library) and loses 26 gas when checks are triggered.

Master

·-----------------------------------------------------|---------------------------|-------------|-----------------------------·
|                Solc version: 0.8.13                 ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 10000000 gas  │
······················································|···························|·············|······························
|  Methods                                                                                                                    │
····························|·························|·············|·············|·············|···············|··············
|  Contract                 ·  Method                 ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
····························|·························|·············|·············|·············|···············|··············
|  AddressImpl              ·  functionCall           ·          -  ·          -  ·      29880  ·            2  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  AddressImpl              ·  functionCallWithValue  ·      30017  ·      36789  ·      35096  ·            8  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  AddressImpl              ·  functionDelegateCall   ·          -  ·          -  ·      51597  ·            1  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  AddressImpl              ·  functionStaticCall     ·          -  ·          -  ·      28993  ·            1  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  approve                ·      29996  ·      33388  ·      31172  ·            6  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  decreaseAllowance      ·      33075  ·      33370  ·      33223  ·            2  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  increaseAllowance      ·      33144  ·      33439  ·      33292  ·            4  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  transfer               ·      29796  ·      30079  ·      29938  ·            2  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  transferFrom           ·      30021  ·      30307  ·      30164  ·            2  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  Deployments                                        ·                                         ·  % of limit   ·             │
······················································|·············|·············|·············|···············|··············
|  AddressImpl                                        ·          -  ·          -  ·     725017  ·        7.3 %  ·          -  │
······················································|·············|·············|·············|···············|··············
|  SafeERC20Wrapper                                   ·     741588  ·     741600  ·     741600  ·        7.4 %  ·          -  │
·-----------------------------------------------------|-------------|-------------|-------------|---------------|-------------·

PR

·-----------------------------------------------------|---------------------------|-------------|-----------------------------·
|                Solc version: 0.8.13                 ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 10000000 gas  │
······················································|···························|·············|······························
|  Methods                                                                                                                    │
····························|·························|·············|·············|·············|···············|··············
|  Contract                 ·  Method                 ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
····························|·························|·············|·············|·············|···············|··············
|  AddressImpl              ·  functionCall           ·          -  ·          -  ·      29772  ·            2  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  AddressImpl              ·  functionCallWithValue  ·      29909  ·      36681  ·      34988  ·            8  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  AddressImpl              ·  functionDelegateCall   ·          -  ·          -  ·      51493  ·            1  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  AddressImpl              ·  functionStaticCall     ·          -  ·          -  ·      28889  ·            1  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  approve                ·      30022  ·      33280  ·      31131  ·            6  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  decreaseAllowance      ·      33101  ·      33262  ·      33182  ·            2  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  increaseAllowance      ·      33170  ·      33331  ·      33251  ·            4  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  transfer               ·      29822  ·      29971  ·      29897  ·            2  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  SafeERC20Wrapper         ·  transferFrom           ·      30047  ·      30199  ·      30123  ·            2  ·          -  │
····························|·························|·············|·············|·············|···············|··············
|  Deployments                                        ·                                         ·  % of limit   ·             │
······················································|·············|·············|·············|···············|··············
|  AddressImpl                                        ·          -  ·          -  ·     686087  ·        6.9 %  ·          -  │
······················································|·············|·············|·············|···············|··············
|  SafeERC20Wrapper                                   ·     745896  ·     745908  ·     745908  ·        7.5 %  ·          -  │
·-----------------------------------------------------|-------------|-------------|-------------|---------------|-------------·

Amxx
Amxx previously requested changes Jun 20, 2022
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

With this PR, there are two version of verifyCallResult with different arguments.

I'm worried that the two do not have the same behavior, which is not clear from naming alone:

  • verifyCallResult(bool,bytes,string) is not modified. It will return the data if success. Otherwise it will revert with data (if not empty) or with the fallback string
  • verifyCallResult(address,bool,bytes,string) is a new version. It will return the data is success and if either data is not empty or target is a contract. Otherwise it will revert with data (if not empty) or with the fallback string.

The second one forces the returns data to either be not empty, of target to be a contract, but doesn't enforce that target is actually the address that was called.

I think there is a clear possibility of user error or confusion, that we want to avoid.

@frangio
Copy link
Contributor

frangio commented Jun 27, 2022

@ZumZoom Please rename this to verifyCallResultFromTarget, and update the changelog to mention it.

@frangio frangio changed the title check isContract only on success and no returndata Optimize Address.functionCall removing redundant isContract check Jun 27, 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.

Thanks!

@frangio frangio merged commit 6f88199 into OpenZeppelin:master Jun 27, 2022
@ZumZoom ZumZoom deleted the feauture/optimize-address-calls branch June 28, 2022 02:25
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
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.

3 participants