fix(server/v2/cometbft): proper query error #22746
Conversation
julienrbrt
added
the
backport/v0.52.x
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces changes to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
github-actions
bot
added
C:server/v2
julienrbrt
requested review from
a team,
facundomedica and
sontrinh16
as code owners
|
@julienrbrt your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/v2/cometbft/utils.go (1)
273-291: Consider enhancing error mappingThe gRPC to SDK error mapping could be improved in the following ways:
- Add mapping for additional gRPC codes (e.g.,
Unavailable,DeadlineExceeded)- Consider adding error context preservation
func gRPCErrorToSDKError(err error) error { status, ok := grpcstatus.FromError(err) if !ok { return sdkerrors.ErrInvalidRequest.Wrap(err.Error()) } switch status.Code() { case codes.NotFound: return sdkerrors.ErrKeyNotFound.Wrap(err.Error()) case codes.InvalidArgument: return sdkerrors.ErrInvalidRequest.Wrap(err.Error()) case codes.FailedPrecondition: return sdkerrors.ErrInvalidRequest.Wrap(err.Error()) case codes.Unauthenticated: return sdkerrors.ErrUnauthorized.Wrap(err.Error()) + case codes.Unavailable: + return sdkerrors.ErrInvalidRequest.Wrap("service temporarily unavailable: " + err.Error()) + case codes.DeadlineExceeded: + return sdkerrors.ErrInvalidRequest.Wrap("request timeout: " + err.Error()) default: return sdkerrors.ErrUnknownRequest.Wrap(err.Error()) } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/abci.go(3 hunks)server/v2/cometbft/go.mod(1 hunks)server/v2/cometbft/utils.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
server/v2/cometbft/go.mod (1)
24-24: LGTM: Direct dependency on errors package
Making cosmossdk.io/errors a direct dependency is appropriate given the enhanced error handling being implemented.
server/v2/cometbft/utils.go (2)
23-23: LGTM: Clear documentation of error package usage
The comment explains why v2 of errors package isn't used, providing good context for future maintainers.
Line range hint 263-270: LGTM: Function renamed to follow Go conventions
Renaming QueryResult to queryResult correctly reflects its package-private scope.
server/v2/cometbft/abci.go (2)
183-183: LGTM: Consistent error handling in Query method
The error handling is now consistent throughout the Query method, properly using queryResult for all error cases.
Also applies to: 197-197, 201-201
286-291: LGTM: Improved gRPC error handling
The error handling now properly converts gRPC errors to SDK errors, maintaining the query height in the response.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/utils.go (2)
273-300: Consider handling additional gRPC status codesThe error mapping is good but could be enhanced by handling additional common gRPC codes:
codes.Unavailablefor service unavailabilitycodes.DeadlineExceededfor timeout scenarioscodes.Internalfor internal errors (instead of usingErrUnknownRequestin default case)func gRPCErrorToSDKError(err error) *abci.QueryResponse { // ... existing code ... switch status.Code() { // ... existing cases ... + case codes.Unavailable: + return toQueryResp(sdkerrors.ErrServiceUnavailable, err) + case codes.DeadlineExceeded: + return toQueryResp(sdkerrors.ErrTimeout, err) + case codes.Internal: + return toQueryResp(sdkerrors.ErrInternalError, err) default: return toQueryResp(sdkerrors.ErrUnknownRequest, err) } }🧰 Tools
🪛 golangci-lint (1.62.2)
280-280: File is not
gofumpt-ed with-extra(gofumpt)
274-281: Consider extracting the helper functionThe
toQueryResphelper function could be extracted to package level for better testability and reuse. This would also improve readability by separating concerns.+func toQueryResp(sdkErr *errorsmod.Error, err error) *abci.QueryResponse { + return &abci.QueryResponse{ + Code: sdkErr.ABCICode(), + Codespace: sdkErr.Codespace(), + Log: err.Error(), + } +} + func gRPCErrorToSDKError(err error) *abci.QueryResponse { - toQueryResp := func(sdkErr *errorsmod.Error, err error) *abci.QueryResponse { - return &abci.QueryResponse{ - Code: sdkErr.ABCICode(), - Codespace: sdkErr.Codespace(), - Log: err.Error(), - } - } // ... rest of the function }🧰 Tools
🪛 golangci-lint (1.62.2)
280-280: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci.go(3 hunks)server/v2/cometbft/utils.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/abci.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/utils.go
280-280: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (2)
server/v2/cometbft/utils.go (2)
15-16: LGTM: Well-structured import changes
The new imports are appropriately organized and the comment explains the rationale for using errorsmod instead of errors/v2.
Also applies to: 23-23, 27-27
Line range hint 263-271: LGTM: Appropriate function visibility change
Good change to make the function unexported as it's an internal utility. The function maintains its core functionality while following Go naming conventions.
Co-authored-by: mmsqe <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/utils.go (2)
273-306: Add documentation for error mapping logicThe function implements comprehensive gRPC to SDK error mapping, but would benefit from documentation explaining the mapping rationale and when it's used.
Add a doc comment like:
+// gRPCErrorToSDKError converts gRPC errors to appropriate SDK errors while preserving +// the original error message. It maps common gRPC status codes to their SDK counterparts +// to ensure consistent error handling across the application. func gRPCErrorToSDKError(err error) *abci.QueryResponse {🧰 Tools
🪛 golangci-lint (1.62.2)
286-286: File is not
gofumpt-ed with-extra(gofumpt)
274-286: Consider simplifying the nested functionThe nested
toQueryRespfunction could be simplified and potentially moved out for better readability.Consider this refactoring:
-func gRPCErrorToSDKError(err error) *abci.QueryResponse { - toQueryResp := func(sdkErr *errorsmod.Error, err error) *abci.QueryResponse { - res := &abci.QueryResponse{ - Code: sdkErr.ABCICode(), - Codespace: sdkErr.Codespace(), - } - type grpcStatus interface{ GRPCStatus() *grpcstatus.Status } - if grpcErr, ok := err.(grpcStatus); ok { - res.Log = grpcErr.GRPCStatus().Message() - } else { - res.Log = err.Error() - } - return res - } +// toQueryResp creates an ABCI QueryResponse from an SDK error and the original error +func toQueryResp(sdkErr *errorsmod.Error, err error) *abci.QueryResponse { + res := &abci.QueryResponse{ + Code: sdkErr.ABCICode(), + Codespace: sdkErr.Codespace(), + } + + if grpcErr, ok := err.(interface{ GRPCStatus() *grpcstatus.Status }); ok { + res.Log = grpcErr.GRPCStatus().Message() + } else { + res.Log = err.Error() + } + return res +} + +func gRPCErrorToSDKError(err error) *abci.QueryResponse {🧰 Tools
🪛 golangci-lint (1.62.2)
286-286: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
server/v2/cometbft/utils.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/utils.go
286-286: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (2)
server/v2/cometbft/utils.go (2)
15-16: LGTM: Import changes align with error handling improvements
The new imports appropriately support gRPC error handling integration, and the comment explains the rationale for using errorsmod instead of errors/v2.
Also applies to: 23-23, 27-27
Line range hint 263-271: LGTM: Appropriate function visibility change
The renaming of QueryResult to queryResult correctly reflects its internal usage scope while maintaining the existing error handling functionality.
Co-authored-by: mmsqe <[email protected]> (cherry picked from commit 556102c)
Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #22742
Replicates baseapp module query error handling:
cosmos-sdk/baseapp/abci.go
Lines 1200 to 1232 in 99dd55b
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
errorspackage.