Fix compilation issue with solidity versions >= 0.8.12

akshay-ap commented 4 months ago

Fixes: #735 and #773

Changes in PR

Add a hook function onAfterExecTransactionFromModule that gets called after execution from module.
SafeL2 overrides this function to emit event SafeModuleTransaction.
Add test that checks if execTransactionFromModuleReturnData in SafeL2 emits SafeModuleTransaction event.

Codesize increase by `33` bytes with compiler version 0.7.6

This PR:

Safe 21814 bytes (limit is 24576)
SafeL2 22632 bytes (limit is 24576)

Main branch

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

Impact on gas usage with `Safe` contract

Changes in this PR add additional overhead of 49 gas

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              ·  execTransactionFromModule            ·      51630  ·      184223  ·     127865  ·           11  ·           -  │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransactionFromModuleReturnData  ·      52273  ·      183979  ·     107919  ·            5  ·           -  │

Main branch

·------------------------------------------------------------|----------------------------|-------------|------------------------------·
|                    Solc version: 0.7.6                     ·  Optimizer enabled: false  ·  Runs: 200  ·  Block limit: 100000000 gas  │
·····························································|····························|·············|·······························
|  Methods                                                                                                                             │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransactionFromModule            ·      51581  ·      184186  ·     127818  ·           11  ·           -  │
·····················|·······································|·············|··············|·············|···············|···············
|  Safe              ·  execTransactionFromModuleReturnData  ·      52224  ·      183930  ·     107870  ·            5  ·           -  │

Why this approach?

Clean code
Introduces a hook that allows contracts inheriting Safe to do additional post processing.

Alternatives considered A.

Move the function execTransactionFromModule(...) from ModuleManager to Safe contract. https://github.com/safe-global/safe-smart-account/pull/772/commits/3521a4bd8b468ef5ae517729898f8bbc1fc325fa
But this is not an ideal approach as ModuleManager does not implement execTransactionFromModule.
Pro: Minimal code changes, no impact on codesize

B.

Create _onExecTransactionFromModule method that gets called by execTransactionFromModule
~~Con: This approach still has repeating code~~
Approach is similar to A with new function. Here, _onExecTransactionFromModule still calls its parent.
Pro: Increase in codesize by only 24 bytes

Side note:

Why event SafeModuleTransaction is not emitted by execTransactionFromModuleReturnData in SafeL2? Why SafeL2 does not override execTransactionFromModuleReturnData. -> execTransactionFromModuleReturnData in SafeL2 should also emit event. Might have been missed in the past. ~~Would be covered in a separate PR.~~. This PR fixes the issue.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9646938946

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.899%

Totals
Change from base Build 9462835574:	0.0%
Covered Lines:	388
Relevant Lines:	400

💛 - Coveralls

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9660691754

Details

5 of 5 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 92.909%

Totals
Change from base Build 9462835574:	0.01%
Covered Lines:	389
Relevant Lines:	401

💛 - Coveralls

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9660962226

Details

5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 92.909%

Totals
Change from base Build 9462835574:	0.01%
Covered Lines:	389
Relevant Lines:	401

💛 - Coveralls

mmv08 commented 4 months ago

Why event SafeModuleTransaction is not emitted by execTransactionFromModuleReturnData in SafeL2?

I think because previously execTransactionFromModuleReturnData used to call execTransactionFromModule internally but with the introduction of guards it couldn't rely on the return data buffer anymore and this was changed and overlooked in https://github.com/safe-global/safe-smart-account/pull/728

mmv08 commented 4 months ago

good catch sir! would be great to add a test for this

akshay-ap commented 4 months ago

good catch sir! would be great to add a test for this

Already added in the PR here: https://github.com/safe-global/safe-smart-account/pull/774/files

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9664189214

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.899%

Totals
Change from base Build 9462835574:	0.0%
Covered Lines:	388
Relevant Lines:	400

💛 - Coveralls

nlordell commented 4 months ago

Do we have some analysis on the gas cost implications of this change?

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9665699454

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.899%

Totals
Change from base Build 9462835574:	0.0%
Covered Lines:	388
Relevant Lines:	400

💛 - Coveralls

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9665717420

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.899%

Totals
Change from base Build 9462835574:	0.0%
Covered Lines:	388
Relevant Lines:	400

💛 - Coveralls

akshay-ap commented 4 months ago

Do we have some analysis on the gas cost implications of this change?

Added it in PR description.

nlordell commented 4 months ago

I also noticed that this implementation started emitting events for execTransactionFromModuleReturnData, so we should add tests for it as @mmv08 suggested.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9675853335

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.899%

Totals
Change from base Build 9462835574:	0.0%
Covered Lines:	388
Relevant Lines:	400

💛 - Coveralls

akshay-ap commented 4 months ago

I also noticed that this implementation started emitting events for execTransactionFromModuleReturnData, so we should add tests for it as @mmv08 suggested.

Added here: https://github.com/safe-global/safe-smart-account/pull/772/commits/4e719c27da667004d87ba8ee99fa0bc76e7c2f7b

Now, we don't need #774

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9675916114

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.899%

Totals
Change from base Build 9462835574:	0.0%
Covered Lines:	388
Relevant Lines:	400

💛 - Coveralls

akshay-ap commented 4 months ago

I'm not sure I understand this "con" from option B in the PR description:

aah, my bad. I have updated the description.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9676182437

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.899%

Totals
Change from base Build 9462835574:	0.0%
Covered Lines:	388
Relevant Lines:	400

💛 - Coveralls

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9676677224

Details

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 92.909%

Totals
Change from base Build 9462835574:	0.01%
Covered Lines:	389
Relevant Lines:	401

💛 - Coveralls

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9676677615

Details

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 92.909%

Totals
Change from base Build 9462835574:	0.01%
Covered Lines:	389
Relevant Lines:	401

safe-global / safe-smart-account