-
Notifications
You must be signed in to change notification settings - Fork 104
feat: grant spent amounts #3757
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
base: main
Are you sure you want to change the base?
Conversation
* feat: add grant spent amount table * fix: change interval start/end string to timestamp * feat: fix filename, update op grant table * fix: rm erroneously added migration to tsconfig
* feat: calculate grant spent amounts from new table * chore: linting * fix: revise grant spent amounts calc to track interval * fix: throw better error, dont inti interval amounts unless they exist * fix: linter, wip commented out code * refactor(backend): rename interval statuses * fix: ensure correct amount (interval/total) is used for grant spent amount * test(backend): grant spent amounts * fix(backend): make logic more typesafe, avoid having to defend w/ internal server error * test(backend): legacy grant calculations * test(backend): classify payment interval fn * test(backend): rm commented-out classify payment interval case * refactor(backend): improve semantics of interval helper fn * test(backend): add non-interval cases to create op * Apply suggestions from code review Co-authored-by: Max Kurapov <[email protected]> * refactor: simplifiy interval calc * chore: rm unused imports * refactor: validate grant payment logic * refactor: improve grant spent amount calc type safety * refactor: improve grant spent amount calc type safety * fix: missing trx * chore: handle bad interval state * chore: rm some superfluous comments * chore: format --------- Co-authored-by: Max Kurapov <[email protected]>
) * feat: calculate grant spent amounts from new table * chore: linting * fix: revise grant spent amounts calc to track interval * fix: throw better error, dont inti interval amounts unless they exist * fix: linter, wip commented out code * refactor(backend): rename interval statuses * fix: ensure correct amount (interval/total) is used for grant spent amount * test(backend): grant spent amounts * fix(backend): make logic more typesafe, avoid having to defend w/ internal server error * test(backend): legacy grant calculations * test(backend): classify payment interval fn * test(backend): rm commented-out classify payment interval case * refactor(backend): improve semantics of interval helper fn * test(backend): add non-interval cases to create op * Apply suggestions from code review Co-authored-by: Max Kurapov <[email protected]> * feat: partially handled grant spent amount scenarios on settle * feat: handle non-interval partial settleamount cases * test(backend): failure grant spent amount case for sucessive payment * test(backend): some grant calc interval cases * test(backend): counting grant payments across interval boundaries * chore(backend): rm commented out test * test(backend): new grant spent amount on payment completion * test(backend): improve interval grant calc test to show summation * test(backend): failure edge case * chore(backend): rm comment * chore(backend): fix lint errors * chore(backend): rm debug logs * test(backend): fix failing * Update packages/backend/src/open_payments/payment/outgoing/lifecycle.ts Co-authored-by: Max Kurapov <[email protected]> * fix(backend): rm extra spent amount record check * feat(backend): return debit amount from .pay * test(backend): failing test for grant spent amount bug - if 2 payments are create then 2 payments are processed, it assocaites the wrong spent amount with the wrong payment * fix(backend): grant spent amounts calc race conditions - partially implemented fix (missing interval stuff), not fully validated by tests yet * test(backend): grant calc race condition * fix(backend): grant spent amount race conditions with failure * chore(backend): format * test(backend): add failing interval race condition test * fix(backend): interval boundary/race condition edge case - edge case is when there are create/complete race conditions around interval boundaries * test(backend): improve edge case test - ensures we are testing that the latest interval amount is used as the base for interval amounts, not just the payment in the interval being completed * fix(backend): dont query for latest interval payment unecunnecessarily - not necessary if we are within the interval of the payment being completed * fix(backend): type mismatch * test(backend): payment retries do not add additional spent record * fix(backend): use correct debit amount for spent amount recalc * fix(backend): set correct payment state on grant spent amount updates * refactor(backend): dedupe revert/handle grant spent amounts - turned duplicated logic into shared functions - example fn where revert/update is all in one, but felt it was more complicated * chore: rm unused fn * test(backend): fix ilp pay return expectations * refactor(backend): move revert/update grant spent amounts to op service * chore: rm unused import * fix: handle grant spent amounts on payment cancellation * chore: cleanup commented out code * chore: rm submodule added in error * chore(backend): add error logs * fix(backend): explicit grant spent amount formation, more test assertions * refactor(backend): only return receive amt from .pay * fix(backend): build error --------- Co-authored-by: Max Kurapov <[email protected]>
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🚀 Performance Test ResultsTest Configuration:
Test Metrics:
📜 Logs |
* feat: add /GET outgoing-payment-grant route * fix: change null state return to return null * fix: rm old comments * fix: add get grant spent amount service tests * fix: add get grant spent amount controller tests * fix(backend): linter * fix(backend): tenanted path * fix: dont require identifier in outgoing payment access item introspection * feat(backend): rm unecessary check * test(auth): add test * test(backend): add middleware test * refactor(backend): cleanup grant spent amount retrieval * chore: fix lint * test(backend): fix grant id check * test(backend): simplify logic, return 0 amount when able * refactor(backend): token introspection middleware - make 2nd introspection middleware for new route instead of generalizing exisitng middleware * test(backend): rm no longer needed test * fix: rm unused import * Update packages/backend/src/open_payments/auth/middleware.ts Co-authored-by: Max Kurapov <[email protected]> * fix: return 403 for non create grant for spent amounts used to return null spent amounts * fix: add logs, use luxon interval method * fix: scope of else block * Update packages/backend/src/open_payments/auth/middleware.ts Co-authored-by: Max Kurapov <[email protected]> * chore: fix name * feat: test for new middleware/shared fn * chore: make action required --------- Co-authored-by: Max Kurapov <[email protected]>
mkurapov
left a comment
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.
After we have updated the NodeJS client, we should also update an integration test to verify grant spent amounts as well
|
|
||
| await middleware(ctx, next) | ||
|
|
||
| expect(mockTokenIntrospectionClient.introspect).toHaveBeenCalledWith({ |
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.
let's also validate that next was called heere
njlie
left a comment
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.
LGTM, seemed to work when I tested the local env a bit
| }) | ||
| .then(() => { | ||
| return knex.raw( | ||
| 'CREATE INDEX outgoingPaymentGrantSpentAmounts_grantId_createdAt_desc_idx ON "outgoingPaymentGrantSpentAmounts" ("grantId", "createdAt" DESC)' |
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 think I'll leave it for now.... but I was thinking about changing this:
'CREATE INDEX outgoingPaymentGrantSpentAmounts_grantId_createdAt_desc_idx ON "outgoingPaymentGrantSpentAmounts" ("grantId", "createdAt" DESC)'To include the interval fields:
knex.raw('CREATE INDEX ... ON "outgoingPaymentGrantSpentAmounts" ("grantId", "intervalStart", "intervalEnd", "createdAt" DESC)')`because we look it up like this in the worker when processing the payment:
await OutgoingPaymentGrantSpentAmounts.query(deps.knex)
.where('grantId', grantId)
.where('intervalStart', latestPaymentSpentAmounts.intervalStart)
.where('intervalEnd', latestPaymentSpentAmounts.intervalEnd)
.orderBy('createdAt', 'desc')
.first()Dont have great intuition on tradeoffs. I guess it would slow writes some amount in all cases and speed up reads some amount in a subset of cases (when we lookup by interval - only in worker I think). Probably no db storage space difference since a created_at index is going to already lead to a unique index entry per row. Overall I guess it feels like a bad idea. I guess we probably read a bit more (read and write on create, read and write in worker, read in GET /outgoing-payment-grant) but we only lookup by interval in one place.
|
Made a few small tweaks:
|
Parent branch for grant spent amounts calculation rework. Most of the code was already reviewed, but I did add something here that was not captured in the other issues. And that is getting the grant spent amounts in this edge case:
GET/outgoing-payment-grant` is called (no spent amounts, but amount was spent against the grant)In this case we are summing all historical payments (added that in this PR). We do the same thing on the outgoing payment create side so I shared the logic.
fixes #3369
linear https://linear.app/interledger/issue/RAF-1031/implement-new-grant-spent-amount-calculation