op-node: Add dedicated PlasmaConfig struct to rollup.Config #10499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trianglesphere
requested review from
protolambda,
ajsutton and
a team
as code owners
WalkthroughWalkthroughThe recent code changes focus on restructuring the handling of Plasma configurations by introducing a new Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: .coderabbit.yml Files ignored due to path filters (4)
Files selected for processing (12)
Files skipped from review as they are similar to previous changes (12)
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (2)
op-node/rollup/derive/engine_queue.go (1)
144-145: Ensure the calculation logic incalcFinalityLookbackis well-documented for maintainability.Consider adding a comment explaining how the finality lookback is calculated when Plasma mode is enabled, as this could help future developers understand the logic more quickly.
op-node/rollup/derive/engine_queue_test.go (1)
1253-1255: Ensure consistent naming for configuration fields.The naming convention for configuration fields should be consistent. Consider renaming
BlockTimeandSeqWindowSizeto follow a similar pattern asDAChallengeWindowandDAResolveWindow, such asBlockTimeWindowandSequenceWindowSize.
trianglesphere
force-pushed
the
jg/plasma_config
branch
from
f6fee3f to
250575c
Compare
bitwiseguy
reviewed
May 11, 2024
bitwiseguy
reviewed
May 11, 2024
ajsutton
reviewed
May 12, 2024
There was a problem hiding this comment.
CI apparently said no:
testing.tRunner.func1.2({0x1dfd2a0, 0x40b1d60})
/usr/local/go/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
/usr/local/go/src/testing/testing.go:1548 +0x397
panic({0x1dfd2a0?, 0x40b1d60?})
/usr/local/go/src/runtime/panic.go:914 +0x21f
github.com/ethereum-optimism/optimism/op-node/rollup.(*Config).GetOPPlasmaConfig(0xc0034b6050)
/root/project/op-node/rollup/types.go:452 +0x3c
github.com/ethereum-optimism/optimism/op-node/node.(*OpNode).initL2(0xc000324ea0, {0x306d560, 0x45f7280}, 0xc0034b6000, {0x30812b0, 0xc002206190})
/root/project/op-node/node/node.go:390 +0x33f
github.com/ethereum-optimism/optimism/op-node/node.(*OpNode).init(0xc000324ea0, {0x306d560, 0x45f7280}, 0xc0034b6000, {0x30812b0, 0xc002206190})
/root/project/op-node/node/node.go:131 +0x1de
github.com/ethereum-optimism/optimism/op-node/node.New({0x306d560, 0x45f7280}, 0xc0034b6000, {0x3081418, 0xc001e08630}, {0x30812b0, 0xc002206190}, {0x0, 0x0}, 0xc00340ef00)
d4ceb3b to
226aeef
Compare
|
I should have fixed CI. This also now sets the local plasma config from the superchain plasma config |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #10499 +/- ##
============================================
+ Coverage 42.36% 81.92% +39.55%
============================================
Files 73 10 -63
Lines 4836 1079 -3757
Branches 766 0 -766
============================================
- Hits 2049 884 -1165
+ Misses 2680 163 -2517
+ Partials 107 32 -75
Flags with carried forward coverage won't be shown. Click here to find out more. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
trianglesphere
force-pushed
the
jg/plasma_config
branch
from
796eaea to
71dbd38
Compare
This commit adds a dedicated PlasmaConfig section to the rollup config. This collects all Plasma Mode configuration to the same place and enables future expansion of Plasma Mode configuration to be confined to a single location. In the interim, the op-node can read from both the legacy fields and the dedicated struct. If both are set, they must be consistent. The genesis creation tooling currently only writes with the new config.
trianglesphere
force-pushed
the
jg/plasma_config
branch
from
71dbd38 to
b093a52
Compare
carterqw2
pushed a commit
to celo-org/optimism
that referenced
this pull request
May 29, 2024
…-optimism#10499) * op-node: Add dedicated PlasmaConfig struct to rollup.Config This commit adds a dedicated PlasmaConfig section to the rollup config. This collects all Plasma Mode configuration to the same place and enables future expansion of Plasma Mode configuration to be confined to a single location. In the interim, the op-node can read from both the legacy fields and the dedicated struct. If both are set, they must be consistent. The genesis creation tooling currently only writes with the new config. * Set plasma mode from superchain registry * Remove LegacyUsePlasma usage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This commit adds a dedicated PlasmaConfig section to the rollup config. This collects all Plasma Mode configuration to the same place and enables future expansion of Plasma Mode configuration to be confined to a single location.
In the interim, the op-node can read from both the legacy fields and the dedicated struct. If both are set, they must be consistent. The genesis creation tooling currently only writes with the new config.
This also updates to a new version of the superchain registry to enable Plasma Mode chains in the superchain registry to properly set the rollup config when configured by the superchain flag name.
Tests
Tested locally & the output JSON looks good. Unfortunately
omitemptydoes not work on the address type.Metadata