Cosmos-sdk: Intelligently format transaction responses & errors in CLI

[cwgoes]

• We don't need to show code / codespace / etc. - those are for code consumption, not human consumption
• Write reverse mapping functions for the error codes, display the strings we actually set in the module errors (which are generally informative)

cc @jackzampolin

[cwgoes]

This might require changing the upstream library (some error type in tendermint/libs IIRC).

[truted]

Cosmos message failures return an inconsistent ABCI Log value. In some instances the Log message is prefixed with Msg 0 failed: [json_error_string] while others just have [json_error_string]. The Msg 0 failed: defeats the purpose of having a json error string since the log can no longer be JSON parsed.

Without prefix:

"{\"codespace\":1,\"code\":12,\"abci_code\":65548,\"message\":\"out of gas in location: WritePerByte\"}"

"{\"codespace\":1,\"code\":3,\"abci_code\":65539,\"message\":\"Invalid sequence. Got 4, expected 5\"}"

With prefix:

"Msg 0 failed: {\"codespace\":10,\"code\":1004,\"abci_code\":656364,\"message\":\"Custom message\"}"

The Msg 0 failed: prefix is appended when an error is returned inside a message handler such as during msg.ValidateBasic(). I think it happens here:

https://github.com/cosmos/cosmos-sdk/blob/243576143e3c72e6c6d60699323a341f549b5ad7/baseapp/baseapp.go#L630

[faboweb]

Yeeeeeeeees 🚀

[alexanderbez]

@cwgoes is this needed prelaunch? Can we come up with a standardized format in regards to the response (e.g. {codeMappedString}: {errorMessage})? Or do we still want JSON with only a message here?

[jackzampolin]

@alexanderbez It would be great to have prelaunch. This issue pretty significantly affects the user experience on the CLI. Ideally the errors can be formatted in a couple of different ways (string format[s] would be nice too), and we aren't displaying codespace info.

[alexanderbez]

What I'm asking for is essentially what is the actionable item here? Do we still want JSON? Do we want plaintext? If so, what format? etc...

[alexanderbez]

So the Msg {#} is prefixed when the msg gets actually executed, otherwise, if it fails during the ante handler, it doesn't get prefixed and that makes sense/is intended behavior since you haven't run any message yet. So in BroadcastTxAndAwaitCommit which the CLI and REST clients use, I'd have no way of knowing unless I do a regex.

[jackzampolin]

@alexanderbez What about removing the prefix all together to allow for parsing?

[alexanderbez]

But if we remove the prefix, you wont know what error is for what message when you have multi-msg txs.

[alexanderbez]

We could have a struct that is {MsgIndex: {d}, Log: {msgResult.Log}} but this would require JSON serialization for every message executed.

[truted]

Can the log be an array with one entry for every message? It looks like currently its a long string with a \n between each message log: Log: strings.Join(logs, "\n"),. Using an array would remove the need to prefix the logs with the corresponding message index. The array index now indicates which message it corresponds to. If you received a code that execution failed you could check the message at the last array index to figure out which one went wrong and why.

[alexanderbez]

Perhaps, but in either case, this is an ABCI change that must happen in Tendermint first. An issue should be opened to change the type of Log to maybe a struct of some sort and discussion should take place there :)

[jackzampolin]

So I took a peek at this this morning. It seems like the stringification of the SDK errors happens in types/errors.go. We could change the type of Log to be []Log (I think this is what @alexanderbez is suggesting) where log is a struct like:

type Log struct {
  MsgIdx int
  MsgLog string
}

This doesn't account for tendermint errors however. Those will be passed error back in the cmnError field of the sdk.Result.Error. That is defined in tendermint. The string representation is formed by naively formatting an unexported struct.

So it seems like we _could_ improve the way the SDK reports errors, but there will still be an issue with the way tendermint returns errors tho. Its unclear to me why the change to TM would require ABCI changes. We could just switch to a format thats parsable by the SDK right?

[alexanderbez]

Yes, correct. It's because Log is used in the Result return types defined by ABCI.

[cwgoes]

cc @alessio thoughts / want to work on this?

[aaronc]

This issue is related to the PR I'm working on #3276.

Currently, that PR is effectively an "escape hatch" for the current way CLI tx responses are formatted so that I can do useful formatting when I know what type of response to expect.

These things may have been discussed elsewhere, but as an SDK user I just want to share what I would like to see in terms of intelligent tx response formatting (in addition to what's already been discussed here):

• Tags printed as strings, it seems that #3277 will at least partially address that
• The ability to optionally interpret Data as a string
• Possibly outputting JSON as the default. The current %+v formatting doesn't seem too helpful
• Elimination of unneeded fields from the response such as XXX_NoUnkeyedLiteral
• FeeAmount and FeeDenom recovered from sdk.Result
• An escape hatch like in #3276 when the default simply doesn't meet the SDK user's needs

Hope this is helpful.

[alessio]

Possibly outputting JSON as the default. The current %+v formatting doesn't seem too helpful

Not as default, please. JSON as an option is most definitely a must have, we also need human-readable version format.

[aaronc]

Possibly outputting JSON as the default. The current %+v formatting doesn't seem too helpful

Not as default, please. JSON as an option is most definitely a must have, we also need human-readable version format.

How about something like:

Tags:
  action: send
GasUsed: 2000
GasWanted: 2500
...

Which is effectively yaml, but human readable

[alessio]

Makes sense for transaction responses

[aaronc]

So I'd like to make a concrete proposal for the default, human readable transaction response printing in the case when there are no errors. Not quite yaml but close (Tags for one isn't actually a map):

Transaction XXXXXXXXXXXXXXXX Committed at Block NNNNN
Fees: NNNN denom
Gas Used/Wanted: 2000/2500
Data: sdxbrwh25abldg351
Log: some message
Tags:
  tag1: abcd
  tag2: efgh

If any of the fields Data, Log or Tags are empty they just don't get printed. Currently we can't recover fees from ABCI ResponseDeliverTx, but it would be nice to have it in the future.

By default Tags print as strings and Data gets printed as base64. But that behavior could be tweaked by this struct field on CLIContext:

type TxResponseFormatting struct {
    // makes Data print as a string in the default transaction response formatting
    // Data gets printed as base64 by default
    StringData bool

    // makes Tags print as binary in the default transaction response formatting
    // Tags get printed as strings by default
    BinaryTags bool
}

Thoughts?

[alexanderbez]

Looks like #3451 will close this ultimately.

[alexanderbez]

I'm not sure we want to fully close this issue (perhaps we can track further progress via #3601), but afaik we have not fully tackled the issue of failed messages and the way they're rendered in the ABCI log (not proper JSON).