Standardized errors #936
Comments
|
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, 
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 |
|
I looked into how things work a bit. If I understand correctly, the VM currently conflates these two ideas:
It sounds a lot like you'd want to disentangle those in nearcore. Is that right? @frol EDIT: I'm assuming that the VM needs to be aware of when an error happened even if no rollback is needed there. |
|
@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 and there is already near-sdk-rs/near-sdk-macros/src/core_impl/code_generator/item_impl_info.rs Lines 696 to 723 in e4cb471 |
Open questions/concerns about that:
|
|
@uint All great questions and I would like to invite @agostbiro here as well.
|
|
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:
#[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>),
}
#[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 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 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. |
|
@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 |
|
Thanks for elaborating @uint ! I meant to expand a scope a bit, because I think it'd be very useful to have standardized errors for common scenarios. For example, if a method call fails because of insufficient storage balance, it'd helpful to the caller (let that be a test, a contract, a wallet, a bot or whatever) to have a catchall that can detect that no matter what contract is called. The Ethereum ecosystem is moving into this direction with EIP-6093 as well.
I like how JSON-RPC handles this by defining a few standard errors and reserving error codes for implementation defined errors. While it's somewhat controversial, this practice has been expanded to HTTP status codes as well. |
I could be very wrong here, so someone correct me if I am. I suspect insufficient storage balance would raise
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. |
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:
Social DB
Note that
and
should be probably the same error type as the user has to increase their storage deposit in both cases. While 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.
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. :) |
Yeah, that makes sense.
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.
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:
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. |
|
@agostbiro @uint Thanks for thinking through it! Great ideas and I think we are all on the same page here, and the only thing I really want to mention here is that there are several problems at different levels we need to address:
|
|
One more thing worth to mention in a separate comment is:
|
|
@frol @agostbiro @uint Let me drop my 5 cents into the conversation.
I may be wrong with my assumption, and I am open to any evidence of the opposite :) That said, I see an option to introduce an attribute, say, Thus, 4 variants:
pub fn method(&mut self) {
panic!("Stereotypical error");
}
#[persist]
pub fn method(&mut self) {
self.state += 1;
panic!("Error but state is saved");
}
#[handle_result]
pub fn method(&mut self) -> Result<(), MyError> {
Err(MyError::SomethingWrong)
}
#[persist]
#[handle_result]
pub fn method(&mut self) -> Result<(), MyError> {
self.state+= 1;
Err(MyError::SomethingWrong)
}And I haven't checked if it's technically feasible, "request for comments". |
|
@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 Other points:
That said, I don't see a reason to introduce even more magic into error handling instead of actually making it simpler. |
|
@frol Agree. Then, what's the option to signalize an error with a saved state: some dedicated error variant? // Pseudo code
enum Error {
RollbackError(String),
StoreStateError(String),
} |
|
The key part here is the structured errors. Parsing error strings / panic messages should be gone long time ago. |
|
Ideally we don't introduce breaking changes. @fadeevab , I think we don't need more macros. |
|
Hi! I would like to work on this issue. Discussed it with @frol. I see 3 different tasks for now:
So the user code was like this previously: #[near]
impl Adder {
/// Adds two pairs point-wise.
pub fn add(&self, a: Pair, b: Pair) -> Pair {
sum_pair(&a, &b)
}
}Now it will be like this: #[derive(ContractError)]
enum InvalidArguments {
msg: string
}
#[near]
impl Adder {
/// Adds two pairs point-wise.
pub fn my_add(&self, a: Pair, b: Pair) -> Result<Pair, InvalidArguments> {
if (b.0 == 0 && b.1 == 0) {
contractError(InvalidArguments("b can't be equal to zero")
}
Ok(sum_pair(&a, &b))
}
}
{
"status": "success",
"data": {
"balance": 10
}
}
If you add #[near]
impl Adder {
/// Adds two pairs point-wise.
#[persist_on_error]
pub fn my_add(&self, a: Pair, b: Pair) -> Result<Pair, InvalidArguments> {
if (b.0 == 0 && b.1 == 0) {
Err(InvalidArguments("b can't be equal to zero"))
}
Ok(sum_pair(&a, &b))
}
}
{
"status": "success",
"data": {
"balance": 10
}
}
{
"status": "error",
"error": {
"kind": "ACCOUNT_DOES_NOT_EXIST",
"data": {
"account_id": "qwe"
}
}
}
It seems like that this does not make anything existing incompatible, including contracts, explorers, etc. Additionally, with the first point, it will be possible to update contracts using Furthermore, all these three things can be documented in one Also, there is a problem that we should not forget: currently, when you call rpc, and contract panics, it does not return a structured error which would say that the contract panicked. However it returns some string representation of that. Which is subject to fix as well, otherwise this task does not make any sense. Naming is going to be discussed more. I would like to start with the first two ones. So what do you guys think of this? @austinabell hi, do you have any comments on this? |
|
@PolyProgrammist I wonder, is it possible to simplify the return type to |
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 |
|
@PolyProgrammist I see... What if instead of hardcoding the #[derive(ContractError)]
enum MyError {
InvalidArguments
}
pub fn my_add(&self, a: Pair, b: Pair) -> Result<Pair, MyError> {
Err(MyError::InvalidAgruments)
}
|
Sounds reasonable, thanks for pointing that out. Changed the comment |
|
Hey, so the things changed a bit: As Vlad suggested, for persisting on error, we can write state in current chunk and create a new contract call so that error is in another chunk in panic_str. And in this case error is never in status but always in panic_str and therefore it's better to not use "status" field so that things remain as before. |
|
@frol @fadeevab so I actually don't understand how do I implement the |
|
@PolyProgrammist I don't understand the problem. Could you provide the "expanded" code snippet of how you want it to be and what does not work? |
|
So, here is what is happening: As @fadeevab mentions here, the user may want to use: fn main() -> Result<(), Box<dyn ContractError>> {}So that the error which implements This code tries to implement that: // --- NEAR SDK ---
trait ContractError: Debug + Display + serde::Serialize {}
// https://doc.rust-lang.org/src/alloc/boxed.rs.html#2215
impl<'a, E: ContractError + 'a> From<E> for Box<dyn ContractError + 'a> {
fn from(err: E) -> Box<dyn ContractError + 'a> {
Box::new(err)
}
}
// --- Contract ---
use std::fmt::{Debug, Display, self};
#[derive(Debug)]
enum MyError {
Zeroes,
Overflow,
}
impl Display for MyError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "this is my error")
}
}
impl ContractError for MyError {}
fn add(a: u32, b: u32) -> Result<u32, MyError> {
if a == 0 && b == 0 {
return Err(MyError::Zeroes);
}
if a == 0xffffffff || b == 0xffffffff {
return Err(MyError::Overflow);
}
Ok(a + b)
}
fn main() -> Result<(), Box<dyn ContractError>> {
let sum = add(1, 2)?; // Ok
println!("{sum}");
let sum = add(0, 0)?; // Throws an error
println!("{sum}");
Ok(())
}But it does not compile: Compiling playground v0.0.1 (/playground)
error[E0038]: the trait `ContractError` cannot be made into an object
--> src/main.rs:5:45
|
5 | impl<'a, E: ContractError + 'a> From<E> for Box<dyn ContractError + 'a> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `ContractError` cannot be made into an object
|
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
--> /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.199/src/ser/mod.rs:249:8
|
249 | fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
| ^^^^^^^^^ ...because method `serialize` has generic type parameters
|
::: src/main.rs:2:7
|
2 | trait ContractError: Debug + Display + serde::Serialize {}
| ------------- this trait cannot be made into an object...
= help: consider moving `serialize` to another trait
= help: only type `MyError` implements the trait, consider using it directly insteadWhy is that? The error "the trait ContractError can't be made into an object" arises as soon as So the question is, how do I make this code work: fn main() -> Result<(), Box<dyn ContractError>> {} |
|
So what if instead I do this: // --- NEAR SDK ---
use serde::Serialize;
// near sdk
#[derive(Serialize, Debug)]
struct BaseError {
error: String,
}
// near sdk
trait ContractError {
fn to_string(&self) -> &String;
}
// user defined
// #[contract_error]
// struct MyError1 {
// err: String,
// }
// generated by #[contract_error]
struct MyError1 {
error: String
}
impl ContractError for MyError1 {
fn to_string(&self) -> &String {
return &self.error;
}
}
impl From<MyError1> for BaseError {
fn from(err: MyError1) -> Self {
BaseError{
error: err.to_string().clone(),
}
}
}
fn add(a: u32, b: u32) -> Result<u32, MyError1> {
if a == 0 && b == 0 {
return Err(MyError1 {error: String::from("error 1")});
}
if a == 0xffffffff || b == 0xffffffff {
return Err(MyError1 {error: String::from("error 2")});
}
Ok(a + b)
}
fn main() -> Result<(), BaseError> {
let sum = add(1, 2)?; // Ok
println!("{sum}");
let sum = add(0, 0)?; // Throws an error
println!("{sum}");
Ok(())
} |
|
@PolyProgrammist I got it! You found that the following snippet is not sufficient for the smart contract because // Error
#[derive(ContractError)]
enum MyError {
InvalidArguments
}Thus, you consider 2 options: Option 1: #[derive(ContractError, Serialize)]
enum MyError {
InvalidArguments
}Option 2: #[contract_error]
enum MyError {
InvalidArguments
}The following is NOT an option: trait ContractError: serde::Serialize {}cause the error type itself ( The bottom line: #[derive(ContractError, Serialize)]
enum MyError {
InvalidArguments
}It should allow you to get rid of the Is it correct? |
|
@fadeevab but for the method: fn main() -> Result<(), Box<dyn ContractError>>to be usable, |
|
@PolyProgrammist Oh, you're right, the It means you have the only option: yours :) (what I called "option 2"). I think it's okay as long as it looks like the following snippet for the end user: #[contract_error]
enum MyError {
InvalidArguments
}...because the end user doesn't see the |
|
@PolyProgrammist Theoretically, there should be also an "option 3" where the #[derive(ContractError)]
enum MyError {
InvalidArguments
} |
Right now it is unclear what kind of errors a contract (or even a function) can fail with. It would be nice to have a standardized way of declaring contract errors, something like this:
We can then include all contract errors as a part of the ABI, this proposal also goes really well with the
Result-based error handling as we can statically infer what errors a function can fail with.The text was updated successfully, but these errors were encountered: