feat: error handling

[ChaoticTempest]

This PR resolves #100

Naming is always challenging, so let me know if there's any naming in the errors that are confusing.

Also, note that I didn't change any of the compile_project related errors since I didn't wanna snowball this one -- I'll change those later after this PR lands

Errors layout:

The surface Error enum type is used specifically to pinpoint the error a user might want. The underlying errors such as ParseError and RpcError hides the details being exposed from dependencies so that we can represent the data however we want if the data changes from these types on nearcore side in the future.

I might've also messed up how thiserror library is intended to be used since I used it on the ErrorKind type so I could inline the error message into the variant itself without having to write the Display impl.

@matklad, let me know what you think if you have time

[matklad]

I have one big design consideration: what is the motivation for not making the error opaque error type? To me, for a sprawling, high-level library like workspaces an opaque error types seems like the most obvious solution, by a big marging.

Exposing internal details:

• constraints us semver-wise
• confuses the user (which error variants should they handle)
• gives "this is not well-designed" feeling overall: why there's "UnknownError" but also "Other"? why "BytesError" are grouped, but sandbox errors aren't?

[matklad]

Consider making this method infalible and using reqwest approach of postponing the error to the point where the request is made:

https://docs.rs/reqwest/latest/src/reqwest/async_impl/request.rs.html#442-455

[austinabell]

Going to have a deeper look into this later looking into those conversions, but seems like it's trending in the right direction!

[austinabell]

I have one big design consideration: what is the motivation for not making the error opaque error type? To me, for a sprawling, high-level library like workspaces an opaque error types seems like the most obvious solution, by a big margin.

I believe the motivation is that certain errors may want to be handled differently than others (for example some internal error vs some network failure) and obfuscating details or forcing converting into a string and matching a pattern might not be the best solution. Maybe there is a way we can expose details about the types of errors without exposing everything as is with this PR, because this currently definitely is more of a backwards-compatible issue than the anyhow errors previously.

[ChaoticTempest]

I have one big design consideration: what is the motivation for not making the error opaque error type? To me, for a sprawling, high-level library like workspaces an opaque error type seems like the most obvious solution, by a big margin.

What @austinabell mentioned is why I went with this top-level error enum. It's just unwieldy for a user if they want to handle a specific class of errors which I feel would be a common case. Not all classes of errors would fit in with one kind of data representation either. The reqwest example you linked out to just exposes representation relating to network, but these errors in workspaces can be more than just those, so the top-level enum was what I thought could encapsulate this, where each variant is its own representation of the underlying data.

Maybe there is a way we can expose details about the types of errors without exposing everything as is with this PR, because this currently definitely is more of a backwards-compatible issue than the anyhow errors previously.

I intentionally left some of the errors as unwrapped and bloating top-level to get some feedback, but I can wrap up them into their own ErrorKind/ErrorRepr type

[matklad]

if they want to handle a specific class of errors which I feel would be a common case.

What are some specific examples here? If the API forces the caller to handle a subset of the errors, it feels like a footgun. It's better if the cases which need handling can be expressed separately, such that all the errors are unconditionally propagated.

[ChaoticTempest]

Something like the following is what I was leaning into:

let result = worker.call(...).transact().await;
if let Err(error) = result {
   match error {
       ExecutionError(err: String) => { // do something with err }
       SerializationError(err) => { // do something with err }
       other => Err(other)?,  // propagate errors we don't care about
   }
}

where the user can match to a specific class of errors such as an ExecutionError vs a SerializationError, which can come from that single function call into a contract.

If the API forces the caller to handle a subset of the errors, it feels like a footgun. It's better if the cases which need handling can be expressed separately, such that all the errors are unconditionally propagated.

I feel the above example represents that we can handle errors separately unless I'm missing something with your statement.

[matklad]

Yeah, I understand that making error an enum allows matching on the error variants. What I don't understand is why would the user need to do that? That is what that "something" would be?

My prior is that matching on errors should be exceedingly rare, and that we shouldn't optimize for this use-case.

[ChaoticTempest]

Ah, I see what you're getting at. For something like Serialization/Parse error, we don't need to explicitly handle that case since they're more informative for debugging purposes (in which case I'll just get rid of those for something more opaque), but errors like RpcError can be useful for determining exact causes and handling them can be useful for when one wants to make an automation script out of workspaces instead of just testing. I guess in that case, we can just eliminate the Error enum down to just ExecutionError, RpcError, and something like Other to capture the rest. WDYT?

[matklad]

I suggest taking a look at how azure sdk handles errors: https://docs.rs/azure_core/0.3.0/azure_core/error/index.html

Imo, that's the right pattern:

• opaque error struct
• with a kind method
• which returns very little data (mostly a field-less enum, empathically not the actual internal representation)
• with a special case for HTTP errors

[austinabell]

Intuitively seems like someone might want to check that the execution error was a specific string/substring. We can always keep the API minimal and add later, though

[matklad]

I would say it would be wrong to use ErrorKind::Execution to get details about execution. tx execution outcome is not a Result<T, E> -- even a failed transaction spends gas. The API should push the users who wish to inspect execution errors to use CallExecutionDetails directly. If the user need to see what the error was for debugging purposes, there's fmt::Display for that

[austinabell]

I suppose my comment could be targeted more specifically at the issue that CallExecutionDetails execution errors get automatically converted into the workspaces::Result, which is why I indicate this as a motivation. For example, this test passes:

#[test(tokio::test)]
async fn test_failure() -> anyhow::Result<()> {
    let worker = workspaces::sandbox().await?;
    let account = worker.dev_create_account().await?;

    let res = account.call(&worker, account.id(), "doesn't_exist").transact().await;
    assert!(matches!(res.unwrap_err().kind(), workspaces::error::ErrorKind::Execution));

    Ok(())
}

and it gives developers no way of inspecting the error message other than going through formatting the error into a string somehow. I generally think the API holistically needs to be more consistent and streamlined because there are a few inconsistencies that will make it harder for devs to use.

[matklad]

Hm, that shoudn't automatically call try_into_result, that should be done by the user:

let res = account.call(&worker, account.id(), "doesn't_exist").transact()
  .await? // this can fail if the sandbox is down
  .success()? // this will fail if the tx got to the network, but was faulty
;

CallExecutionDetails should be #[must_use], and it shouldn't have data-erasign API like

fn try_into_result(self) -> Result<Self>

good catch!

[matklad]

It's not only the question of error message: the user should be able to tell, eg, how much gas was spend by a failed transaction.

[ChaoticTempest]

#150 was gonna resolve the #[must_use] so I'll leave it for that after I sync it up with all the recent changes from this current PR

[matklad]

LGTM! This now feels like something we can easily support&extend in the future, thanks for working on this!

[matklad]

I would say it would be wrong to use ErrorKind::Execution to get details about execution. tx execution outcome is not a Result<T, E> -- even a failed transaction spends gas. The API should push the users who wish to inspect execution errors to use CallExecutionDetails directly. If the user need to see what the error was for debugging purposes, there's fmt::Display for that

[austinabell]

#149 (comment) is the only blocking comment from me, everything else is good enough to come in. Can also pull in and tackle in a separate PR since that is an issue that was not technically introduced in this PR, and these changes don't make it any worse