Skip to content

Bump solc from 0.8.15 & 0.8.19 to 0.8.25#12356

Closed
AmadiMichael wants to merge 15 commits into
developfrom
sc/solc-version-update
Closed

Bump solc from 0.8.15 & 0.8.19 to 0.8.25#12356
AmadiMichael wants to merge 15 commits into
developfrom
sc/solc-version-update

Conversation

@AmadiMichael

@AmadiMichael AmadiMichael commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Description

Bump the SOLC version of all OP stack contracts from 0.8.15 and 0.8.19 to 0.8.25.
Also involves updating the lib-keccak dependency to the latest that has a loose pragma version of ^0.8.0 and not a strict 0.8.15.

Metadata

@semgrep-app

semgrep-app Bot commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Semgrep found 13 sol-style-malformed-require findings:

  • packages/contracts-bedrock/src/cannon/PreimageOracle.sol
  • packages/contracts-bedrock/scripts/periphery/deploy/DeployPeriphery.s.sol
  • packages/contracts-bedrock/scripts/libraries/DeployUtils.sol
  • packages/contracts-bedrock/scripts/deploy/DeployOwnership.s.sol
  • packages/contracts-bedrock/scripts/deploy/DeployConfig.s.sol
  • packages/contracts-bedrock/scripts/deploy/Deploy.s.sol
  • packages/contracts-bedrock/scripts/deploy/ChainAssertions.sol
  • packages/contracts-bedrock/scripts/autogen/SemverLock.s.sol
  • packages/contracts-bedrock/scripts/DeploySuperchain.s.sol

"challenge period too large" Malformed require statement style.

Ignore this finding from sol-style-malformed-require.

@AmadiMichael AmadiMichael marked this pull request as ready for review October 7, 2024 17:40
@AmadiMichael AmadiMichael requested a review from a team as a code owner October 7, 2024 17:40

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This implies that the proxy is still being compiled with the older version of solc?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wondering if there is a way around this somehow

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope, everything uses 0.8.25. But using Proxy.sol:Proxy like before, the test fails saying it's not unique... Is there a better solution to this?

@smartcontracts smartcontracts Oct 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suspect that some other file is also defining something called Proxy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps forge clean fixes this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rip, good catch. This is an example as to why reducing external deps makes things a bit harder. I could be ok with it as is although I do not love it. I would prefer to have our own beacon proxy impl that doesn't use the crazy inheritance of oz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the following syntax will work:

bytes memory code = vm.getDeployedCode("universal/Proxy.sol:Proxy");

taken from #12321

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's how it was used in some places which i had to change to forge-artifacts/Proxy.sol/Proxy.json because it was not working

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm able to get src/universal/Proxy.sol:Proxy working on this commit: 553f4d872985edea0b62eb88980e53a665d4e78bj.

Resolving.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ruh roh, no it looks like the failure is not in solidity but in the go implementation of vm.getCode()... :/

Comment thread packages/contracts-bedrock/lib/lib-keccak Outdated
@codecov

codecov Bot commented Oct 7, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.85%. Comparing base (4b1c12a) to head (4c6c1bb).
Report is 217 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12356      +/-   ##
===========================================
- Coverage    64.02%   63.85%   -0.18%     
===========================================
  Files           55       55              
  Lines         4606     4606              
===========================================
- Hits          2949     2941       -8     
- Misses        1474     1483       +9     
+ Partials       183      182       -1     
Flag Coverage Δ
cannon-go-tests 63.85% <ø> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@tynes

tynes commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Looks like this needs a rebase

@maurelian maurelian force-pushed the sc/solc-version-update branch from 382cc0a to 553f4d8 Compare October 29, 2024 17:31
@maurelian maurelian force-pushed the sc/solc-version-update branch from 918a0d2 to eb402b1 Compare October 29, 2024 20:47
@maurelian maurelian force-pushed the sc/solc-version-update branch from eb402b1 to 3d0932c Compare October 29, 2024 21:02
@maurelian maurelian closed this Nov 26, 2024
@maurelian maurelian deleted the sc/solc-version-update branch November 26, 2024 19:03
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 this pull request may close these issues.

EVM Engineering: Update all 0.8.15 code to 0.8.25

4 participants