yearn / yearn-sdk

🦧 SDK for the yearn.finance platform. WIP
https://npm.im/@yfi/sdk
MIT License
53 stars 56 forks source link

[WEB-1186-187] Improve simulations error forwarding #187

Closed FiboApe closed 2 years ago

FiboApe commented 2 years ago

Elevator Pitch\ In order for the FE to be able to handle specific errors that come from simulation we have to start forwarding a bit more information about, where the error happened and what type of error happened. The idea is to catch the most common errors thrown by each of the points of contacts of the simulation endpoint and throw a specific error code for each and a default error message for the ones that we are not catching.

We are already catching what is throwing which error on https://github.com/yearn/yearn-sdk/blob/master/src/types/custom/simulation.ts#L3 so it's just the suggested structure to those type of errors

Definition of Done

Design Approach\ The idea is to have the errors thrown by the SDK to have the following structure:

{
  error_code: string, // The specific error code that happened
  error_type: string, // Where the error originated from (zapper, contracts, tenderly, sdk own error, etc)
  message: string // Default message to show to the user in case it is not being handled
  data: any, // metadata that might come with the error
}

Also the SDK should export error types for each type of error, like ZapperError, this would allow us to also type which error_codes are accepted. (See https://medium.com/geekculture/how-to-strongly-type-try-catch-blocks-in-typescript-4681aff406b9 for more information about how to strong type errors from exceptions)

Just as an example for https://github.com/yearn/yearn-sdk/blob/70bbec3c1b8bbac81e4f67933390c72b3d082ca7/src/interfaces/simulation.ts#L68 Instead of throwing throw new ZapperError("approval state"); we can throw throw new ZapperError( 'approval state', 4)

And ZapperError would look like

 class CustomError extends Error {
  constructor(message, error_code) {
    super(message);
    this.error_code = error_code;
  }
}

class ZapperError extends CustomError {
  constructor(message, error_code) {
    super(message, error_code);
    this.error_type = 'zapper';
  }
}

Additional Context\ This has to be done as part of https://github.com/yearn/yearn-finance-v3/issues/440

jstashh commented 2 years ago

@FiboApe thanks for such a detailed write up, looks great. My only suggestion would be to try and have the data inside the error using types where possible instead of strings i.e. error_type: string, // Where the error originated from (zapper, contracts, tenderly, sdk own error, etc) would be good to specify a type here since we know there are only distinct values of error types. And maybe make data: any into data?: any since I doubt we'd use it everywhere

jstashh commented 2 years ago

Tbh I think it'd be fine to remove the ZapperError etc types if we're going to specify the error_type itself, don't see the need to have that information in two places. What do you think @FiboApe ?

FiboApe commented 2 years ago

@jstashh Oh 100% agree, error_types will be typed and error_codes will be typed as well for each type of specific error_codes that each error might have.

Having ZapperError's and the like is going to enable us to later export those clases and type-check errors on the FE so I would still keep them, so they serve two different purposes

jstashh commented 2 years ago

@FiboApe ok great, all sounds good then!