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

chore: add logging, outgoing payment service changes #2419

Merged
merged 2 commits into from
Feb 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
} from './model'
import { ServiceDependencies } from './service'
import { Receiver } from '../../receiver/model'
import { TransactionOrKnex } from 'objection'

// "payment" is locked by the "deps.knex" transaction.
export async function handleSending(
Expand Down Expand Up @@ -143,7 +144,8 @@ async function handleCompleted(
export async function sendWebhookEvent(
deps: ServiceDependencies,
payment: OutgoingPayment,
type: OutgoingPaymentEventType
type: OutgoingPaymentEventType,
trx?: TransactionOrKnex
): Promise<void> {
// TigerBeetle accounts are only created as the OutgoingPayment is funded.
// So default the amountSent and balance to 0 for outgoing payments still in the funding state
Expand All @@ -168,7 +170,7 @@ export async function sendWebhookEvent(
}
: undefined

await OutgoingPaymentEvent.query(deps.knex).insert({
await OutgoingPaymentEvent.query(trx || deps.knex).insert({
outgoingPaymentId: payment.id,
type,
data: payment.toData({ amountSent, balance }),
Expand Down
46 changes: 44 additions & 2 deletions packages/backend/src/open_payments/payment/outgoing/routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ import { createTestApp, TestContainer } from '../../../tests/app'
import { Config, IAppConfig } from '../../../config/app'
import { IocContract } from '@adonisjs/fold'
import { initIocContainer } from '../../..'
import { AppServices, CreateContext, ListContext } from '../../../app'
import {
AppServices,
CreateContext,
ListContext,
ReadContext
} from '../../../app'
import { truncateTables } from '../../../tests/tableManager'
import { createAsset } from '../../../tests/asset'
import { errorToCode, errorToMessage, OutgoingPaymentError } from './errors'
Expand Down Expand Up @@ -122,7 +127,25 @@ describe('Outgoing Payment Routes', (): void => {
list: (ctx) => outgoingPaymentRoutes.list(ctx),
urlPath: OutgoingPayment.urlPath
})
})

describe('get', () => {
test('returns 500 for unexpected error', async (): Promise<void> => {
jest
.spyOn(outgoingPaymentService, 'get')
.mockRejectedValueOnce(new Error('unexpected'))
const ctx = setupContext<ReadContext>({
reqOpts: {},
walletAddress
})
await expect(outgoingPaymentRoutes.get(ctx)).rejects.toMatchObject({
status: 500,
message: 'Unhandled error when trying to get outgoing payment'
})
})
})

describe('list', () => {
test('returns 500 for unexpected error', async (): Promise<void> => {
jest
.spyOn(outgoingPaymentService, 'getWalletAddressPage')
Expand All @@ -133,7 +156,7 @@ describe('Outgoing Payment Routes', (): void => {
})
await expect(outgoingPaymentRoutes.list(ctx)).rejects.toMatchObject({
status: 500,
message: `Error trying to list outgoing payments`
message: 'Unhandled error when trying to list outgoing payments'
})
})
})
Expand Down Expand Up @@ -242,5 +265,24 @@ describe('Outgoing Payment Routes', (): void => {
})
}
)

test('returns 500 on unhandled error', async (): Promise<void> => {
const quoteId = uuid()
const ctx = setup({
quoteId: `${baseUrl}/quotes/${quoteId}`
})
const createSpy = jest
.spyOn(outgoingPaymentService, 'create')
.mockRejectedValueOnce(new Error('Some error'))

await expect(outgoingPaymentRoutes.create(ctx)).rejects.toMatchObject({
message: 'Unhandled error when trying to create outgoing payment',
status: 500
})
expect(createSpy).toHaveBeenCalledWith({
walletAddressId: walletAddress.id,
quoteId
})
})
})
})
63 changes: 49 additions & 14 deletions packages/backend/src/open_payments/payment/outgoing/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ import { Logger } from 'pino'
import { ReadContext, CreateContext, ListContext } from '../../../app'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just logging & more gracefully handling 500 errors

import { IAppConfig } from '../../../config/app'
import { OutgoingPaymentService } from './service'
import { isOutgoingPaymentError, errorToCode, errorToMessage } from './errors'
import {
isOutgoingPaymentError,
errorToCode,
errorToMessage,
OutgoingPaymentError
} from './errors'
import { OutgoingPayment } from './model'
import { listSubresource } from '../../wallet_address/routes'
import {
Expand Down Expand Up @@ -49,8 +54,13 @@ async function getOutgoingPayment(
id: ctx.params.id,
client: ctx.accessAction === AccessAction.Read ? ctx.client : undefined
})
} catch (_) {
ctx.throw(500, 'Error trying to get outgoing payment')
} catch (err) {
const errorMessage = 'Unhandled error when trying to get outgoing payment'
deps.logger.error(
{ err, id: ctx.params.id, walletAddressId: ctx.walletAddress.id },
errorMessage
)
return ctx.throw(500, errorMessage)
}
if (!outgoingPayment || !outgoingPayment.walletAddress) return ctx.throw(404)
ctx.body = outgoingPaymentToBody(
Expand All @@ -77,19 +87,34 @@ async function createOutgoingPayment(
return ctx.throw(400, 'invalid quoteId')
}

const paymentOrErr = await deps.outgoingPaymentService.create({
walletAddressId: ctx.walletAddress.id,
quoteId,
metadata: body.metadata,
client: ctx.client,
grant: ctx.grant
})
let outgoingPaymentOrError: OutgoingPayment | OutgoingPaymentError

if (isOutgoingPaymentError(paymentOrErr)) {
return ctx.throw(errorToCode[paymentOrErr], errorToMessage[paymentOrErr])
try {
outgoingPaymentOrError = await deps.outgoingPaymentService.create({
walletAddressId: ctx.walletAddress.id,
quoteId,
metadata: body.metadata,
client: ctx.client,
grant: ctx.grant
})
} catch (err) {
const errorMessage =
'Unhandled error when trying to create outgoing payment'
deps.logger.error(
{ err, quoteId, walletAddressId: ctx.walletAddress.id },
errorMessage
)
return ctx.throw(500, errorMessage)
}

if (isOutgoingPaymentError(outgoingPaymentOrError)) {
return ctx.throw(
errorToCode[outgoingPaymentOrError],
errorToMessage[outgoingPaymentOrError]
)
}
ctx.status = 201
ctx.body = outgoingPaymentToBody(ctx.walletAddress, paymentOrErr)
ctx.body = outgoingPaymentToBody(ctx.walletAddress, outgoingPaymentOrError)
}

async function listOutgoingPayments(
Expand All @@ -106,7 +131,17 @@ async function listOutgoingPayments(
if (err instanceof Koa.HttpError) {
throw err
}
ctx.throw(500, 'Error trying to list outgoing payments')

const errorMessage = 'Unhandled error when trying to list outgoing payments'
deps.logger.error(
{
err,
request: ctx.request.query,
walletAddressId: ctx.walletAddress.id
},
errorMessage
)
return ctx.throw(500, errorMessage)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,86 @@ describe('OutgoingPaymentService', (): void => {
})
})

describe('get', (): void => {
test('throws error if cannot find liquidity account for SENDING payment', async () => {
const quote = await createQuote(deps, {
walletAddressId,
receiver,
debitAmount,
method: 'ilp'
})

const payment = await outgoingPaymentService.create({
walletAddressId,
quoteId: quote.id
})
assert.ok(!isOutgoingPaymentError(payment))

await expect(
outgoingPaymentService.get({
id: payment.id
})
).resolves.toEqual(payment)
await expect(
outgoingPaymentService.fund({
id: payment.id,
amount: payment.debitAmount.value,
transferId: uuid()
})
).resolves.toBeDefined()
jest
.spyOn(accountingService, 'getTotalSent')
.mockResolvedValueOnce(undefined)
await expect(
outgoingPaymentService.get({
id: payment.id
})
).rejects.toThrow(
'Could not get amount sent for payment. There was a problem getting the associated liquidity account.'
)
})
})

describe('getWalletAddressPage', (): void => {
test('throws error if cannot find liquidity account for SENDING payment', async () => {
const quote = await createQuote(deps, {
walletAddressId,
receiver,
debitAmount,
method: 'ilp'
})

const payment = await outgoingPaymentService.create({
walletAddressId,
quoteId: quote.id
})
assert.ok(!isOutgoingPaymentError(payment))

await expect(
outgoingPaymentService.get({
id: payment.id
})
).resolves.toEqual(payment)
await expect(
outgoingPaymentService.fund({
id: payment.id,
amount: payment.debitAmount.value,
transferId: uuid()
})
).resolves.toBeDefined()
jest
.spyOn(accountingService, 'getAccountsTotalSent')
.mockResolvedValueOnce([undefined])
await expect(
outgoingPaymentService.getWalletAddressPage({
walletAddressId: walletAddressId
})
).rejects.toThrow(
'Could not get amount sent for payment. There was a problem getting the associated liquidity account.'
)
})
})

describe('create', (): void => {
enum GrantOption {
Existing = 'existing',
Expand Down
Loading
Loading