-
Notifications
You must be signed in to change notification settings - Fork 409
Allow setting a payer_note
on pay_for_offer_from_human_readable_name
#3808
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
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @carlaKC as a reviewer! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm once @tnull's comments + request for a test are added 👍
pay_for_offer and pay_for_offer_from_human_readable_name take RouteParametersConfig and not max_total_routing_fee_msat.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3808 +/- ##
==========================================
+ Coverage 89.73% 89.78% +0.04%
==========================================
Files 159 159
Lines 128910 128993 +83
Branches 128910 128993 +83
==========================================
+ Hits 115682 115810 +128
+ Misses 10535 10485 -50
- Partials 2693 2698 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
.. as we'd otherwise never catch any bugs particular to the `dnssec` feature.
.. and link all instances for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
noted, thanks for the review! @tnull |
🔔 1st Reminder Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review. |
Closes #3780
Allows setting a
payer_note
onpay_for_offer_from_human_readable_name
and added a similarly named field to theAwaitingOffer
. When paying the offer, it will get thepayer_note
fromparams_for_payment_awaiting_offer
.