Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle specific errors cases in request.ts #219

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

gudnuf
Copy link
Collaborator

@gudnuf gudnuf commented Dec 3, 2024

Fixes: error handling

Description

There are a variety of errors that can occur when making a request to a mint.

  • fetch fail -> instanceof NetworkError
  • fetch succeeds, but status is neither 2xx nor 400 -> instanceof HttpResponseError
  • fetch succeeds and status is 400 -> instanceof MintOperationError with specific Cashu Error code as defined in the nuts

...

Changes

  • added NetworkError and MintOperationError classes
  • catch fetch error and throw NetworkError
  • If fetch succeeds but !response.ok (ie. http error), then throw MintOperationError if applicable otherwise HttpResponseError

PR Tasks

  • Open PR
  • run npm run test --> no failing unit tests
  • run npm run format

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 4, 2024

Hey man, thanks for tasking this on! Your logic looks solid, the problems you are seeing in your tests stems from the pipeline transpiling to old days JavaScript. Classes were not a thing in ES5, so transpilers will build old-times prototype chains. However the Error object is special and overwrites these chains.

Fixing this is as easy as overwriting the prototype yourself:

export class NetworkError extends Error {
    status: number;
    constructor(message: string, status: number) {
        super(message);
        this.status = status;
        Object.setPrototypeOf(this, NetworkError.prototype);
    }
}

However we are going to move away from ES5 and the old pipeline very soon anyways. Depending on the timeline, this fix might not even be necessary

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 4, 2024

Regarding the structure of the classes:
I think having a separate sub-class for each error type is overkill. These classes do not really do anything, and the only differentiator is the error code and detail. Instead we could have a single MintOperationError that consumes the code and detail and that can be checked against quite easily like:

catch(e: unknown) {
  if (e instance of MintOperationError) {
    switch(e.code) {
      case x: 
        // do smth.
        break;
      case y:
        // do smth else
        break;
    }
  }
}

@gudnuf
Copy link
Collaborator Author

gudnuf commented Dec 4, 2024

@Egge21M I made some changes. I kept the error messages for specific codes, but moved that into the MintOperationError constructor. I think this is useful in case the mint does not return a detail and then the client can have consistent error messages. And my tests pass now, thanks!!

Should I just remove the ApiError type? I don't really understand how it is being used. It looks like most of the responses have ApiError tagged on, but all the actual errors just thrown. Seems like we should get rid of ApiError and make the user catch and handle errors on their own.

@gudnuf gudnuf marked this pull request as ready for review December 29, 2024 20:46
@gudnuf gudnuf changed the title wip add more verbose error types handle specific errors cases in request.ts Dec 29, 2024
@gudnuf gudnuf requested a review from Egge21M December 29, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants