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

perf: use directly IERC165 in inheritance instead of ERC165 #238

Closed
wants to merge 2 commits into from

Conversation

CJ42
Copy link
Collaborator

@CJ42 CJ42 commented Sep 19, 2023

What does this PR introduce?

Note: awaiting for #236 to be merged first.

♻️ Refactor

Remove IERC165 from the inheritance list of IERC725X and IERC725Y.

⚡️ Performance

Reduce deployment cost by an extra -45.814 gas compared to #236 by using directly IERC165 in the inheritance instead of ERC165 and calling super.supportsInterface.

The checks for interface ID ERC165 are directly inlined as a result inside the supportsInterface(bytes4) function of ERC725XCore, ERC725YCore and ERC725 contracts.

PR Checklist

  • Wrote Tests
  • Wrote Documentation
  • Ran npm run lint && npm run lint:solidity
  • Ran npm run format (prettier)
  • Ran npm run build
  • Ran npm run test

Previous gas cost (develop branch)

···············|·······················································|·············|··············|·············|···············|··············
|  Contract    ·  Method                                               ·  Min        ·  Max         ·  Avg        ·  # calls      ·  usd (avg)  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  execute(uint256,address,uint256,bytes)               ·      34726  ·      127088  ·      47290  ·          114  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  executeBatch(uint256[],address[],uint256[],bytes[])  ·      60485  ·      155455  ·      92070  ·           16  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  renounceOwnership()                                  ·      31283  ·       31328  ·      31306  ·            4  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  setData(bytes32,bytes)                               ·      32983  ·     8244933  ·    1494244  ·           17  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  setDataBatch(bytes32[],bytes[])                      ·      35151  ·     8247117  ·    1417890  ·           18  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  transferOwnership(address)                           ·      32069  ·       32091  ·      32084  ·            6  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Init  ·  initialize(address)                                  ·      51749  ·       51771  ·      51749  ·          124  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725X     ·  execute(uint256,address,uint256,bytes)               ·      31954  ·      124204  ·      44508  ·          114  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725X     ·  executeBatch(uint256[],address[],uint256[],bytes[])  ·      57611  ·      152526  ·      89181  ·           16  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725X     ·  renounceOwnership()                                  ·          -  ·           -  ·      23661  ·            2  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725X     ·  transferOwnership(address)                           ·          -  ·           -  ·      29219  ·            2  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Y     ·  renounceOwnership()                                  ·          -  ·           -  ·      23662  ·            2  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Y     ·  setData(bytes32,bytes)                               ·      30252  ·     8239813  ·    1491087  ·           17  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Y     ·  setDataBatch(bytes32[],bytes[])                      ·      32396  ·     8241968  ·    1414731  ·           18  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Y     ·  transferOwnership(address)                           ·          -  ·           -  ·      29197  ·            4  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  Deployments                                                         ·                                          ·  % of limit   ·             │
·······································································|·············|··············|·············|···············|··············
|  ERC725                                                              ·          -  ·           -  ·    2393988  ·          8 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725Init                                                          ·          -  ·           -  ·    2600372  ·        8.7 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725X                                                             ·          -  ·           -  ·    1832301  ·        6.1 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725XInit                                                         ·          -  ·           -  ·    2044314  ·        6.8 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725Y                                                             ·          -  ·           -  ·    1134143  ·        3.8 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725YInit                                                         ·          -  ·           -  ·    1344141  ·        4.5 %  ·          -  │
·----------------------------------------------------------------------|-------------|--------------|-------------|---------------|-------------·

New Gas costs (this branch)

·----------------------------------------------------------------------|----------------------------|-------------|-----------------------------·
|                         Solc version: 0.8.17                         ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 30000000 gas  │
·······································································|····························|·············|······························
|  Methods                                                                                                                                      │
···············|·······················································|·············|··············|·············|···············|··············
|  Contract    ·  Method                                               ·  Min        ·  Max         ·  Avg        ·  # calls      ·  usd (avg)  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  execute(uint256,address,uint256,bytes)               ·      34726  ·      127088  ·      47290  ·          114  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  executeBatch(uint256[],address[],uint256[],bytes[])  ·      60485  ·      155455  ·      92070  ·           16  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  renounceOwnership()                                  ·      31283  ·       31328  ·      31306  ·            4  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  setData(bytes32,bytes)                               ·      32983  ·     8083629  ·    1465779  ·           17  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  setDataBatch(bytes32[],bytes[])                      ·      35151  ·     8085813  ·    1391006  ·           18  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725      ·  transferOwnership(address)                           ·      32069  ·       32091  ·      32084  ·            6  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Init  ·  initialize(address)                                  ·      51749  ·       51771  ·      51749  ·          124  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725X     ·  execute(uint256,address,uint256,bytes)               ·      31954  ·      124204  ·      44508  ·          114  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725X     ·  executeBatch(uint256[],address[],uint256[],bytes[])  ·      57611  ·      152526  ·      89181  ·           16  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725X     ·  renounceOwnership()                                  ·          -  ·           -  ·      23661  ·            2  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725X     ·  transferOwnership(address)                           ·          -  ·           -  ·      29219  ·            2  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Y     ·  renounceOwnership()                                  ·          -  ·           -  ·      23662  ·            2  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Y     ·  setData(bytes32,bytes)                               ·      30252  ·     8078561  ·    1462631  ·           17  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Y     ·  setDataBatch(bytes32[],bytes[])                      ·      32396  ·     8080716  ·    1387855  ·           18  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  ERC725Y     ·  transferOwnership(address)                           ·          -  ·           -  ·      29197  ·            4  ·          -  │
···············|·······················································|·············|··············|·············|···············|··············
|  Deployments                                                         ·                                          ·  % of limit   ·             │
·······································································|·············|··············|·············|···············|··············
|  ERC725                                                              ·          -  ·           -  ·    2348169  ·        7.8 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725Init                                                          ·          -  ·           -  ·    2554540  ·        8.5 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725X                                                             ·          -  ·           -  ·    1828454  ·        6.1 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725XInit                                                         ·          -  ·           -  ·    2040383  ·        6.8 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725Y                                                             ·          -  ·           -  ·    1130213  ·        3.8 %  ·          -  │
·······································································|·············|··············|·············|···············|··············
|  ERC725YInit                                                         ·          -  ·           -  ·    1340258  ·        4.5 %  ·          -  │
·----------------------------------------------------------------------|-------------|--------------|-------------|---------------|-------------·

@CJ42 CJ42 marked this pull request as ready for review September 19, 2023 16:44
@CJ42 CJ42 marked this pull request as draft September 19, 2023 17:06
@CJ42 CJ42 force-pushed the refactor/custom-error branch 2 times, most recently from 5ca5fa0 to 7112a50 Compare September 20, 2023 10:54
@CJ42 CJ42 force-pushed the refactor/custom-error branch 5 times, most recently from a0c1c1a to b0acce2 Compare September 26, 2023 15:56
Base automatically changed from refactor/custom-error to develop September 27, 2023 08:52
@CJ42 CJ42 marked this pull request as ready for review October 4, 2023 10:52
Copy link
Collaborator

@YamenMerhi YamenMerhi left a comment

Choose a reason for hiding this comment

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

We need to keep super.supportsInterface, otherwise we will stop the inheritance chain in supportsInterface of child contracts such as LSP9, and LSP0 for example.

@CJ42
Copy link
Collaborator Author

CJ42 commented Oct 6, 2023

We need to keep super.supportsInterface, otherwise we will stop the inheritance chain in supportsInterface of child contracts such as LSP9, and LSP0 for example.

Tested and this actually will break the inheritance call via super. indeed, if one of the ERC725 contract is in the middle of the inheritance hierarchy. I will close this PR then as we cannot implement this change.

@CJ42 CJ42 closed this Oct 6, 2023
skimaharvey pushed a commit to skimaharvey/ERC725 that referenced this pull request Feb 22, 2024
…ders2

refactor: prepare wrapper utils for provider merge
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