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

odds and ends: tls SAN cfg and payer note option #144

Merged
merged 6 commits into from
Aug 5, 2024

Conversation

orbitalturtle
Copy link
Collaborator

This PR includes a few smaller requested changes in one PR:

  • Right now the SAN in the tls certificate we autogenerate is hardcoded as localhost. We add a tls-ip config option for specifying other ip addresses/domains. Multiple can be added if separated by commas.
  • We allow a user to add a "payer note" with either the pay-offer and get-invoice commands.
  • Adds cli documentation for baking a custom macaroon that can be used for paying an offer.

Closes #134.

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (7f9e0f9) to head (b3f9c30).
Report is 4 commits behind head on master.

Files Patch % Lines
src/main.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master    #144   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           1       1           
  Lines          97      97           
======================================
  Misses         97      97           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@orbitalturtle orbitalturtle added this to the 0.2.0 milestone Jul 23, 2024
@mrfelton
Copy link
Contributor

The TLS IP stuff is working well. One observation with it was that the TLS cert was automatically regenerated when we restarted with the ip settings in place. It might be an idea to follow the model of how this works in lnd - which is to only regenerate the cert after deleting the existing ones from the file system, regardless of wether the startup options have changed. This provides a level of stability ensuring that in use tls certs don't get invalidated without explicit action to trigger that (deleting the old ones and restarting the node),

@orbitalturtle
Copy link
Collaborator Author

@mrfelton I just did some tests, and I believe that is how it works? It should only regenerate the certificate if either the key or certificate in ~/.lndk have been deleted:

if !key_path.exists() || !cert_path.exists() {
. (Though I think we should add a comment to the tls-ip config option explaining that this is the case - if a cert already exists, it must be deleted before it will regenerate.)

@mrfelton
Copy link
Contributor

Retested and confirmed that the cert does only get regenerated if it's deleted from the disk.

@mrfelton
Copy link
Contributor

I added an extra commit in orbitalturtle#10 which adds the payer note into the fetch/decode invoice response data.

@mrfelton
Copy link
Contributor

Looks like when no payer note is provided, it's actually setting the payer note on the invoice request as an empty string which I don't think is correct. The resulting invoice has payer_note: Some("") when it probably should have payer_note: None

@orbitalturtle
Copy link
Collaborator Author

@mrfelton Yeah good point. I wasn't sure how to do that without getting a move error, but Jeff from LDK showed me how they do it on their end. Just pushed up a version with that change.

Just added the payer_note to the decoded invoice as well.

Copy link
Collaborator

@dunxen dunxen 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 to me from non-API perspective.

@@ -20,6 +21,7 @@ message PayOfferResponse {
message GetInvoiceRequest {
string offer = 1;
optional uint64 amount = 2;
optional string payer_note = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's actually been a while since I wrote protos but I'm quite glad they brought back optional fields to v3 finally :)

@orbitalturtle orbitalturtle merged commit 25e3e2d into lndk-org:master Aug 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Ability to specify payer note when fetching an invoice
3 participants