Skip to content

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 akshay-ap, mmv08, remedcu and rmeissner and removed request for a team February 26, 2024 09:14
@remedcu
Copy link
Contributor

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
@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.

4 participants