Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Properly fold results #381

Closed
wants to merge 8 commits into from
Closed

Properly fold results #381

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4952a79
Add failing test case for writing data when we shouldn't
ethanfrey Aug 9, 2021
bd21de6
Add test cases for positive and negative data overwrite
ethanfrey Aug 9, 2021
b18753f
Add negative test case
ethanfrey Aug 9, 2021
dc0a106
Update expected events to new format
ethanfrey Aug 9, 2021
15797ca
Corrected data propagation in multi-test
hashedone Aug 9, 2021
2440816
Additional corrections for properly overwrite data
hashedone Aug 9, 2021
05512dd
Overwrite data with value returned from reply also if originally data…
hashedone Aug 10, 2021
549f3a9
Documentation updated for multi-test reply handling
hashedone Aug 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 101 additions & 13 deletions packages/multi-test/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,11 +744,14 @@ mod test {
let top_msg = echo::Message {
data: Some("outside".into()),
events: vec![Event::new("topmsg").add_attribute("called", "true")],
sub_msg: vec![SubMsg::new(WasmMsg::Execute {
contract_addr: echo_addr.to_string(),
msg: to_binary(&sub_msg).unwrap(),
funds: vec![],
})],
sub_msg: vec![SubMsg::reply_always(
WasmMsg::Execute {
contract_addr: echo_addr.to_string(),
msg: to_binary(&sub_msg).unwrap(),
funds: vec![],
},
1,
)],
attributes: vec![],
};

Expand All @@ -759,11 +762,12 @@ mod test {
// ensure data is set via reply
assert_eq!(res.data.unwrap().as_slice(), b"inside");
// ensure expected events
assert_eq!(res.events.len(), 4, "{:?}", res.events);
assert_eq!(res.events.len(), 5, "{:?}", res.events);
assert_eq!("execute", &res.events[0].ty);
assert_eq!("wasm-topmsg", &res.events[1].ty);
assert_eq!("execute", &res.events[2].ty);
assert_eq!("wasm-submsg", &res.events[3].ty);
assert_eq!("reply", &res.events[4].ty);
}

#[test]
Expand Down Expand Up @@ -1073,6 +1077,28 @@ mod test {
contract: Addr,
data: impl Into<Option<&'static str>>,
sub_msg: Vec<SubMsg>,
id: u64,
) -> SubMsg {
let data = data.into().map(|s| s.to_owned());
SubMsg::reply_always(
CosmosMsg::Wasm(WasmMsg::Execute {
contract_addr: contract.into(),
msg: to_binary(&echo::Message {
data,
sub_msg,
..echo::Message::default()
})
.unwrap(),
funds: vec![],
}),
id,
)
}

fn make_echo_submsg_no_reply(
contract: Addr,
data: impl Into<Option<&'static str>>,
sub_msg: Vec<SubMsg>,
) -> SubMsg {
let data = data.into().map(|s| s.to_owned());
SubMsg::new(CosmosMsg::Wasm(WasmMsg::Execute {
Expand Down Expand Up @@ -1130,7 +1156,7 @@ mod test {
contract.clone(),
&echo::Message {
data: Some("First".to_owned()),
sub_msg: vec![make_echo_submsg(contract, "Second", vec![])],
sub_msg: vec![make_echo_submsg(contract, "Second", vec![], 1)],
..echo::Message::default()
},
&[],
Expand All @@ -1140,6 +1166,33 @@ mod test {
assert_eq!(response.data, Some("Second".as_bytes().into()));
}

#[test]
fn single_submsg_no_reply() {
let mut app = mock_app();

let owner = Addr::unchecked("owner");

let contract_id = app.store_code(echo::contract());
let contract = app
.instantiate_contract(contract_id, owner.clone(), &EmptyMsg {}, &[], "Echo", None)
.unwrap();

let response = app
.execute_contract(
owner,
contract.clone(),
&echo::Message {
data: Some("First".to_owned()),
sub_msg: vec![make_echo_submsg_no_reply(contract, "Second", vec![])],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does look like it passes. Hmm...

..echo::Message::default()
},
&[],
)
.unwrap();

assert_eq!(response.data, Some("First".as_bytes().into()));
}

#[test]
fn single_no_data() {
let mut app = mock_app();
Expand All @@ -1157,7 +1210,7 @@ mod test {
contract.clone(),
&echo::Message {
data: Some("First".to_owned()),
sub_msg: vec![make_echo_submsg(contract, None, vec![])],
sub_msg: vec![make_echo_submsg(contract, None, vec![], 1)],
..echo::Message::default()
},
&[],
Expand Down Expand Up @@ -1185,10 +1238,10 @@ mod test {
&echo::Message {
data: Some("Orig".to_owned()),
sub_msg: vec![
make_echo_submsg(contract.clone(), None, vec![]),
make_echo_submsg(contract.clone(), "First", vec![]),
make_echo_submsg(contract.clone(), "Second", vec![]),
make_echo_submsg(contract, None, vec![]),
make_echo_submsg(contract.clone(), None, vec![], 1),
make_echo_submsg(contract.clone(), "First", vec![], 2),
make_echo_submsg(contract.clone(), "Second", vec![], 3),
make_echo_submsg(contract, None, vec![], 4),
],
..echo::Message::default()
},
Expand All @@ -1199,6 +1252,38 @@ mod test {
assert_eq!(response.data, Some("Second".as_bytes().into()));
}

#[test]
fn multiple_submsg_no_reply() {
let mut app = mock_app();

let owner = Addr::unchecked("owner");

let contract_id = app.store_code(echo::contract());
let contract = app
.instantiate_contract(contract_id, owner.clone(), &EmptyMsg {}, &[], "Echo", None)
.unwrap();

let response = app
.execute_contract(
owner,
contract.clone(),
&echo::Message {
data: Some("Orig".to_owned()),
sub_msg: vec![
make_echo_submsg_no_reply(contract.clone(), None, vec![]),
make_echo_submsg_no_reply(contract.clone(), "First", vec![]),
make_echo_submsg_no_reply(contract.clone(), "Second", vec![]),
make_echo_submsg_no_reply(contract, None, vec![]),
],
..echo::Message::default()
},
&[],
)
.unwrap();

assert_eq!(response.data, Some("Orig".as_bytes().into()));
}

#[test]
fn nested_submsg() {
let mut app = mock_app();
Expand All @@ -1225,9 +1310,12 @@ mod test {
vec![make_echo_submsg(
contract.clone(),
"Second",
vec![make_echo_submsg(contract, None, vec![])],
vec![make_echo_submsg(contract, None, vec![], 4)],
3,
)],
2,
)],
1,
)],
..echo::Message::default()
},
Expand Down
26 changes: 13 additions & 13 deletions packages/multi-test/src/wasm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,28 +389,26 @@ where
});

// call reply if meaningful
if let Ok(r) = res {
if let Ok(mut r) = res {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if matches!(reply_on, ReplyOn::Always | ReplyOn::Success) {
let mut orig = r.clone();
let reply = Reply {
id,
result: ContractResult::Ok(SubMsgExecutionResponse {
events: r.events,
data: r.data,
events: r.events.clone(),
data: r.data.clone(),
}),
};
// do reply and combine it with the original response
let res2 = self._reply(api, router, storage, block, contract, reply)?;
// override data if set
if let Some(data) = res2.data {
orig.data = Some(data);
}
// override data
r.data = res2.data;
// append the events
orig.events.extend_from_slice(&res2.events);
Ok(orig)
r.events.extend_from_slice(&res2.events);
} else {
Ok(r)
r.data = None;
}

Ok(r)
} else if let Err(e) = res {
if matches!(reply_on, ReplyOn::Always | ReplyOn::Error) {
let reply = Reply {
Expand Down Expand Up @@ -508,11 +506,13 @@ where
let AppResponse { mut events, data } = response;

// recurse in all messages
let data = messages.into_iter().try_fold(data, |data, resend| {
let data = messages.into_iter().try_fold(data, |data, submsg| {
let subres =
self.execute_submsg(api, router, storage, block, contract.clone(), resend)?;
self.execute_submsg(api, router, storage, block, contract.clone(), submsg)?;

events.extend_from_slice(&subres.events);

// Data should be overwritten if and only if replay was actually called
if data.is_some() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Ok::<_, String>(subres.data.or(data))
} else {
Expand Down