chore: remove duplicate package import#3331
Conversation
📝 WalkthroughWalkthroughAdds an HTTP/2 configuration helper and applies it across RPC server/service handlers, examples, tests, and node startup; consolidates an RPC client import in an E2E test to use ChangesRPC HTTP/2 configuration and callers
RPC client import consolidation
Docs, markdownlint, and formatting updates
Preallocation and slice initialization tweaks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/failover_e2e_test.go (1)
881-889:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: variable
clientshadows theclientpackage, breaking the build.On line 882, the local variable
client(an*ethclient.Client) shadows the importedgithub.com/evstack/ev-node/pkg/rpc/clientpackage. Line 886 then callsclient.NewClient(d.rpcAddr)which resolves to a method call on the local*ethclient.Clientvariable rather than the package —*ethclient.Clienthas noNewClientmethod, so this will fail to compile. The previousrpcclientalias is exactly what avoided this collision.🛠️ Proposed fix: rename the local variable to avoid the shadow
d.extClientOnce.Do(func() { - client, err := ethclient.Dial(d.ethAddr) + ethClient, err := ethclient.Dial(d.ethAddr) require.NoError(t, err) - d.xEthClient.Store(client) - t.Cleanup(client.Close) + d.xEthClient.Store(ethClient) + t.Cleanup(ethClient.Close) rpcClient := client.NewClient(d.rpcAddr) require.NotNil(t, rpcClient) d.xRPCClient.Store(rpcClient) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/failover_e2e_test.go` around lines 881 - 889, The local variable named `client` in the extClientOnce.Do closure shadows the imported `client` package and causes the call to `client.NewClient(d.rpcAddr)` to resolve incorrectly; rename the local variable (e.g., to `ethClient`) returned by `ethclient.Dial(d.ethAddr)` and update subsequent uses (`d.xEthClient.Store`, `t.Cleanup(ethClient.Close)`) so the package call uses the imported package name (e.g., `rpcclient.NewClient(d.rpcAddr)`) instead of the shadowed identifier; ensure `rpcClient` stays the result stored by `d.xRPCClient.Store`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/e2e/failover_e2e_test.go`:
- Around line 881-889: The local variable named `client` in the extClientOnce.Do
closure shadows the imported `client` package and causes the call to
`client.NewClient(d.rpcAddr)` to resolve incorrectly; rename the local variable
(e.g., to `ethClient`) returned by `ethclient.Dial(d.ethAddr)` and update
subsequent uses (`d.xEthClient.Store`, `t.Cleanup(ethClient.Close)`) so the
package call uses the imported package name (e.g.,
`rpcclient.NewClient(d.rpcAddr)`) instead of the shadowed identifier; ensure
`rpcClient` stays the result stored by `d.xRPCClient.Store`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2c464f7-2a6b-4a06-afd3-513e26570081
📒 Files selected for processing (1)
test/e2e/failover_e2e_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3331 +/- ##
==========================================
- Coverage 60.86% 60.72% -0.14%
==========================================
Files 127 127
Lines 13762 13781 +19
==========================================
- Hits 8376 8369 -7
- Misses 4474 4498 +24
- Partials 912 914 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
could you fix the lint errors? |
Head branch was pushed to by a user without write access
@julienrbrt Modified. Please review it again. Thanks! |
|
It is still failing. Can you run |
Signed-off-by: caltechustc <caltechustc@outlook.com>
In fact, the CI failure isn’t related to my initial changes. However, in this PR I also fixed it, and after running just lint-fix, there are no errors. 
@julienrbrt Please review it again. Thanks! |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/guides/ha/cluster-setup.md (2)
454-454: ⚡ Quick winRemove unnecessary blank line after list introduction.
Standard markdown formatting does not include a blank line between a list introduction ("Causes:") and its items. This is inconsistent with markdown conventions and may violate markdownlint rules.
📝 Proposed fix
Causes: - - `heartbeat_timeout` is too short for your network RTT — increase it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/ha/cluster-setup.md` at line 454, Remove the extraneous blank line immediately after the list introduction line "Causes:" so the bullet items follow directly on the next line (i.e., change the pattern "Causes:" + blank line + "- item" to "Causes:" + "- item"); update the section in cluster-setup.md where the "Causes:" heading appears to comply with standard markdown/list formatting.
433-433: ⚡ Quick winRemove unnecessary blank line after list introduction.
Standard markdown formatting does not include a blank line between a list introduction ("Check that:") and its items. This may violate markdownlint rules (MD032) and is inconsistent with markdown conventions.
📝 Proposed fix
Check that: - - At least 3 out of 5 nodes are running and can reach each other on port 5001.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/ha/cluster-setup.md` at line 433, Remove the extra blank line that follows the list introduction "Check that:" in docs/guides/ha/cluster-setup.md so the list items immediately follow that line (i.e., change the block where "Check that:" is followed by a blank line and then list items to have no blank line between the introduction and the first list item) to satisfy markdownlint MD032 and standard markdown list formatting.docs/learn/config.md (1)
1361-1361: 💤 Low valueConsider improving adverb placement for better flow.
Moving "already" closer to the verb ("has already persisted" instead of "already has persisted") creates more natural English flow, though the current phrasing is grammatically correct.
📝 Suggested refinement
-- If the node already has persisted raft configuration in `raft.raft_dir`, it starts in rejoin mode. +- If the node has already persisted raft configuration in `raft.raft_dir`, it starts in rejoin mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/learn/config.md` at line 1361, Update the sentence that reads "If the node already has persisted raft configuration in `raft.raft_dir`, it starts in rejoin mode." to move "already" next to the verb for smoother flow; change it to "If the node has already persisted raft configuration in `raft.raft_dir`, it starts in rejoin mode." Ensure the exact identifier `raft.raft_dir` remains unchanged and only reposition the adverb.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/guides/ha/cluster-setup.md`:
- Line 454: Remove the extraneous blank line immediately after the list
introduction line "Causes:" so the bullet items follow directly on the next line
(i.e., change the pattern "Causes:" + blank line + "- item" to "Causes:" + "-
item"); update the section in cluster-setup.md where the "Causes:" heading
appears to comply with standard markdown/list formatting.
- Line 433: Remove the extra blank line that follows the list introduction
"Check that:" in docs/guides/ha/cluster-setup.md so the list items immediately
follow that line (i.e., change the block where "Check that:" is followed by a
blank line and then list items to have no blank line between the introduction
and the first list item) to satisfy markdownlint MD032 and standard markdown
list formatting.
In `@docs/learn/config.md`:
- Line 1361: Update the sentence that reads "If the node already has persisted
raft configuration in `raft.raft_dir`, it starts in rejoin mode." to move
"already" next to the verb for smoother flow; change it to "If the node has
already persisted raft configuration in `raft.raft_dir`, it starts in rejoin
mode." Ensure the exact identifier `raft.raft_dir` remains unchanged and only
reposition the adverb.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74f6e280-c41f-42c0-947c-5f22dd6a5767
📒 Files selected for processing (27)
.markdownlint.yamlCLAUDE.mdREADME.mdRELEASE.mdapps/evm/README.mdblock/internal/syncing/da_retriever.godocs/adr/adr-009-state-fraud-proofs.mddocs/adr/adr-012-based-sequencing.mddocs/adr/adr-019-forced-inclusion-mechanism.mddocs/guides/da/visualizer.mddocs/guides/deploy/mainnet.mddocs/guides/deploy/overview.mddocs/guides/deploy/testnet.mddocs/guides/ha/cluster-setup.mddocs/guides/ha/overview.mddocs/guides/quick-start.mddocs/learn/config.mdnode/failover.gonode/light.gopkg/p2p/client.gopkg/rpc/client/client_test.gopkg/rpc/example/example.gopkg/rpc/server/http.gopkg/rpc/server/server.gopkg/sync/sync_service.gotest/e2e/failover_e2e_test.gotools/blob-decoder/README.md
💤 Files with no reviewable changes (1)
- README.md
✅ Files skipped from review due to trivial changes (14)
- docs/guides/deploy/overview.md
- docs/guides/deploy/testnet.md
- docs/guides/quick-start.md
- tools/blob-decoder/README.md
- CLAUDE.md
- docs/guides/da/visualizer.md
- docs/guides/deploy/mainnet.md
- docs/adr/adr-012-based-sequencing.md
- docs/adr/adr-009-state-fraud-proofs.md
- pkg/p2p/client.go
- docs/guides/ha/overview.md
- apps/evm/README.md
- RELEASE.md
- docs/adr/adr-019-forced-inclusion-mechanism.md
2 participants
Overview
remove duplicate package import
Summary by CodeRabbit