chore: Minor cleanup #26
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request includes modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
RollkitBot
requested review from
a team,
yarikbratashchuk and
Manav-Aggarwal
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
sequencing/sequencer.go (5)
Line range hint 287-287: Consider using a cancellable context instead of context.Background().
Using context.Background() creates a context that cannot be canceled, which may prevent graceful shutdowns of the sequencer. It's better to use a cancellable context or accept a context parameter to allow for proper cancellation and cleanup.
Apply this diff to modify the context initialization:
-func NewSequencer(daAddress, daAuthToken string, daNamespace []byte, batchTime time.Duration, dbPath string) (*Sequencer, error) {
- ctx := context.Background()
+func NewSequencer(ctx context.Context, daAddress, daAuthToken string, daNamespace []byte, batchTime time.Duration, dbPath string) (*Sequencer, error) {
+ if ctx == nil {
+ ctx = context.Background()
+ }And update the calls to NewSequencer to pass the appropriate context.
Line range hint 344-344: Inconsistent use of context in batchSubmissionLoop.
The batchSubmissionLoop method accepts a context ctx, but within the function, it uses c.ctx. This inconsistency can lead to unexpected behavior if different contexts are used.
Consider using the passed-in context ctx consistently within the function:
-func (c *Sequencer) batchSubmissionLoop(ctx context.Context) {
+func (c *Sequencer) batchSubmissionLoop() {
batchTimer := time.NewTimer(0)
defer batchTimer.Stop()
for {
select {
- case <-c.ctx.Done():
+ case <-ctx.Done():
return
case <-batchTimer.C:
}
// ...
}
}Alternatively, remove the parameter ctx if it's not needed, or ensure that all uses of context within the function refer to ctx.
Line range hint 389-407: Potential inclusion of non-batch keys into seenBatches.
The LoadSeenBatchesFromDB function adds all keys from BadgerDB into c.seenBatches, which may include keys like "lastBatchHash" and transaction keys. This can lead to incorrect behavior when verifying batches.
Consider filtering the keys to include only batch hashes, possibly by using a key prefix for batch entries:
for it.Rewind(); it.Valid(); it.Next() {
item := it.Item()
key := item.Key()
+ if !bytes.HasPrefix(key, []byte("batch_")) {
+ continue
+ }
// Add the batch hash to the seenBatches map
c.seenBatches[string(key)] = struct{}{}
}This change assumes that batch entries are stored with a "batch_" prefix. Adjust the prefix as per your storage design.
Line range hint 573-574: Use bytes.Equal instead of reflect.DeepEqual for byte slice comparison.
Comparing byte slices using bytes.Equal is more efficient and idiomatic than reflect.DeepEqual.
Apply this diff to make the change:
-if !reflect.DeepEqual(lastBatchHash, req.LastBatchHash) {
+if !bytes.Equal(lastBatchHash, req.LastBatchHash) {
return nil, fmt.Errorf("batch hash mismatch: lastBatchHash = %x, req.LastBatchHash = %x", lastBatchHash, req.LastBatchHash)
}Ensure that the bytes package is imported if it's not already.
Line range hint 615-620: Implement DA verification in VerifyBatch method.
The VerifyBatch method currently has a TODO indicating that DA (Data Availability) verification needs to be added. This is crucial for ensuring the integrity of batches.
Would you like assistance in implementing DA verification? I can help draft the code necessary to verify batches against the DA layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Makefile (1 hunks)
- README.md (2 hunks)
- sequencing/sequencer.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🔇 Additional comments (1)
Makefile (1)
13-13: Verify if version information is needed in the binary.
The removal of ${LDFLAGS} aligns with the PR objective to remove unneeded flags. However, these flags are sometimes used to embed version information, git commit hashes, or build timestamps into the binary, which can be helpful for debugging in production environments.
Let's verify if version information was previously used:
✅ Verification successful
Let me gather more information about version usage in the codebase.
Removal of LDFLAGS is safe - no version information was being used
Based on the verification results:
- No version-related imports or variables are defined in the codebase
- No version flags are used in the command line arguments
- No version information was being injected through LDFLAGS
- The main package doesn't handle version information
The simplification of the build command by removing LDFLAGS aligns with the PR objectives and doesn't impact any existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if version information was previously used in the codebase
# Test 1: Look for version-related variables or imports
echo "Checking for version-related code..."
rg -i "version|buildinfo|ldflags" --type go
# Test 2: Check git history for previous LDFLAGS usage
echo "Checking git history for LDFLAGS..."
git log -p | rg "LDFLAGS"
Length of output: 313
Script:
#!/bin/bash
# Check for version-related code patterns more thoroughly
# Look for version imports and variables in Go files
echo "Checking for version package imports..."
ast-grep --pattern 'import $_ "version"'
echo "Checking for version variable declarations..."
ast-grep --pattern 'var Version string'
ast-grep --pattern 'const Version string'
# Check if there are any version-related command line flags
echo "Checking for version flag usage..."
rg -i "flag\.(String|Bool)Var.*version" --type go
# Check for version in main package
echo "Checking main package for version handling..."
rg -i "func main|version" --type go -C 5
# Check Makefile history for LDFLAGS definition
echo "Checking Makefile history..."
git log -p Makefile | rg "LDFLAGS.*="
Length of output: 1018
Overview
Remove unneeded
ldflagsfrom build command.Remove all mention of celestia. If approved we should update the repo description as well.
Summary by CodeRabbit
Documentation
Bug Fixes
Chores