refactor: use new da-codec interfaces #1068
Conversation
…eum into refactor/use-new-da-codec-interfaces
* use new da-codec interface in syncing from l1 * delete unused * nit * uncomment * typo --------- Co-authored-by: colinlyguo <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes in this pull request involve significant modifications to the handling of batch data within the codebase. Key functions related to batch chunk range management have been removed, including writing, reading, and deleting batch chunk ranges. Corresponding test functions have also been eliminated. The schema has been updated to reflect the removal of batch chunk range keys and the addition of a new key for tracking synced L1 block numbers. Furthermore, several methods in the Changes
Poem
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: 10
🧹 Outside diff range and nitpick comments (5)
rollup/da_syncer/batch_queue.go (1)
Line range hint
1-95: Overall assessment: Code change aligns with refactoring objectives.The modification to the
NextBatchmethod reflects a broader shift in batch type handling across the system. While the change itself is straightforward and localized, it's crucial to ensure that this refactoring is consistently applied throughout the codebase.Consider the following recommendations:
- Update documentation: Ensure that any documentation referring to batch types is updated to reflect this change.
- Review tests: Verify that all relevant test cases have been updated to use the new
CommitBatchWithBlobTypewhere appropriate.- Performance impact: Assess whether this consolidation of batch types has any performance implications, particularly in terms of data processing and storage.
- Migration plan: If this change affects existing data or APIs, ensure there's a clear migration plan for transitioning from the old batch types to the new one.
These considerations will help maintain consistency and prevent potential issues as the refactoring is rolled out across the system.
rollup/da_syncer/da/commitV0.go (1)
27-27: LGTM: New codec parameter enhances flexibility.The addition of the
codec encoding.Codecparameter aligns with the PR's refactoring objective and allows for more flexible handling of different encoding schemes.Consider adding a nil check for the
codecparameter to improve error handling:if codec == nil { return nil, fmt.Errorf("codec cannot be nil") }rollup/da_syncer/da/commitV1.go (3)
Line range hint
23-33: Add Nil Check for thecodecParameterThe function
NewCommitBatchDAWithBlobuses thecodecparameter without checking if it isnil. This could lead to anilpointer dereference ifcodecisnil.Consider adding a nil check at the beginning of the function:
if codec == nil { return nil, fmt.Errorf("codec is nil") }
Line range hint
49-52: Fix Logical Error in Blob Null CheckThe condition
blob == nil && err != nilin the error check is logically incorrect. Iferr != nil, it should have already returned earlier. The unexpected case is whenblob == nil && err == nil.Update the condition and error message as follows:
- if blob == nil { - return nil, fmt.Errorf("unexpected, blob == nil and err != nil, batch index: %d, versionedHash: %s, blobClient: %T", batchIndex, versionedHash.String(), blobClient) + if err == nil && blob == nil { + return nil, fmt.Errorf("unexpected nil blob with no error, batch index: %d, versionedHash: %s, blobClient: %T", batchIndex, versionedHash.String(), blobClient) }
77-77: UpdateType()Method DocumentationThe return value of the
Type()method has changed toCommitBatchWithBlobType. Ensure that any documentation or comments reflecting the return type are updated accordingly.Consider updating any relevant comments to match the new return type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
- core/rawdb/accessors_rollup_event.go (0 hunks)
- core/rawdb/accessors_rollup_event_test.go (0 hunks)
- core/rawdb/schema.go (0 hunks)
- go.mod (2 hunks)
- rollup/da_syncer/batch_queue.go (1 hunks)
- rollup/da_syncer/da/calldata_blob_source.go (3 hunks)
- rollup/da_syncer/da/commitV0.go (4 hunks)
- rollup/da_syncer/da/commitV1.go (5 hunks)
- rollup/da_syncer/da/commitV2.go (0 hunks)
- rollup/da_syncer/da/commitV4.go (0 hunks)
- rollup/da_syncer/da/da.go (1 hunks)
- rollup/rollup_sync_service/rollup_sync_service.go (11 hunks)
- rollup/rollup_sync_service/rollup_sync_service_test.go (13 hunks)
💤 Files with no reviewable changes (5)
- core/rawdb/accessors_rollup_event.go
- core/rawdb/accessors_rollup_event_test.go
- core/rawdb/schema.go
- rollup/da_syncer/da/commitV2.go
- rollup/da_syncer/da/commitV4.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🧰 Additional context used
🔇 Additional comments (16)
rollup/da_syncer/da/da.go (1)
14-15: LGTM! Consider verifying impact and updating documentation.The consolidation of multiple commit batch types into a single
CommitBatchWithBlobTypesimplifies the enum and makes it more flexible for future versions. This change looks good.To ensure this change doesn't introduce any issues, please run the following script to check for any remaining references to the old types:
Consider updating any relevant documentation or comments in other files to reflect this change in batch type representation.
✅ Verification successful
Verified: All old CommitBatchV1Type to CommitBatchV4Type references have been successfully removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to old CommitBatch types # Test: Search for old CommitBatch type references echo "Searching for old CommitBatch type references:" rg -i "CommitBatchV[1-4]Type" --type go # Test: Search for uses of the new CommitBatchWithBlobType echo "Searching for uses of the new CommitBatchWithBlobType:" rg "CommitBatchWithBlobType" --type goLength of output: 627
rollup/da_syncer/batch_queue.go (1)
44-44: LGTM. Verify impact on other components.The consolidation of batch types to include
da.CommitBatchWithBlobTypealigns with the broader changes in the system. This simplification is a positive step.To ensure this change doesn't introduce compatibility issues, please run the following verification:
Ensure that all references to the removed batch types have been updated and that the new
CommitBatchWithBlobTypeis consistently used across the codebase.✅ Verification successful
Verified. All references to removed batch types have been successfully eliminated, and the new
CommitBatchWithBlobTypeis consistently utilized across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to removed batch types and verify the introduction of the new type. # Test 1: Search for any remaining references to removed batch types echo "Searching for references to removed batch types..." rg -i "CommitBatchV1Type|CommitBatchV2Type" # Test 2: Verify the introduction and usage of the new batch type echo "Verifying usage of new CommitBatchWithBlobType..." rg -i "CommitBatchWithBlobType" # Test 3: Check for any TODO comments or FIXME related to batch types echo "Checking for TODO or FIXME comments related to batch types..." rg -i "TODO|FIXME" | rg -i "batch.*type|type.*batch"Length of output: 810
rollup/da_syncer/da/commitV0.go (7)
20-20: LGTM: Type change aligns with refactoring objective.The change from
[]*codecv0.DAChunkRawTxto[]*encoding.DAChunkRawTxis consistent with the PR's objective of refactoring to use new da-codec interfaces. This update likely improves the code's flexibility by using a more genericencodingpackage.
35-35: LGTM: Decoding logic updated to use new codec.The change to use
codec.DecodeDAChunksRawTx(chunks)for decoding is consistent with the newcodecparameter and the overall refactoring objective. The error handling remains appropriate.
47-47: LGTM: Parameter type updated consistently.The change of
decodedChunksparameter type from[]*codecv0.DAChunkRawTxto[]*encoding.DAChunkRawTxis consistent with the earlier changes and the overall refactoring objective.
103-105: LGTM: Improved encapsulation in daBlock usage.The changes to use method calls (e.g.,
daBlock.NumTransactions(),daBlock.NumL1Messages()) instead of direct field access improve encapsulation. This is a good practice and is consistently applied across all usages ofdaBlock.Also applies to: 110-110, 117-120
132-132: LGTM: Function signature updated consistently.The change of the
decodedChunksparameter type from[]*codecv0.DAChunkRawTxto[]*encoding.DAChunkRawTxin thegetTotalMessagesPoppedFromChunksfunction is consistent with earlier changes and the overall refactoring objective.
136-136: LGTM: Consistent method call update.The change to use
block.NumL1Messages()instead of direct field access is consistent with earlier changes and improves encapsulation.
Line range hint
1-172: Overall assessment: Well-executed refactoring with improved design.The changes in this file consistently implement the refactoring to use new da-codec interfaces. Key improvements include:
- Transition from
codecv0to a more genericencodingpackage.- Introduction of a
codecparameter for flexible encoding handling.- Enhanced encapsulation through the use of method calls instead of direct field access.
These changes align well with the PR objectives and contribute to a more flexible and maintainable codebase. The refactoring has been applied consistently throughout the file, maintaining the existing functionality while improving the overall design.
rollup/da_syncer/da/commitV1.go (1)
Line range hint
23-33: Verify All Callers of Renamed Function Are UpdatedThe function
NewCommitBatchDAV1has been renamed toNewCommitBatchDAWithBlob. Ensure that all references toNewCommitBatchDAV1in the codebase are updated to prevent undefined function errors.Run the following script to find any remaining references to the old function name:
rollup/da_syncer/da/calldata_blob_source.go (1)
8-8: Addition of 'encoding' package import is appropriateThe import of the
encodingpackage is necessary for the codec functionality introduced in the code.rollup/rollup_sync_service/rollup_sync_service.go (3)
218-221: Code logic for handling committed batch metadata looks goodThe error handling and writing of
committedBatchMetaare correctly implemented.
Line range hint
355-407: Validation and error handling ingetCommittedBatchMetaare appropriateThe function correctly handles potential errors and provides informative messages.
583-604: Proper error handling when decoding block ranges from encoded chunksThe function correctly handles errors and validates input data.
rollup/rollup_sync_service/rollup_sync_service_test.go (2)
568-571: Consistent initialization ofcommittedBatchMetaEnsure that the
committedBatchMetainitialization is consistent across different codec versions. Currently,BlobVersionedHashesis set tonil. If this is intentional forCodecV0, please confirm that downstream code can handle anilvalue without errors.
739-751: Ensure correct handling ofBlobVersionedHashesforCodecV3In the
committedBatchMeta1initialization forCodecV3, verify that theBlobVersionedHashesslice contains the correct hash values corresponding to the blobs used. Incorrect hashes may lead to validation failures.
1. Purpose or design rationale of this PR
2. 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:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Release Notes
New Features
CalldataBlobSource.Bug Fixes
getCommittedBatchMetamethod to improve clarity and reduce complexity.Refactor
BatchQueue.RollupSyncService, removing unnecessary complexity.Chores