-
Notifications
You must be signed in to change notification settings - Fork 10
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
refactor(l2geth dependency): downgrade to l2geth latest develop #32
Conversation
WalkthroughThe changes in this pull request involve modifications to several files related to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
encoding/codecv0_test.go (2)
Line range hint
15-297
: Consider refactoring long test functions into table-driven testsWhile the test coverage is comprehensive, some test functions (like
TestCodecV0BlockEncode
andTestCodecV0ChunkEncode
) are quite long and contain repetitive patterns. Consider refactoring these into table-driven tests to improve maintainability and make it easier to add new test cases.Example refactor for
TestCodecV0BlockEncode
:-func TestCodecV0BlockEncode(t *testing.T) { +func TestCodecV0BlockEncode(t *testing.T) { + tests := []struct { + name string + jsonFile string + want string + }{ + { + name: "empty block", + want: "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + }, + { + name: "block 02", + jsonFile: "testdata/blockTrace_02.json", + want: "00000000000000020000000063807b2a0000000000000000000000000000000000000000000000000000000000001de9000355418d1e818400020000", + }, + // Add more test cases... + } + + codecv0, err := CodecFromVersion(CodecV0) + require.NoError(t, err) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var block DABlock + if tt.jsonFile == "" { + block = &daBlockV0{} + } else { + b := readBlockFromJSON(t, tt.jsonFile) + block, err = codecv0.NewDABlock(b, 0) + require.NoError(t, err) + } + encoded := hex.EncodeToString(block.Encode()) + assert.Equal(t, tt.want, encoded) + }) + } +}
Line range hint
298-595
: Add validation for test data formatWhile the test data organization is good, consider adding validation for the test data format to ensure the JSON files contain the expected structure. This would help catch issues with malformed test data early.
+func validateBlockJSON(t *testing.T, data []byte) error { + var block struct { + Number string `json:"number"` + Timestamp string `json:"timestamp"` + Hash string `json:"hash"` + // Add other expected fields + } + if err := json.Unmarshal(data, &block); err != nil { + return fmt.Errorf("invalid block JSON format: %v", err) + } + return nil +} func readBlockFromJSON(t *testing.T, path string) *Block { data, err := os.ReadFile(path) require.NoError(t, err) + require.NoError(t, validateBlockJSON(t, data)) // ... rest of the function }encoding/codecv1_test.go (1)
Line range hint
1890-2130
: Consider adding test cases for invalid inputs inTestCodecV1BatchStandardTestCases
The function
TestCodecV1BatchStandardTestCases
robustly tests various standard scenarios, including empty and non-empty chunks, maximum chunk limits, and full blobs. To further strengthen the test suite, consider adding test cases for invalid or edge-case inputs, such as:
- Chunks containing data that exceed maximum allowed sizes.
- Batches with an excessive number of chunks beyond the limit.
- Chunks with malformed or corrupted data.
Including these tests will ensure that the codec handles erroneous inputs gracefully and maintains stability under all conditions.
encoding/codecv4_test.go (4)
Line range hint
111-115
: Ensure error handling for nil inputs inTestCodecV4BatchCompressedDataCompatibilityCheck
Consider adding test cases to handle
nil
or invalid batch inputs to verify that the function behaves correctly under edge conditions.
Line range hint
169-173
: Expand test coverage inTestCodecV4ChunkCompressedDataCompatibilityCheck
Include additional test cases with mixed transaction types and malformed data to thoroughly test the chunk compatibility logic.
Line range hint
227-230
: Clarify purpose inTestDACodecV4SimpleMethods
Adding comments or descriptive names for test cases can improve the readability and maintainability of the tests for simple methods.
Line range hint
232-488
: Improve readability by splittingTestCodecV4BatchStandardTestCasesEnableCompression
This test function covers multiple scenarios with extensive data. Splitting it into smaller, focused test functions can enhance readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
encoding/codecv0_test.go
(1 hunks)encoding/codecv1_test.go
(1 hunks)encoding/codecv2_test.go
(1 hunks)encoding/codecv3_test.go
(1 hunks)encoding/codecv4_test.go
(1 hunks)encoding/interfaces_test.go
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- encoding/interfaces_test.go
🔇 Additional comments (8)
encoding/codecv0_test.go (1)
7-8
: Good addition of testify imports!
The addition of testify/assert
and testify/require
packages improves test reliability by providing better assertion handling. require
is correctly used for critical checks that should stop test execution, while assert
is used for non-critical comparisons.
encoding/codecv1_test.go (2)
11-12
: LGTM!
The import statements for assert
and require
from the testify
library are correctly added and will assist in enhancing the test assertions.
Line range hint 2132-2145
: LGTM!
The simple methods Version
and EstimateBlockL1CommitCalldataSize
are appropriately tested in TestDACodecV1SimpleMethods
. The tests effectively confirm the correct versioning and calldata size estimations.
encoding/codecv2_test.go (1)
11-12
: Good use of testing libraries
The addition of github.com/stretchr/testify/assert
and github.com/stretchr/testify/require
imports enhances the readability and effectiveness of the test cases.
encoding/codecv3_test.go (1)
13-15
: Appropriate inclusion of testify assertions
Including assert
and require
from the testify
library improves test readability and ensures consistent assertion handling.
encoding/codecv4_test.go (3)
12-13
: Appropriate inclusion of testing packages
Importing the assert
and require
packages from github.com/stretchr/testify
aligns with best practices for writing concise and readable tests.
Line range hint 490-733
: Verify handling of maximum data sizes in TestCodecV4BatchStandardTestCasesDisableCompression
Ensure that the test cases accurately simulate scenarios with maximum blob sizes and that all edge cases related to data limits are appropriately handled.
Line range hint 735-746
: Good practice in TestDACodecV4SimpleMethods
The test correctly verifies the Version()
method, ensuring that the codec version is returned as expected.
Purpose or design rationale of this PR
This PR downgrades to l2geth latest develop and fixes all incompatibilities.
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:
Breaking change label
Does this PR have the
breaking-change
label?Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores