Amino JSON representation of inner message in Msg{Instantiate,Migrate,Execute}Contract

[webmaster128]

With the change in #520 we most likely changed the JSON representation of the msg field in MsgInstantiateContract, MsgMigrateContract and MsgExecuteContract by accident. It is now a base64 encoded string of the JSON. The problem with this is that Ledger users cannot read the content of the smart contract interaction anymore. The same applies for other generic Amino JSON signers that do not have a dedicated UI for those message types.

While most of the #520 makes sense, this diff in the autogenerated x/wasm/types/tx.pb.go is problematic:

As you can see in https://play.golang.org/p/ois4zkjeRbr, the type defines the JSON serialization.

Seems like we should have a custom seralizer for those types that

1. Keeps the []byte type for the inner messages because we have no clue what is in there
2. Converts to JSON (can fail) and then embeds the message as an object as part of the signdoc generation

[ethanfrey]

Seems like we should have a custom seralizer for those types that

1. Keeps the []byte type for the inner messages because we have no clue what is in there
2. Converts to JSON (can fail) and then embeds the message as an object as part of the signdoc generation

I agree with this. Just a custom JSON encoder, right?

I think allowing non-valid json and encoding as base64 is a nice touch. So if someone did want to use eg. protobuf messages, nothing in wasmd stops them. But we optimize UX when they are JSON

[webmaster128]

Just a custom JSON encoder, right?

Yes, we only need this for signature verification, i.e. protobuf encoding -> Go struct -> JSON -> sign bytes conversion.

I think allowing non-valid json and encoding as base64 is a nice touch. So if someone did want to use eg. protobuf messages, nothing in wasmd stops them. But we optimize UX when they are JSON

I don't think we should allow this without adapting a whole lot of tooling. The readability of the signdoc is an important feature of Cosmos and we'd provide a way around it. It would require us to write extra code (if JSON validation fails fall back to base64). Not much, but still. This brings the risk that someone is tricks Go into considering something invalid JSON just to hide the content from the users.

[ethanfrey]

Fair enough

[ethanfrey]

@alpe I think this is the most important/breaking item between 0.20 and 1.0-beta

[alpe]

I have looked into the issue and the changes in #520 were causing the issues. Nice find @webmaster128
Unfortunately I don't remember what was driving the changes in that PR. 🙄

One option is to revert to the json RawMessage type as the validate methods ensure that we only accept valid json in the inner payloads but I assume I had a good reason for the refactoring ;-)
The custom marshaler would either require a custom type that represents the byte array or a lot of work in the UnmarshalJSON function of the message/proposal types to do it at that level.

I also looked a bit deeper at queries and contract responses and we have a mix of raw bytes and json.RawMessage now. A custom type with marshaler seems to be the cleanest and most extendable solution. I have implemented it in #658 to show the changes.

[webmaster128]

Unfortunately I don't remember what was driving the changes in that PR.

The problem is that the type annotations are casts. At the protobuf level no JSON type exists and all bytes are perfectly valid. Now with the casttype annotation in the generated Go struct, unvalidated bytes are casted to json.RawMessage. But the type guarantees to be a raw encoded JSON value. It must never ever hold non-JSON bytes.

If we had a validating bytes -> json.RawMessage converter, this would be fine. But a cast is unsafe.

One option is to revert to the json RawMessage type as the validate methods ensure that we only accept valid json in the inner payloads but I assume I had a good reason for the refactoring ;-)

This still gives you instances of json.RawMessage with invalid data. Even if we validate them at some point, I consider this error prone and unintuitive.

The custom marshaler would either require a custom type that represents the byte array

Right, this is what we need. Raw bytes intended to hold JSON data but not guaranteed to be valid JSON.