-
Notifications
You must be signed in to change notification settings - Fork 234
chore: remove unused flag #2765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @tac0turtle, 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 performs a minor cleanup by removing an unused flag from the Docker Compose configuration for the EVM execution environment. The Highlights
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
|
|
Claude finished @tac0turtle's task —— View job Code Review CompleteI've reviewed this PR which removes the OverviewThis PR performs cleanup by:
Critical Issue: Gemini's Concern Requires ClarificationGemini Code Assist raised a valid concern that needs to be addressed in the PR description: The
These are evolve-specific extensions to the Engine API that require special support in ev-reth. The concern is:
My Analysis:Looking at the other flag present in all configurations: --engine.always-process-payload-attributes-on-canonical-headThis flag suggests ev-reth has custom Engine API behavior. The fact that:
Strongly suggests the flag is now obsolete - likely because the feature is now enabled by default in newer ev-reth versions, or was consolidated into the However, this critical context is missing from the PR description, making it impossible for reviewers to confidently approve without manual testing. Detailed Review1. Configuration Cleanup ✅Files Modified:
Changes: Consistently removed Quality:
2. Dependency Updatesexecution/evm/test/go.mod: - github.com/celestiaorg/tastora v0.5.0
+ github.com/celestiaorg/tastora v0.7.3test/e2e/go.mod: - github.com/celestiaorg/tastora v0.7.1
+ github.com/celestiaorg/tastora v0.7.3Analysis:
Note: @tac0turtle mentioned "we need a new version of testora" - this appears to be referring to tastora (likely a typo). This dependency bump is likely the actual reason for this PR, with the flag removal being a related cleanup. 3. Testing & CIObservations:
Security & Performance ReviewSecurity ✅
Performance ✅
Code Quality & ArchitectureStrengths:
Minor Issues:
Required Actions Before Merge1. Update PR Description (High Priority)The PR description is currently empty except for "## Overview". Please add: ## Overview
Removes the deprecated `--ev-reth.enable` flag from all ev-reth configurations.
This flag is no longer needed as of ev-reth version X.Y.Z, where this functionality
is now enabled by default (or consolidated into --engine.always-process-payload-attributes-on-canonical-head).
Also updates tastora dependency to v0.7.3 for [specific reason - link to changelog if available].
## Testing
- [ ] Verified block production works on local testnet
- [ ] Confirmed custom Engine API attributes (transactions, gasLimit) are still processed
- [ ] E2E tests passing2. Verify E2E Tests (Critical)Even though CI is passing, explicitly confirm:
3. Document ev-reth Version Dependency (Nice to Have)Consider adding a comment or documentation noting the minimum ev-reth version required: # Requires ev-reth >= X.Y.Z (--ev-reth.enable is deprecated)
image: ghcr.io/evstack/ev-reth:latestRecommendationConditional Approval: This PR is well-executed from a code quality perspective, but requires clarification in the PR description before final approval. If the flag is truly obsolete (as the evidence suggests), this is a good cleanup. If not, this would break block production in a non-obvious way that wouldn't be caught until runtime. Given that @tac0turtle is the author and likely has context that confirms this is safe, I recommend:
Additional NotesThe tastora version bump from v0.5.0 → v0.7.3 is a minor version jump that could potentially include breaking changes or important fixes. Consider reviewing the tastora changelog to ensure no unexpected behavior changes. Files Reviewed: 8/8 |
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
There was a problem hiding this 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 removes the --ev-reth.enable flag from the docker-compose.yml file for the ev-reth service. While the change itself is a single line deletion, it poses a significant risk. My review has identified a potential critical issue: the EngineClient implementation in this repository relies on custom, non-standard Engine API behavior that this flag most likely enables in the ev-reth execution client. Removing it without corresponding client-side changes could break the communication between the services and halt block production.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2765 +/- ##
=======================================
Coverage 61.42% 61.42%
=======================================
Files 81 81
Lines 8622 8622
=======================================
Hits 5296 5296
Misses 2828 2828
Partials 498 498
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:
|
|
we need a new version of testora |
Overview