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

R4R: Parsed Err msgs #2119

Merged
merged 5 commits into from
Aug 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ BREAKING CHANGES
* SDK
* [core] \#1807 Switch from use of rational to decimal
* [types] \#1901 Validator interface's GetOwner() renamed to GetOperator()
* [types] \#2119 Parsed error messages and ABCI log errors to make them more human readable.

* Tendermint

Expand Down Expand Up @@ -51,7 +52,7 @@ IMPROVEMENTS
* [cli] #2060 removed `--select` from `block` command

* Gaia
* [x/stake] [#2023](https://github.com/cosmos/cosmos-sdk/pull/2023) Terminate iteration loop in `UpdateBondedValidators` and `UpdateBondedValidatorsFull` when the first revoked validator is encountered and perform a sanity check.
* [x/stake] [#2023](https://github.com/cosmos/cosmos-sdk/pull/2023) Terminate iteration loop in `UpdateBondedValidators` and `UpdateBondedValidatorsFull` when the first revoked validator is encountered and perform a sanity check.
* [x/auth] Signature verification's gas cost now accounts for pubkey type. [#2046](https://github.com/tendermint/tendermint/pull/2046)

* SDK
Expand Down
44 changes: 36 additions & 8 deletions types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package types

import (
"fmt"
"strings"

cmn "github.com/tendermint/tendermint/libs/common"

"github.com/cosmos/cosmos-sdk/wire"
abci "github.com/tendermint/tendermint/abci/types"
)

Expand Down Expand Up @@ -225,7 +227,11 @@ func (err *sdkError) TraceSDK(format string, args ...interface{}) Error {
// Implements ABCIError.
// Overrides err.Error.Error().
func (err *sdkError) Error() string {
return fmt.Sprintf("Error{%d:%d,%#v}", err.codespace, err.code, err.cmnError)
return fmt.Sprintf(`ERROR:
Codespace: %d
Code: %d
Message: %#v
`, err.codespace, err.code, parseCmnError(err.cmnError.Error()))
}

// Implements ABCIError.
Expand All @@ -245,13 +251,20 @@ func (err *sdkError) Code() CodeType {

// Implements ABCIError.
func (err *sdkError) ABCILog() string {
return fmt.Sprintf(`=== ABCI Log ===
Codespace: %v
Code: %v
ABCICode: %v
Error: %#v
=== /ABCI Log ===
`, err.codespace, err.code, err.ABCICode(), err.cmnError)
cdc := wire.NewCodec()
parsedErrMsg := parseCmnError(err.cmnError.Error())
jsonErr := humanReadableError{
Codespace: err.codespace,
Code: err.code,
ABCICode: err.ABCICode(),
Message: parsedErrMsg,
}
bz, er := cdc.MarshalJSON(jsonErr)
if er != nil {
panic(er)
}
stringifiedJSON := string(bz)
return stringifiedJSON
}

func (err *sdkError) Result() Result {
Expand All @@ -268,3 +281,18 @@ func (err *sdkError) QueryResult() abci.ResponseQuery {
Log: err.ABCILog(),
}
}

func parseCmnError(err string) string {
Copy link
Contributor

@ValarDragon ValarDragon Aug 23, 2018

Choose a reason for hiding this comment

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

This seems like a suboptimal way to parse this, we could have just had a better string method on cmn.error (For tendermint pr)

Copy link
Collaborator Author

@fedekunze fedekunze Aug 23, 2018

Choose a reason for hiding this comment

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

Will open a separate PR for tendermint as well to change cmn.Error.Error()

if idx := strings.Index(err, "{"); idx != -1 {
err = err[idx+1 : len(err)-1]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a good way of doing this. This means when someone comes around to update the sdk to the next tm version, they won't see a break here, which means were more likely to have this legacy code laying around forever. It should have been something that will break, with a comment for how to upgrade imo.

@cwgoes when we have multiple people reviewing can we wait a bit between last update and merge? (Timezones meant I didn't see it between last update and merge)

Copy link
Contributor

@cwgoes cwgoes Aug 23, 2018

Choose a reason for hiding this comment

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

Generally I agree - @fedekunze didn't want to wait for the Tendermint PR, and this is blocking downstream.

This could indeed be written to break intentionally. I think tracking the necessary fix in an issue is fine too.

Copy link
Contributor

Choose a reason for hiding this comment

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

return err
}

// nolint
type humanReadableError struct {
Codespace CodespaceType `json:"codespace"`
Code CodeType `json:"code"`
ABCICode ABCICodeType `json:"abci_code"`
Message string `json:"message"`
}
3 changes: 3 additions & 0 deletions types/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ func TestErrFn(t *testing.T) {
err := errFn("")
codeType := codeTypes[i]
require.Equal(t, err.Code(), codeType, "Err function expected to return proper code. tc #%d", i)
require.Equal(t, err.Codespace(), CodespaceRoot, "Err function expected to return proper codespace. tc #%d", i)
require.Equal(t, err.Result().Code, ToABCICode(CodespaceRoot, codeType), "Err function expected to return proper ABCICode. tc #%d")
require.Equal(t, err.QueryResult().Code, uint32(err.ABCICode()), "Err function expected to return proper ABCICode from QueryResult. tc #%d")
require.Equal(t, err.QueryResult().Log, err.ABCILog(), "Err function expected to return proper ABCILog from QueryResult. tc #%d")
}

require.Equal(t, ABCICodeOK, ToABCICode(CodespaceRoot, CodeOK))
Expand Down