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

Replace maze of BOLT11 payment utilities with parameter generators #2727

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

TheBlueMatt
Copy link
Collaborator

lightning-invoice was historically responsible for actually paying invoices, handling retries and everything. However, that turned out to be buggy and hard to maintain, so the payment logic was eventually moved into ChannelManager. However, the old utilites remain.

Because our payment logic has a number of tunable parameters and there are different ways to pay a BOLT11 invoice, we ended up with six different methods to pay or probe a BOLT11 invoice, with more requested as various options still were not exposed.

Instead, here, we replace all six methods with two simple ones which return the arguments which need to be passed to ChannelManager. Those arguments can be further tweaked before passing them on, allowing more flexibility.

`lightning-invoice` was historically responsible for actually
paying invoices, handling retries and everything. However, that
turned out to be buggy and hard to maintain, so the payment logic
was eventually moved into `ChannelManager`. However, the old
utilites remain.

Because our payment logic has a number of tunable parameters and
there are different ways to pay a BOLT11 invoice, we ended up with
six different methods to pay or probe a BOLT11 invoice, with more
requested as various options still were not exposed.

Instead, here, we replace all six methods with two simple ones
which return the arguments which need to be passed to
`ChannelManager`. Those arguments can be further tweaked before
passing them on, allowing more flexibility.
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (6e40e5f) 88.80% compared to head (22305a9) 89.02%.
Report is 2 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2727      +/-   ##
==========================================
+ Coverage   88.80%   89.02%   +0.22%     
==========================================
  Files         113      113              
  Lines       89170    89871     +701     
  Branches    89170    89871     +701     
==========================================
+ Hits        79188    80009     +821     
+ Misses       7729     7596     -133     
- Partials     2253     2266      +13     
Files Coverage Δ
lightning-invoice/src/payment.rs 96.45% <96.42%> (+19.38%) ⬆️

... and 13 files with indirect coverage changes

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

@benthecarman
Copy link
Contributor

I think this closes #2390

@TheBlueMatt
Copy link
Collaborator Author

#2390 is about send_payment_with_path (and friends), not this. I actually thought about doing that in the same PR as well but its a bit more annoying since its used everywhere in tests.

Since there's a much simpler way to go about it with
`Bolt11Invoice::expires_at`.
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK

The maze has been successfully navigated!

The latest sets of functions have been clarified, making optimal use of common functions wherever possible.
Additionally, the code is also well-documented.

I have also added a summary of my analysis of this PR, which can potentially help fellow contributors in their review process.

Summary:

Let's break it down function by function:

  1. Simplifying Invoice Handling:
    • pay_zero_value_invoice has been replaced with payment_parameters_from_zero_amount_invoice.
      • Removed retry strategy and ChannelManager from params.
      • Switched to using params_from_invoice instead of the convoluted pay_zero_value_invoice_with_id.
      • Added a check for a zero-amount invoice, eliminating the need for another function (pay_zero_value_invoice_with_id).
  2. Cleaner Invoice Payment Process:
    • pay_zero_value_invoice_with_id has been removed.
    • pay_invoice_using_amount is now replaced with payment_parameters_from_invoice.
      • Instead of paying within this function, the process is sent to a common params_from_invoice function, reducing complexity.
      • The function now returns payment-related values upstream.
  3. Streamlining Probing Functions:
    • preflight_probe_invoice and preflight_probe_zero_value_invoice have been removed, as ChannelManager now handles the probing previously done here.
  4. Enhancing Code Structure:
    • Removed expiry_time_from_unix_epoch function, now using with_expiry_time defined within Payment Parameters.
  5. Testing Improvements:
    • Removed invoice and zero_value_invoice helper functions used in testing. Respective invoices are now created in their test functions.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Good to wait on @G8XSU's ACK before landing.

@G8XSU G8XSU merged commit 0456b0e into lightningdevkit:main Nov 14, 2023
15 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.

None yet

6 participants