-
Notifications
You must be signed in to change notification settings - Fork 37
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
bump: cometbft to add more invalid block checker #131
Conversation
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
🪧 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: 1
🧹 Outside diff range and nitpick comments (1)
jsonrpc/namespaces/eth/filters/api_test.go (1)
56-56
: Consider making tests more robustInstead of using fixed sleep durations, consider implementing a more robust waiting mechanism:
- Use polling with timeout for indexer readiness
- Add helper functions to wait for specific conditions
Example implementation:
+ func waitForIndexer(t *testing.T, indexer indexer.EVMIndexer, timeout time.Duration) error { + start := time.Now() + for { + if time.Since(start) > timeout { + return fmt.Errorf("timeout waiting for indexer") + } + if indexer.IsReady() { + return nil + } + time.Sleep(100 * time.Millisecond) + } + }Then replace the sleep calls with:
- time.Sleep(3 * time.Second) + require.NoError(t, waitForIndexer(t, input.indexer, 10*time.Second))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sum
is excluded by!**/*.sum
integration-tests/go.sum
is excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod
(1 hunks)integration-tests/go.mod
(1 hunks)jsonrpc/namespaces/eth/filters/api.go
(4 hunks)jsonrpc/namespaces/eth/filters/api_test.go
(1 hunks)
🔇 Additional comments (5)
jsonrpc/namespaces/eth/filters/api.go (2)
60-60
: LGTM: Well-designed channel for subscription counting.
The addition of status chan chan int
and its initialization follows the established pattern for channel-based communication in the FilterAPI struct.
Also applies to: 81-81
170-171
: LGTM: Thread-safe subscription count implementation.
The status channel handler correctly returns the subscription count while maintaining thread safety, as the subscriptions map is only accessed from the eventLoop goroutine.
integration-tests/go.mod (1)
274-274
: Verify compatibility with updated CometBFT version.
The update to CometBFT (v0.0.0-20241209060054-203460274117
) should be tested thoroughly to ensure compatibility with the new invalid block checks.
✅ Verification successful
Let me generate additional scripts to verify the CometBFT changes more thoroughly.
Based on the verification results, I can now provide a final response:
CometBFT update is consistent across the codebase
The verification shows that:
- Both main and integration-tests go.mod files use the same CometBFT version (
v0.0.0-20241209060054-203460274117
) - CometBFT usage is primarily in test files and infrastructure code (ABCI, RPC clients)
- No breaking changes in the CometBFT API usage patterns were detected
The update appears to be a coordinated change across the codebase with consistent versioning and no API incompatibilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in the CometBFT update
# Check git log for breaking changes
gh api graphql -f query='
{
repository(owner: "initia-labs", name: "cometbft") {
object(expression: "v0.0.0-20241209060054-203460274117") {
... on Commit {
history(first: 10) {
nodes {
messageHeadline
message
}
}
}
}
}
}'
# Check for API changes in CometBFT usage
rg -A 5 "cometbft" --type go
Length of output: 11835
Script:
#!/bin/bash
# Check for CometBFT version changes in both go.mod files
echo "=== Checking CometBFT versions ==="
echo "Main go.mod:"
rg "github.com/cometbft/cometbft =>" go.mod || echo "No replace directive found"
echo -e "\nIntegration tests go.mod:"
rg "github.com/cometbft/cometbft =>" integration-tests/go.mod || echo "No replace directive found"
# Check for any CometBFT API usage changes
echo -e "\n=== Checking CometBFT API usage ==="
ast-grep --pattern 'import (
$$$
"github.com/cometbft/cometbft/$_"
$$$
)'
# Check for any recent changes in files using CometBFT
echo -e "\n=== Checking recent changes in files using CometBFT ==="
for file in $(rg -l "cometbft" --type go); do
echo "=== $file ==="
git diff HEAD^1 -- "$file" 2>/dev/null || echo "No changes"
done
Length of output: 73061
go.mod (1)
291-291
: LGTM: Consistent dependency updates.
The dependency updates in both go.mod files are aligned, particularly the CometBFT version which supports the PR objective of enhancing invalid block checks.
jsonrpc/namespaces/eth/filters/api_test.go (1)
56-56
: Verify if timeout increase aligns with PR objectives
The increase in FilterTimeout from 3 to 10 seconds appears reasonable given the complex operations in the tests, but please clarify how this change relates to the PR's objective of "adding more invalid block checker".
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
=======================================
Coverage 56.93% 56.93%
=======================================
Files 110 110
Lines 10063 10063
=======================================
Hits 5729 5729
Misses 3508 3508
Partials 826 826 |
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Chores