near-sdk-rs: Standardized errors

@agostbiro I feel like the “standarized errors” bit in the title created a little confusion here. I don’t think @itegulov meant standard errors (like HTTP error codes you mentioned), but a “standardized way of declaring contract errors” (sic!).

I’d vote for just having custom errors and not having standard errors - at least not at the SDK level.

Prescribed HTTP error codes were created because in principle, the HTTP design was all about fetching/posting/updating documents and other resources, with predefined operations for those purposes. It was never meant for custom apps with a tight client-server coupling like we see today. I don’t think this is good inspiration for us.

In turn, our system is for apps with mostly a tight client-server coupling. Each app has its own set of operations and problems, and app devs should define all those themselves. It’s closer to how RPC systems work, and every RPC system I’ve ever seen lets you define your app-specific errors without imposing any.

I think the question of how we compose sets of errors is very valid though. Errors like NotTheOwner could be provided by an access control lib, and then a dev creating a concrete contract would benefit from having a convenient way of including those AC errors.

I assume tuple/struct variants would be forbidden here?

@uint I don’t think this is the intent.

This issue is more about making error handling explicit as currently the norm is that contract developers just panic in the middle of the execution, and that is treated as an error.

Let me bring some context here:

In NEAR, transaction execution often happens across several blocks (please, watch/read this nice article). Contract call action is processed when receipt is processed and if during the execution the function wants to make a cross-contract call or make a transfer it will initiate more receipts. Each receipt execution is atomic (either completely successful and the changes are committed to the blockchain or completely rejected (e.g. on panic) and changes are not saved). Note that transactions in NEAR (in contrast to individual receipts) are not atomic.

That said, panic is often used to indicate that no changes should be saved, but the result string and logs are still returned, see this example:

This issue suggests we standardize this error handling and instead of returning a text message, we actually return some structured error.

On a similar note, it is sometimes useful to communicate an error without failing the execution (let it save the changes it did so far) [here is one recent discussion about it], so it is worth addressing this scenario together while we are discussing it. cc @robert-zaremba @fadeevab

@PolyProgrammist I wonder, is it possible to simplify the return type to Result<ReturnType, T>? If it’s an Err variant of a Result then it’s already an error. And it still works: “if there is no #[handle_result], then inspect the result”

There are some contracts that already use Result<ReturnType, T> and if we are going to panic on error, their behaviour is going to change

@fadeevab When you take a step back and think how it looks from a perspective when you are not laser-focused on error handling, it just feels odd:

#[persist]
fn inc(&mut self) {
    self.counter += 1;
}

Would it mean that without #[persist] it would not save/persist anything?

Other points:

1. Adding more attributes makes it even harder for users to learn the tech. Less is more.
2. panic is not the right way to signal errors in Rust world.
3. panic is not designed to help with structuring errors, and I don’t think we should promote unstructured errors going forward, so having proper infrastructure for Result handling will be the best way forward, I believe.

That said, I don’t see a reason to introduce even more magic into error handling instead of actually making it simpler.

My understanding is that the NEAR runtime raises an error if the contract’s balance is too low, but if the contract implements storage management to pass storage costs onto users, then the contract will panic with some string message.

Yeah, that makes sense.

Ethereum has no storage staking, and most Ethereum apps operate in terms of tokens (e.g. follows are NFTs in Lens), so these errors are as generic as it gets in Ethereum smart contracts. 😃

I don’t agree that it’s a question of how many contracts implement these interfaces. To me, it’s a question of if we’re talking about errors that are tied to the runtime/SDK logic, or if we’re talking about errors tied to the logic of a standard/interface/contract. I wouldn’t want to conflate them. I was also definitely not implying Ethereum has storage staking.

Now the question remains, should these be part of some global error type provided by the SDK or should the error types live in the storage management trait? I think we should approach that question from what’s more convenient to the caller.

Yes, good call. I’m fairly sure with the right design either option would be fine from the dev exp perspective, though I don’t have a good feel for how much dev work we’re looking at here. What I worry about more than convenience is managing complexity and separating concerns on our side, which is also crucial.

Having error types related to an interface live within the definition of that interface (rather than globally defined in the SDK) has these advantages:

• Interfaces can evolve independently from the SDK. Changes to error types defined in interfaces won’t require coordinating changes in the SDK.
• We won’t have to draw arbitrary lines around which error types do or do not make it into the SDK.
• The error namespace in the SDK won’t be as polluted, and the SDK will be more focused. Since it’s already a complex piece of software, I’d argue it’s great when we can avoid adding a new concern to it.

I haven’t yet looked very deeply at how composability is handled when it comes to standards/interfaces. I’ll take a look sometime soon.

For the record, I think you touched on this: I definitely agree when contracts deal with cross-contract calls, they should be able to reason about all possible errors, including ones that arise in the runtime or interfaces.

I could be very wrong here, so someone correct me if I am.

I suspect insufficient storage balance would raise HostError::BalanceExceeded in the runtime. I imagine tests and clients can catch that and handle it. It doesn’t look like that error would be propagated in a cross-contract call, but I’m sure that could be fixed.

My understanding is that the NEAR runtime raises an error if the contract’s balance is too low, but if the contract implements storage management to pass storage costs onto users, then the contract will panic with some string message. Examples:

Fungible token:

"The attached deposit is less than the minimum storage balance"

"The amount is greater than the available storage balance"

Social DB

"Not enough storage balance"

"The attached deposit is less than the minimum storage balance"

"The amount is greater than the available storage balance"

Note that

"Not enough storage balance"

and

"The attached deposit is less than the minimum storage balance"

should be probably the same error type as the user has to increase their storage deposit in both cases. While "The amount is greater than the available storage balance" message should be a different error type since in this case the user tries to withdraw more than what they’ve put in.

Now the question remains, should these be part of some global error type provided by the SDK or should the error types live in the storage management trait? I think we should approach that question from what’s more convenient to the caller.

I’m not sure I understand. The linked EIP seems to be about introducing some custom contract errors as a standard, not as part of an SDK or runtime. It also doesn’t seem to involve universal smart contract errors (like running out of gas/storage money), but ones strictly tied to their token standards.

The errors defined in the EIP originated in the OpenZeppelin contract library. Ethereum has no storage staking, and most Ethereum apps operate in terms of tokens (e.g. follows are NFTs in Lens), so these errors are as generic as it gets in Ethereum smart contracts. 😃

Thanks for the tag @frol! Zooming out a bit, I’m strongly in support of promoting standardized errors for smart contracts. Having smart contracts fail with standardized error messages instead of panics would help with code reuse and testing and it’d be great if we could make it work with cross-contract calls as well.

To expand on @itegulov’s suggestion we could provide a standard contract error implementation in this repo with well-defined semantics for common errors such as authorization or insufficient deposit. We could take JSON-RPC and HTTP 4xx errors as inspiration here. Custom errors could then be defined by the contract implementation.

The question is then how would standard errors and custom errors play together then. I see two ways:

1. Have the standard encapsulate custom errors such as this

#[derive(thiserror::Error, Debug)]
pub enum StandardError {
    #[error("Caller is not the owner of this contract")]
    NotTheOwner,
    #[error("Caller is not allowed to call this function")]
    NotAllowed,
    #[error(transparent)]
    Custom(#[from] Box<dyn std::error::Error + Send + Sync>),
}

1. Have the custom errors encapsulate standard errors such as this

#[derive(thiserror::Error, Debug)]
pub enum CustomError {
    #[error("Some contract specific error")]
    MyError,
    #[error(transparent)]
    Standard(#[from] StandardError),
}

The advantage of going with option 1 is that the most common error types are easy to handle and the same error type could be used by all contracts. The disadvantage is the type erasure in the StandardError::Custom which makes it tedious to work with inside a contract.

This code example explains the problems with option 1:

// Converting standard error to custom error is easy
let into_custom: CustomError = StandardError::NotAllowed.into();

// Converting from custom to standard requires implementing conversion.
// We could do this in a macro.
impl From<CustomError> for StandardError {
    fn from(error: CustomError) -> StandardError {
        match error {
            CustomError::Standard(standard) => standard,
            _ => StandardError::Custom(Box::new(error)),
        }
    }
}

let original_standard: StandardError = into_custom.into();
assert!(matches!(original_standard, StandardError::NotAllowed));

// Converting custom error into standard error causes type erasure though
let into_standard: StandardError = CustomError::MyError.into();
let erased_custom: CustomError = into_standard.into();
assert!(matches!(erased_custom, CustomError::Standard(_)));

// We can provide a method to return the custom error optionally, but using
// it is a bit tedious, since there is no guarantee that the downcast value
// is not a `CustomError::Standard` variant.
impl CustomError {
    /// If this variant holds a standard error created from a custom error,
    /// return the original custom error.
    pub fn downcast(self) -> Option<Self> {
        match self {
            CustomError::Standard(StandardError::Custom(maybe_custom)) => {
                maybe_custom.downcast::<CustomError>().ok().map(|b| *b)
            }
            _ => None,
        }
    }
}

let original_custom = erased_custom.downcast().unwrap();
assert!(matches!(original_custom, CustomError::MyError));

So I’m leaning more towards option 2 as if it should be more ergonomic to use even if as a result contracts won’t share the same error type which could make cross-contract calls more tedious. We could provide adding the Standard variant to CustomError and the conversion in option 2 in a macro to make the custom error types more consistent.

A third option would be to promote option 1 in methods that should be called externally and option 2 in internal methods. But maybe this just leads to more confusion.

I’m thinking more about the specific implementation questions, just wanted to write this down.

@uint Actually, I was not thinking about changing nearcore, at least not yet. I don’t see a problem with receipts rolling back on guest error.

As proposed in the discussion on Linkdrop NEP that I referenced in the previous comment, it would be great to standardize Result::Err-like signaling from smart contract with an option to make it with and without rollback. I have not dived into the topic yet, so I only have half-backed and ugly solutions at the moment, but I will share them just to give you the direction of thoughts:

near-sdk-rs could abuse logs to serialize an error message similar to how events are already implemented. Logs are recorded even when receipt fails, so near-sdk-rs could handle return Result::Err value by logging the error value serialized as JSON before exiting. See how near_bindgen creates extern "C" functions for the struct methods:

https://github.com/near/near-sdk-rs/blob/e4cb471631c55fb8831403ffea12afc71030abc1/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs#L36-L52

and there is already #[handle_result] attribute which we might deprecate and handle Result “natively”:

https://github.com/near/near-sdk-rs/blob/e4cb471631c55fb8831403ffea12afc71030abc1/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs#L696-L723