Error Handling

[matklad]

Continuing discussion from:

near/nearcore#6367 (comment)

TL;DR:

My understanding is that today worspaces expose nearcore-internal errors as anyhow::Error. That is, it's possible to get the original error of of anyhow::Error by downcasting.

This is a backwards-compatability hazard – at nearcore, we are free to change the errors however we like, but this now can break clients of workspaces.

In terms of what to do about this – if we don't expect the clients to actually inspect the errors by downcasting, than this is probably fine. Like, this is still something to be fixed in a top-notch library, but it's unlikely to cause significant trouble.

If we do expect the clients to actually downcast the error to handle it, than that I think is suboptimal. anyhow can be use for selective error handling, but it's not ergonomic or reliably (as one forgoes static checks). If the users indeed might want to handle errors originating from workspaces, I suggest using the error kind pattern instead:

pub struct WorkspaceError 

 { // Opaque representation, repr: Whatever } 

impl WorkspaceError 

 { pub fn kind(&self) -> WorkspaceErrorKind; } 

#[non_exhaustive]
 pub enum WorkspaceErrorKind 

 { SandboxNotRunning, ServerError, ... } 

It strikes a nice balance between allowing the caller to selectively handle errors, and allowing implementer making semver-compatible changes.

---

One related bit is that we have this TxExecutionError "error" type here:

https://github.com/near/nearcore/blob/cf6c3177dd0b0bc883038d5c320d92197dd9f315/core/primitives/src/views.rs#L1017

I think is would be wrong to treat this as an error at the workspace level – TxExectuionError is not some kind of transient environmental error. It's a normal outcome of a transaction which failed to apply. It's a value type with a specific serialization format, etc. I think the signature we want for transct method which submits transaction and gets it should have roughly the same shape:

fn transact(&self, tx: Transaction) -> anyhow::Result

pub struct ExecutionOutcome 

 { pub logs: ... pub gas_burnt: ... pub error: Option , } 

 

That is: TxExecutionError is not treated like an error, it is attached to outcome, which contains things like gas_burnt, etc.

[ChaoticTempest]

Sorry for getting back to this one just now.

We shouldn't be exposing TxExecutionError in workspaces right? Just wanted to be clear since ExecutionOutcome in your example exposes it. To prevent any sort of dependency on the type itself, I think its best we expose it as a sort of info message instead

[matklad]

I think, for the start, the API should be

pub struct TxExecutionError {
  // private
  inner: nearcore::TxExecutionError
}

impl fmt::Display for TxExecutionError {}

Long term though, we should expose the structure of TxExecutionError -- it's a serialized struct, and a part of our JSON RPC, so it is de-facto stable. But that depends on extracting a stable crate with RPC types work