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

feat(open-payments): add create quote #888

Merged
merged 26 commits into from
Jan 13, 2023
Merged

feat(open-payments): add create quote #888

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
cf98952
feat(open-payments): add create quote method
raducristianpopa Dec 16, 2022
5b86967
feat(open-payments): add tests
raducristianpopa Dec 16, 2022
592e91a
Merge branch 'main' into radup--810
raducristianpopa Dec 20, 2022
d1164bc
chore(backend): update create quote args
raducristianpopa Dec 20, 2022
58dbed4
Merge branch 'main' into radup--810
raducristianpopa Dec 21, 2022
7e91f91
chore(backend): update create quote args type
raducristianpopa Dec 21, 2022
b848718
chore(backend): update tests
raducristianpopa Dec 21, 2022
2245f53
chore(backend): revert tests
raducristianpopa Dec 21, 2022
8fd1df0
chore(backend): fix quote service and routes
raducristianpopa Dec 21, 2022
a2b572e
chore(backend): update quote tests
raducristianpopa Dec 21, 2022
613d57d
chore(backend): update create quote resolver
raducristianpopa Dec 21, 2022
4518098
feat(backend): check for multiple amounts in resolver
raducristianpopa Dec 22, 2022
43f94f6
chore(backend): enforce types for quote `CreateBody`
raducristianpopa Dec 22, 2022
f2f213a
chore(open-payments): update quote routes
raducristianpopa Dec 22, 2022
675b8c3
feat(open-payments): add quote tests
raducristianpopa Dec 22, 2022
22ae401
chore(backend): update quote routes tests
raducristianpopa Dec 22, 2022
e4b0634
chore(open-payments): fix lint warnings
raducristianpopa Dec 22, 2022
6e93aa0
chore(backend): revert options check in service
raducristianpopa Jan 3, 2023
6e54d15
chore(backend): remove quote resolver check
raducristianpopa Jan 5, 2023
72ebdbd
chore(open-payments): update quote args type
raducristianpopa Jan 5, 2023
9dd6de0
feat(open-payments): update create quote route
raducristianpopa Jan 5, 2023
67d0488
chore(open-payments): update tests
raducristianpopa Jan 5, 2023
1b5c01a
chore(backend): remove expect ditto
raducristianpopa Jan 5, 2023
56d5777
chore: merge main & resolve conflicts
raducristianpopa Jan 10, 2023
f3bf0ac
chore(open-payments): fix typo
raducristianpopa Jan 10, 2023
0f24dfa
Merge branch 'main' into radup--810
raducristianpopa Jan 13, 2023
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
10 changes: 9 additions & 1 deletion packages/backend/src/graphql/resolvers/quote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { Quote } from '../../open_payments/quote/model'
import { ApolloContext } from '../../app'
import { getPageInfo } from '../../shared/pagination'
import { Pagination } from '../../shared/baseModel'
import { CreateQuoteOptions } from '../../open_payments/quote/service'

export const getQuote: QueryResolvers<ApolloContext>['quote'] = async (
parent,
Expand All @@ -32,8 +33,15 @@ export const getQuote: QueryResolvers<ApolloContext>['quote'] = async (
export const createQuote: MutationResolvers<ApolloContext>['createQuote'] =
async (parent, args, ctx): Promise<ResolversTypes['QuoteResponse']> => {
const quoteService = await ctx.container.use('quoteService')
const options: CreateQuoteOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because graphql doesn't do mutually exclusive types (:disappointed:), I think we need to move the old check from the quote service here

rafiki/packages/backend/src/open_payments/quote/service.ts

Lines 76 to 78 in 4db6f8e

if (options.sendAmount && options.receiveAmount) {
return QuoteError.InvalidAmount
}

Suggested change
const options: CreateQuoteOptions = {
if (args.input.sendAmount && args.input.receiveAmount) {
return {
code: errorToCode[QuoteError.InvalidAmount].toString(),
success: false,
message: errorToMessage[QuoteError.InvalidAmount]
}
}
const options: CreateQuoteOptions = {

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially I was looking at the @oneOf directive (RFC). Even though it's available in a couple of packages, is still a feature from the future and the behavior could change slightly. It would be nice if we can enforce the types at the GraphQL schema later on.

While playing around with it, I got to this point:

// schema.graphql

input QuoteAmount @oneOf {
  sendAmount: AmountInput
  receiveAmount: AmountInput
}

input CreateQuoteInput {
  paymentPointerId: String!
  amount: QuoteAmount
  receiver: String!
}
// graphql.generated.ts

export type QuoteAmount =
  { sendAmount: AmountInput; receiveAmount?: never; }
  |  { sendAmount?: never; receiveAmount: AmountInput; };

export type CreateQuoteInput = {
  paymentPointerId: Scalars['String'];
  amount?: InputMaybe<QuoteAmount>;
  receiver: Scalars['String'];
};

I think it partially fulfills our needs for the CreateQuoteInput. 🤔

paymentPointerId: args.input.paymentPointerId,
receiver: args.input.receiver
}
if (args.input.sendAmount) options.sendAmount = args.input.sendAmount
if (args.input.receiveAmount)
options.receiveAmount = args.input.receiveAmount
return quoteService
.create(args.input)
.create(options)
.then((quoteOrErr: Quote | QuoteError) =>
isQuoteError(quoteOrErr)
? {
Expand Down
28 changes: 13 additions & 15 deletions packages/backend/src/open_payments/quote/routes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,22 +190,20 @@ describe('Quote Routes', (): void => {
'$description',
async ({ sendAmount, receiveAmount }): Promise<void> => {
options = {
receiver,
sendAmount: sendAmount
? {
value: sendAmount,
assetCode: asset.code,
assetScale: asset.scale
}
: undefined,
receiveAmount: receiveAmount
? {
value: receiveAmount,
assetCode: asset.code,
assetScale: asset.scale
}
: undefined
receiver
}
if (sendAmount)
options.sendAmount = {
value: sendAmount,
assetCode: asset.code,
assetScale: asset.scale
}
if (receiveAmount)
options.receiveAmount = {
value: receiveAmount,
assetCode: asset.code,
assetScale: asset.scale
}
const ctx = setup({})
let quote: Quote | undefined
const quoteSpy = jest
Expand Down
30 changes: 21 additions & 9 deletions packages/backend/src/open_payments/quote/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Logger } from 'pino'
import { ReadContext, CreateContext } from '../../app'
import { IAppConfig } from '../../config/app'
import { QuoteService } from './service'
import { CreateQuoteOptions, QuoteService } from './service'
import { isQuoteError, errorToCode, errorToMessage } from './errors'
import { Quote } from './model'
import { AmountJSON, parseAmount } from '../amount'
Expand Down Expand Up @@ -43,25 +43,37 @@ async function getQuote(
ctx.body = body
}

export type CreateBody = {
interface CreateBodyBase {
receiver: string
}

interface CreateBodyWithSendAmount extends CreateBodyBase {
sendAmount?: AmountJSON
receiveAmount?: never
}

interface CreateBodyWithReceiveAmount extends CreateBodyBase {
sendAmount?: never
receiveAmount?: AmountJSON
}

export type CreateBody = CreateBodyWithSendAmount | CreateBodyWithReceiveAmount

async function createQuote(
deps: ServiceDependencies,
ctx: CreateContext<CreateBody>
): Promise<void> {
const { body } = ctx.request
const options: CreateQuoteOptions = {
paymentPointerId: ctx.paymentPointer.id,
receiver: body.receiver,
clientId: ctx.grant?.clientId
}
if (body.sendAmount) options.sendAmount = parseAmount(body.sendAmount)
if (body.receiveAmount)
options.receiveAmount = parseAmount(body.receiveAmount)
try {
const quoteOrErr = await deps.quoteService.create({
paymentPointerId: ctx.paymentPointer.id,
receiver: body.receiver,
sendAmount: body.sendAmount && parseAmount(body.sendAmount),
receiveAmount: body.receiveAmount && parseAmount(body.receiveAmount),
clientId: ctx.grant?.clientId
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should be able to make sendAmount and receiveAmount mutually exclusive in CreateBody (like with CreateQuoteOptions) and leave this as it was before.
This is enforced by the OpenAPI request validator thanks to:

Copy link
Member Author

Choose a reason for hiding this comment

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

With the changes to the CreateBody type, it's still complaining:
Types of property 'sendAmount' are incompatible. Type 'Amount' is not assignable to type 'never'

But I think we should keep the changes to CreateBody.

const quoteOrErr = await deps.quoteService.create(options)

if (isQuoteError(quoteOrErr)) {
throw quoteOrErr
Expand Down
32 changes: 16 additions & 16 deletions packages/backend/src/open_payments/quote/service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,10 @@ describe('QuoteService', (): void => {
paymentPointerId,
receiver: toConnection
? connectionService.getUrl(incomingPayment)
: incomingPayment.url,
sendAmount,
receiveAmount
: incomingPayment.url
}
if (sendAmount) options.sendAmount = sendAmount
if (receiveAmount) options.receiveAmount = receiveAmount
expected = {
...options,
paymentType
Expand Down Expand Up @@ -639,7 +639,6 @@ describe('QuoteService', (): void => {

test.each`
sendAmount | receiveAmount | description
${sendAmount} | ${receiveAmount} | ${'with multiple amounts'}
${{ ...sendAmount, value: BigInt(0) }} | ${undefined} | ${'with sendAmount of zero'}
${{ ...sendAmount, value: BigInt(-1) }} | ${undefined} | ${'with negative sendAmount'}
${{ ...sendAmount, assetScale: 3 }} | ${undefined} | ${'with wrong sendAmount asset'}
Expand All @@ -649,18 +648,19 @@ describe('QuoteService', (): void => {
`(
'fails to create $description',
async ({ sendAmount, receiveAmount }): Promise<void> => {
await expect(
quoteService.create({
paymentPointerId,
receiver: (
await createIncomingPayment(deps, {
paymentPointerId: receivingPaymentPointer.id
})
).url,
sendAmount,
receiveAmount
})
).resolves.toEqual(QuoteError.InvalidAmount)
const options: CreateQuoteOptions = {
paymentPointerId,
receiver: (
await createIncomingPayment(deps, {
paymentPointerId: receivingPaymentPointer.id
})
).url
}
if (sendAmount) options.sendAmount = sendAmount
if (receiveAmount) options.receiveAmount = receiveAmount
await expect(quoteService.create(options)).resolves.toEqual(
QuoteError.InvalidAmount
)
}
)

Expand Down
21 changes: 17 additions & 4 deletions packages/backend/src/open_payments/quote/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,26 @@ async function getQuote(
return Quote.query(deps.knex).get(options).withGraphFetched('asset')
}

export interface CreateQuoteOptions {
interface QuoteOptionsBase {
paymentPointerId: string
sendAmount?: Amount
receiveAmount?: Amount
receiver: string
clientId?: string
}

interface QuoteOptionsWithSendAmount extends QuoteOptionsBase {
receiveAmount?: never
sendAmount?: Amount
}

interface QuoteOptionsWithReceiveAmount extends QuoteOptionsBase {
receiveAmount?: Amount
sendAmount?: never
}

export type CreateQuoteOptions =
| QuoteOptionsWithSendAmount
| QuoteOptionsWithReceiveAmount

async function createQuote(
deps: ServiceDependencies,
options: CreateQuoteOptions
Expand All @@ -90,7 +102,8 @@ async function createQuote(
) {
return QuoteError.InvalidAmount
}
} else if (options.receiveAmount) {
}
if (options.receiveAmount) {
if (options.receiveAmount.value <= BigInt(0)) {
return QuoteError.InvalidAmount
}
Expand Down
10 changes: 9 additions & 1 deletion packages/backend/src/tests/outgoingPayment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,15 @@ export async function createOutgoingPayment(
'quoteId'
>
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change options to 👇 you won't need the changes below

  options: CreateTestQuoteOptions &
    Omit<CreateOutgoingPaymentOptions, 'quoteId'>

but then it looks like there's some calls to createOutgoingPayment that would need to updated.

Copy link
Member Author

@raducristianpopa raducristianpopa Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 I think we should keep the type as it was before, since our changes are affecting only the createQuote function ?

Copy link
Member Author

@raducristianpopa raducristianpopa Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think about refactoring createOutgoingPayment later on, to something like:

function createOutgoingPayment(
  deps: IocContract<AppServices>,
  quoteOptions: CreateTestQuoteOptions,
  outgoingPaymentOptions: Omit<CreateOutgoingPaymentOptions, 'quoteId'>
): Promise<OutgoingPayment>

): Promise<OutgoingPayment> {
const quote = await createQuote(deps, options)
const quoteOptions: CreateTestQuoteOptions = {
paymentPointerId: options.paymentPointerId,
clientId: options.clientId,
receiver: options.receiver,
validDestination: options.validDestination
}
if (options.sendAmount) quoteOptions.sendAmount = options.sendAmount
if (options.receiveAmount) quoteOptions.receiveAmount = options.receiveAmount
const quote = await createQuote(deps, quoteOptions)
const outgoingPaymentService = await deps.use('outgoingPaymentService')
const receiverService = await deps.use('receiverService')
if (options.validDestination === false) {
Expand Down
Loading