Submessage reply can overwrite caller response

[alpe]

Resolves #495

Although there a good tests for submessages that work with wasm contracts I found it very hard to to test the reply workflow without mocks. As a result I extracted:

• MessageDispatcher to handle the message/submessage flow
• DefaultWasmVMContractResponseHandler to coordinate the dispatching order and result overwrite. This may become a nice extension point

Not related to the task but "refactored as needed" : rename of WASMVMQueryHandler

[codecov]

Codecov Report

Merging #502 (854f88c) into master (9ebeb85) will increase coverage by 0.17%.
The diff coverage is 97.11%.

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
+ Coverage   58.45%   58.62%   +0.17%     
==========================================
  Files          41       42       +1     
  Lines        4460     4474      +14     
==========================================
+ Hits         2607     2623      +16     
+ Misses       1636     1635       -1     
+ Partials      217      216       -1     

Impacted Files	Coverage Δ
x/wasm/keeper/options.go	73.33% <ø> (ø)
x/wasm/keeper/query_plugins.go	76.92% <ø> (ø)
x/wasm/keeper/msg_dispatcher.go	95.94% <95.94%> (ø)
x/wasm/keeper/keeper.go	84.97% <100.00%> (-0.74%)	⬇️
x/wasm/keeper/relay.go	100.00% <100.00%> (ø)

[ethanfrey]

Awesome work!

My one question is whether the reply interface that actually goes over FFI to a wasm contract properly distinguished between nil and []byte(""). I will look more into that.

[ethanfrey]

The refactor of types and adding a feature in one PR make it hard to see what you did. I will verify what I see.

This is https://github.com/CosmWasm/wasmd/pull/502/files#diff-4e16010ab332946722b789b22962fb5a0814d529cd8de443b925d20ccd13705fL772-L780 but now handling the rsp != nil case. Note: do we need to differentiate between nil and []byte{} here? I guess so?

[ethanfrey]

After researching wasmvm, I came to the conclusion this all works perfectly fine with the existing Go-Rust interface

[alpe]

ja, I realized later that it may not be easy to review this as it is displayed as new content. Apologies for this. Tests were not refactored though.

Note: do we need to differentiate between nil and []byte{} here

If we consider empty []bye a valid response from a contract then we need to be able to overwrite this by the reply. The nil result is considered an opt out to overwrite the response.

After researching wasmvm, I came to the conclusion this all works perfectly fine with the existing Go-Rust interface

👏

[ethanfrey]

I checked the handling of "" vs null in the Response.Data field. https://play.golang.org/p/EeiOLNtwRiR verified they are handled differently. Relevant wasmvm code:

We parse raw Rust-encoded response into a ContractResult type:

    data, gasUsed, err := api.Reply(vm.cache, checksum, envBin, replyBin, &gasMeter, store, &goapi, &querier, gasLimit, vm.printDebug)
    if err != nil {
        return nil, gasUsed, err
    }

    var resp types.ContractResult
    err = json.Unmarshal(data, &resp)

This type defines Data []byte field, does that differentiate between None (json: null) and Some(vec![]) (json: "")? The Go playgound above say yes.

// ContractResult is the raw response from the instantiate/execute/migrate calls.
// This is mirrors Rust's ContractResult<Response>.
type ContractResult struct {
    Ok  *Response `json:"ok,omitempty"`
    Err string    `json:"error,omitempty"`
}

// Response defines the return value on a successful instantiate/execute/migrate.
// This is the counterpart of [Response](https://github.com/CosmWasm/cosmwasm/blob/v0.14.0-beta1/packages/std/src/results/response.rs#L73-L88)
type Response struct {
    // Submessages are like Messages, but they guarantee a reply to the calling contract
    // after their execution, and return both success and error rather than auto-failing on error
    Submessages []SubMsg `json:"submessages"`
    // Messages comes directly from the contract and is it's request for action
    Messages []CosmosMsg `json:"messages"`
    // base64-encoded bytes to return as ABCI.Data field
    Data []byte `json:"data"`
    // attributes for a log event to return over abci interface
    Attributes []EventAttribute `json:"attributes"`
}

[alpe]

Thank you for this detailed review feedback. Especially for the time you took to look into the wasmvm response data de/serialization. Very happy that it makes sense 😄