feat: add wallet address to subresource requests #305

njlie · 2023-09-26T22:18:01Z

Changes proposed in this pull request

Adds the walletAddress field to request bodies for:
- POST /outgoing-payments
- POST /incoming-payments
- POST /incoming-payments/{id}/complete
Adds the wallet-address query parameter to requests for:
- GET /incoming-payments/{id}
- GET /outgoing-payments/{id}
- GET /incoming-payments
- GET /outgoing-payments

Context

Fixes #304.

changeset-bot · 2023-09-26T22:18:05Z

🦋 Changeset detected

Latest commit: 0f67aaf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@interledger/open-payments	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2023-09-26T22:18:06Z

✅ Deploy Preview for openpayments-preview ready!

Name	Link
🔨 Latest commit	`0f67aaf`
🔍 Latest deploy log	https://app.netlify.com/sites/openpayments-preview/deploys/65270e120484820008c9f343
😎 Deploy Preview	https://deploy-preview-305--openpayments-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

njlie · 2023-09-26T22:19:02Z

Still need to add a changeset before this will be marked ready for review.

mkurapov

What do you think about always using the walletAddress in the query params, both in the GETs and the POSTs? To keep it consistent?

In any case, we should make wallet address always required + update the client package

njlie · 2023-09-28T16:33:15Z

What do you think about always using the walletAddress in the query params, both in the GETs and the POSTs? To keep it consistent?

In any case, we should make wallet address always required + update the client package

Do we need to support RESTful clients? I think those don't allow for query params in POSTs.

mkurapov

Do we need to support RESTful clients? I think those don't allow for query params in POSTs.

I wouldn't say that having query params in POST is necessarily not RESTful, but I wasn't sure of the tradeoff between the weirdness of that vs needing to pass in the walletAddress differently depending on the method. We can just go with the more standard approach of no query params, was just curious to hear thoughts

Also, for the usage of the client, we can actually abstract that away by having it take in walletAddress as it's own parameter and do what we need to do internally in the client depending on the method:

client.incomingPayment.get({ url: '<some URL for the RS>', walletAddress: '<walletAddress>'...})  // uses wallet address in query params
client.incomingPayment.create({ url: '<some URL for the RS>', walletAddress: '<walletAddress', accessToken: '' }, { expiresAt: ... }) // uses wallet address in request body

mkurapov · 2023-10-02T18:01:13Z

openapi/resource-server.yaml

+            examples:
+              Complete incoming payment for https://openpayments.guide/alice:
+                value:
+                  walletAddress: 'https://openpayments.guide/alice/'


maybe this can just be the ilp.money wallet address one will be good so it works with Blair's PR regardless who merges first

mkurapov · 2023-10-02T18:02:03Z

openapi/resource-server.yaml

@@ -334,6 +339,9 @@ paths:
            schema:
              type: object
              properties:
+                walletAddress:


POST /quote request body should also contain walletAddress

njlie · 2023-10-02T18:28:09Z

Do we need to support RESTful clients? I think those don't allow for query params in POSTs.

I wouldn't say that having query params in POST is necessarily not RESTful, but I wasn't sure of the tradeoff between the weirdness of that vs needing to pass in the walletAddress differently depending on the method. We can just go with the more standard approach of no query params, was just curious to hear thoughts

Also, for the usage of the client, we can actually abstract that away by having it take in walletAddress as it's own parameter and do what we need to do internally in the client depending on the method:
client.incomingPayment.get({ url: '<some URL for the RS>', walletAddress: '<walletAddress>'...})  // uses wallet address in query params
client.incomingPayment.create({ url: '<some URL for the RS>', walletAddress: '<walletAddress', accessToken: '' }, { expiresAt: ... }) // uses wallet address in request body

Yeah, I'm thinking the no query parameters in POST will work. If someone asks "why did they do it this way?", then it may be a stronger argument to point to that dogma than a personal preference for consistency

mkurapov

LGTM, not sure if this should be merged before or after #307, however

mkurapov · 2023-10-10T14:16:35Z

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

    AuthenticatedRequestArgs {}

-export interface CollectionRequestArgs extends AuthenticatedRequestArgs {
+export interface ResourceOrCollectionRequestArgs


Just a thought I had, should we change the name of the url argument to be instead resourceServerUrl for the POST requests (since we end up appending the resource name, e.g. /incoming-payment), to be more clear that we will be appending the resource name during those requests?

I think that can be part of Adrian's PR

feat: extract wallet address from RS #315

Right now, wallet address and all other resources are part of the same spec with the same server url. As soon as wallet address moves to its own spec, this makes sense.

Sorry, this 👆was supposed to be a comment to you question below.

mkurapov · 2023-10-10T15:23:08Z

Do we need to return the resourceServerUrl in the wallet address lookup?

njlie · 2023-10-10T17:15:31Z

Do we need to return the resourceServerUrl in the wallet address lookup?

I think #305 (comment) also covers this. I think this version of Open Payments doesn't need to account for resource servers differing from the wallet address until Adrian's PR. Just need to make sure it's captured in an issue or in the scope of that draft PR

…on to component

…s client

sabineschaller

One small little thing

sabineschaller · 2023-10-11T09:14:01Z

openapi/resource-server.yaml

@@ -650,6 +671,7 @@ paths:
      description: A client can fetch the latest state of an incoming payment to determine the amount received into the wallet address.
    parameters:
      - $ref: '#/components/parameters/id'
+      - $ref: '#/components/parameters/wallet-address'


I don't think the wallet address is needed here because the id is unique.

Is it also the case for completing an incoming payment (https://github.com/interledger/open-payments/pull/305/files#diff-9debc2038853698222051b2c2a428cee93e9dda6af6076de559971933879580dR722-R725) since the path still contains that unique id?

Yes, indeed.

sabineschaller · 2023-10-11T09:14:44Z

openapi/resource-server.yaml

@@ -748,6 +785,7 @@ paths:
        - $ref: '#/components/parameters/signature'
    parameters:
      - $ref: '#/components/parameters/id'
+      - $ref: '#/components/parameters/wallet-address'


sabineschaller

LGTM

njlie requested review from BlairCurrey, mkurapov and sabineschaller September 26, 2023 22:18

njlie marked this pull request as draft September 26, 2023 22:18

njlie marked this pull request as ready for review September 27, 2023 15:11

mkurapov reviewed Sep 27, 2023

View reviewed changes

njlie force-pushed the nl-add-wallet-address-to-requests branch from e026da9 to 64b56e5 Compare October 2, 2023 17:38

mkurapov reviewed Oct 2, 2023

View reviewed changes

njlie force-pushed the nl-add-wallet-address-to-requests branch 3 times, most recently from d5fc764 to 34c6a68 Compare October 5, 2023 20:40

njlie requested a review from mkurapov October 6, 2023 16:38

mkurapov approved these changes Oct 10, 2023

View reviewed changes

njlie added 7 commits October 10, 2023 10:16

feat: add wallet address to subresource requests

511814e

feat: add examples, add walletAddress to /incoming-payments/complete

75c35f0

fix: remove wallet-address from /complete query params, add descripti…

7b24784

…on to component

feat: add wallet-address to list endpoints

dea540e

feat: add changeset

250d95d

feat: make wallet addresses required in requests; update open payment…

bdb2f56

…s client

fix: openapi package tests

e860b72

njlie force-pushed the nl-add-wallet-address-to-requests branch from 34c6a68 to e860b72 Compare October 10, 2023 17:19

sabineschaller reviewed Oct 11, 2023

View reviewed changes

njlie force-pushed the nl-add-wallet-address-to-requests branch from 0f4d8af to 8b42dfc Compare October 11, 2023 20:45

feat: remove query param from paths already with unique id

0f67aaf

njlie force-pushed the nl-add-wallet-address-to-requests branch from 8b42dfc to 0f67aaf Compare October 11, 2023 21:05

njlie requested a review from sabineschaller October 12, 2023 16:52

sabineschaller approved these changes Oct 16, 2023

View reviewed changes

njlie merged commit 5809202 into main Oct 16, 2023
10 checks passed

njlie deleted the nl-add-wallet-address-to-requests branch October 16, 2023 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add wallet address to subresource requests #305

feat: add wallet address to subresource requests #305

njlie commented Sep 26, 2023

changeset-bot bot commented Sep 26, 2023 •

edited

Loading

netlify bot commented Sep 26, 2023 •

edited

Loading

njlie commented Sep 26, 2023

mkurapov left a comment

njlie commented Sep 28, 2023

mkurapov left a comment •

edited

Loading

mkurapov Oct 2, 2023

mkurapov Oct 2, 2023

njlie commented Oct 2, 2023

mkurapov left a comment

mkurapov Oct 10, 2023 •

edited

Loading

sabineschaller Oct 10, 2023 •

edited

Loading

sabineschaller Oct 11, 2023

mkurapov commented Oct 10, 2023

njlie commented Oct 10, 2023

sabineschaller left a comment

sabineschaller Oct 11, 2023

njlie Oct 11, 2023 •

edited

Loading

sabineschaller Oct 16, 2023

sabineschaller Oct 11, 2023

sabineschaller left a comment

feat: add wallet address to subresource requests #305

feat: add wallet address to subresource requests #305

Conversation

njlie commented Sep 26, 2023

Changes proposed in this pull request

Context

changeset-bot bot commented Sep 26, 2023 • edited Loading

🦋 Changeset detected

netlify bot commented Sep 26, 2023 • edited Loading

✅ Deploy Preview for openpayments-preview ready!

njlie commented Sep 26, 2023

mkurapov left a comment

Choose a reason for hiding this comment

njlie commented Sep 28, 2023

mkurapov left a comment • edited Loading

Choose a reason for hiding this comment

mkurapov Oct 2, 2023

Choose a reason for hiding this comment

mkurapov Oct 2, 2023

Choose a reason for hiding this comment

njlie commented Oct 2, 2023

mkurapov left a comment

Choose a reason for hiding this comment

mkurapov Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

sabineschaller Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

sabineschaller Oct 11, 2023

Choose a reason for hiding this comment

mkurapov commented Oct 10, 2023

njlie commented Oct 10, 2023

sabineschaller left a comment

Choose a reason for hiding this comment

sabineschaller Oct 11, 2023

Choose a reason for hiding this comment

njlie Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

sabineschaller Oct 16, 2023

Choose a reason for hiding this comment

sabineschaller Oct 11, 2023

Choose a reason for hiding this comment

sabineschaller left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Sep 26, 2023 •

edited

Loading

netlify bot commented Sep 26, 2023 •

edited

Loading

mkurapov left a comment •

edited

Loading

mkurapov Oct 10, 2023 •

edited

Loading

sabineschaller Oct 10, 2023 •

edited

Loading

njlie Oct 11, 2023 •

edited

Loading