Allow setting a `payer_note` on `pay_for_offer_from_human_readable_name` #3808

elnosh · 2025-05-28T21:11:38Z

Allows setting a payer_note on pay_for_offer_from_human_readable_name and added a similarly named field to the AwaitingOffer. When paying the offer, it will get the payer_note from params_for_payment_awaiting_offer.

ldk-reviews-bot · 2025-05-28T21:11:41Z

👋 Thanks for assigning @carlaKC as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull

Mostly looks good to me. Could we add some test coverage in lightning-dns-resolver's end_to_end_test to ensure this is working as expected?

lightning/src/ln/channelmanager.rs

carlaKC

lgtm once @tnull's comments + request for a test are added 👍

codecov · 2025-05-29T16:46:53Z

Codecov Report

Attention: Patch coverage is 95.87629% with 4 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (c6caad8) to head (91cedd1).

Files with missing lines	Patch %	Lines
lightning-dns-resolver/src/lib.rs	96.62%	0 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3808      +/-   ##
==========================================
+ Coverage   89.89%   89.90%   +0.01%     
==========================================
  Files         160      160              
  Lines      129136   129223      +87     
  Branches   129136   129223      +87     
==========================================
+ Hits       116088   116183      +95     
+ Misses      10354    10349       -5     
+ Partials     2694     2691       -3

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull

LGTM

I took the liberty of pushing three additional commits to this PR branch that cleanup preexisting doc breakage/nits under the dnssec feature, and ensure we cargo test/check/doc lightning with dnssec in CI to make sure we catch such things going forward. (cc @TheBlueMatt)

tnull · 2025-05-30T09:17:15Z

lightning-dns-resolver/src/lib.rs

@@ -405,7 +406,86 @@ mod test {
 		let params = RouteParametersConfig::default();
 		nodes[0]
 			.node
-			.pay_for_offer_from_human_readable_name(name, amt, payment_id, retry, params, resolvers)
+			.pay_for_offer_from_human_readable_name(


Should be okay for now because kinda pre-existing, but going forward we'll probably want to DRY up that test code instead of copy/pasting the same flow.

+1, happy to review a followup DRYing up these tests

elnosh · 2025-05-30T13:24:26Z

noted, thanks for the review! @tnull

ldk-reviews-bot · 2025-05-31T00:00:30Z

🔔 1st Reminder

Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-02T00:00:22Z

🔔 1st Reminder

Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-02T00:01:44Z

🔔 2nd Reminder

Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

carlaKC

Nice change, good to have docs properly enforced too 👍

carlaKC · 2025-06-02T12:42:37Z

lightning-dns-resolver/src/lib.rs

@@ -405,7 +406,86 @@ mod test {
 		let params = RouteParametersConfig::default();
 		nodes[0]
 			.node
-			.pay_for_offer_from_human_readable_name(name, amt, payment_id, retry, params, resolvers)
+			.pay_for_offer_from_human_readable_name(


+1, happy to review a followup DRYing up these tests

lightning/src/ln/channelmanager.rs

tnull

This entire branch needs a rebase now though (i.e., don't forget to pull the additional commits first).

pay_for_offer and pay_for_offer_from_human_readable_name take RouteParametersConfig and not max_total_routing_fee_msat.

.. as we'd otherwise never catch any bugs particular to the `dnssec` feature.

.. and link all instances for consistency.

carlaKC

rebase LGTM!

ldk-reviews-bot requested a review from TheBlueMatt May 28, 2025 21:22

tnull self-requested a review May 29, 2025 08:00

tnull reviewed May 29, 2025

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

carlaKC reviewed May 29, 2025

View reviewed changes

elnosh force-pushed the payer-note-hrn branch from 194e6ab to 0f75233 Compare May 29, 2025 16:33

carlaKC self-requested a review May 29, 2025 17:07

tnull previously approved these changes May 30, 2025

View reviewed changes

carlaKC previously approved these changes Jun 2, 2025

View reviewed changes

tnull reviewed Jun 3, 2025

View reviewed changes

elnosh and others added 5 commits June 3, 2025 09:25

Allow to set payer_note on pay_for_offer_from_human_readable_name

29efc76

Update docs with use of RouteParametersConfig

ba837a1

pay_for_offer and pay_for_offer_from_human_readable_name take RouteParametersConfig and not max_total_routing_fee_msat.

Fix doc breakage under dnssec feature

1ca41b5

Ensure we check/test/doc the lightning crate under dnssec

05dbcb5

.. as we'd otherwise never catch any bugs particular to the `dnssec` feature.

Drop () from RouteParameters::default in docs

91cedd1

.. and link all instances for consistency.

elnosh dismissed stale reviews from carlaKC and tnull via 91cedd1 June 3, 2025 14:41

elnosh force-pushed the payer-note-hrn branch from b94f6f6 to 91cedd1 Compare June 3, 2025 14:41

carlaKC approved these changes Jun 3, 2025

View reviewed changes

Allow setting a payer_note on pay_for_offer_from_human_readable_name #3808

Are you sure you want to change the base?

Allow setting a payer_note on pay_for_offer_from_human_readable_name #3808

Conversation

elnosh commented May 28, 2025

Uh oh!

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

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

tnull May 30, 2025

Choose a reason for hiding this comment

Uh oh!

carlaKC Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

elnosh commented May 30, 2025

Uh oh!

ldk-reviews-bot commented May 31, 2025

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

carlaKC Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Allow setting a `payer_note` on `pay_for_offer_from_human_readable_name` #3808

Allow setting a `payer_note` on `pay_for_offer_from_human_readable_name` #3808

ldk-reviews-bot commented May 28, 2025 •

edited

Loading

codecov bot commented May 29, 2025 •

edited

Loading

tnull left a comment •

edited

Loading