Skip to content
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

app/eth2wrap: fix error comparison for synthetic blocks #2702

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

dB2510
Copy link
Contributor

@dB2510 dB2510 commented Nov 10, 2023

There was a small issue when comparing eth2api errors when searching for signed beacon block. The error returned should be part of eth2wrap multi implementation which was not able to decompose eth2 errors properly. This PR fixes decomposition of eth2 errors and adds a function to check if the relevant field is present as part of the error chain.

category: bug
ticket: #2695

@@ -330,7 +330,7 @@ func incError(endpoint string) {
// wrapError returns the error as a wrapped structured error.
func wrapError(ctx context.Context, err error, label string, fields ...z.Field) error {
// Decompose go-eth2-client http errors
if apiErr := new(eth2api.Error); errors.As(err, apiErr) {
if apiErr := new(eth2api.Error); errors.As(err, &apiErr) {
Copy link
Contributor Author

@dB2510 dB2510 Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this was not working at all until now :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There must be a linter somewhere that checks for this...

if apiErr.StatusCode == http.StatusNotFound {
continue
}
if fieldExists(err, zap.Int("status_code", http.StatusNotFound)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed as the returned error will be a wrapped error from eth2wrap.Multi struct

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f3803b2) 53.19% compared to head (ae2fb60) 53.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2702      +/-   ##
==========================================
+ Coverage   53.19%   53.47%   +0.27%     
==========================================
  Files         201      201              
  Lines       27596    27615      +19     
==========================================
+ Hits        14679    14766      +87     
+ Misses      11099    11032      -67     
+ Partials     1818     1817       -1     
Files Coverage Δ
app/eth2wrap/eth2wrap.go 61.27% <100.00%> (+10.63%) ⬆️
app/eth2wrap/synthproposer.go 66.48% <81.81%> (+3.55%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented Nov 10, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
23.8% 23.8% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@@ -204,6 +204,34 @@ func (h *synthWrapper) syntheticProposal(ctx context.Context, slot eth2p0.Slot,
return proposal, nil
}

// fieldExists checks if the given field exists as part of the given error.
func fieldExists(err error, field zap.Field) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if instead of declaring this function here, we should create a z.Fields() function that returns []z.Field.

The casting logic seems to be a 1:1 copy off z.Err.

Copy link
Collaborator

@gsora gsora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving and opening an issue to later address https://github.com/ObolNetwork/charon/pull/2702/files#r1389222292

@gsora gsora added the merge when ready Indicates bulldozer bot may merge when all checks pass label Nov 10, 2023
@obol-bulldozer obol-bulldozer bot merged commit 8203a6a into main Nov 10, 2023
13 of 15 checks passed
@obol-bulldozer obol-bulldozer bot deleted the dhruv/fixsynthproposer branch November 10, 2023 11:26
gsora pushed a commit that referenced this pull request Nov 10, 2023
There was a small issue when comparing eth2api errors when searching for signed beacon block. The error returned should be part of eth2wrap multi implementation which was not able to decompose eth2 errors properly. This PR fixes decomposition of eth2 errors and adds a function to check if the relevant field is present as part of the error chain.

category: bug
ticket: #2695
obol-bulldozer bot pushed a commit that referenced this pull request Nov 10, 2023
This PR cherry-picks #2702 onto `main-v0.18`

---

There was a small issue when comparing eth2api errors when searching for signed beacon block. The error returned should be part of eth2wrap multi implementation which was not able to decompose eth2 errors properly. This PR fixes decomposition of eth2 errors and adds a function to check if the relevant field is present as part of the error chain.

category: bug
ticket: #2695
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants