Properly handling data in submessages in multi-test #369
Conversation
hashedone
force-pushed
the
352-repsonse-data-handling
branch
from
a289ed3 to
2eedb20
Compare
|
Events get appended (they should already!). Data may get overwritten if set to Some(_). Only the Response of I would merge this (once linting fixed), and I'd be happy to see your echo contract used to test all events (+attributed) being properly propagated up. If they don't, then yes, please fix that as well. (That can be another PR - test and possibly fix events) |
packages/multi-test/src/app.rs
Outdated
| contract: Addr, | ||
| data: impl Into<Option<&'static str>>, | ||
| sub_msg: Vec<SubMsg<Binary>>, | ||
| ) -> SubMsg<Binary> { |
There was a problem hiding this comment.
Why do you need SubMsg<Binary>?
They type T here just controls the type of CosmosMsg::Custom variant, so we can compile with code that uses it. We return an error if anyone actually returns those variants in multi-test (this is a future TODO to support it)
| _env: Env, | ||
| _info: MessageInfo, | ||
| _msg: EmptyMsg, | ||
| ) -> Result<Response<Binary>, StdError> { |
There was a problem hiding this comment.
Commented above, but I am quite sure there is no need for Response<Binary> unless you do something like:
let msg = CosmosMsg::Custom(Binary::from(b"foo"));
res.add_message(msg);The type here is unused unless you use some blockchain-specific custom return types. Which is only the case for tgrade-specific contracts (PoE), terra-specific contracts, etc. Not for any tests. Multi-test does not support those messages when returned
| msg: Message, | ||
| ) -> Result<Response<Binary>, StdError> { | ||
| let mut resp = Response::new(); | ||
| if let Some(data) = msg.data { |
| to_binary(&msg) | ||
| } | ||
|
|
||
| fn reply(_deps: DepsMut, _env: Env, msg: Reply) -> Result<Response<Binary>, StdError> { |
There was a problem hiding this comment.
And this will re-play whatever reply data came from the submsg. Nice for tests
| let data = response | ||
| .messages | ||
| .into_iter() | ||
| .try_fold(response.data, |data, resend| { |
There was a problem hiding this comment.
Interesting rework of loop to functional, but looks good
There was a problem hiding this comment.
I could do that or introduce mutable variable which is updated. I find folding way more readable that mutating, as if you understand folding, nothing surprises you. Analyze mutation always requires you to carefully read the loop. Otherwise I won't bother changing it.
packages/multi-test/src/app.rs
Outdated
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(response.data, Some("Data".to_owned().into_bytes().into())); |
There was a problem hiding this comment.
That is some chain... Doesn't it work without the to_owned?
Or "Data".to_bytes().into()?
packages/multi-test/src/app.rs
Outdated
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(response.data, Some("Second".to_owned().into_bytes().into())); |
packages/multi-test/src/app.rs
Outdated
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(response.data, Some("First".to_owned().into_bytes().into())); |
| &echo::Message { | ||
| data: Some("Orig".to_owned()), | ||
| sub_msg: vec![ | ||
| make_echo_submsg(contract.clone(), None, vec![]), |
There was a problem hiding this comment.
Very cool test case for the fold
| contract.clone(), | ||
| &echo::Message { | ||
| data: Some("Orig".to_owned()), | ||
| sub_msg: vec
hashedone
force-pushed
the
352-repsonse-data-handling
branch
from
2eedb20 to
6b52132
Compare
|
Nice updates |
Closes #352
I am not entirely sure - the issue mentions only data, but I am pretty sure that events from sub messages should also be merged (however I can misunderstand something) - as this would be similar and simple change, I think I can add it to the same PR.