-
Notifications
You must be signed in to change notification settings - Fork 143
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
refactor(core): clean pending invoice 'markAsPaid' signals #4028
Conversation
8d020de
to
1986270
Compare
a08a638
to
5138f46
Compare
return ProcessPendingInvoiceResult.err(lndService) | ||
} | ||
// Returns 'true' on re-runs if invoice is already settled in lnd |
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 if the invoice is already settled we need to early return right?
The way I currently read the code it is possible for 339 const journal = await LedgerFacade.recordReceiveOffChain({
to be executed more than once.
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.
We do higher up I believe, but I just noticed that it's a paid
check and not a processingCompleted
check. Fixed here 52b1db1
@@ -173,7 +183,7 @@ const processPendingInvoice = async ({ | |||
currency: WalletCurrency.Btc, | |||
}) | |||
if (receivedBtc instanceof Error) { | |||
return ProcessPendingInvoiceResult.paidWithError(receivedBtc) | |||
return ProcessPendingInvoiceResult.processAsPaidWithError(receivedBtc) |
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.
if we have a conversion error it should be marked as paid/processed or just error?
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.
True, this is the same kind of error as the checkedToSats
one just above it, a validation error from an invalid amount passed. I made a change to treat them both the same and to decline the invoice if an invalid amount somehow comes through the process cdcfcd7
In theory this specific check will only fail if the amount passed in is a non-integer value, which should never really happen.
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.
just few questions.. also... I couldn't find the milisats tests did you remove them in the bats refactor?
…pired' are mutually exclusive
cdcfcd7
to
ee294b7
Compare
Description
This is a refactor to clean up and clarify when we mark transactions as "processed" or "paid and processed" depending on the outcome of the "update pending invoices" function.
This was prompted by issues observed where invoices would sometimes be wrongly marked as paid.