Properly fold results #381
Conversation
ethanfrey
force-pushed
the
properly-fold-results
branch
from
3d3fc8f
to
dc0a106
Compare
hashedone
force-pushed
the
properly-fold-results
branch
3 times, most recently
from
a32b64f
to
c82285a
Compare
|
I tested also #36 with this change, seems to be working. |
hashedone
force-pushed
the
properly-fold-results
branch
from
c82285a
to
2440816
Compare
packages/multi-test/src/wasm.rs
Outdated
| @@ -512,7 +512,12 @@ where | |||
| let subres = | |||
| self.execute_submsg(api, router, storage, block, contract.clone(), resend)?; | |||
| events.extend_from_slice(&subres.events); | |||
| Ok::<_, String>(subres.data.or(data)) | |||
|
|
|||
| if data.is_some() { | |||
There was a problem hiding this comment.
This may cover the test cases, but is not the general solution we need.
It still makes no distinction between data returned in the sub message execute and in the reply processing submessage execute
There was a problem hiding this comment.
It does, just not in this very place. execute_submsg is modified, so it doesn't return data at all if reply was not executed, as in such a case those data are never to be used. At some point I even added checking for ReplyOn here, but when went deeper in this I realized it doesn't make sense - the logic behind deciding if reply is to be executed doesn't belong here, instead if there is no reply, then what is visible from execute_submsg should actually match what we expect - no data is set (as in such a case also wasmd implementation does not process this result in any way).
| contract.clone(), | ||
| &echo::Message { | ||
| data: Some("First".to_owned()), | ||
| sub_msg: vec![make_echo_submsg_no_reply(contract, "Second", vec![])], |
There was a problem hiding this comment.
This does look like it passes. Hmm...
| @@ -389,28 +389,26 @@ where | |||
| }); | |||
|
|
|||
| // call reply if meaningful | |||
| if let Ok(r) = res { | |||
| if let Ok(mut r) = res { | |||
There was a problem hiding this comment.
This if-let implementation was actually root cause of the problem. Previously when there was a submsg response received, but no reply was called, replay was returned including data received from submsg execution. It does not make sense, as submsg execution result is not propagated directly, it is only send to reply handled and forget, those in case when there is response from submsg, but no reply is called, Ok(None) is now returned here.
|
I think there is still errors here. Please look at #384. It is a minimal change (building off lots of tests from other PRs). It fixes all tests and solves the issue. But I think it is a bit ugly. |
|
This is strict equivalent of my change. I am pretty sure that you missed that I also altered The only actual difference is, that you overwrite data on the one from reply also in case, when original message returned |
… was not returned from execution in multi-test
This shows an issue coming up with multi-test
If the top-level has a reply statement, it may return a new data to over-ride what was previously set with execute / instantiate / migrate.
However, if the submessage returns a result, but we don't set the data in reply, it must not override the top level reply.
This has lead to improper contract data being returned in some cases (one instantiate calls another instantiate, the second data is returned on the top level).
My "quick fix" here broke the reply-set-data functionality completely, and it seems a bit tricky to address. There is a broken test case however that should show a working solution when it passes.
@hashedone I'd be happy if you take a look and push to this branch if you see fit.