feat: calculate grant spent amounts from new table #3412

BlairCurrey · 2025-04-28T18:27:37Z

Changes proposed in this pull request

changes create method on outgoing payment service to use new grant spent amount tables to get the spent amounts. Falls back to existing method of summing all payments for grants that pre-exist this change.

Context

fixes: #3373

Checklist

Related issues linked using fixes #number
Tests added/updated
Make sure that all checks pass
Bruno collection updated (if necessary)
Documentation issue created with user-docs label (if necessary)
OpenAPI specs updated (if necessary)

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

BlairCurrey · 2025-04-28T18:39:01Z

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

+          let existingOutgoingPaymentGrant: OutgoingPaymentGrant | undefined
+
          if (grantId) {
-            const stopTimerGrant = deps.telemetry.startTimer(
-              'outgoing_payment_service_insertgrant_time_ms',
-              {
-                callName: 'OutgoingPaymentGrantModel:insert',
-                description: 'Time to insert grant in outgoing payment'
-              }
-            )
-            await OutgoingPaymentGrant.query(trx)
-              .insert({
-                id: grantId
-              })
-              .onConflict('id')
-              .ignore()
-            stopTimerGrant()
+            const existingGrant =
+              await OutgoingPaymentGrant.query(trx).findById(grantId)
+
+            if (!existingGrant) {
+              // TODO: insert w/ interval
+              await OutgoingPaymentGrant.query(trx)
+                .insert({ id: grantId })
+                .onConflict('id')
+                .ignore()
+            }


We need to insert this grant record before creating the payment (grantId no payment is a fk of this table) but we also need to know if a record already existed (to check if its a legacy grant). Hence the existing grant query.

I thought maybe this meant we could not do the onConflict/ignore but its still necessary (race condition observed in one of the unit tests).

github-actions · 2025-04-28T19:00:52Z

🚀 Performance Test Results

Test Configuration:

VUs: 4
Duration: 1m0s

Test Metrics:

Requests/s: 44.79
Iterations/s: 14.95
Failed Requests: 0.00% (0 of 2697)

📜 Logs


> [email protected] run-tests:testenv /home/runner/work/rafiki/rafiki/test/performance
> ./scripts/run-tests.sh -e test "-k" "-q" "--vus" "4" "--duration" "1m"

Cloud Nine GraphQL API is up: http://localhost:3101/graphql
Cloud Nine Wallet Address is up: http://localhost:3100/
Happy Life Bank Address is up: http://localhost:4100/
cloud-nine-wallet-test-backend already set
cloud-nine-wallet-test-auth already set
happy-life-bank-test-backend already set
happy-life-bank-test-auth already set
     data_received..................: 940 kB 16 kB/s
     data_sent......................: 1.9 MB 32 kB/s
     http_req_blocked...............: avg=6.21µs   min=1.81µs   med=5.21µs   max=410.01µs p(90)=6.22µs   p(95)=6.72µs  
     http_req_connecting............: avg=239ns    min=0s       med=0s       max=149.93µs p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=88.65ms  min=8.33ms   med=74.25ms  max=545.33ms p(90)=152.69ms p(95)=173.65ms
       { expected_response:true }...: avg=88.65ms  min=8.33ms   med=74.25ms  max=545.33ms p(90)=152.69ms p(95)=173.65ms
     http_req_failed................: 0.00%  ✓ 0         ✗ 2697
     http_req_receiving.............: avg=86.95µs  min=28.81µs  med=77.66µs  max=1.46ms   p(90)=110.96µs p(95)=143.51µs
     http_req_sending...............: avg=33.55µs  min=8.1µs    med=27.46µs  max=2.41ms   p(90)=40.25µs  p(95)=54.03µs 
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=88.53ms  min=8.17ms   med=74.14ms  max=545.22ms p(90)=152.6ms  p(95)=173.41ms
     http_reqs......................: 2697   44.791891/s
     iteration_duration.............: avg=267.25ms min=175.13ms med=254.59ms max=1.07s    p(90)=326.19ms p(95)=359.9ms 
     iterations.....................: 900    14.947238/s
     vus............................: 4      min=4       max=4 
     vus_max........................: 4      min=4       max=4

mkurapov · 2025-04-29T13:48:24Z

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

+    intervalReceiveAmountValue: paymentLimits.interval
+      ? payment.receiveAmount.value
+      : null,


We also need to add the newSpentAmounts to the cumulative interval value (if we are still within the same interval, that is). This will require changing the starting point for newSpentAmounts

…mount

BlairCurrey · 2025-05-15T19:06:53Z

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

+            expect(spentAmounts).toEqual(
+              expect.objectContaining({
+                grantId: grant.id,
+                outgoingPaymentId: payment.id,
+                debitAmountCode: debitAmount.assetCode,
+                debitAmountScale: debitAmount.assetScale,
+                paymentDebitAmountValue: debitAmount.value,
+                grantTotalDebitAmountValue: debitAmount.value * BigInt(i + 1),
+                receiveAmountCode: debitAmount.assetCode,
+                receiveAmountScale: debitAmount.assetScale,
+                paymentReceiveAmountValue: adjustedReceiveAmountValue,
+                grantTotalReceiveAmountValue:
+                  adjustedReceiveAmountValue * BigInt(i + 1),
+                intervalDebitAmountValue: null,
+                intervalReceiveAmountValue: null,
+                intervalStart: null,
+                intervalEnd: null,
+                paymentState: 'FUNDING'
+              })


the grant spent amounts are kind of an implementation detail. at the end of the day its grantSpent[Receive|Debit]Amounts which matter, which is already tested. IDK, left for now but considered not including this.

mkurapov · 2025-06-02T11:49:42Z

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

+  if (createdTime < start) {
+    return 'next'
+  } else if (createdTime >= end) {
+    return 'previous'


Double checking this: given an interval [start, end]:
if createdAt < start: would this be in the previous interval? (createdAt equal to start would put us in the current interval)
Conversely, if createdTime > end, we would be in the next interval, right? (maybe if createdAt is equal to end, we count it as being part of the current interval)

I will revisit this. The refactor of this function in general from returning a boolean to the status. I was thinking I'd need more resolution based off the general logic we discussed but Im not sure I ended up using it.

I was thinking I'd need more resolution based off the general logic we discussed but Im not sure I ended up using it.

I think it will be useful (you will need the three cases) for calculating the spent amounts on outgoing payment failure (RAF-1036), so its good to have. We just need to flip the conditions.

mkurapov · 2025-06-02T12:57:41Z

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

+  [OutgoingPaymentError.InvalidGrantSpentAmountState]:
+    'invalid grant spent amounts'


I think we should differentiate between errors served up to an Open Payments client vs those that are just on the Admin API (since this error would not be necessarily useful to an OP client)

OK I think I've sidestepped this by making it more typesafe here: 934e9f6 Yay, make the invalid state unrepresentable.

Maybe not important anymore, but I'm not exactly following - how would open payments client not care about this but admin api would (given it could happen in either open payments api/admin api)?

It was a bad state (500/internal server error) so it's true that there is nothing the client could really do to recover. But not sure what the difference between the two APIs would be. Again, it's a moot point because it no longer throws the error.

OK I think I've sidestepped this by making it more typesafe here: 934e9f6 Yay, make the invalid state unrepresentable.
Getting rid of the error entirely works great :)

My original thinking was to separate errorToMessage, and have an errorToGraphQLMessage vs errorToOpenPaymentsMessage (something like this) such that we dont show "invalid grant spent amounts" to the OP client, and maybe something more generic instead.
As an aside, we would never end up throwing the InvalidGrantSpentAmountState error in the Admin API since there is no grant associated when its a first-party, Admin API payment.

…ernal server error

feat: calculate grant spent amounts from new table

9dbcfbe

github-actions bot added pkg: backend Changes in the backend package. type: source Changes business logic labels Apr 28, 2025