Skip to content

663/mk/create outgoing payment #807

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

Merged
merged 8 commits into from
Dec 5, 2022
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 @@ -419,10 +419,10 @@ async function addSentAmount(
): Promise<OutgoingPayment> {
const fundingZeroOrUndefined =
payment.state === OutgoingPaymentState.Funding ? BigInt(0) : undefined
let sent = value || (await deps.accountingService.getTotalSent(payment.id))
Copy link
Contributor Author

@mkurapov mkurapov Dec 1, 2022

Choose a reason for hiding this comment

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

This is a guess about the desired behaviour

the main issue here was using || instead of ??. When we call this function just above:

return await addSentAmount(deps, payment, BigInt(0))

we pass in value: BigInt(0) . BigInt(0) is falsy, so we end up calling deps.accountingService.getTotalSent(payment.id)) even though we actually just wanted to set sent to be 0.

if (sent === undefined) {
sent = fundingZeroOrUndefined
}
const sent =
value ??
(await deps.accountingService.getTotalSent(payment.id)) ??
fundingZeroOrUndefined

if (sent !== undefined) {
payment.sentAmount = {
Expand Down
3 changes: 2 additions & 1 deletion packages/open-payments/src/client/grant.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@ describe('grant', (): void => {

const axiosInstance = defaultAxiosInstance
const logger = silentLogger
const client = 'https://example.com/.well-known/pay'

describe('createGrantRoutes', (): void => {
test('creates response validator for grant requests', async (): Promise<void> => {
jest.spyOn(openApi, 'createResponseValidator')

createGrantRoutes({ axiosInstance, openApi, logger })
createGrantRoutes({ axiosInstance, openApi, logger, client })
expect(openApi.createResponseValidator).toHaveBeenCalledTimes(1)
expect(openApi.createResponseValidator).toHaveBeenCalledWith({
path: '/',
Expand Down
12 changes: 6 additions & 6 deletions packages/open-payments/src/client/incoming-payment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ describe('incoming-payment', (): void => {
test('returns incoming payment if passes validation', async (): Promise<void> => {
const incomingPayment = mockIncomingPayment()

nock(baseUrl).get('/incoming-payment').reply(200, incomingPayment)
nock(baseUrl).get('/incoming-payments').reply(200, incomingPayment)

const result = await getIncomingPayment(
{
axiosInstance,
logger
},
{
url: `${baseUrl}/incoming-payment`,
url: `${baseUrl}/incoming-payments`,
accessToken: 'accessToken'
},
openApiValidators.successfulValidator
Expand All @@ -76,7 +76,7 @@ describe('incoming-payment', (): void => {
}
})

nock(baseUrl).get('/incoming-payment').reply(200, incomingPayment)
nock(baseUrl).get('/incoming-payments').reply(200, incomingPayment)

await expect(() =>
getIncomingPayment(
Expand All @@ -85,7 +85,7 @@ describe('incoming-payment', (): void => {
logger
},
{
url: `${baseUrl}/incoming-payment`,
url: `${baseUrl}/incoming-payments`,
accessToken: 'accessToken'
},
openApiValidators.successfulValidator
Expand All @@ -96,7 +96,7 @@ describe('incoming-payment', (): void => {
test('throws if incoming payment does not pass open api validation', async (): Promise<void> => {
const incomingPayment = mockIncomingPayment()

nock(baseUrl).get('/incoming-payment').reply(200, incomingPayment)
nock(baseUrl).get('/incoming-payments').reply(200, incomingPayment)

await expect(() =>
getIncomingPayment(
Expand All @@ -105,7 +105,7 @@ describe('incoming-payment', (): void => {
logger
},
{
url: `${baseUrl}/incoming-payment`,
url: `${baseUrl}/incoming-payments`,
accessToken: 'accessToken'
},
openApiValidators.failedValidator
Expand Down
131 changes: 126 additions & 5 deletions packages/open-payments/src/client/outgoing-payment.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
createOutgoingPayment,
createOutgoingPaymentRoutes,
getOutgoingPayment,
validateOutgoingPayment
Expand All @@ -12,6 +13,7 @@ import {
silentLogger
} from '../test/helpers'
import nock from 'nock'
import { v4 as uuid } from 'uuid'

describe('outgoing-payment', (): void => {
let openApi: OpenAPI
Expand Down Expand Up @@ -39,21 +41,35 @@ describe('outgoing-payment', (): void => {
method: HttpMethod.GET
})
})

test('creates createOutgoingPaymentOpenApiValidator properly', async (): Promise<void> => {
jest.spyOn(openApi, 'createResponseValidator')

createOutgoingPaymentRoutes({
axiosInstance,
openApi,
logger
})
expect(openApi.createResponseValidator).toHaveBeenCalledWith({
path: '/outgoing-payments',
method: HttpMethod.POST
})
})
})

describe('getOutgoingPayment', (): void => {
test('returns outgoing payment if passes validation', async (): Promise<void> => {
const outgoingPayment = mockOutgoingPayment()

nock(baseUrl).get('/outgoing-payment').reply(200, outgoingPayment)
nock(baseUrl).get('/outgoing-payments').reply(200, outgoingPayment)

const result = await getOutgoingPayment(
{
axiosInstance,
logger
},
{
url: `${baseUrl}/outgoing-payment`,
url: `${baseUrl}/outgoing-payments`,
accessToken: 'accessToken'
},
openApiValidators.successfulValidator
Expand Down Expand Up @@ -84,15 +100,15 @@ describe('outgoing-payment', (): void => {
logger
},
{
url: `${baseUrl}/outgoing-payment`,
url: `${baseUrl}/outgoing-payments`,
accessToken: 'accessToken'
},
openApiValidators.successfulValidator
)
).rejects.toThrowError()
})

test('throws is outgoing payment does not pass open api validation', async (): Promise<void> => {
test('throws if outgoing payment does not pass open api validation', async (): Promise<void> => {
const outgoingPayment = mockOutgoingPayment()

nock(baseUrl).get('/outgoing-payment').reply(200, outgoingPayment)
Expand All @@ -104,7 +120,7 @@ describe('outgoing-payment', (): void => {
logger
},
{
url: `${baseUrl}/outgoing-payment`,
url: `${baseUrl}/outgoing-payments`,
accessToken: 'accessToken'
},
openApiValidators.failedValidator
Expand All @@ -113,6 +129,111 @@ describe('outgoing-payment', (): void => {
})
})

describe('createOutgoingPayment', (): void => {
const quoteId = `${baseUrl}/quotes/${uuid()}`

test.each`
description | externalRef
${'Some description'} | ${'#INV-1'}
${undefined} | ${undefined}
`(
'creates outgoing payment',
async ({ description, externalRef }): Promise<void> => {
const outgoingPayment = mockOutgoingPayment({
quoteId,
description,
externalRef
})

const scope = nock(baseUrl)
.post('/outgoing-payments')
.reply(200, outgoingPayment)

const result = await createOutgoingPayment(
{
axiosInstance,
logger
},
{
url: `${baseUrl}/outgoing-payments`,
accessToken: 'accessToken',
body: {
quoteId,
description,
externalRef
}
},
openApiValidators.successfulValidator
)
expect(result).toEqual(outgoingPayment)
scope.done()
}
)

test('throws if outgoing payment does not pass validation', async (): Promise<void> => {
const outgoingPayment = mockOutgoingPayment({
sendAmount: {
assetCode: 'USD',
assetScale: 3,
value: '5'
},
sentAmount: {
assetCode: 'CAD',
assetScale: 3,
value: '0'
}
})

const scope = nock(baseUrl)
.post('/outgoing-payments')
.reply(200, outgoingPayment)

await expect(() =>
createOutgoingPayment(
{
axiosInstance,
logger
},
{
url: `${baseUrl}/outgoing-payments`,
accessToken: 'accessToken',
body: {
quoteId: uuid()
}
},
openApiValidators.successfulValidator
)
).rejects.toThrowError()
scope.done()
})

test('throws if outgoing payment does not pass open api validation', async (): Promise<void> => {
const outgoingPayment = mockOutgoingPayment()

const scope = nock(baseUrl)
.post('/outgoing-payments')
.reply(200, outgoingPayment)

await expect(() =>
createOutgoingPayment(
{
axiosInstance,
logger
},
{
url: `${baseUrl}/outgoing-payments`,
accessToken: 'accessToken',
body: {
quoteId: uuid()
}
},
openApiValidators.failedValidator
)
).rejects.toThrowError()
scope.done()
})
})

describe('validateOutgoingPayment', (): void => {
test('returns outgoing payment if passes validation', async (): Promise<void> => {
const outgoingPayment = mockOutgoingPayment()
Expand Down
51 changes: 47 additions & 4 deletions packages/open-payments/src/client/outgoing-payment.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
import { HttpMethod, ResponseValidator } from 'openapi'
import { BaseDeps, RouteDeps } from '.'
import { getRSPath, OutgoingPayment } from '../types'
import { get } from './requests'
import { CreateOutgoingPaymentArgs, getRSPath, OutgoingPayment } from '../types'
import { get, post } from './requests'

interface GetArgs {
url: string
accessToken: string
}

interface PostArgs<T> {
url: string
body: T
accessToken: string
}

export interface OutgoingPaymentRoutes {
get(args: GetArgs): Promise<OutgoingPayment>
create(args: PostArgs<CreateOutgoingPaymentArgs>): Promise<OutgoingPayment>
}

export const createOutgoingPaymentRoutes = (
Expand All @@ -23,12 +30,24 @@ export const createOutgoingPaymentRoutes = (
method: HttpMethod.GET
})

const createOutgoingPaymentOpenApiValidator =
openApi.createResponseValidator<OutgoingPayment>({
path: getRSPath('/outgoing-payments'),
method: HttpMethod.POST
})

return {
get: (args: GetArgs) =>
getOutgoingPayment(
{ axiosInstance, logger },
args,
getOutgoingPaymentOpenApiValidator
),
create: (args: PostArgs<CreateOutgoingPaymentArgs>) =>
createOutgoingPayment(
{ axiosInstance, logger },
args,
createOutgoingPaymentOpenApiValidator
)
}
}
Expand All @@ -39,11 +58,35 @@ export const getOutgoingPayment = async (
validateOpenApiResponse: ResponseValidator<OutgoingPayment>
) => {
const { axiosInstance, logger } = deps
const { url } = args
const { url, accessToken } = args

const outgoingPayment = await get(
{ axiosInstance, logger },
args,
{ url, accessToken },
validateOpenApiResponse
)

try {
return validateOutgoingPayment(outgoingPayment)
} catch (error) {
const errorMessage = 'Could not validate outgoing payment'
logger.error({ url, validateError: error?.message }, errorMessage)

throw new Error(errorMessage)
}
}

export const createOutgoingPayment = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the only other validation I could think of doing on a newly created outgoing payment (that wasn't already covered in validateOutgoingPayment) was checking whether sentAmount.value === 0. Feels like a weird behaviour to already start processing a payment before returning it, but does the caller care?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that does somehow seem less concerning than a non-zero receivedAmount on a new incoming payment.

deps: BaseDeps,
args: PostArgs<CreateOutgoingPaymentArgs>,
validateOpenApiResponse: ResponseValidator<OutgoingPayment>
) => {
const { axiosInstance, logger } = deps
const { url, body, accessToken } = args

const outgoingPayment = await post(
{ axiosInstance, logger },
{ url, body, accessToken },
validateOpenApiResponse
)

Expand Down
Loading