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

Enable Solidity IR Optimizations #71

Merged
merged 2 commits into from
Feb 26, 2024
Merged

Conversation

nlordell
Copy link
Collaborator

@nlordell nlordell commented Feb 26, 2024

This PR turns on the IR optimization flag when invoking the Solidity compiler for the Safe token lock contracts. There are small gas savings across the board (exception with small gas overhead when withdrawing 0 amounts, which we do not need to optimize for) and moderate code size savings:

Before

·----------------------------------|---------------------------|------------------|-----------------------------·
|       Solc version: 0.8.23       ·  Optimizer enabled: true  ·  Runs: 10000000  ·  Block limit: 30000000 gas  │
···································|···························|··················|······························
|  Methods                                                                                                      │
··················|················|·············|·············|··················|·············|················
|  Contract       ·  Method        ·  Min        ·  Max        ·  Avg             ·  # calls    ·  usd (avg)    │
··················|················|·············|·············|··················|·············|················
|  SafeTokenLock  ·  lock          ·      44146  ·      87946  ·           74071  ·         31  ·            -  │
··················|················|·············|·············|··················|·············|················
|  SafeTokenLock  ·  recoverERC20  ·          -  ·          -  ·           52052  ·          1  ·            -  │
··················|················|·············|·············|··················|·············|················
|  SafeTokenLock  ·  unlock        ·      52127  ·      52151  ·           52128  ·         78  ·            -  │
··················|················|·············|·············|··················|·············|················
|  SafeTokenLock  ·  withdraw      ·      24190  ·      82140  ·           68218  ·         25  ·            -  │
·-----------------|----------------|-------------|-------------|------------------|-------------|---------------·
SafeTokenLock 5089 bytes (limit is 24576)

After

·----------------------------------|---------------------------|------------------|-----------------------------·
|       Solc version: 0.8.23       ·  Optimizer enabled: true  ·  Runs: 10000000  ·  Block limit: 30000000 gas  │
···································|···························|··················|······························
|  Methods                                                                                                      │
··················|················|·············|·············|··················|·············|················
|  Contract       ·  Method        ·  Min        ·  Max        ·  Avg             ·  # calls    ·  usd (avg)    │
··················|················|·············|·············|··················|·············|················
|  SafeTokenLock  ·  lock          ·      43639  ·      87439  ·           73564  ·         31  ·            -  │
··················|················|·············|·············|··················|·············|················
|  SafeTokenLock  ·  recoverERC20  ·          -  ·          -  ·           51886  ·          1  ·            -  │
··················|················|·············|·············|··················|·············|················
|  SafeTokenLock  ·  unlock        ·      51633  ·      51657  ·           51634  ·         78  ·            -  │
··················|················|·············|·············|··················|·············|················
|  SafeTokenLock  ·  withdraw      ·      24261  ·      81598  ·           67836  ·         25  ·            -  │
·-----------------|----------------|-------------|-------------|------------------|-------------|---------------·
SafeTokenLock 4536 bytes (limit is 24576)

@nlordell nlordell requested a review from a team as a code owner February 26, 2024 09:14
@nlordell nlordell requested review from rmeissner, akshay-ap, mmv08 and remedcu and removed request for a team February 26, 2024 09:14
@remedcu
Copy link
Member

remedcu commented Feb 26, 2024

solidity-coverage >= 0.8.7 should solve the coverage issue.

Source: https://github.com/sc-forks/solidity-coverage/releases/tag/v0.8.7

@nlordell nlordell force-pushed the chore/enable-ir-optimizations branch from b145d61 to a1bcb49 Compare February 26, 2024 11:38
@nlordell
Copy link
Collaborator Author

Oh man, this led down a rabbit hole... but should be fixed now :)

expect(safeTokenLock.connect(alice).rescueToken(ZeroAddress, ZeroAddress, 0))
.to.be.revertedWithCustomError(safeTokenLock, 'OwnableUnauthorizedAccount')
.withArgs(alice)
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that I changed the order of the tests... While this may seem silly, there is a reason. The solidity-coverage took has some extra "hash de-duplication" logic when compiling via IR which messes up branch coverage computation. Changing the order of the tests fixes this. See:

sc-forks/solidity-coverage#863
sc-forks/solidity-coverage#868

@nlordell nlordell enabled auto-merge (squash) February 26, 2024 11:41
@nlordell nlordell merged commit 93cb636 into main Feb 26, 2024
8 checks passed
@nlordell nlordell deleted the chore/enable-ir-optimizations branch February 26, 2024 11:42
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants