662/mk/use open payments types #1024

mkurapov · 2023-01-24T22:52:29Z

Changes proposed in this pull request

Exporting additional IncomingPayment types from open-payments package
Adding toOpenPaymentsType() method for subresources to convert from model to the corresponding open-payments type, removing toJSON types and method where necessary
use toOpenPaymentsType() methods in the route handlers when creating response

Context

Fixes Use generated Open Payments OpenAPI types #662

Checklist

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

mkurapov · 2023-01-26T18:34:46Z

Currently running into this challenge: when calling .toOpenPaymentsType on a model, I need the corresponding paymentPointer.url. However, other than the IncomingPayment model, Model.paymentPointer is optional, since it needs to be fetched from the DB. The IncomingPayment model gets around this via .withGraphFetched(..., 'paymentPointer') existing on every IncomingPayment query, but I feel like it could be easy to miss when making those calls in the service, and as a result, erroring when expecting the non-nullable paymentPointer field. A few options exist here:

Go with the IncomingPayment method, add a non-null assertion (paymentPointer!) to each of the PaymentPointerSubresources, and add .withGraphFetched(..., 'paymentPointer') everytime when requesting the model from the DB.
Make toOpenPaymentsType async, and have a helper method getPaymentPointer/getWithPaymentPointer that fetches the paymentPointer if doesn't already exist on the model instance. This is pretty straightforward but will require some finessing converting all methods that call toOpenPaymentsType to be async
Make paymentPointer non-nullable on PaymentPointerSubresources by always eager fetching it. I'm exploring a few ways of doing this, one of them (possibly?) being as such:

class SubresourceQueryBuilder {
  ...
  execute() {
    this.withGraphFetched('paymentPointer')
    return super.execute()
  }
}

related issue

Am I overthinking this, and should just go with the standard way we have it now via .withGraphFetched method, like option 1, since we are already doing this with asking for asset on almost every PaymentSubresource query?

@wilsonianb @sabineschaller

wilsonianb · 2023-01-26T19:40:54Z

Reading option 1 made think of doing something like option 3.

Adrian's idea 👇 might eventually help with this.

Make resource collection URLs explicit open-payments#206

mkurapov · 2023-01-27T08:15:04Z

I was trying it out with the example above of overriding execute(), it seemed to work. I was testing out with the debugger, seeing all of the different SQL calls that were made, and even though its straightforward request, I'm a bit concerned that every single request, insert, get, update, it would do this DB call.

Instead of solving it programmatically, what if we just stored a paymentPointerUrl foreign key on the PaymentPointerSubresource models? It is already a non-nullable unique index.

Otherwise, I'm leaning toward making toOpenPaymentsType async, since we fully control when we need that data, and there will never be a possibility of us not adding a field to withGraphFetched

wilsonianb · 2023-01-27T13:22:46Z

I vote async toOpenPaymentsType over storing paymentPointerUrl on each subresource
~~but only if we rename every synchronous function *Sync to make it clear that toOpenPaymentsType is async~~ 😜

mkurapov · 2023-01-28T02:38:02Z

packages/backend/src/open_payments/payment/incoming/model.ts

@@ -84,7 +84,6 @@ export class IncomingPayment
    }
  }

-  public paymentPointer!: PaymentPointer


no non-null assertion since we can start removing withGraphFetched('paymentPointer') from the DB calls

mkurapov · 2023-01-28T02:38:29Z

packages/backend/src/open_payments/payment/incoming/model.ts

@@ -131,8 +130,15 @@ export class IncomingPayment
    this.receivedAmountValue = amount.value
  }

-  public get url(): string {
-    return `${this.paymentPointer.url}${IncomingPayment.urlPath}/${this.id}`
+  public async getUrl(fetchedPaymentPointer?: PaymentPointer): Promise<string> {


Adding fetchedPaymentPointer in-case you already have the paymentPointer and don't want to grab it again, like in toOpenPaymentsType

What if callers just assigned incomingPayment.paymentPointer = fetchedPaymentPointer, so that getUrl+getPaymentPointer only dealt with whether this.paymentPointer is defined or not?

Taking a step back, if/when we change subresource urls to not include the payment pointer, we'll no longer need an async getUrl.

Make resource collection URLs explicit open-payments#206

I'm hesitant to do that, since
a) we either have to change the type definitions of getUrl/toOpenPaymentsType to possibly return undefined, or throw inside the method if paymentPointer isn't defined
b) it's not entirely clear that the caller has to do that in order to make these methods work.

Maybe we can

have toOpenPaymentsType take in a paymentPointer (this would also make it sync), it would require passing in ctx.paymentPointer in most places but I think that's ok

A bit more complicated, but we make a separate <subresource>WithPaymentPointer class that constructs a new model with the paymentPointer defined, although I'm not sure the additional boilerplate is worth it

To clarify, I'm only suggesting changing getUrl to

Suggested change

public async getUrl(fetchedPaymentPointer?: PaymentPointer): Promise<string> {

public async getUrl(): Promise<string> {

and callers can optionally define incomingPayment.paymentPointer to avoid $relatedQuery in getPaymentPointer.

mkurapov · 2023-01-28T07:18:58Z

packages/backend/src/open_payments/payment/outgoing/model.ts

+    return {
+      id: await this.getUrl(paymentPointer),
+      paymentPointer: paymentPointer.url,
+      quoteId: (await this.quote?.getUrl()) ?? undefined,


this is optional in the Open Payments API -> should we keep it?

🤔 I wonder if we should make it required in the spec

If you can only create an Outgoing Payment with a required quoteId, it would make sense that this would be required here

mkurapov · 2023-01-28T07:19:42Z

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

@@ -66,10 +68,7 @@ export const listSubresource = async <M extends PaymentPointerSubresource>({
  )
  const result = {
    pagination: pageInfo,
-    result: page.map((item: M) => {
-      item.paymentPointer = ctx.paymentPointer


We are fetching paymentPointer on the fly (whenever it's missing), this line isn't necessary

mkurapov · 2023-01-30T16:29:18Z

packages/backend/src/open_payments/payment/incoming/model.ts

-  id: string
-  paymentPointer: string
-  incomingAmount?: AmountJSON
-  receivedAmount: AmountJSON


@wilsonianb Keeping AmountJSON over OpenPaymentsAmount makes sense right? The Amount interface is a more generic thing we use across the code, not necessarily being an "open payments" concept

👍 I'm good with keeping amount values as strings in open-payments and leaving it to callers to (de)serialize.

mkurapov · 2023-01-30T16:29:41Z

Going to update the auth package with a few of it's types in a follow-up

wilsonianb · 2023-01-24T22:58:20Z

packages/backend/src/open_payments/payment/incoming/model.ts

+  ):
+    | OpenPaymentsIncomingPayment
+    | OpenPaymentsIncomingPaymentWithConnection
+    | OpenPaymentsIncomingPaymentWithConnectionUrl {


I was going to suggest adding an abstract toOpenPaymentsType to PaymentPointerSubresource but this one for incoming payments would complicate it

rafiki/packages/backend/src/open_payments/payment_pointer/model.ts

Line 165 in 16a4f82

export abstract class PaymentPointerSubresource extends BaseModel {

wilsonianb · 2023-01-30T17:35:37Z

packages/backend/src/open_payments/payment/incoming/routes.ts

+      toBody: async (payment) =>
+        await incomingPaymentToBody(
+          payment,
+          deps.connectionService.getUrl(payment)


Could we either make this getUrl async for consistency or rename it?

I think it should be ok as it is, since it's a service method, not a model?

I'm also ok keeping it as is.
It just stood out against the new async getUrls

wilsonianb · 2023-01-30T17:41:34Z

packages/backend/src/open_payments/payment/outgoing/model.ts

+    return {
+      id: await this.getUrl(paymentPointer),
+      paymentPointer: paymentPointer.url,
+      quoteId: (await this.quote?.getUrl()) ?? undefined,


🤔 I wonder if we should make it required in the spec

wilsonianb · 2023-01-30T17:45:32Z

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

@@ -80,7 +80,7 @@ async function createQuote(
    }

    ctx.status = 201
-    ctx.body = quoteToBody(quoteOrErr)
+    ctx.body = await quoteToBody(quoteOrErr)


Suggested change

ctx.body = await quoteToBody(quoteOrErr)

ctx.body = await quoteOrErr.toOpenPaymentsType()

Remove quoteToBody

wilsonianb · 2023-01-30T17:49:37Z

packages/backend/src/open_payments/payment/incoming/model.ts

@@ -131,8 +130,15 @@ export class IncomingPayment
    this.receivedAmountValue = amount.value
  }

-  public get url(): string {
-    return `${this.paymentPointer.url}${IncomingPayment.urlPath}/${this.id}`
+  public async getUrl(fetchedPaymentPointer?: PaymentPointer): Promise<string> {


What if callers just assigned incomingPayment.paymentPointer = fetchedPaymentPointer, so that getUrl+getPaymentPointer only dealt with whether this.paymentPointer is defined or not?

* chore(backend): make toOpenPaymentType sync * chore(backend): make quote.toOpenPaymentsType sync * chore(backend): convert tests to sync * chore(backend): update tests

wilsonianb

Oops, here's some several day old comments that I didn't realize were pending in a review

wilsonianb · 2023-01-30T17:54:49Z

packages/backend/src/open_payments/payment/incoming/model.ts

-  id: string
-  paymentPointer: string
-  incomingAmount?: AmountJSON
-  receivedAmount: AmountJSON


👍 I'm good with keeping amount values as strings in open-payments and leaving it to callers to (de)serialize.

wilsonianb · 2023-01-30T17:59:13Z

packages/backend/src/open_payments/payment/incoming/model.ts

@@ -131,8 +130,15 @@ export class IncomingPayment
    this.receivedAmountValue = amount.value
  }

-  public get url(): string {
-    return `${this.paymentPointer.url}${IncomingPayment.urlPath}/${this.id}`
+  public async getUrl(fetchedPaymentPointer?: PaymentPointer): Promise<string> {


Taking a step back, if/when we change subresource urls to not include the payment pointer, we'll no longer need an async getUrl.

Make resource collection URLs explicit open-payments#206

wilsonianb · 2023-01-30T18:19:27Z

packages/backend/src/open_payments/payment/incoming/routes.ts

+      toBody: async (payment) =>
+        await incomingPaymentToBody(
+          payment,
+          deps.connectionService.getUrl(payment)


I'm also ok keeping it as is.
It just stood out against the new async getUrls

wilsonianb · 2023-01-30T19:10:32Z

packages/backend/src/open_payments/payment/incoming/model.ts

@@ -131,8 +130,15 @@ export class IncomingPayment
    this.receivedAmountValue = amount.value
  }

-  public get url(): string {
-    return `${this.paymentPointer.url}${IncomingPayment.urlPath}/${this.id}`
+  public async getUrl(fetchedPaymentPointer?: PaymentPointer): Promise<string> {


To clarify, I'm only suggesting changing getUrl to

Suggested change

public async getUrl(fetchedPaymentPointer?: PaymentPointer): Promise<string> {

public async getUrl(): Promise<string> {

and callers can optionally define incomingPayment.paymentPointer to avoid $relatedQuery in getPaymentPointer.

wilsonianb · 2023-02-02T16:23:39Z

packages/backend/src/open_payments/payment/incoming/model.ts

+    | OpenPaymentsIncomingPaymentWithConnection
+    | OpenPaymentsIncomingPaymentWithConnectionUrl
+
+  public toOpenPaymentsType(


What if we added some model.test.tss at least to test toOpenPaymentsType?
Or are existing response body checks in routes tests good enough?

wilsonianb · 2023-02-02T16:28:58Z

packages/backend/src/open_payments/payment/incoming/routes.test.ts

-              assetScale: incomingPayment.receivedAmount.assetScale
-            }
-      }),
+      getBody: (incomingPayment, list) => {


Could this instead be

Suggested change

getBody: (incomingPayment, list) => {

getBody: (incomingPayment, list) => incomingPayment.toOpenPaymentsType(paymentPointer)

with some possible special assignment for ilpStreamConnection?

In terms of the connection, we would be able to do something like:

getBody: (incomingPayment, list) => ({ ...incomingPayment.toOpenPaymentsType(paymentPointer), ilpStreamConnection: list ? `${config.openPaymentsUrl}/connections/${incomingPayment.connectionId}` : { id: `${config.openPaymentsUrl}/connections/${incomingPayment.connectionId}`, ilpAddress: expect.stringMatching( /^test\.rafiki\.[a-zA-Z0-9_-]{95}$/ ), sharedSecret: expect.stringMatching(/^[a-zA-Z0-9-_]{43}$/), assetCode: incomingPayment.receivedAmount.assetCode, assetScale: incomingPayment.receivedAmount.assetScale } }),

Since incomingPayment.toOpenPaymentsType expect a Connection, we wouldn't be able to use expect.stringMatching(...), but would have to create a Connection instance with proper credentials etc

i.e. I'm not sure if that's a much better solution

packages/backend/src/open_payments/payment/outgoing/model.ts

wilsonianb · 2023-02-02T18:59:08Z

packages/backend/src/open_payments/payment/incoming/model.test.ts

+  })
+
+  describe('toOpenPaymentsType', () => {
+    test('returns incoming payment without connection provided', async () => {


What if there was a single test.each to make clear that output is the same other than ilpStreamConnection?

I was thinking this, but having a test case where the connection var in the test.each case was either

const connection = `${config.openPaymentsUrl}/connections/${incomingPayment.connectionId}`

or

const connection = Connection.fromPayment({ payment: incomingPayment, openPaymentsUrl: config.openPaymentsUrl, credentials: { ilpAddress: 'test.ilp' as IlpAddress, sharedSecret: Buffer.from('') } })

didn't feel nice

packages/backend/src/open_payments/payment/outgoing/model.ts

packages/open-payments/src/test/helpers.ts

mkurapov added 5 commits January 24, 2023 15:49

feat(open-payments): export the different IncomingPayment types

c3ccd88

feat(backend): use open-payment types for IncomingPayment

3415f5f

feat(backend): use open-payment types for Connection

4693a79

feat(backend): use open-payment types for OutgoingPayment

c633b30

feat(backend): use open-payment types for Quote

909462a

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

mkurapov added 10 commits January 24, 2023 15:55

chore(backend): fix IncomingPayments.toOpenPaymentsType

8feaacb

chore(open-payments): update mocks

5300073

chore(backend): formatting

01a0ba0

chore(backend): add better typing for incomingPayment.toOpenPaymentsType

36d8b8e

chore(backend): remove QuoteJSON

d04f994

Merge branch 'main' into 662/mk/use-open-payments-types

379aee8

chore(open-payments): fix mocks

11fa542

chore(backend): fix IncomingPayment types

27cfdde

chore(open-payments): fix incoming payment types

4efddb3

chore(backend): fix incoming payment method

dc051ec

chore(backend): make toOpenPaymentsType async

0fddf9e

mkurapov commented Jan 28, 2023

View reviewed changes

mkurapov added 4 commits January 27, 2023 19:40

chore(backend): getUrl for OutgoingPayment

ad57048

chore(backend): fix tests with async toOpenPaymentsType

d48dc0a

chore(backend): return undefined instead of null

9302bad

chore(backend): add awaits where necessary

5e5d4e8

mkurapov added 3 commits January 27, 2023 22:47

chore(open-payments): make getBody async

25972d3

chore(backend): fix outgoingPayment model method

4800c80

chore(backend): fix list tests

cf95410

mkurapov commented Jan 28, 2023

View reviewed changes

chore(backend): update receiver tests

9a967f1

mkurapov marked this pull request as ready for review January 30, 2023 07:19

mkurapov requested review from wilsonianb and sabineschaller January 30, 2023 15:03

mkurapov commented Jan 30, 2023

View reviewed changes

wilsonianb reviewed Jan 30, 2023

View reviewed changes

Merge branch 'main' into 662/mk/use-open-payments-types

e5f3c03

mkurapov mentioned this pull request Feb 1, 2023

chore(backend): make toOpenPaymentType sync #1056

Merged

4 tasks

chore(backend): make toOpenPaymentType sync (#1056)

5733224

* chore(backend): make toOpenPaymentType sync * chore(backend): make quote.toOpenPaymentsType sync * chore(backend): convert tests to sync * chore(backend): update tests

mkurapov requested a review from wilsonianb February 2, 2023 13:20

wilsonianb reviewed Feb 2, 2023

View reviewed changes

mkurapov added 4 commits February 2, 2023 18:41

chore(backend): remove $formatJson where applicable

28a34df

chore(backend): remove $formatJson where applicable

5e95486

chore(backend): add test for incomingPayment model

b6d25c6

chore(backend): fix incomingPayment model test

09a0c10

wilsonianb reviewed Feb 2, 2023

View reviewed changes

mkurapov added 3 commits February 2, 2023 22:08

chore(open-payments): use suggestion

c079942

chore(backend): fix test

c55e586

chore(backend): use suggestion

fc311e2

wilsonianb approved these changes Feb 2, 2023

View reviewed changes

mkurapov merged commit 8a2dff6 into main Feb 2, 2023

mkurapov deleted the 662/mk/use-open-payments-types branch February 2, 2023 21:46

mkurapov mentioned this pull request Feb 3, 2023

feat(backend): rotate expired access token #1028

Merged

4 tasks

	public async getUrl(fetchedPaymentPointer?: PaymentPointer): Promise<string> {
	public async getUrl(): Promise<string> {

	ctx.body = await quoteToBody(quoteOrErr)
	ctx.body = await quoteOrErr.toOpenPaymentsType()

	getBody: (incomingPayment, list) => {
	getBody: (incomingPayment, list) => incomingPayment.toOpenPaymentsType(paymentPointer)

662/mk/use open payments types #1024

662/mk/use open payments types #1024

Conversation

mkurapov commented Jan 24, 2023 • edited Loading

Changes proposed in this pull request

Context

Checklist

mkurapov commented Jan 26, 2023 • edited Loading

wilsonianb commented Jan 26, 2023

mkurapov commented Jan 27, 2023

wilsonianb commented Jan 27, 2023

mkurapov Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

mkurapov Jan 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov commented Jan 30, 2023

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

wilsonianb 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

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 commented Jan 24, 2023 •

edited

Loading

mkurapov commented Jan 26, 2023 •

edited

Loading

mkurapov Jan 28, 2023 •

edited

Loading

mkurapov Jan 28, 2023 •

edited

Loading