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

Compilation fails with solc 0.8.23 #735

Closed
mmv08 opened this issue Jan 28, 2024 · 4 comments · Fixed by #772
Closed

Compilation fails with solc 0.8.23 #735

mmv08 opened this issue Jan 28, 2024 · 4 comments · Fixed by #772
Assignees

Comments

@mmv08
Copy link
Member

mmv08 commented Jan 28, 2024

Description

The project cannot be compiled with solc 0.8.23

Environment

  • Compiler version: 0.8.23
  • Compiler options (if applicable, e.g. optimizer enabled): default
  • Operating system: Ubuntu 22.04.3 LTS x86_64

Steps to reproduce

  1. Clone the repo
  2. Checkout to the commit 914d0f8fab0e8f73ef79581f6fbce86e34b049c3 (the latest one on the main branch at the moment of writing)
  3. npm i
  4. run cp .env.sample .env
  5. Set SOLIDITY_VERSION='0.8.23'
  6. run npm run build

Additional context

The command output:

➜  safe-smart-account git:(main) npm run build

> @safe-global/safe-smart-account@1.4.1-build.0 build
> hardhat compile

Solidity 0.8.23 is not fully supported yet. You can still use Hardhat, but some features, like stack traces, might not work correctly.

Learn more at https://hardhat.org/hardhat-runner/docs/reference/solidity-support

TypeError: Function needs to specify overridden contracts "IModuleManager" and "ModuleManager".
  --> contracts/SafeL2.sol:70:14:
   |
70 |     ) public override returns (bool success) {
   |              ^^^^^^^^
Note: This contract: 
  --> contracts/base/ModuleManager.sol:18:1:
   |
18 | abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager, IModuleManager {
   | ^ (Relevant source part starts here and spans across multiple lines).
Note: This contract: 
  --> contracts/interfaces/IModuleManager.sol:14:1:
   |
14 | interface IModuleManager is IGuardManager {
   | ^ (Relevant source part starts here and spans across multiple lines).

@remedcu
Copy link
Member

remedcu commented Jan 28, 2024

Running with compiler version v0.8.11 works, and it starts to have this bug from v0.8.12.

In the release note of v0.8.12, it has this line:

Inheritance: Consider functions in all ancestors during override analysis.

Based on the documentation, it seems we need to pass the path:

In this sense, an override path for a signature is a path through the inheritance graph that starts at the contract under consideration and ends at a contract mentioning a function with that signature that does not override.

But that results in compiling issues with v0.7.x.

@nlordell
Copy link
Collaborator

nlordell commented Feb 2, 2024

Might be worth, once resolving, adding a CI job that compiles with the latest compiler and optimisations so that we don’t regress on this.

mmv08 added a commit that referenced this issue Feb 8, 2024
This PR:
- Fix the benchmark CI job to prebent regressions like
#735. It is
fixed by removing the OR operator because if a benchmark failed, it only
outputted a message but didn't return a non-zero exit code.
- Also bumps the `0.8.x` compiler version to the latest one (0.8.24)
@opensea712
Copy link

@mmv08 so any solution for this issue yet?

@mmv08
Copy link
Member Author

mmv08 commented Mar 21, 2024

@mmv08 so any solution for this issue yet?

not yet, we use compiler v0.7.6 and this issue is a low priority for us

akshay-ap added a commit that referenced this issue Jun 26, 2024
akshay-ap added a commit that referenced this issue Jun 26, 2024
akshay-ap added a commit that referenced this issue Jun 26, 2024
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.
3521a4b
- 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.

---------

Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
akshay-ap added a commit that referenced this issue Jun 28, 2024
Fixes #775, #735

To have a broader context on overall changes in the code use this
[diff](https://github.com/safe-global/safe-smart-account/compare/499b17ad..improvement-execTransaction-post-call-hook)

### Changes in PR:
- Add a pre-hook function onBeforeExecTransaction
- Refactor changes in #772, #774
- Add a pre-hook function onBeforeExecTransactionFromModule
- Change function visibility of execTransaction from public to external

### 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](499b17a)
(Prior to merging #772)

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

### Gas usage with Safe contract

- Increase by 79 gas in execTransaction calls
- Increase by 44 gas in execTransactionFromModule* calls

#### 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](499b17a)
(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: 
- If a transaction updates threshold of a Safe, then event emits new
threshold rather than emitting the value prior to the update. I tried
adding `additionalInfo` as a method parameter and building this value
prior to the execution but compiler reports CompilerError: `Stack too
deep, try removing local variables`.
- Changes order of emitted events.

---------

Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
dodger213 added a commit to dodger213/safe-smart-contract that referenced this issue Aug 18, 2024
This PR:
- Fix the benchmark CI job to prebent regressions like
safe-global/safe-smart-account#735. It is
fixed by removing the OR operator because if a benchmark failed, it only
outputted a message but didn't return a non-zero exit code.
- Also bumps the `0.8.x` compiler version to the latest one (0.8.24)
BinaryBard-0604 added a commit to BinaryBard-0604/safe-smart-contract that referenced this issue Sep 23, 2024
This PR:
- Fix the benchmark CI job to prebent regressions like
safe-global/safe-smart-account#735. It is
fixed by removing the OR operator because if a benchmark failed, it only
outputted a message but didn't return a non-zero exit code.
- Also bumps the `0.8.x` compiler version to the latest one (0.8.24)
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 a pull request may close this issue.

5 participants