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

op-chain-ops,op-node: interop feature-set deploy/rollup/chain-config #8256

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

protolambda
Copy link
Contributor

@protolambda protolambda commented Nov 23, 2023

Description

Metadata

Fix https://github.com/ethereum-optimism/interop/issues/24

Summary by CodeRabbit

  • New Features

    • Introduced an experimental feature-set activation system, similar to a hardfork, with a configurable activation time (InteropTime).
  • Enhancements

    • Updated system configuration to include the new InteropTime field for better control over feature activation.
    • Improved logging and description methods to display the InteropTime for increased transparency and easier debugging.
  • Documentation

    • Added explanations for the InteropTime field and its impact on the system's operation in relevant sections.

Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Merging #8256 (dee66db) into develop (82b21d2) will increase coverage by 0.04%.
Report is 11 commits behind head on develop.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8256      +/-   ##
===========================================
+ Coverage    40.04%   40.09%   +0.04%     
===========================================
  Files          164      164              
  Lines         6175     6175              
  Branches      1002     1002              
===========================================
+ Hits          2473     2476       +3     
+ Misses        3628     3623       -5     
- Partials        74       76       +2     
Flag Coverage Δ
cannon-go-tests 63.48% <ø> (ø)
chain-mon-tests 27.14% <ø> (ø)
common-ts-tests 26.74% <ø> (ø)
contracts-bedrock-tests 20.59% <ø> (+0.14%) ⬆️
contracts-ts-tests 100.00% <ø> (ø)
core-utils-tests 44.03% <ø> (ø)
sdk-next-tests 42.18% <ø> (ø)
sdk-tests 42.18% <ø> (ø)

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

see 3 files with indirect coverage changes

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Sorry about the go.mod conflicts with the geth update. In hindsight I could have pulled these in with the 1.13.5 changes.

@protolambda protolambda force-pushed the interop-deploy-config branch from 454281b to 0c89106 Compare November 29, 2023 21:42
Copy link
Contributor

coderabbitai bot commented Nov 29, 2023

Walkthrough

The codebase has introduced a new feature-set activation mechanism, akin to a hard fork, named Interop. This includes a new field InteropTime across various configurations and structs to determine the activation time of this feature-set. The DeployConfig struct now includes a method to calculate the InteropTime, and this time is integrated into the rollup.Config during deployment. The Config struct in op-node/rollup/types.go has been updated to check for the activation of Interop and to include its time in descriptions and logs.

Changes

File Path Change Summary
op-chain-ops/genesis/config.go
op-chain-ops/genesis/genesis.go
op-e2e/e2eutils/setup.go
op-e2e/setup.go
Added L2GenesisInteropTimeOffset and InteropTime fields to handle the new Interop hard fork time calculation and initialization.
op-node/rollup/types.go Added InteropTime field and IsInterop method to Config struct, and updated Description and LogDescription methods to include Interop time.

"In the realm of code, a hop and a skip,
A new fork in time, with a carrot to pick. 🥕
Interop springs forth, with a timely tick,
CodeRabbit 🐇 cheers, for the future's new trick!"


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

@protolambda
Copy link
Contributor Author

Rebased on develop, resolved the conflicts in the configs, the delta-hardfork definition PR touched the exact same code-paths

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 82b21d2 and 0c89106.
Files selected for processing (5)
  • op-chain-ops/genesis/config.go (3 hunks)
  • op-chain-ops/genesis/genesis.go (1 hunks)
  • op-e2e/e2eutils/setup.go (1 hunks)
  • op-e2e/setup.go (1 hunks)
  • op-node/rollup/types.go (4 hunks)
Additional comments: 11
op-chain-ops/genesis/config.go (3)
  • 115-120: The addition of the L2GenesisInteropTimeOffset field with the corresponding JSON tag is correctly implemented.

  • 480-488: The InteropTime method is correctly implemented to calculate the Interop hard fork time based on the L2GenesisInteropTimeOffset field.

  • 527-531: The RollupConfig method has been correctly updated to include the InteropTime in the returned rollup.Config.

op-chain-ops/genesis/genesis.go (1)
  • 65-71: The summary states that the NewL2Genesis function signature has been changed to include InteropTime, but the hunk does not reflect this change. The function signature should be updated to accept InteropTime as a parameter.
op-e2e/e2eutils/setup.go (2)
  • 159-162: The addition of the InteropTime field to the rollupCfg struct aligns with the summary and pull request description, ensuring that the end-to-end tests account for the new interop feature-set.

  • 162-162: Verify that the InteropTime method is correctly calculating the Interop hard fork time based on the L2GenesisInteropTimeOffset and that the L1GenesisBlockTimestamp is the intended reference timestamp for this calculation.

op-e2e/setup.go (1)
  • 434-437: The summary states that the InteropTime field was added to the SystemConfig struct, but the hunk shows it being added to the rollup.Config within the makeRollupConfig function. Please verify if the summary should be updated to reflect this, or if the code should be modified to match the summary.
op-node/rollup/types.go (4)
  • 86-88: The addition of the InteropTime field to the Config struct aligns with the summary and pull request description. This field is used to determine the activation time of the experimental feature-set, similar to a hardfork.

  • 288-291: The IsInterop method correctly checks if the Interop hardfork is active based on the given timestamp. This method is consistent with the other hardfork activation methods in the Config struct.

  • 319-322: The Description method has been updated to include the Interop time in the banner, which is consistent with the summary and pull request description.

  • 347-350: The LogDescription method has been updated to log the Interop time, ensuring that logs reflect the new configuration as described in the summary and pull request description.

@protolambda protolambda force-pushed the interop-deploy-config branch from 0c89106 to dee66db Compare November 29, 2023 22:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f8c5291 and dee66db.
Files ignored due to filter (2)
  • go.mod
  • go.sum
Files selected for processing (5)
  • op-chain-ops/genesis/config.go (3 hunks)
  • op-chain-ops/genesis/genesis.go (1 hunks)
  • op-e2e/e2eutils/setup.go (1 hunks)
  • op-e2e/setup.go (1 hunks)
  • op-node/rollup/types.go (4 hunks)
Files skipped from review due to trivial changes (1)
  • op-chain-ops/genesis/genesis.go
Additional comments: 9
op-chain-ops/genesis/config.go (3)
  • 115-120: The addition of L2GenesisInteropTimeOffset to the DeployConfig struct is consistent with the summary and allows for configuration of the Interop hard fork activation time.

  • 480-488: The InteropTime method correctly calculates the Interop activation time based on the genesis time and the L2GenesisInteropTimeOffset. This method will be used to determine the activation time for the Interop hard fork.

  • 527-531: The RollupConfig method has been successfully updated to include the InteropTime in the rollup.Config it returns. This ensures that the Interop activation time is correctly propagated through the system.

op-e2e/e2eutils/setup.go (1)
  • 159-162: The addition of the InteropTime field to the rollupCfg struct and its initialization using deployConf.InteropTime method is correct and aligns with the summary provided. Ensure that the InteropTime field is being used correctly wherever rollupCfg is utilized in the codebase.
op-e2e/setup.go (1)
  • 434-437: The summary states that InteropTime was added to the SystemConfig struct, but the hunk shows it being added to the rollup.Config struct. Please verify the accuracy of the summary or update the code to reflect the summary accurately.
op-node/rollup/types.go (4)
  • 83-92: The addition of the InteropTime field to the Config struct is consistent with the summary and follows the pattern of other similar fields for network upgrades.

  • 285-291: The IsInterop method is correctly implemented and follows the established pattern for checking the activation of network upgrades based on timestamps.

  • 319-325: The Description method has been correctly updated to include the InteropTime in the banner, enhancing the visibility of the Interop activation status.

  • 347-352: The LogDescription method has been correctly updated to include the InteropTime in the log output, which is consistent with the summary and improves observability.

@protolambda protolambda added this pull request to the merge queue Nov 29, 2023
Merged via the queue into develop with commit ca87b90 Nov 29, 2023
61 checks passed
@protolambda protolambda deleted the interop-deploy-config branch November 29, 2023 23:23
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.

2 participants