feat: expose execution client params to ev-node #2982
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 2m 37s —— View job Code Review CompleteI've completed a comprehensive code review of PR #2982. This is a well-executed refactoring that significantly improves the architecture by separating concerns and making execution parameters dynamically queryable. Below are my findings organized by category. SummaryThis PR successfully refactors the Overall Assessment: ✅ Approve with minor recommendations 1. Architecture & Code Quality ⭐Strengths
Issues & Recommendations🔴 Critical: Protobuf backward compatibility issueLocation: Problem: The
Current state: message InitChainResponse {
bytes state_root = 1;
uint64 max_bytes = 2; // ⚠️ Still here but not returned by Go code
}Recommendation:
Learn more about protobuf field deprecation 🟡 Medium: Incomplete test coverage for new functionalityLocation: Problem: As noted by Gemini's review, the test validation was weakened: if len(resp.Msg.Txs) != len(expectedTxs) {
t.Fatalf("expected %d txs, got %d", len(expectedTxs), len(resp.Msg.Txs))
}
// ⚠️ Missing: content validation of returned transactionsRecommendation: Add content validation: for i, tx := range resp.Msg.Txs {
if !bytes.Equal(tx, expectedTxs[i]) {
t.Errorf("tx %d: expected %x, got %x", i, expectedTxs[i], tx)
}
}🟡 Medium: Missing tests for new RPC methodsLocation: Problem: No dedicated tests for Recommendation: Add test cases following the existing pattern in
🟢 Low: Documentation could be clearer about non-gas execution layersLocation: Observation: The comments mention "For non-gas-based execution layers, return ExecutionInfo{MaxGas: 0}" but don't clarify the behavior implications. Recommendation: Expand the comment to explain what happens when MaxGas=0: // For non-gas-based execution layers, return ExecutionInfo{MaxGas: 0}.
// When MaxGas is 0, the sequencer will include all DA transactions without gas filtering.2. Error Handling & Resilience ✅Strengths
Issues🟢 Low: Silent failure on GetExecutionInfo errorLocation: info, err := c.executor.GetExecutionInfo(ctx, 0)
if err != nil {
c.logger.Warn().Err(err).Msg("failed to get execution info for gas filtering, proceeding without gas filter")
filteredForcedTxs = forcedTxs // ⚠️ Silently continues
}Observation: While logging the error is good, this could mask a persistent issue (e.g., network problems with remote executor). Consider adding metrics or rate-limiting the warnings. Recommendation: Add a metric counter for GetExecutionInfo failures to track if this is happening frequently. 3. Security 🔒Strengths
Issues🟢 Low: Consider adding telemetry for filtered gibberish transactionsLocation: Observation: Gibberish transactions are filtered with debug logging, but there's no metric tracking how many malicious/malformed transactions are being filtered from DA. Recommendation: Add a counter metric for security monitoring: c.logger.Debug().Int("tx_index", i).Err(err).Msg("filtering out invalid DA transaction (gibberish)")
// Add: metrics.IncrCounter("da_transactions_filtered_gibberish")4. Performance & Resource Efficiency ⚡Strengths
Issues🟡 Medium: Potential N+1 query patternLocation: Problem: Recommendation: Consider caching the ExecutionInfo with a short TTL (e.g., 1 second) since gas limits typically don't change frequently: // Add to Sequencer struct:
type Sequencer struct {
// ...
cachedExecInfo execution.ExecutionInfo
cachedExecInfoTime time.Time
execInfoCacheTTL time.Duration
}
// In GetNextBatch:
if time.Since(c.cachedExecInfoTime) > c.execInfoCacheTTL {
info, err := c.executor.GetExecutionInfo(ctx, 0)
if err == nil {
c.cachedExecInfo = info
c.cachedExecInfoTime = time.Now()
}
}🟢 Low: Telemetry wrapper creates span on every callLocation: Observation: The tracing wrapper creates OpenTelemetry spans for every method call. While this is correct, it adds overhead. Ensure tracing is disabled in production if performance is critical. Note: This is expected behavior for distributed tracing, but worth documenting. 5. Testing & Reliability 🧪Strengths
Issues🔴 Critical: Low patch coverage (35.59%)Location: Codecov report shows 38 missing lines Primary culprits:
Recommendation:
Example test to add: func TestTracedExecutor_GetExecutionInfo(t *testing.T) {
mockExec := &mockExecutor{
getExecutionInfoFunc: func(ctx context.Context, height uint64) (execution.ExecutionInfo, error) {
return execution.ExecutionInfo{MaxGas: 30000000}, nil
},
}
traced := telemetry.WithTracingExecutor(mockExec)
info, err := traced.GetExecutionInfo(context.Background(), 0)
require.NoError(t, err)
assert.Equal(t, uint64(30000000), info.MaxGas)
}🟡 Medium: Missing negative test casesRecommendation: Add tests for:
6. Documentation & Developer Experience 📚Strengths
Issues🟢 Low: Missing CHANGELOG entryObservation: This is a significant API change that affects all executor implementations. Recommendation: Add a CHANGELOG entry or migration guide: ## Executor Interface Changes
### Breaking Changes (for implementers)
- `InitChain` and `ExecuteTxs` no longer return `maxBytes`
- All executors must now implement `GetExecutionInfo(ctx, height) ExecutionInfo`
### Optional Enhancements
- Implement `DATransactionFilter` interface for gas-based DA transaction filtering🟢 Low: Consider adding architecture decision record (ADR)Recommendation: Document why 7. Concurrency & Thread Safety 🧵Strengths
No Issues Identified ✅8. Consistency Across Codebase 🎯Strengths
No Issues Identified ✅Action Items SummaryMust Fix Before Merge 🔴
Should Fix 🟡
Nice to Have 🟢
ConclusionThis is a high-quality refactoring that improves the codebase architecture significantly. The separation of execution parameters into a dedicated query method ( The main concerns are:
Once the test coverage is improved and the protobuf compatibility is clarified, this PR will be ready to merge. Estimated effort to address critical issues: ~2-3 hours for test additions and protobuf clarification. |
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 significantly refactors the interaction between Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 pull request refactors the Executor interface by removing the maxBytes return value from InitChain and ExecuteTxs. It introduces new interfaces, ExecutionInfoProvider and DATransactionFilter, to expose execution client parameters like MaxGas more explicitly. The changes are applied consistently throughout the codebase, including implementations, mocks, and tests. This is a solid refactoring that improves the separation of concerns. I've found one issue in a test file where a check was removed, weakening the test's validation.
| if len(resp.Msg.Txs) != len(expectedTxs) { | ||
| t.Fatalf("expected %d txs, got %d", len(expectedTxs), len(resp.Msg.Txs)) | ||
| } |
There was a problem hiding this comment.
The check for the content of the returned transactions has been removed, which weakens the test. It now only verifies the number of transactions, not that they are the correct ones. It would be better to verify the content as well to ensure the server is returning the expected data.
if len(resp.Msg.Txs) != len(expectedTxs) {
t.Fatalf("expected %d txs, got %d", len(expectedTxs), len(resp.Msg.Txs))
}
for i, tx := range resp.Msg.Txs {
if string(tx) != string(expectedTxs[i]) {
t.Errorf("tx %d: expected %s, got %s", i, expectedTxs[i], tx)
}
}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2982 +/- ##
==========================================
- Coverage 59.13% 58.75% -0.38%
==========================================
Files 102 102
Lines 9822 9931 +109
==========================================
+ Hits 5808 5835 +27
- Misses 3397 3476 +79
- Partials 617 620 +3
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:
|
julienrbrt
force-pushed
the
julien/gas
branch
from
a898b7b to
2800a75
Compare
Closes: #2965