feat(backend): add clientId to OP resources #535
Conversation
github-actions
bot
added
pkg: backend
sabineschaller
force-pushed
the
s2-add-clientId-to-resources
branch
from
a005758
to
04f9d1b
Compare
packages/backend/migrations/20220228162503_create_incoming_payments_table.js
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/payment/incoming/routes.test.ts
Outdated
Show resolved
Hide resolved
| description?: string | ||
| expiresAt?: Date | ||
| incomingAmount?: Amount | ||
| externalRef?: string | ||
| } | ||
|
|
||
| export interface IncomingPaymentService { | ||
| get(id: string): Promise<IncomingPayment | undefined> | ||
| get(id: string, clientId?: string): Promise<IncomingPayment | undefined> |
There was a problem hiding this comment.
| get(id: string, clientId?: string): Promise<IncomingPayment | undefined> | |
| get(id: string): Promise<IncomingPayment | undefined> |
What if the clientId check is moved out to the calling routes functions?
I'm thinking however/wherever we check clientId is where we'll also check the payment pointer, both of which are only relevant to the routes.
There was a problem hiding this comment.
Which check are you referring to?
There was a problem hiding this comment.
Matching the clientId (and paymentPointerId)
What if incomingPaymentService.get still just took the id, and IncomingPaymentRoutes.get would check the clientId (and paymentPointerId) of the returned incoming payment.
packages/backend/src/open_payments/payment/incoming/service.test.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/grantReference/service.test.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/grantReference/service.test.ts
Outdated
Show resolved
Hide resolved
| await grantReferenceService.lock(grantRef.id, trx) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| await new Promise((f: any) => setTimeout(f, 6000)) | ||
| await grantReferenceService.get(grantRef.id, trx) |
There was a problem hiding this comment.
You could also just lock it and then do
await expect(grantReferenceService.get(grantRef.id, trx).skipLocked()).resolves.toBeUndefined()
There was a problem hiding this comment.
That is probably the faster version. 👍
There was a problem hiding this comment.
So I can't do skipedLocked on the get method, instead I put
await GrantReference.transaction(async (trx) => {
await grantReferenceService.lock(grantRef.id, trx)
await expect(
GrantReference.query(trx).findById(grantRef.id).skipLocked()
).resolves.toBeUndefined()
})
However, that resulted in Rejected to value: [Error: .skipLocked() can only be used after a call to .forShare() or .forUpdate()!].
There was a problem hiding this comment.
You could add one of those to query or just leave the test as is.
There was a problem hiding this comment.
That also didn't work. It queries it successfully. It probably has to do with the fact that we can't do forNoKeyUpdate() using objection. I'll just leave it as is.
| options: CreateGrantReferenceOptions, | ||
| trx?: Transaction | ||
| ): Promise<GrantReference> | ||
| lock(grantId: string, trx: TransactionOrKnex): Promise<void> |
There was a problem hiding this comment.
| lock(grantId: string, trx: TransactionOrKnex): Promise<void> | |
| lock(grantId: string, trx: Transaction): Promise<void> |
There was a problem hiding this comment.
This has implications on the validateGrant function in the OutgoingPaymentService. Right now, one of its input parameters is deps:ServiceDependencies, which includes deps.knex:TransactionOrKnex. If we don't want to use that, I have to specifically pass the transaction to validateGrant. Do you think we should make that change?
There was a problem hiding this comment.
Changes proposed in this pull request
Context
Checklist
fixes #number