Skip to content

handle error #59

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

Merged
merged 2 commits into from
Jun 3, 2025
Merged

handle error #59

merged 2 commits into from
Jun 3, 2025

Conversation

ZzzzHui
Copy link
Collaborator

@ZzzzHui ZzzzHui commented May 15, 2025

No description provided.

@ZzzzHui ZzzzHui force-pushed the feature/handle-error branch 2 times, most recently from eb13388 to e0d8004 Compare May 16, 2025 04:58
@ZzzzHui ZzzzHui marked this pull request as ready for review May 16, 2025 04:59
@ZzzzHui ZzzzHui force-pushed the feature/handle-error branch from e0d8004 to 38d46db Compare May 16, 2025 07:39
@miltonjonat miltonjonat mentioned this pull request May 21, 2025
lastProcessedL1Block, l1FinalizedTimestamp = e.readL1(ctx, app, currentEspressoBlockHeight, lastProcessedL1Block)
lastProcessedL1Block, l1FinalizedTimestamp, err = e.readL1(ctx, app, currentEspressoBlockHeight, lastProcessedL1Block)
if err != nil {
slog.Error("failed reading L1", "error", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it ok to just skip?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're right. This should be a break

@sandhilt sandhilt requested a review from Copilot May 28, 2025 13:28
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Updates error handling across EVM and Espresso readers to return and propagate errors, and improves HTTP handlers to send appropriate responses.

  • Changed EvmReader to return errors instead of swallowing them
  • Enhanced EspressoReaderService endpoints to write HTTP error responses and close request bodies
  • Refactored EspressoReader to propagate errors from readL1 and stop silent skips in readEspresso

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
internal/evmreader/input.go Return wrapped errors on repository failures instead of logging and continuing
internal/espressoreader/espressoreader_service.go Send HTTP error responses, set Content-Type, and close request bodies
internal/espressoreader/espresso_reader_unit_test.go Decode test input with hex.DecodeString instead of hardcoded bytes
internal/espressoreader/espresso_reader.go Propagate errors from readL1 and adjust readEspresso loop control
Comments suppressed due to low confidence (2)

internal/espressoreader/espressoreader_service.go:191

  • Ensure the request body is closed in the submit handler by adding defer r.Body.Close() immediately after entry.
body, err := io.ReadAll(r.Body)

internal/espressoreader/espresso_reader_unit_test.go:919

  • Avoid ignoring the error from hex.DecodeString; handle or assert it, e.g., inputData, err := hex.DecodeString(...); require.NoError(err).
inputData, _ := hex.DecodeString("69689578...0000")

@@ -153,19 +153,23 @@ func (r *EvmReader) ReadAndStoreInputs(
combinedIndex, err := r.repository.GetInputIndexWithTx(ctx, dbTx, address.Hex())
if err != nil {
slog.Error("evmreader: failed to read index", "app", address, "error", err)
return errors.New("evmreader: failed to read index")
Copy link
Preview

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

Wrap and return the original error to preserve context, e.g., fmt.Errorf("evmreader: failed to read index: %w", err).

Suggested change
return errors.New("evmreader: failed to read index")
return fmt.Errorf("evmreader: failed to read index: %w", err)

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change doesn't seem necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

This change doesn't seem necessary

The only drawback here is that the original error is not preserved in the tracker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The original error message is printed as error message. So should be fine

input.RawData = modifiedRawData
if err != nil {
slog.Error("evmreader: failed to modify index", "app", address, "error", err)
return errors.New("evmreader: failed to modify index")
Copy link
Preview

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

Wrap the original error when returning, e.g., fmt.Errorf("evmreader: failed to modify index: %w", err).

Suggested change
return errors.New("evmreader: failed to modify index")
return fmt.Errorf("evmreader: failed to modify index: %w", err)

Copilot uses AI. Check for mistakes.

}
// update input index
err = r.repository.UpdateInputIndexWithTx(ctx, dbTx, address.Hex())
if err != nil {
slog.Error("failed to update index", "app", address, "error", err)
return errors.New("evmreader: failed to update index")
Copy link
Preview

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

Wrap the repository error to keep the original error context, e.g., fmt.Errorf("evmreader: failed to update index: %w", err).

Suggested change
return errors.New("evmreader: failed to update index")
return fmt.Errorf("evmreader: failed to update index: %w", err)

Copilot uses AI. Check for mistakes.

@@ -201,15 +205,13 @@ func (r *EvmReader) ReadAndStoreInputs(
"address", address,
"error", err,
)
dbTx.Rollback(ctx)
continue
return errors.New("evmreader: Error storing inputs and epochs")
Copy link
Preview

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

Use consistent lowercase for error messages and wrap the original error, e.g., fmt.Errorf("evmreader: error storing inputs and epochs: %w", err).

Suggested change
return errors.New("evmreader: Error storing inputs and epochs")
return fmt.Errorf("evmreader: error storing inputs and epochs: %w", err)

Copilot uses AI. Check for mistakes.

@@ -107,18 +107,24 @@ func (s *EspressoReaderService) requestNonce(w http.ResponseWriter, r *http.Requ
w.Header().Set("Access-Control-Allow-Origin", "*")
w.Header().Set("Access-Control-Allow-Headers", "Access-Control-Allow-Headers, Origin,Accept, X-Requested-With, Content-Type, Access-Control-Request-Method, Access-Control-Request-Headers")
if r.Method != http.MethodPost {
w.WriteHeader(http.StatusMethodNotAllowed)
Copy link
Preview

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

Set the Content-Type header before writing JSON, e.g., w.Header().Set("Content-Type", "application/json").

Suggested change
w.WriteHeader(http.StatusMethodNotAllowed)
w.WriteHeader(http.StatusMethodNotAllowed)
w.Header().Set("Content-Type", "application/json")

Copilot uses AI. Check for mistakes.

@@ -441,7 +454,7 @@ func (e *EspressoReader) readEspresso(ctx context.Context, appEvmType evmreader.
)
if err != nil {
slog.Error("could not store Espresso input", "err", err)
continue
return
Copy link
Preview

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

Returning here exits the entire readEspresso loop instead of skipping the failed log; consider using continue or returning an error to the caller.

Suggested change
return
continue

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no. The loop should be exited

Copy link
Contributor

Choose a reason for hiding this comment

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

no. The loop should be exited

In Go, it's common practice to return the error to the caller rather than relying on nil.
You can return the error itself if needed.

@fabiooshiro
Copy link
Collaborator

❤️ @Copilot

@ZzzzHui ZzzzHui requested review from fabiooshiro and sandhilt May 30, 2025 23:54
@ZzzzHui ZzzzHui merged commit 1357f07 into main Jun 3, 2025
4 checks passed
@ZzzzHui ZzzzHui deleted the feature/handle-error branch June 3, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants