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.

Sign up for GitHub

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

chore: move abci errors to baseapp #20756

Merged
merged 2 commits into from
Jun 25, 2024
Merged

chore: move abci errors to baseapp #20756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2620b8e
remove abci.go from types//errors
tac0turtle Jun 24, 2024
4835065
add changelog
tac0turtle Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* (client) [#20255](https://github.com/cosmos/cosmos-sdk/pull/20255) Use comet proofOp proto type instead of sdk version to avoid needing to translate to later be proven in the merkle proof runtime.
* (all) [#19726](https://github.com/cosmos/cosmos-sdk/pull/19726) Integrate comet v1
* (client) [#20616](https://github.com/cosmos/cosmos-sdk/pull/20616) gentx subcommand output goes to `cmd.ErrOrStderr()` instead of being hardcoded to `os.Stderr`
* (types/errors) [#20756](https://github.com/cosmos/cosmos-sdk/pull/20756) Remove `ResponseCheckTxWithEvents`, `ResponseExecTxResultWithEvents` & `QueryResult` from types/errors pkg. They have been moved to `baseapp/errors.go` and made private
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix trailing spaces in the CHANGELOG entry.

There's a markdown lint issue due to trailing spaces at the end of the entry. It's important to adhere to markdown best practices to maintain the quality of the documentation.

- * (types/errors) [#20756](https://github.com/cosmos/cosmos-sdk/pull/20756) Remove `ResponseCheckTxWithEvents`, `ResponseExecTxResultWithEvents` & `QueryResult` from types/errors pkg. They have been moved to `baseapp/errors.go` and made private 
+ * (types/errors) [#20756](https://github.com/cosmos/cosmos-sdk/pull/20756) Remove `ResponseCheckTxWithEvents`, `ResponseExecTxResultWithEvents` & `QueryResult` from types/errors pkg. They have been moved to `baseapp/errors.go` and made private
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* (types/errors) [#20756](https://github.com/cosmos/cosmos-sdk/pull/20756) Remove `ResponseCheckTxWithEvents`, `ResponseExecTxResultWithEvents` & `QueryResult` from types/errors pkg. They have been moved to `baseapp/errors.go` and made private
* (types/errors) [#20756](https://github.com/cosmos/cosmos-sdk/pull/20756) Remove `ResponseCheckTxWithEvents`, `ResponseExecTxResultWithEvents` & `QueryResult` from types/errors pkg. They have been moved to `baseapp/errors.go` and made private


### Client Breaking Changes

Expand Down
34 changes: 17 additions & 17 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func (app *BaseApp) Query(_ context.Context, req *abci.QueryRequest) (resp *abci
// Ref: https://github.com/cosmos/cosmos-sdk/pull/8039
defer func() {
if r := recover(); r != nil {
resp = sdkerrors.QueryResult(errorsmod.Wrapf(sdkerrors.ErrPanic, "%v", r), app.trace)
resp = queryResult(errorsmod.Wrapf(sdkerrors.ErrPanic, "%v", r), app.trace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor to use a more descriptive error message.

The error message in queryResult could be more descriptive to provide better context about the error.

- resp = queryResult(errorsmod.Wrapf(sdkerrors.ErrPanic, "%v", r), app.trace)
+ resp = queryResult(errorsmod.Wrapf(sdkerrors.ErrPanic, "Query recovery from panic: %v", r), app.trace)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resp = queryResult(errorsmod.Wrapf(sdkerrors.ErrPanic, "%v", r), app.trace)
resp = queryResult(errorsmod.Wrapf(sdkerrors.ErrPanic, "Query recovery from panic: %v", r), app.trace)

}
}()

Expand All @@ -180,7 +180,7 @@ func (app *BaseApp) Query(_ context.Context, req *abci.QueryRequest) (resp *abci
defer telemetry.MeasureSince(telemetry.Now(), req.Path)

if req.Path == QueryPathBroadcastTx {
return sdkerrors.QueryResult(errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "can't route a broadcast tx message"), app.trace), nil
return queryResult(errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "can't route a broadcast tx message"), app.trace), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider improving error message clarity.

The error message "can't route a broadcast tx message" could be more specific to explain why the routing cannot be done.

- return queryResult(errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "can't route a broadcast tx message"), app.trace), nil
+ return queryResult(errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "BroadcastTx messages are not supported in this context"), app.trace), nil
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return queryResult(errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "can't route a broadcast tx message"), app.trace), nil
return queryResult(errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "BroadcastTx messages are not supported in this context"), app.trace), nil

}

// handle gRPC routes first rather than calling splitPath because '/' characters
Expand All @@ -191,7 +191,7 @@ func (app *BaseApp) Query(_ context.Context, req *abci.QueryRequest) (resp *abci

path := SplitABCIQueryPath(req.Path)
if len(path) == 0 {
return sdkerrors.QueryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "no query path provided"), app.trace), nil
return queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "no query path provided"), app.trace), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve error message for better user guidance.

The error message "no query path provided" could provide guidance on what paths are expected.

- return queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "no query path provided"), app.trace), nil
+ return queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "No query path provided. Please specify a valid query path."), app.trace), nil
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "no query path provided"), app.trace), nil
return queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "No query path provided. Please specify a valid query path."), app.trace), nil

}

switch path[0] {
Expand All @@ -206,7 +206,7 @@ func (app *BaseApp) Query(_ context.Context, req *abci.QueryRequest) (resp *abci
resp = handleQueryP2P(app, path)

default:
resp = sdkerrors.QueryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "unknown query path"), app.trace)
resp = queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "unknown query path"), app.trace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve error message for unknown paths.

The error message could be more informative by suggesting possible correct paths.

- resp = queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "unknown query path"), app.trace)
+ resp = queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "Unknown query path. Please refer to the documentation for supported paths."), app.trace)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resp = queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "unknown query path"), app.trace)
resp = queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "Unknown query path. Please refer to the documentation for supported paths."), app.trace)

}

return resp, nil
Expand Down Expand Up @@ -365,7 +365,7 @@ func (app *BaseApp) CheckTx(req *abci.CheckTxRequest) (*abci.CheckTxResponse, er

gInfo, result, anteEvents, err := app.runTx(mode, req.Tx)
if err != nil {
return sdkerrors.ResponseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace), nil
return responseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Inconsistent use of responseCheckTxWithEvents for error handling.

The function responseCheckTxWithEvents is not used consistently across similar contexts for error handling. Here are some instances where responses are returned directly without using responseCheckTxWithEvents:

  • server/v2/cometbft/utils.go
  • baseapp/abci.go

Please ensure that responseCheckTxWithEvents is used uniformly for consistency in error handling.

Analysis chain

Ensure consistency in error handling.

The function responseCheckTxWithEvents is used here for error handling. Ensure that it is used consistently across similar contexts for error handling to maintain uniformity.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent use of `responseCheckTxWithEvents` across similar contexts.

# Test: Search for `responseCheckTxWithEvents` usage. Expect: Consistent usage in similar error handling contexts.
rg --type go 'responseCheckTxWithEvents'

Length of output: 396

Script:

#!/bin/bash
# Description: Retrieve the definition of `responseCheckTxWithEvents` to understand its purpose and usage.

# Extract the function definition of `responseCheckTxWithEvents` from baseapp/errors.go
ast-grep --lang go --pattern 'func responseCheckTxWithEvents($_, $_, $_, $_, $_) $_ { $$$ }' baseapp/errors.go

Length of output: 112

Script:

#!/bin/bash
# Description: Retrieve the definition of `responseCheckTxWithEvents` to understand its purpose and usage.

# Search for the function definition of `responseCheckTxWithEvents` in baseapp/errors.go and extract the relevant lines.
rg 'func responseCheckTxWithEvents' baseapp/errors.go -A 20

Length of output: 807

Script:

#!/bin/bash
# Description: Verify consistent use of `responseCheckTxWithEvents` across similar contexts.

# Search for functions that handle errors and return responses in the codebase.
rg --type go 'return &abci\.(CheckTxResponse|ExecTxResult)' -A 5

Length of output: 1267

}

return &abci.CheckTxResponse{
Expand Down Expand Up @@ -825,7 +825,7 @@ func (app *BaseApp) internalFinalizeBlock(ctx context.Context, req *abci.Finaliz
// In the case where a transaction included in a block proposal is malformed,
// we still want to return a default response to comet. This is because comet
// expects a response for each transaction included in a block proposal.
response = sdkerrors.ResponseExecTxResultWithEvents(
response = responseExecTxResultWithEvents(
sdkerrors.ErrTxDecode,
0,
0,
Expand Down Expand Up @@ -1023,7 +1023,7 @@ func handleQueryApp(app *BaseApp, path []string, req *abci.QueryRequest) *abci.Q

gInfo, res, err := app.Simulate(txBytes)
if err != nil {
return sdkerrors.QueryResult(errorsmod.Wrap(err, "failed to simulate tx"), app.trace)
return queryResult(errorsmod.Wrap(err, "failed to simulate tx"), app.trace)
}

simRes := &sdk.SimulationResponse{
Expand All @@ -1033,7 +1033,7 @@ func handleQueryApp(app *BaseApp, path []string, req *abci.QueryRequest) *abci.Q

bz, err := codec.ProtoMarshalJSON(simRes, app.interfaceRegistry)
if err != nil {
return sdkerrors.QueryResult(errorsmod.Wrap(err, "failed to JSON encode simulation response"), app.trace)
return queryResult(errorsmod.Wrap(err, "failed to JSON encode simulation response"), app.trace)
}

return &abci.QueryResponse{
Expand All @@ -1050,11 +1050,11 @@ func handleQueryApp(app *BaseApp, path []string, req *abci.QueryRequest) *abci.Q
}

default:
return sdkerrors.QueryResult(errorsmod.Wrapf(sdkerrors.ErrUnknownRequest, "unknown query: %s", path), app.trace)
return queryResult(errorsmod.Wrapf(sdkerrors.ErrUnknownRequest, "unknown query: %s", path), app.trace)
}
}

return sdkerrors.QueryResult(
return queryResult(
errorsmod.Wrap(
sdkerrors.ErrUnknownRequest,
"expected second parameter to be either 'simulate' or 'version', neither was present",
Expand All @@ -1065,13 +1065,13 @@ func handleQueryStore(app *BaseApp, path []string, req abci.QueryRequest) *abci.
// "/store" prefix for store queries
queryable, ok := app.cms.(storetypes.Queryable)
if !ok {
return sdkerrors.QueryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "multi-store does not support queries"), app.trace)
return queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "multi-store does not support queries"), app.trace)
}

req.Path = "/" + strings.Join(path[1:], "/")

if req.Height <= 1 && req.Prove {
return sdkerrors.QueryResult(
return queryResult(
errorsmod.Wrap(
sdkerrors.ErrInvalidRequest,
"cannot query with proof when height <= 1; please provide a valid height",
Expand All @@ -1081,7 +1081,7 @@ func handleQueryStore(app *BaseApp, path []string, req abci.QueryRequest) *abci.
sdkReq := storetypes.RequestQuery(req)
resp, err := queryable.Query(&sdkReq)
if err != nil {
return sdkerrors.QueryResult(err, app.trace)
return queryResult(err, app.trace)
}
resp.Height = req.Height

Expand All @@ -1093,7 +1093,7 @@ func handleQueryStore(app *BaseApp, path []string, req abci.QueryRequest) *abci.
func handleQueryP2P(app *BaseApp, path []string) *abci.QueryResponse {
// "/p2p" prefix for p2p queries
if len(path) < 4 {
return sdkerrors.QueryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "path should be p2p filter <addr|id> <parameter>"), app.trace)
return queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "path should be p2p filter <addr|id> <parameter>"), app.trace)
}

var resp *abci.QueryResponse
Expand All @@ -1110,7 +1110,7 @@ func handleQueryP2P(app *BaseApp, path []string) *abci.QueryResponse {
}

default:
resp = sdkerrors.QueryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "expected second parameter to be 'filter'"), app.trace)
resp = queryResult(errorsmod.Wrap(sdkerrors.ErrUnknownRequest, "expected second parameter to be 'filter'"), app.trace)
}

return resp
Expand Down Expand Up @@ -1166,12 +1166,12 @@ func (app *BaseApp) getContextForProposal(ctx sdk.Context, height int64) sdk.Con
func (app *BaseApp) handleQueryGRPC(handler GRPCQueryHandler, req *abci.QueryRequest) *abci.QueryResponse {
ctx, err := app.CreateQueryContext(req.Height, req.Prove)
if err != nil {
return sdkerrors.QueryResult(err, app.trace)
return queryResult(err, app.trace)
}

resp, err := handler(ctx, req)
if err != nil {
resp = sdkerrors.QueryResult(gRPCErrorToSDKError(err), app.trace)
resp = queryResult(gRPCErrorToSDKError(err), app.trace)
resp.Height = req.Height
return resp
}
Expand Down
2 changes: 1 addition & 1 deletion baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,7 @@ func (app *BaseApp) deliverTx(tx []byte) *abci.ExecTxResult {
gInfo, result, anteEvents, err := app.runTx(execModeFinalize, tx)
if err != nil {
resultStr = "failed"
resp = sdkerrors.ResponseExecTxResultWithEvents(
resp = responseExecTxResultWithEvents(
err,
gInfo.GasWanted,
gInfo.GasUsed,
Expand Down
14 changes: 7 additions & 7 deletions types/errors/abci.go → baseapp/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
package errors
package baseapp

import (
abci "github.com/cometbft/cometbft/api/cometbft/abci/v1"

errorsmod "cosmossdk.io/errors"
)

// ResponseCheckTxWithEvents returns an ABCI ResponseCheckTx object with fields filled in
// responseCheckTxWithEvents returns an ABCI ResponseCheckTx object with fields filled in
// from the given error, gas values and events.
func ResponseCheckTxWithEvents(err error, gw, gu uint64, events []abci.Event, debug bool) *abci.CheckTxResponse {
func responseCheckTxWithEvents(err error, gw, gu uint64, events []abci.Event, debug bool) *abci.CheckTxResponse {
space, code, log := errorsmod.ABCIInfo(err, debug)
return &abci.CheckTxResponse{
Codespace: space,
Expand All @@ -20,9 +20,9 @@ func ResponseCheckTxWithEvents(err error, gw, gu uint64, events []abci.Event, de
}
}

// ResponseExecTxResultWithEvents returns an ABCI ExecTxResult object with fields
// responseExecTxResultWithEvents returns an ABCI ExecTxResult object with fields
// filled in from the given error, gas values and events.
func ResponseExecTxResultWithEvents(err error, gw, gu uint64, events []abci.Event, debug bool) *abci.ExecTxResult {
func responseExecTxResultWithEvents(err error, gw, gu uint64, events []abci.Event, debug bool) *abci.ExecTxResult {
space, code, log := errorsmod.ABCIInfo(err, debug)
return &abci.ExecTxResult{
Codespace: space,
Expand All @@ -34,9 +34,9 @@ func ResponseExecTxResultWithEvents(err error, gw, gu uint64, events []abci.Even
}
}

// QueryResult returns a ResponseQuery from an error. It will try to parse ABCI
// queryResult returns a ResponseQuery from an error. It will try to parse ABCI
// info from the error.
func QueryResult(err error, debug bool) *abci.QueryResponse {
func queryResult(err error, debug bool) *abci.QueryResponse {
space, code, log := errorsmod.ABCIInfo(err, debug)
return &abci.QueryResponse{
Codespace: space,
Expand Down
Loading