R4R: Parsed Err msgs #2119

fedekunze · 2018-08-22T14:21:57Z

Closes #2044 and luniehq/lunie#1131

New error formatting:

err.Error():

ERROR:
Codespace: 1
Code: 9
Message: "unknown address"

ERROR:
Codespace: 1
Code: 10
Message: "insufficient coins"

err.ABCILog():

{"codespace":1,"code":9,"abci_code":65545,"message":"unknown address"}
{"codespace":1,"code":10,"abci_code":65546,"message":"insufficient coins"}

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-08-22T14:37:08Z

Codecov Report

Merging #2119 into develop will increase coverage by 1.63%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2119      +/-   ##
===========================================
+ Coverage     62.2%   63.83%   +1.63%     
===========================================
  Files          115      113       -2     
  Lines         6874     6684     -190     
===========================================
- Hits          4276     4267       -9     
+ Misses        2314     2133     -181     
  Partials       284      284

cwgoes · 2018-08-22T16:18:53Z

types/errors.go

-Error: %#v
-=== /ABCI Log ===
-`, err.codespace, err.code, err.ABCICode(), err.cmnError)
+ parsedErrMsg := parseCmnError(err.cmnError.Error())


Hmm do we want to change this upstream instead (or additionally)?

@cwgoes you mean moving it to another util folder and export/import it ?

I mean changing the cmn.Error default format

Given that cmn.Error is our own struct, we should totally change this there imo. We can merge this for now, and fix on a latrer tm release

I can change it as well, but that would go on a separate PR for tendermint

@ValarDragon just updated the parseCmnError function so that once that we update cmn.Error.Error() on Tm it doesn't break the SDK code

cwgoes · 2018-08-22T16:28:10Z

types/errors.go

-`, err.codespace, err.code, err.ABCICode(), err.cmnError)
+ parsedErrMsg := parseCmnError(err.cmnError.Error())
+ jsonErr := newHumanReadableError(err.codespace, err.code, err.ABCICode(), parsedErrMsg)
+ bz, er := json.Marshal(jsonErr)


Can we use the Amino JSON marshal here? Keeps things more consistent.

Let's also use must marshal json

There's no MustMarshalJSON function @ValarDragon . I'll just use cdc.MarshalJSON

ValarDragon · 2018-08-23T07:25:41Z

PENDING.md

 * [x/auth] Signature verification's gas cost now accounts for pubkey type. [#2046](https://github.com/tendermint/tendermint/pull/2046)

 * SDK
 * [tools] Make get_vendor_deps deletes `.vendor-new` directories, in case scratch files are present.
+ * [types] \#2119 Parsed error messages and ABCI log errors to make them more human readable.


Would this be considered a breaking change, since it may break certain automation scripts?

ValarDragon · 2018-08-23T07:26:55Z

types/errors.go

-Error: %#v
-=== /ABCI Log ===
-`, err.codespace, err.code, err.ABCICode(), err.cmnError)
+ parsedErrMsg := parseCmnError(err.cmnError.Error())


Given that cmn.Error is our own struct, we should totally change this there imo. We can merge this for now, and fix on a latrer tm release

ValarDragon · 2018-08-23T07:28:52Z

types/errors.go

-`, err.codespace, err.code, err.ABCICode(), err.cmnError)
+ parsedErrMsg := parseCmnError(err.cmnError.Error())
+ jsonErr := newHumanReadableError(err.codespace, err.code, err.ABCICode(), parsedErrMsg)
+ bz, er := json.Marshal(jsonErr)


Let's also use must marshal json

ValarDragon · 2018-08-23T07:30:07Z

types/errors.go

@@ -268,3 +275,25 @@ func (err *sdkError) QueryResult() abci.ResponseQuery {
 Log: err.ABCILog(),
 }
 }
+
+func parseCmnError(err string) string {


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

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

ValarDragon · 2018-08-23T07:32:09Z

types/errors.go

+ Message string `json:"message"`
+}
+
+func newHumanReadableError(codespace CodespaceType, code CodeType, ABCICode ABCICodeType, msg string) HumanReadableError {


A new method seems unnecessary here.

ValarDragon · 2018-08-23T07:32:34Z

types/errors.go

+}
+
+// nolint
+type HumanReadableError struct {


Let's not export this, it shouldn't be used anywhere else.

…edekunze/2044-JSON-err-msgs Merge develop

addressed comments

cwgoes

utACK, let's remove parseCmnError when we update upstream

faboweb · 2018-08-23T11:55:32Z

Good work @fedekunze

fedekunze · 2018-08-23T12:26:37Z

Thanks @faboweb ! 🙌

ValarDragon · 2018-08-23T14:12:01Z

types/errors.go

+func parseCmnError(err string) string {
+ if idx := strings.Index(err, "{"); idx != -1 {
+ err = err[idx+1 : len(err)-1]
+ }


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)

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.

Federico Kunze added 2 commits August 22, 2018 16:15

Parsed Error msgs and added tests

092a178

Delete unnecesary line

11ee255

fedekunze requested a review from faboweb August 22, 2018 14:21

fedekunze requested review from cwgoes, ebuchman and rigelrozanski as code owners August 22, 2018 14:21

Update PENDING.md

b02ecf9

fedekunze added C:ABCI ready-for-review labels Aug 22, 2018

cwgoes reviewed Aug 22, 2018

View reviewed changes

ValarDragon previously requested changes Aug 23, 2018

View reviewed changes

Federico Kunze added 2 commits August 23, 2018 11:31

Merge branch 'develop' of https://github.com/cosmos/cosmos-sdk into f…

1ec9d16

…edekunze/2044-JSON-err-msgs Merge develop

Addressed Chris and Dev's comments

f143c92

cwgoes approved these changes Aug 23, 2018

View reviewed changes

cwgoes merged commit aa82f6f into develop Aug 23, 2018

cwgoes deleted the fedekunze/2044-JSON-err-msgs branch August 23, 2018 11:55

fedekunze mentioned this pull request Aug 23, 2018

Parse LCD errors in the lcdClient luniehq/lunie#1131

Closed

ValarDragon reviewed Aug 23, 2018

View reviewed changes

cwgoes mentioned this pull request Aug 23, 2018

Remove parseCmnError on new Tendermint version #2133

Closed

faboweb mentioned this pull request Aug 29, 2018

test if errors work in voyager luniehq/lunie#1192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Parsed Err msgs #2119

R4R: Parsed Err msgs #2119

fedekunze commented Aug 22, 2018

codecov bot commented Aug 22, 2018 •

edited

Loading

cwgoes Aug 22, 2018

fedekunze Aug 22, 2018

cwgoes Aug 22, 2018

ValarDragon Aug 23, 2018

fedekunze Aug 23, 2018

fedekunze Aug 23, 2018

cwgoes Aug 22, 2018

ValarDragon Aug 23, 2018

fedekunze Aug 23, 2018 •

edited

Loading

ValarDragon Aug 23, 2018

ValarDragon Aug 23, 2018

ValarDragon Aug 23, 2018

ValarDragon Aug 23, 2018 •

edited

Loading

fedekunze Aug 23, 2018 •

edited

Loading

ValarDragon Aug 23, 2018

ValarDragon Aug 23, 2018

cwgoes left a comment •

edited

Loading

faboweb commented Aug 23, 2018

fedekunze commented Aug 23, 2018

ValarDragon Aug 23, 2018

cwgoes Aug 23, 2018 •

edited

Loading

cwgoes Aug 23, 2018

R4R: Parsed Err msgs #2119

R4R: Parsed Err msgs #2119

Conversation

fedekunze commented Aug 22, 2018

codecov bot commented Aug 22, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

fedekunze Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment • edited Loading

Choose a reason for hiding this comment

faboweb commented Aug 23, 2018

fedekunze commented Aug 23, 2018

Choose a reason for hiding this comment

cwgoes Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2018 •

edited

Loading

fedekunze Aug 23, 2018 •

edited

Loading

ValarDragon Aug 23, 2018 •

edited

Loading

fedekunze Aug 23, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading

cwgoes Aug 23, 2018 •

edited

Loading