chore(execution): Dropped DenebPlus redundant check #2069
Conversation
WalkthroughThe changes in this pull request focus on modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -116,7 +117,8 @@ func (s *EngineClient[ | |||
| s.metrics.incrementForkchoiceUpdateTimeout() | |||
| } | |||
| return nil, nil, s.handleRPCError(err) | |||
| } else if result == nil { | |||
There was a problem hiding this comment.
not strictly necessary for this PR, but I like reducing indentation as much as possible
| switch { | ||
| case err != nil: | ||
| if err != nil { | ||
| if errors.Is(err, engineerrors.ErrEngineAPITimeout) { | ||
| s.metrics.incrementGetPayloadTimeout() | ||
| } | ||
| return result, s.handleRPCError(err) | ||
| case result == nil: | ||
| } | ||
| if result == nil { | ||
| return result, engineerrors.ErrNilExecutionPayloadEnvelope | ||
| case result.GetBlobsBundle() == nil && | ||
| ((forkVersion >= version.Deneb) || (forkVersion >= version.DenebPlus)): |
There was a problem hiding this comment.
forkVersion check is done in s.Client.GetPayload
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/execution/pkg/client/engine.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
mod/execution/pkg/client/engine.go (3)
64-66: LGTM: Improved error handlingThe changes simplify the error handling logic by directly checking if the result is nil. This improves code readability while maintaining the same functionality.
120-122: LGTM: Consistent error handling improvementThe changes in this method follow the same pattern as in
NewPayload, simplifying the error handling logic. This consistency improves the overall code quality and maintainability.
Line range hint
1-203: Summary: Improved error handling and data integrity checksThe changes in this file consistently enhance error handling across multiple methods (
NewPayload,ForkchoiceUpdated, andGetPayload). The modifications simplify the code structure, improve readability, and add important data integrity checks, particularly for the Deneb fork version.These improvements contribute to a more robust and maintainable codebase, reducing the likelihood of unhandled edge cases and improving the overall reliability of the engine client implementation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2069 +/- ##
=======================================
Coverage 22.25% 22.25%
=======================================
Files 356 356
Lines 16023 16021 -2
Branches 12 12
=======================================
Hits 3566 3566
+ Misses 12305 12303 -2
Partials 152 152
|
| if errors.Is(err, engineerrors.ErrEngineAPITimeout) { | ||
| s.metrics.incrementGetPayloadTimeout() | ||
| } | ||
| return result, s.handleRPCError(err) | ||
| case result == nil: | ||
| } | ||
| if result == nil { |
There was a problem hiding this comment.
note: all the result == nil checks in this file are redundant. Not sure we want to drop them, nor that in case I should do it in this PR. Prolly will open a separate one
Summary by CodeRabbit
Bug Fixes
Refactor