Skip to content

Conversation

@julienrbrt
Copy link
Member

we decided to remove the sequencer go.mod, as ev-node can provide directly the sequencer implementation (sequencers/single was already depending on ev-node anyway)

this means no go.mod need to be added for the new based sequencers in #2797

@github-actions
Copy link
Contributor

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedNov 12, 2025, 4:42 PM

@claude
Copy link
Contributor

claude bot commented Nov 12, 2025

Claude finished @julienrbrt's task —— View job


Code Review Complete

I've thoroughly reviewed this PR that removes the sequencers/single go.mod file and consolidates it into the main ev-node module. Here's my comprehensive analysis:


Executive Summary

Overall Assessment: ✅ LGTM with Minor Observations

This is a well-executed architectural consolidation that simplifies the module structure. The change is clean, properly documented, and aligns with the project's architectural principles. The sequencer implementation remains intact in the codebase at sequencers/single/, but no longer has its own go.mod, making it part of the main ev-node module.


1. Code Quality & Architecture

✅ Strengths:

  • Clean consolidation: The PR removes 869 lines (mostly go.mod/go.sum) while keeping the actual sequencer implementation intact
  • Consistent with zero-dependency core pattern: The sequencer package imports from github.com/evstack/ev-node/core/sequencer (the interface) and the implementation is now directly part of ev-node
  • Proper module references: All application packages (evm/single, grpc/single, testapp) correctly import from github.com/evstack/ev-node/sequencers/single, which now resolves to the main module
  • Maintains modularity: Despite removing the separate module, the code organization remains clean with clear package boundaries

📋 Observations:

  • The sequencer code still imports remain at github.com/evstack/ev-node/sequencers/single in all three applications:

    • apps/evm/single/cmd/run.go:13
    • apps/grpc/single/cmd/run.go:20
    • apps/testapp/cmd/run.go:19

    These imports are now resolved via the root go.mod, which is the intended behavior.

🔍 Architectural Rationale:

The PR description states: "sequencers/single was already depending on ev-node anyway" - this is a key insight. Having a circular dependency situation (sequencer depending on ev-node, but being a separate module) justified this consolidation. This matches the architectural principle from CLAUDE.md: "Components are wired together via dependency injection" and the core package pattern.


2. Error Handling & Resilience

✅ No Impact: This PR only removes module boundaries, not implementation code. The sequencer's error handling, recovery mechanisms, and resilience features remain unchanged.


3. Security

✅ No Security Concerns:

  • No changes to authentication, authorization, or credential handling
  • No new external dependencies introduced
  • Actually reduces attack surface by consolidating modules
  • Dependency tree simplified by removing intermediate module layer

4. Performance & Resource Efficiency

✅ Neutral to Positive Impact:

  • Build times may improve slightly due to fewer module boundaries
  • No runtime performance changes (code remains identical)
  • Simpler dependency graph for Go toolchain to resolve

5. Testing & Reliability

✅ Strengths:

  • The sequencer implementation and its tests remain in place at sequencers/single/
  • CI is running (currently in progress) to validate the changes
  • Buf checks passed ✅

⚠️ Recommendations:

  1. Verify all tests pass: Especially integration tests that exercise the sequencer functionality across the three applications
  2. Check go.work compatibility: The PR removes sequencers/single from go.work.example (line 11), which is correct. Developers using go workspaces should update their local go.work files accordingly

6. Documentation & Developer Experience

✅ Excellent Documentation Updates:

The RELEASE.md changes are thorough and well-structured:

  1. Removed obsolete Phase 3 (sequencer releases) from the release process
  2. Updated dependency graph - removed the intermediate sequencer layer from the ASCII diagram (lines 79-84)
  3. Simplified release order - consolidated from 4 phases to 3 phases
  4. Updated release commands - removed sequencer-specific go get commands from application releases
  5. Formatting improvements - added blank lines for better readability

📝 Minor Documentation Observations:

  1. Consider adding a migration note: While the RELEASE.md is updated, consider adding a brief note in a changelog or migration guide about this architectural change for external users who may have been depending on the sequencer as a separate module
  2. go.work.example is correct: Developers using Go workspaces will need to update their local go.work files to remove the ./sequencers/single entry

7. Specific File-by-File Review

RELEASE.md (MODIFIED)

  • Lines removed: 58 lines of obsolete sequencer release documentation
  • Lines added: 11 lines of improved formatting
  • Assessment: ✅ Excellent - documentation accurately reflects new architecture
  • Specific improvements:
    • Dependency graph simplified (removed Phase 3 sequencer layer)
    • Release process now goes: Core → First-Level Deps → Applications (not 4 phases)
    • Commands updated to remove go get github.com/evstack/ev-node/sequencers/[email protected]

apps/evm/single/go.mod (MODIFIED)

  • Change: Removed dependency on github.com/evstack/ev-node/sequencers/single v1.0.0-beta.3
  • Assessment: ✅ Correct - now inherits from root module

apps/grpc/single/go.mod (MODIFIED)

  • Change: Removed dependency on github.com/evstack/ev-node/sequencers/single
  • Assessment: ✅ Correct

apps/testapp/go.mod (MODIFIED)

  • Change: Removed dependency line
  • Assessment: ✅ Correct

go.work.example (MODIFIED)

  • Change: Removed ./sequencers/single from workspace
  • Assessment: ✅ Correct - no longer a separate module

sequencers/single/go.mod (DELETED)

  • Assessment: ✅ This is the main change - 161 lines removed

sequencers/single/go.sum (DELETED)

  • Assessment: ✅ Consistent with go.mod deletion - 652 lines removed

8. Verification Checklist

✅ Completed Checks:

  • All go.mod files updated consistently
  • No lingering references to github.com/evstack/ev-node/sequencers/single as an external module
  • Documentation updated to reflect new structure
  • go.work.example properly updated
  • Sequencer implementation code remains in place
  • Import paths in applications remain valid (now resolve via root module)

⏳ Pending Verification (CI should confirm):

  • All tests pass with new module structure
  • make lint passes
  • make test passes for all modules
  • make build-all succeeds

9. Alignment with Related Work

The PR description mentions: "this means no go.mod need to be added for the new based sequencers in #2797"

✅ This is architecturally sound: The new sequencers/based implementation can be added directly to the sequencers directory without creating another module boundary, following the pattern established here.


10. Recommendations

Required Actions: None (PR is ready to merge pending CI)

Optional Improvements:

  1. Add CHANGELOG entry: Document this architectural change for users tracking module versions

  2. Version strategy note: Since this is a breaking change for anyone depending on the separate github.com/evstack/ev-node/sequencers/single module, consider:

    • Documenting this in release notes as a breaking change
    • Providing a migration path (simply remove the explicit dependency, it's now transitive via ev-node)
  3. Developer communication: Update any developer documentation or onboarding guides that might reference the old module structure


Final Verdict

Recommendation: ✅ APPROVE (pending CI)

This PR represents a thoughtful architectural improvement that:

  • Eliminates unnecessary module boundaries
  • Simplifies the dependency graph and release process
  • Maintains all functionality while reducing complexity
  • Is thoroughly documented with comprehensive RELEASE.md updates
  • Aligns with the project's zero-dependency core pattern and modular design principles

The code quality is excellent, security is maintained, and the developer experience is improved through clearer release procedures. Once CI passes, this is ready to merge.


CI Status

Currently running:

  • ✅ Buf checks: PASSED
  • ⏳ CI: IN PROGRESS - View run
  • ⏳ Semantic PR: PASSED

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the project's module structure by eliminating the standalone sequencers/single Go module. This change is based on the decision that the ev-node can directly provide the sequencer implementation, thereby simplifying the dependency graph for various applications. The update includes removing module declarations and dependencies, and revising the release documentation to reflect the streamlined architecture.

Highlights

  • Module Removal: The dedicated sequencers/single Go module has been entirely removed, along with its go.mod and go.sum files.
  • Dependency Simplification: Applications like apps/evm/single, apps/grpc/single, and apps/testapp no longer directly depend on sequencers/single, streamlining their go.mod and go.sum files.
  • Release Process Update: The RELEASE.md documentation has been updated to reflect the removal of the sequencers/single module from the Go module release phases and commands, simplifying the overall release guide.
  • Go Workspace Configuration: The go.work.example file has been adjusted to remove the reference to the now-deleted sequencers/single directory.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to simplify the Go module structure by removing the separate go.mod for the sequencer. The changes correctly remove dependencies and update the release documentation for sequencers/single. However, I've identified a potential inconsistency regarding the sequencers/based package, which is removed from the documentation but its module files are not touched in this PR. Please see my detailed comment.

@codecov
Copy link

codecov bot commented Nov 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.65%. Comparing base (8436bf4) to head (c24f794).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2837      +/-   ##
==========================================
- Coverage   64.74%   64.65%   -0.09%     
==========================================
  Files          81       81              
  Lines        7170     7170              
==========================================
- Hits         4642     4636       -6     
- Misses       1989     1994       +5     
- Partials      539      540       +1     
Flag Coverage Δ
combined 64.65% <ø> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@julienrbrt julienrbrt added this pull request to the merge queue Nov 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 12, 2025
@julienrbrt julienrbrt added this pull request to the merge queue Nov 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 12, 2025
@julienrbrt julienrbrt merged commit bdef117 into main Nov 13, 2025
30 of 31 checks passed
@julienrbrt julienrbrt deleted the julien/remove-sequencer-gomod branch November 13, 2025 06:39
@github-project-automation github-project-automation bot moved this to Done in Evolve Nov 13, 2025
alpe added a commit that referenced this pull request Nov 17, 2025
* main:
  fix: remove duplicate error logging in light node shutdown (#2841)
  chore: fix incorrect function name in comment (#2840)
  chore: remove sequencer go.mod (#2837)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants