Skip to content

feat: ask for payment pointer from ASE #1501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jun 23, 2023
Merged

Conversation

mkurapov
Copy link
Contributor

@mkurapov mkurapov commented Jun 8, 2023

Changes proposed in this pull request

  • Add payment_pointer.not_found webhook event
    • Adds support for webhook in mock account servicing entity
  • Add polling utility functions

Context

Fixes #1419

When a payment pointer is requested but not found in Rafiki, we will now will publish a payment_pointer.not_found webhook event, and start polling for the ASE to create a payment pointer (via createPaymentPointer mutation). The polling timeout (and frequency) is configurable. This enables creation of payment pointers on the fly: giving a chance for the ASE to resolve any requested payment pointer that they may have in their system.

This can be demonstrated via a new seed user, Alice Smith, who doesn't have a payment pointer to start with:

Screenshot 2023-06-09 at 00 06 48

but then upon GET https://happy-life-bank-backend/accounts/asmith one is created asynchronously:

Screenshot 2023-06-09 at 00 11 48

This new payment pointer can be used via the {{asmithPaymentPointer}} Postman collection variable.

Considerations

Ideally, requests need to be rate-limited (per IP?): for a payment pointer that the ASE cannot resolve (the account doesn't exist), it's a bit of a heavy operation every GET request & can take up some resources.

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass

@netlify
Copy link

netlify bot commented Jun 8, 2023

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit a58ff7f
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/649597bc576434000816f152

@github-actions github-actions bot added pkg: backend Changes in the backend package. pkg: documentation Changes in the documentation package. type: documentation type: source Changes business logic type: tests Testing related labels Jun 8, 2023
@@ -0,0 +1,21 @@
import { IAppConfig } from '../config/app'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied from auth package

'returns undefined if no payment pointer exists with url',
withConfigOverride(
() => config,
{ paymentPointerLookupTimeoutMs: 1 },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

purely so the tests run faster, and don't need to wait the 1.5s for the timeout


response = await requestWithTimeout(
() => request(),
timeoutMs - elapsedTimeMs
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeoutMs - elapsedTimeMs essentially lets timeoutMs to be the maximum timeout for the polling period. i.e. we can break early without needing for the given request to complete before timing out

Comment on lines +176 to +178
data: {
paymentPointerUrl: url
}
Copy link
Contributor Author

@mkurapov mkurapov Jun 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also just return the payment pointer path here, i.e. just gfranklin instead of the full https://rafiki.money/gfranklin, but I think this keeps it consistent, we just let the ASE to the stripping of the string as they see fit

@@ -21,6 +21,12 @@ accounts:
initialBalance: 2000
path: accounts/planex
postmanEnvVar: planexPaymentPointer
- name: 'Alice Smith'
path: accounts/asmith
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe alias is better than path but just went with what was there originally

@mkurapov mkurapov marked this pull request as ready for review June 8, 2023 22:17
BlairCurrey
BlairCurrey previously approved these changes Jun 21, 2023
Copy link
Contributor

@BlairCurrey BlairCurrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and it works as described.

Ideally, requests need to be rate-limited (per IP?): for a payment pointer that the ASE cannot resolve (the account doesn't exist), it's a bit of a heavy operation every GET request & can take up some resources.

That does seem like a concern. We should probably create an issue to address this vulnerability (whether by rate limiting or other means) so it doesn't get lost

# Conflicts:
#	packages/backend/src/config/app.ts
#	packages/backend/src/index.ts
#	packages/backend/src/open_payments/payment_pointer/service.test.ts
#	packages/backend/src/open_payments/payment_pointer/service.ts
#	packages/documentation/docs/integration/deployment.md
Comment on lines -64 to -65
| `EXCHANGE_RATES_LIFETIME` | `15_000` | milliseconds |
| `EXCHANGE_RATES_URL` | `undefined` | endpoint on the Account Servicing Entity to request receiver fees |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved up to alphabetize

@mkurapov mkurapov requested a review from BlairCurrey June 23, 2023 12:43
@@ -37,7 +37,6 @@ describe('Open Payments Payment Pointer Service', (): void => {
let paymentPointerService: PaymentPointerService
let accountingService: AccountingService
let knex: Knex
let config: IAppConfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we can remove line 48 as well (duplicate config = await deps.use('config') in beforeAll)

@mkurapov mkurapov merged commit 22ef1dc into main Jun 23, 2023
@mkurapov mkurapov deleted the 1419/mk/fetch-payment-pointers branch June 23, 2023 14:11
Muasa-harman pushed a commit to Muasa-harman/rafiki that referenced this pull request Jul 13, 2023
* feat: ask for payment pointer from ASE

* docs: update language

* docs: add webhook information

* add webhook to OpenAPI

* docs: update wording

* feat(mase): allow for on the fly payment pointer creation

* chore(mase): update webhook handler

* docs: update wording, link to section

* fix test

* docs: alphabetize env vars in

* chore: remove duplicate line
@huijing huijing added the type: documentation (archived) Improvements or additions to documentation label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. pkg: documentation Changes in the documentation package. pkg: mock-ase type: documentation (archived) Improvements or additions to documentation type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interact with ASE on Payment Pointer request
3 participants