feat(swap): Add code to ServiceError class [WEB-977] #654

niklasnatter · 2024-03-26T15:19:40Z

No description provided.

codecov-commenter · 2024-03-26T17:09:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.01%. Comparing base (6f4c9d3) to head (31b3406).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #654      +/-   ##
==========================================
+ Coverage   87.79%   88.01%   +0.21%     
==========================================
  Files         101      101              
  Lines        1631     1610      -21     
  Branches      239      232       -7     
==========================================
- Hits         1432     1417      -15     
+ Misses        190      186       -4     
+ Partials        9        7       -2

Flag	Coverage Δ
sdk	`93.37% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

niklasnatter · 2024-03-26T17:39:36Z

packages/swap/src/utils/ServiceError.ts

@@ -5,30 +5,43 @@ type IgnoredField =
  | 'prepareStackTrace'
  | 'stackTraceLimit';

+type ErrorCode = 'not-found' | 'bad-request' | 'internal-error' | 'rpc-error' | 'invalid-amount';


i am not sure about the granularity here. eg an invalid address will lead to bad-request at the moment, should this be invalid-addressinstead?

the problem with the current approach is that we cannot really change from a general code (bad-request) to a more specific one (invalid-address) because this could break consumers.

we could add a third property (detail) which will be set for specific errors (eg invalid-amount, invalid-address) only? but this feels overly complicated

TIL that there is a IETF RFC for exposing errors: https://datatracker.ietf.org/doc/html/rfc7807
will check if this makes sense in our case

there even is a new RPC from 2023 that obsoletes the old one: https://datatracker.ietf.org/doc/html/rfc9457
but it seems like this standard is not widely used and the format feels specific to errors returned from an api (which makes sense for the rpc)

i am leaning to use the following format:

{ type: 'NOT_FOUND' | 'BAD_REQUEST'; code: 'INVALID_AMOUNT' | 'INUSFFICIENT_LIQUIDITY' | undefined, message: 'human readable explanation' }

this would allow use to introduce specific error codes later on without breaking any consumer. (eg. adding a INVALID_ADDRESS code)

this would be kinda similar to what stripe/twilio/square does:

https://docs.stripe.com/api/errors

https://www.twilio.com/docs/usage/troubleshooting/debugging-your-application#debugging-calls-to-the-rest-api

https://developer.squareup.com/docs/build-basics/general-considerations/handling-errors

Should we maybe just use numeric codes instead of keywords? Using keywords is an approach prone to collisions and the chances that we might need to change an old code (keyword) to be more/less explicit to accomodate a new error code is higher.
If we use numeric codes we can simply guarantee they wont change while we can still change the keywords/description to provide more context.

Sorry if i am expressing this concern a little bit late.

Imagine using INVALID_AMOUNT at this point for indicating that there was an issue with the swap amount. What if tomorrow we add another endpoint that requires amounts to be provided. At that point, the INVALID_AMOUNT might apply to both cases, and it will not be possible to change this old code (keyword). Maybe your idea is to reuse this keyword code in all cases where it applies and then provide more info via the message, but that will become tricky/not straightforward to explain in the docs for example.

i agree that this could lead to keywords being less explicit than they could be bc of backwards compatibility, a number would allow you to clarify the "meaning" later on. but this also means that the number itself does not give you an idea about the error (without consulting documentation). to solve that you could provide constants with meaningful names, but then you kinda end up with the same problem of not being able to change the constant name 🙈

about the second point i am not too concerned. i think it would be fine to return INVALID_AMOUNT from a different endpoint as well. if the consumer wants to handle the error in a useful way, he needs to catch it where he calls the endpoint (and therefore knows what endpoint is called). why do you think this would need to be a different code?

acdibble · 2024-03-27T08:49:50Z

packages/swap/src/utils/ServiceError.ts

@@ -5,30 +5,43 @@ type IgnoredField =
  | 'prepareStackTrace'
  | 'stackTraceLimit';

+type ErrorCode = 'not-found' | 'bad-request' | 'internal-error' | 'rpc-error' | 'invalid-amount';


i think NOT_FOUND would be preferable

acdibble · 2024-03-27T08:50:46Z

packages/swap/src/utils/ServiceError.ts

+    code: ErrorCode = 'bad-request',
+    message = 'the request payload is not valid',


I think the message will specified more often than the code

Suggested change

code: ErrorCode = 'bad-request',

message = 'the request payload is not valid',

message = 'the request payload is not valid',

code: ErrorCode = 'bad-request',

acdibble · 2024-03-27T08:52:08Z

packages/swap/src/utils/ServiceError.ts

 export default class ServiceError extends Error {
-  static badRequest(message: string): ServiceError {
-    return new ServiceError(message, 400);
+  static badRequest(


could be interesting to add a code method that you can chain:

throw ServiceError.badRequest('some message').code('INVALID_AMOUNT')

GabrielBuragev · 2024-03-27T11:25:12Z

packages/swap/src/routes/swap.ts

@@ -268,7 +273,7 @@ router.post(
    const result = openSwapDepositChannelSchema.safeParse(req.body);
    if (!result.success) {
      logger.info('received bad request for new swap', { body: req.body });
-      throw ServiceError.badRequest('invalid request body');
+      throw ServiceError.badRequest('bad-request', 'invalid request body');


Could we provide more info as to which fields were invalid?

i was looking into generating a message for zod validation errors yesterday but decided to not include that in this pr bc it was not trivial. but i agree that it would be nice to give a hint about the validation error later on 🙂

GabrielBuragev · 2024-03-27T11:25:38Z

packages/swap/src/routes/swap.ts

+    ServiceError.assert(
+      swapDepositChannel || swap || failedSwap,
+      'notFound',
+      'not-found',


We could be more explicit: swap-not-found

GabrielBuragev · 2024-03-27T11:36:32Z

packages/swap/src/utils/ServiceError.ts

@@ -5,30 +5,43 @@ type IgnoredField =
  | 'prepareStackTrace'
  | 'stackTraceLimit';

+type ErrorCode = 'not-found' | 'bad-request' | 'internal-error' | 'rpc-error' | 'invalid-amount';


Should we maybe just use numeric codes instead of keywords? Using keywords is an approach prone to collisions and the chances that we might need to change an old code (keyword) to be more/less explicit to accomodate a new error code is higher.
If we use numeric codes we can simply guarantee they wont change while we can still change the keywords/description to provide more context.

Sorry if i am expressing this concern a little bit late.

GabrielBuragev · 2024-03-27T11:38:55Z

packages/swap/src/utils/ServiceError.ts

@@ -5,30 +5,43 @@ type IgnoredField =
  | 'prepareStackTrace'
  | 'stackTraceLimit';

+type ErrorCode = 'not-found' | 'bad-request' | 'internal-error' | 'rpc-error' | 'invalid-amount';


Imagine using INVALID_AMOUNT at this point for indicating that there was an issue with the swap amount. What if tomorrow we add another endpoint that requires amounts to be provided. At that point, the INVALID_AMOUNT might apply to both cases, and it will not be possible to change this old code (keyword). Maybe your idea is to reuse this keyword code in all cases where it applies and then provide more info via the message, but that will become tricky/not straightforward to explain in the docs for example.

feat(swap): Add code to ServiceError class

a1bb2e5

niklasnatter force-pushed the feat/error-codes branch from 0acaae1 to 191733e Compare March 26, 2024 17:06

niklasnatter changed the title ~~feat(swap): Add code to ServiceError class~~ feat(swap): Add code to ServiceError class [WEB-977] Mar 26, 2024

niklasnatter force-pushed the feat/error-codes branch from 191733e to f0cd1e0 Compare March 26, 2024 17:38

niklasnatter commented Mar 26, 2024

View reviewed changes

Add multiple error codes

31b3406

niklasnatter force-pushed the feat/error-codes branch from f0cd1e0 to 31b3406 Compare March 26, 2024 17:52

acdibble reviewed Mar 27, 2024

View reviewed changes

niklasnatter marked this pull request as ready for review March 27, 2024 10:09

niklasnatter requested a review from a team as a code owner March 27, 2024 10:09

niklasnatter requested review from jerryafr and acdibble March 27, 2024 10:09

GabrielBuragev reviewed Mar 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(swap): Add code to ServiceError class [WEB-977] #654

feat(swap): Add code to ServiceError class [WEB-977] #654

niklasnatter commented Mar 26, 2024

codecov-commenter commented Mar 26, 2024 •

edited

niklasnatter Mar 26, 2024

niklasnatter Mar 26, 2024 •

edited

niklasnatter Mar 27, 2024

niklasnatter Mar 27, 2024

niklasnatter Mar 27, 2024 •

edited

niklasnatter Mar 27, 2024 •

edited

GabrielBuragev Mar 27, 2024 •

edited

GabrielBuragev Mar 27, 2024 •

edited

niklasnatter Mar 27, 2024 •

edited

niklasnatter Mar 27, 2024

acdibble Mar 27, 2024

acdibble Mar 27, 2024

acdibble Mar 27, 2024

GabrielBuragev Mar 27, 2024

niklasnatter Mar 27, 2024

GabrielBuragev Mar 27, 2024

GabrielBuragev Mar 27, 2024 •

edited

GabrielBuragev Mar 27, 2024 •

edited

		code: ErrorCode = 'bad-request',
		message = 'the request payload is not valid',

feat(swap): Add code to ServiceError class [WEB-977] #654

Are you sure you want to change the base?

feat(swap): Add code to ServiceError class [WEB-977] #654

Conversation

niklasnatter commented Mar 26, 2024

codecov-commenter commented Mar 26, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

niklasnatter Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasnatter Mar 27, 2024 • edited

Choose a reason for hiding this comment

niklasnatter Mar 27, 2024 • edited

Choose a reason for hiding this comment

GabrielBuragev Mar 27, 2024 • edited

Choose a reason for hiding this comment

GabrielBuragev Mar 27, 2024 • edited

Choose a reason for hiding this comment

niklasnatter Mar 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GabrielBuragev Mar 27, 2024 • edited

Choose a reason for hiding this comment

GabrielBuragev Mar 27, 2024 • edited

Choose a reason for hiding this comment

codecov-commenter commented Mar 26, 2024 •

edited

niklasnatter Mar 26, 2024 •

edited

niklasnatter Mar 27, 2024 •

edited

niklasnatter Mar 27, 2024 •

edited

GabrielBuragev Mar 27, 2024 •

edited

GabrielBuragev Mar 27, 2024 •

edited

niklasnatter Mar 27, 2024 •

edited

GabrielBuragev Mar 27, 2024 •

edited

GabrielBuragev Mar 27, 2024 •

edited