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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1bb2e5
feat(swap): Add code to ServiceError class
niklasnatter Mar 26, 2024
31b3406
Add multiple error codes
niklasnatter Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions packages/shared/src/node-apis/RpcClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import assert from 'assert';
/* eslint-disable max-classes-per-file */
import EventEmitter, { once } from 'events';
import { filter, firstValueFrom, Subject, timeout } from 'rxjs';
import type { Logger } from 'winston';
Expand All @@ -13,6 +13,8 @@ type RpcResponse =
| { id: number; result: unknown }
| { id: number; error: { code: number; message: string } };

export class RpcClientError extends Error {}

export default class RpcClient<
Req extends Record<string, z.ZodTypeAny>,
Res extends Record<string, z.ZodTypeAny>,
Expand Down Expand Up @@ -126,7 +128,7 @@ export default class RpcClient<
// if the socket closes after sending a request but before getting a
// response, we need to retry the request
once(this, DISCONNECT, { signal: controller.signal }).then(() => {
throw new Error('disconnected');
throw new RpcClientError('server disconnected before response was received');
}),
]);
controller.abort();
Expand All @@ -137,9 +139,13 @@ export default class RpcClient<
}
}

assert(response, 'no response received');
if (!response) {
throw new RpcClientError('no response received');
}

if ('error' in response) throw new Error(response.error.message);
if ('error' in response) {
throw new RpcClientError(response.error.message);
}

this.logger?.info(
`received response from rpc client: ${response} ${JSON.stringify(response)}}`,
Expand Down
5 changes: 3 additions & 2 deletions packages/shared/src/rpc/__tests__/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe(validateSwapAmount, () => {

expect(result).toEqual({
success: false,
reason: 'expected amount is below minimum swap amount (100000000000000000)',
reason: 'given amount (100) is below minimum swap amount (100000000000000000)',
});
});

Expand All @@ -28,7 +28,8 @@ describe(validateSwapAmount, () => {

expect(result).toEqual({
success: false,
reason: 'expected amount is above maximum swap amount (1000000000000000000)',
reason:
'given amount (1000000000000000001) is above maximum swap amount (1000000000000000000)',
});
});

Expand Down
4 changes: 2 additions & 2 deletions packages/shared/src/rpc/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export const validateSwapAmount = (
if (amount < minimumAmount) {
return {
success: false,
reason: `expected amount is below minimum swap amount (${minimumAmount})`,
reason: `given amount (${amount}) is below minimum swap amount (${minimumAmount})`,
};
}

Expand All @@ -22,7 +22,7 @@ export const validateSwapAmount = (
if (maxAmount != null && amount > maxAmount) {
return {
success: false,
reason: `expected amount is above maximum swap amount (${maxAmount})`,
reason: `given amount (${amount}) is above maximum swap amount (${maxAmount})`,
};
}

Expand Down
1 change: 0 additions & 1 deletion packages/swap/src/event-handlers/__tests__/__snapshots__/batchSwap.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ exports[`batch swap flow handles all the events: broadcast 1`] = `
"requestedBlockIndex": "1-280",
"succeededAt": 1970-01-01T00:00:12.000Z,
"succeededBlockIndex": "2-296",
"transactionRef": null,
"type": "BATCH",
"updatedAt": Any<Date>,
}
Expand Down
1 change: 0 additions & 1 deletion packages/swap/src/event-handlers/__tests__/__snapshots__/ccmSwap.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ exports[`batch swap flow handles all the events: broadcast 1`] = `
"requestedBlockIndex": "2-81",
"succeededAt": 1970-01-01T00:00:18.000Z,
"succeededBlockIndex": "3-25",
"transactionRef": null,
"type": "CCM",
"updatedAt": Any<Date>,
}
Expand Down
5 changes: 3 additions & 2 deletions packages/swap/src/handlers/openSwapDepositChannel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,21 @@ export default async function openSwapDepositChannel(
) {
if (!validateAddress(input.destChain, input.destAddress, env.CHAINFLIP_NETWORK)) {
throw ServiceError.badRequest(
'bad-request',
`Address "${input.destAddress}" is not a valid "${input.destChain}" address`,
);
}

if (await screenAddress(input.destAddress)) {
throw ServiceError.badRequest(`Address "${input.destAddress}" is sanctioned`);
throw ServiceError.badRequest('bad-request', `Address "${input.destAddress}" is sanctioned`);
}

const result = await validateSwapAmount(
{ asset: input.srcAsset, chain: input.srcChain },
BigInt(input.expectedDepositAmount),
);

if (!result.success) throw ServiceError.badRequest(result.reason);
if (!result.success) throw ServiceError.badRequest('invalid-amount', result.reason);

const {
address: depositAddress,
Expand Down
4 changes: 2 additions & 2 deletions packages/swap/src/routes/__tests__/quote.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ describe('server', () => {

expect(status).toBe(400);
expect(body).toMatchObject({
message: 'expected amount is below minimum swap amount (16777215)',
message: 'given amount (5) is below minimum swap amount (16777215)',
});
});

Expand All @@ -156,7 +156,7 @@ describe('server', () => {

expect(status).toBe(400);
expect(body).toMatchObject({
message: 'expected amount is above maximum swap amount (1)',
message: 'given amount (5) is above maximum swap amount (1)',
});
});

Expand Down
6 changes: 3 additions & 3 deletions packages/swap/src/routes/__tests__/swap.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ describe('server', () => {
const { body, status } = await request(server).get(`/swaps/1`);

expect(status).toBe(404);
expect(body).toEqual({ message: 'resource not found' });
expect(body).toEqual({ code: 'not-found', message: 'no swap for id "1" found' });
});

it(`retrieves a swap in ${State.AwaitingDeposit} status`, async () => {
Expand Down Expand Up @@ -1377,7 +1377,7 @@ describe('server', () => {

expect(status).toBe(400);
expect(body).toMatchObject({
message: 'expected amount is below minimum swap amount (16777215)',
message: 'given amount (5) is below minimum swap amount (16777215)',
});
});

Expand All @@ -1397,7 +1397,7 @@ describe('server', () => {

expect(status).toBe(400);
expect(body).toMatchObject({
message: 'expected amount is above maximum swap amount (1)',
message: 'given amount (5) is above maximum swap amount (1)',
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/swap/src/routes/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export const handleError = (res: Response, error: unknown) => {
logger.customInfo('received error', {}, { error });

if (error instanceof ServiceError) {
res.status(error.code).json({ message: error.message });
res.status(error.httpCode).json({ code: error.code, message: error.message });
} else {
logger.customError('unknown error occurred', { alertCode: 'UnknownError' }, { error });
res.status(500).json({ message: 'unknown error' });
Expand Down
41 changes: 32 additions & 9 deletions packages/swap/src/routes/quote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import express from 'express';
import type { Server } from 'socket.io';
import { Asset, Assets, Chain, Chains, getInternalAsset } from '@/shared/enums';
import { bigintMin, getPipAmountFromAmount } from '@/shared/functions';
import { RpcClientError } from '@/shared/node-apis/RpcClient';
import { quoteQuerySchema, SwapFee } from '@/shared/schemas';
import { calculateIncludedSwapFees, estimateIngressEgressFeeAssetAmount } from '@/swap/utils/fees';
import { getPools } from '@/swap/utils/pools';
Expand Down Expand Up @@ -55,14 +56,14 @@ const quote = (io: Server) => {
query: req.query,
error: queryResult.error,
});
throw ServiceError.badRequest('invalid request');
throw ServiceError.badRequest('bad-request', `the quote request parameters are invalid`);
}

logger.info('received a quote request', { query: req.query });

// detect if ingress and egress fees are exposed as gas asset amount or fee asset amount
// https://github.com/chainflip-io/chainflip-backend/pull/4497
// TODO: remove this once all networks are upraded to 1.3
// TODO: remove this once all networks are upgraded to 1.3
const ingressEgressFeeIsGasAssetAmount =
(await getIngressFee({ chain: 'Ethereum', asset: 'FLIP' })) ===
(await getIngressFee({ chain: 'Ethereum', asset: 'USDC' }));
Expand All @@ -74,7 +75,7 @@ const quote = (io: Server) => {
const amountResult = await validateSwapAmount(srcChainAsset, BigInt(query.amount));

if (!amountResult.success) {
throw ServiceError.badRequest(amountResult.reason);
throw ServiceError.badRequest('invalid-amount', amountResult.reason);
}

const includedFees: SwapFee[] = [];
Expand All @@ -95,6 +96,7 @@ const quote = (io: Server) => {
let ingressFee = await getIngressFee(srcChainAsset);
if (ingressFee == null) {
throw ServiceError.internalError(
'rpc-error',
`could not determine ingress fee for ${getInternalAsset(srcChainAsset)}`,
);
}
Expand All @@ -112,7 +114,10 @@ const quote = (io: Server) => {
});
swapInputAmount -= ingressFee;
if (swapInputAmount <= 0n) {
throw ServiceError.badRequest(`amount is lower than estimated ingress fee (${ingressFee})`);
throw ServiceError.badRequest(
'invalid-amount',
`deposit amount (${query.amount}) is lower than estimated ingress fee (${ingressFee})`,
);
}

if (query.brokerCommissionBps) {
Expand Down Expand Up @@ -170,6 +175,7 @@ const quote = (io: Server) => {
let egressFee = await getEgressFee(destChainAsset);
if (egressFee == null) {
throw ServiceError.internalError(
'rpc-error',
`could not determine egress fee for ${getInternalAsset(destChainAsset)}`,
);
}
Expand All @@ -193,7 +199,8 @@ const quote = (io: Server) => {

if (egressAmount < minimumEgressAmount) {
throw ServiceError.badRequest(
`egress amount (${egressAmount}) is lower than minimum egress amount (${minimumEgressAmount})`,
'invalid-amount',
`expected egress amount (${egressAmount}) is lower than minimum egress amount (${minimumEgressAmount})`,
);
}

Expand Down Expand Up @@ -223,13 +230,29 @@ const quote = (io: Server) => {
} catch (err) {
if (err instanceof ServiceError) throw err;

const message =
err instanceof Error ? err.message : 'unknown error (possibly no liquidity)';
let httpCode = 500;
let errorPayload;
if (err instanceof RpcClientError) {
if (err.message.includes('InsufficientLiquidity')) {
httpCode = 400;
errorPayload = {
code: 'invalid-amount',
message: `insufficient liquidity for swapping deposit amount (${query.amount})`,
error: err.message,
};
} else {
errorPayload = { code: 'rpc-error', message: err.message, error: err.message };
}
} else {
const message =
err instanceof Error ? err.message : 'an unexpected internal error occurred';
errorPayload = { code: 'rpc-error', message, error: message };
}

logger.error('error while collecting quotes:', err);

// DEPRECATED(1.3): remove `error`
res.status(500).json({ message, error: message });
// DEPRECATED(1.3): remove `error`, return ServiceError.internalError instead
res.status(httpCode).json(errorPayload);
}
}),
);
Expand Down
11 changes: 8 additions & 3 deletions packages/swap/src/routes/swap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const coerceChain = (chain: string) => {
case 'POLKADOT':
return screamingSnakeToPascalCase(uppercase);
default:
throw ServiceError.badRequest(`invalid chain "${chain}"`);
throw ServiceError.badRequest('bad-request', `invalid chain "${chain}"`);
}
};

Expand Down Expand Up @@ -126,7 +126,12 @@ router.get(
}
}

ServiceError.assert(swapDepositChannel || swap || failedSwap, 'notFound', 'resource not found');
ServiceError.assert(
swapDepositChannel || swap || failedSwap,
'notFound',
'not-found',
Copy link
Contributor

Choose a reason for hiding this comment

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

We could be more explicit: swap-not-found

`no swap for id "${id}" found`,
);

let state: State;
let failureMode;
Expand Down Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙂

}

const { srcChainExpiryBlock, channelOpeningFee, ...response } = await openSwapDepositChannel(
Expand Down
6 changes: 3 additions & 3 deletions packages/swap/src/routes/thirdPartySwap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ router.post(
logger.info('received bad request for new third party swap', {
body: req.body,
});
throw ServiceError.badRequest('invalid request body');
throw ServiceError.badRequest('bad-request', 'invalid request body');
}
try {
await prisma.thirdPartySwap.create({
Expand All @@ -31,7 +31,7 @@ router.post(
});
res.sendStatus(201);
} catch (err) {
if (err instanceof Error) throw ServiceError.internalError(err.message);
if (err instanceof Error) throw ServiceError.internalError('internal-error', err.message);
throw ServiceError.internalError();
}
}),
Expand All @@ -54,7 +54,7 @@ router.get(
res.json({ ...swap });
} catch (err) {
if (err instanceof ServiceError) throw err;
if (err instanceof Error) throw ServiceError.internalError(err.message);
if (err instanceof Error) throw ServiceError.internalError('internal-error', err.message);
throw ServiceError.internalError();
}
}),
Expand Down
31 changes: 22 additions & 9 deletions packages/swap/src/utils/ServiceError.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,43 @@ type IgnoredField =
| 'prepareStackTrace'
| 'stackTraceLimit';

type ErrorCode = 'not-found' | 'bad-request' | 'internal-error' | 'rpc-error' | 'invalid-amount';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@niklasnatter niklasnatter Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@niklasnatter niklasnatter Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@niklasnatter niklasnatter Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@GabrielBuragev GabrielBuragev Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@GabrielBuragev GabrielBuragev Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@niklasnatter niklasnatter Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think NOT_FOUND would be preferable


export default class ServiceError extends Error {
static badRequest(message: string): ServiceError {
return new ServiceError(message, 400);
static badRequest(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

code: ErrorCode = 'bad-request',
message = 'the request payload is not valid',
Comment on lines +12 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

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

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',

): ServiceError {
return new ServiceError(code, message, 400);
}

static notFound(message = 'resource not found'): ServiceError {
return new ServiceError(message, 404);
static notFound(
code: ErrorCode = 'not-found',
message = 'the requested resource was not found',
): ServiceError {
return new ServiceError(code, message, 404);
}

static internalError(message = 'internal error'): ServiceError {
return new ServiceError(message, 500);
static internalError(
code: ErrorCode = 'internal-error',
message = 'an unexpected internal error occurred',
): ServiceError {
return new ServiceError(code, message, 500);
}

static assert(
condition: unknown,
code: Exclude<keyof typeof ServiceError, IgnoredField>,
type: Exclude<keyof typeof ServiceError, IgnoredField>,
code: ErrorCode,
message: string,
): asserts condition {
if (!condition) throw ServiceError[code](message);
if (!condition) throw ServiceError[type](code, message);
}

constructor(
readonly code: ErrorCode,
message: string,
readonly code: number,
readonly httpCode: number,
) {
super(message);

Expand Down
Loading