How to distinguish errors in smart contracts?

[orkunkl]

As I've seen so far, developers define their errors using StdErr:generic_err("this is error message").
In testing, I need to check if a handler throws the expected error. Is pattern matching internal message string the only way?

Update after the discussion, I pulled an approach we agreed upon into two separate issues:

• The vm/wasm/ffi boundary returns Result<T, String> rather than Result<T, StdError> #527
• Demonstrate returning CustomErr in contract::handle #528

[maurolacy]

If you consider it's worth it, you can always add specific enums for identifying your errors.

[webmaster128]

A best practice test for this looks like

    let res: InitResult = init(&mut deps, env, msg);
    match res.unwrap_err() {
        StdError::GenericErr { msg, .. } => {
            assert_eq!(msg, "You can only use this contract for migrations")
        }
        err => panic!("Unexpected error: {:?}", err),
    }

The create assert_matches can potentially add syntactic sugar to it. I never used it but know it is used by the NEAR team. Probably worth trying it internally at some point and sharing experience to evaluate if it is worth an additional dependency.

If you consider it's worth it, you can always add specific enums for identifying your errors.

This is true for Rust in general. However, StdError and StdResult used in the contract-VM interface do no support that.

[orkunkl]

This is what I come up with:

pub enum PermissionErr {
    Delegate {},
    Redelegate {},
    Undelegate {},
    Withdraw {},
}

impl Into<String> for PermissionErr {
    fn into(self) -> String {
        return String::from(match self {
            PermissionErr::Redelegate {} => "Redelegate is not allowed",
            PermissionErr::Delegate {} => "Delegate is not allowed",
            PermissionErr::Undelegate {} => "Undelegate is not allowed",
            PermissionErr::Withdraw {} => "Withdraw is not allowed",
        });
    }
}

impl From<PermissionErr> for StdError {
    fn from(err: PermissionErr) -> Self {
        let msg: String = err.into();
        StdError::generic_err(msg)
    }
}

From and Into traits enable implicit conversion from PermissionErr to StdErr and PermissionErr to string. Maybe somehow mixing this with assert_eq! or assert_matches! could be a good practice. The topic needs some thought to be put in.

[webmaster128]

Instead of Into<String> it would be better to implement ToString. This gives you Display for free. You need to create a new string anyways, so consuming the owned self in fn into(self) -> String does not help. Then the other code becomes:

impl From<PermissionErr> for StdError {
    fn from(err: PermissionErr) -> Self {
        StdError::generic_err(err.to_string())
    }
}

[webmaster128]

it would be better to implement ToString. This gives you Display for free.

This was wrong, it is the other way round: implement Display to get ToString for free.

[webmaster128]

If you are looking for a one-liner, the following should work as well:

assert!(res.unwrap_err().to_string().contains("Redelegate is not allowed"));

This is similar to what I wrote above but ignored the StdError enum case that was used to produce the error.

[ethanfrey]

What we could also do is allow handle to return a custom error that implements Into<StdError> and then improve all the boiler plate.

Rather than return:

pub type HandleResult<U = Empty> = StdResult<HandleResponse<U>>;

We would want something like:

Result<HandleResponse<U>, E>;

We would need something like:

/// do_handle should be wrapped in an external "C" export, containing a contract-specific function as arg
pub fn do_handle<T, U, E>(
    handle_fn: &dyn Fn(
        &mut Extern<ExternalStorage, ExternalApi, ExternalQuerier>,
        Env,
        T,
    ) -> Result<HandleResponse<U>, E>,
    env_ptr: u32,
    msg_ptr: u32,
) -> u32
where
    T: DeserializeOwned + JsonSchema,
    U: Serialize + Clone + fmt::Debug + PartialEq + JsonSchema,
    E: Into<StdError>,
{ }

[webmaster128]

Then you need to be able to convert all StdError created by the standard library into E because handle only allows error type E.

[ethanfrey]

With this we could have:

pub fn handle<S: Storage, A: Api, Q: Querier>(
    deps: &mut Extern<S, A, Q>,
    env: Env,
    msg: HandleMsg,
) -> Result<HandleResponse, MyCustomError> { }

And then do something like:

pub enum MyCustomError {
    Std(StdError),
    // this is whatever we want
    Special{ count: i32 },
}

impl Into<StdError> for MyCustomError {
  fn into(self) -> StdError {
    match self {
      Std(err) => err,
      Special{count} => StdError::generic_err(format!("Special #{}", count)),
  }
}

And to make this nicer when using, eg. .may_load(..)? which returns StdError:

impl From<StdError> for MyCustomError {
  fn from(orig: StdError) -> Self {
    MyCustomError::Std(orig)
  }
}

It would buy us more clarity in the unit tests (even using handle, init high-level interfaces), but would all compress down to strings when running in a vm.

[ethanfrey]

@webmaster128 was responding with details to your comment when you wrote.

I have not tested this code, but it seems like a way to do it. I was trying something similar with Into traits in the boilerplate back in January, but I didn't have the rust experience then. I would be happy for someone to take a contract and see if this code compiles (for unit tests, not wasm) and if it gets ergonomic for the unit tests, then we can work on making all the extern "C" init and entry_points::do_init type functions to handle this.

[maurolacy]

Looks good. I can try these patterns in the unit tests for one of the contracts I worked on recently, like subkeys or atomic swaps.

[webmaster128]

Do we even need the structured error on the VM side at all? We do a lot of work serializing, deserializing StdError and mirroring to it Go and at the end of the day we throw away all the structure in https://github.com/CosmWasm/go-cosmwasm/blob/v0.10.0/lib.go#L148.

If we give up StdError on the contract-VM interface, we can have a handle function that returns MyCustomError: ToString and call_handle returns VmResult<Result<HandleResponse<U>, String>> instead of VmResult<Result<HandleResponse<U>, StdError>>. Then custom errors don't need to implement impl Into<StdError> for MyCustomError.

[ethanfrey]

Interesting point.

I tried to capture all the info I could for the go-cosmwasm interface, but it is not used in wasmd.

When writing cosmwasm-vm based integration tests, it is nice to have the same info that the unit tests have, but maybe we can define a set of best practices here, such that unit tests check a clear set of errors, integration tests just check if it errored or not.

[ethanfrey]

Looks good. I can try these patterns in the unit tests for one of the contracts I worked on recently, like subkeys or atomic swaps.

@maurolacy this will break wasm builds as the various cosmwasm-std::entry_points code requires StdError return. Best to try to update one of the contracts in cosmwasm-std, so we can try it and update the other cosmwasm-std deps in one PR. They are all quite simple contracts. queue is probably the simplest one, and hackatom is where we put most of the generic test cases

[ethanfrey]

If we give up StdError on the contract-VM interface, we can have a handle function that returns MyCustomError: ToString and call_handle returns VmResult<Result<HandleResponse, String>> instead of VmResult<Result<HandleResponse, StdError>>. Then custom errors don't need to implement impl Into for MyCustomError.

I would consider this as a second step (and different PR). For one contract it is not much code to support the current interfaces (and a much smaller change). We can see what that looks like first. Once this works and we see it is adding noticeable overhead to the return errors, let's make the bigger infrastructure changes to make the error a string (which affects 3 repos).

I do check the error cases in go-cosmwasm tests, but that is not strictly necessary.

[webmaster128]

such that unit tests check a clear set of errors, integration tests just check if it errored or not.

Integration tests can easily check the error type as well. Just as a string, not as an enum match.

I really don't like the conversion StdError -> Custom -> StdError (which requires conversion boilerplate in every contract). If we want the Custom in the middle for error matching in unit tests, then we should have a specific X at the end StdError -> Custom -> X. And X = String is the most universal thing every Custom can be converted to.

[webmaster128]

You can easily test your approach without affecting the framework:

pub fn handle<S: Storage, A: Api, Q: Querier>(
    deps: &mut Extern<S, A, Q>,
    env: Env,
    msg: HandleMsg,
)  -> StdResult<HandleResponse> {
    Ok(handle_impl(deps, env, msg)?)
}

pub fn handle_impl<S: Storage, A: Api, Q: Querier>(
    deps: &mut Extern<S, A, Q>,
    env: Env,
    msg: HandleMsg,
) -> Result<HandleResponse, MyCustomError> { }

and then test handle_impl in unit tests.

[ethanfrey]

That is a very good first step

[maurolacy]

If we want the Custom in the middle for error matching in unit tests, then we should have a specific X at the end StdError -> Custom -> X. And X = String is the most universal thing every Custom can be converted to.

What about just StdError -> Custom? I mean, making Custom generic / public enough, to be used in all places where it's needed.

Excuse me if there's a strong a reason this wouldn't work (dependency issues?). I'm not familiar enough with the platform(s) yet.

Update: I guess the point is that we need a generic handler / container at the higher levels. What about making more errors common across contracts? Many contracts need to check for / error on similar things. And for the other, contract specific errors, GenericErr will still be there.

Does that makes any sense?

[webmaster128]

What about just StdError -> Custom

Let's assume Custom is serializable. Then the contract serializes the results from init/migrate/handle it into JSON and UTF8 when returning the result to the VM (because at that boundary only raw bytes are supported). The VM then needs to be able to parse it into a structure again. Since the VM does not know how Custom looks like, this is not possible.

Many contracts need to check for / error on similar things. And for the other more specific errors, GenericErr will still be there.

That is what we tried with StdError. I think it is safe to say it failed because you have too many error cases and and at the same time still too few to model the contract's logic.

[ethanfrey]

The issue is if we want to add custom errors on a per-contract basis, we need to downcast to a string (GenericErr) or use a pre-defined category in StdError.

We can keep updating StdError with all categories that may be used in some contract, but that is quite unweildy, as it involves changing the cosmwasm-std, cosmwasm-vm, and go-cosmwasm.

The idea was to expose these more detailed cases for unit tests, to allow better error checks, but keeping a similar public interface. I agree with Simon that StdError should be simpler if anything. And with Orkun, that he wants more than checking strings to verify the proper error case was triggered in unit tests

[ethanfrey]

I would leave StdError for the errors returned by cosmwasm-std, so those are typed, and we can access that in the unit tests. But I guess we don't need to return more than strings over the vm boundary.

So we make the unit tests (with backtraces) more specific and customizable, and simplify the public interface (vm, go-cosmwasm) at the same time. I guess this goes along with my emphasis on unit tests over integration tests recently

[webmaster128]

and simplify the public interface (vm, go-cosmwasm) at the same time. I guess this goes along with my emphasis on unit tests over integration tests recently

This also goes along with respecting that unit tests and integration tests are fundamentally different things. It will make supporting other languages much easier since they only need to implement creating a compatible serialization of Result<HandleResponse, String> instead of Result<HandleResponse<U>, StdError>.

[ethanfrey]

I will pull out two sub-issues on this that are much clearer in scope. (So someone can do it without reading all the comments)

[maurolacy]

I'm not sure this has been discussed before, but I was thinking this morning that another (or complementary) option would be to move to "flat" (String) errors entirely, and provide macros and format strings for error creation and checking.

All the errors / format strings for a contract can be listed in the same file for convenience (src/error.rs), and the macros (along with common / reusable errors) can be provided at the framework level.

This will ensure error style / handling is uniform across contracts / projects. And, the macros will also help with error creation / matching. The macros can define an "error code" or "error identifier", which can be matched easily (also using a macro). And that, without needing to extend / modify StdError.

Simon is spot on about errors being in the end not more than string messages. This proposal extends or draws in that direction.

Update: Upon further reflection, the original proposal sounds like a good compromise... this proposal of using only strings is probably a little bit "outdated". Now, given that Rust has macros... :-)

[webmaster128]

Closing in favour of #527 and #528