Skip to content

Simplify Bolt11 Payments and Remove Redundant Code - #3617

Merged
TheBlueMatt merged 5 commits into
lightningdevkit:mainfrom
shaavan:i3262b
Mar 6, 2025
Merged

Simplify Bolt11 Payments and Remove Redundant Code #3617
TheBlueMatt merged 5 commits into
lightningdevkit:mainfrom
shaavan:i3262b

Conversation

@shaavan

Copy link
Copy Markdown
Member

resolves#3262
builds on: #3342

Summary

This PR introduces pay_for_bolt11_invoice, simplifying how Bolt11 payments are processed by allowing direct invoice-based payments while still enabling custom routing parameters. With this change, the redundant bolt11_payment.rs utility functions are no longer needed and have been removed.

Why This Change?

  • Previously, paying a Bolt11 invoice required manually extracting payment_id, payment_hash, and other parameters before passing them to send_payment.
  • pay_for_bolt11_invoice introduces a more streamlined approach, mirroring the existing convenience of Bolt12 payments.
  • With this function now handling parameter extraction within ChannelManager, bolt11_payment.rs is redundant and can be safely removed.

Changes

  • Introduced pay_for_bolt11_invoice:
    • Takes a Bolt11 invoice directly.
    • Allows users to set custom routing parameters via RouteParametersConfig.
  • Removed bolt11_payment.rs:
    • All relevant logic is now handled within ChannelManager.
    • Removed associated test cases for redundant utility functions.

Impact

  • Reduces complexity for Bolt11 payments.
  • Cleans up redundant code, making the implementation more maintainable.

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! This needs rebase now.

Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs Outdated
@shaavan

Copy link
Copy Markdown
MemberAuthor

Updated from pr3617.01 to pr3617.02 (diff):

Changes:

  1. Rebase on main.

@shaavan

Copy link
Copy Markdown
MemberAuthor

Updated from pr3617.02 to pr3617.03 (diff):
Addressed @TheBlueMatt comments

Changes:

  1. Introduce payment_id as parameter, and update documentation

@codecov

codecovBot commented Feb 26, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 84.50704% with 11 lines in your changes missing coverage. Please review.

Project coverage is 88.57%. Comparing base (1610854) to head (16368c0).
Report is 2 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/ln/outbound_payment.rs81.57%3 Missing and 4 partials ⚠️
lightning/src/events/mod.rs0.00%2 Missing ⚠️
lightning/src/routing/router.rs86.66%0 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3617 +/- ##
==========================================
- Coverage 88.60% 88.57% -0.03% 
==========================================
Files 151 150 -1 Lines 118454 118377 -77 Branches 118454 118377 -77 ==========================================
- Hits 104957 104856 -101 - Misses 10985 10990 +5 - Partials 2512 2531 +19 

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

TheBlueMatt
TheBlueMatt previously approved these changes Feb 27, 2025

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks!

Comment threadlightning/src/events/mod.rs Outdated
Comment on lines +1711 to +1712
Some(PaymentFailureReason::InvalidAmount) =>
&Some(PaymentFailureReason::InvalidAmount)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't this break downgrades? We need to map to a variant available prior to version 0.0.124. I guess it is never serialized in an event, and instead return directly? I wonder if we should make a Bolt11PaymentError similar to Bolt12PaymentError instead.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oops, right.

We def could add a Bolt11PaymentError, tho it seemed like way too much complexity for one variant to me 🤷

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks a lot for the pointer! Updated in pr3617.04
I think it makes sense that we introduce a new Bolt11PaymentError, as its more 'proper' error type, and also will be useful incase the enum expands in future.

tho it seemed like way too much complexity for one variant to me

After, introducing the variant, I think I was able to get it to work, while keeping the complexity manageable!
Let me know if it looks good! Thanks!

Comment threadlightning/src/ln/bolt11_payment.rs
@shaavan

Copy link
Copy Markdown
MemberAuthor

Updated from pr3617.03 to pr3617.04 (diff):
Addressed @jkczyz comments

Changes:

  1. Introduce new variant Bolt11PaymentError, to not break downgrades.
  2. Move the relevant test from bolt11_payment.rs, to the codebase, to keep the test suite exhaustive.

@shaavan

Copy link
Copy Markdown
MemberAuthor

Updated from pr3617.04 to pr3617.05 (diff):
Addressed @jkczyz comment

Changes:

  1. Introduce explicit check for failure case in the introduced test.

Comment threadlightning/src/ln/invoice_utils.rs Outdated
@jkczyz
jkczyz requested a review from TheBlueMattMarch 3, 2025 18:00

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, sadly needs another rebase now :/

@shaavan

Copy link
Copy Markdown
MemberAuthor

Updated from pr3617.05 to pr3617.06 (diff):
Addressed @jkczyz comment

Changes:

  1. Renamed the bolt11_payment.rs -> bolt11_payment_tests.rs
  2. Move the introduce tests to bolt11_payment_tests.rs

In the upcoming commits, functions will require variant to handle cases where a
Bolt11 invoice specifies an invalid amount. This commit introduces the variant,
setting the stage for its usage in subsequent commits.
Previously, paying a Bolt11 invoice required manually extracting the
`payment_id`, `payment_hash`, and other parameters before passing them to
`send_payment`.
This commit introduces `pay_for_bolt11_invoice`, bringing the same simplicity
already available for Bolt12 invoices. It allows users to pass the entire
Bolt11 invoice directly while also supporting custom routing parameters
via `RouteParametersConfig`.
@shaavan

Copy link
Copy Markdown
MemberAuthor

Updated from pr3617.06 to pr3617.07 (diff):
Addressed @TheBlueMatt comments

Changes:

  1. Rebase on main to resolve merge conflicts

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like bolt11_payment_tests.rs is using spaces instead of tabs

@shaavan

Copy link
Copy Markdown
MemberAuthor

Updated from pr3617.07 to pr3617.08 (diff):

Changes:

  1. Fixed rust formatting for bolt11_payment_tests.rs

@jkczyz
jkczyz requested a review from TheBlueMattMarch 6, 2025 15:56
@TheBlueMatt
TheBlueMatt merged commit e6267d3 into lightningdevkit:mainMar 6, 2025
Sign up for freeto 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.

BOLT12 sending doesn't allow to override {Route,Payment}Parameters

3 participants

@shaavan@TheBlueMatt@jkczyz