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

feat: handling of backend errors #191

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

DNR500
Copy link
Contributor

@DNR500 DNR500 commented Jun 11, 2024

Jira: LF-8182

The PR looks to make some changes around backend errors
Screenshot 2024-06-11 at 16 17 41

Pre-requisties

  • It appears that in Widget we are basically using the LiFiErrorCodes to know and understand which errors have been throw. This presents that idea that we probably care more about the contents of the error, ie which error code is presented rather than than class type the error is instantiated from. I think this gives us more freedom in terms of implementation.

This PR...

  • Looks at the potential to provide more information when outputting error messages - Specifically for use in browsers, so that a more verbose error messages can provide more information which might help with debugging. This should present more information when screenshots of errors are shared or if that error message is copied and pasted.
  • Attempts to simplify the HTTP errors when making requests against the LiFI backend - originally we were throw a new error in response to our fetch request returning a response that wasn't ok, then in turn catch that error and throw another error in order to establish LiFi error codes and error type information. Here we try to use the original error, letting that bubble up and decorating that error with the additional LiFi error codes and error type information. Aim is to reduce the complexity of the error handling while passing as much contextual information as possible.
    • Current LiFi error codes that were generated in response to status codes should still be respected
    • The error stack no long needs to be passed on to another error, and the response object will be accessibly as part of the error.
    • Should reduce the number of try and catch block in the api service functions - output and dealing with http errors can be restricted to just the our request function. Error handling does not need to happen in the API service functions themselves.
    • Also note, we do use the request function in a couple of places to make requests from sources outside of the LiFi domain - we make requests to npm and tenderly endpoints. I've added a flag so that requests can opt out of using the LiFi error codes in situations like this.

I've tried to add in some make sure the code is well tested and I've removed some tests that I don't think are needed anymore.

Points to consider

  • Given that error messages like this might be shared across platforms like stack overflow - are there an sensitive pieces of information we might prefer to redact? For example, header information for the fetch request could be output here as present but not with the specific details. eg
{
  headers: {
    "x-lifi-api-key": "XXXXXXXXX",
    "x-lifi-userid": "XXXXXXXXX",
    "x-lifi-integrator": "XXXXXXXXX",
  }
}
  • Errors displayed in the browser will present more information. Errors in a node environment however will likely have duplicate information as node.js tends to output error class properties. One thing that might be worth thinking about as part of this work is addition of a verboseErrors opt in/out in the SDK config. When verboseErrors is not true it could just read as
HTTPError: [ValidationError] Request failed with status code 400 Bad Request
        responseMessage: The from amount must be greater than zero.

@DNR500 DNR500 added the WIP Work in progress label Jun 11, 2024
src/helpers.ts Outdated
@@ -62,7 +62,8 @@ export const convertQuoteToRoute = (step: LiFiStep): Route => {

export const fetchTxErrorDetails = async (txHash: string, chainId: number) => {
const response = await request<TenderlyResponse>(
`https://api.tenderly.co/api/v1/public-contract/${chainId}/tx/${txHash}`
`https://api.tenderly.co/api/v1/public-contract/${chainId}/tx/${txHash}`,
{ disableLiFiErrorCodes: true }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the request function in a couple of places to make requests from sources outside of the LiFi domain - This flag opts out of using the LiFi error codes in situations like this.

)
return response
} catch (e) {
throw await parseBackendError(e)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBackendError as basically do two main things - using response.json() to access the response body message and then also creating an error with sets of error codes and error types dependant on status code of the response.

Both these tasks have been moved into the HTTPError class - with the async method to process the response.json() being triggered in the request function. The try/catch shouldn't be needed here anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant