fix(backend): remove wallet address from OP response id and fix receiver service #2115

sabineschaller · 2023-10-25T13:17:27Z

Changes proposed in this pull request

removes wallet address from id in OP responses (incoming payment, quote, outgoing payment)
adds auth server url to public incoming payment response
fixes receiver service to get public incoming payment instead of wallet address to figure out where to find the auth server
fixes getting the wallet address on resource GET requests through the wallet address middleware

Context

fixes Remove wallet address from path of URL returned in create incoming payment response's id field #2098
fixes POST /quotes cannot retrieve incoming payment #2109
fixes GET /incoming-payment not able to find incoming payment in db #2100

Checklist

…ver service

BlairCurrey · 2023-10-25T14:51:39Z

packages/backend/src/open_payments/wallet_address/middleware.ts

I take it the changes here mean we can remove the wallet address from body/query params in the open payments spec for /quotes, /incoming-payments, /outgoing-payments, correct? Since we're just getting it via the resource id.

mkurapov

first pass

mkurapov · 2023-10-25T14:40:50Z

packages/backend/src/app.ts

@@ -477,6 +477,7 @@ export class App {
    // Read incoming payment
    router.get<DefaultState, SubresourceContextWithAuthenticatedStatus>(
      '/incoming-payments/:id',
+      createWalletAddressMiddleware(),


If this middleware is already fetching the resources, does it make sense to also then call incomingPaymentRoutes.get?

(applies to all of them)

We could (this isn't great per se) store the fetched resource right on the context, and the skip the second lookup if we already have something defined in the ctx

As per call, disregard ^

Please create issue for that @mkurapov

mkurapov · 2023-10-25T14:49:53Z

packages/backend/src/open_payments/wallet_address/middleware.ts

+          id: ctx.params.id
+        })
+        if (!incomingPayment || !incomingPayment.walletAddress) {
+          ctx.throw(401)


401 as to not give away whether the resource exists or not?

I wasn't really thinking that through but I think that counts as reasoning ;-)

mkurapov · 2023-10-25T14:51:21Z

packages/backend/src/open_payments/wallet_address/middleware.ts

-        ctx.walletAddressUrl = ctx.query['wallet-address'] as string
-      } else if (ctx.method === 'POST') {
-        ctx.walletAddressUrl = (ctx.request.body as CreateBody).walletAddress
+    if (ctx.method === 'GET') {


We should break this up into multiple middlewares or at least separate functions in this file

Later. Please create issue 😉

mkurapov · 2023-10-25T14:56:59Z

@BlairCurrey

I take it the changes here mean we can remove the wallet address from body/query params in the open payments spec for /quotes, /incoming-payments, /outgoing-payments, correct? Since we're just getting it via the resource id.

We still need it, during resource creation we don't have the resource id yet so we need a way to specify for which walletAddress the request is for

Likewise when we are GETting a list of resources, we don't have just a single resource to go off of

mkurapov · 2023-10-25T15:22:13Z

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

+    return undefined
+  }
+  const publicIncomingPayment = await publicIncomingPaymentResponse.json()
+  if (!publicIncomingPayment || !publicIncomingPayment.authServer) {


do we have an issue for adding authServer to the public incoming payment?

Not yet. It also needs to be added to the spec. Once we have that, we can use the op sdk

njlie · 2023-10-25T15:41:45Z

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

    })
-    ctx.body = incomingPayment?.toPublicOpenPaymentsType()
+    ctx.body = incomingPayment?.toPublicOpenPaymentsType(
+      deps.config.authServerGrantUrl


Would there ever be a reason we'd need to return this in the private incoming payment response as well?

I don't think so because if you can get a private one, you've already gone through the auth flow.

njlie · 2023-10-25T15:54:32Z

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

-  })
-  if (!walletAddress) {
+  // TODO: replace with OP client method
+  // const publicIncomingPayment =


nit: remove comment?

Should we keep it to not forget?

njlie · 2023-10-25T15:59:25Z

packages/backend/src/open_payments/wallet_address/middleware.ts

        ctx.walletAddressUrl = ctx.query['wallet-address'] as string
-      } else if (ctx.method === 'POST') {
-        ctx.walletAddressUrl = (ctx.request.body as CreateBody).walletAddress
+        console.log(ctx.walletAddressUrl)


Want to keep this console.log?

* feat(backend): rename "payment pointer" tables & columns to use "wallet address" (#1937) Co-authored-by: Nathan Lie <nathanlie@Nathans-MacBook-Pro.local> * feat(backend, mock-ase): rename payment pointer to wallet address in rafiki code (#2029) * fix(backend): rename paymentPointerId column in combinedPaymentsView to walletAddressId (#2043) * docs: replace payment pointer with wallet address (#1951) * feat(backend): remove connections route (#1940) (#2051) Co-authored-by: Ömer Talha Özdemir <omer@fynbos.dev> * feat(backend): unauthenticated get incoming payment (#1952) (#2052) Co-authored-by: Ömer Talha Özdemir <omer@fynbos.dev> Co-authored-by: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> * feat(auth): fix auth tests for access tokens (#2053) * feat(backend): remove payment pointer from url, retrieve it from request query or body (#2042) * refactor(backend): swap connection for methods (#2054) Co-authored-by: Nathan Lie <lie4nathan@gmail.com> * fix(backend): resolve breaking build after main rebase (#2085) * fix(backend): combined_payment, ilp service tests (#2094) * fix(backend): combined_payment, ilp service tests * chore(backend): format * fix(backend): bad test refactor * fix(postman): invalid json (#2096) * fix: fetch walletAddress graph in quote and outgoing payment service (#2099) * fix(postman): env var usage (#2107) * feat(backend): add quotes route to exceptions in wallet address middleware (#2108) * chore: formatting * fix(backend): fix GET /incoming-payments (#2111) * fix(backend): remove wallet address from OP response id and fix receiver service (#2115) * fix: remove wallet address from op resource urls * feat: return auth server url in public incoming payment and fix receiver service * fix: quote issue * fix: walletAddress middleware * chore: update postman * fix: listing * chore: remove console.log * fix: progress towards making listing work * fix(backend): fix create remote incoming payment (#2121) * fix(postman): misc postman fixes * fix(postman): complete incoming payment (#2126) * fix: file name correction (#2127) * fix: GET resource lists, use op client (#2122) * fix(postman): update list urls * fix(openapi): rm defaults for first, last * refactor(postman): query param order for readability * fix(backend): update to new open-payments pkgs * fix(backend): use op client to get authServer * fix(backend): rm old code * fix(backend): receiver tests * chore: cleanup * feat: make list query parameter configurable * chore: update OP package and fix types --------- Co-authored-by: Sabine Schaller <sabine@interledger.org> * fix(postman): additional fixes * fix: wallet address on the fly creation (#2129) * chore: update postman environments * Postman: update description for P2P example * Postman: Remove finalizationReason from getGrants API + fix createWalletAddress * chore: bump open-payments package in auth * chore: update last few remaining mentions of payment pointers * chore(postman): remove references to payment pointer * chore: update another reference to pp --------- Co-authored-by: Nathan Lie <nathanlie@Nathans-MacBook-Pro.local> Co-authored-by: Ömer Talha Özdemir <omer@fynbos.dev> Co-authored-by: Blair Currey <12960453+BlairCurrey@users.noreply.github.com> Co-authored-by: Sabine Schaller <sabine@interledger.org> Co-authored-by: Sarah Jones <sarah38186@gmail.com> Co-authored-by: Max Kurapov <maxkurapov@gmail.com> Co-authored-by: Max Kurapov <max@interledger.org>

sabineschaller added 4 commits October 24, 2023 21:18

fix: remove wallet address from op resource urls

dcad031

feat: return auth server url in public incoming payment and fix recei…

b465df2

…ver service

Merge branch 'nl-open-payments-updates' into s2-fix-op-responses

85f7cfb

fix: quote issue

a641437

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Oct 25, 2023

fix: walletAddress middleware

fba0e17

sabineschaller marked this pull request as ready for review October 25, 2023 14:34

sabineschaller requested review from njlie, BlairCurrey and mkurapov October 25, 2023 14:34

chore: update postman

34a8b78

BlairCurrey reviewed Oct 25, 2023

View reviewed changes

mkurapov reviewed Oct 25, 2023

View reviewed changes

BlairCurrey mentioned this pull request Oct 25, 2023

Fix GET /incoming-payments list #2116

Closed

mkurapov reviewed Oct 25, 2023

View reviewed changes

fix: listing

1f74c74

BlairCurrey mentioned this pull request Oct 25, 2023

Un-hack public incoming payment request for authServer #2119

Closed

njlie reviewed Oct 25, 2023

View reviewed changes

sabineschaller added 2 commits October 25, 2023 19:01

chore: remove console.log

8964000

fix: progress towards making listing work

ab4df37

sabineschaller requested review from BlairCurrey, mkurapov and njlie October 25, 2023 17:16

BlairCurrey approved these changes Oct 25, 2023

View reviewed changes

sabineschaller merged commit 6aea8ba into nl-open-payments-updates Oct 25, 2023

sabineschaller deleted the s2-fix-op-responses branch October 25, 2023 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(backend): remove wallet address from OP response id and fix receiver service #2115

fix(backend): remove wallet address from OP response id and fix receiver service #2115

sabineschaller commented Oct 25, 2023 •

edited

Loading

BlairCurrey Oct 25, 2023 •

edited

Loading

mkurapov left a comment

mkurapov Oct 25, 2023

mkurapov Oct 25, 2023

mkurapov Oct 25, 2023

sabineschaller Oct 25, 2023

mkurapov Oct 25, 2023

sabineschaller Oct 25, 2023

mkurapov Oct 25, 2023

sabineschaller Oct 25, 2023

mkurapov commented Oct 25, 2023

mkurapov Oct 25, 2023

sabineschaller Oct 25, 2023

BlairCurrey Oct 25, 2023

njlie Oct 25, 2023

sabineschaller Oct 25, 2023

njlie Oct 25, 2023

sabineschaller Oct 25, 2023

njlie Oct 25, 2023

sabineschaller Oct 25, 2023

fix(backend): remove wallet address from OP response id and fix receiver service #2115

fix(backend): remove wallet address from OP response id and fix receiver service #2115

Conversation

sabineschaller commented Oct 25, 2023 • edited Loading

Changes proposed in this pull request

Context

Checklist

BlairCurrey Oct 25, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkurapov commented Oct 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabineschaller commented Oct 25, 2023 •

edited

Loading

BlairCurrey Oct 25, 2023 •

edited

Loading