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.

Sign up for GitHub

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

Jump to bottom

Offers: invoice verification #89

Merged
merged 3 commits into from
Mar 7, 2024
Merged

Offers: invoice verification #89

merged 3 commits into from
Mar 7, 2024

Conversation

orbitalturtle
Copy link
Collaborator

@orbitalturtle orbitalturtle commented Jan 20, 2024 •
edited
Loading

Verify the returned BOLT 12 invoice. Depends on #87

@orbitalturtle orbitalturtle requested a review from carlaKC January 22, 2024 03:35
@orbitalturtle orbitalturtle force-pushed the invoice-verification branch 2 times, most recently from b6ef4a9 to 092d078 Compare January 23, 2024 20:19
@orbitalturtle
Copy link
Collaborator Author

There's one unit test failure here that I'm still looking into.

@orbitalturtle orbitalturtle force-pushed the invoice-verification branch 2 times, most recently from 6522401 to 2bcd34b Compare January 25, 2024 23:16
carlaKC
carlaKC reviewed Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Major comment here is better handling the commits between this and the previous PR so we don't have to have double refactors. Happy for you to squash any changes you need with #85 to cut down on this 👍

src/lndk_offers.rs Outdated Show resolved Hide resolved
src/lndk_offers.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
Comment on lines 289 to 250
// TODO: Eventually we can use the returned payment id below to check if this
// payment has been sent twice.
Ok(_payment_id) => Some(OffersMessage::Invoice(invoice)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it safe to leave this as a todo? Seems like this is the key thing that we need to be worried about, people sending us multiple invoices and us double-paying. Ignore me if this happens in a later on PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm this might be worth some discussion... It seems to me like a PaymentId is only very useful when we have multiple payments in flight and need help distinguishing them. You're definitely right, we need to make sure we aren't getting double invoice requests. But for now in the lndk-cli pay-offer scenario, where there's only going to be one offer/invoice request in the OfferHandler at a time anyway, one option would be just to reject the invoice if we already updated the OfferState to InvoiceReceived, InvoicePaymentDispatched, or InvoicePaid. (I would add this to the next PR since that's where we add the invoice to a queue.) Thoughts?

Also, as you suggested, I wrote up some more context in the comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha. Can we update that todo to explain why we can leave this (for now) and still be safe?

@orbitalturtle orbitalturtle force-pushed the send-invoice-req branch 4 times, most recently from 60dc2da to 7709037 Compare January 31, 2024 00:48
@orbitalturtle orbitalturtle force-pushed the send-invoice-req branch 4 times, most recently from bb0c078 to d932363 Compare February 1, 2024 02:31
@orbitalturtle orbitalturtle force-pushed the invoice-verification branch 2 times, most recently from 48bb807 to c9c7d63 Compare February 1, 2024 03:27
carlaKC
carlaKC approved these changes Feb 2, 2024
View reviewed changes
Copy link
Collaborator

@carlaKC carlaKC 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 once upstream is ready

@orbitalturtle orbitalturtle force-pushed the invoice-verification branch 3 times, most recently from 5230aac to b221d0e Compare February 6, 2024 07:02
@orbitalturtle orbitalturtle force-pushed the send-invoice-req branch 3 times, most recently from cb6764b to e14f2b4 Compare February 8, 2024 01:34
@orbitalturtle orbitalturtle force-pushed the invoice-verification branch 3 times, most recently from 89423f4 to c3a4f7d Compare February 8, 2024 01:51
@orbitalturtle orbitalturtle mentioned this pull request Mar 1, 2024
Merged
@orbitalturtle
multi: add verification details to invoice request
bf03063 
Once we get an invoice returned from the offer creator, we'll be able to
verify that the invoice is a response to the invoice request we sent. We can
also use the PaymentId we set to verify we didn't already pay the invoice.
@orbitalturtle
multi: split off uir signing portion of create_invoice_request into a…
a6c72c6 
… method

We split the unsigned invoice request (uir) signing portion into
another function to make it easier to mock. Earlier we had hardcoded
a signature for our lndk_offers.rs unit tests. But now that we've
changed the uir being signed (by adding verification metadata), the
hardcoded signature is invalid. We refactor our mocking to get rid of
this hardcoded signature data completely.
@orbitalturtle
offers: verify invoice upon return
df53046
@orbitalturtle orbitalturtle force-pushed the invoice-verification branch from be2714c to df53046 Compare March 6, 2024 21:26
@carlaKC carlaKC changed the base branch from send-invoice-req to master March 7, 2024 17:40
@carlaKC carlaKC merged commit 0e74b50 into master Mar 7, 2024
3 checks passed
@orbitalturtle orbitalturtle deleted the invoice-verification branch May 30, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlaKC carlaKC carlaKC approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@orbitalturtle @carlaKC