refactor(backend): handle grant lookup more gracefully in remote incoming payment service #2888
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…etOrCreate function
github-actions
bot
added
pkg: backend
mkurapov
force-pushed
the
2852/mk/get-or-create-ip-grants
branch
from
5a69214 to
4bff426
Compare
mkurapov
force-pushed
the
2852/mk/improve-ip-grant-lookups
branch
from
2e014e5 to
1b85b85
Compare
mkurapov
commented
Aug 23, 2024
Comment on lines
+101
to
+102
| const resourceServerUrl = | ||
| walletAddress.resourceServer ?? new URL(walletAddress.id).origin |
There was a problem hiding this comment.
We don't really need new URL(walletAddress.id).origin at this point, but being defensive
mkurapov
commented
Aug 23, 2024
| return createIncomingPayment(deps, { | ||
| createArgs, | ||
| walletAddress, | ||
| retryOnTokenError: false |
There was a problem hiding this comment.
we could make it such that we retry multiple times, but for our use case, we just want to retry with a new grant once
# Conflicts: # packages/backend/src/open_payments/grant/service.test.ts # packages/backend/src/open_payments/grant/service.ts
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
BlairCurrey
approved these changes
Aug 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes proposed in this pull request
A new
grantService.getOrCreatefunction was created in #2887, that, besides storing grant information, does all of the actual Open Payments grant requests with the OP client (as well as doing token rotation for those grant tokens as necessary).Previously, the logic for requesting grants and doing token rotation was repeated in the
remoteIncomingPaymentServiceas well as thereceiverService. Both services were doing the same thing: using the OP client to do grant requests and do token rotation. Now, since this logic is consolidated ingrantService.getOrCreate, we are able to clean up those respective services:remoteIncomingPaymentServicecallsgrantService.getOrCreatein both of itscreateandgetmethods, which allows us to clean it up considerablyreceiverServicejust has to callremoteIncomingPaymentService.get/createto get & create remote incoming payments (which under the hood handles the grant management), which allows us to to clean up its logicAdditionally, in the
remoteIncomingPaymentService, there is additional logic that is a bit more graceful in its incoming payment requests: if we fail to get or create an incoming payment with an existing grant (by checking for 401 or 403 errors), we delete the old grant, and try a request once more.Finally, we remove old external grantService methods which are no longer used after these changes.
Context
Fixes #2852
Checklist
fixes #number