Allow setting an HRN in invoice_requests built by `pay_for_offer` #3903

TheBlueMatt · 2025-07-01T14:33:52Z

If a user did their own BIP 353 lookup to fetch an offer from a
human readable name, we still want them to be able to use
`ChannelManager::pay_for_offer`.

Because BIP 353 offer payments require that the `invoice_request`
include the human readable name, we need to add an argument to set
the `invoice_request` HRN to `pay_for_offer`, which we do here.

and a few other misc cleanups of the pay_for_offer API.

ldk-reviews-bot · 2025-07-01T14:33:54Z

👋 I see @tankyleo was un-assigned.
If you'd like another reviewer assignemnt, please click here.

`ChannelManager::pay_for_offer` and `ChannelManager::pay_for_offer_from_human_readable_name` have accumulated a handful of arguments, most of which we expect users to never actually set. Instead, here, we move `quantity` and `payer_note` (which we expect users to never set) as well as `route_params_config` and `retry_strategy` (which we expect users to generally stick with the defaults on) into a new struct, which implements `Default`. This cleans up a good bit of cruft on payment calls.

For some time we've automatically opened a connection to the blinded path introduction point when we need to send a message we generated. However, the "Limitations" section in `ChannelManager::pay_for_offer` lists having a direct connection as required for the payment to succeed, which is not true. Instead, we simply remove the section from both `pay_for_offer` and `pay_for_offer_from_human_readable_name`.

If a user did their own BIP 353 lookup to fetch an offer from a human readable name, we still want them to be able to use `ChannelManager::pay_for_offer`. Because BIP 353 offer payments require that the `invoice_request` include the human readable name, we need to add an argument to set the `invoice_request` HRN to `pay_for_offer`, which we do here.

tnull

While I see the motivation for these changes, I'm not super convinced about some of the API changes in this PR.

tnull · 2025-07-02T06:38:29Z

lightning/src/ln/channelmanager.rs

+	///
+	#[cfg_attr(
+		feature = "dnssec",
+		doc = "Note that setting this will cause [`ChannelManager::pay_for_offer_from_human_readable_name`] to fail."


This is confusing. Why move everything to this struct if we introduce a footgun for the user?

More generally, do we really care that much that the LDK method has three more arguments or so?

Its not a huge deal, no, but it did seem like an opportunity to hide parameters we expect to never actually be used behind a default(). Especially the quantity just feels like a weird thing to put in the top-level function signature, though also the routing parameters.

I'm open to dropping the move here (or, honestly, dropping the quantity argument entirely and just setting it to 1 if the offer needs it), if you feel strongly.

(or, honestly, dropping the quantity argument entirely and just setting it to 1 if the offer needs it), if you feel strongly.

Hmm, not sure if we can just drop it, but it does have some footguns that would be nice to tackle at some point, see for example #3233

But presumably @jkczyz would have a stronger/better informed view on how to fix this/how the API should look like.

Seems pay_for_offer_from_human_readable_name should take a different type and have it translate it to OptionalOfferPaymentInfo. Or alternatively just leave quantity as a parameter to pay_for_offer.

Or we could introduce an OfferSupportingQuantity wrapper type on Offer -- which can only be created when Offer::expects_quantity is true -- and have a separate pay_for_offer_using_quantity method taking that type and having a required quantity parameter. Then drop the quantity parameter from pay_for_offer -- setting it internally to 1 if Offer::expects_quantity is true.

tnull · 2025-07-02T06:41:49Z

lightning/src/ln/channelmanager.rs

@@ -11196,12 +11196,6 @@ where
 	/// to construct a [`BlindedMessagePath`] for the reply path. For further privacy implications, see the
 	/// docs of the parameterized [`Router`], which implements [`MessageRouter`].
 	///
-	/// # Limitations
-	///
-	/// Requires a direct connection to an introduction node in [`Offer::paths`] or to


Hmm, I think the limitation generally still holds, it's just that we emit a ConnectionNeeded event that needs to be handled appropriately by the user? Maybe it would be worth documenting the latter?

tnull · 2025-07-02T06:44:28Z

lightning/src/ln/channelmanager.rs

@@ -11215,7 +11218,7 @@ where
 	/// [Avoiding Duplicate Payments]: #avoiding-duplicate-payments
 	pub fn pay_for_offer(
 		&self, offer: &Offer, amount_msats: Option<u64>, payment_id: PaymentId,
-		optional_info: OptionalOfferPaymentInfo,
+		optional_info: OptionalOfferPaymentInfo, derived_from_hrn: Option<HumanReadableName>,


If we do that, why not actually lean into the switch to bitcoin-payment-instructions and drop the pay_for_offer_from_hrn method. I don't think any of our users really implements HRNs yet, so we wouldn't even break API for anybody, AFAIK.

Yea, we could. Its still useful for a lightning-only wallet, but maybe we don't care too much about those?

What determines if something goes in OptionalOfferPaymentInfo vs added as a new parameter. Seems derived_from_hrn would rarely be Some, so why add another parameter for it?

tnull · 2025-07-02T06:45:38Z

lightning/src/ln/channelmanager.rs

@@ -11348,7 +11348,8 @@ where
 	/// If the wallet supports paying on-chain schemes, you should instead use
 	/// [`OMNameResolver::resolve_name`] and [`OMNameResolver::handle_dnssec_proof_for_uri`] (by
 	/// implementing [`DNSResolverMessageHandler`]) directly to look up a URI and then delegate to
-	/// your normal URI handling.
+	/// your normal URI handling. The `bitcoin-payment-instructions` crate provides an


Mind linking to it (crates.io, or GH) also?

jkczyz · 2025-07-02T13:51:25Z

lightning/src/ln/channelmanager.rs

@@ -11215,7 +11218,7 @@ where
 	/// [Avoiding Duplicate Payments]: #avoiding-duplicate-payments
 	pub fn pay_for_offer(
 		&self, offer: &Offer, amount_msats: Option<u64>, payment_id: PaymentId,
-		optional_info: OptionalOfferPaymentInfo,
+		optional_info: OptionalOfferPaymentInfo, derived_from_hrn: Option<HumanReadableName>,


What determines if something goes in OptionalOfferPaymentInfo vs added as a new parameter. Seems derived_from_hrn would rarely be Some, so why add another parameter for it?

jkczyz · 2025-07-02T13:53:07Z

lightning/src/ln/channelmanager.rs

+/// .
+///
+/// These fields will often not need to be set, and the provided [`Self::default`] can be used.
+pub struct OptionalOfferPaymentInfo {


s/Info/Params

jkczyz · 2025-07-02T13:53:54Z

lightning/src/ln/channelmanager.rs

+/// These fields will often not need to be set, and the provided [`Self::default`] can be used.
+pub struct OptionalOfferPaymentInfo {
+	/// The quantity of the offer which we wish to pay for. This is communicated to the recipient
+	/// and determines the minimum value which we must pay.


s/which/that

jkczyz · 2025-07-02T14:32:22Z

lightning/src/ln/channelmanager.rs

+	///
+	#[cfg_attr(
+		feature = "dnssec",
+		doc = "Note that setting this will cause [`ChannelManager::pay_for_offer_from_human_readable_name`] to fail."


Seems pay_for_offer_from_human_readable_name should take a different type and have it translate it to OptionalOfferPaymentInfo. Or alternatively just leave quantity as a parameter to pay_for_offer.

Or we could introduce an OfferSupportingQuantity wrapper type on Offer -- which can only be created when Offer::expects_quantity is true -- and have a separate pay_for_offer_using_quantity method taking that type and having a required quantity parameter. Then drop the quantity parameter from pay_for_offer -- setting it internally to 1 if Offer::expects_quantity is true.

ldk-reviews-bot requested a review from tankyleo July 1, 2025 14:44

TheBlueMatt force-pushed the 2025-07-bpi branch from bf47f4b to befb3f2 Compare July 1, 2025 18:59

TheBlueMatt added 4 commits July 1, 2025 23:32

Mention the bitcoin-payment-instructions crate in docs

076b2b3

TheBlueMatt force-pushed the 2025-07-bpi branch from befb3f2 to 076b2b3 Compare July 1, 2025 23:32

tnull reviewed Jul 2, 2025

View reviewed changes

tnull requested review from jkczyz and removed request for tankyleo July 2, 2025 06:47

jkczyz reviewed Jul 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow setting an HRN in invoice_requests built by `pay_for_offer` #3903

Allow setting an HRN in invoice_requests built by `pay_for_offer` #3903

TheBlueMatt commented Jul 1, 2025

Uh oh!

ldk-reviews-bot commented Jul 1, 2025 •

edited

Loading

Uh oh!

tnull left a comment •

edited

Loading

Uh oh!

tnull Jul 2, 2025

Uh oh!

TheBlueMatt Jul 2, 2025

Uh oh!

tnull Jul 2, 2025

Uh oh!

jkczyz Jul 2, 2025

Uh oh!

tnull Jul 2, 2025

Uh oh!

tnull Jul 2, 2025

Uh oh!

TheBlueMatt Jul 2, 2025

Uh oh!

jkczyz Jul 2, 2025

Uh oh!

tnull Jul 2, 2025

Uh oh!

jkczyz Jul 2, 2025

Uh oh!

jkczyz Jul 2, 2025

Uh oh!

jkczyz Jul 2, 2025

Uh oh!

jkczyz Jul 2, 2025

Uh oh!

Uh oh!

Allow setting an HRN in invoice_requests built by pay_for_offer #3903

Are you sure you want to change the base?

Allow setting an HRN in invoice_requests built by pay_for_offer #3903

Conversation

TheBlueMatt commented Jul 1, 2025

Uh oh!

ldk-reviews-bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Allow setting an HRN in invoice_requests built by `pay_for_offer` #3903

Allow setting an HRN in invoice_requests built by `pay_for_offer` #3903

ldk-reviews-bot commented Jul 1, 2025 •

edited

Loading

tnull left a comment •

edited

Loading