feat: either receiveAmount or debitAmount allowed for outgoing payment limits #3449

oana-lolea · 2025-06-05T11:56:19Z

Changes proposed in this pull request

Updated OutgoingPaymentLimit to no longer accept both receiveAmount and debitAmount
Added error when both types of amount are provided
Updated bruno request

Context

Fixes #3337

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)

github-actions · 2025-06-05T12:01:09Z

🚀 Performance Test Results

Test Configuration:

VUs: 4
Duration: 1m0s

Test Metrics:

Requests/s: 42.30
Iterations/s: 14.11
Failed Requests: 0.00% (0 of 2543)

📜 Logs


> performance@1.0.0 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..................: 886 kB 15 kB/s
     data_sent......................: 1.8 MB 30 kB/s
     http_req_blocked...............: avg=9.56µs   min=2.03µs   med=5.75µs   max=2.38ms   p(90)=7.04µs   p(95)=7.78µs  
     http_req_connecting............: avg=2.81µs   min=0s       med=0s       max=2.3ms    p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=93.89ms  min=9.54ms   med=79.59ms  max=569.92ms p(90)=160.24ms p(95)=182.46ms
       { expected_response:true }...: avg=93.89ms  min=9.54ms   med=79.59ms  max=569.92ms p(90)=160.24ms p(95)=182.46ms
     http_req_failed................: 0.00%  ✓ 0         ✗ 2543
     http_req_receiving.............: avg=95.96µs  min=25.53µs  med=82.68µs  max=2.48ms   p(90)=121.36µs p(95)=161.83µs
     http_req_sending...............: avg=37.83µs  min=10.47µs  med=29.61µs  max=2.03ms   p(90)=43.86µs  p(95)=59.28µs 
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=93.75ms  min=9.15ms   med=79.44ms  max=569.82ms p(90)=160.09ms p(95)=182.35ms
     http_reqs......................: 2543   42.303645/s
     iteration_duration.............: avg=283.25ms min=183.82ms med=267.53ms max=1.12s    p(90)=349.23ms p(95)=379.92ms
     iterations.....................: 848    14.10676/s
     vus............................: 4      min=4       max=4 
     vus_max........................: 4      min=4       max=4

mkurapov · 2025-06-05T13:04:55Z

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

+  if (limits.debitAmount && limits.receiveAmount) {
+    throw OutgoingPaymentError.OnlyOneAmountAllowed
+  }


It is good to check this at the resource server, but also we need to check this at the auth server as well, since that is the place where the grant with the access is actually being created.

In other words, we should prevent the grant being created in the first place with the incorrect access amounts.

mkurapov · 2025-06-05T13:05:02Z

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

  limits: Limits
 ): boolean {
+  if (limits.debitAmount && limits.receiveAmount) {
+    throw OutgoingPaymentError.OnlyOneAmountAllowed


Suggested change

throw OutgoingPaymentError.OnlyOneAmountAllowed

throw OutgoingPaymentError.OnlyOneGrantAmountAllowed

…or both types of amount

mkurapov · 2025-06-10T11:06:36Z

packages/auth/src/access/service.ts

+function validateLimits(accessRequests: AccessRequest[]) {
+  const areBothLimitsSet =
+    accessRequests.findIndex(
+      (access) => access.limits?.debitAmount && access.limits.receiveAmount
+    ) !== -1
+
+  if (areBothLimitsSet) {
+    throw new Error('Only one of receiveAmount or debitAmount allowed')
+  }
+}


Following the stack trace for the error, we would end up sending a 500 - Internal server error back to the client.
Instead, we should reply with a 400 invalid_request.

mkurapov · 2025-06-10T12:00:07Z

After going through the Bruno Examples > Open Payments requests, I end up with some failures in the mock ASE.

when I don't provide receiveAmount and just debitAmount:

and then without debitAmount, just receiveAmount:

no amounts (which should be possible still, just not recommended 😅):

mkurapov · 2025-06-12T12:19:54Z

packages/auth/src/grant/routes.ts

+    if (typeof err === 'string') {
+      throw new GNAPServerRouteError(400, GNAPErrorCode.InvalidRequest, err)
+    }


I think it would be useful to map errors from the services to the HTTP status & GNAPErrorCodes, similar to how we are doing in the backend's incoming payments for example.

packages/auth/src/access/errors.ts

mkurapov · 2025-06-16T11:00:32Z

packages/auth/src/grant/routes.ts


  const trx = await Grant.startTransaction()

+  const grant = await createGrantOrThrowError(grantService, body, trx)


To simplify, we could probably:

Only return Promise<Grant> in the grantService.create (without GrantError) and just let the thrown AccessError propagate to grantRoutes.createPendingGrant

In the createPendingGrant catch block down below, check for isAccessError(err)

With this, you'll only end up adding an extra check in the createPendingGrant catch block or the new error without needing to rollback the trx separately

mkurapov

Looks good - a final few comments for auth & backend.
Otherwise the only thing left is making sure the MASE frontend is updated to handle the different amount cases

mkurapov · 2025-06-16T15:36:47Z

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

  [OutgoingPaymentError.NegativeReceiveAmount]: 400,
-  [OutgoingPaymentError.InvalidReceiver]: 400
+  [OutgoingPaymentError.InvalidReceiver]: 400,
+  [OutgoingPaymentError.OnlyOneGrantAmountAllowed]: 400


Suggested change

[OutgoingPaymentError.OnlyOneGrantAmountAllowed]: 400

[OutgoingPaymentError.OnlyOneGrantAmountAllowed]: 500

Since from the perspective of the client, the input they passed to create outgoing payment could have been OK, it was the auth server handling of the grant request that was incorrect (and a server-side error)

mkurapov · 2025-06-16T15:40:31Z

packages/auth/src/grant/service.test.ts

        }

        const grant1 = await grantService.create(grantRequest)
+        assert.ok(!isAccessError(grant1))


Looks like we don't need this now, since we only can return Grant

… debit amount

mkurapov

Getting this screen after accepting a grant with a defined receiveAmount

mkurapov · 2025-06-23T15:33:35Z

localenv/mock-account-servicing-entity/app/routes/mock-idp.consent.tsx

          <p>
-            You gave {thirdPartyName} permission to send {currencyDisplayCode}{' '}
-            {amount.toFixed(2)} out of your account.
+            You gave {thirdPartyName} permission to send{' '}


Suggested change

You gave {thirdPartyName} permission to send{' '}

You gave {thirdPartyName} permission to send {' '}

…nses from OP (#3429) * Removed updatedAt from incoming and outgoing payments responses from OP * Removed 'updatedAt' from test-lib 'createReceiver', updated OP version for auth, test-lib and integration packages * Updated pnpm-lock * Updated bruno requests * Removed 'updatedAt' from webhook.yaml * feat: either receiveAmount or debitAmount allowed for outgoing payment limits (#3449) * Set limits to have either receive or debit amount * Updated auth limits and bruno request * Added the case when both types of amount are missing * Updated mock ASE consent screen, renamed error, added check in auth for both types of amount * Added error enum for access, added tests and updated types * Added error mapping for grants, updated tests * Format fixes and ignored eslint error * Updated createGrant function for testing * Forgot to rollback transaction when error will be thrown * Simplified access error handling * Removed unnecessary assert and changed error code * Fixed mock consent screen so it will support having either receive or debit amount * Updated consent messages * Updated mock ASE consent screen to handle all limit amount cases * Updated token-introspection OP version to 7.0.0, and removed updatedAt from outgoing payments webhooks schema

oana-lolea added 4 commits June 3, 2025 17:44

Set limits to have either receive or debit amount

edcbd67

Merge branch 'oana/raf-1044' into oana/raf-994

c44c1cb

Updated auth limits and bruno request

e30bb4b

Added the case when both types of amount are missing

feee7c0

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic pkg: auth Changes in the GNAP auth package. labels Jun 5, 2025

oana-lolea requested a review from mkurapov June 5, 2025 12:42

oana-lolea marked this pull request as ready for review June 5, 2025 12:42

mkurapov reviewed Jun 5, 2025

View reviewed changes

Updated mock ASE consent screen, renamed error, added check in auth f…

4728d0c

…or both types of amount

github-actions bot added the pkg: mock-ase label Jun 10, 2025

mkurapov reviewed Jun 10, 2025

View reviewed changes

Added error enum for access, added tests and updated types

bf17ea2

mkurapov reviewed Jun 12, 2025

View reviewed changes

oana-lolea added 4 commits June 12, 2025 17:18

Added error mapping for grants, updated tests

da4cd01

Format fixes and ignored eslint error

0c4ff6b

Updated createGrant function for testing

29e7f9f

Forgot to rollback transaction when error will be thrown

6033cbc

mkurapov reviewed Jun 16, 2025

View reviewed changes

Simplified access error handling

46f06ac

mkurapov reviewed Jun 16, 2025

View reviewed changes

oana-lolea added 3 commits June 16, 2025 19:47

Removed unnecessary assert and changed error code

0401229

Fixed mock consent screen so it will support having either receive or…

e8dfb26

… debit amount

Updated consent messages

899654c

oana-lolea requested a review from mkurapov June 23, 2025 15:27

mkurapov mentioned this pull request Jun 23, 2025

feat(auth): add subject to grant #3440

Merged

6 tasks

mkurapov reviewed Jun 23, 2025

View reviewed changes

Updated mock ASE consent screen to handle all limit amount cases

f8709da

oana-lolea requested a review from mkurapov June 26, 2025 14:25

mkurapov approved these changes Jul 2, 2025

View reviewed changes

oana-lolea merged commit 1e2c020 into oana/raf-1044 Jul 2, 2025
38 checks passed

oana-lolea deleted the oana/raf-994 branch July 2, 2025 08:13

oana-lolea mentioned this pull request Jul 2, 2025

chore!: removed 'updatedAt' from incoming and outgoing payments responses from OP #3429

Merged

6 tasks

	throw OutgoingPaymentError.OnlyOneAmountAllowed
	throw OutgoingPaymentError.OnlyOneGrantAmountAllowed


		const trx = await Grant.startTransaction()

		const grant = await createGrantOrThrowError(grantService, body, trx)

	[OutgoingPaymentError.OnlyOneGrantAmountAllowed]: 400
	[OutgoingPaymentError.OnlyOneGrantAmountAllowed]: 500

	You gave {thirdPartyName} permission to send{' '}
	You gave {thirdPartyName} permission to send {' '}

feat: either receiveAmount or debitAmount allowed for outgoing payment limits #3449

feat: either receiveAmount or debitAmount allowed for outgoing payment limits #3449

Uh oh!

Conversation

oana-lolea commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request

Context

Checklist

Uh oh!

github-actions bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Performance Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurapov commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurapov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurapov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oana-lolea commented Jun 5, 2025 •

edited

Loading

github-actions bot commented Jun 5, 2025 •

edited

Loading