Skip to content
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

chore: prevent WalletInvoice deletion #3557

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Conversation

UncleSamtoshi
Copy link
Contributor

@UncleSamtoshi UncleSamtoshi commented Nov 15, 2023

  • add processingCompleted field to replace deleting canceled invoices

To Do:

  • After this pr has been merged and is live for more than the expiration period of invoices, migrate mongo db to set all non-existent processingCompleted fields to true
  • After db migration change processingCompleted field to be non-optional

@github-actions github-actions bot added the core label Nov 15, 2023
@UncleSamtoshi UncleSamtoshi force-pushed the prevent-invoice-deletion branch 2 times, most recently from 4c1e047 to 455dec5 Compare November 16, 2023 15:36
Copy link
Collaborator

@dolcalmi dolcalmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just few comments

@@ -302,6 +316,7 @@ const updatePendingInvoiceBeforeFinally = async ({
"invoices.finalRecipient": JSON.stringify(recipientWalletDescriptor),
})

// Is this crossing the lock boundary?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove, also not sure... what you mean? this can be done outside the lock but need to be done previous to ledger record (that must be inside lock)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove it, but lnInvoiceLookup is populated before the lock, so its state could change by the time the lock is acquired.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh I thought you referred to settle... you are right, feel free to move it

@@ -69,6 +69,7 @@ interface WalletInvoiceRecord {
currency: string
timestamp: Date
selfGenerated: boolean
processingCompleted?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to add it as optional? isnt default value working?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't default value only used on write? I have a database migration that will go back and populate this field and then i will make it non optional.

@UncleSamtoshi UncleSamtoshi merged commit 6b16663 into main Nov 16, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants