Unify payment methods into single pay() function #20
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9948d6762d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
martinsaposnic
force-pushed
the
unified-pay-method
branch
from
9948d67 to
4916a47
Compare
|
@codex review |
Replace payLnurl(), payBolt11(), and payBolt12Offer() with a single pay() method that auto-detects the destination type using bitcoin_payment_instructions. Supported destinations: - BOLT12 offers (lno...) - LNURL (lnurl...) - Lightning addresses (user@domain) - Zero-amount BOLT11 invoices This is a breaking change: BOLT11 invoices with pre-set amounts are no longer supported through this API. The design enforces that the caller always specifies the amount, which: - Simplifies the API surface (one method instead of three) - Ensures consistent amount handling across all payment types - Protects against malicious LNURL services returning wrong amounts The amount_msat parameter is now always required.
martinsaposnic
force-pushed
the
unified-pay-method
branch
from
4916a47 to
edcc1a6
Compare
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
|
||
| // Validate BOLT11 invoice amount matches requested (protects against malicious LNURL services) | ||
| if let PaymentTarget::Bolt11(ref invoice, _) = payment_target { | ||
| if let Some(invoice_amount) = invoice.amount_milli_satoshis() { |
There was a problem hiding this comment.
What if the LNURL returns no amount?
There was a problem hiding this comment.
if it returns no amount, we will skip validation and proceed with the requested amount. we always use send_using_amount with the requested amount so I think this is safe
|
|
||
| /// Internal enum representing a resolved payment target with amount | ||
| enum PaymentTarget { | ||
| Bolt11(Bolt11Invoice, u64), // invoice + amount_msat |
There was a problem hiding this comment.
nit: Worth boxing the Bolt11Invoice too? Think it's also large enough to warrant it but not certain
Summary
payLnurl(),payBolt11(), andpayBolt12Offer()with a singlepay()methodbitcoin_payment_instructionscrateSupported destinations
lno...)lnurl...)user@domain)Breaking change
BOLT11 invoices with pre-set amounts are no longer supported directly. The
amount_msatparameter is always required. This enforces consistent amount handling across all payment types.For BOLT11 invoices returned from LNURL/lightning address resolution, the code validates that any embedded amount matches the requested amount (protection against malicious services).
Test plan