Submessages drop data

[luca992]

I might be miss-understanding something. But, I think the behavior of returned data is different than the cosmwasm semantics specify they should behave here handling-the-reply:

The one critical difference with reply, is that we do not drop data. If reply returns data: Some(value) in the Response object, we will overwrite the data field returned by the caller. That is, if execute returns data: Some(b"first thought") and the reply (with all the extra information it is privy to) returns data: Some(b"better idea"), then this will be returned to the caller of execute (either the client or another transaction), just as if the original execute and returned data: Some(b"better idea"). If reply returns data: None, it will not modify any previously set data state. If there are multiple submessages all setting this, only the last one is used (they all overwrite any previous data value). As a consequence, you can use data: Some(b"") to clear previously set data. This will be represented as a JSON string instead of null and handled as any other Some value.

Imagine the scenario where you execute a transaction on contract A that returns a response with a submessage for contract B and sets no data. Then the submessage gets executed in contract B and sets some data in its own response. Then finally back in contract A, reply is called and it reads and handles the data set by contract B, and returns a response which does not set any data of its own. Finally the transaction is completed and the caller gets the transaction result.

According to the cosmwasm semantics in this scenario, from what I understand, the data set by contract B should be returned to the caller in the transaction result since it never gets overwritten by any subsequent responses.

But that is not what happens. The final result would just be data which is empty.

Tested on v1.5.1-patch.3

[liorbond]

Looks like you are right or at least it is what I can also understand from the documentation you've sent. I'll open an internal issue for us to be able to track this and fix it as soon as we can.
Is it something that is blocking you?

[luca992]

Looks like you are right or at least it is what I can also understand from the documentation you've sent. I'll open an internal issue for us to be able to track this and fix it as soon as we can. Is it something that is blocking you?

No it's not blocking me. But, it's just led to confusion in during development because the behavior doesn't match the documentation. It's the second time I've ran into inconsistencies in submessage behavior compared to other chains, I tried bringing it up the last time I ran into an issue

[liorbond]

Looks like you are right or at least it is what I can also understand from the documentation you've sent. I'll open an internal issue for us to be able to track this and fix it as soon as we can. Is it something that is blocking you?

No it's not blocking me. But, it's just led to confusion in during development because the behavior doesn't match the documentation. It's the second time I've ran into inconsistencies in submessage behavior compared to other chains, I tried bringing it up the last time I ran into an issue

For sure, sorry missed it, will totally have this fixed as soon as possible

[luca992]

Side note: the issue I brought up in that documentation pr is slightly different but related.

When reply is called on success of an instantiate submessage
msg.result.data will be null
But, on other chains they get:
msg.result.data = MsgInstantiateContractResponse protobuf binary

[luca992]

Just tested it again in a different scenario with the same result:

Localsecret logs where I create a submessage to instantiating a contract where I set data I want available in the reply function:

INFO  [enclave_contract_engine::io] Output before encryption: 
{
  "Ok": {
    "messages": [
      {
        "id": 1,
        "msg": {
          "wasm": {
            "instantiate": {
              "code_id": 61,
              "code_hash": "652fe4a0f33c34e7024a9b88b84955f3b5f755ab85d415d7ba61344a141a7425",
              "msg": "eyJuZXciOnsibmFtZSI6IkVRT1RZOjI4MTUxfFJPWUFMVFk6MCIsInN5bWJvbCI6IkVRMjgxNTFST1lBTFRZMCIsImFkbWluIjoic2VjcmV0MXB4eXd1cW5hZ3Z5NmxoNHJ5ZnNrcDd2ZnpkZGtzdjI3bHF0dTl1IiwiZW50cm9weSI6IldlJ3JlIGdvaW5nIHRvIG5lZWQgYSBiaWdnZXIgYm9hdCIsInJveWFsdHlfaW5mbyI6bnVsbCwiY29uZmlnIjp7InB1YmxpY190b2tlbl9zdXBwbHkiOnRydWUsInB1YmxpY19vd25lciI6dHJ1ZSwiZW5hYmxlX3NlYWxlZF9tZXRhZGF0YSI6bnVsbCwidW53cmFwcGVkX21ldGFkYXRhX2lzX3ByaXZhdGUiOm51bGwsIm1pbnRlcl9tYXlfdXBkYXRlX21ldGFkYXRhIjpudWxsLCJvd25lcl9tYXlfdXBkYXRlX21ldGFkYXRhIjpudWxsLCJlbmFibGVfYnVybiI6ZmFsc2V9LCJwb3N0X2luaXRfY2FsbGJhY2siOm51bGx9fQ==",
              "send": [],
              "label": "EQ:28151|ROYALTY:0-v0"
            }
          }
        },
        "gas_limit": null,
        "reply_on": "success"
      }
    ],
    "attributes": [],
    "events": [],
    "data": "eyJ0b3RhbF9zaGFyZXMiOm51bGwsImluaXRpYWxfcm95YWx0eV90b2tlbl9yZWNpcGllbnRzIjpbeyJyZWNpcGllbnQiOiJzZWNyZXQxbmxmZXlzOTA4ZWx2NWdlM241NGFoZ3o2OTI5MzVsa3Z3eHp3Y2giLCJzaGFyZXMiOm51bGx9XSwicHVibGljX21ldGFkYXRhIjpudWxsLCJwcml2YXRlX21ldGFkYXRhIjpudWxsLCJlbnRyb3B5IjoiV2UncmUgZ29pbmcgdG8gbmVlZCBhIGJpZ2dlciBib2F0Iiwicm95YWx0eV9pbmZvIjpudWxsfQ=="
  }
}

Localsecret logs after instantiating the contract. The contract sets no data:

INFO  [enclave_contract_engine::io] Output before encryption: 
{
  "Ok": {
    "messages": [],
    "attributes": [],
    "events": [],
    "data": null
  }
}

Localsecret debug print where I log msg.reply.result

INFO  [enclave_contract_engine::wasm3] debug_print: "
SubMsgResponse{    
events: [        
	Event {            
		ty: \"instantiate\",            
		attributes: [                
			Attribute {                    
				key: \"code_id\",                    
				value: \"61\",                    
				encrypted: true,                
			},                
			Attribute {                    
				key: \"contract_address\",                    
				value: "secret1l99u5f572229q9uwdeftp7aaacqw84npztnyrr\",                    
				encrypted: true,                
			},            
		],        
	},        
	Event {            
		ty: \"wasm\",            
		attributes: [                
			Attribute {                    
				key: \"contract_address\",                    
				value: \"secret1l99u5f572229q9uwdeftp7aaacqw84npztnyrr\",                    
				encrypted: true,                
			},
		],        
	},    
],    
data: None,
}
" 

Basically, the data I set that in the first message that I wanted available in Reply isn't there.

[luca992]

This seems like the issue to me:

SecretNetwork/x/compute/internal/keeper/msg_dispatcher.go

Line 255 in c3ef704

responseData = data[0]

It always just picks the first data array from data[][]. It should pick the first data array which is not an empty byte array. Or empty arrays shouldn't be added to the list in the first place.

[assafmo]

This seems like the issue to me:

SecretNetwork/x/compute/internal/keeper/msg_dispatcher.go

Line 255 in c3ef704

responseData = data[0]

It always just picks the first data array from data[][]. It should pick the first data array which is not an empty byte array. Or empty arrays shouldn't be added to the list in the first place.

Thanks for this! My concern right now is that existing contracts might rely on the faulty behavior already and in that case they'll stop functioning properly after we fix this.

[luca992]

This seems like the issue to me:

SecretNetwork/x/compute/internal/keeper/msg_dispatcher.go

Line 255 in c3ef704

responseData = data[0]

It always just picks the first data array from data[][]. It should pick the first data array which is not an empty byte array. Or empty arrays shouldn't be added to the list in the first place.

Thanks for this! My concern right now is that existing contracts might rely on the faulty behavior already and in that case they'll stop functioning properly after we fix this.

Yeah that's definitely a concern. Also, I haven't checked if other chains have this issue.. but I suspect they do as well since standard wasmd has the same implementation... unless as I mentioned other chains make sure to not add empty data arrays to data[][]

https://github.com/CosmWasm/wasmd/blob/e5049ba686ab71164a01f6e71e54347710a1f740/x/wasm/keeper/msg_dispatcher.go#L136-L147

[luca992]

Also another concern is that contracts might leak data they didn't mean to return to the user if their contract doesn't explicitly clear the data as the documentation says you should in the result of the last thing that a contract call returns

[assafmo]

I would not rely that much on the CosmWasm docs, as they had been recently deprecated. It would be amazing if you could test this behavior on a vanilla CosmWasm chain and see if it behaves differently. We forked their implementation, but had to modify behaviors a bit in order for it to play nice with our encryption protocol.