chore!: rename evm-single and grpc-single
#2839
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task —— View job Comprehensive Code Review CompleteI've reviewed the PR that renames Critical Issues1. Variable Declaration Mismatch in Test File (test/e2e/evm_test_common.go)Severity: High The variable is declared as // Line 41-42
// evmSingleBinaryPath is the path to the evm-single binary used in tests
var evmSingleBinaryPath stringRecommendation: Rename the variable to // evmBinaryPath is the path to the evm binary used in tests
var evmBinaryPath stringUpdate all 12 references in the file: lines 42, 45, 247, 302, 326, 343, 369, 393, 424, 488, 605, 651, 701. 2. Inconsistent Volume Names in Docker Compose (apps/evm/docker-compose.yml)Severity: Medium Lines 64 and 82 still use volumes:
- evm-single-data:/root/.evm/ # Line 64
volumes:
evm-single-data: # Line 82Recommendation: Rename to volumes:
- evm-data:/root/.evm/
volumes:
evm-data:3. Incorrect Database Name in Testapp Rollback Command (apps/testapp/cmd/rollback.go)Severity: Medium-High Line 42 uses Current state (appears correct): rawEvolveDB, err := store.NewDefaultKVStore(nodeConfig.RootDir, nodeConfig.DBPath, "testapp")Documentation and Consistency Issues4. RELEASE.md Numbering (RELEASE.md)Severity: Low The list numbering in the "Release Order" section has an inconsistency. After the Phase 1 section lists item 1, Phase 2 starts with item 1 again instead of item 2. While this is technically acceptable as a nested structure, it could be clearer. Current: #### Phase 1: Core Package
1. **github.com/evstack/ev-node/core**
#### Phase 2: First-Level Dependencies
1. **github.com/evstack/ev-node/da** - Path: `./da`
2. **github.com/evstack/ev-node** - Path: `./` (root)Consider either:
5. Old Tag References in RELEASE.mdSeverity: Low Multiple examples in RELEASE.md still reference
Recommendation: Update all examples to use Code Quality AssessmentPositive Observations
Architecture & Go Code QualityThe renamed applications maintain good Go practices:
Docker Configuration Reviewapps/evm/Dockerfile:
apps/grpc/Dockerfile:
Test Configurationtest/e2e/evm_test_common.go:
Security ObservationsNo security concerns introduced by this refactoring. The rename maintains:
Performance ConsiderationsNo performance impact expected from this rename. The changes are purely nomenclature. Summary of Required ChangesMust Fix Before Merge:
Should Fix (Documentation):
Verification:
Testing RecommendationsBefore merging, please verify:
ConclusionThis is a solid refactoring PR that improves naming clarity. The identified issues are straightforward to fix. The most critical item is the Docker volume name inconsistency in Overall Assessment: ✅ Approved with minor changes requested The rename is well-executed and follows through consistently across the codebase. Great job on maintaining coherence across 37 files! |
Summary of ChangesHello @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 Highlights
Ignored Files
Using Gemini Code AssistThe 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
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 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
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. 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
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:
|
Rename
evm-singletoevmandgrpc-singletoevgrpcfor clarity.ref: #2797 (comment)
DNM until #2797 is merged!