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.

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

Add context to multitest execution errors #597

Merged
merged 10 commits into from
Jan 4, 2022
152 changes: 151 additions & 1 deletion packages/multi-test/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,7 @@ mod test {
};

use crate::error::Error;
use crate::test_helpers::contracts::{echo, hackatom, payout, reflect};
use crate::test_helpers::contracts::{caller, echo, error, hackatom, payout, reflect};
use crate::test_helpers::{CustomMsg, EmptyMsg};
use crate::transactions::StorageTransaction;

Expand Down Expand Up @@ -2580,4 +2580,154 @@ mod test {
assert_eq!(exec_res.data, Some(Binary::from(b"hello")));
}
}

mod errors {
use super::*;

#[test]
fn simple_instantiation() {
let owner = Addr::unchecked("owner");
let mut app = App::default();

// set up contract
let code_id = app.store_code(error::contract(false));
let msg = EmptyMsg {};
let err = app
.instantiate_contract(code_id, owner, &msg, &[], "error", None)
.unwrap_err();

// we should be able to retrieve the original error by downcasting
let source: &StdError = err.downcast_ref().unwrap();
if let StdError::GenericErr { msg } = source {
assert_eq!(msg, "Init failed");
} else {
panic!("wrong StdError variant");
}

// we're expecting exactly 3 nested error types
// (the original error, initiate msg context, WasmMsg context)
assert_eq!(err.chain().count(), 3);
}

#[test]
fn simple_call() {
let owner = Addr::unchecked("owner");
let mut app = App::default();

// set up contract
let code_id = app.store_code(error::contract(true));
let msg = EmptyMsg {};
let contract_addr = app
.instantiate_contract(code_id, owner, &msg, &[], "error", None)
.unwrap();

// execute should error
let err = app
.execute_contract(Addr::unchecked("random"), contract_addr, &msg, &[])
.unwrap_err();

// we should be able to retrieve the original error by downcasting
let source: &StdError = err.downcast_ref().unwrap();
if let StdError::GenericErr { msg } = source {
assert_eq!(msg, "Handle failed");
} else {
panic!("wrong StdError variant");
}

// we're expecting exactly 3 nested error types
// (the original error, execute msg context, WasmMsg context)
assert_eq!(err.chain().count(), 3);
}

#[test]
fn nested_call() {
let owner = Addr::unchecked("owner");
let mut app = App::default();

let error_code_id = app.store_code(error::contract(true));
let caller_code_id = app.store_code(caller::contract());

// set up contracts
let msg = EmptyMsg {};
let caller_addr = app
.instantiate_contract(caller_code_id, owner.clone(), &msg, &[], "caller", None)
.unwrap();
let error_addr = app
.instantiate_contract(error_code_id, owner, &msg, &[], "error", None)
.unwrap();

// execute should error
let msg = WasmMsg::Execute {
contract_addr: error_addr.into(),
msg: to_binary(&EmptyMsg {}).unwrap(),
funds: vec![],
};
let err = app
.execute_contract(Addr::unchecked("random"), caller_addr, &msg, &[])
.unwrap_err();

// we can downcast to get the original error
let source: &StdError = err.downcast_ref().unwrap();
if let StdError::GenericErr { msg } = source {
assert_eq!(msg, "Handle failed");
} else {
panic!("wrong StdError variant");
}

// we're expecting exactly 4 nested error types
// (the original error, execute msg context, 2 WasmMsg contexts)
assert_eq!(err.chain().count(), 4);
}

#[test]
fn double_nested_call() {
let owner = Addr::unchecked("owner");
let mut app = App::default();

let error_code_id = app.store_code(error::contract(true));
let caller_code_id = app.store_code(caller::contract());

// set up contracts
let msg = EmptyMsg {};
let caller_addr1 = app
.instantiate_contract(caller_code_id, owner.clone(), &msg, &[], "caller", None)
.unwrap();
let caller_addr2 = app
.instantiate_contract(caller_code_id, owner.clone(), &msg, &[], "caller", None)
.unwrap();
let error_addr = app
.instantiate_contract(error_code_id, owner, &msg, &[], "error", None)
.unwrap();

// caller1 calls caller2, caller2 calls error
Copy link
Member

Choose a reason for hiding this comment

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

good demo

let msg = WasmMsg::Execute {
contract_addr: caller_addr2.into(),
msg: to_binary(&WasmMsg::Execute {
contract_addr: error_addr.into(),
msg: to_binary(&EmptyMsg {}).unwrap(),
funds: vec![],
})
.unwrap(),
funds: vec![],
};
let err = app
.execute_contract(Addr::unchecked("random"), caller_addr1, &msg, &[])
.unwrap_err();

// uncomment to have the test fail and see how the error stringifies
// panic!("{:?}", err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A dirty way to take a peek at the debug formatted error with two levels of nesting.

Copy link
Member

Choose a reason for hiding this comment

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

I get:

---- app::test::errors::double_nested_call stdout ----
thread 'app::test::errors::double_nested_call' panicked at 'error executing WasmMsg:
sender: random
Execute { contract_addr: "Contract #0", msg: Binary([123, 34, 101, 120, 101, 99, 117, 116, 101, 34, 58, 123, 34, 99, 111, 110, 116, 114, 97, 99, 116, 95, 97, 100, 100, 114, 34, 58, 34, 67, 111, 110, 116, 114, 97, 99, 116, 32, 35, 49, 34, 44, 34, 109, 115, 103, 34, 58, 34, 101, 121, 74, 108, 101, 71, 86, 106, 100, 88, 82, 108, 73, 106, 112, 55, 73, 109, 78, 118, 98, 110, 82, 121, 89, 87, 78, 48, 88, 50, 70, 107, 90, 72, 73, 105, 79, 105, 74, 68, 98, 50, 53, 48, 99, 109, 70, 106, 100, 67, 65, 106, 77, 105, 73, 115, 73, 109, 49, 122, 90, 121, 73, 54, 73, 109, 85, 122, 77, 68, 48, 105, 76, 67, 74, 109, 100, 87, 53, 107, 99, 121, 73, 54, 87, 49, 49, 57, 102, 81, 61, 61, 34, 44, 34, 102, 117, 110, 100, 115, 34, 58, 91, 93, 125, 125]), funds: [] }

Caused by:
    0: error executing WasmMsg:
       sender: Contract #0
       Execute { contract_addr: "Contract #1", msg: Binary([123, 34, 101, 120, 101, 99, 117, 116, 101, 34, 58, 123, 34, 99, 111, 110, 116, 114, 97, 99, 116, 95, 97, 100, 100, 114, 34, 58, 34, 67, 111, 110, 116, 114, 97, 99, 116, 32, 35, 50, 34, 44, 34, 109, 115, 103, 34, 58, 34, 101, 51, 48, 61, 34, 44, 34, 102, 117, 110, 100, 115, 34, 58, 91, 93, 125, 125]), funds: [] }
    1: error executing WasmMsg:
       sender: Contract #1
       Execute { contract_addr: "Contract #2", msg: Binary([123, 125]), funds: [] }
    2: Contract returned an error on execute msg:
       EmptyMsg
    3: Generic error: Handle failed', packages/multi-test/src/app.rs:2693:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Copy link
Member

Choose a reason for hiding this comment

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

This is a great output.
I just also realise we need a better debug encoding for Binary.

Added CosmWasm/cosmwasm#1199 which could be an interesting follow-up sometime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! It's probably not super important that the binary stuff is readable here - it's usually going to be something predictable like WasmMsg::Execute, and then the contract execute msg itself will be shown in its deserialized form. Like 2: in your output.


// we can downcast to get the original error
let source: &StdError = err.downcast_ref().unwrap();
if let StdError::GenericErr { msg } = source {
assert_eq!(msg, "Handle failed");
} else {
panic!("wrong StdError variant");
}

// we're expecting exactly 5 nested error types
// (the original error, execute msg context, 3 WasmMsg contexts)
assert_eq!(err.chain().count(), 5);
}
}
}
48 changes: 32 additions & 16 deletions packages/multi-test/src/contracts.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use schemars::JsonSchema;
use serde::de::DeserializeOwned;
use std::error::Error;
use std::fmt::{self, Debug, Display};

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 +66,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 +89,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 +133,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,14 +318,14 @@ 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,
T2: DeserializeOwned,
T3: DeserializeOwned,
T1: DeserializeOwned + Debug + Clone,
T2: DeserializeOwned + Debug + Clone,
Copy link
Member

Choose a reason for hiding this comment

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

Would remove Clone from T1, T2, T3

T3: DeserializeOwned + Debug + Clone,
T4: DeserializeOwned,
T6: DeserializeOwned,
E1: Display + Debug + Send + Sync + 'static,
E2: Display + Debug + Send + Sync + 'static,
E3: Display + Debug + Send + Sync + 'static,
E1: Display + Debug + Send + Sync + Error + 'static,
E2: Display + Debug + Send + Sync + Error + 'static,
E3: Display + Debug + Send + Sync + Error + 'static,
E4: Display + Debug + Send + Sync + 'static,
E5: Display + Debug + Send + Sync + 'static,
E6: Display + Debug + Send + Sync + 'static,
Expand All @@ -337,8 +338,13 @@ 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)?;
(self.execute_fn)(deps, env, info, msg.clone())
.map_err(anyhow::Error::from)
.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

"Contract returned an error on execute msg:\n{:?}",
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 All @@ -348,13 +354,23 @@ where
info: MessageInfo,
msg: Vec<u8>,
) -> AnyResult<Response<C>> {
let msg = from_slice(&msg)?;
(self.instantiate_fn)(deps, env, info, msg).map_err(|err| anyhow!(err))
let msg: T2 = from_slice(&msg)?;
(self.instantiate_fn)(deps, env, info, msg.clone())
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought, we could remove the Clone bound if you did the following, right?

let ctx = format!("Contract returned an error on instantiate msg:\n{:?}", msg);
(self.instantiate_fn)(deps, env, info, msg).map_err(anyhow::Error::from).context(ctx)

Nothing really important, just like reducing required bounds and removing clones when possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, yeah, that looks much better. Will do 👍

.map_err(anyhow::Error::from)
.context(format!(
"Contract returned an error on instantiate msg:\n{:?}",
msg,
))
}

fn query(&self, deps: Deps, env: Env, msg: Vec<u8>) -> AnyResult<Binary> {
let msg = from_slice(&msg)?;
(self.query_fn)(deps, env, msg).map_err(|err| anyhow!(err))
let msg: T3 = from_slice(&msg)?;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks to add these two and the simple test. Looks good to me

(self.query_fn)(deps, env, msg.clone())
.map_err(anyhow::Error::from)
.context(format!(
"Contract returned an error on query msg:\n{:?}",
msg,
))
}

// this returns an error if the contract doesn't implement sudo
Expand Down
12 changes: 6 additions & 6 deletions packages/multi-test/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,20 +98,20 @@ 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.into_string(),
msg: binary_msg,
funds: send_funds.to_vec(),
};
let mut res = self.execute(sender, msg.into())?;
let mut res = self.execute(sender, wrapped_msg.into())?;
res.data = res
.data
.and_then(|d| parse_execute_response_data(d.as_slice()).unwrap().data);
Expand Down
1 change: 1 addition & 0 deletions packages/multi-test/src/test_helpers/contracts.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Module for simple contracts to be used in tests

pub mod caller;
pub mod echo;
pub mod error;
pub mod hackatom;
Expand Down
40 changes: 40 additions & 0 deletions packages/multi-test/src/test_helpers/contracts/caller.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
use std::fmt;

use cosmwasm_std::{Binary, Deps, DepsMut, Env, MessageInfo, Response, StdError, SubMsg, WasmMsg};
use schemars::JsonSchema;

use crate::{test_helpers::EmptyMsg, Contract, ContractWrapper};

fn instantiate(
_deps: DepsMut,
_env: Env,
_info: MessageInfo,
_msg: EmptyMsg,
) -> Result<Response, StdError> {
Ok(Response::default())
}

fn execute(
Copy link
Member

Choose a reason for hiding this comment

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

This is an even simpler form of reflect. Great example

_deps: DepsMut,
_env: Env,
_info: MessageInfo,
msg: WasmMsg,
) -> Result<Response, StdError> {
let message = SubMsg::new(msg);

Ok(Response::new().add_submessage(message))
}

fn query(_deps: Deps, _env: Env, _msg: EmptyMsg) -> Result<Binary, StdError> {
Err(StdError::generic_err(
"query not implemented for the `caller` contract",
))
}

pub fn contract<C>() -> Box<dyn Contract<C>>
where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
let contract = ContractWrapper::new_with_empty(execute, instantiate, query);
Box::new(contract)
}
19 changes: 16 additions & 3 deletions packages/multi-test/src/test_helpers/contracts/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use schemars::JsonSchema;

use crate::{test_helpers::EmptyMsg, Contract, ContractWrapper};

fn instantiate(
fn instantiate_err(
_deps: DepsMut,
_env: Env,
_info: MessageInfo,
Expand All @@ -14,6 +14,15 @@ fn instantiate(
Err(StdError::generic_err("Init failed"))
}

fn instantiate_ok(
_deps: DepsMut,
_env: Env,
_info: MessageInfo,
_msg: EmptyMsg,
) -> Result<Response, StdError> {
Ok(Response::default())
}

fn execute(
_deps: DepsMut,
_env: Env,
Expand All @@ -27,10 +36,14 @@ fn query(_deps: Deps, _env: Env, _msg: EmptyMsg) -> Result<Binary, StdError> {
Err(StdError::generic_err("Query failed"))
}

pub fn contract<C>() -> Box<dyn Contract<C>>
pub fn contract<C>(instantiable: bool) -> Box<dyn Contract<C>>
where
C: Clone + fmt::Debug + PartialEq + JsonSchema + 'static,
{
let contract = ContractWrapper::new_with_empty(execute, instantiate, query);
let contract = if instantiable {
ContractWrapper::new_with_empty(execute, instantiate_ok, query)
} else {
ContractWrapper::new_with_empty(execute, instantiate_err, query)
};
Box::new(contract)
}
Loading