Fix message representation for signing #658
Conversation
|
Happy if @webmaster128 takes a look here - he understands the issue better than I |
Codecov Report
@@ Coverage Diff @@
## master #658 +/- ##
==========================================
+ Coverage 59.95% 60.11% +0.16%
==========================================
Files 48 48
Lines 5324 5346 +22
==========================================
+ Hits 3192 3214 +22
+ Misses 1906 1904 -2
- Partials 226 228 +2
|
x/wasm/types/tx.go
Outdated
| if a == nil { | ||
| return errors.New("unmarshalJSON on nil pointer") | ||
| } | ||
| *a = append((*a)[0:0], b...) |
There was a problem hiding this comment.
Using this type rather than json.RawMessage everywhere gives flexibility.
What about doing ValidateBasic() in UnmarshalJSON()? Then we guarantee it is always valid
There was a problem hiding this comment.
I was thinking about this as well but then ended with not adding it here. My main driver was not mixing concerns.
In the sdk message ValidateBasic functions we would need to call RawContractMessage.ValidateBasic anyway for the cases when we initialize the objects not from json sources like protobuf or in the handler_plugin_encoders.
We are in good company here as the stdlib json type has separated this as well.
x/wasm/types/tx.go
Outdated
| if r == nil { | ||
| return []byte("null"), nil | ||
| } | ||
| return r, nil |
There was a problem hiding this comment.
Not sure if this is what Ethan actually meant, but I think we need JSON validation here. An RawContractMessage instance should hold valid JSON but may or may not have corrupted data. Even if the current flow calls ValidateBasic first, the type itself should not make assumtions how it is used. The use cases change and I can imagine use cases such as client signing where ValidateBasic is not called before the JSON representation is created.
Now in order to avoid the Cosmos logic in here, I'd use two funtions:
RawContractMessage#IsValidJSONthat is called hereRawContractMessage#ValidateBasicthat calls 1. but could also implement additional checks such as length limits.
This should address the separation of concerns mentioned in the other comment.
* Introduce RawContractMessage type * Add json signbytes test for proposals * No assumptions on MsgIBCSend.data content * Smart query uses RawContractMessage * Revert method signature change to be consistent * Review comment * Update after discussions (cherry picked from commit dfba139)
* Introduce RawContractMessage type * Add json signbytes test for proposals * No assumptions on MsgIBCSend.data content * Smart query uses RawContractMessage * Revert method signature change to be consistent * Review comment * Update after discussions
Resolves #642
This PR introduces the
RawContractMessagetype for incoming and outgoing data to a contract.I have also revisited some other places where we were using
json.RawMessagecurrently or before the change:The protobuf diffs describe the changes best