Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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): request grant to query incoming payment receiver #779
feat(backend): request grant to query incoming payment receiver #779
Changes from 5 commits
ce5c135
6f7b31f
2988e54
3763cd6
da07fea
417e6a3
4a453a9
aa571e1
19fe828
291ec8c
f29d867
2e2a3a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 added this after noticing 👇 while testing:
However, I am now sometimes getting the following when starting the localenv:
I wonder if that can be avoided by splitting these into separate/subsequent
docker compose
commands:rafiki/package.json
Lines 20 to 21 in 632bdfb
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.
Is it sufficient to not store the entire access array of the grant?
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 is for now since the RS is only requesting read-all incoming payment grants, and the new
GrantService
only supports creating grants of a single access type.Do we anticipate the RS requesting multi-type grants that we should consider future-proofing for?
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.
Anything we can do here (naming wise) to make it more explicit what the difference between Grant and GrantReference is?
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.
What about
GrantReference
->ResourceGrant
(or(PaymentPointer)SubresourceGrant
)?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'm also tempted to backtrack on this: #535 (comment)
We could instead store
clientId
(soon to beclient
payment pointer #737) on subresources.Rename
GrantReference
asOutgoingPaymentGrants
(since it's only really needed to lock on limit checking 👇) and drop theclientId
columnrafiki/packages/backend/src/open_payments/payment/outgoing/service.ts
Lines 255 to 256 in a5c36d7
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.
😬
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.
Renaming is the right move since we only store references to outgoing payment grants. I'll have to look into the other PR to understand why we can drop
clientId
.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.
At the moment, we store references for grants for incoming payments, quotes, and outgoing payments because it is how we associate
clientId
with those resources.