R4R: JSON-stringify ABCI Log w/ Msg Indexes

[alexanderbez]

JSON-stringify the ABCI log in BaseApp to include the message index, log, and success value. Log now contains a JSON encoded array parsable by clients.

e.g. REST response

{
    "height": "163",
    "txhash": "32B104480A93EE6CC3A0D63E567251522C5394B085C4952BA2D3FD443B481D48",
    "log": "[{\"msg_index\":\"0\",\"success\":true,\"log\":\"\"}]",
    "gas_wanted": "30497",
    "gas_used": "30477",
    "tags": [

    ]
}

e.g. CLI response

Response:
  Height: 196
  TxHash: A842BADDDC2832E24AAF252AB6FB0A276B8368C5F194CAEC60B65D91A16064FA
  Log: [{"msg_index":"0","success":true,"log":""}]
  GasWanted: 30407
  GasUsed: 30387
  Tags:
    - action = send
    - sender = cosmos1n85kjmvzxtnk6dv864gqyd662mtwkqcrql3987
    - recipient = cosmos1y8e5p7p3qt0749cke3t87dxpyq2gypdrqnyaxz

closes: #3601
closes: #2953

---

• Targeted PR against correct branch (see CONTRIBUTING.md)

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

• rereviewed 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)

[jackzampolin]

cc @cosmos/cosmos-ui

[codecov]

Codecov Report

Merging #3606 into develop will increase coverage by 0.03%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #3606      +/-   ##
===========================================
+ Coverage    60.97%   61.01%   +0.03%     
===========================================
  Files          187      187              
  Lines        14065    14069       +4     
===========================================
+ Hits          8576     8584       +8     
+ Misses        4945     4941       -4     
  Partials       544      544

[faboweb]

imo log should be a JSON object and not a string. Could you give an example of how an error would look like?

[alexanderbez]

@faboweb @alessio @jackzampolin I updated the Log to be a JSON encoded array. This should always be JSON-parsable now.

[faboweb]

I still propose log to be an actual Array/Object instead of a string. What is the reasoning for making clients parse this value?

Expected result:

{
    "height": "163",
    "txhash": "32B104480A93EE6CC3A0D63E567251522C5394B085C4952BA2D3FD443B481D48",
    "log": [{"msg_index":0,"success":true,"log":""}],
    "gas_wanted": "30497",
    "gas_used": "30477",
    "tags": [

    ]
}

[faboweb]

Also why is log an Array? The result only displays one txhash. What would be an example of multiple log entries?

[faboweb]

Could you give an example of how an error would look like?

Ping for this.

[alessio]

Also why is log an Array?

If the Tx carries multiple messages, there would be a log entry for each message - @alexanderbez correct me if I am mistaken please

[faboweb]

If the Tx carries multiple messages, there would be a log entry for each message

ah ok, but there would still just be one hash for the whole multi message tx?

[alexanderbez]

Correct one tx hash for one tx @faboweb. If you would like to change the actual type of Log that is an ABCI Tendermint change. I think this is a good middle ground solution.

[jackzampolin]

I think this is a good change that makes life better for clients of the SDK. Obviously we can't fix the root cause which requires ABCI changes. LGTM.

[faboweb]

Obviously we can't fix the root cause which requires ABCI changes.

I disagree. We can post process the ABCI message in the REST server and deserialize the string to JSON (object). This is probably one line of code.

[alexanderbez]

I don't follow @faboweb, you're ultimately returning the following:

return abci.ResponseCheckTx{  // Or ResponseDeliverTx
  Code:      uint32(result.Code),
  Data:      result.Data,
  Log:       result.Log,  // this is a string and must be per ABCI (which is already JSON encoded!!!)
  GasWanted: int64(result.GasWanted),
  GasUsed:   int64(result.GasUsed),
  Tags:      result.Tags,
}

What are you proposing to change? Can you point to some code or suggested code?

[faboweb]

result.Log is a string but it should be deserialized to be an object in the final response so we don't mix JSON and stringified JSON.

In your version:

{
    ...
    "log": "[{\"msg_index\":\"0\",\"success\":true,\"log\":\"\"}]",
    ...
}

log is a stringified JSON.

I expect:

{
    ...
    "log": [{"msg_index":0,"success":true,"log":""}],
    ...
}

here log is an array which a client can directly go through without deserializing it.

[alexanderbez]

So you want it to be a map[string]interface{}?

@faboweb if you disapprove of this PR, which I personally think is a huge step forward and should be merged, then please decline and open a separate issue in Tendermint where further discussion can take place.

[jackzampolin]

@faboweb Happy to discuss this in more detail, but as @alexanderbez mentions, there is no consistency with the return in the log field from tendermint. Everything from custom error messages to full stack traces get dumped in there and we don't have a good way to parse that out. This PR represents a step forward in providing at least marginally useful error messages to clients. Let's not let perfect be the enemy of good here.

[alessio]

ACK (pending conflicts are resolved)