refactor(rollup-relayer): remove outdated logic #1561
Conversation
colinlyguo
requested review from
zimpha,
georgehao,
Thegaram and
0xmountaintop
WalkthroughThe pull request includes a version increment from Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 4
🧹 Outside diff range and nitpick comments (21)
rollup/internal/controller/watcher/bundle_proposer_test.go (1)
Line range hint
59-90: Consider moving defer statement closer to db initialization.While the database cleanup is handled correctly with
defer database.CloseDB(db), it's a good practice to place the defer statement immediately after the resource acquisition to ensure cleanup in case of early returns or panics.db := setupDB(t) + defer database.CloseDB(db) // Add genesis batch. block := &encoding.Block{ Header: &gethTypes.Header{ Number: big.NewInt(0), }, RowConsumption: &gethTypes.RowConsumption{}, } - defer database.CloseDB(db)rollup/internal/controller/watcher/bundle_proposer.go (2)
157-161: LGTM! Consider adding debug loggingThe codec version validation is well-placed and provides a clear error message. However, it might be helpful to add debug logging before the check to aid in troubleshooting.
codecVersion := encoding.CodecVersion(batches[0].CodecVersion) +log.Debug("checking codec version compatibility", "version", codecVersion, "required", encoding.CodecV4) if codecVersion <= encoding.CodecV3 { return fmt.Errorf("unsupported codec version: %v, expected at least %v", codecVersion, encoding.CodecV4) }
Line range hint
134-161: Well-structured codec version enforcementThe changes effectively enforce CodecV4 through two layers of protection:
- At the database query level (line 134)
- Through explicit validation (lines 158-160)
This defensive approach ensures robust handling of codec versions while maintaining good error reporting.
Consider documenting this codec version requirement in the package documentation to make it clear for future maintainers.
rollup/internal/controller/watcher/chunk_proposer_test.go (2)
Line range hint
19-167: Consider improving test maintainabilityThe test function is well-structured but could benefit from the following improvements:
- Extract test data setup into a helper function to reduce duplication
- Define constants for magic numbers (e.g., 50000000000, 1000000)
- Add comments explaining the rationale for specific test cases
Example refactor for test data setup:
+ func setupTestCase(t *testing.T, db *gorm.DB) error { + l2BlockOrm := orm.NewL2Block(db) + return l2BlockOrm.InsertL2Blocks(context.Background(), []*encoding.Block{block1, block2}) + } func testChunkProposerLimitsCodecV4(t *testing.T) { // ... test cases ... for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { db := setupDB(t) defer database.CloseDB(db) - l2BlockOrm := orm.NewL2Block(db) - err := l2BlockOrm.InsertL2Blocks(context.Background(), []*encoding.Block{block1, block2}) + err := setupTestCase(t, db)
Line range hint
189-243: Simplify codec version handling and improve test constantsThe test function has several areas that could be improved:
- Redundant codec version checks at lines 202-205 and 229-232
- Magic numbers for block counts and limits
- Brittle test data file path
Consider applying these improvements:
+ const ( + maxTestBlocks = 510 + maxBlocksPerChunk = 255 + maxExpectedBlocks = 2000 + testDataFile = "testdata/blockTrace_03.json" + ) func testChunkProposerBlobSizeLimitCodecV4(t *testing.T) { - codecVersions := []encoding.CodecVersion{encoding.CodecV4} - for _, codecVersion := range codecVersions { db := setupDB(t) block := readBlockFromJSON(t, "../../../testdata/blockTrace_03.json") - for i := int64(0); i < 510; i++ { + for i := int64(0); i < maxTestBlocks; i++ { // ... } - var chainConfig *params.ChainConfig - if codecVersion == encoding.CodecV4 { - chainConfig = ¶ms.ChainConfig{...} - } else { - assert.Fail(t, "unsupported codec version, expected CodecV4") - } + chainConfig := ¶ms.ChainConfig{ + LondonBlock: big.NewInt(0), + BernoulliBlock: big.NewInt(0), + CurieBlock: big.NewInt(0), + DarwinTime: new(uint64), + DarwinV2Time: new(uint64), + } cp := NewChunkProposer(context.Background(), &config.ChunkProposerConfig{ - MaxBlockNumPerChunk: 255, + MaxBlockNumPerChunk: maxBlocksPerChunk, // ... }, chainConfig, db, nil) // ... - if codecVersion == encoding.CodecV4 { - numBlocksMultiplier = 255 - } else { - assert.Fail(t, "unsupported codec version, expected CodecV4") - } + numBlocksMultiplier = maxBlocksPerChunk for i, chunk := range chunks { expected := numBlocksMultiplier * (uint64(i) + 1) - if expected > 2000 { - expected = 2000 + if expected > maxExpectedBlocks { + expected = maxExpectedBlocks } assert.Equal(t, expected, chunk.EndBlockNumber) } database.CloseDB(db) - } }rollup/tests/rollup_test.go (2)
55-66: Enhance error message and configuration setup.Consider the following improvements:
- Make the error message more descriptive for unsupported codec versions.
- Extract chain configuration setup into a helper function for better maintainability.
- assert.Fail(t, "unsupported codec version, expected CodecV4") + assert.Fail(t, "codec version %v is no longer supported, only CodecV4 is supported", codecVersion) // Consider adding a helper function: +func newTestChainConfig() *params.ChainConfig { + return ¶ms.ChainConfig{ + LondonBlock: big.NewInt(0), + BernoulliBlock: big.NewInt(0), + CurieBlock: big.NewInt(0), + DarwinTime: new(uint64), + DarwinV2Time: new(uint64), + } +}
194-211: Enhance assertion messages for better test diagnostics.The assertions are comprehensive but could benefit from more descriptive messages to help diagnose failures.
- assert.Len(t, bundles, 1) + assert.Len(t, bundles, 1, "expected exactly one bundle to be created") - assert.NotEmpty(t, bundle.FinalizeTxHash) + assert.NotEmpty(t, bundle.FinalizeTxHash, "bundle finalization transaction hash should not be empty") - assert.Equal(t, gethTypes.ReceiptStatusSuccessful, receipt.Status) + assert.Equal(t, gethTypes.ReceiptStatusSuccessful, receipt.Status, "bundle finalization transaction should be successful") - assert.Len(t, batches, 2) + assert.Len(t, batches, 2, "bundle should contain exactly 2 batches") - assert.Equal(t, batch.RollupStatus, bundle.RollupStatus) - assert.Equal(t, bundle.FinalizeTxHash, batch.FinalizeTxHash) + assert.Equal(t, batch.RollupStatus, bundle.RollupStatus, "batch rollup status should match bundle status") + assert.Equal(t, bundle.FinalizeTxHash, batch.FinalizeTxHash, "batch finalization transaction hash should match bundle")coordinator/internal/logic/provertask/batch_prover_task.go (1)
Remove commented hard fork compatibility checks across prover task files
The hard fork compatibility checks are consistently commented out across all prover task files (batch, chunk, and bundle). This appears to be an intentional removal of this validation logic rather than accidental commenting. Since these checks are uniformly disabled across all prover tasks, they should be removed entirely to maintain clean code.
coordinator/internal/logic/provertask/batch_prover_task.gocoordinator/internal/logic/provertask/chunk_prover_task.gocoordinator/internal/logic/provertask/bundle_prover_task.go🔗 Analysis chain
Line range hint
180-187: Verify if the commented hard fork compatibility check should be removed.There's a commented-out hard fork compatibility check in the code. Since we're removing outdated logic, should this commented section be removed as well?
Let's check if this pattern exists elsewhere:
Let me gather more context about the specific commented code section mentioned in the review.
Let me check if this hard fork compatibility check is actively used in other prover task files to understand if this is part of a broader pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for similar commented compatibility checks # Search for commented compatibility checks rg -A 2 '//.*compatible|//.*version|//.*hard.*fork' --type goLength of output: 10873
Script:
#!/bin/bash # Look for the specific commented code section in batch_prover_task.go rg -B 5 -A 5 'incompatible prover version' coordinator/internal/logic/provertask/batch_prover_task.goLength of output: 482
Script:
#!/bin/bash # Search for active (non-commented) hard fork compatibility checks in prover task files ast-grep --pattern 'if _, ok := taskCtx.HardForkNames[$_]; !ok { $$$ }' # Also check for any active usage of HardForkNames rg 'taskCtx.HardForkNames' --type goLength of output: 801
rollup/internal/controller/watcher/chunk_proposer.go (2)
Line range hint
139-182: Add safety mechanism for compatibility check loop.While the compatibility check and truncation logic is well-implemented, the infinite loop could potentially continue indefinitely in edge cases. Consider adding a maximum iteration limit or tracking the original chunk size to ensure termination.
Here's a suggested improvement:
func (p *ChunkProposer) updateDBChunkInfo(chunk *encoding.Chunk, codecVersion encoding.CodecVersion, metrics *utils.ChunkMetrics) error { if chunk == nil { return nil } compatibilityBreachOccurred := false + originalBlockCount := len(chunk.Blocks) + maxIterations := originalBlockCount // One iteration per block at most for { + if maxIterations <= 0 { + return fmt.Errorf("exceeded maximum iterations while checking chunk compatibility: original blocks %d", originalBlockCount) + } + maxIterations-- + compatible, err := encoding.CheckChunkCompressedDataCompatibility(chunk, codecVersion) if err != nil { log.Error("Failed to check chunk compressed data compatibility", "start block number", chunk.Blocks[0].Header.Number, "codecVersion", codecVersion, "err", err)
Line range hint
165-182: Extract metrics recalculation to a separate method.The metrics recalculation logic after truncation could be extracted to improve code maintainability and reusability.
Consider refactoring like this:
+func (p *ChunkProposer) recalculateChunkMetrics(chunk *encoding.Chunk, codecVersion encoding.CodecVersion) (*utils.ChunkMetrics, error) { + metrics, err := utils.CalculateChunkMetrics(chunk, codecVersion) + if err != nil { + return nil, fmt.Errorf("failed to calculate chunk metrics, start block number: %v, error: %w", + chunk.Blocks[0].Header.Number, err) + } + p.recordTimerChunkMetrics(metrics) + p.recordAllChunkMetrics(metrics) + return metrics, nil +} func (p *ChunkProposer) updateDBChunkInfo(chunk *encoding.Chunk, codecVersion encoding.CodecVersion, metrics *utils.ChunkMetrics) error { // ... previous code ... if compatibilityBreachOccurred { p.compressedDataCompatibilityBreachTotal.Inc() - // recalculate chunk metrics after truncation - var calcErr error - metrics, calcErr = utils.CalculateChunkMetrics(chunk, codecVersion) - if calcErr != nil { - return fmt.Errorf("failed to calculate chunk metrics, start block number: %v, error: %w", - chunk.Blocks[0].Header.Number, calcErr) - } - p.recordTimerChunkMetrics(metrics) - p.recordAllChunkMetrics(metrics) + var err error + metrics, err = p.recalculateChunkMetrics(chunk, codecVersion) + if err != nil { + return err + } }rollup/internal/orm/orm_test.go (1)
381-412: Consider adding error handling test casesWhile the changes correctly implement CodecV4 support, consider adding test cases to verify proper error handling for:
- Attempts to insert batches with unsupported codec versions
- Attempts to insert bundles with mismatched codec versions between batches
Example test case:
t.Run("InsertBundleWithInvalidCodec", func(t *testing.T) { invalidCodec := encoding.CodecVersion(999) _, err := bundleOrm.InsertBundle(context.Background(), []*Batch{dbBatch1}, invalidCodec) assert.Error(t, err) assert.Contains(t, err.Error(), "unsupported codec version") })rollup/internal/controller/watcher/batch_proposer_test.go (3)
284-287: Redundant Else Clause: Only Codec V4 is SupportedIn the
ifstatement, since onlyencoding.CodecV4is expected, theelseclause may not be necessary if other codec versions are no longer supported. However, if you intend to handle unsupported codecs explicitly, consider leaving a comment or logging a message.
377-381: Unnecessary Loop Over Single Codec VersionYou are iterating over
codecVersions := []encoding.CodecVersion{encoding.CodecV4}, which contains only one element. This loop may be unnecessary and can be simplified for clarity.Apply this diff to simplify the code:
- codecVersions := []encoding.CodecVersion{encoding.CodecV4} - for _, codecVersion := range codecVersions { + // Directly use encoding.CodecV4 since only one codec version is supported + codecVersion := encoding.CodecV4
285-287: Consistency in Codec Version HandlingEnsure that the codec version check is consistent across all tests. If only
encoding.CodecV4is supported, you might remove the conditional check altogether to simplify the code.rollup/internal/controller/relayer/l2_relayer_test.go (3)
54-64: Simplify the code by removing the unnecessary loopSince
codecVersionsonly containsencoding.CodecV4, the loop overcodecVersionsis unnecessary. You can remove the loop and directly setchainConfigforencoding.CodecV4.Apply this diff to simplify the code:
func testL2RelayerProcessPendingBatches(t *testing.T) { - codecVersions := []encoding.CodecVersion{encoding.CodecV4} - for _, codecVersion := range codecVersions { db := setupL2RelayerDB(t) defer database.CloseDB(db) l2Cfg := cfg.L2Config var chainConfig *params.ChainConfig - if codecVersion == encoding.CodecV4 { chainConfig = ¶ms.ChainConfig{ LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64), } - } else { - assert.Fail(t, "unsupported codec version, expected CodecV4") - } relayer, err := NewLayer2Relayer(context.Background(), l2Cli, db, l2Cfg.RelayerConfig, chainConfig, true, ServiceTypeL2RollupRelayer, nil) assert.NoError(t, err)
106-114: Simplify the code by removing the unnecessary loopSince only
encoding.CodecV4is used, the loop overcodecVersionscan be eliminated. Directly setchainConfigwithout the loop.Apply this diff:
func testL2RelayerProcessPendingBundles(t *testing.T) { - codecVersions := []encoding.CodecVersion{encoding.CodecV4} - for _, codecVersion := range codecVersions { db := setupL2RelayerDB(t) defer database.CloseDB(db) l2Cfg := cfg.L2Config var chainConfig *params.ChainConfig - if codecVersion == encoding.CodecV4 { chainConfig = ¶ms.ChainConfig{ LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64), } - } relayer, err := NewLayer2Relayer(context.Background(), l2Cli, db, l2Cfg.RelayerConfig, chainConfig, true, ServiceTypeL2RollupRelayer, nil) assert.NoError(t, err)
Line range hint
166-176: Simplify the code by removing the unnecessary loopThe loop over
codecVersionswith a single element can be removed to simplify the test function. SetchainConfigdirectly forencoding.CodecV4.Apply this diff:
func testL2RelayerFinalizeTimeoutBundles(t *testing.T) { - codecVersions := []encoding.CodecVersion{encoding.CodecV4} - for _, codecVersion := range codecVersions { db := setupL2RelayerDB(t) defer database.CloseDB(db) l2Cfg := cfg.L2Config l2Cfg.RelayerConfig.EnableTestEnvBypassFeatures = true l2Cfg.RelayerConfig.FinalizeBundleWithoutProofTimeoutSec = 0 var chainConfig *params.ChainConfig - if codecVersion == encoding.CodecV4 { chainConfig = ¶ms.ChainConfig{ LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64), } - } relayer, err := NewLayer2Relayer(context.Background(), l2Cli, db, l2Cfg.RelayerConfig, chainConfig, true, ServiceTypeL2RollupRelayer, nil) assert.NoError(t, err)rollup/internal/controller/relayer/l2_relayer.go (4)
571-574: Correct the error message to match the CodecV4 functionThe error message mentions "codecv3" but should reference "CodecV4" to align with the current function
constructFinalizeBundlePayloadCodecV4.Apply this diff to fix the error message:
- return fmt.Errorf("failed to construct finalizeBundle payload codecv3, index: %v, err: %w", dbBatch.Index, err) + return fmt.Errorf("failed to construct finalizeBundle payload codecv4, index: %v, err: %w", dbBatch.Index, err)
Line range hint
777-813: Handle potential nil values to prevent runtime errorsIn
constructCommitBatchPayloadCodecV4, there is a risk ofblobDataProoforblobbeingnil, which could lead to runtime panics when used. It's important to check fornilbefore proceeding.Add error handling to validate
blobDataProofandblob:blobDataProof, err := daBatch.BlobDataProofForPointEvaluation() if err != nil { return nil, nil, fmt.Errorf("failed to get blob data proof for point evaluation: %w", err) } + if blobDataProof == nil { + return nil, nil, fmt.Errorf("blobDataProof is nil") + } calldata, packErr := r.l1RollupABI.Pack("commitBatchWithBlobProof", daBatch.Version(), dbParentBatch.BatchHeader, encodedChunks, daBatch.SkippedL1MessageBitmap(), blobDataProof) if packErr != nil { return nil, nil, fmt.Errorf("failed to pack commitBatchWithBlobProof: %w", packErr) } + blob := daBatch.Blob() + if blob == nil { + return nil, nil, fmt.Errorf("blob is nil") + } return calldata, daBatch.Blob(), nil
Line range hint
819-837: Refactor to eliminate duplicate code inconstructFinalizeBundlePayloadCodecV4The function contains duplicated logic for packing calldata with and without proof. Refactoring can enhance readability and maintainability.
Refactor the function to reduce duplication:
func (r *Layer2Relayer) constructFinalizeBundlePayloadCodecV4(dbBatch *orm.Batch, aggProof *message.BundleProof) ([]byte, error) { var ( methodName string args []interface{} ) if aggProof != nil { // finalizeBundle with proof methodName = "finalizeBundleWithProof" args = []interface{}{ dbBatch.BatchHeader, common.HexToHash(dbBatch.StateRoot), common.HexToHash(dbBatch.WithdrawRoot), aggProof.Proof, } } else { // finalizeBundle without proof methodName = "finalizeBundle" args = []interface{}{ dbBatch.BatchHeader, common.HexToHash(dbBatch.StateRoot), common.HexToHash(dbBatch.WithdrawRoot), } } calldata, packErr := r.l1RollupABI.Pack(methodName, args...) if packErr != nil { return nil, fmt.Errorf("failed to pack %s: %w", methodName, packErr) } return calldata, nil }
Residual codec version references found in implementation code require attention
Several implementation files still contain references to older codec versions that need to be addressed:
rollup/internal/controller/relayer/l2_relayer.go: UsesCodecV0for inserting chunks and batchesrollup/internal/controller/watcher/chunk_proposer.go: Contains comparison withCodecV3rollup/internal/controller/watcher/bundle_proposer.go: Contains comparison withCodecV3rollup/internal/controller/watcher/batch_proposer.go: Contains comparison withCodecV3coordinator/internal/orm/batch.goandchunk.go: Initialize withCodecV0These references to older codec versions should be updated to use
CodecV4to maintain consistency with the current implementation and prevent potential compatibility issues.🔗 Analysis chain
Line range hint
422-429: Verify removal of residual references to earlier codec versionsEnsure that all references to previous codec versions (
CodecV0,CodecV1,CodecV2,CodecV3) have been removed from the codebase to prevent unintended behavior.Run the following script to search for usages of earlier codec versions:
Let me analyze the references found to determine if they are problematic. I'll need to check if these references are in test files or if they are in actual implementation code that could affect runtime behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for references to CodecV0 through CodecV3 in the codebase. # Expectation: No occurrences of CodecV0, CodecV1, CodecV2, or CodecV3. rg 'CodecV[0-3]'Length of output: 7230
Script:
#!/bin/bash # Let's check the actual implementation files for CodecV0-V3 usage, excluding tests and mocks rg 'CodecV[0-3]' \ --type go \ -g '!**/*test*' \ -g '!**/mock*' \ -g '!**/tests/*'Length of output: 842
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
common/version/version.go(1 hunks)coordinator/internal/logic/provertask/batch_prover_task.go(1 hunks)rollup/cmd/rollup_relayer/app/app.go(0 hunks)rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/relayer/l2_relayer_test.go(5 hunks)rollup/internal/controller/relayer/relayer_test.go(0 hunks)rollup/internal/controller/watcher/batch_proposer.go(1 hunks)rollup/internal/controller/watcher/batch_proposer_test.go(9 hunks)rollup/internal/controller/watcher/bundle_proposer.go(3 hunks)rollup/internal/controller/watcher/bundle_proposer_test.go(2 hunks)rollup/internal/controller/watcher/chunk_proposer.go(1 hunks)rollup/internal/controller/watcher/chunk_proposer_test.go(5 hunks)rollup/internal/controller/watcher/watcher_test.go(1 hunks)rollup/internal/orm/orm_test.go(3 hunks)rollup/tests/bridge_test.go(1 hunks)rollup/tests/rollup_test.go(2 hunks)
💤 Files with no reviewable changes (2)
- rollup/cmd/rollup_relayer/app/app.go
- rollup/internal/controller/relayer/relayer_test.go
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (20)
rollup/internal/controller/watcher/watcher_test.go (3)
107-110: Well-structured and comprehensive test coverage
The batch proposer test cases are well-organized and cover all essential aspects:
- Basic proposer limits
- Gas and calldata size estimation
- Blob size limits
- Chunk number per batch limits
The consolidation to CodecV4 is clean and maintains good test coverage.
113-113: Consider expanding bundle proposer test coverage
While consolidating to CodecV4 is good, having only a single test case for bundle proposer limits might be insufficient. Consider adding more specific test cases to cover different scenarios.
#!/bin/bash
# Description: Analyze the test coverage for bundle proposer
# to determine if additional test cases are needed
# Search for the old bundle proposer test implementations
rg -A 20 "TestBundleProposer.*Codecv[0-3]"
# Search for the new implementation to compare coverage
rg -A 20 "TestBundleProposerLimitsCodecV4"
# Look for bundle proposer implementation to identify testable scenarios
ast-grep --pattern 'type BundleProposer struct {
$$$
}'103-104: Verify test coverage for consolidated chunk proposer tests
The consolidation of codec v0-v3 tests into CodecV4 tests aligns with the refactoring objective. However, let's ensure all critical test scenarios from the previous versions are preserved.
✅ Verification successful
Let me gather more specific information about the chunk proposer tests.
Let me search for the actual test implementations to compare the coverage.
Based on the git diff output, I can now generate the final response:
Test consolidation maintains coverage while removing legacy codec support
The consolidation of codec tests is appropriate and maintains test coverage:
- The test structure and assertions remain identical between old and new versions
- Only configuration-specific changes were made (adding
DarwinV2Time) - The blob size limit test was simplified to only test CodecV4 while maintaining the core test logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the test coverage between old and new implementations
# to ensure no critical test scenarios were lost in the consolidation.
# Search for the old test implementations to analyze their coverage
rg -A 10 "TestChunkProposerCodecv[0-3]"
# Search for the new test implementations to compare coverage
rg -A 10 "TestChunkProposerLimitsCodecV4|TestChunkProposerBlobSizeLimitCodecV4"
Length of output: 2823
Script:
#!/bin/bash
# Search for the chunk proposer test implementations
rg -A 30 "func testChunkProposerLimitsCodecV4|func testChunkProposerBlobSizeLimitCodecV4" rollup/internal/controller/watcher/watcher_test.go
# Search for any removed test functions related to chunk proposer
git log -p | rg "^-.*func.*testChunkProposer.*Codecv[0-3]"
Length of output: 415
Script:
#!/bin/bash
# Get the implementation of one of the old tests for comparison
git log -p | rg -A 30 "^-func testChunkProposerCodecv3Limits"
# Get the implementation of the new test
rg -A 30 "^func testChunkProposerLimitsCodecV4" rollup/internal/controller/watcher/watcher_test.go
Length of output: 1721
rollup/internal/controller/watcher/bundle_proposer_test.go (3)
23-23: LGTM! Clear function naming.
The function rename to include CodecV4 suffix clearly indicates its focused scope and aligns with the codebase's move to support only Codec V4.
Line range hint 24-57: LGTM! Comprehensive test coverage.
The test cases are well-structured and cover important scenarios including:
- Edge cases with MaxUint64
- Boundary conditions (0, 1)
- Timeout scenarios
- Various batch number limits
91-91: Verify hardfork timing configurations.
While the initialization includes all required fields, please verify if using zero values for DarwinTime and DarwinV2Time is appropriate for the test scenarios.
✅ Verification successful
The search results show a consistent pattern across all test files where DarwinTime and DarwinV2Time are initialized using new(uint64), which creates zero-valued pointers. Let's verify if there are any test cases that use non-zero values to ensure we're not missing important test scenarios.
Zero-valued hardfork timing configuration is consistent with test patterns
The initialization of DarwinTime and DarwinV2Time as zero-valued pointers (new(uint64)) is consistent across all test files in the codebase. This pattern is used uniformly in test scenarios for batch proposers, chunk proposers, and rollup tests. No test cases were found that require non-zero timing values, indicating this is the intended test configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files to verify consistent hardfork time initialization patterns
rg -A 1 "DarwinTime.*new\(uint64\)" --type go
Length of output: 3732
Script:
#!/bin/bash
# Search for test cases where DarwinTime or DarwinV2Time are set to non-zero values
rg "DarwinTime.*=.*[1-9]" --type go
rg "DarwinV2Time.*=.*[1-9]" --type go
Length of output: 77
rollup/tests/bridge_test.go (1)
211-211: LGTM! The changes align with the codec version streamlining.
The addition of TestCommitBatchAndFinalizeBundleCodecV4 and removal of multi-codec test cases aligns with the PR objective of removing outdated logic. This change reflects the codebase's transition to exclusively support CodecV4.
Let's verify the test coverage for CodecV4:
✅ Verification successful
Test implementation for CodecV4 is comprehensive and properly isolated
The verification shows that:
- The new test
testCommitBatchAndFinalizeBundleCodecV4is well-implemented with proper setup, assertions, and cleanup - It exclusively tests CodecV4 functionality with appropriate chain configuration
- The remaining references to older codec versions in test files are:
- A comment in
rollup_test.goabout historical behavior - Test data setup in
gas_oracle_test.gousingCodecV0which is isolated from the rollup functionality
- A comment in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all CodecV4-related test cases are properly implemented
# and no legacy codec version tests remain.
# Check for any remaining references to old codec versions in test files
echo "Checking for legacy codec version references..."
rg -i "codec.*v[0-3]" "rollup/tests"
# Look for the implementation of the new test case
echo "Verifying new test implementation..."
ast-grep --pattern 'func testCommitBatchAndFinalizeBundleCodecV4($$$) {
$$$
}'
Length of output: 12725
rollup/internal/controller/watcher/bundle_proposer.go (1)
134-134: Verify the impact of restricting to CodecV4
The change to GetBatchesGEIndexGECodecVersion with encoding.CodecV4 means older batches with CodecV3 will be rejected. While this aligns with removing outdated logic, we should ensure there are no active CodecV3 batches in the system.
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient evidence about CodecV3 usage in the codebase.
Change to CodecV4 restriction is consistent with existing checks
The change to restrict batches to CodecV4 or higher is safe and consistent with the codebase. All proposer components already have explicit checks rejecting CodecV3:
batch_proposer.gorejects CodecV3 with error "unsupported codec version"chunk_proposer.gohas the same restrictionbundle_proposer.goitself already had this check
The only references to CodecV3 are in test files (orm_test.go) which appear to be testing version compatibility scenarios. The change aligns with the existing validation logic across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing batches using CodecV3 or lower that might be affected
# Search for any references to CodecV3 in batch-related code
rg -A 5 "CodecV3" --type go
# Look for any test cases that might need updating
ast-grep --pattern 'func Test$_($_, $_) {
$$$
CodecV3
$$$
}'
Length of output: 2915
rollup/internal/controller/watcher/chunk_proposer_test.go (1)
Line range hint 1-243: Changes align well with PR objectives
The refactoring successfully removes support for older codec versions while maintaining comprehensive test coverage for Codec V4. The changes are consistent with the PR's goal of removing outdated logic.
rollup/tests/rollup_test.go (1)
Line range hint 55-211: LGTM! The test changes align well with the PR objectives.
The test successfully validates the transition to CodecV4 and properly tests the batch commitment and bundle finalization process. The removal of outdated codec version support aligns with the PR's goal of removing outdated logic.
coordinator/internal/logic/provertask/batch_prover_task.go (1)
246-247: LGTM! Clear error handling for codec version enforcement.
The change aligns with the PR objective of removing outdated logic by enforcing Codec V4. The error message is clear and informative.
Let's verify if there are any remaining references to older codec versions:
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information. The search reveals that:
-
The codebase has consistently moved to enforce CodecV4 across multiple components:
- Bundle proposer:
bundle_proposer.goenforces CodecV4 - Batch proposer:
batch_proposer.goenforces CodecV4 - Chunk proposer:
chunk_proposer.goenforces CodecV4 - L2 Relayer: Only handles CodecV4 in commit batch operations
- Bundle proposer:
-
All test files have been updated to use CodecV4:
- Test cases explicitly check for CodecV4
- Older codec versions are marked as unsupported
- Error messages consistently indicate CodecV4 as the minimum required version
LGTM! The codec version enforcement is consistent across the codebase.
The change aligns with the broader codebase where CodecV4 is now the standard version. All components that process batches enforce the same version requirement with similar error messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to older codec versions that might need updating
# Search for codec version references
rg -A 2 "CodecV[0-9]|codec.*version" --type go
# Search for potential version checks
ast-grep --pattern 'if $_ != encoding.CodecV4'
Length of output: 28883
rollup/internal/controller/watcher/batch_proposer.go (2)
250-254: LGTM! Verify codec version usage across the codebase.
The codec version validation is well-placed and aligns with the PR objective of removing outdated logic. The error message is clear and informative.
Let's verify that no other parts of the codebase still expect CodecV3 or lower:
✅ Verification successful
LGTM! The codebase is properly enforcing CodecV4 as the minimum version.
The verification shows that:
- All codec version checks consistently enforce CodecV4 as the minimum supported version
- Test cases have been updated to only test with CodecV4
- Database schema and ORM layers properly handle the codec version field
- The L2 relayer has specific handlers for CodecV4 payloads
- No legacy code paths exist that would accept older codec versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to CodecV3 or lower versions that might need updating
# Search for CodecV3 references
echo "Searching for CodecV3 references:"
rg -A 3 "CodecV3|Codec.*V3"
# Search for potential version comparisons
echo "Searching for codec version comparisons:"
ast-grep --pattern 'codec.Version() $_ encoding.Codec'
Length of output: 73418
Script:
#!/bin/bash
# Search for references to CodecV4 and version checks to verify the migration
echo "Searching for CodecV4 references and version checks:"
rg -A 3 "CodecV4|Codec.*V4"
# Search for batch codec version handling
echo "Searching for batch codec version handling:"
ast-grep --pattern 'codec.Version() $_ encoding.CodecV4'
# Search for any remaining codec version comparisons
echo "Searching for codec version comparisons:"
ast-grep --pattern 'codec.Version() $_ encoding.Codec'
Length of output: 19898
250-254: Consider documenting upgrade implications.
While the codec version restriction is correct, this change could affect system behavior during upgrades. Consider:
- How will the system handle existing batches created with older codec versions?
- Is there a migration strategy for handling in-flight batches during the upgrade?
- Should we add logging to track rejected batches during the transition period?
Let's check for existing batches with older codec versions:
✅ Verification successful
Let me generate additional scripts to search for upgrade-related code and migration handling:
This will help us understand:
- How codec versions are defined and checked
- If there are any migration mechanisms in place
- How version checks are implemented in batch processing
The results will help determine if there are adequate upgrade mechanisms or if additional safeguards are needed.
Based on the search results, I can now provide a final response since I have enough information about how codec versions are handled in the codebase:
Codec version upgrade is properly managed with clear migration paths
The codebase has robust handling for codec version upgrades:
-
Database schema supports codec versioning through migrations:
- Tables track codec versions for batches, chunks and bundles
- Proper indices exist for version-based queries
- Migration scripts handle version updates
-
Version checks are consistently enforced:
- All components (chunk proposer, batch proposer, bundle proposer) validate minimum codec version
- Clear error messages guide users to upgrade
- Database queries filter by codec version using
GetBatchesGEIndexGECodecVersion
-
Backward compatibility is maintained:
- Database can store multiple codec versions
- Version checks prevent processing of unsupported versions
- Proper error handling for version mismatches
The original concern about documenting upgrade implications is already well-addressed by the existing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for code that handles batch codec version migrations or upgrades
# Search for batch version handling code
echo "Searching for batch version handling:"
rg -A 5 "codec.*version|batch.*version|version.*migration"
# Search for upgrade-related code
echo "Searching for upgrade handling:"
rg -A 5 "upgrade.*batch|migrate.*batch"
Length of output: 73418
Script:
#!/bin/bash
# Search for upgrade-related code and migration handling
# Search for codec version constants and version checks
echo "Searching for codec version definitions and checks:"
rg -A 5 "CodecV|codec.*version.*check"
# Search for migration-related code
echo "Searching for migration code:"
rg -A 5 "migrate|migration|upgrade"
# Search for batch processing with version checks
echo "Searching for batch version handling:"
ast-grep --pattern 'if $_ <= $_ {
$$$
}'
Length of output: 106876
rollup/internal/controller/watcher/chunk_proposer.go (1)
280-282: LGTM! Verify impact on existing deployments.
The codec version check is well-implemented with clear error messaging. However, since this is a breaking change for systems using CodecV3 or lower, we should verify the impact.
Let's check for any remaining CodecV3 usage in the codebase:
✅ Verification successful
Based on the search results, I can generate the final response since I have enough information to verify the concern:
CodecV3 deprecation is consistently enforced across the codebase
The codebase shows consistent enforcement of CodecV4 as the minimum supported version across multiple components:
chunk_proposer.go: Checks for CodecV4bundle_proposer.go: Checks for CodecV4batch_proposer.go: Checks for CodecV4
The only remaining CodecV3 references are in:
- Test files (
orm_test.go) which validate compatibility across versions - Mock contracts (
MockBridge.sol) that maintain backward compatibility for testing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for CodecV3 references to ensure complete migration
# Test: Look for CodecV3 usage patterns that might indicate incomplete migration
rg -A 5 "CodecV3|V3"
# Test: Look for potential version checks that might need updating
ast-grep --pattern 'if $_ <= encoding.CodecV3 { $$$_ }'
Length of output: 38380
rollup/internal/orm/orm_test.go (2)
169-169: LGTM: CodecV4 support added to chunk tests
The addition of encoding.CodecV4 to the test cases ensures proper coverage for the new codec version while maintaining backward compatibility tests.
232-232: LGTM: CodecV4 support added to batch tests
The addition of encoding.CodecV4 to batch tests ensures comprehensive testing of the new codec version while maintaining tests for previous versions.
rollup/internal/controller/watcher/batch_proposer_test.go (2)
63-68: Validation: New Test Case for Gas Limit
The test case "MaxL1CommitGasPerBatchIsFirstChunk" correctly checks the scenario when the maxL1CommitGas limit is exactly the gas of the first chunk. The test parameters seem appropriate.
69-76: Validation: New Test Case for Calldata Size Limit
The test case "MaxL1CommitCalldataSizePerBatchIsFirstChunk" appropriately tests the condition when the maxL1CommitCalldataSize limit equals the size of the first chunk's calldata. The parameters are correctly set.
rollup/internal/controller/relayer/l2_relayer_test.go (1)
332-333: Update to CodecV4 in InsertBundle is appropriate
Using encoding.CodecV4 in InsertBundle aligns the test with the updated codec version and ensures consistency.
rollup/internal/controller/relayer/l2_relayer.go (1)
422-424:
Update error message to reflect CodecV4 instead of V3/V4
The error message references "V3/V4" even though the code now supports only CodecV4. Updating the error message will improve clarity and maintain consistency.
Apply this diff to fix the error message:
- log.Error("failed to construct commitBatchWithBlobProof payload for V3/V4", "codecVersion", codecVersion, "index", dbBatch.Index, "err", err)
+ log.Error("failed to construct commitBatchWithBlobProof payload for CodecV4", "codecVersion", codecVersion, "index", dbBatch.Index, "err", err)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
rollup/internal/controller/relayer/l2_relayer.go (2)
422-427: Improve error message clarity for unsupported codec versions.The error message could be more specific to indicate that only CodecV4 is supported.
- log.Error("unsupported codec version", "codecVersion", codecVersion) + log.Error("unsupported codec version: only CodecV4 is supported", "codecVersion", codecVersion)
Line range hint
819-841: Add documentation for proof and no-proof cases.The function handles both cases well, but could benefit from documentation explaining when each case is used.
Add a comment like:
+// constructFinalizeBundlePayloadCodecV4 constructs the finalize bundle payload for codec v4. +// It supports two modes: +// 1. With proof (aggProof != nil): Used in production for secure finalization +// 2. Without proof (aggProof == nil): Used in test environments when EnableTestEnvBypassFeatures is true func (r *Layer2Relayer) constructFinalizeBundlePayloadCodecV4(dbBatch *orm.Batch, aggProof *message.BundleProof) ([]byte, error) {rollup/internal/controller/watcher/batch_proposer_test.go (5)
255-256: Simplify Test Function by Removing Unnecessary LoopSince only
CodecV4is supported, the loop overcodecVersionswith a single element adds unnecessary complexity. Consider removing the loop to streamline the test functiontestBatchProposerBlobSizeLimitCodecV4.Apply this diff to simplify the test:
-func testBatchProposerBlobSizeLimitCodecV4(t *testing.T) { - codecVersions := []encoding.CodecVersion{encoding.CodecV4} - for _, codecVersion := range codecVersions { +func testBatchProposerBlobSizeLimitCodecV4(t *testing.T) {
284-287: Remove Unreachable Code for Unsupported CodecsThe
elseblock checking for unsupported codec versions is unnecessary since onlyCodecV4is being used. Removing it will simplify the code.Apply this diff to remove the unreachable
elseblock:if codecVersion == encoding.CodecV4 { chainConfig = ¶ms.ChainConfig{ LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64), } - } else { - assert.Fail(t, "unsupported codec version, expected CodecV4") }
332-336: Confirm Conditional Logic for Codec VersionIn the conditional statement, ensure that the logic correctly handles only
CodecV4. Since other codecs are no longer supported, consider simplifying the condition or removing theelseblock if it's unnecessary.
347-348: Simplify Test Function by Eliminating Unnecessary LoopSimilar to previous feedback, in
testBatchProposerMaxChunkNumPerBatchLimitCodecV4, the loop overcodecVersionscan be removed to simplify the test structure.Apply this diff to streamline the function:
-func testBatchProposerMaxChunkNumPerBatchLimitCodecV4(t *testing.T) { - codecVersions := []encoding.CodecVersion{encoding.CodecV4} - for _, codecVersion := range codecVersions { +func testBatchProposerMaxChunkNumPerBatchLimitCodecV4(t *testing.T) {
377-381: Eliminate Redundant Conditional ChecksWithin the test, the conditional check for
codecVersion == encoding.CodecV4is redundant since onlyCodecV4is used. Removing this condition can make the test clearer.Apply this diff to remove the unnecessary condition:
- var expectedChunkNum uint64 - var chainConfig *params.ChainConfig - if codecVersion == encoding.CodecV4 { chainConfig = ¶ms.ChainConfig{ LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64), } expectedChunkNum = 45 - } else { - assert.Fail(t, "unsupported codec version, expected CodecV4") - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
rollup/internal/controller/relayer/l2_relayer.go(4 hunks)rollup/internal/controller/watcher/batch_proposer_test.go(9 hunks)
🔇 Additional comments (4)
rollup/internal/controller/relayer/l2_relayer.go (1)
Line range hint 777-818: LGTM: Well-structured implementation with proper error handling.
The implementation properly handles batch encoding, blob data proof generation, and includes descriptive error messages with context.
rollup/internal/controller/watcher/batch_proposer_test.go (3)
23-23: Function Renaming Reflects Codec V4 Testing
The function testBatchProposerLimitsCodecV4 is appropriately renamed to indicate that it tests the limits specific to CodecV4. This improves clarity and maintainability.
69-76: Ensure Calldata Size Limit Accurate in Test Case
In the test case "MaxL1CommitCalldataSizePerBatchIsFirstChunk", the maxL1CommitCalldataSize is set to 60. Please confirm that this value correctly represents the calldata size limit for the first chunk under CodecV4.
Run the following script to validate the calldata size limit:
#!/bin/bash
# Description: Verify that the calldata size limit matches the expected value for CodecV4
# Expected calldata size limit value
expected_calldata_size=60
# Command to extract calldata size values from the codebase (replace with actual path if needed)
rg --no-heading --line-number 'TotalL1CommitCalldataSize' . | awk '{print $NF}' | sort -u
# Check if the expected value exists
if rg -q "$expected_calldata_size" <<< "$(rg 'TotalL1CommitCalldataSize' .)"; then
echo "Calldata size limit $expected_calldata_size found in the codebase."
else
echo "Calldata size limit $expected_calldata_size not found. Please verify."
fi63-68: Verify Gas Limit Value in Test Case
In the test case "MaxL1CommitGasPerBatchIsFirstChunk", the maxL1CommitGas is set to 249179. Please ensure that this value accurately represents the gas limit for the first chunk under CodecV4.
Run the following script to confirm that 249179 is the correct gas limit:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1561 +/- ##
===========================================
- Coverage 52.39% 52.12% -0.27%
===========================================
Files 157 157
Lines 12647 12429 -218
===========================================
- Hits 6626 6479 -147
+ Misses 5447 5393 -54
+ Partials 574 557 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
Line range hint
836-877: Consider adding error recovery mechanism.While the implementation is correct, consider adding retry logic for transient errors during blob data proof generation.
func (r *Layer2Relayer) constructCommitBatchPayloadCodecV4(dbBatch *orm.Batch, dbParentBatch *orm.Batch, dbChunks []*orm.Chunk, chunks []*encoding.Chunk) ([]byte, *kzg4844.Blob, error) { + const maxRetries = 3 + var lastErr error + + for i := 0; i < maxRetries; i++ { batch := &encoding.Batch{ Index: dbBatch.Index, TotalL1MessagePoppedBefore: dbChunks[0].TotalL1MessagesPoppedBefore, ParentBatchHash: common.HexToHash(dbParentBatch.Hash), Chunks: chunks, } codec, err := encoding.CodecFromVersion(encoding.CodecVersion(dbBatch.CodecVersion)) if err != nil { return nil, nil, fmt.Errorf("failed to get codec from version %d, err: %w", dbBatch.CodecVersion, err) } daBatch, createErr := codec.NewDABatch(batch) if createErr != nil { return nil, nil, fmt.Errorf("failed to create DA batch: %w", createErr) } encodedChunks := make([][]byte, len(dbChunks)) for i, c := range dbChunks { daChunk, createErr := codec.NewDAChunk(chunks[i], c.TotalL1MessagesPoppedBefore) if createErr != nil { return nil, nil, fmt.Errorf("failed to create DA chunk: %w", createErr) } encodedChunks[i], err = daChunk.Encode() if err != nil { return nil, nil, fmt.Errorf("failed to encode DA chunk: %w", err) } } blobDataProof, err := daBatch.BlobDataProofForPointEvaluation() if err != nil { + lastErr = err + continue } calldata, packErr := r.l1RollupABI.Pack("commitBatchWithBlobProof", daBatch.Version(), dbParentBatch.BatchHeader, encodedChunks, daBatch.SkippedL1MessageBitmap(), blobDataProof) if packErr != nil { return nil, nil, fmt.Errorf("failed to pack commitBatchWithBlobProof: %w", packErr) } return calldata, daBatch.Blob(), nil + } + return nil, nil, fmt.Errorf("failed to generate blob data proof after %d retries, last error: %w", maxRetries, lastErr) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
common/version/version.go(1 hunks)rollup/cmd/rollup_relayer/app/app.go(0 hunks)rollup/internal/controller/relayer/l2_relayer.go(6 hunks)rollup/tests/bridge_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- rollup/cmd/rollup_relayer/app/app.go
🚧 Files skipped from review as they are similar to previous changes (2)
- common/version/version.go
- rollup/tests/bridge_test.go
🔇 Additional comments (6)
rollup/internal/controller/relayer/l2_relayer.go (6)
397-403: LGTM: Codec version consistency check is well implemented.
The validation ensures that all chunks in a batch use the same codec version, which is crucial for maintaining data consistency.
426-429: LGTM: Backward compatibility check is properly implemented.
The check prevents codec version regression by ensuring that parent batches don't have a higher codec version than their children.
435-440: LGTM: Clean codec V4 integration.
The switch case cleanly handles the codec V4 payload construction while maintaining extensibility for future versions.
562-594: LGTM: Bundle finalization checks are comprehensive.
The implementation properly validates codec version consistency between bundles and their constituent batches.
630-632: Update error message to reference CodecV4.
The error message still references "codecv3" but should be updated to "CodecV4" for consistency.
Line range hint 878-901: LGTM: Clean separation of proof and non-proof finalization paths.
The implementation cleanly handles both proof and non-proof finalization scenarios with proper error handling.
colinlyguo
force-pushed
the
feat-rollup-relayer-remove-legacy-logic
branch
from
9290dee
to
a4e06a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
rollup/internal/controller/watcher/chunk_proposer_test.go (3)
Line range hint
19-167: Consider improving test case readability and maintainabilityThe test cases are comprehensive and cover important edge cases. However, consider these improvements:
- Extract magic numbers into named constants (e.g.,
50000000000,1000000)- Add more descriptive comments for each test case explaining the specific scenario being tested
- Consider moving chain configuration setup to a helper function as it's likely to be reused
Example refactor for better readability:
+const ( + defaultMaxGas = 50000000000 + defaultMaxCalldata = 1000000 + defaultMaxRows = 1000000 + defaultTimeout = 1000000000000 +) + +// setupChainConfig returns a chain configuration for codec v4 tests +func setupChainConfig() *params.ChainConfig { + return ¶ms.ChainConfig{ + LondonBlock: big.NewInt(0), + BernoulliBlock: big.NewInt(0), + CurieBlock: big.NewInt(0), + DarwinTime: new(uint64), + DarwinV2Time: new(uint64), + } +} tests := []struct { name string maxBlockNum uint64 maxTxNum uint64 maxL1CommitGas uint64 maxL1CommitCalldataSize uint64 maxRowConsumption uint64 chunkTimeoutSec uint64 expectedChunksLen int expectedBlocksInFirstChunk int }{ + // Test case: Verifies that when all limits are set to high values, + // no chunks are created as limits aren't reached { name: "NoLimitReached", maxBlockNum: 100, maxTxNum: 10000, - maxL1CommitGas: 50000000000, - maxL1CommitCalldataSize: 1000000, - maxRowConsumption: 1000000, - chunkTimeoutSec: 1000000000000, + maxL1CommitGas: defaultMaxGas, + maxL1CommitCalldataSize: defaultMaxCalldata, + maxRowConsumption: defaultMaxRows, + chunkTimeoutSec: defaultTimeout, expectedChunksLen: 0, },
Line range hint
189-243: Simplify test structure and improve maintainabilityThe test function has several areas that could be improved:
- The codec version loop is unnecessary since only CodecV4 is supported
- Magic numbers (510, 255, 2000) should be documented or extracted as constants
- Chain configuration setup is duplicated
- Contains redundant version checks that always pass
Consider this refactor:
-func testChunkProposerBlobSizeLimitCodecV4(t *testing.T) { - codecVersions := []encoding.CodecVersion{encoding.CodecV4} - for _, codecVersion := range codecVersions { +func testChunkProposerBlobSizeLimitCodecV4(t *testing.T) { + const ( + initialBlockCount = 510 // Number of blocks to initialize + maxBlocksPerChunk = 255 // Maximum blocks allowed per chunk + totalBlocksLimit = 2000 // Maximum total blocks to process + ) + db := setupDB(t) block := readBlockFromJSON(t, "../../../testdata/blockTrace_03.json") - for i := int64(0); i < 510; i++ { + for i := int64(0); i < initialBlockCount; i++ { l2BlockOrm := orm.NewL2Block(db) block.Header.Number = big.NewInt(i + 1) err := l2BlockOrm.InsertL2Blocks(context.Background(), []*encoding.Block{block}) assert.NoError(t, err) } - var chainConfig *params.ChainConfig - if codecVersion == encoding.CodecV4 { - chainConfig = ¶ms.ChainConfig{LondonBlock: big.NewInt(0), BernoulliBlock: big.NewInt(0), CurieBlock: big.NewInt(0), DarwinTime: new(uint64), DarwinV2Time: new(uint64)} - } else { - assert.Fail(t, "unsupported codec version, expected CodecV4") - } + chainConfig := setupChainConfig() // Use the helper function from previous refactor cp := NewChunkProposer(context.Background(), &config.ChunkProposerConfig{ - MaxBlockNumPerChunk: 255, + MaxBlockNumPerChunk: maxBlocksPerChunk, MaxTxNumPerChunk: math.MaxUint64, MaxL1CommitGasPerChunk: math.MaxUint64, MaxL1CommitCalldataSizePerChunk: math.MaxUint64, MaxRowConsumptionPerChunk: math.MaxUint64, ChunkTimeoutSec: math.MaxUint32, GasCostIncreaseMultiplier: 1, MaxUncompressedBatchBytesSize: math.MaxUint64, - }, encoding.CodecV4, chainConfig, db, nil) + }, encoding.CodecV4, chainConfig, db, nil) for i := 0; i < 2; i++ { cp.TryProposeChunk() } chunkOrm := orm.NewChunk(db) chunks, err := chunkOrm.GetChunksGEIndex(context.Background(), 0, 0) assert.NoError(t, err) var expectedNumChunks int = 2 - var numBlocksMultiplier uint64 - if codecVersion == encoding.CodecV4 { - numBlocksMultiplier = 255 - } else { - assert.Fail(t, "unsupported codec version, expected CodecV4") - } + numBlocksMultiplier := uint64(maxBlocksPerChunk) assert.Len(t, chunks, expectedNumChunks) for i, chunk := range chunks { expected := numBlocksMultiplier * (uint64(i) + 1) - if expected > 2000 { - expected = 2000 + if expected > totalBlocksLimit { + expected = totalBlocksLimit } assert.Equal(t, expected, chunk.EndBlockNumber) } database.CloseDB(db) - } }
Let me analyze the test coverage results and generate a final response.
Add tests for untested constructors and utility functions
The test coverage analysis reveals several untested functions across the codebase, particularly:
Constructor functions:
NewChunkProposerin chunk_proposer.goNewBatchProposerin batch_proposer.goNewBundleProposerin bundle_proposer.goNewLayer1RelayerandNewLayer2Relayerin relayer packagesUtility functions:
finetuneAccessList,getGasFeeCap,getBlobGasFeeCapin estimategas.gotxsToTxsDatain l2_watcher.gotxDataToRpcTxin transaction_signer.goMetrics initialization:
initL1RelayerMetrics,initL2RelayerMetricsinitL1WatcherMetrics,initL2WatcherMetricsinitSenderMetricsWhile the existing chunk proposer tests cover the core functionality well, adding tests for these untested functions would help improve the overall patch coverage beyond the current 30.95%.
🔗 Analysis chain
Line range hint
1-243: Verify test coverage for all code pathsWhile these tests provide good coverage for chunk proposer functionality with codec v4, the PR comments indicate low patch coverage (30.95%). Consider adding tests for:
- Error cases and edge conditions
- Integration with other components that interact with the chunk proposer
- Verification of codec version checks in various areas as mentioned in PR objectives
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for related files # Find all test files in the rollup directory echo "Looking for test files:" fd -e go -e _test.go . rollup/ # Check for untested functions in related files echo -e "\nChecking for untested functions:" for file in $(fd -e go -E '*_test.go' . rollup/internal/controller/); do echo "=== $file ===" # Extract function names and check if they have corresponding tests rg -N '^func \w+' "$file" | while read -r line; do func_name=$(echo "$line" | awk '{print $2}' | cut -d'(' -f1) test_name="Test${func_name}" if ! rg -q "${test_name}" "${file%.*}_test.go" 2>/dev/null; then echo "Missing test for: $func_name" fi done doneLength of output: 16230
rollup/tests/rollup_test.go (2)
100-100: Consider using constants for configuration valuesThe proposer configurations use magic numbers which could make maintenance more difficult. Consider:
- Extracting these values into named constants
- Documenting the rationale for using
math.MaxUint64for size limitsExample refactor:
const ( // MaxBlocksPerChunk is the maximum number of blocks that can be included in a chunk MaxBlocksPerChunk = 100 // MaxTxPerChunk is the maximum number of transactions that can be included in a chunk MaxTxPerChunk = 10000 // DefaultL1CommitGas represents the default gas limit for L1 commit operations DefaultL1CommitGas = 50000000000 // DefaultCalldataSize represents the default calldata size limit DefaultCalldataSize = 1000000 // DefaultRowConsumption represents the default row consumption limit DefaultRowConsumption = 1048319 // DefaultTimeoutSec represents the default timeout in seconds DefaultTimeoutSec = 300 )Also applies to: 107-107, 112-112
194-211: Enhance assertion messages for better test diagnosticsWhile the assertions are comprehensive, adding descriptive messages would make test failures more informative.
- assert.Len(t, bundles, 1) + assert.Len(t, bundles, 1, "expected exactly one bundle to be created") - assert.NotEmpty(t, bundle.FinalizeTxHash) + assert.NotEmpty(t, bundle.FinalizeTxHash, "bundle finalize transaction hash should not be empty") - assert.Equal(t, gethTypes.ReceiptStatusSuccessful, receipt.Status) + assert.Equal(t, gethTypes.ReceiptStatusSuccessful, receipt.Status, "bundle finalization transaction should be successful") - assert.Len(t, batches, 2) + assert.Len(t, batches, 2, "bundle should contain exactly 2 batches")rollup/internal/controller/watcher/batch_proposer.go (1)
252-256: Add error logging before returnWhile the version check is correctly implemented, consider logging the error before returning to maintain consistent error logging throughout the codebase.
if codec.Version() < p.minCodecVersion { + log.Error("unsupported codec version", "actual", codec.Version(), "minimum", p.minCodecVersion) return fmt.Errorf("unsupported codec version: %v, expected at least %v", codec.Version(), p.minCodecVersion) }rollup/internal/controller/watcher/chunk_proposer.go (2)
64-64: Consider adding minCodecVersion to the log statement.The constructor's log statement should include the new
minCodecVersionparameter to aid in debugging and monitoring.log.Info("new chunk proposer", + "minCodecVersion", minCodecVersion, "maxBlockNumPerChunk", cfg.MaxBlockNumPerChunk, "maxTxNumPerChunk", cfg.MaxTxNumPerChunk,
282-284: Add error logging for codec version mismatch.While the version check is correctly placed and the error message is clear, this critical validation failure should be logged before returning the error.
if codecVersion < p.minCodecVersion { + log.Error("unsupported codec version detected", + "current", codecVersion, + "minimum", p.minCodecVersion, + "blockNumber", blocks[0].Header.Number) return fmt.Errorf("unsupported codec version: %v, expected at least %v", codecVersion, p.minCodecVersion) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
common/utils/flags.go(2 hunks)rollup/cmd/rollup_relayer/app/app.go(2 hunks)rollup/internal/controller/watcher/batch_proposer.go(4 hunks)rollup/internal/controller/watcher/batch_proposer_test.go(13 hunks)rollup/internal/controller/watcher/bundle_proposer.go(6 hunks)rollup/internal/controller/watcher/bundle_proposer_test.go(4 hunks)rollup/internal/controller/watcher/chunk_proposer.go(4 hunks)rollup/internal/controller/watcher/chunk_proposer_test.go(6 hunks)rollup/tests/process_start_test.go(2 hunks)rollup/tests/rollup_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rollup/internal/controller/watcher/batch_proposer_test.go
- rollup/internal/controller/watcher/bundle_proposer_test.go
🔇 Additional comments (17)
rollup/tests/process_start_test.go (2)
39-39: LGTM! Consistent parameter usage.
The same --min-codec-version "4" parameter is correctly added to the metrics-enabled test case, maintaining consistency with the base test case.
22-22: LGTM! Verify CLI parameter registration.
The addition of --min-codec-version "4" aligns with the PR's objective to enforce newer codec versions.
Let's verify the parameter is properly registered in the app:
✅ Verification successful
Parameter registration verified successfully
The --min-codec-version parameter is properly registered in the codebase:
- Defined in
common/utils/flags.goasMinCodecVersionFlag - It's marked as a required parameter
- Usage description: "Minimum required codec version for the chunk/batch/bundle proposers"
- The parameter is used throughout the codebase for codec version validation in chunk/batch/bundle proposers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the --min-codec-version parameter is registered in the app's CLI interface
# Search for the parameter registration in the app code
rg -A 3 "min-codec-version" "rollup/cmd/rollup_relayer"
Length of output: 53
Script:
#!/bin/bash
# Let's try a broader search for the parameter in the entire codebase
rg -A 3 "min-codec-version"
# Also search for codec version related flags/parameters
rg -A 3 "codec.*version|version.*codec"
# Look for flag definitions in the relayer app
rg -A 5 "flag\.(Int|String|Bool)Var" "rollup/cmd/rollup_relayer"
Length of output: 22080
common/utils/flags.go (2)
24-24: LGTM!
The new flag is correctly added to the RollupRelayerFlags slice, maintaining consistency with the file's structure.
94-99: Verify version enforcement and consider adding validation
The flag definition looks good, but since we're removing codec versions v0-v3, we should ensure proper version enforcement.
Let's verify the version checks across the codebase:
Consider adding validation to ensure the minimum version is at least 4 (since v0-v3 are being removed). Example:
MinCodecVersionFlag = cli.UintFlag{
Name: "min-codec-version",
Usage: "Minimum required codec version for the chunk/batch/bundle proposers",
Required: true,
+ Action: func(context *cli.Context, v uint) error {
+ if v < 4 {
+ return fmt.Errorf("min-codec-version must be >= 4, got %d", v)
+ }
+ return nil
+ },
}rollup/cmd/rollup_relayer/app/app.go (4)
11-11: LGTM!
The addition of the da-codec/encoding import is necessary for the codec version handling.
88-91: LGTM! Consistent application of codec version checks.
The addition of minCodecVersion parameter to all proposers aligns with the PR objective to introduce codec version checks and remove outdated codec logic.
Line range hint 115-115: Verify the impact of removing ProcessCommittedBatches loop.
The removal of the ProcessCommittedBatches loop (previously running every 15 seconds) is a significant change. Let's verify that this functionality is either no longer needed or has been replaced.
#!/bin/bash
# Check for any remaining references to ProcessCommittedBatches
rg "ProcessCommittedBatches"
# Look for related commit/finalize batch processing
rg "Process.*Batch|Commit.*Batch|Finalize.*Batch" --type go88-88: Consider adding codec version validation.
The minCodecVersion is directly used without validation. Consider adding checks to ensure the version is supported and within expected bounds.
Let's check the codec version constants and usage:
Consider adding validation like:
+if minCodecVersion < encoding.CodecV4 {
+ log.Crit("unsupported codec version", "version", minCodecVersion)
+}
chunkProposer := watcher.NewChunkProposer(subCtx, cfg.L2Config.ChunkProposerConfig, minCodecVersion, genesis.Config, db, registry)rollup/internal/controller/watcher/bundle_proposer.go (4)
32-33: LGTM: Clean struct field additions
The new fields minCodecVersion and chainCfg are well-placed and properly typed, supporting the codec version validation requirements.
45-45: Consider dynamic configuration for codec versions
While the constructor changes are technically sound, the current approach of passing minCodecVersion as a parameter might still face maintainability challenges when new codec versions are introduced. As noted in previous reviews, consider making this a dynamic configuration that can be adjusted without code changes.
Also applies to: 56-56
136-136: LGTM: Robust codec version validation
The codec version validation is well-implemented with clear error messages. The check ensures compatibility before proceeding with bundle processing.
Also applies to: 159-162
159-162: Add test coverage for codec version validation
According to the PR coverage report, this new validation logic lacks test coverage. Please add test cases to verify:
- Successful validation with supported codec version
- Error handling with unsupported codec version
rollup/tests/rollup_test.go (2)
Line range hint 55-211: LGTM! Test refactoring successfully focuses on CodecV4
The test has been well-refactored to:
- Remove support for deprecated codec versions (V0-V3)
- Maintain comprehensive test coverage for CodecV4
- Keep a clear and organized test structure
The changes align well with the PR objective of removing outdated logic.
55-66: Improve error message for unsupported codec versions
The error message could be more descriptive to help developers understand which codec versions are no longer supported.
- assert.Fail(t, "unsupported codec version, expected CodecV4")
+ assert.Fail(t, "codec versions V0-V3 are no longer supported, only CodecV4 is allowed")Let's verify the test coverage for this function:
rollup/internal/controller/watcher/batch_proposer.go (2)
37-38: LGTM: Well-structured field addition
The new minCodecVersion field is appropriately placed and typed, maintaining good struct organization.
62-62: LGTM: Constructor properly updated
The constructor signature and initialization are correctly updated to handle the new minCodecVersion parameter.
Also applies to: 82-82
rollup/internal/controller/watcher/chunk_proposer.go (1)
37-38: LGTM! Field addition is well-structured.
The new minCodecVersion field is appropriately typed and well-placed within the struct, maintaining good organization of configuration-related fields.
github-actions
bot
dismissed stale reviews from georgehao and jonastheis
via
b7428d6
Purpose or design rationale of this PR
This PR removes:
This PR also adds codec version checks in different places:
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
v4.4.77, introducing enhancements and optimizations.CodecV4across various components, enhancing robustness.MinCodecVersionFlag, to specify the minimum codec version required for operations.Bug Fixes
Tests
CodecV4, improving clarity and maintainability.CodecV4functionality, while removing outdated tests.Chores