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

Add context to multitest execution errors #597

Merged
merged 10 commits into from
Jan 4, 2022
Merged

Add context to multitest execution errors #597

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b4e8e67
add context to multitest execution errors
uint Dec 22, 2021
582e47e
multitest: add a contract that tests WasmMsg calls
uint Jan 3, 2022
0291e82
multitest: add tests for contract errors
uint Jan 4, 2022
2c90217
multitest: handle nested error contexts properly
uint Jan 4, 2022
05cf444
clippy
uint Jan 4, 2022
3eb5e2d
multitest: test comments
uint Jan 4, 2022
ad155f5
Merge branch 'main' into 575-multitest-debug
uint Jan 4, 2022
708e572
multitest: style
uint Jan 4, 2022
ef50e4b
multitest: context for queries and instantiations
uint Jan 4, 2022
ede69a5
cw1-whitelist: missing InstantiateMsg derives
uint Jan 4, 2022
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
26 changes: 19 additions & 7 deletions packages/multi-test/src/contracts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use cosmwasm_std::{
from_slice, Binary, CosmosMsg, Deps, DepsMut, Empty, Env, MessageInfo, Reply, Response, SubMsg,
};

use anyhow::{anyhow, bail, Result as AnyResult};
use anyhow::{anyhow, bail, Context, Result as AnyResult};

/// Interface to call into a Contract
pub trait Contract<T>
Expand Down Expand Up @@ -65,7 +65,7 @@ pub struct ContractWrapper<
T6 = Empty,
E6 = anyhow::Error,
> where
T1: DeserializeOwned,
T1: DeserializeOwned + Debug,
T2: DeserializeOwned,
T3: DeserializeOwned,
T4: DeserializeOwned,
Expand All @@ -88,7 +88,7 @@ pub struct ContractWrapper<

impl<T1, T2, T3, E1, E2, E3, C> ContractWrapper<T1, T2, T3, E1, E2, E3, C>
where
T1: DeserializeOwned + 'static,
T1: DeserializeOwned + Debug + 'static,
T2: DeserializeOwned + 'static,
T3: DeserializeOwned + 'static,
E1: Display + Debug + Send + Sync + 'static,
Expand Down Expand Up @@ -132,7 +132,7 @@ where
impl<T1, T2, T3, E1, E2, E3, C, T4, E4, E5, T6, E6>
ContractWrapper<T1, T2, T3, E1, E2, E3, C, T4, E4, E5, T6, E6>
where
T1: DeserializeOwned + 'static,
T1: DeserializeOwned + Debug + 'static,
T2: DeserializeOwned + 'static,
T3: DeserializeOwned + 'static,
T4: DeserializeOwned + 'static,
Expand Down Expand Up @@ -317,7 +317,7 @@ where
impl<T1, T2, T3, E1, E2, E3, C, T4, E4, E5, T6, E6> Contract<C>
for ContractWrapper<T1, T2, T3, E1, E2, E3, C, T4, E4, E5, T6, E6>
where
T1: DeserializeOwned,
T1: DeserializeOwned + Debug + Clone,
T2: DeserializeOwned,
T3: DeserializeOwned,
T4: DeserializeOwned,
Expand All @@ -337,8 +337,20 @@ where
info: MessageInfo,
msg: Vec<u8>,
) -> AnyResult<Response<C>> {
let msg = from_slice(&msg)?;
(self.execute_fn)(deps, env, info, msg).map_err(|err| anyhow!(err))
let msg: T1 = from_slice(&msg)?;
let address = env.contract.address.clone();
(self.execute_fn)(deps, env, info.clone(), msg.clone())
.map_err(|err| anyhow!("{}", err))
.context(format!(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ueco-jb This stuff takes the ContractError you'd normally return in a contract entrypoint, makes an anyhow::Error out of it, and then wraps that in another anyhow::Error, now with extra info (address, sender, etc). So we're not getting extra stdout logs, it's just the error now has more info.

If you look at the failing test, it refers to places like this in code: https://github.com/CosmWasm/cw-plus/blob/main/contracts/cw3-flex-multisig/src/contract.rs#L1089. Basically it looks like downcast stopped working with the added context, and I'm not sure why yet.

If you're happy with the direction though, I'll investigate when I'm back.

Copy link
Member

Choose a reason for hiding this comment

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

Adding context to the error does seem like a very nice way of handling it.

I assume this context is print out when we do res.unwrap() on the Err variant. If so, this seems like a very nice addition.

Question: if this is nested - execute calls execute in submsg, 2nd contract errors - this will show context for both contracts, right?

A test showing the error message in such a case would be great to evaluate it. (I know... super hard to do CI there, maybe just something marked "#[skip]" that I can modify that line and run locally to evaluate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: if this is nested - execute calls execute in submsg, 2nd contract errors - this will show context for both contracts, right?

Yes, although I haven't tested it with more levels of nesting I think. A test would be nice for that. I'll look at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, the nested thing didn't quite work, but I fixed it. There are tests showing it works. There's a way to take a peek what the display looks like now: https://github.com/CosmWasm/cw-plus/pull/597/files#r778142632

r#"Contract returned an error on execute
Contract address: {}
Message sender: {}
Funds: {:?}
Message dump:
{:?}
"#,
address, info.sender, info.funds, msg,
))
}

fn instantiate(
Copy link
Member

Choose a reason for hiding this comment

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

We should do the same map error here and for query, right?

I assume that will be a follow-up PR, but just want to double check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I guess I shouldn't close the issue until those are implemented.

Expand Down
25 changes: 18 additions & 7 deletions packages/multi-test/src/executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use schemars::JsonSchema;
use serde::Serialize;
use utils::{parse_execute_response_data, parse_instantiate_response_data};

use anyhow::Result as AnyResult;
use anyhow::{Context, Result as AnyResult};

#[derive(Default, Clone, Debug)]
pub struct AppResponse {
Expand Down Expand Up @@ -98,20 +98,31 @@ where
/// Execute a contract and process all returned messages.
/// This is just a helper around execute(),
/// but we parse out the data field to that what is returned by the contract (not the protobuf wrapper)
fn execute_contract<T: Serialize>(
fn execute_contract<T: Serialize + std::fmt::Debug>(
&mut self,
sender: Addr,
contract_addr: Addr,
msg: &T,
send_funds: &[Coin],
) -> AnyResult<AppResponse> {
let msg = to_binary(msg)?;
let msg = WasmMsg::Execute {
contract_addr: contract_addr.into(),
msg,
let binary_msg = to_binary(msg)?;
let wrapped_msg = WasmMsg::Execute {
contract_addr: contract_addr.to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

to_string is more explicit, but does require a clone.
into_string?

(Note: this is pedantic to even worry about performance here... this is test code after all... just my usual review bugs chipped in)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

msg: binary_msg,
funds: send_funds.to_vec(),
};
let mut res = self.execute(sender, msg.into())?;
let mut res = self
.execute(sender.clone(), wrapped_msg.into())
.context(format!(
r#"Contract returned an error on execute
Contract address: {}
Message sender: {}
Funds: {:?}
Message dump:
{:?}
"#,
contract_addr, sender, send_funds, msg,
))?;
res.data = res
.data
.and_then(|d| parse_execute_response_data(d.as_slice()).unwrap().data);
Expand Down