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

Bump solc from 0.8.15 & 0.8.19 to 0.8.25 #12356

Draft
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

AmadiMichael
Copy link
Member

@AmadiMichael AmadiMichael commented Oct 7, 2024

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

Copy link
Contributor

semgrep-app bot commented Oct 7, 2024

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
@@ -550,7 +550,7 @@ contract DeployImplementations is Script {
vm.broadcast(msg.sender);
IProxy proxy = IProxy(
DeployUtils.create1({
_name: "Proxy",
_name: "forge-artifacts/Proxy.sol/Proxy.json",
Copy link
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
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
Member 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?

Copy link
Contributor

@smartcontracts smartcontracts Oct 7, 2024

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
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
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
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
Member 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
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
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()... :/

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.85%. Comparing base (4b1c12a) to head (4c6c1bb).
Report is 11 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
Copy link
Contributor

tynes commented Oct 7, 2024

Looks like this needs a rebase

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