-
Notifications
You must be signed in to change notification settings - Fork 90
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
feat(backend): support Open Payments connection as receiver #603
Conversation
Auth is only required on mocked payment pointer endpoints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's late and I had a glass of wine because it's my wedding anniversary. If you need another review, I can look at it first thing tomorrow morning.
} | ||
|
||
const CONNECTION_URL_REGEX = /\/connections\/(.){36}$/ | ||
const INCOMING_PAYMENT_URL_REGEX = /\/incoming-payments\/(.){36}$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The id doesn't necessarily have to be a uuid so I'd remove the (.){36}
constraint. Maybe just check that it is alpha-numerical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's currently enforced as uuid
in the OpenAPI spec:
https://github.com/interledger/open-payments/blob/6d128e2298301a3a76d2421581e2ffa097ebf33c/open-api-spec.yaml#L629-L636
and this would make it even more so:
https://github.com/interledger/open-payments/pull/174/files#diff-aaff56ee0c5bb1e404fc4e010a7e38034fbc755542d28199e4cf0acb5ef580c9R625
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting 🤔. I'm wondering whether we actually want to enforce that in the spec. It seems to me like more of a business decision.
@@ -255,6 +255,6 @@ export type IncomingPaymentJSON = { | |||
externalRef?: string | |||
createdAt: string | |||
updatedAt: string | |||
expiresAt: string | |||
expiresAt?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we had that as required since it is either set to the client's value or the server's default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct for Rafiki the Open Payments server.
But Rafiki the client of an arbitrary Open Payments server has to go by the OpenAPI spec in which expiresAt
isn't required:
https://github.com/interledger/open-payments/blob/6d128e2298301a3a76d2421581e2ffa097ebf33c/open-api-spec.yaml#L915-L939
if (typeof incomingPayment.ilpStreamConnection !== 'object') { | ||
return undefined | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit pick: I'd move that in front of the previous one.
Changes proposed in this pull request
Context
Checklist
fixes #number