Skip to content

Commit

Permalink
refactor: improve error messages (#11762)
Browse files Browse the repository at this point in the history
## Description

Closes: #XXXX

---

### 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...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers 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.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 8f51644)

# Conflicts:
#	errors/CHANGELOG.md
#	errors/abci.go
  • Loading branch information
fedekunze authored and mergify[bot] committed May 6, 2022
1 parent 162fb49 commit 5fab741
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 6 deletions.
51 changes: 51 additions & 0 deletions errors/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<!--
Guiding Principles:
Changelogs are for humans, not machines.
There should be an entry for every single version.
The same types of changes should be grouped.
Versions and sections should be linkable.
The latest version comes first.
The release date of each version is displayed.
Mention whether you follow Semantic Versioning.
Usage:
Change log entries are to be added to the Unreleased section under the
appropriate stanza (see below). Each entry should ideally include a tag and
the Github issue reference in the following format:
* (<tag>) [#<issue-number>] Changelog message.
Types of changes (Stanzas):
"Features" for new features.
"Improvements" for changes in existing functionality.
"Deprecated" for soon-to-be removed features.
"Bug Fixes" for any bug fixes.
"API Breaking" for breaking exported APIs used by developers building on SDK.
Ref: https://keepachangelog.com/en/1.0.0/
-->

# Changelog

## [Unreleased]

### Improvements

* [\#11762](https://github.com/cosmos/cosmos-sdk/pull/11762) Improve error messages

## v1.0.0

### Features

* [\#10779](https://github.com/cosmos/cosmos-sdk/pull/10779) Import code from the `github.com/cosmos/cosmos-sdk/types/errors` package.
* [\#11274](https://github.com/cosmos/cosmos-sdk/pull/11274) Add `RegisterWithGRPCCode` function to associate a gRPC error code with errors.

### API Breaking

* [\#11274](https://github.com/cosmos/cosmos-sdk/pull/11274) `New` now is an alias for `Register` and should only be used in initialization code.

### Bug Fixes

* [\#11714](https://github.com/cosmos/cosmos-sdk/pull/11714) Add wrapped error messages in `GRPCStatus()`
129 changes: 129 additions & 0 deletions errors/abci.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
package errors

import (
"fmt"
"reflect"
)

const (
// SuccessABCICode declares an ABCI response use 0 to signal that the
// processing was successful and no error is returned.
SuccessABCICode uint32 = 0

// All unclassified errors that do not provide an ABCI code are clubbed
// under an internal error code and a generic message instead of
// detailed error string.
internalABCICodespace = UndefinedCodespace
internalABCICode uint32 = 1
)

// ABCIInfo returns the ABCI error information as consumed by the tendermint
// client. Returned codespace, code, and log message should be used as a ABCI response.
// Any error that does not provide ABCICode information is categorized as error
// with code 1, codespace UndefinedCodespace
// When not running in a debug mode all messages of errors that do not provide
// ABCICode information are replaced with generic "internal error". Errors
// without an ABCICode information as considered internal.
func ABCIInfo(err error, debug bool) (codespace string, code uint32, log string) {
if errIsNil(err) {
return "", SuccessABCICode, ""
}

encode := defaultErrEncoder
if debug {
encode = debugErrEncoder
}

return abciCodespace(err), abciCode(err), encode(err)
}

// The debugErrEncoder encodes the error with a stacktrace.
func debugErrEncoder(err error) string {
return fmt.Sprintf("%+v", err)
}

// The defaultErrEncoder applies Redact on the error before encoding it with its internal error message.
func defaultErrEncoder(err error) string {
return Redact(err).Error()
}

type coder interface {
ABCICode() uint32
}

// abciCode tests if given error contains an ABCI code and returns the value of
// it if available. This function is testing for the causer interface as well
// and unwraps the error.
func abciCode(err error) uint32 {
if errIsNil(err) {
return SuccessABCICode
}

for {
if c, ok := err.(coder); ok {
return c.ABCICode()
}

if c, ok := err.(causer); ok {
err = c.Cause()
} else {
return internalABCICode
}
}
}

type codespacer interface {
Codespace() string
}

// abciCodespace tests if given error contains a codespace and returns the value of
// it if available. This function is testing for the causer interface as well
// and unwraps the error.
func abciCodespace(err error) string {
if errIsNil(err) {
return ""
}

for {
if c, ok := err.(codespacer); ok {
return c.Codespace()
}

if c, ok := err.(causer); ok {
err = c.Cause()
} else {
return internalABCICodespace
}
}
}

// errIsNil returns true if value represented by the given error is nil.
//
// Most of the time a simple == check is enough. There is a very narrowed
// spectrum of cases (mostly in tests) where a more sophisticated check is
// required.
func errIsNil(err error) bool {
if err == nil {
return true
}
if val := reflect.ValueOf(err); val.Kind() == reflect.Ptr {
return val.IsNil()
}
return false
}

var errPanicWithMsg = Wrapf(ErrPanic, "error message redacted to hide potential sensitive info. Use the '--trace' flag if you are running a node to see the full stack trace error")

// Redact replaces an error that is not initialized as an ABCI Error with a
// generic internal error instance. If the error is an ABCI Error, that error is
// simply returned.
func Redact(err error) error {
if ErrPanic.Is(err) {
return errPanicWithMsg
}
if abciCode(err) == internalABCICode {
return errInternal
}

return err
}
10 changes: 5 additions & 5 deletions types/decimal.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func NewDecFromIntWithPrec(i Int, prec int64) Dec {
// CONTRACT - This function does not mutate the input str.
func NewDecFromStr(str string) (Dec, error) {
if len(str) == 0 {
return Dec{}, ErrEmptyDecimalStr
return Dec{}, fmt.Errorf("%s: %w", str, ErrEmptyDecimalStr)
}

// first extract any negative symbol
Expand All @@ -154,7 +154,7 @@ func NewDecFromStr(str string) (Dec, error) {
}

if len(str) == 0 {
return Dec{}, ErrEmptyDecimalStr
return Dec{}, fmt.Errorf("%s: %w", str, ErrEmptyDecimalStr)
}

strs := strings.Split(str, ".")
Expand All @@ -172,7 +172,7 @@ func NewDecFromStr(str string) (Dec, error) {
}

if lenDecs > Precision {
return Dec{}, fmt.Errorf("invalid precision; max: %d, got: %d", Precision, lenDecs)
return Dec{}, fmt.Errorf("value '%s' exceeds max precision by %d decimal places: max precision %d", str, Precision-lenDecs, Precision)
}

// add some extra zero's to correct to the Precision factor
Expand All @@ -182,10 +182,10 @@ func NewDecFromStr(str string) (Dec, error) {

combined, ok := new(big.Int).SetString(combinedStr, 10) // base 10
if !ok {
return Dec{}, fmt.Errorf("failed to set decimal string: %s", combinedStr)
return Dec{}, fmt.Errorf("failed to set decimal string with base 10: %s", combinedStr)
}
if combined.BitLen() > maxDecBitLen {
return Dec{}, fmt.Errorf("decimal out of range; bitLen: got %d, max %d", combined.BitLen(), maxDecBitLen)
return Dec{}, fmt.Errorf("decimal '%s' out of range; bitLen: got %d, max %d", str, combined.BitLen(), maxDecBitLen)
}
if neg {
combined = new(big.Int).Neg(combined)
Expand Down
2 changes: 1 addition & 1 deletion types/errors/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ func (s *abciTestSuite) TestABCIInfoSerializeErr() {
},
"redact in default encoder": {
src: myPanic,
exp: "panic message redacted to hide potentially sensitive system info: panic",
exp: "error message redacted to hide potential sensitive info. Use the '--trace' flag if you are running a node to see the full stack trace error: panic",
},
"do not redact in debug encoder": {
src: myPanic,
Expand Down

0 comments on commit 5fab741

Please sign in to comment.