938/mk/create receiver #986

mkurapov · 2023-01-17T19:06:34Z

Changes proposed in this pull request

Adding CreateReceiver mutation to GraphQL admin API, which allows creating a remote (or local) incoming payment

Context

Fixes Allow creating remote incoming payments via GraphQL API #938

Checklist

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

mkurapov · 2023-01-17T19:09:54Z

packages/backend/src/open_payments/payment/incoming_remote/service.ts

+  const { paymentPointerUrl } = args
+  const grant = await getGrant(deps, paymentPointerUrl, [
+    AccessAction.Create,
+    AccessAction.ReadAll


Do we want to have a GetRemoteIncomingPayment query in the Admin API in addition to this "Create" one?

Created separate issue, I'll mark it as nice-to-have:

Create GetReceiver GraphQL Admin Query #1005

CC @wilsonianb

mkurapov · 2023-01-17T19:13:21Z

packages/backend/src/open_payments/payment/incoming_remote/service.ts

+  openPaymentsClient: AuthenticatedClient
+}
+
+export async function createRemoteIncomingPaymentService(


In a separate PR, I want the ReceiverService to use this service to fetch remote incoming payments, since it has some similar logic currently. (ReceiverService determines whether an incoming payment (or connection) is local and remote, and fetches either)

What if the mutation was instead CreateReceiver?
I like the idea of making it clear what the purpose of this is.
We could add a ReceiverService.create (which could create locally or remotely) but still move remote management to RemoteIncomingPaymentService if we wanted.

🤔

Are you suggesting we also remove CreateIncomingPayment and consolidate both remote and incoming payment creation with CreateReceiver?

If not, is it confusing having both CreateIncomingPayment and CreateReceiver? Could someone get confused why are they different at first glance?

I was thinking we have both

CreateIncomingPayment for a user to get paid

CreateReceiver for a user to pay someone

It does fit in well with the quote creation, receiver attribute as well.

RemoteIncomingPayment GraphQL type can also be also a Receiver instead, going to make that change

mkurapov · 2023-01-17T19:15:13Z

packages/backend/src/tests/openPaymentsMocks.ts

@@ -0,0 +1,92 @@
+import { v4 as uuid } from 'uuid'


I took this from the open-payments package, since I wanted to mock the results from the open-paymentsclient calls. I could also export these mocks directly from the library if that feels like a better solution

I'm good with exported mocks

wilsonianb · 2023-01-17T20:50:29Z

packages/backend/src/open_payments/payment/incoming_remote/service.ts

+
+  const grantOptions = {
+    authServer: paymentPointer.authServer,
+    accessType: 'incoming-payment' as const,


feat(types): replace access type/action enums with generated unions #956

I was hoping 👆 would allow 👇 but 😞

Suggested change

accessType: 'incoming-payment' as const,

accessType: AccessType.IncomingPayment,

It does allow AccessType.IncomingPayment, but I still need to do as incoming-payment` when making the call for the grant request below

wilsonianb · 2023-01-17T21:04:09Z

packages/backend/src/tests/openPaymentsMocks.ts

@@ -0,0 +1,92 @@
+import { v4 as uuid } from 'uuid'


I'm good with exported mocks

wilsonianb · 2023-01-17T21:09:21Z

packages/backend/src/open_payments/payment/incoming_remote/service.ts

+  openPaymentsClient: AuthenticatedClient
+}
+
+export async function createRemoteIncomingPaymentService(


What if the mutation was instead CreateReceiver?
I like the idea of making it clear what the purpose of this is.
We could add a ReceiverService.create (which could create locally or remotely) but still move remote management to RemoteIncomingPaymentService if we wanted.

* feat(backend): add create method for receiverService * chore(backend): createRemoteIncomingPayment mutation -> createReceiver mutation * chore(backend): update ReceiverService tests to use mocks when fetching remote incoming payments * chore(backend): use open-payments mocks * chore(backend): fix receiver tests * chore(backend): update receiver tests * chore(backend): couple more updated tests

…ervice.create

mkurapov · 2023-01-19T21:22:47Z

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

@@ -245,7 +245,7 @@ export class IncomingPayment
    ilpStreamConnection: Connection
  }): OpenPaymentsIncomingPayment {
    return {
-      id: this.id,
+      id: this.url,


(Copying comment from other PR for visibility):

OpenPayment API IncomingPayments return the full url instead of just the uuid. This reflects what is done in the IncomingPaymentRoutes, but standardizes it when converting between IncomingPayment model and what Open Payments expects

mkurapov · 2023-01-19T21:28:53Z

packages/backend/src/app.ts

@@ -155,6 +157,8 @@ export interface AppServices {
  paymentPointerKeyRoutes: Promise<PaymentPointerKeyRoutes>
  paymentPointerRoutes: Promise<PaymentPointerRoutes>
  incomingPaymentService: Promise<IncomingPaymentService>
+  remoteIncomingPaymentService: Promise<RemoteIncomingPaymentService>
+  receiverService: Promise<ReceiverService>


Not new, just adding so we get proper typing when we resolve this dependency

mkurapov · 2023-01-19T21:31:50Z

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

+  return incomingPaymentOrError.toOpenPaymentsType({
+    ilpStreamConnection: connection
+  })


Once I split out the IncomingPayment types in open-payments with and without the ilpStreamConnection

Use generated Open Payments OpenAPI types #662

The corresponding connection fetching will (should?) be optional

mkurapov · 2023-01-19T21:35:34Z

Will also update the Postman collection once this is merged in

# Conflicts: # packages/backend/src/app.ts

packages/backend/src/graphql/schema.graphql

packages/backend/src/graphql/resolvers/receiver.test.ts

wilsonianb · 2023-01-19T22:46:59Z

packages/backend/src/open_payments/payment/incoming_remote/service.ts

+  }
+}
+
+async function getGrant(


Could we somehow consolidate this and

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

Line 195 in 752efd7

async function getIncomingPaymentGrant(

Yup, for sure. I was going to do that as a follow up so the PR was manageable

packages/backend/src/open_payments/payment/incoming_remote/service.ts

wilsonianb · 2023-01-19T23:22:58Z

packages/backend/src/open_payments/payment/incoming_remote/service.ts

+export interface RemoteIncomingPaymentService {
+  create(
+    args: CreateRemoteIncomingPaymentArgs
+  ): Promise<OpenPaymentsIncomingPayment>


Suggested change

): Promise<OpenPaymentsIncomingPayment>

): Promise<OpenPaymentsIncomingPayment | IncomingPaymentError>

to be able to return IncomingPaymentError.UnknownPaymentPointer (instead of throwing) and return the corresponding code and message in the createReceiver resolver

rafiki/packages/backend/src/open_payments/payment/incoming/errors.ts

Lines 1 to 2 in 752efd7

export enum IncomingPaymentError {

UnknownPaymentPointer = 'UnknownPaymentPointer',

Actually, do you mean

// ReceiverService async function createLocalIncomingPayment( deps: ServiceDependencies, args: CreateReceiverArgs, paymentPointer: PaymentPointer ): Promise<OpenPaymentsIncomingPayment | IncomingPaymentError> <---

instead? RemotePaymentService can only return the OpenPaymentsIncomingPayment since it uses the client

edit: I see what you mean, although I'd have to pass it all the way through and edit receiverService.create return type as well 🤔

I wonder if having a custom Error object with something like a userMessage property would be a better approach

No, I think it needs to be in RemotePaymentService.
The payment pointer lookup in getGrant should somehow bubble up an error to allow the resolver to return 4XX.

I wonder if having a custom Error object with something like a userMessage property would be a better approach

That'd probably work too. I'm just thinking through the lens of the other services' *Errors.

Makes sense, I'll give it a try 👍

…ngs/urls

…r receivers

mkurapov · 2023-01-23T17:34:36Z

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

+    const errorMessage = 'Could not get connection for local incoming payment'
+    deps.logger.error({ incomingPaymentOrError }, errorMessage)
+
+    throw new Error(errorMessage)


Maybe I can also return an additional type of ReceiverError here as well, together with L93?

mkurapov · 2023-01-23T17:35:39Z

packages/backend/src/open_payments/receiver/errors.ts

+export const receiverErrorToCode = (error: ReceiverError): number =>
+  isIncomingPaymentError(error)
+    ? incomingPaymentErrorToCode[error]
+    : remoteIncomingPaymentErrorToCode[error]
+
+export const receiverErrorToMessage = (error: ReceiverError): string =>
+  isIncomingPaymentError(error)
+    ? incomingPaymentErrorToMessage[error]
+    : remoteIncomingPaymentErrorToMessage[error]


I could have also just spread the two errorToMessage/errorToCode objects into a new receiver errorToMessage/errorToCode object, but thought this was easier to follow

packages/backend/src/open_payments/payment/incoming_remote/service.test.ts

packages/backend/src/open_payments/receiver/errors.ts

wilsonianb · 2023-01-23T18:14:23Z

packages/backend/src/graphql/resolvers/receiver.test.ts

+              ? serializeAmount(incomingAmount)
+              : undefined
+          })
+        ) as Receiver


Would it be worth adding a createReceiver (here or in tests/receiver) that returns a guaranteed Receiver?

Maybe it would be better to have Receiver.fromIncomingPayment/fromConnectionto throw errors instead of returning undefined? It'll allow us to return error messages back to the caller

👍 that or return Receiver | ReceiverError 😬

Ended up going with just throwing, will likely be changing out how the Receiver class works anyway if we want to go with type Receiver = IncomingPayment | Connection

…undefined

wilsonianb · 2023-01-23T19:48:31Z

packages/backend/src/open_payments/receiver/model.test.ts

-      expect(receiver).toBeUndefined()
+      const connection = connectionService.get(incomingPayment) as Connection
+
+      ;(connection.ilpAddress as string) = 'not base 64 encoded'


is this you or pnpm format?

pnpm format

mkurapov added 3 commits January 17, 2023 12:03

feat(backend): add RemoteIncomingPaymentService

c9557a6

feat(backend, map): add createRemoteIncomingPayment to graphql schema

6e0d5d7

feat(backend): add resolver for CreateRemoteIncomingPayment mutation

5f762d6

github-actions bot added pkg: backend Changes in the backend package. pkg: map Changes in mock-account-provider package type: source Changes business logic type: tests Testing related labels Jan 17, 2023

mkurapov commented Jan 17, 2023

View reviewed changes

mkurapov added 3 commits January 17, 2023 12:27

chore(backend): use new Access types, update logging

faf9e06

chore(backend): prettier for test

41f8257

chore(backend): update grantOptions.accessType

ad46549

mkurapov marked this pull request as ready for review January 17, 2023 19:58

mkurapov requested review from wilsonianb, sabineschaller and JoblersTune January 17, 2023 19:59

wilsonianb reviewed Jan 17, 2023

View reviewed changes

chore(backend, open-payments): export open payment mocks from library

63a0e0c

github-actions bot added the pkg: open-payments label Jan 18, 2023

mkurapov mentioned this pull request Jan 18, 2023

938/mk/create receiver #998

Merged

4 tasks

mkurapov added 3 commits January 18, 2023 17:50

Merge branch 'main' into 938/mk/create-remote-incoming-payment

80d47b1

Merge branch 'main' into 938/mk/create-remote-incoming-payment

e6f4c42

mkurapov changed the title ~~938/mk/create remote incoming payment~~ 938/mk/create receiver Jan 19, 2023

mkurapov added 4 commits January 19, 2023 11:51

chore(open-payments): fix uuid types

fe0524b

chore(backend): fix tests and types after merge

83af67b

chore(backend): test fixing

140e5ed

feat(backend): allow creation of local incoming payment via receiverS…

7d04e84

…ervice.create

mkurapov commented Jan 19, 2023

View reviewed changes

mkurapov requested a review from wilsonianb January 19, 2023 21:37

mkurapov mentioned this pull request Jan 19, 2023

Allow RO to revoke grant / access token #803

Closed

mkurapov added 2 commits January 19, 2023 15:17

chore(backend): fix test name

38c1061

Merge branch 'main' into 938/mk/create-remote-incoming-payment

ab033cd

# Conflicts: # packages/backend/src/app.ts

wilsonianb reviewed Jan 19, 2023

View reviewed changes

mkurapov added 4 commits January 19, 2023 16:41

chore(backend, map): make sure receiver & incomingPayment id are stri…

d2c5225

…ngs/urls

chore(backend): remove optional chaining

295913f

feat(backend): return errors when creating remote incoming payments o…

0a925c2

…r receivers

chore(backend): fix any ts warning

3f7d4fe

mkurapov commented Jan 23, 2023

View reviewed changes

mkurapov requested a review from wilsonianb January 23, 2023 17:36

wilsonianb previously approved these changes Jan 23, 2023

View reviewed changes

mkurapov added 3 commits January 23, 2023 12:28

chore(backend): addressing feedback

c906e3b

chore(backend): addressing feedback

f0ae768

feat(backend): throw in static Receiver methods instead of returning …

1194e8e

…undefined

mkurapov dismissed wilsonianb’s stale review via 1194e8e January 23, 2023 19:40

chore(backend): remove unnecessary type assertion in tests

9ba9314

wilsonianb previously approved these changes Jan 23, 2023

View reviewed changes

chore(backend): replace type assertions with asserts

7596fbb

mkurapov dismissed wilsonianb’s stale review via 7596fbb January 23, 2023 19:57

Merge branch 'main' into 938/mk/create-remote-incoming-payment

3221c35

mkurapov requested a review from wilsonianb January 23, 2023 20:11

wilsonianb approved these changes Jan 23, 2023

View reviewed changes

mkurapov merged commit d40c2e6 into main Jan 24, 2023

mkurapov deleted the 938/mk/create-remote-incoming-payment branch January 24, 2023 00:59

wilsonianb mentioned this pull request Jan 25, 2023

feat(backend): rotate expired access token #1028

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

938/mk/create receiver #986

938/mk/create receiver #986

mkurapov commented Jan 17, 2023 •

edited

Loading

mkurapov Jan 17, 2023

mkurapov Jan 19, 2023 •

edited

Loading

mkurapov Jan 17, 2023

wilsonianb Jan 17, 2023

mkurapov Jan 17, 2023

wilsonianb Jan 17, 2023 •

edited

Loading

mkurapov Jan 18, 2023

mkurapov Jan 17, 2023 •

edited

Loading

wilsonianb Jan 17, 2023

wilsonianb Jan 17, 2023

mkurapov Jan 17, 2023

wilsonianb Jan 17, 2023

wilsonianb Jan 17, 2023

mkurapov Jan 19, 2023

mkurapov Jan 19, 2023

mkurapov Jan 19, 2023

mkurapov commented Jan 19, 2023

wilsonianb Jan 19, 2023

mkurapov Jan 19, 2023

wilsonianb Jan 19, 2023

mkurapov Jan 19, 2023

mkurapov Jan 19, 2023 •

edited

Loading

mkurapov Jan 19, 2023

wilsonianb Jan 19, 2023 •

edited

Loading

wilsonianb Jan 19, 2023

mkurapov Jan 20, 2023

mkurapov Jan 23, 2023

mkurapov Jan 23, 2023

wilsonianb Jan 23, 2023

mkurapov Jan 23, 2023 •

edited

Loading

wilsonianb Jan 23, 2023

mkurapov Jan 23, 2023

wilsonianb Jan 23, 2023

mkurapov Jan 23, 2023

	accessType: 'incoming-payment' as const,
	accessType: AccessType.IncomingPayment,

	): Promise<OpenPaymentsIncomingPayment>
	): Promise<OpenPaymentsIncomingPayment \| IncomingPaymentError>

	export enum IncomingPaymentError {
	UnknownPaymentPointer = 'UnknownPaymentPointer',

938/mk/create receiver #986

938/mk/create receiver #986

Conversation

mkurapov commented Jan 17, 2023 • edited Loading

Changes proposed in this pull request

Context

Checklist

Choose a reason for hiding this comment

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

wilsonianb Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov Jan 17, 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 commented Jan 19, 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

mkurapov Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb Jan 19, 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

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

mkurapov commented Jan 17, 2023 •

edited

Loading

mkurapov Jan 19, 2023 •

edited

Loading

wilsonianb Jan 17, 2023 •

edited

Loading

mkurapov Jan 17, 2023 •

edited

Loading

mkurapov Jan 19, 2023 •

edited

Loading

wilsonianb Jan 19, 2023 •

edited

Loading

mkurapov Jan 23, 2023 •

edited

Loading