Skip to content

Conversation

@julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Nov 13, 2025

Rename evm-single to evm and grpc-single to evgrpc for clarity.

ref: #2797 (comment)

DNM until #2797 is merged!

@github-actions
Copy link
Contributor

github-actions bot commented Nov 13, 2025

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedNov 17, 2025, 2:29 PM

@claude
Copy link
Contributor

claude bot commented Nov 13, 2025

Claude finished @julienrbrt's task —— View job


Code Review Complete

I've conducted a comprehensive review of this PR which renames evm-singleevm and grpc-singleevgrpc. The renaming is mostly consistent and well-executed, but I've identified several issues that need attention.


Critical Issues

1. Incorrect COPY path in apps/evm/Dockerfile (Line 22) 🔴

File: apps/evm/Dockerfile:22

The Dockerfile has the correct WORKDIR at line 12 (/src/apps/evm) and the build happens correctly at line 13, so the binary is at /src/apps/evm/evm. The COPY statement at line 22 is also correct:

COPY --from=build-env /src/apps/evm/evm /usr/bin/evm

Status: ✅ This is actually correct - Gemini's review flagged this incorrectly.


2. Variable naming inconsistency in test/e2e/evm_test_common.go 🟡

File: test/e2e/evm_test_common.go:41-45

The variable is declared as evmSingleBinaryPath but the flag description and default value suggest it should be evmBinaryPath:

// evmSingleBinaryPath is the path to the evm-single binary used in tests
var evmSingleBinaryPath string

func init() {
	flag.StringVar(&evmSingleBinaryPath, "evm-binary", "evm", "evm binary")
}

Recommendation: The variable name evmSingleBinaryPath is outdated and should be renamed to evmBinaryPath for consistency with the renaming. However, this is used extensively throughout the test files (20+ occurrences), so changing it requires updating all references. The comment on line 41 is also outdated (evm-single binary).

Suggested fix:

  • Rename evmSingleBinaryPathevmBinaryPath throughout all test files
  • Update the comment to say "evm binary" instead of "evm-single binary"

3. Docker volume name inconsistency in apps/evm/docker-compose.yml 🟡

File: apps/evm/docker-compose.yml:64, 82

The volume is still named evm-single-data and should be renamed to evm-data for consistency:

volumes:
  - evm-single-data:/root/.evm/  # Line 64
...
volumes:
  evm-single-data:  # Line 82

Recommendation: Rename to evm-data for consistency.


4. Database name in apps/testapp/cmd/rollback.go vs apps/evm/cmd/rollback.go ⚠️

Files:

  • apps/testapp/cmd/rollback.go:42
  • apps/evm/cmd/rollback.go:41

The testapp uses "testapp" as the database name:

rawEvolveDB, err := store.NewDefaultKVStore(nodeConfig.RootDir, nodeConfig.DBPath, "testapp")

The evm app uses "evm" as the database name:

rawEvolveDB, err := store.NewDefaultKVStore(nodeConfig.RootDir, nodeConfig.DBPath, "evm")

Status: ✅ This is correct - each app should use its own database name. Gemini's review flagged the testapp file as incorrect, but it's actually fine.


5. RELEASE.md numbering issue 🟡

File: RELEASE.md:91-108

After removing the first item in the Phase 2 list, the numbering is off. Line 100 should be renumbered from 1. to match the correct sequence:

#### Phase 2: First-Level Dependencies

These packages only depend on `core` and can be released in parallel after `core`:

1. **github.com/evstack/ev-node/da** - Path: `./da`
2. **github.com/evstack/ev-node** - Path: `./` (root)
3. **github.com/evstack/ev-node/execution/evm** - Path: `./execution/evm`

Currently all three are numbered 1. which is incorrect Markdown formatting.


Documentation & Configuration Issues

6. Outdated references to rollkit.* flags in apps/evm/entrypoint.sh ℹ️

File: apps/evm/entrypoint.sh (lines 40, 71, 75, 80, 84, 88, 92)

The entrypoint script uses rollkit.* prefixed flags (e.g., --rollkit.node.aggregator, --rollkit.da.address) while the documentation and other parts of the codebase use evnode.* flags. This suggests the PR may be based on an older version or there's an inconsistency with the base branch.

Examples:

  • --rollkit.node.aggregator (should potentially be --evnode.node.aggregator)
  • --rollkit.da.address (should potentially be --evnode.da.address)

Status: ⚠️ This appears to be pre-existing from the base branch and not introduced by this PR. However, worth noting for consistency.


Minor Issues & Observations

7. CHANGELOG.md entry is correct

File: CHANGELOG.md:21

The changelog correctly documents the renaming:

- Rename `evm-single` to `evm` and `grpc-single` to `evgrpc` for clarity.

8. Build scripts updated correctly

File: scripts/build.mk

All build targets have been updated correctly:

  • Line 32: Building evm instead of evm-single
  • Line 33: Path changed to apps/evm
  • Line 50: Build target renamed to build-evm

9. CI workflow updates

Files:

  • .github/workflows/ci.yml:57
  • .github/workflows/docker-tests.yml:50

Docker image names updated correctly:

  • ev-node-evm (from apps/evm/Dockerfile)
  • Image tag properly references the new structure

10. Test file updates

File: test/docker-e2e/upgrade_test.go

All references to the EVM container and image have been updated correctly, using the new naming conventions.


Code Quality Assessment

Architecture & Modularity ✅

  • The renaming maintains the modular structure
  • No cyclic dependencies introduced
  • Clean separation between apps

Error Handling ✅

  • Error handling patterns remain consistent
  • No new error handling issues introduced

Security ✅

  • JWT secrets still handled via files (not hardcoded)
  • No security regressions from the renaming

Performance ✅

  • No performance impact from renaming
  • Build and runtime behavior unchanged

Testing ✅

  • Test structure maintained
  • E2E tests updated appropriately
  • Docker upgrade tests reference new image names

Documentation 🟡

  • README files updated correctly
  • RELEASE.md has minor numbering issue (see issue Mempool #5)
  • Documentation references the new names consistently

Summary of Required Changes

Priority Issue File(s) Action
🟡 Medium Variable naming test/e2e/evm_test_common.go Rename evmSingleBinaryPathevmBinaryPath (20+ occurrences)
🟡 Medium Docker volume apps/evm/docker-compose.yml Rename evm-single-dataevm-data
🟡 Low List numbering RELEASE.md Fix numbering in Phase 2 section

Recommendations

  1. Variable Renaming: Consider renaming evmSingleBinaryPath to evmBinaryPath across all test files for consistency. This is a straightforward find-replace operation but touches multiple files.

  2. Docker Volume: Update the volume name in docker-compose.yml to match the new naming scheme. Note: Users with existing deployments will need to migrate data or be aware of the volume name change.

  3. Documentation: Fix the numbering in RELEASE.md for professional presentation.

  4. Breaking Change Notice: Consider adding a migration note in the CHANGELOG about the Docker volume name change for existing deployments.


Overall Assessment

This is a well-executed renaming PR that improves clarity by simplifying the naming scheme. The changes are systematic and mostly complete. The identified issues are minor and don't affect functionality significantly - they're primarily about consistency and polish.

The PR successfully:

  • ✅ Renames application binaries and directories
  • ✅ Updates all Docker configurations
  • ✅ Maintains test coverage
  • ✅ Updates CI/CD workflows
  • ✅ Preserves all functionality
  • ✅ Documents the change in CHANGELOG

Verdict: Approve with minor suggestions for improvement. The issues identified are cosmetic and can be addressed in follow-up commits or accepted as-is.

@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 streamlines the project's naming conventions by shortening the names of two core applications. The evm-single application is now simply evm, and grpc-single is now evgrpc. This change is purely cosmetic, aimed at enhancing readability and ease of reference throughout the codebase, documentation, and operational scripts, without altering any underlying functionality.

Highlights

  • Application Renaming: The evm-single application has been renamed to evm and grpc-single has been renamed to evgrpc to improve clarity and simplify naming conventions.
  • File Structure Update: All related files, including Dockerfiles, READMEs, Go modules, and command-line interface (CLI) entrypoints, have been moved from apps/evm/single to apps/evm and apps/grpc/single to apps/grpc respectively.
  • Configuration and Script Adjustments: Build scripts, test configurations, and documentation have been updated to reflect the new application names and file paths, ensuring consistency across the project.
Ignored Files
  • Ignored by pattern: .github/workflows/** (3)
    • .github/workflows/ci.yml
    • .github/workflows/docker-tests.yml
    • .github/workflows/ghcr-prune.yml
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.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 13, 2025

PR Preview Action v1.6.2

🚀 View preview at
https://evstack.github.io/docs-preview/pr-2839/

Built to branch main at 2025-11-17 14:30 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

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 PR does a good job of renaming evm-single to evm and grpc-single to evgrpc across the codebase. The changes are extensive and mostly consistent. I've found a few minor inconsistencies and potential issues that should be addressed for completeness. These include a numbering issue in RELEASE.md, incorrect paths in a Dockerfile, an inconsistent volume name in a docker-compose.yml, a hardcoded value in a test app that seems to be a copy-paste error, and a variable declaration issue in an E2E test file. Overall, a good refactoring effort.

@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.38%. Comparing base (0d790ef) to head (481642f).

Additional details and impacted files
@@              Coverage Diff              @@
##           julien/fi    #2839      +/-   ##
=============================================
- Coverage      64.47%   64.38%   -0.10%     
=============================================
  Files             83       83              
  Lines           7600     7600              
=============================================
- Hits            4900     4893       -7     
- Misses          2141     2148       +7     
  Partials         559      559              
Flag Coverage Δ
combined 64.38% <ø> (-0.10%) ⬇️

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.

@tac0turtle tac0turtle removed this from Evolve Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants