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

Switching from String to anyhow::Error for error type in multi-test #389

Merged
merged 2 commits into from
Aug 19, 2021

Conversation

hashedone
Copy link
Contributor

Closes #361

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks great.

@@ -15,6 +15,8 @@ use crate::executor::{AppResponse, Executor};
use crate::transactions::transactional;
use crate::wasm::{ContractData, Wasm, WasmKeeper};

use anyhow::Result;
Copy link
Member

Choose a reason for hiding this comment

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

I find it somewhat confusing to use Result as anything but std::result::Result.

How about use anyhow::Result as AnyResult?
Or just skipping the use and returning anyhow::Result<AppResponse> below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my experience if I am using anyhow in the module, I am not using any other results, and it matches this case. I also see common to just use anyhow result like this. However if it bothers you, I don't see a reason not to import like use anyhow::Result as AnyResult.

Copy link
Member

Choose a reason for hiding this comment

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

It is more me jumping from module to module. I see StdResult, ContractResult, etc. When I see Result<T> with no error case, and no name saying what is there, it is a bit odd.

I'd appreciate the rename to AnyResult

@@ -1420,7 +1426,7 @@ mod tests {
&[],
)
.unwrap_err();
assert_eq!(ContractError::Unauthorized {}.to_string(), err);
assert_eq!(ContractError::Unauthorized {}, err.downcast().unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

I need to look at docs for downcast(), but it reads nicely.

How does it know the type? Does it work with matches!? Like matches!(err.downcast().unwrap(), ContractError::InvalidAddr{..})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Downcast is basically generic with signature fn downcast<T: Any>(self) -> T;, and you can call it just like err.downcast::<Error>(). Here it is just type elision - assert_eq internally uses PartiallEq, so type elision checks all PartiallEq implementations of ContractError. It turns out that there is only one such implementation, so right side have to be of proper type (here ContractError).

Note - because of how it worsk, the downcasted error have to be on the righ side of == - because left-side PartialEq implementation is used. If error is on left side, type elision need to solve query "Find any type which have implementation of PartialEq<ContractError>" which is semantically impossible, as anyone may add such implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see it does runtime checks...

I tried:

        let unwrapped = err.downcast::<&str>().unwrap();
        assert_eq!("unauthorized", unwrapped);

and got

thread 'contract::tests::execute_group_changes_from_proposal' panicked at 'called `Result::unwrap()` on an `Err` value: Unauthorized', contracts/cw3-flex-multisig/src/contract.rs:1429:48

Anyway, makes better sense to me how this works in practice. Code is fine, just my learning the new lib.

use cosmwasm_std::{WasmMsg, WasmQuery};
use thiserror::Error;

#[derive(Debug, Error, PartialEq)]
Copy link
Member

Choose a reason for hiding this comment

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

Oh, cool, you have concrete types for all the internal / framework error messages. Very cool.

E4: ToString,
E5: ToString,
E6: ToString,
E1: Into<anyhow::Error>,
Copy link
Member

Choose a reason for hiding this comment

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

So String no longer works as a placeholder.
It must implement StdError, right? https://docs.rs/anyhow/1.0.43/anyhow/struct.Error.html#impl-From%3CE%3E

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It must implement std::Error + Send + Sync + 'static. String doesn't work, but I don't think it is a problem. Our contracts are using concrete errors anyway.

However I considered to make this even less bounded - instead Into<anyhow::Error> make it Display + Debug, then anyhow! macro can be used to "map" an error. The problem is, I am not 100% sure how does it work with downcasting, I will investigate it.

@@ -58,22 +60,22 @@ pub struct ContractWrapper<
E3,
C = Empty,
T4 = Empty,
E4 = String,
E5 = String,
E4 = anyhow::Error,
Copy link
Member

Choose a reason for hiding this comment

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

Okay, new placeholders

res.map_err(|e| e.to_string())
fn reply(&self, deps: DepsMut, env: Env, reply_data: Reply) -> Result<Response<C>> {
match &self.reply_fn {
Some(reply) => reply(deps, env, reply_data).map_err(Into::into),
Copy link
Member

Choose a reason for hiding this comment

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

map_err(Into::into) 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically what ? does.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but Some(reply) => Ok(reply(deps, env, reply_data)?) doesn't compile here.

I just thought it was a nice trick.

@ethanfrey
Copy link
Member

Looks good, please merge once you rename to AnyResult. The other comments are just me understanding anyhow

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

lgtm.

@@ -617,7 +617,7 @@ mod tests {
None,
)
.unwrap_err();
assert_eq!(err, ContractError::ZeroThreshold {}.to_string());
assert_eq!(ContractError::ZeroThreshold {}, err.downcast().unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼 I think this is more robust. Probably more efficient too.

assert_eq!(
StdError::overflow(OverflowError::new(OverflowOperation::Sub, 0, 3)),
err.downcast().unwrap()
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear: We can continue to use string matching if we want. By example for complex cases, or for clarity. Basically by using err.to_string().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically you can convert anyhow error to string, but I would say it is the opposite of maintainability. It might look clearer, but it is the same as with magic constants - you compare your error to something with unknown origin. And if for some reason message format changes, you would need to correct your test cases. Besides of that - you don't have any confirmation if an error you are expecting to occurred, or just one with same description (especially if you don't check full error text, but like it was before here - just look for generic substring).

Copy link
Contributor

Choose a reason for hiding this comment

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

I know. That's why we are moving away from String in the first place. Just mentioning the options.

.may_load(bank_storage, &account)
.map_err(|e| e.to_string())?;
fn get_balance(&self, bank_storage: &dyn Storage, account: &Addr) -> Result<Vec<Coin>> {
let val = BALANCES.may_load(bank_storage, &account)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

res.map_err(|e| e.to_string())
) -> Result<Response<C>> {
let msg = from_slice(&msg)?;
(self.execute_fn)(deps, env, info, msg).map_err(Into::into)
Copy link
Contributor

@maurolacy maurolacy Aug 18, 2021

Choose a reason for hiding this comment

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

I may be wrong, but I guess this can be simplified further (probably in another iteration) by making E1 and friends simply an anyhow::Error type, instead of an Into trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible, I am not happy on how much bounds are there. I didn't spend too much time here, but I think this particular module could use some love.

Copy link
Member

Choose a reason for hiding this comment

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

If we are wrapping everything, we could probably use some Box<dyn Trait> over generics to reduce the number of bounds. Or maybe some more clever approaches. But yeah, that is another issue.

CONTRACTS
.load(&prefixed_read(storage, NAMESPACE_WASM), address)
.map_err(|e| e.to_string())
.map_err(Into::into)
Copy link
Contributor

Choose a reason for hiding this comment

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

And this will go away when we migrate everything to anyhow. :-)

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to change the contracts themselves???

They return eg. ContractError, which is nice and clear type.
We turn that into a string when we come to the wasm boundary.
I see turning ContractError to anyhow::Error very nice in the frame of multitest but this is the only place that we have multiple contracts communicating in rust and we need to handle many error types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would not return anyhow from contracts directly. The idea about using it in place of strings. The most important downside is contract re-usability. We may want to execute functions from contracts in another contracts (it already happens), and it is good to have possibility to handling those error by their types. You can make an argument, that you can always have internal error type, and just return it as anyhow (and possibly downcast it when needed), but it is a regression. First problem is, that downcast has runtime overhead, which is 100% obsolete. The second, probably most important, that it is boilerplate - you need to do downcast().unwrap(), and you have ugly inwrap there which you should handle better (but why to handle it better when you are sure about internals?).

If .map_err(Into::into) is pain, then there is always a Ok(res?) which I personally find nasty (kind of strange way to express that what you want to do is just to map an error to more generic type). TBH I lack of function like Result::map_into which would be shorthand for doing what ? does.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I understand. I was thinking of anyhow more like an equivalent / replacement for thiserror. Now I see that it is more like a complement.

Also, I thought that for the wasm interface, we can always convert anyhow to string, so that it would be equivalent to the current solution.

let bin = data
.as_ref()
.ok_or_else(|| "No data response".to_string())?
.ok_or_else(|| anyhow!("No data response"))?
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, anyhow! is the constructor helper macro, and bail! is the return helper macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anyhow! is more than constructor, it is macro creating wrapper for an error value (same for bail!). It basically allows to create anyhow::Error from types which are not Into<anyhow::Error> (maybe they are not Send, or not std::Error). It only requires internals to be Display + Debug, which string hopefully is. And also those have neat functionality of standard string formatting.

@maurolacy
Copy link
Contributor

maurolacy commented Aug 18, 2021

Looks good, please merge once you rename to AnyResult.

I think that could be a good idea for now, but once we migrate all of cw-plus, and perhaps, all of cosmwasm to anyhow, that wouldn't be necessary anymore.

@ethanfrey
Copy link
Member

I think that could be a good idea for now, but once we migrate all of cw-plus, and perhaps, all of cosmwasm to anyhow, that wouldn't be necessary anymore.

We do not want that. We have clear type info and only turn to String on the wasm boundary (where anyhow would not work)

@hashedone
Copy link
Contributor Author

I agree with @ethanfrey. anyhow is not substitution of thiserror, it is it complement for cases where type erased error are needed. Rule of thumb is - make your errors strongly typed in your libraries so one can handle it when needed, and make your errors type erased for binaries, as if you don't handle them immediately, you would probably just log them. Testing framework is somewhere in between, but the most issues with erased errors are not applicable here - runtime overhead is not an issue with test, and if error type doesn't match expected (when downcasting), you basically don't care and fail (unwrapping).

@ethanfrey
Copy link
Member

This looks good, agree with the design.
Just the AnyError rename and please merge.

Happy if you open a new issue (for the v0.9.0 milestone) about cleaning up the bounds in contracts.rs. We now use these tests enough places that if it breaks something, the CI will complain, so it is safer to clean up and simplify.

@hashedone
Copy link
Contributor Author

Sure, later today (at evening).

@hashedone hashedone merged commit 6084e55 into main Aug 19, 2021
@hashedone hashedone deleted the 361-anyhow-in-multitest branch August 19, 2021 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider replacing String errors with anyhow::Error in interfaces
3 participants