-
Notifications
You must be signed in to change notification settings - Fork 103
feat: handle grant spent amount calculation on payment completion #3674
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
Conversation
…ernal server error
Co-authored-by: Max Kurapov <max@interledger.org>
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.
A few thoughts
| state: OutgoingPaymentState.Funding, | ||
| grantId | ||
| grantId, | ||
| createdAt: new Date() |
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.
This is done automatically
(we have table.timestamp('createdAt').defaultTo(knex.fn.now()) in the create outgoing payments table migration)
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 had to do this so it can be controlled via fake timers in the tests. Otherwise there was a test that failed. It was some business logic checking mocked time vs. real outgoingPayment.createdAt (since it is set by postgres and uncontrollable by jest's fake timers).
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.
Would it still work if the test patched the outgoing payment after it was created?
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.
Good thought on the patch, but I just tried it and no it doesnt work, because this check that comes right after creating the payment in outgoingPaymentService.create:
if (
payment.walletAddressId !== payment.quote.walletAddressId ||
payment.quote.expiresAt.getTime() <= payment.createdAt.getTime()
) {
throw OutgoingPaymentError.InvalidQuote
}Where the quote's expiresAt is subject to the mocked time but the payment is not. Trying to patch after its created results in a InvalidQuote error.
I think it's probably generally useful to be able to control it from the application level, so it can be mocked like this. Functionally setting it to new Date() should be the same as knex.fn.now().
packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Outdated
Show resolved
Hide resolved
| }, | ||
| 'No outgoingPaymentGrantSpentAmounts record found for grant interval' | ||
| ) | ||
| return |
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.
Need to think through this a bit, but I think we would still want to update the OutgoingPaymentGrantSpentAmounts instead of returning early
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.
Like, as a fallback? Is that your thinking?
I guess maybe that makes sense. This should never happen though so IDK how we'd get the correct interval/grant totals. Since it's totally unexpected I dont think we can just assume the interval/grant amounts were 0 or something. I suppose we can sum them all up like we do for the legacy path on the payment create side.
packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Outdated
Show resolved
Hide resolved
| state: OutgoingPaymentState.Funding, | ||
| grantId | ||
| grantId, | ||
| createdAt: new Date() |
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.
Would it still work if the test patched the outgoing payment after it was created?
amount for spent amount recalc
grant spent amount updates
packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/payment/outgoing/lifecycle.ts
Outdated
Show resolved
Hide resolved
- turned duplicated logic into shared functions - example fn where revert/update is all in one, but felt it was more complicated
| ) | ||
|
|
||
| await OutgoingPaymentGrantSpentAmounts.query(deps.knex).insert({ | ||
| ...latestPaymentSpentAmounts, |
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.
To be safe, let's not have this spread operator and just define all of the fields in-line, otherwise we might include fields that we don't want to update
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.
changed
| .first() | ||
|
|
||
| // Should have new spent amounts with payment factored out | ||
| expect(latestGrantSpentAmounts).toMatchObject({ |
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.
minor, but lets check there are two records that were created here
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.
added a check to make sure they are different records.
| }, | ||
| 'ILP payment completed' | ||
| ) | ||
| return { receive: receipt.amountDelivered, debit: finalDebitAmount } |
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 we should leave a comment here explaining the behaviour why not the receipt.amountSent
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 this point I think its just easier and more clear to just remove the debit amount from the return type. updated w/ that.
1be38b5
into
bc/raf-1031/grant-spent-amounts
resolves #3374 (linear)
Some relevant context:
OutgoingPaymentGrantSpentAmounts)Changes
handleSendingandhandleFailedto get the latest spent amounts and update as needed. This happens within the lifecycle of the worker (outgoingPayment.processNext).lifecycle.test.tswhich tests the grant total counting by callingoutgoingPaymentService.processNext. This required mocking thepaymentMethodHandler.paymethod since we dont have another real rafiki to run it against and we need the control of manipulating the response (it fails, it completes partially, completes fully)Deviations from the issue as stated:
Manual Test
I mostly tested using unit tests because I needed control over the time (for intervals) and how
paywas resolving. Here is how you can do some manual tests via bruno to verify the happy path.Basic single payment case.
Example > Open Paymentsflowcloud_nine_wallet_backenddatabase using your preferred method. You can connect directly to the psql shell withdocker exec -it rafiki-shared-database-1 su - postgres -c "psql cloud_nine_wallet_backend"and then enter\xto make the output more readable.select * from "outgoingPaymentGrantSpentAmounts";(filter by grantId/outgoingPaymentId etc. as needed if not a fresh volume).