refactor: bump to ev-node v1.1.0 and disable base sequencing#390
refactor: bump to ev-node v1.1.0 and disable base sequencing#390julienrbrt merged 3 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR updates Go toolchain versions from 1.25 to 1.26 across CI/CD workflows and Dockerfile, upgrades EVNODE dependency to v1.1.0 in multiple configurations, updates numerous Go module dependencies in go.mod, adds context parameter to signer interface methods, and removes based sequencer implementation from server initialization logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/start.go (1)
549-551:⚠️ Potential issue | 🟡 MinorUpdate comments and error text to match the new behavior.
Line 550-551 says based sequencer is created when enabled, but Line 561 now always returns an error. The message also has a typo (
constaints) and inconsistent wording (basevsbased).Proposed patch
-// If BasedSequencer is enabled, it creates a based sequencer that fetches transactions from DA. -// Otherwise, it creates a single (traditional) sequencer. +// If BasedSequencer is enabled, return an error because based sequencing is unsupported in ev-abci. +// Otherwise, create a single (traditional) sequencer. func createSequencer( @@ if nodeConfig.Node.BasedSequencer { - return nil, fmt.Errorf("base sequencing is not supported by ev-abci due to IBC constaints") + return nil, fmt.Errorf("based sequencing is not supported by ev-abci due to IBC constraints") }Also applies to: 560-562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/start.go` around lines 549 - 551, The comment block and final error text in createSequencer are out-of-sync with the implementation: update the comment above createSequencer and the error returned by createSequencer/related branch to reflect that the "based" sequencer path is currently unsupported (or always returns an error), correct the typo "constaints" to "constraints", and use consistent term "based" everywhere (not "base"); make the error message clear (e.g., "based sequencer not supported: requires DA constraints" or similar) and apply the same wording fixes to the other error location that mirrors lines 560-562 so comments and returned errors consistently describe current behavior.
🧹 Nitpick comments (1)
server/start.go (1)
560-562: Consider fail-fast validation in config validation path.This unsupported-mode check currently happens late (during node setup). Since
server/init_cmd.go:28-42already callscfg.Validate(), adding the same rule there would fail earlier and avoid unnecessary initialization work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/start.go` around lines 560 - 562, Add a fail-fast validation for the unsupported base-sequencer mode by moving the check into the config validation path: update cfg.Validate() (the validation called from server/init_cmd.go) to inspect cfg.Node.BasedSequencer and return an error like "base sequencing is not supported by ev-abci due to IBC constraints" when true; keep or remove the duplicate check in start.go (nodeConfig.Node.BasedSequencer) as desired, but ensure the same validation logic exists in cfg.Validate() so startup fails earlier during init_cmd.go's cfg.Validate() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/docker/Dockerfile.gm`:
- Line 8: The Dockerfile builder stage uses Go 1.25 while go.mod requires Go
1.26.2; update the builder image FROM line in the Dockerfile to use
golang:1.26.2 (or the exact 1.26.2 tag) so the container Go version matches the
go.mod requirement and GitHub workflows. Locate the builder stage FROM ...
golang:1.25 (or any explicit 1.25 reference) and change it to golang:1.26.2, and
if there are ARGs or version variables related to Go, update them to 1.26.2 as
well to keep consistency.
---
Outside diff comments:
In `@server/start.go`:
- Around line 549-551: The comment block and final error text in createSequencer
are out-of-sync with the implementation: update the comment above
createSequencer and the error returned by createSequencer/related branch to
reflect that the "based" sequencer path is currently unsupported (or always
returns an error), correct the typo "constaints" to "constraints", and use
consistent term "based" everywhere (not "base"); make the error message clear
(e.g., "based sequencer not supported: requires DA constraints" or similar) and
apply the same wording fixes to the other error location that mirrors lines
560-562 so comments and returned errors consistently describe current behavior.
---
Nitpick comments:
In `@server/start.go`:
- Around line 560-562: Add a fail-fast validation for the unsupported
base-sequencer mode by moving the check into the config validation path: update
cfg.Validate() (the validation called from server/init_cmd.go) to inspect
cfg.Node.BasedSequencer and return an error like "base sequencing is not
supported by ev-abci due to IBC constraints" when true; keep or remove the
duplicate check in start.go (nodeConfig.Node.BasedSequencer) as desired, but
ensure the same validation logic exists in cfg.Validate() so startup fails
earlier during init_cmd.go's cfg.Validate() call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d21ea27b-7dad-441f-b1cb-b2795dbb9056
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.github/workflows/integration_test.yml.github/workflows/lint.yml.github/workflows/migration_test.yml.github/workflows/test.ymlDockerfilego.modpkg/rpc/core/blocks_test.gopkg/signer/signer.goserver/start.gotests/integration/docker/Dockerfile.gm
2 participants
Bump to ev-node v1.1.0 and disable base sequencing
Summary by CodeRabbit
Release Notes
Deprecated Features
Chores