feat: add spent amounts to POST /outgoing-payment #2753

BlairCurrey · 2024-05-29T17:22:07Z

Changes proposed in this pull request

adds grantSpentDebitAmount and grantSpentReceiveAmount to outgoing payment create response
adds "Web Monetization" example to Bruno collection

Context

Checklist

netlify · 2024-05-29T17:22:21Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`d43dfb0`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/667240100a841a0008de9375

BlairCurrey · 2024-05-29T19:30:34Z

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

+            expect(payment.grantSpentReceiveAmount.value).toBe(
+              // Must account for interledger/pay off-by-one issue (even with 0 slippage/fees)
+              BigInt((debitAmount.value - BigInt(1)) * BigInt(i))
+            )


didn't see an issue for this quirk identified in the comment (not sure where we left it) but figured id note why receive amount should be 1 less than the debit amount here

It's a feature, not a bug 😆

BlairCurrey · 2024-06-04T02:09:25Z

bruno/collections/Rafiki/Examples/Web Monetization/Grant Request Outgoing Payment.bru

I guess with the consent interaction after this step it's not exactly what the web monetization flow will look like. Otherwise I think it's similar? Not sure if there is a better name for this example but I felt like there were enough changes compared to the Open Payments flow to warrant including this new set of requests.

workaround for error: "TypeError: A dynamic import callback was invoked without --experimental-vm-modules" introduced by new op version

mkurapov · 2024-06-12T10:59:57Z

test/integration/package.json

@@ -16,7 +16,7 @@
  "devDependencies": {
    "@apollo/client": "^3.10.4",
    "@interledger/http-signature-utils": "2.0.2",
-    "@interledger/open-payments": "^6.11.1",
+    "@interledger/open-payments": "6.11.1",


pinning it just in case?

As I recall I did this in the middle of fixing the open payments issue we saw related to the ESM import. I noticed the others were pinned as well so I changed this one but at this point it may not matter.

I see, they were always pinned. I guess it's ok.

mkurapov · 2024-06-13T11:08:54Z

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

+      assetCode: payment.asset.code,
+      assetScale: payment.asset.scale,


I think the received amount should be the asset of the receiver itself, ie. it can be a different asset than the payment. In this case, we should use grant.limits to get this asset information instead.

I think debitAmount should always be the outgoing payment asset, however.

updated: fc4adba

I used the paymentLimits (comes from grant.limits) and added some typeguards. Otherwise its a bit tedious to check which of the amounts are defined.

mkurapov · 2024-06-13T11:09:47Z

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

@@ -170,6 +170,7 @@ export async function resolveReceiver(
 ): Promise<Receiver> {
  const receiver = await deps.receiverService.get(options.receiver)
  if (!receiver) {
+    deps.logger.info({ receiver: options.receiver }, 'Receiver not found')


Maybe we can add something like "could not create quote" in the log message

mkurapov · 2024-06-13T11:13:32Z

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

+            assert.ok(!isOutgoingPaymentError(payment))
+            expect(payment.grantSpentDebitAmount.value).toBe(
+              BigInt(debitAmount.value * BigInt(i))
+            )
+            expect(payment.grantSpentReceiveAmount.value).toBe(
+              // Must account for interledger/pay off-by-one issue (even with 0 slippage/fees)
+              BigInt((debitAmount.value - BigInt(1)) * BigInt(i))
+            )
+          }


Maybe it makes sense to split the test for grantSpentDebitAmount and grantSpentReceiveAmount, since we want to allow a grant to have either or only, not both:

Update limits of outgoing payment grant creation open-payments#471

yeah that does make sense. split

mkurapov · 2024-06-13T11:14:26Z

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

+      assetCode: this.asset.code,
+      assetScale: this.asset.scale


same comment as in the service file. The asset code may be different than the payment itself.

I guess this means we dont have any sort of default for the grantSpentReceiveAmount since this depends on being created with a grant limit. So we simply cant return grantSpentReceiveAmount for outgoing payments created in this case.

Maybe that's fine... it just seems incongruent with the grantSpentDebitAmount, which we can return a default value for (0 with outgoing payment's assetCode and assetScale). I guess maybe that's OK, but it seems non-obvious from the caller's perspective.

I guess we could:

just leave as-is

not return any defualt/null version of grantSpentDebitAmount either

return something like {value: 0, assetCode: "", assetScale: -1 } for grantSpentReceiveAmount as a default (and maybe grantSpentDebitAmount as well)

or maybe we can get the asset from somewhere other than grant.limits so that we can form it without grant limits...

It's kinda edge-casey so maybe we shouldn't worry about it too much. Im just trying to envision what the most intuitive way to handle it would be. Is it going to be confusing when someone creates an outgoing payment for the first time and sees grantSpentDebitAmount of {0, asset: 'XYZ', scale: '2' } and no grantSpentReceiveAmount?

What if instead of storing grantSpentReceiveAmountValue: bigint and grantSpentDebitAmountValue: bigint, we just allow setting the whole Amount? Otherwise if not set, it just gets returned as undefined?

I've changed it set the whole amount locally already so in full agreement there.

After sitting on this for a bit, I think undefined and then not returning them in the /POST /outgoing-payment seems fine. I guess my default expectation is to not include/exclude properties when they're in a sort of null state but in this case I think it may be the most straightforward.

updated to above ^ 710897d

mkurapov · 2024-06-13T11:21:25Z

bruno/collections/Rafiki/Examples/Web Monetization/Continuation Request.bru

@@ -0,0 +1,33 @@
+meta {


I'm trying to think whether we need a separate folder for this. Technically, we can get a large grant, and then keep re-running the same "Create Outgoing Payment" request. Maybe it's good to have, however, just thinking out loud

I'm trying to think whether we need a separate folder for this

In other words, maybe not adding this new "Web Monetization" example?

I mentioned my reasoning a bit in this comment (while questioning the name) #2753 (comment). Basically I didn't find it trivial to alter the existing Open Payments one to do this. It took me some time to put together and I dont even remember all the changes off the top of my head (I would not want to have to re-figure it out). Feel like it could save myself and other some time.

mkurapov · 2024-06-18T12:42:13Z

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

+  if (hasDebitAmount(paymentLimits)) {
+    paymentLimitAmount = paymentLimits.debitAmount
+  } else if (hasReceiveAmount(paymentLimits)) {
+    paymentLimitAmount = paymentLimits.receiveAmount
+  } else {
+    return { isValid: true }
  }


I think we can only do this once

Update limits of outgoing payment grant creation open-payments#471

is merged? Since now, we can have both

I renamed paymentLimit to amountForReceivedAmount but the logic should be the same as it was before. I think the paymentLimit was misleading - it was only being used to get the asset code/scale (per your request to get it from grant limits). I also removed the typeguards (had bigger plans for them originally, but doing nothing compared to paymentLimit.debitAmount etc. checks now).

Actually, looking at it, we can actually do something easier: I think we can use payment.receiveAmount -> that will have the asset code & scale of the receiver (since it uses the quote.receiveAmount) under the hood for it

mkurapov · 2024-06-18T12:42:43Z

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

+        debitAmount: options.debitAmount,
+        incomingAmount: receiver.incomingAmount
+      },
+      'debitAmount or incomingAmount required'


Same thing with "could not create quote"

mkurapov · 2024-06-18T12:43:44Z

test/integration/package.json

@@ -16,7 +16,7 @@
  "devDependencies": {
    "@apollo/client": "^3.10.4",
    "@interledger/http-signature-utils": "2.0.2",
-    "@interledger/open-payments": "^6.11.1",
+    "@interledger/open-payments": "6.11.1",


I see, they were always pinned. I guess it's ok.

mkurapov · 2024-06-18T12:46:51Z

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

+            expect(payment.grantSpentReceiveAmount.value).toBe(
+              // Must account for interledger/pay off-by-one issue (even with 0 slippage/fees)
+              BigInt((debitAmount.value - BigInt(1)) * BigInt(i))
+            )


It's a feature, not a bug 😆

BlairCurrey added 4 commits May 29, 2024 10:46

feat(backend): add logs

c7d79f5

feat(bruno): add webmo example

12df39a

feat(backend): return spent amounts on outgoing payment create

714deff

chore: rm log

4a23af8

github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels May 29, 2024

BlairCurrey changed the title ~~Bc/2678/add spent amounts~~ feat: add spent amounts to POST /outgoing-payment May 29, 2024

test(backend): many outgoing payments against one grant

9a125f3

github-actions bot added the type: tests Testing related label May 29, 2024

chore(backend): lint

014cf8c

BlairCurrey commented May 29, 2024

View reviewed changes

fix(bruno): rm unecessary quote requests

d563556

BlairCurrey commented Jun 4, 2024

View reviewed changes

fix: use latest op version

7659f60

github-actions bot added the pkg: auth Changes in the GNAP auth package. label Jun 4, 2024

BlairCurrey added 4 commits June 4, 2024 11:38

chore(integration): add experimental flag

8cca985

workaround for error: "TypeError: A dynamic import callback was invoked without --experimental-vm-modules" introduced by new op version

chore: add experimental flag

de01fa7

Merge branch 'main' into bc/2678/add-spent-amounts

82d93d6

Merge branch 'main' into bc/2678/add-spent-amounts

bca8620

github-actions bot removed the pkg: auth Changes in the GNAP auth package. label Jun 6, 2024

fix(backend): route test

92f3288

BlairCurrey marked this pull request as ready for review June 6, 2024 02:05

BlairCurrey requested review from mkurapov, sabineschaller, koekiebox and njlie June 6, 2024 02:05

mkurapov linked an issue Jun 11, 2024 that may be closed by this pull request

Return amount spent on the outgoing payment grant during outgoing payment creation #2678

Closed

2 tasks

BlairCurrey removed request for koekiebox and mkurapov June 11, 2024 14:43

njlie previously approved these changes Jun 11, 2024

View reviewed changes

mkurapov reviewed Jun 13, 2024

View reviewed changes

BlairCurrey added 2 commits June 13, 2024 12:23

chore(backend): improve log

d7d53cf

refactor(backend): split tests

7548f1f

BlairCurrey dismissed njlie’s stale review via 7548f1f June 13, 2024 16:58

raducristianpopa mentioned this pull request Jun 14, 2024

fix(deps): update dependency @interledger/open-payments to ^6.11.1 interledger/web-monetization-extension#327

Merged

1 task

BlairCurrey added 2 commits June 14, 2024 09:33

Merge branch 'main' into bc/2678/add-spent-amounts

b85fe90

fix(backend): use correct asset

fc4adba

mkurapov reviewed Jun 18, 2024

View reviewed changes

BlairCurrey added 4 commits June 18, 2024 10:15

fix: rename var, rm typegurads

ccc2dc2

Merge branch 'main' into bc/2678/add-spent-amounts

4dbb2d7

chore: update quote log

8b4938d

fix: spent amount null state

710897d

BlairCurrey requested a review from mkurapov June 18, 2024 15:37

BlairCurrey added 5 commits June 18, 2024 11:53

fix: test

a6d7192

fix: test

0322afd

fix(backend): use receiveAmount asset for grantSpentReceiveAmount

8286edc

fix(backend): lint

6870848

Merge branch 'main' into bc/2678/add-spent-amounts

d43dfb0

mkurapov approved these changes Jun 20, 2024

View reviewed changes

mkurapov merged commit 2b6f171 into main Jun 20, 2024
42 checks passed

mkurapov deleted the bc/2678/add-spent-amounts branch June 20, 2024 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add spent amounts to POST /outgoing-payment #2753

feat: add spent amounts to POST /outgoing-payment #2753

BlairCurrey commented May 29, 2024 •

edited

Loading

netlify bot commented May 29, 2024 •

edited

Loading

BlairCurrey May 29, 2024

mkurapov Jun 18, 2024

BlairCurrey Jun 4, 2024 •

edited

Loading

mkurapov Jun 12, 2024

BlairCurrey Jun 13, 2024

mkurapov Jun 18, 2024

mkurapov Jun 13, 2024

BlairCurrey Jun 14, 2024

mkurapov Jun 13, 2024

mkurapov Jun 13, 2024

BlairCurrey Jun 14, 2024

mkurapov Jun 13, 2024

BlairCurrey Jun 14, 2024 •

edited

Loading

BlairCurrey Jun 14, 2024 •

edited

Loading

mkurapov Jun 18, 2024

BlairCurrey Jun 18, 2024

BlairCurrey Jun 18, 2024

mkurapov Jun 13, 2024

BlairCurrey Jun 13, 2024

mkurapov Jun 18, 2024

BlairCurrey Jun 18, 2024

BlairCurrey Jun 18, 2024

mkurapov Jun 18, 2024

mkurapov Jun 18, 2024

mkurapov Jun 18, 2024

mkurapov Jun 18, 2024

		assetCode: payment.asset.code,
		assetScale: payment.asset.scale,

feat: add spent amounts to POST /outgoing-payment #2753

feat: add spent amounts to POST /outgoing-payment #2753

Conversation

BlairCurrey commented May 29, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented May 29, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Jun 4, 2024 • 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

BlairCurrey Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey Jun 14, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented May 29, 2024 •

edited

Loading

netlify bot commented May 29, 2024 •

edited

Loading

BlairCurrey Jun 4, 2024 •

edited

Loading

BlairCurrey Jun 14, 2024 •

edited

Loading

BlairCurrey Jun 14, 2024 •

edited

Loading