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

raducristianpopa · 2022-12-20T07:40:17Z

Changes proposed in this pull request

add create quote functionality to OP Client
update types for the quote creation args in backend

Context

fixes (open-payments): create quote #810
fixes Enforce create quote args with types #834

Checklist

Related issues linked using fixes #number
Tests added/updated
Documentation added
Make sure that all checks pass

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

wilsonianb · 2022-12-21T19:02:01Z

packages/backend/src/graphql/resolvers/quote.ts

@@ -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 = {


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 = {

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. 🤔

wilsonianb · 2022-12-21T19:13:17Z

packages/backend/src/open_payments/quote/routes.ts

-      sendAmount: body.sendAmount && parseAmount(body.sendAmount),
-      receiveAmount: body.receiveAmount && parseAmount(body.receiveAmount),
-      clientId: ctx.grant?.clientId
-    })


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:

224/mk/post quote update open-payments#230

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.

wilsonianb · 2022-12-21T19:26:10Z

packages/backend/src/tests/outgoingPayment.ts

@@ -15,7 +15,15 @@ export async function createOutgoingPayment(
    'quoteId'
  >


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.

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

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>

mkurapov

Looks good, approving with some comments

mkurapov · 2022-12-23T05:40:33Z

packages/open-payments/src/client/quote.ts

 }

 export interface QuoteRoutes {
  get(args: GetArgs): Promise<Quote>
+  create(args: PostArgs<CreateQuoteArgs>): Promise<Quote>


For create method and list methods, I was thinking we would do something like this:

interface CreateArgs { // or something along these lines paymentPointer: string accessToken: string } ```suggestion create(createArgs: CreateArgs, createQuoteArgs: CreateQuoteArgs): Promise<Quote>

and then

const url = `${paymentPointer}${getRSPath('/quotes')}`

when making the actual POST call. This is so that the client doesn't need to provide the full route.

Let me know what you think

@wilsonianb had a discussion about it here: #823 (comment)

mkurapov · 2022-12-23T05:45:45Z

packages/open-payments/src/types.ts

+export type CreateQuoteArgs =
+  RSOperations['create-quote']['requestBody']['content']['application/json']


I think we would also need to do explicitly provide sendAmount: never or receiveAmount: never like you have above here as well, since having just this, typescript won't complain if we do

.create({ url, accessToken, body: { receiver: quote.receiver, sendAmount: { assetCode: '4', assetScale: 4, value: '4' }, receiveAmount: { assetCode: '4', assetScale: 4, value: '4' } }

We could even use the XOR helper type, if you'd like: https://stackoverflow.com/a/53229567

packages/open-payments/src/client/quote.test.ts

mkurapov · 2022-12-23T06:17:16Z

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

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


@raducristianpopa @wilsonianb
I'm leaning on keeping this business logic sanity check here (even though the OpenAPI validator would prevent it). If there are some issues with validator we would still be ok in the service

An alternative could be to move the check to routes.ts (like we had to do with the graphql resolver), but it'd make more sense to just keep the one check here.
So I vote we drop the resolver check in favor of this, and maybe we revisit if/when we have graphql @oneOf.

🤔 do we only change CreateBody then, and revert CreateQuoteOptions?
I think that'd avoid having to change the tests' createOutgoingPayment

🤔 i think we should keep the changes for the CreateQuoteOptions type for consistency.

My only reservation about that is we'd also need to check in the resolver.

I think the resolver will need the check in order to be able to call quoteService.create if the options type is:

export type CreateQuoteOptions = | QuoteOptionsWithSendAmount | QuoteOptionsWithReceiveAmount

With the actual changes to the resolver:

rafiki/packages/backend/src/graphql/resolvers/quote.ts

Lines 33 to 46 in 1b5c01a

export const createQuote: MutationResolvers<ApolloContext>['createQuote'] =

async (parent, args, ctx): Promise<ResolversTypes['QuoteResponse']> => {

const quoteService = await ctx.container.use('quoteService')

const options: CreateQuoteOptions = {

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(options)

.then((quoteOrErr: Quote | QuoteError) =>

isQuoteError(quoteOrErr)

i was able to make a GraphQL request with both sendAmount and receiveAmount.
quoteService.create gets called properly even with the options type being

export type CreateQuoteOptions = | QuoteOptionsWithSendAmount | QuoteOptionsWithReceiveAmount

😕
I think declaring options as CreateQuoteOptions makes the call to quoteService.create happy.
But I'm surprised we don't need the receiveAmount assignment in an else if (args.input.receiveAmount).
Maybe it's due to:

Re-enable strict type checking #781

I've just enabled strict type checking for the backend package, but TypeScript it's still not complaining about it. 🤔

I think it's not complaining because both receiveAmount and sendAmount are optional in their respective types

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

wilsonianb · 2023-01-03T13:20:46Z

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

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


An alternative could be to move the check to routes.ts (like we had to do with the graphql resolver), but it'd make more sense to just keep the one check here.
So I vote we drop the resolver check in favor of this, and maybe we revisit if/when we have graphql @oneOf.

wilsonianb · 2023-01-03T13:23:24Z

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

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


🤔 do we only change CreateBody then, and revert CreateQuoteOptions?
I think that'd avoid having to change the tests' createOutgoingPayment

mkurapov

Looks good with the latest changes, just need to resolve merge conflicts and good to go

packages/open-payments/src/types.ts

wilsonianb

Sorry, I gave you more conflicts to resolve...

raducristianpopa · 2023-01-13T07:20:50Z

Resolved the conflicts.

My final thoughts:

I tend to think that we should keep both changes to CreateBody and CreateQuoteOptions in order to be type consistent (from schema to service level)
the only drawback at the current moment is that we can not enforce the type changes for the GraphQL schema
when and if the @oneOf directive will be available, I think it will fulfill our needs for CreateQuoteInput

raducristianpopa added 4 commits December 16, 2022 11:27

feat(open-payments): add create quote method

cf98952

feat(open-payments): add tests

5b86967

Merge branch 'main' into radup--810

592e91a

chore(backend): update create quote args

d1164bc

github-actions bot added pkg: backend Changes in the backend package. pkg: open-payments type: source Changes business logic type: tests Testing related labels Dec 20, 2022

raducristianpopa marked this pull request as draft December 20, 2022 07:42

raducristianpopa commented Dec 20, 2022

View reviewed changes

packages/backend/src/open_payments/quote/service.ts Outdated Show resolved Hide resolved

wilsonianb reviewed Dec 20, 2022

View reviewed changes

packages/backend/src/open_payments/quote/service.ts Outdated Show resolved Hide resolved

packages/backend/src/open_payments/quote/service.ts Outdated Show resolved Hide resolved

packages/backend/src/open_payments/quote/service.ts Outdated Show resolved Hide resolved

raducristianpopa added 7 commits December 21, 2022 10:36

Merge branch 'main' into radup--810

58dbed4

chore(backend): update create quote args type

7e91f91

chore(backend): update tests

b848718

chore(backend): revert tests

2245f53

chore(backend): fix quote service and routes

8fd1df0

chore(backend): update quote tests

a2b572e

chore(backend): update create quote resolver

613d57d

wilsonianb reviewed Dec 21, 2022

View reviewed changes

raducristianpopa added 6 commits December 22, 2022 09:51

feat(backend): check for multiple amounts in resolver

4518098

chore(backend): enforce types for quote CreateBody

43f94f6

chore(open-payments): update quote routes

f2f213a

feat(open-payments): add quote tests

675b8c3

chore(backend): update quote routes tests

22ae401

chore(open-payments): fix lint warnings

e4b0634

raducristianpopa marked this pull request as ready for review December 22, 2022 08:28

raducristianpopa requested review from wilsonianb and mkurapov December 22, 2022 08:28

mkurapov previously approved these changes Dec 23, 2022

View reviewed changes

chore(backend): revert options check in service

6e93aa0

wilsonianb reviewed Jan 3, 2023

View reviewed changes

raducristianpopa added 4 commits January 5, 2023 11:07

chore(backend): remove quote resolver check

6e54d15

chore(open-payments): update quote args type

72ebdbd

feat(open-payments): update create quote route

9dd6de0

chore(open-payments): update tests

67d0488

raducristianpopa dismissed mkurapov’s stale review via 67d0488 January 5, 2023 09:13

chore(backend): remove expect ditto

1b5c01a

mkurapov previously approved these changes Jan 10, 2023

View reviewed changes

packages/open-payments/src/types.ts Outdated Show resolved Hide resolved

raducristianpopa added 2 commits January 10, 2023 10:28

chore: merge main & resolve conflicts

56d5777

chore(open-payments): fix typo

f3bf0ac

raducristianpopa dismissed mkurapov’s stale review via f3bf0ac January 10, 2023 08:29

mkurapov previously approved these changes Jan 10, 2023

View reviewed changes

mkurapov mentioned this pull request Jan 12, 2023

663/mk/standardize op client requests #971

Merged

4 tasks

wilsonianb reviewed Jan 12, 2023

View reviewed changes

Merge branch 'main' into radup--810

0f24dfa

raducristianpopa dismissed mkurapov’s stale review via 0f24dfa January 13, 2023 07:08

raducristianpopa requested review from wilsonianb and mkurapov January 13, 2023 07:20

mkurapov approved these changes Jan 13, 2023

View reviewed changes

mkurapov merged commit 30977a6 into main Jan 13, 2023

mkurapov deleted the radup--810 branch January 13, 2023 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raducristianpopa commented Dec 20, 2022 •

edited

Loading

wilsonianb Dec 21, 2022

raducristianpopa Dec 22, 2022

wilsonianb Dec 21, 2022

raducristianpopa Dec 22, 2022

wilsonianb Dec 21, 2022

raducristianpopa Dec 22, 2022 •

edited

Loading

raducristianpopa Dec 22, 2022 •

edited

Loading

mkurapov left a comment

mkurapov Dec 23, 2022

mkurapov Dec 23, 2022

mkurapov Dec 23, 2022

mkurapov Dec 23, 2022

wilsonianb Jan 3, 2023

wilsonianb Jan 3, 2023

raducristianpopa Jan 5, 2023 •

edited

Loading

wilsonianb Jan 5, 2023

wilsonianb Jan 9, 2023

raducristianpopa Jan 10, 2023

wilsonianb Jan 10, 2023

raducristianpopa Jan 10, 2023

mkurapov Jan 10, 2023

wilsonianb Jan 3, 2023

wilsonianb Jan 3, 2023

mkurapov left a comment

wilsonianb left a comment

raducristianpopa commented Jan 13, 2023

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

-    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 = {

		@@ -15,7 +15,15 @@ export async function createOutgoingPayment(
		'quoteId'
		>

		export type CreateQuoteArgs =
		RSOperations['create-quote']['requestBody']['content']['application/json']

	export const createQuote: MutationResolvers<ApolloContext>['createQuote'] =
	async (parent, args, ctx): Promise<ResolversTypes['QuoteResponse']> => {
	const quoteService = await ctx.container.use('quoteService')
	const options: CreateQuoteOptions = {
	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(options)
	.then((quoteOrErr: Quote \| QuoteError) =>
	isQuoteError(quoteOrErr)

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

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

Conversation

raducristianpopa commented Dec 20, 2022 • edited Loading

Changes proposed in this pull request

Context

Checklist

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

raducristianpopa Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

raducristianpopa Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

mkurapov left a 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

raducristianpopa Jan 5, 2023 • edited Loading

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

Choose a reason for hiding this comment

mkurapov left a comment

Choose a reason for hiding this comment

wilsonianb left a comment

Choose a reason for hiding this comment

raducristianpopa commented Jan 13, 2023

raducristianpopa commented Dec 20, 2022 •

edited

Loading

raducristianpopa Dec 22, 2022 •

edited

Loading

raducristianpopa Dec 22, 2022 •

edited

Loading

raducristianpopa Jan 5, 2023 •

edited

Loading