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

feat: implement custom error types for DA #115

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

tzdybal
Copy link
Member

@tzdybal tzdybal commented Oct 3, 2024

Overview

  • replaced sentinel error with types
  • defined common error codes
  • added error mapping for JSON-RPC - codes are used on the transport layer
  • refactored ErrTooHigh into ErrFutureHeight (renamed, moved, added error code)
  • added GRPCStatus handling to error types - gRPC well-known codes are used and da-codes are passed as detail.
  • fixed error handling in tests

Resolves #65

Summary by CodeRabbit

  • New Features

    • Introduced structured error handling for JSON-RPC and gRPC protocols, enhancing clarity in error reporting.
    • Added new error codes and types for better management of blob and transaction errors.
    • Enhanced client and server error handling with mappings for known errors.
  • Bug Fixes

    • Updated error handling in the implementation to use specific error types instead of generic messages.
  • Tests

    • Improved test assertions to check for specific error types, enhancing robustness in error handling tests.

Introduced multiple custom error types for the DA package.
Because of JSON-RPC library used, each error has to be it's own type (can't use sentinel values).
gRPC on the other hand doesn't support wrapping typed errors, and codes are "standardized" (currently only "Not Found" used).
Copy link

coderabbitai bot commented Oct 3, 2024

Warning

Rate limit exceeded

@tzdybal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 33 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Files that changed from the base of the PR and between 181b674 and 61b9787.

Walkthrough

The changes introduce a comprehensive error handling mechanism across multiple files, focusing on JSON-RPC and gRPC protocols. A new file errors.go defines custom error types and codes for various transaction and blob-related errors. The Protocol Buffers definition is updated to include an enumeration for these error codes. Modifications in client and server files enhance error mapping and handling, ensuring structured responses.

Changes

File Change Summary
errors.go Introduced custom error types and codes for JSON-RPC and gRPC, including Code type and various error constants. Added getGRPCStatus function and GRPCStatus() methods for error types.
proto/da/da.proto Added ErrorCode enum and ErrorDetails message for structured error handling in DAService.
proxy/grpc/errors.go Introduced tryToMapError and errorForCode functions for enhanced gRPC error handling.
proxy/jsonrpc/client.go Updated newClient function to use jsonrpc.NewMergeClient for integrated error handling. Added getKnownErrorsMapping function.
proxy/jsonrpc/errors.go Added getKnownErrorsMapping function to map known error codes to error types.
proxy/jsonrpc/server.go Modified NewServer function to include error mapping options for server error handling.

Poem

In the land of code where errors roam,
We crafted a map to guide them home.
With blobs and transactions, we’ve set the scene,
A structured approach, crisp and clean.
So hop along, dear bugs, don’t be shy,
For with our new errors, you’ll surely fly! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@tzdybal
Copy link
Member Author

tzdybal commented Oct 3, 2024

I'll try to define protobuf for error (code/enum) and use WithDetails to avoid matching by strings.

Implemented detailed gRPC error handling by defining custom error types and appropriate error codes for better differentiation in the DA package. Each error type now provides a gRPC status with granular error details using the newly introduced error codes.
Introduced a new error code and corresponding error type for when a requested height is from the future. Updated functions to handle this new type and included its gRPC status representation.
Previously, only `Get` method supported proper error mapping.
@tzdybal tzdybal marked this pull request as ready for review October 6, 2024 22:10
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (8)
proxy/jsonrpc/errors.go (1)

11-21: Consider ordering error registrations for improved readability.

The current order of error registrations doesn't follow a clear pattern. Consider ordering them alphabetically by error code or grouping them by related functionality. This would improve readability and make it easier to maintain the list in the future.

Here's an example of alphabetical ordering:

errs := jsonrpc.NewErrors()
errs.Register(jsonrpc.ErrorCode(da.CodeBlobNotFound), new(*da.ErrBlobNotFound))
errs.Register(jsonrpc.ErrorCode(da.CodeBlobSizeOverLimit), new(*da.ErrBlobSizeOverLimit))
errs.Register(jsonrpc.ErrorCode(da.CodeContextDeadline), new(*da.ErrContextDeadline))
errs.Register(jsonrpc.ErrorCode(da.CodeFutureHeight), new(*da.ErrFutureHeight))
errs.Register(jsonrpc.ErrorCode(da.CodeTxAlreadyInMempool), new(*da.ErrTxAlreadyInMempool))
errs.Register(jsonrpc.ErrorCode(da.CodeTxIncorrectAccountSequence), new(*da.ErrTxIncorrectAccountSequence))
errs.Register(jsonrpc.ErrorCode(da.CodeTxTimedOut), new(*da.ErrTxTimedOut))
errs.Register(jsonrpc.ErrorCode(da.CodeTxTooLarge), new(*da.ErrTxTooLarge))
proto/da/da.proto (1)

133-148: Add documentation for new error handling elements

The new ErrorCode enum and ErrorDetails message are well-integrated into the existing proto file. To further improve the documentation, consider adding comments explaining their purpose and usage.

Here's a suggested addition:

// ErrorCode defines specific error scenarios that can occur in the DA service.
// Codes in the 32xxx range are reserved for custom DA-specific errors.
enum ErrorCode {
  // ... (enum values as previously suggested)
}

// ErrorDetails encapsulates error information to be included in gRPC responses.
message ErrorDetails {
  ErrorCode code = 1;
}

These comments provide context for developers who will be working with these new elements.

🧰 Tools
🪛 buf

135-135: Enum value name "Unknown" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


135-135: Enum value name "Unknown" should be UPPER_SNAKE_CASE, such as "UNKNOWN".

(ENUM_VALUE_UPPER_SNAKE_CASE)


135-135: Enum zero value name "Unknown" should be suffixed with "_UNSPECIFIED".

(ENUM_ZERO_VALUE_SUFFIX)


136-136: Enum value name "BlobNotFound" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


136-136: Enum value name "BlobNotFound" should be UPPER_SNAKE_CASE, such as "BLOB_NOT_FOUND".

(ENUM_VALUE_UPPER_SNAKE_CASE)


137-137: Enum value name "BlobSizeOverLimit" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


137-137: Enum value name "BlobSizeOverLimit" should be UPPER_SNAKE_CASE, such as "BLOB_SIZE_OVER_LIMIT".

(ENUM_VALUE_UPPER_SNAKE_CASE)


138-138: Enum value name "TxTimedOut" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


138-138: Enum value name "TxTimedOut" should be UPPER_SNAKE_CASE, such as "TX_TIMED_OUT".

(ENUM_VALUE_UPPER_SNAKE_CASE)


139-139: Enum value name "TxAlreadyInMempool" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


139-139: Enum value name "TxAlreadyInMempool" should be UPPER_SNAKE_CASE, such as "TX_ALREADY_IN_MEMPOOL".

(ENUM_VALUE_UPPER_SNAKE_CASE)


140-140: Enum value name "TxIncorrectAccountSequence" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


140-140: Enum value name "TxIncorrectAccountSequence" should be UPPER_SNAKE_CASE, such as "TX_INCORRECT_ACCOUNT_SEQUENCE".

(ENUM_VALUE_UPPER_SNAKE_CASE)


141-141: Enum value name "TxTooLarge" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


141-141: Enum value name "TxTooLarge" should be UPPER_SNAKE_CASE, such as "TX_TOO_LARGE".

(ENUM_VALUE_UPPER_SNAKE_CASE)


142-142: Enum value name "ContextDeadline" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


142-142: Enum value name "ContextDeadline" should be UPPER_SNAKE_CASE, such as "CONTEXT_DEADLINE".

(ENUM_VALUE_UPPER_SNAKE_CASE)


143-143: Enum value name "FutureHeight" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


143-143: Enum value name "FutureHeight" should be UPPER_SNAKE_CASE, such as "FUTURE_HEIGHT".

(ENUM_VALUE_UPPER_SNAKE_CASE)

test/test_suite.go (2)

144-144: Improved error assertion in ConcurrentReadWriteTest

The change enhances the error assertion by using assert.ErrorIs to check for the specific da.ErrFutureHeight error type. This is a good improvement that aligns with the PR's objective of using custom error types.

For consistency, consider updating the error variable name from err to a more descriptive name, such as errFutureHeight. This would make the code even more readable:

errFutureHeight := d.GetIDs(ctx, i, []byte{})
if errFutureHeight != nil {
    assert.ErrorIs(t, errFutureHeight, &da.ErrFutureHeight{})
}

165-165: Improved error assertion in HeightFromFutureTest

The change enhances the error assertion by using assert.ErrorIs to check for the specific da.ErrFutureHeight error type. This improvement aligns well with the PR's objective of using custom error types and improving error handling.

For consistency with other tests and to provide more context, consider adding an error message to the assertion:

assert.ErrorIs(t, err, &da.ErrFutureHeight{}, "Expected ErrFutureHeight when querying a future height")

This addition would make the test output more informative if the assertion fails.

proxy/grpc/client.go (1)

Line range hint 1-163: Summary: Consistent error handling improvements with areas for verification.

The changes in this file consistently apply tryToMapError to standardize error handling across client methods, aligning with the PR objective. This approach enhances error management and potentially allows for more structured error responses.

Points to verify:

  1. The implementation of tryToMapError and its impact on error types returned to callers.
  2. The slightly different error return pattern in the Validate method and its handling by callers.
  3. Ensure that these changes do not break existing error handling in the codebase that depends on these client methods.

Consider documenting the new error handling approach, including the purpose and behavior of tryToMapError, to maintain clarity for future development and debugging.

proxy/grpc/errors.go (2)

51-52: Provide more informative error messages in the default case.

In the default case of the errorForCode function, the returned error message is "unknown error code". Including the actual error code in the message can aid in debugging and provide clearer context.

Apply this diff to enhance the error message:

 default:
-    return errors.New("unknown error code")
+    return fmt.Errorf("unknown error code: %v", code)
 }

Remember to import the fmt package if it's not already included:

 import (
     "errors"
+    "fmt"

3-10: Optimize import statements by grouping and ordering.

For improved readability and maintenance, group standard library imports separately from third-party imports and order them alphabetically within their groups.

Apply this diff to reorganize the imports:

 import (
-    "errors"
-
-    "google.golang.org/grpc/status"
-
     "errors"
+    "fmt"

+    "google.golang.org/grpc/status"
+
     "github.com/rollkit/go-da"
     pbda "github.com/rollkit/go-da/types/pb/da"
 )
errors.go (1)

69-69: Grammar Correction: Adjust wording in the comment

To improve clarity, change "when context deadline exceeds" to "when the context deadline is exceeded".

Apply this diff:

-// ErrContextDeadline is the error message returned by the DA when context deadline exceeds.
+// ErrContextDeadline is the error message returned by the DA when the context deadline is exceeded.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between df792b1 and 0240d62.

⛔ Files ignored due to path filters (1)
  • types/pb/da/da.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (9)
  • errors.go (1 hunks)
  • proto/da/da.proto (1 hunks)
  • proxy/grpc/client.go (7 hunks)
  • proxy/grpc/errors.go (1 hunks)
  • proxy/jsonrpc/client.go (1 hunks)
  • proxy/jsonrpc/errors.go (1 hunks)
  • proxy/jsonrpc/server.go (1 hunks)
  • test/dummy.go (2 hunks)
  • test/test_suite.go (3 hunks)
🧰 Additional context used
🪛 buf
proto/da/da.proto

135-135: Enum value name "Unknown" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


135-135: Enum value name "Unknown" should be UPPER_SNAKE_CASE, such as "UNKNOWN".

(ENUM_VALUE_UPPER_SNAKE_CASE)


135-135: Enum zero value name "Unknown" should be suffixed with "_UNSPECIFIED".

(ENUM_ZERO_VALUE_SUFFIX)


136-136: Enum value name "BlobNotFound" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


136-136: Enum value name "BlobNotFound" should be UPPER_SNAKE_CASE, such as "BLOB_NOT_FOUND".

(ENUM_VALUE_UPPER_SNAKE_CASE)


137-137: Enum value name "BlobSizeOverLimit" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


137-137: Enum value name "BlobSizeOverLimit" should be UPPER_SNAKE_CASE, such as "BLOB_SIZE_OVER_LIMIT".

(ENUM_VALUE_UPPER_SNAKE_CASE)


138-138: Enum value name "TxTimedOut" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


138-138: Enum value name "TxTimedOut" should be UPPER_SNAKE_CASE, such as "TX_TIMED_OUT".

(ENUM_VALUE_UPPER_SNAKE_CASE)


139-139: Enum value name "TxAlreadyInMempool" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


139-139: Enum value name "TxAlreadyInMempool" should be UPPER_SNAKE_CASE, such as "TX_ALREADY_IN_MEMPOOL".

(ENUM_VALUE_UPPER_SNAKE_CASE)


140-140: Enum value name "TxIncorrectAccountSequence" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


140-140: Enum value name "TxIncorrectAccountSequence" should be UPPER_SNAKE_CASE, such as "TX_INCORRECT_ACCOUNT_SEQUENCE".

(ENUM_VALUE_UPPER_SNAKE_CASE)


141-141: Enum value name "TxTooLarge" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


141-141: Enum value name "TxTooLarge" should be UPPER_SNAKE_CASE, such as "TX_TOO_LARGE".

(ENUM_VALUE_UPPER_SNAKE_CASE)


142-142: Enum value name "ContextDeadline" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


142-142: Enum value name "ContextDeadline" should be UPPER_SNAKE_CASE, such as "CONTEXT_DEADLINE".

(ENUM_VALUE_UPPER_SNAKE_CASE)


143-143: Enum value name "FutureHeight" should be prefixed with "ERROR_CODE_".

(ENUM_VALUE_PREFIX)


143-143: Enum value name "FutureHeight" should be UPPER_SNAKE_CASE, such as "FUTURE_HEIGHT".

(ENUM_VALUE_UPPER_SNAKE_CASE)

🔇 Additional comments (20)
proxy/jsonrpc/errors.go (3)

3-7: LGTM: Imports are appropriate and concise.

The imported packages are relevant to the function's purpose. go-jsonrpc is used for error handling, and go-da is the source of error codes and types.


9-10: LGTM: Function signature is well-defined.

The function getKnownErrorsMapping() has a clear, descriptive name and returns the appropriate type jsonrpc.Errors. Its unexported status (lowercase first letter) is suitable for internal use within the package.


11-21: Verify completeness of error mappings.

It's important to ensure that all relevant error codes from the go-da package are mapped in this function. Please verify that no essential error codes are missing from this list.

To help with this verification, you can run the following script:

This script will help you compare the error codes defined in the go-da package with those registered in getKnownErrorsMapping. Please review the output to ensure all necessary error codes are included.

proxy/jsonrpc/server.go (1)

35-35: Enhance error handling with known error mapping

The addition of jsonrpc.WithServerErrors(getKnownErrorsMapping()) aligns well with the PR objectives of implementing custom error types. This change will improve error reporting in the JSON-RPC server.

To ensure the correctness of this change, please verify the following:

  1. The getKnownErrorsMapping() function is properly defined and imported.
  2. Client-side error handling is updated to handle the new error format, if necessary.

Run the following script to verify the getKnownErrorsMapping() function:

Consider adding a comment explaining the purpose of this new option, for example:

// Use custom error mapping to enhance error reporting
rpc := jsonrpc.NewServer(jsonrpc.WithServerErrors(getKnownErrorsMapping()))

Ensure that the documentation is updated to reflect these changes in error handling. This will help maintain consistency between the code and its documentation, making it easier for developers to understand and use the updated error reporting system.

✅ Verification successful

Enhance error handling with known error mapping - Verified

The addition of jsonrpc.WithServerErrors(getKnownErrorsMapping()) has been verified. The getKnownErrorsMapping() function is properly defined in proxy/jsonrpc/errors.go, and no missing imports were found. This change improves error reporting in the JSON-RPC server.

Consider adding a comment explaining the purpose of this new option, for example:

// Use custom error mapping to enhance error reporting
rpc := jsonrpc.NewServer(jsonrpc.WithServerErrors(getKnownErrorsMapping()))

Ensure that the documentation is updated to reflect these changes in error handling. This will help maintain consistency between the code and its documentation, making it easier for developers to understand and use the updated error reporting system.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence and import of getKnownErrorsMapping() function

# Test 1: Search for the function definition
echo "Searching for getKnownErrorsMapping() function definition:"
rg --type go "func getKnownErrorsMapping\(\)"

# Test 2: Check for imports in the current file
echo "Checking imports in proxy/jsonrpc/server.go:"
rg --type go "^import \(" proxy/jsonrpc/server.go -A 10

Length of output: 572

proto/da/da.proto (1)

146-148: LGTM: ErrorDetails message

The ErrorDetails message is well-structured and appropriate for conveying error information. It effectively uses the ErrorCode enum, providing a clear and concise way to include error details in responses.

test/test_suite.go (2)

84-86: Improved error handling and assertion

The changes in the CheckErrors function enhance the error handling and assertion:

  1. The error message for an invalid blob ID is now more specific ("invalid blob id" instead of just "invalid").
  2. An additional assertion using assert.ErrorIs checks for the specific error type da.ErrBlobNotFound.

These improvements align well with the PR objective of enhancing error handling and using custom error types. The code is now more robust and provides better error information.


Line range hint 1-167: Overall improvements in error handling and assertions

The changes in this file consistently enhance the test suite by:

  1. Utilizing custom error types (da.ErrBlobNotFound, da.ErrFutureHeight) instead of generic errors.
  2. Improving assertions with assert.ErrorIs to check for specific error types.
  3. Providing more descriptive error messages.

These improvements align well with the PR objectives of enhancing error handling and using custom error types. The test suite is now more robust and provides better error information, which will help in identifying and debugging issues more effectively.

proxy/grpc/client.go (7)

62-62: LGTM. Consistent error handling.

The change maintains consistency in error handling across client methods by using tryToMapError.


73-73: LGTM. Consistent error handling maintained.

The change continues the consistent use of tryToMapError for standardized error handling across client methods.


106-106: LGTM. Error handling consistency maintained.

The change continues the pattern of using tryToMapError for consistent error handling across client methods.


122-122: LGTM. Consistent error handling approach.

The change maintains the consistent use of tryToMapError for standardized error handling in the Submit method.


144-144: LGTM. Error handling consistency maintained.

The change in the SubmitWithOptions method continues the pattern of using tryToMapError for consistent error handling.


163-163: LGTM. Note the different error return pattern.

The change maintains consistency in using tryToMapError. However, note that in this method, the error is returned alongside the response results, which differs from other methods. Ensure this pattern is intentional and properly handled by callers.

To verify the consistency of error handling in callers, please run:

✅ Verification successful

Verification Successful: Consistent Error Handling Confirmed.
All callers of the Validate method properly handle the returned errors, either by asserting no errors or by appropriately propagating them. The change to return errors alongside response results maintains consistency across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent error handling in Validate method callers

# Test: Search for calls to the Validate method and their error handling
rg --type go -A 5 'Validate\(.*\)' | grep -v 'func.*Validate'

Length of output: 4293


46-46: LGTM. Verify tryToMapError implementation.

The change standardizes error handling by using tryToMapError. This is consistent with the PR objective to enhance error handling.

To ensure the correct implementation of tryToMapError, please run the following script:

✅ Verification successful

Verified the tryToMapError implementation. The function correctly standardizes error handling by appropriately mapping gRPC errors, ensuring consistency across client methods. No issues found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of tryToMapError function

# Test: Search for the tryToMapError function definition
rg --type go -A 10 'func tryToMapError'

Length of output: 539


Script:

#!/bin/bash
# Description: Retrieve the full implementation of the tryToMapError function

rg --type go -A 20 'func tryToMapError'

Length of output: 909

test/dummy.go (3)

78-78: Improved error handling with custom error type

The change from a string-based error to da.ErrBlobNotFound{} is a good improvement. It aligns with the PR objective of using custom error types, which enhances error handling and makes it easier to handle specific error cases programmatically.


90-90: Enhanced error handling with descriptive custom error type

The replacement of ErrTooHigh with da.ErrFutureHeight{} is a positive change. It not only aligns with the PR objective of using custom error types but also provides a more descriptive name for the error condition. This improvement enhances code readability and makes error handling more robust.


Line range hint 1-180: Verify error handling in dependent code

The changes to error handling in DummyDA improve the overall error management by using custom error types. This aligns well with the PR objectives and enhances the robustness of the error handling mechanism.

To ensure smooth integration:

  1. Verify that all code dependent on DummyDA is updated to handle these new error types correctly.
  2. Update any relevant documentation to reflect these changes in error handling.

To help with the verification, you can run the following script to find potential places where the error handling might need to be updated:

This will help identify areas where DummyDA is used and error handling is performed, which may need to be updated to handle the new error types.

✅ Verification successful

Error handling in dependent code is properly managed

The shell script results indicate that there are no external dependencies using DummyDA that require updates to handle the new custom error types. Therefore, the changes to error handling in DummyDA are verified and pose no issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for uses of DummyDA and potential error handling
rg --type go "DummyDA" -C 5 | rg "if err != nil"

Length of output: 79

proxy/grpc/errors.go (1)

32-53: Handle all possible error codes in errorForCode.

Currently, the errorForCode function does not cover all potential error codes defined in pbda.ErrorCode. To ensure all error codes are appropriately mapped, consider adding cases for any missing error codes or implementing a default case that provides more context about the unknown code.

Run the following script to list all error codes not handled:

proxy/jsonrpc/client.go (2)

114-114: Ensure correct usage of jsonrpc.NewMergeClient with error mapping.

On line 114, the jsonrpc.NewMergeClient function is called with the additional option jsonrpc.WithErrors(errs). Verify that the errs variable is structured correctly as per the expectations of jsonrpc.WithErrors(). This ensures that custom error handling is integrated properly with the JSON-RPC client.

Check the go-jsonrpc package documentation to confirm that jsonrpc.WithErrors(errs) accepts the correct format and that errs matches the required structure.


112-112: Verify the existence and accessibility of getKnownErrorsMapping().

On line 112, the function getKnownErrorsMapping() is called, but its implementation is not shown in this file. Please ensure that getKnownErrorsMapping() is properly defined and accessible within the codebase to prevent any compilation errors.

Run the following script to confirm that getKnownErrorsMapping is defined:

✅ Verification successful

getKnownErrorsMapping() is properly defined and accessible within proxy/jsonrpc/client.go, ensuring no compilation issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for the definition of `getKnownErrorsMapping` in Go files.

# Test: Look for the function definition. Expect: At least one match.
rg --type go --no-filename --no-line-number --context 2 'func\s+getKnownErrorsMapping\s*\('

Length of output: 362


Script:

#!/bin/bash
# Description: Verify if the package containing `getKnownErrorsMapping` is imported in client.go.

# Step 1: Find the file path where `getKnownErrorsMapping` is defined.
definition_file=$(rg --type go --files-with-matches 'func\s+getKnownErrorsMapping\s*\(' | head -n 1)

# Step 2: Extract the package name from the definition file.
package_name=$(rg --type go '^package\s+' "$definition_file" | awk '{print $2}')

# Step 3: Check if this package is imported in proxy/jsonrpc/client.go.
rg --type go "import\s+\(\s+\"$package_name\"\s+\)" proxy/jsonrpc/client.go

Length of output: 294

proxy/jsonrpc/errors.go Outdated Show resolved Hide resolved
proto/da/da.proto Outdated Show resolved Hide resolved
proxy/grpc/errors.go Show resolved Hide resolved
proxy/grpc/errors.go Show resolved Hide resolved
proxy/grpc/errors.go Outdated Show resolved Hide resolved
errors.go Outdated Show resolved Hide resolved
errors.go Outdated Show resolved Hide resolved
errors.go Outdated Show resolved Hide resolved
Centralize the error code to error type mapping by creating a reusable `getKnownErrorsMapping` function. This change reduces code redundancy and improves maintainability.
Unified the indentation style for all enum entries in ErrorCode. This improves code readability and consistency within the proto file.
@tzdybal tzdybal force-pushed the tzdybal/errors branch 2 times, most recently from e00bb86 to 0974e2a Compare October 6, 2024 22:30
@@ -130,3 +130,19 @@ message ValidateRequest {
message ValidateResponse {
repeated bool results = 1;
}

enum ErrorCode {
ERROR_CODE_UNSPECIFIED = 0;
Copy link
Member

Choose a reason for hiding this comment

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

when does this happen? doesn't look like it is being used anywhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: For Review
Development

Successfully merging this pull request may close these issues.

[Feature Request]: standard error codes
2 participants