contracts-bedrock: forge-std v1.8.1 #10161

tynes · 2024-04-15T23:41:46Z

Description

Updates to latest forge-std release to get access to the latest
cheatcodes with a nice API.

https://github.com/foundry-rs/forge-std/releases/tag/v1.8.1

coderabbitai · 2024-04-15T23:42:11Z

Walkthrough

The updates across the contracts-bedrock package primarily focus on enhancing code clarity and efficiency by modifying visibility and mutability of functions in various contracts. Changes include the removal of unused constants, addition of new configurations, and widespread updates to function visibility to view or pure, emphasizing non-state-modifying behaviors. This refactoring aims to improve testing frameworks and ensure functions are optimized for performance and security.

Changes

Files	Changes
`.../lib/forge-std`, `.../scripts/DeployConfig.s.sol`, `.../scripts/Deployer.sol`	Updated subproject references and configurations, removed constants, and added new functions for testing environments.
`.../scripts/fpac/FPACOPS.s.sol`, `.../test`, `.../test/setup/CommonTest.sol`, `.../test/vendor/AddressAliasHelper.t.sol`	Updated function visibility to `internal view`, `external view`, `view`, or `pure` across various contracts and tests to indicate non-state-modifying behavior.
`.../.gas-snapshot`, `.../test/invariants`, `.../test/kontrol/deployment/DeploymentSummary.t.sol`, `.../test/legacy`, `.../test/libraries`	Changes in gas benchmarks and visibility updates in invariants and library tests, emphasizing clarity and non-state-modification.
`.../test/libraries/rlp`, `.../test/periphery/AssetReceiver.t.sol`	Added `pure` modifier and updated function visibility to enhance clarity and enforce non-state-modifying behavior in RLP handling and asset receiving functionalities.

These changes collectively aim to streamline the contract codebase, ensuring better maintainability and clearer understanding of each function's impact on the system state.

Recent Review Details

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between 46977a7 and 25ce3f3.

Files ignored due to path filters (1)

packages/contracts-bedrock/snapshots/state-diff/Kontrol-Deploy.json is excluded by !**/*.json

Files selected for processing (57)

packages/contracts-bedrock/.gas-snapshot (1 hunks)
packages/contracts-bedrock/lib/forge-std (1 hunks)
packages/contracts-bedrock/scripts/DeployConfig.s.sol (2 hunks)
packages/contracts-bedrock/scripts/Deployer.sol (2 hunks)
packages/contracts-bedrock/scripts/fpac/FPACOPS.s.sol (1 hunks)
packages/contracts-bedrock/test/L1/L1CrossDomainMessenger.t.sol (2 hunks)
packages/contracts-bedrock/test/L1/L1ERC721Bridge.t.sol (2 hunks)
packages/contracts-bedrock/test/L1/L1StandardBridge.t.sol (2 hunks)
packages/contracts-bedrock/test/L1/L2OutputOracle.t.sol (1 hunks)
packages/contracts-bedrock/test/L1/OptimismPortal.t.sol (2 hunks)
packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol (2 hunks)
packages/contracts-bedrock/test/L1/ProtocolVersions.t.sol (1 hunks)
packages/contracts-bedrock/test/L1/ResourceMetering.t.sol (1 hunks)
packages/contracts-bedrock/test/L1/SuperchainConfig.t.sol (1 hunks)
packages/contracts-bedrock/test/L1/SystemConfig.t.sol (2 hunks)
packages/contracts-bedrock/test/L2/CrossDomainOwnable3.t.sol (1 hunks)
packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol (3 hunks)
packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol (2 hunks)
packages/contracts-bedrock/test/L2/L2ERC721Bridge.t.sol (2 hunks)
packages/contracts-bedrock/test/L2/L2StandardBridge.t.sol (3 hunks)
packages/contracts-bedrock/test/L2/SequencerFeeVault.t.sol (1 hunks)
packages/contracts-bedrock/test/Safe/LivenessGuard.t.sol (1 hunks)
packages/contracts-bedrock/test/Safe/LivenessModule.t.sol (2 hunks)
packages/contracts-bedrock/test/cannon/PreimageOracle.t.sol (1 hunks)
packages/contracts-bedrock/test/dispute/AnchorStateRegistry.t.sol (1 hunks)
packages/contracts-bedrock/test/dispute/DelayedWETH.t.sol (1 hunks)
packages/contracts-bedrock/test/dispute/DisputeGameFactory.t.sol (2 hunks)
packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol (3 hunks)
packages/contracts-bedrock/test/dispute/WETH98.t.sol (1 hunks)
packages/contracts-bedrock/test/dispute/lib/LibClock.t.sol (1 hunks)
packages/contracts-bedrock/test/dispute/lib/LibGameId.t.sol (1 hunks)
packages/contracts-bedrock/test/dispute/lib/LibPosition.t.sol (10 hunks)
packages/contracts-bedrock/test/governance/GovernanceToken.t.sol (1 hunks)
packages/contracts-bedrock/test/governance/MintManager.t.sol (1 hunks)
packages/contracts-bedrock/test/invariants/AddressAliasHelper.t.sol (1 hunks)
packages/contracts-bedrock/test/invariants/Burn.Eth.t.sol (1 hunks)
packages/contracts-bedrock/test/invariants/Burn.Gas.t.sol (1 hunks)
packages/contracts-bedrock/test/invariants/CrossDomainMessenger.t.sol (2 hunks)
packages/contracts-bedrock/test/invariants/Encoding.t.sol (2 hunks)
packages/contracts-bedrock/test/invariants/Hashing.t.sol (3 hunks)
packages/contracts-bedrock/test/invariants/L2OutputOracle.t.sol (1 hunks)
packages/contracts-bedrock/test/invariants/OptimismPortal.t.sol (1 hunks)
packages/contracts-bedrock/test/invariants/OptimismPortal2.t.sol (1 hunks)
packages/contracts-bedrock/test/invariants/ResourceMetering.t.sol (5 hunks)
packages/contracts-bedrock/test/invariants/SafeCall.t.sol (2 hunks)
packages/contracts-bedrock/test/invariants/SystemConfig.t.sol (1 hunks)
packages/contracts-bedrock/test/kontrol/deployment/DeploymentSummary.t.sol (5 hunks)
packages/contracts-bedrock/test/legacy/DeployerWhitelist.t.sol (1 hunks)
packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol (1 hunks)
packages/contracts-bedrock/test/libraries/Bytes.t.sol (10 hunks)
packages/contracts-bedrock/test/libraries/Constants.t.sol (1 hunks)
packages/contracts-bedrock/test/libraries/Encoding.t.sol (2 hunks)
packages/contracts-bedrock/test/libraries/Hashing.t.sol (2 hunks)
packages/contracts-bedrock/test/libraries/rlp/RLPReader.t.sol (7 hunks)
packages/contracts-bedrock/test/libraries/rlp/RLPWriter.t.sol (9 hunks)
packages/contracts-bedrock/test/libraries/trie/MerkleTrie.t.sol (10 hunks)
packages/contracts-bedrock/test/periphery/AssetReceiver.t.sol (1 hunks)

Files not processed due to max files limit (18)

packages/contracts-bedrock/test/periphery/Transactor.t.sol
packages/contracts-bedrock/test/periphery/drippie/dripchecks/CheckBalanceLow.t.sol
packages/contracts-bedrock/test/periphery/drippie/dripchecks/CheckGelatoLow.t.sol
packages/contracts-bedrock/test/periphery/drippie/dripchecks/CheckTrue.t.sol
packages/contracts-bedrock/test/periphery/faucet/Faucet.t.sol
packages/contracts-bedrock/test/periphery/op-nft/Optimist.t.sol
packages/contracts-bedrock/test/periphery/op-nft/OptimistAllowlist.t.sol
packages/contracts-bedrock/test/periphery/op-nft/OptimistInviter.t.sol
packages/contracts-bedrock/test/setup/CommonTest.sol
packages/contracts-bedrock/test/universal/CrossDomainMessenger.t.sol
packages/contracts-bedrock/test/universal/FeeVault.t.sol
packages/contracts-bedrock/test/universal/OptimismMintableERC20.t.sol
packages/contracts-bedrock/test/universal/OptimismMintableERC20Factory.t.sol
packages/contracts-bedrock/test/universal/OptimismMintableERC721.t.sol
packages/contracts-bedrock/test/universal/OptimismMintableERC721Factory.t.sol
packages/contracts-bedrock/test/universal/ProxyAdmin.t.sol
packages/contracts-bedrock/test/universal/StandardBridge.t.sol
packages/contracts-bedrock/test/vendor/AddressAliasHelper.t.sol

Files skipped from review as they are similar to previous changes (55)

packages/contracts-bedrock/lib/forge-std
packages/contracts-bedrock/scripts/DeployConfig.s.sol
packages/contracts-bedrock/scripts/Deployer.sol
packages/contracts-bedrock/scripts/fpac/FPACOPS.s.sol
packages/contracts-bedrock/test/L1/L1CrossDomainMessenger.t.sol
packages/contracts-bedrock/test/L1/L1ERC721Bridge.t.sol
packages/contracts-bedrock/test/L1/L1StandardBridge.t.sol
packages/contracts-bedrock/test/L1/L2OutputOracle.t.sol
packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol
packages/contracts-bedrock/test/L1/ProtocolVersions.t.sol
packages/contracts-bedrock/test/L1/ResourceMetering.t.sol
packages/contracts-bedrock/test/L1/SuperchainConfig.t.sol
packages/contracts-bedrock/test/L1/SystemConfig.t.sol
packages/contracts-bedrock/test/L2/CrossDomainOwnable3.t.sol
packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol
packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol
packages/contracts-bedrock/test/L2/L2ERC721Bridge.t.sol
packages/contracts-bedrock/test/L2/L2StandardBridge.t.sol
packages/contracts-bedrock/test/L2/SequencerFeeVault.t.sol
packages/contracts-bedrock/test/Safe/LivenessGuard.t.sol
packages/contracts-bedrock/test/Safe/LivenessModule.t.sol
packages/contracts-bedrock/test/cannon/PreimageOracle.t.sol
packages/contracts-bedrock/test/dispute/AnchorStateRegistry.t.sol
packages/contracts-bedrock/test/dispute/DelayedWETH.t.sol
packages/contracts-bedrock/test/dispute/DisputeGameFactory.t.sol
packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol
packages/contracts-bedrock/test/dispute/WETH98.t.sol
packages/contracts-bedrock/test/dispute/lib/LibClock.t.sol
packages/contracts-bedrock/test/dispute/lib/LibGameId.t.sol
packages/contracts-bedrock/test/dispute/lib/LibPosition.t.sol
packages/contracts-bedrock/test/governance/GovernanceToken.t.sol
packages/contracts-bedrock/test/governance/MintManager.t.sol
packages/contracts-bedrock/test/invariants/AddressAliasHelper.t.sol
packages/contracts-bedrock/test/invariants/Burn.Eth.t.sol
packages/contracts-bedrock/test/invariants/Burn.Gas.t.sol
packages/contracts-bedrock/test/invariants/CrossDomainMessenger.t.sol
packages/contracts-bedrock/test/invariants/Encoding.t.sol
packages/contracts-bedrock/test/invariants/Hashing.t.sol
packages/contracts-bedrock/test/invariants/L2OutputOracle.t.sol
packages/contracts-bedrock/test/invariants/OptimismPortal.t.sol
packages/contracts-bedrock/test/invariants/OptimismPortal2.t.sol
packages/contracts-bedrock/test/invariants/ResourceMetering.t.sol
packages/contracts-bedrock/test/invariants/SafeCall.t.sol
packages/contracts-bedrock/test/invariants/SystemConfig.t.sol
packages/contracts-bedrock/test/kontrol/deployment/DeploymentSummary.t.sol
packages/contracts-bedrock/test/legacy/DeployerWhitelist.t.sol
packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol
packages/contracts-bedrock/test/libraries/Bytes.t.sol
packages/contracts-bedrock/test/libraries/Constants.t.sol
packages/contracts-bedrock/test/libraries/Encoding.t.sol
packages/contracts-bedrock/test/libraries/Hashing.t.sol
packages/contracts-bedrock/test/libraries/rlp/RLPReader.t.sol
packages/contracts-bedrock/test/libraries/rlp/RLPWriter.t.sol
packages/contracts-bedrock/test/libraries/trie/MerkleTrie.t.sol
packages/contracts-bedrock/test/periphery/AssetReceiver.t.sol

Additional comments not posted (3)

packages/contracts-bedrock/.gas-snapshot (1)

1-11: The updated gas values in the .gas-snapshot file appear consistent and reflect the changes mentioned in the PR summary. Ensure these values are validated against actual test runs to confirm their accuracy.

packages/contracts-bedrock/test/L1/OptimismPortal.t.sol (2)

201-201: The function test_minimumGasLimit_succeeds has been correctly updated to a view function as it does not modify state. This change aligns with best practices for function visibility in Solidity.

917-917: The function test_params_initValuesOnProxy_succeeds has been updated to a view function, which is appropriate given that it only reads state and does not modify it. This change enhances clarity and enforces good coding practices.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

protolambda

TYVM

tynes · 2024-04-16T00:38:06Z

Massive amounts of compiler warnings, i think its because its going from app layer assertions to native assertions

codecov · 2024-04-16T00:45:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.56%. Comparing base (46977a7) to head (25ce3f3).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #10161      +/-   ##
===========================================
+ Coverage    42.40%   43.56%   +1.16%     
===========================================
  Files           73       41      -32     
  Lines         4830     3971     -859     
  Branches       766      614     -152     
===========================================
- Hits          2048     1730     -318     
+ Misses        2676     2135     -541     
  Partials       106      106

Flag	Coverage Δ
cannon-go-tests	`82.29% <ø> (ø)`
chain-mon-tests	`27.14% <ø> (ø)`
common-ts-tests	`?`
contracts-ts-tests	`12.25% <ø> (ø)`
core-utils-tests	`?`
sdk-tests	`40.27% <ø> (ø)`

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

see 32 files with indirect coverage changes

coderabbitai

Actionable comments posted: 2

packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol

tynes · 2024-04-16T01:20:06Z

This slightly reduced build time

2.39 on current branch
2.43 on develop

tynes · 2024-04-16T04:28:28Z

Maybe I am smol brain but foundry-rs/forge-std#503 (comment) isn't just working 🤔

tynes · 2024-04-16T04:33:56Z

The OptimismPortal2 failing tests:

  [19128] OptimismPortal2_Test::test_initialize_succeeds()
    ├─ [489] 0xD64C5B1F2952CBC28Bd79EB02d3065BbA2696E3A::cfg() [staticcall]
    │   └─ ← [Return] DeployConfig: [0x9568d36E291c2C4c34fa5593fcE73715abEf6F9c]
    ├─ [2445] DeployConfig::superchainConfigGuardian() [staticcall]
    │   └─ ← [Return] 0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc
    ├─ [5166] OptimismPortalProxy::disputeGameFactory() [staticcall]
    │   ├─ [189] OptimismPortal::disputeGameFactory() [delegatecall]
    │   │   └─ ← [Revert] EvmError: Revert
    │   └─ ← [Revert] EvmError: Revert
    └─ ← [Revert] EvmError: Revert

Not sure why this would happen, looks like now its calling the portal and not the portal2?

edit: this was because the storage layout of the deploy config changed, so i removed the need to use a magic slot to set the value

Updates to latest `forge-std` release to get access to the latest cheatcodes with a nice API. https://github.com/foundry-rs/forge-std/releases/tag/v1.8.1

coderabbitai

Actionable comments posted: 0

Actionable comments outside the diff hunks (1)

packages/contracts-bedrock/test/L1/OptimismPortal.t.sol (1)
Line range hint 519-519: The usage of blockhash(block.number) in the test_proveWithdrawalTransaction_replayProveChangedOutputRootAndOutputIndex_succeeds function will always return 0, which may not be the intended behavior. Consider using a different block number that is within the last 256 blocks for a valid hash.
- blockhash(block.number)
+ blockhash(block.number - 1)  // Assuming the current block number minus one is within the last 256 blocks

ethereum-optimism/optimism#10161

tynes requested a review from a team as a code owner April 15, 2024 23:41

tynes requested a review from refcell April 15, 2024 23:41

protolambda approved these changes Apr 15, 2024

View reviewed changes

Inphi approved these changes Apr 16, 2024

View reviewed changes

coderabbitai bot reviewed Apr 16, 2024

View reviewed changes

packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol Show resolved Hide resolved

packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol Show resolved Hide resolved

tynes enabled auto-merge April 16, 2024 01:12

tynes force-pushed the ctb/forge-std-v181 branch from ffdfa3a to d1fd84f Compare April 17, 2024 00:04

tynes added 8 commits April 16, 2024 18:35

contracts-bedrock: forge-std v1.8.1

98c8d2e

Updates to latest `forge-std` release to get access to the latest cheatcodes with a nice API. https://github.com/foundry-rs/forge-std/releases/tag/v1.8.1

contracts-bedrock: fix warnings

1639086

lint: fix

1a24bcd

contracts-bedrock: fix compiler

70cbb18

lint: fix

2ed07a1

snapshots: update

454bb17

contracts-bedrock: make deploy config safe + expect safe mem

6e8676e

gas-snapshot

25ce3f3

tynes force-pushed the ctb/forge-std-v181 branch from d1fd84f to 25ce3f3 Compare April 17, 2024 00:40

coderabbitai bot reviewed Apr 17, 2024

View reviewed changes

tynes added this pull request to the merge queue Apr 17, 2024

Merged via the queue into develop with commit 05ab396 Apr 17, 2024
75 checks passed

tynes deleted the ctb/forge-std-v181 branch April 17, 2024 01:25

pcw109550 added a commit to ethereum-optimism/asterisc that referenced this pull request Apr 22, 2024

Remove USE_FAULT_PROOFS_SLOT usage

1288f70

ethereum-optimism/optimism#10161

pcw109550 mentioned this pull request Apr 22, 2024

Update optimism submodule version to d2263b1 ethereum-optimism/asterisc#58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contracts-bedrock: forge-std v1.8.1 #10161

contracts-bedrock: forge-std v1.8.1 #10161

tynes commented Apr 15, 2024

coderabbitai bot commented Apr 15, 2024 •

edited

Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

protolambda left a comment

tynes commented Apr 16, 2024

codecov bot commented Apr 16, 2024 •

edited

Loading

coderabbitai bot left a comment

tynes commented Apr 16, 2024

tynes commented Apr 16, 2024

tynes commented Apr 16, 2024 •

edited

Loading

coderabbitai bot left a comment

contracts-bedrock: forge-std v1.8.1 #10161

contracts-bedrock: forge-std v1.8.1 #10161

Conversation

tynes commented Apr 15, 2024

coderabbitai bot commented Apr 15, 2024 • edited Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

protolambda left a comment

Choose a reason for hiding this comment

tynes commented Apr 16, 2024

codecov bot commented Apr 16, 2024 • edited Loading

Codecov Report

coderabbitai bot left a comment

Choose a reason for hiding this comment

tynes commented Apr 16, 2024

tynes commented Apr 16, 2024

tynes commented Apr 16, 2024 • edited Loading

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Apr 15, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

codecov bot commented Apr 16, 2024 •

edited

Loading

tynes commented Apr 16, 2024 •

edited

Loading