safe-global / safe-smart-account

Safe allows secure management of blockchain assets.
https://safe.global
GNU Lesser General Public License v3.0
1.84k stars 907 forks source link

Create a pre call hook onBeforeExecTransaction #776

Closed akshay-ap closed 3 months ago

akshay-ap commented 3 months ago

Fixes #775, #735

To have a broader context on overall changes in the code use this diff

Changes in PR:

Code size change

Increase by 51 bytes in Safe with diff: https://github.com/safe-global/safe-smart-account/compare/499b17ad..improvement-execTransaction-post-call-hook:

This PR

Safe 21832 bytes (limit is 24576)
SafeL2 22612 bytes (limit is 24576)

Commit (Prior to merging #772)

Safe 21781 bytes (limit is 24576)
SafeL2 22623 bytes (limit is 24576)

Gas usage with Safe contract

This PR

·------------------------------------------------------------|----------------------------|-------------|------------------------------·
|                    Solc version: 0.7.6                     ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 100000000 gas  │
·····························································|····························|·············|·······························
|  Methods                                                                                                                             │
·····················|·······································|·············|··············|·············|···············|···············
|  Contract          ·  Method                               ·  Min        ·  Max         ·  Avg        ·  # calls      ·  usd (avg)   │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransaction                      ·      51999  ·     3638489  ·     133046  ·          248  ·           -  │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransactionFromModule            ·      51625  ·      184218  ·     127860  ·           11  ·           -  │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransactionFromModuleReturnData  ·      52268  ·      183974  ·     107914  ·            5  ·           -  │
·····················|·······································|·············|··············|·············|···············|···············

Commit (Prior to merging #772)

·------------------------------------------------------------|----------------------------|-------------|------------------------------·
|                    Solc version: 0.7.6                     ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 100000000 gas  │
·····························································|····························|·············|·······························
|  Methods                                                                                                                             │
·····················|·······································|·············|··············|·············|···············|···············
|  Contract          ·  Method                               ·  Min        ·  Max         ·  Avg        ·  # calls      ·  usd (avg)   │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransaction                      ·      51920  ·     3638410  ·     132980  ·          248  ·           -  │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransactionFromModule            ·      51581  ·      184186  ·     127818  ·           11  ·           -  │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransactionFromModuleReturnData  ·      52224  ·      183930  ·     107870  ·            5  ·           -  │
·····················|·······································|·············|··············|·············|···············|···············

Alternatives considered

A.

Add a post call hook onAfterExecTransaction

Cons:

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9677986904

Details


Totals Coverage Status
Change from base Build 9676758763: 0.0%
Covered Lines: 389
Relevant Lines: 401

💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9693321609

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
contracts/SafeL2.sol 0 1 0.0%
<!-- Total: 1 2 50.0% -->
Files with Coverage Reduction New Missed Lines %
contracts/SafeL2.sol 4 0.0%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 9676758763: -0.7%
Covered Lines: 384
Relevant Lines: 401

💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9694147162

Details


Totals Coverage Status
Change from base Build 9676758763: 0.0%
Covered Lines: 389
Relevant Lines: 401

💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9694187165

Details


Totals Coverage Status
Change from base Build 9676758763: 0.0%
Covered Lines: 389
Relevant Lines: 401

💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9694410087

Details


Totals Coverage Status
Change from base Build 9676758763: 0.0%
Covered Lines: 389
Relevant Lines: 401

💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9695364178

Details


Files with Coverage Reduction New Missed Lines %
contracts/base/ModuleManager.sol 1 92.93%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 9676758763: 0.0%
Covered Lines: 389
Relevant Lines: 401

💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9695778501

Details


Files with Coverage Reduction New Missed Lines %
contracts/base/ModuleManager.sol 1 92.93%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 9676758763: 0.0%
Covered Lines: 389
Relevant Lines: 401

💛 - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9696059645

Details


Totals Coverage Status
Change from base Build 9676758763: -0.01%
Covered Lines: 388
Relevant Lines: 400

💛 - Coveralls