feat: extend api to include rollup id, add Hash function to Batch, add maxBytes #13
Conversation
WalkthroughThe changes involve updates to the Changes
Assessment against linked issues
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 (
|
RollkitBot
requested review from
a team,
tuxcanfly,
tzdybal and
Manav-Aggarwal
and removed request for
a team
gupadhyaya
requested review from
MSevey,
Manav-Aggarwal and
tuxcanfly
and removed request for
tuxcanfly and
Manav-Aggarwal
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (12)
serialization.go (1)
52-59: Approve implementation with minor improvementsThe
Hashmethod is well-implemented and provides a valuable addition to theBatchstruct. It correctly uses the existingMarshalmethod for consistency and implements proper error handling. The use of SHA-256 is appropriate for generating a unique hash.To improve the code:
- Add a comment explaining the method's purpose and return values.
- Address the static analysis hint by making the comment for this exported method.
Here's a suggested improvement:
+// Hash computes and returns the SHA-256 hash of the serialized Batch. +// It returns the 32-byte hash as a byte slice, or an error if serialization fails. func (batch *Batch) Hash() ([]byte, error) { batchBytes, err := batch.Marshal() if err != nil { return nil, err } hash := sha256.Sum256(batchBytes) return hash[:], nil }This comment addresses both the need for documentation and satisfies the static analysis requirement for exported methods.
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 52-52:
exported: exported method Batch.Hash should have comment or be unexported (revive)test/dummy_test.go (5)
12-31: LGTM with a minor suggestion: Check the error returned by SubmitRollupTransaction.The test function effectively covers the basic functionality of submitting a rollup transaction. It checks for both the absence of errors and the correct state of the sequencer after submission. However, it's advisable to check the error returned by
sequencer.SubmitRollupTransaction.Consider modifying line 24 to check the error:
- resp, err := sequencer.SubmitRollupTransaction(context.Background(), req) + resp, err := sequencer.SubmitRollupTransaction(context.Background(), req) + assert.NoError(t, err)This ensures that any unexpected errors during transaction submission are caught and reported.
33-57: LGTM with suggestions: Improve error handling and consider additional assertions.The test function effectively covers the important edge case of submitting transactions with mismatched rollup IDs. However, there are a few areas for improvement:
- Check the error returned by the first
SubmitRollupTransactioncall.- Consider adding an assertion to verify that the first transaction was successfully added to the queue.
- Use
assert.ErrorIsinstead ofassert.Equalfor error comparison.Here's a suggested improvement:
req1 := sequencing.SubmitRollupTransactionRequest{ RollupId: rollupId1, Tx: tx1, } - sequencer.SubmitRollupTransaction(context.Background(), req1) + _, err := sequencer.SubmitRollupTransaction(context.Background(), req1) + assert.NoError(t, err) + assert.Equal(t, rollupId1, sequencer.RollupId) // Submit a transaction with a different rollup ID (should cause an error) rollupId2 := []byte("test_rollup_id2") tx2 := []byte("test_transaction_2") req2 := sequencing.SubmitRollupTransactionRequest{ RollupId: rollupId2, Tx: tx2, } - _, err := sequencer.SubmitRollupTransaction(context.Background(), req2) + _, err = sequencer.SubmitRollupTransaction(context.Background(), req2) // Assert that the error is ErrorRollupIdMismatch assert.Error(t, err) - assert.Equal(t, err, ErrorRollupIdMismatch) + assert.ErrorIs(t, err, ErrorRollupIdMismatch)These changes will improve error handling and make the test more robust.
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 43-43:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
59-91: LGTM with suggestions: Improve error handling and consider additional assertions.The test function effectively covers the basic functionality of retrieving the next batch after submitting a transaction. However, there are a few areas for improvement:
- Check the error returned by the
SubmitRollupTransactioncall.- Consider adding an assertion to verify the
LastBatchHashin the response.- Verify that the
MaxBytesparameter is respected in the returned batch.Here's a suggested improvement:
} - sequencer.SubmitRollupTransaction(context.Background(), req) + _, err := sequencer.SubmitRollupTransaction(context.Background(), req) + assert.NoError(t, err) // Get the next batch getBatchReq := sequencing.GetNextBatchRequest{ RollupId: rollupId, LastBatchHash: nil, MaxBytes: 1024, } - batchRes, err := sequencer.GetNextBatch(context.Background(), getBatchReq) + batchRes, err := sequencer.GetNextBatch(context.Background(), getBatchReq) assert.NoError(t, err) // Assert that the returned batch contains the correct transaction assert.NotNil(t, batchRes.Batch) assert.Equal(t, 1, len(batchRes.Batch.Transactions)) assert.Equal(t, tx, batchRes.Batch.Transactions[0]) + // Assert that LastBatchHash is set correctly + assert.NotNil(t, batchRes.LastBatchHash) + // Assert that the batch respects the MaxBytes parameter + assert.LessOrEqual(t, len(batchRes.Batch.Transactions[0]), 1024) // Assert timestamp is recent assert.WithinDuration(t, time.Now(), batchRes.Timestamp, time.Second)These changes will improve error handling and make the test more comprehensive.
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 71-71:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
93-130: LGTM with suggestions: Improve error handling and consider additional test cases.The test function effectively covers the full flow from submitting a transaction to verifying a batch. However, there are a few areas for improvement:
- Check the error returned by the
SubmitRollupTransactioncall.- Consider adding a negative test case where verification fails.
- Verify that the
BatchHashin the verification request matches the one returned byGetNextBatch.Here's a suggested improvement:
} - sequencer.SubmitRollupTransaction(context.Background(), req) + _, err := sequencer.SubmitRollupTransaction(context.Background(), req) + assert.NoError(t, err) // Get the next batch to generate batch hash getBatchReq := sequencing.GetNextBatchRequest{ RollupId: rollupId, LastBatchHash: nil, MaxBytes: 1024, } batchRes, err := sequencer.GetNextBatch(context.Background(), getBatchReq) assert.NoError(t, err) batchHash, err := batchRes.Batch.Hash() assert.NoError(t, err) + + // Verify that the BatchHash matches the one from GetNextBatch + assert.Equal(t, batchRes.LastBatchHash, batchHash) + // Verify the batch verifyReq := sequencing.VerifyBatchRequest{ RollupId: rollupId, BatchHash: batchHash, // hash of the submitted transaction } verifyRes, err := sequencer.VerifyBatch(context.Background(), verifyReq) // Assert no error assert.NoError(t, err) // Assert that the batch was verified successfully assert.True(t, verifyRes.Status) + + // Add a negative test case + invalidBatchHash := []byte("invalid_hash") + invalidVerifyReq := sequencing.VerifyBatchRequest{ + RollupId: rollupId, + BatchHash: invalidBatchHash, + } + invalidVerifyRes, err := sequencer.VerifyBatch(context.Background(), invalidVerifyReq) + assert.NoError(t, err) + assert.False(t, invalidVerifyRes.Status)These changes will improve error handling and make the test more comprehensive by including both positive and negative test cases.
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 105-105:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
1-130: Overall, well-structured and comprehensive test suite with room for enhancement.The test file provides good coverage of the main functionalities of the dummy sequencer implementation. The test functions are well-organized, follow consistent patterns, and cover important scenarios such as transaction submission, batch retrieval, and batch verification.
To further improve the test suite, consider the following suggestions:
- Add more edge cases and error scenarios, such as:
- Submitting an empty transaction
- Retrieving a batch with no transactions
- Verifying a batch with an invalid rollup ID
- Implement table-driven tests for scenarios with multiple similar test cases.
- Add benchmarks for performance-critical operations.
- Consider using a setup function to reduce code duplication across test functions.
Here's an example of how you could implement a table-driven test for transaction submission:
func TestSubmitRollupTransactionScenarios(t *testing.T) { scenarios := []struct { name string rollupId []byte tx []byte wantErr bool }{ {"Valid transaction", []byte("valid_id"), []byte("valid_tx"), false}, {"Empty transaction", []byte("valid_id"), []byte{}, false}, {"Empty rollup ID", []byte{}, []byte("valid_tx"), true}, // Add more scenarios as needed } for _, sc := range scenarios { t.Run(sc.name, func(t *testing.T) { sequencer := NewDummySequencer() req := sequencing.SubmitRollupTransactionRequest{ RollupId: sc.rollupId, Tx: sc.tx, } _, err := sequencer.SubmitRollupTransaction(context.Background(), req) if sc.wantErr { assert.Error(t, err) } else { assert.NoError(t, err) assert.Equal(t, sc.rollupId, sequencer.RollupId) } }) } }This approach would allow you to easily add and test multiple scenarios for transaction submission.
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 43-43:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
[failure] 71-71:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
[failure] 105-105:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)sequencing.go (5)
Line range hint
42-44: MissingHashfunction inBatchstructAccording to the PR objectives, a
Hashfunction should be added to theBatchstruct. However, there is noHashmethod implemented forBatch. Please implement theHashfunction to compute the hash of the batch as intended.
49-53: Add field comments toSubmitRollupTransactionRequeststructTo enhance code readability, consider adding comments for each field in the
SubmitRollupTransactionRequeststruct. This will help other developers understand the purpose of each field.
59-64: Add field comments toGetNextBatchRequeststructPlease add comments for each field in the
GetNextBatchRequeststruct, especially for the newMaxBytesparameter, to clarify their purposes.
67-70: Add field comments toGetNextBatchResponsestructTo improve maintainability, consider adding comments for each field in the
GetNextBatchResponsestruct, explaining whatBatchandTimestamprepresent.
73-76: Add field comments toVerifyBatchRequeststructIncluding comments for each field in the
VerifyBatchRequeststruct will enhance code clarity and assist future developers.test/dummy.go (1)
92-92: EnsureBatch.Transactionsis an empty slice, notnilWhen there are no transactions, the
Transactionsfield in the batch is set tonil. This could lead tonilpointer dereferences when iterating overTransactions. It's safer to initialize it as an empty slice.Update the
GetNextBatchmethod inTransactionQueue:func (tq *TransactionQueue) GetNextBatch() *sequencing.Batch { tq.mu.Lock() defer tq.mu.Unlock() size := len(tq.queue) if size == 0 { - return &sequencing.Batch{Transactions: nil} + return &sequencing.Batch{Transactions: []sequencing.Tx{}} } batch := tq.queue[:size] tq.queue = tq.queue[size:] return &sequencing.Batch{Transactions: batch} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtypes/pb/sequencing/sequencing.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
- go.mod (1 hunks)
- proto/sequencing/sequencing.proto (2 hunks)
- proxy/grpc/client.go (1 hunks)
- proxy/grpc/server.go (2 hunks)
- sequencing.go (2 hunks)
- serialization.go (2 hunks)
- test/dummy.go (1 hunks)
- test/dummy_test.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
serialization.go
[failure] 52-52:
exported: exported method Batch.Hash should have comment or be unexported (revive)test/dummy_test.go
[failure] 43-43:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
[failure] 71-71:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
[failure] 105-105:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
🔇 Additional comments (17)
go.mod (5)
7-7: LGTM: Addition of testify frameworkThe addition of
github.com/stretchr/testify v1.8.4as a direct dependency is a good choice. This popular testing toolkit will enhance the project's testing capabilities, making it easier to write and maintain tests.
12-12: Indirect dependency added: go-spewThe addition of
github.com/davecgh/go-spew v1.1.1as an indirect dependency is likely due to its use by the testify framework. This library provides improved formatting for Go data structures, which can be helpful during testing and debugging.
14-14: Indirect dependency added: go-difflibThe addition of
github.com/pmezard/go-difflib v1.0.0as an indirect dependency is expected. This library is often used by testing frameworks like testify to generate unified diffs, which are helpful for comparing expected and actual test results.
Line range hint
1-22: Summary of dependency changesThe updates to the
go.modfile primarily focus on enhancing the project's testing capabilities and potentially introducing new functionality:
- Addition of
github.com/stretchr/testifyas a direct dependency for improved testing.- Indirect dependencies (
go-spew,go-difflib, andyaml.v3) added, likely as requirements for testify or other project needs.These changes should positively impact the project's test coverage and potentially introduce new features or configuration options. Ensure that the new testing framework is utilized effectively across the project to maximize its benefits.
20-20: Verify usage of newly added YAML dependencyThe addition of
gopkg.in/yaml.v3 v3.0.1as an indirect dependency introduces YAML parsing capabilities to the project. While this is likely a requirement of another dependency, it's worth verifying if this addition aligns with any new YAML-related functionality or configuration options in the project.To ensure this dependency is being used appropriately, you can run the following command to check for YAML file usage in the project:
proto/sequencing/sequencing.proto (2)
Line range hint
1-70: Overall changes align well with PR objectives.The modifications to this protobuf file successfully implement the required changes:
- Extended the API to include
rollup_idin bothGetNextBatchRequestandVerifyBatchRequest.- Added
max_bytestoGetNextBatchRequest, addressing issue maxBytes parameter in GetNextBatch #6 and allowing the execution layer to specify a maximum size for batches.These changes will enable better control over batch sizes and improve the handling of rollup-specific data. The naming conventions used (e.g.,
max_bytes) follow protobuf best practices.
59-62: LGTM! New field added as per requirements.The changes to
VerifyBatchRequestlook good and align with the PR objectives:
- Added
rollup_idfield to extend the API.However, please note that reordering the
batch_hashfield is a breaking change that might affect existing clients. Ensure this change is communicated clearly in the changelog and to any affected parties.To verify the impact of this change, let's check for any other occurrences of
VerifyBatchRequest:#!/bin/bash # Description: Check for other occurrences of VerifyBatchRequest # Expected: No other occurrences that might be affected by the field reordering rg --type go "VerifyBatchRequest" -C 5proxy/grpc/server.go (5)
49-49: LGTM: Improved API structure for SubmitRollupTransactionThe change to use a structured
sequencing.SubmitRollupTransactionRequestobject aligns with the PR objectives and improves code clarity. This approach enhances maintainability and readability.
58-58: LGTM: Improved API structure for GetNextBatchThe changes to use structured
sequencing.GetNextBatchRequestand handle the response accordingly improve code clarity and maintainability. The timestamp handling is correct.Also applies to: 62-62, 66-66
71-71: LGTM: Improved API structure for VerifyBatchThe changes to use a structured
sequencing.VerifyBatchRequestand handle the response accordingly are consistent with the improvements made to other methods. This enhances code clarity and maintainability.Also applies to: 75-75
Line range hint
1-75: Overall assessment: Good improvements with one open questionThe changes in this file consistently improve the API structure by using structured request and response objects across all modified methods. This enhances code readability and maintainability, aligning well with the PR objectives.
However, there's an open question regarding the implementation of the
maxBytesparameter mentioned in the PR objectives. This parameter is not visible in the current implementation ofGetNextBatchor other methods in this file. Clarification on where and how this parameter is being implemented would be helpful.#!/bin/bash # Search for maxBytes implementation in the entire codebase rg --type go "maxBytes"
58-58: Query: Implementation ofmaxBytesparameterThe PR objectives mention adding a
maxBytesparameter to allow the execution layer to specify a maximum size for batches. However, this parameter is not visible in the current implementation ofGetNextBatch. Could you clarify where and how themaxBytesparameter is being implemented?test/dummy_test.go (1)
3-10: LGTM: Import statements are appropriate and well-organized.The import statements include all necessary packages for the test file and follow Go conventions for import ordering.
proxy/grpc/client.go (3)
68-68: Handle potential errors when constructing GetNextBatchResponse.When returning the
GetNextBatchResponse, ensure that all fields are correctly populated, and consider handling any potential errors in the mapping process.
72-77: VerifyBatch correctly maps the request and response fields.The
VerifyBatchfunction constructs the request withRollupIdandBatchHashand returns the response status correctly.
80-80: Confirm interface implementation with compile-time assertion.The line
var _ sequencing.Sequencer = &Client{}ensures thatClientimplements thesequencing.Sequencerinterface, providing a compile-time check.test/dummy.go (1)
74-102:⚠️ Potential issueImplement the
maxBytesparameter as per PR objectivesAccording to the PR objectives, the
GetNextBatchmethod should handle amaxBytesparameter to limit the size of the batch. Currently, themaxBytesfunctionality is not implemented.Please ensure that the
maxBytesparameter is utilized to limit the batch size. You might need to adjust theGetNextBatchmethod to considerreq.MaxByteswhen assembling the batch.For example:
func (tq *TransactionQueue) GetNextBatch(maxBytes int) *sequencing.Batch { // Existing locking code... var batch []sequencing.Tx currentSize := 0 for _, tx := range tq.queue { txSize := len(tx.Data) if currentSize+txSize > maxBytes { break } batch = append(batch, tx) currentSize += txSize } tq.queue = tq.queue[len(batch):] return &sequencing.Batch{Transactions: batch} }Update the call in
GetNextBatchmethod:batch := d.tq.GetNextBatch(int(req.MaxBytes))Ensure that you handle cases where
MaxBytesis zero or negative.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
proxy/grpc/server.go (2)
58-66: LGTM: Improved request and response handling for GetNextBatchThe changes enhance the API by using structured types for both request and response, improving encapsulation and type safety. This aligns well with the PR objectives.
A minor suggestion for improvement:
Consider combining the error checks to reduce nesting and improve readability:
resp, err := s.SequencerOutput.GetNextBatch(ctx, sequencing.GetNextBatchRequest{RollupId: req.RollupId, LastBatchHash: req.LastBatchHash}) if err != nil { return nil, err } ts, err := types.TimestampProto(resp.Timestamp) if err != nil { return nil, err } return &pbseq.GetNextBatchResponse{Batch: resp.Batch.ToProto(), Timestamp: ts}, nil
Implement
maxBytesParameter in Server MethodsThe
maxBytesparameter defined intypes/pb/sequencing/sequencing.pb.gois not implemented inproxy/grpc/server.go. To fully meet the PR objectives, please ensure thatmaxBytesis properly handled in the relevant methods, such asGetNextBatch.🔗 Analysis chain
Line range hint
1-75: Overall improvements to API structure, but missingmaxBytesimplementationThe changes in this file consistently improve the API structure by using more structured request and response types across all methods. This enhances type safety and encapsulation, which is a positive direction for the codebase.
However, I noticed that the
maxBytesparameter mentioned in the PR objectives (specifically issue #6) is not implemented in this file.To confirm this, let's check if
maxBytesis implemented elsewhere:If the
maxBytesparameter is not found, consider adding it to the relevant methods, particularlyGetNextBatch, to fulfill the PR objectives completely.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for maxBytes implementation rg --type go 'maxBytes'Length of output: 166
test/dummy_test.go (4)
12-22: Consider expanding test coverage forTestTransactionQueue_AddTransaction.While the current test covers the basic functionality, consider adding the following scenarios to improve test coverage:
- Test adding multiple transactions to ensure they are stored correctly.
- Test adding an empty transaction to verify error handling.
- Test adding transactions up to and beyond the queue's capacity (if applicable).
These additional test cases will help ensure the robustness of the
AddTransactionmethod.
24-42: LGTM! Consider adding edge case tests.The test covers the main functionality well, including multiple transactions and queue emptiness. To further improve it, consider adding:
- A test case for
GetNextBatchon an empty queue.- A test with a large number of transactions to ensure scalability.
These additions will help cover edge cases and ensure the robustness of the
GetNextBatchmethod.
155-167: LGTM! Consider adding a test with a valid hash format.The test correctly verifies the behavior for a non-existent batch. To further improve it, consider adding a test case with a valid hash format that doesn't correspond to any existing batch. This could be done by:
- Submitting and retrieving a valid batch to get a real hash format.
- Modifying one byte of the valid hash.
- Attempting to verify the batch with this modified hash.
This addition will ensure that the verification process correctly handles cases where the hash format is valid but doesn't correspond to any existing batch.
1-167: Overall good test coverage, but consider adding tests formaxBytes.The test suite provides comprehensive coverage for the new functionality, including rollup ID handling and batch processing. However, to fully align with the PR objectives, consider adding tests for the
maxBytesparameter mentioned in the PR summary. Specifically:
- Add a test case in
TestDummySequencer_GetNextBatchthat specifies amaxBytesvalue and verifies that the returned batch does not exceed this size.- Include a test where the
maxByteslimit is smaller than a single transaction to ensure proper handling of edge cases.These additions will ensure that all aspects of the PR objectives are thoroughly tested.
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 78-78:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
[failure] 108-108:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
[failure] 132-132:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtypes/pb/sequencing/sequencing.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
- go.mod (1 hunks)
- proto/sequencing/sequencing.proto (2 hunks)
- proxy/grpc/client.go (1 hunks)
- proxy/grpc/server.go (2 hunks)
- sequencing.go (2 hunks)
- serialization.go (2 hunks)
- test/dummy.go (1 hunks)
- test/dummy_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- proto/sequencing/sequencing.proto
- sequencing.go
- serialization.go
- test/dummy.go
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
test/dummy_test.go
[failure] 78-78:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
[failure] 108-108:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
[failure] 132-132:
Error return value ofsequencer.SubmitRollupTransactionis not checked (errcheck)
🔇 Additional comments (5)
proxy/grpc/server.go (2)
49-49: LGTM: Improved request structure for SubmitRollupTransactionThe change to use
sequencing.SubmitRollupTransactionRequeststruct enhances the API by improving encapsulation and type safety. This aligns well with the PR objectives and the overall direction of the changes.
71-75: LGTM: Improved request and response handling for VerifyBatchThe changes enhance the API by using structured types for both request and response, improving encapsulation and type safety. This aligns well with the PR objectives and maintains consistency with the other method changes in this file.
proxy/grpc/client.go (3)
57-68: GetNextBatch method updated appropriatelyThe
GetNextBatchmethod has been correctly refactored to use the newGetNextBatchRequestandGetNextBatchResponsetypes. The request is properly constructed withRollupIdandLastBatchHash, and the response is appropriately mapped to the sequencing types. Error handling is correctly implemented.
72-77: VerifyBatch method implemented correctlyThe
VerifyBatchmethod now utilizes the new request and response structs, aligning with the updated API design. The request is accurately populated withRollupIdandBatchHash, and the response status is correctly returned. Error handling is properly addressed.
80-80: Assertion of interface complianceThe addition of
var _ sequencing.Sequencer = &Client{}is a good practice. This compile-time assertion ensures thatClientsatisfies thesequencing.Sequencerinterface, helping to catch any deviations early in the development process.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- test/dummy.go (2 hunks)
- test/dummy_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/dummy_test.go
🔇 Additional comments (3)
test/dummy.go (3)
34-40: FunctiontotalBytesis correctly implemented.The
totalBytesfunction accurately calculates the total number of bytes in the provided data slices.
72-77: Proper synchronization for shared resources is implemented.The addition of mutexes
lastBatchHashMutexandseenBatchesMutexensures thread-safe access to shared resources inDummySequencer.
120-122: Potential issue with using raw bytes as map keys inseenBatches.Storing
[]bytekeys as strings may lead to incorrect behavior if the byte slices contain arbitrary data that doesn't represent valid UTF-8 strings.Refer to the previous review comment regarding consistent key formatting in
seenBatches.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/dummy.go (2)
15-15: Add a comment for the exported error variableThe exported variable
ErrInvalidRollupIdshould have a comment explaining its purpose. This will improve code documentation and satisfy the linter requirement.Consider adding a comment like this:
// ErrInvalidRollupId is returned when an invalid rollup ID is provided var ErrInvalidRollupId = errors.New("invalid rollup id")🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 15-15:
exported: exported var ErrInvalidRollupId should have comment or be unexported (revive)
98-134: Improved GetNextBatch method with suggestionThe
GetNextBatchmethod has been significantly improved:
- It now accepts a
GetNextBatchRequeststruct, enhancing API consistency.- Rollup ID validation has been added, improving security.
- The method now uses
batch.Hash(), which is more maintainable.These changes are approved. However, the lastBatchHash comparison logic could be simplified:
Consider simplifying the lastBatchHash comparison logic:
if !bytes.Equal(lastBatchHash, req.LastBatchHash) { return nil, errors.New("provided LastBatchHash does not match the sequencer's last batch hash") }This change would make the code more concise while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- test/dummy.go (2 hunks)
- test/dummy_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/dummy_test.go
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
test/dummy.go
[failure] 15-15:
exported: exported var ErrInvalidRollupId should have comment or be unexported (revive)
🔇 Additional comments (6)
test/dummy.go (6)
46-74: Improved batch size handlingThe updated
GetNextBatchmethod now correctly handles themaxBytesparameter, ensuring that the returned batch does not exceed the specified maximum size. The implementation also properly handles edge cases, such as when no transactions can fit within themaxByteslimit.Great job on implementing this feature! It addresses the objective of introducing a
maxBytesparameter as mentioned in the linked issue #6.
79-85: Improved thread safety and rollup ID handlingThe
DummySequencerstruct has been updated with better thread safety measures and explicit rollup ID handling:
- The
rollupIdfield allows for proper validation of incoming requests.- The
lastBatchHashMutexis now more clearly named.- The new
seenBatchesMutexprevents potential race conditions when accessing theseenBatchesmap.These changes enhance the overall robustness and correctness of the
DummySequencerimplementation.
89-94: Improved SubmitRollupTransaction methodThe
SubmitRollupTransactionmethod has been updated to:
- Accept a
SubmitRollupTransactionRequeststruct, improving API consistency.- Use the
isValidmethod for rollup ID validation, enhancing code reusability.- Return a
SubmitRollupTransactionResponsestruct, providing a more flexible response structure.These changes improve the method's consistency with the rest of the API and make it more robust with explicit rollup ID validation.
138-149: Improved VerifyBatch methodThe
VerifyBatchmethod has been updated with several improvements:
- It now accepts a
VerifyBatchRequeststruct, enhancing API consistency.- Rollup ID validation has been added, improving security.
- The batch verification now uses hex encoding for the map key, addressing potential issues with arbitrary bytes in the hash.
These changes significantly improve the method's robustness and consistency with the rest of the API. The use of hex encoding for the map key is particularly noteworthy as it resolves a potential issue raised in a previous review comment.
151-153: Approve addition of isValid methodThe new
isValidmethod is a good addition to theDummySequencer:
- It enhances code reusability by centralizing the rollup ID validation logic.
- The implementation is correct and straightforward, using
bytes.Equalfor comparison.This method improves the overall code structure and makes rollup ID validation more consistent across different methods.
156-161: Improved NewDummySequencer initializationThe
NewDummySequencerfunction has been updated to accept and store arollupId:
- This change allows for proper initialization of the
DummySequencerwith a specific rollup ID.- It aligns with the new rollup ID validation implemented in other methods.
This improvement ensures that each
DummySequencerinstance is associated with a specific rollup ID from its creation, enhancing the overall consistency and correctness of the implementation.
|
@gupadhyaya what do you think about keeping primitive or close aliases in the go interface, e.g.: then the grpc implementations can cast to their appropriate |
totally agree. its currently not dependent on the protobuf types. instead just wrapped types within the sequencer interface instead of primitive types. see: https://github.com/rollkit/go-sequencing/blob/max_bytes/sequencing.go#L58, these wrapper types are not protobuf types. these wrapper types will help manage future updates to params and returns better. what do you think? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- proxy/grpc/client.go (1 hunks)
🔇 Additional comments (2)
proxy/grpc/client.go (2)
80-80: LGTM: Good practice for interface compliance.The addition of this line is a good practice. It provides a compile-time check to ensure that the
Clientstruct correctly implements all methods of thesequencing.Sequencerinterface.
57-68: Verify handling of LastBatchHash.The method now uses
req.LastBatchHashdirectly without any explicit nil or empty checks. While this might be safe depending on howpbseq.GetNextBatchRequesthandles nil slices, it's worth verifying to ensure robustness.To confirm the safety of this approach, let's check the definition of
pbseq.GetNextBatchRequest:✅ Verification successful
LastBatchHash Handling Verified.
The
LastBatchHashfield inpbseq.GetNextBatchRequestis defined as a byte slice withomitempty, allowing it to be nil or empty. Protobuf serialization appropriately handles empty values by omitting the field. Given thatSequencerOutputClient.GetNextBatchaccepts this field without additional constraints, the current handling is robust and requires no further checks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the definition of pbseq.GetNextBatchRequest # Expected result: The definition should show how LastBatchHash is defined and any default values or constraints rg --type go -A 10 "type GetNextBatchRequest struct" types/pb/sequencing/Length of output: 1146
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
proxy/grpc/client.go (3)
48-56: LGTM! Consider explicit error handling.The changes to
SubmitRollupTransactionlook good. The method now uses a structured request and response, improving encapsulation and API clarity.Consider handling potential errors from the gRPC call more explicitly:
func (c *Client) SubmitRollupTransaction(ctx context.Context, req sequencing.SubmitRollupTransactionRequest) (*sequencing.SubmitRollupTransactionResponse, error) { - _, err := c.SequencerInputClient.SubmitRollupTransaction(ctx, &pbseq.SubmitRollupTransactionRequest{ + resp, err := c.SequencerInputClient.SubmitRollupTransaction(ctx, &pbseq.SubmitRollupTransactionRequest{ RollupId: req.RollupId, Data: req.Tx, }) if err != nil { return nil, err } - return &sequencing.SubmitRollupTransactionResponse{}, nil + return &sequencing.SubmitRollupTransactionResponse{ + // Map any relevant fields from resp + }, nil }This change allows for future expansion of the response structure if needed.
60-71: LGTM! Consider adding a nil check for LastBatchHash.The changes to
GetNextBatchlook good. The method now uses structured request and response types, improving encapsulation and API clarity.Consider adding a nil check for
LastBatchHashto improve robustness:func (c *Client) GetNextBatch(ctx context.Context, req sequencing.GetNextBatchRequest) (*sequencing.GetNextBatchResponse, error) { + if req.LastBatchHash == nil { + req.LastBatchHash = []byte{} + } resp, err := c.SequencerOutputClient.GetNextBatch(ctx, &pbseq.GetNextBatchRequest{RollupId: req.RollupId, LastBatchHash: req.LastBatchHash}) if err != nil { return nil, err } // ... rest of the function }This change ensures that the
LastBatchHashfield is always a valid slice, even if it's empty.
75-80: LGTM! Consider adding input validation.The changes to
VerifyBatchlook good. The method now uses structured request and response types, improving encapsulation and API clarity.Consider adding input validation as suggested in a previous review:
func (c *Client) VerifyBatch(ctx context.Context, req sequencing.VerifyBatchRequest) (*sequencing.VerifyBatchResponse, error) { + if len(req.RollupId) == 0 { + return nil, errors.New("RollupId cannot be empty") + } + if len(req.BatchHash) == 0 { + return nil, errors.New("BatchHash cannot be empty") + } resp, err := c.BatchVerifierClient.VerifyBatch(ctx, &pbseq.VerifyBatchRequest{RollupId: req.RollupId, BatchHash: req.BatchHash}) if err != nil { return nil, err } return &sequencing.VerifyBatchResponse{Status: resp.Status}, nil }This change adds basic input validation to ensure the integrity of the data being passed to the gRPC call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- proxy/grpc/client.go (1 hunks)
🔇 Additional comments (1)
proxy/grpc/client.go (1)
83-83: LGTM! Good practice to include interface assertion.The addition of the interface assertion is a good practice. It ensures at compile-time that the
Clientstruct correctly implements all methods required by thesequencing.Sequencerinterface.
|
🎉 This PR is included in version 0.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes #6 #10 #11
Summary by CodeRabbit
New Features
Batchstruct.Bug Fixes
Tests