Add `SendingParameters` struct for customizable payments #336

slanesuke · 2024-08-02T01:55:57Z

Based on #166 and #328

This PR introduces a PaymentParameters struct to allow users to override fields like max_total_cltv_expiry_delta, max_total_routing_fee_msat, and expiry_time. I'd appreciate suggestions on other useful fields to include as well.

I've added the optional PaymentParameters param to the send methods for both BOLT11 and Spontaneous payments. But, I need advice on whether we should be including it in the send methods for BOLT12 payments when paying for an offer. Specifically, I'm uncertain if values like max_total_cltv_expiry_delta or expiry_time can be overridden in this context.

tnull

Thanks for looking into this!

bindings/ldk_node.udl

src/payment/store.rs

tnull

Maybe we could also expose max_path_count and max_channel_saturation_power_of_half fields? Although, for the latter I wonder if we could come up with a bit more user-friendly format.

FWIW, I wonder the same for the max_total_routing_fee_msat (e.g., could allow to set it in percent)?

Moreover, should we allow to set a default/node-wide PaymentParameters (or whatever we want to call it) as part of Config?

joschisan · 2024-08-09T23:54:14Z

For fedimint just having max_total_routing_fee_msat as an absolute value would be the most user friendly.

tnull · 2024-08-12T09:53:36Z

For fedimint just having max_total_routing_fee_msat as an absolute value would be the most user friendly.

Thanks, good to know!

tnull · 2024-08-13T08:36:09Z

Ah, seems this needs a rebase to make CI pass (which we just fixed, excuse the inconvenience!)

slanesuke · 2024-08-13T12:45:06Z

Ah, seems this needs a rebase to make CI pass (which we just fixed, excuse the inconvenience!)

done!

slanesuke · 2024-08-13T12:45:18Z

rebased!

tnull · 2024-08-13T12:47:24Z

You'll also need to account for the new send argument in all the test cases (CLN, bindings tests, etc)l

tests/integration_tests_cln.rs

slanesuke · 2024-08-19T15:29:39Z

@tnull I could use some advice on how to go about overriding routing params in the bolt12 methods. In bolt11/spontaneous send methods I could access the route parameters so it was a bit easier. But when paying offers I can't find where/how to override the existing routing params. Any thoughts?

slanesuke · 2024-08-19T18:33:04Z

rebased

tnull · 2024-08-21T11:02:51Z

@tnull I could use some advice on how to go about overriding routing params in the bolt12 methods. In bolt11/spontaneous send methods I could access the route parameters so it was a bit easier. But when paying offers I can't find where/how to override the existing routing params. Any thoughts?

I think unfortunately LDK currently doesn't provide a way to override all of the fields, now opened the issue here: lightningdevkit/rust-lightning#3262

Let's keep this PR BOLT11/Spontaneous only and I'll open a tracking issue to make sure we won't forget to add SendingParameters to Bolt12Payment::send once the API becomes available.

tnull · 2024-08-21T11:06:34Z

I think CI fails as you haven't updated the Kotlin tests in bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt yet.

Could you also try to clean up the commit history a bit? Seems there are a few overlapping / back-and-forth commits going on.

slanesuke · 2024-08-21T13:14:11Z

@tnull I could use some advice on how to go about overriding routing params in the bolt12 methods. In bolt11/spontaneous send methods I could access the route parameters so it was a bit easier. But when paying offers I can't find where/how to override the existing routing params. Any thoughts?

I think unfortunately LDK currently doesn't provide a way to override all of the fields, now opened the issue here: lightningdevkit/rust-lightning#3262

Let's keep this PR BOLT11/Spontaneous only and I'll open a tracking issue to make sure we won't forget to add SendingParameters to Bolt12Payment::send once the API becomes available.

Okay, sounds good.

src/payment/spontaneous.rs

tnull · 2024-08-22T13:40:15Z

bindings/ldk_node.udl

Hmm, could we either split this in one commit per field, or add all fields at once? In any case this split of "initial" and "new" fields doesn't make too much sense, given that they are not new. :)

It seems that during the commit history cleanup yesterday, I accidentally included some code in the wrong commits. I’m still working on my skills with interactive rebasing 😅. But, I’ve restructured the commits so that they should make more sense now. I squashed all the fields into one commit and then implemented SendingParameters in each method in the following commits.

src/payment/mod.rs

src/config.rs

`SendingParameters` allows users to override opinionated values while routing a payment such as `max_total_routing_fees`, `max_path_count` `max_total_cltv_delta`, and `max_channel_saturation_power_of_half` Updated docs for `max_channel_saturation_power_of_half` for clarity.

Introduced `sending_parameters_config` to `Config` for node-wide routing and pathfinding configuration. Also, added default values for `SendingParameters` to ensure reasonable defaults when no custom settings are provided by the user.

Updated `Bolt11Payment` `send` method to accept `SendingParameters`, as a parameter. If the user provided sending params the default values are overridden.

Added the optional `SendingParameters` to `send_using_amount` in `Bolt11Payment`. If the user provides sending params the values will be overridden otherwise they'll use the default values.

Added optional SendingParameters to the send method in SpontaneousPayment. If the user provides sending params the values will be overridden otherwise they remain the same.

tnull

Cool, LGTM.

As I only have a few minor nits/comments I'm going to land this and solve them in a brief follow-up.

src/config.rs

bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt

src/payment/bolt11.rs

src/payment/spontaneous.rs

#336 followups

slanesuke force-pushed the 2024-07-introduce-PaymentParameters branch from 2729da1 to 63fc07d Compare August 5, 2024 16:21

tnull reviewed Aug 6, 2024

View reviewed changes

bindings/ldk_node.udl Outdated Show resolved Hide resolved

bindings/ldk_node.udl Outdated Show resolved Hide resolved

src/payment/store.rs Outdated Show resolved Hide resolved

src/payment/store.rs Outdated Show resolved Hide resolved

src/payment/store.rs Outdated Show resolved Hide resolved

tnull reviewed Aug 6, 2024

View reviewed changes

slanesuke changed the title ~~Add PaymentParameters struct for customizable payments~~ Add SendingParameters struct for customizable payments Aug 12, 2024

slanesuke force-pushed the 2024-07-introduce-PaymentParameters branch 2 times, most recently from 0179893 to 9242342 Compare August 12, 2024 20:13

slanesuke force-pushed the 2024-07-introduce-PaymentParameters branch from 9242342 to 7bfed68 Compare August 13, 2024 12:40

slanesuke closed this Aug 13, 2024

slanesuke reopened this Aug 13, 2024

tnull reviewed Aug 13, 2024

View reviewed changes

tests/integration_tests_cln.rs Outdated Show resolved Hide resolved

slanesuke force-pushed the 2024-07-introduce-PaymentParameters branch from 86a07a4 to 7a0d5a1 Compare August 13, 2024 13:16

slanesuke force-pushed the 2024-07-introduce-PaymentParameters branch from 7a0d5a1 to 75cf55c Compare August 19, 2024 18:32

tnull mentioned this pull request Aug 21, 2024

Allow to set SendingParameters in BOLT12 send methods #347

Open

slanesuke force-pushed the 2024-07-introduce-PaymentParameters branch 2 times, most recently from 72775c3 to 850b9d3 Compare August 21, 2024 19:32

tnull reviewed Aug 22, 2024

View reviewed changes

slanesuke force-pushed the 2024-07-introduce-PaymentParameters branch 3 times, most recently from 1fac6dd to 0b9f3cd Compare August 22, 2024 18:54

slanesuke added 5 commits August 26, 2024 18:52

Add default SendingParameters to node config

bf4ddff

Introduced `sending_parameters_config` to `Config` for node-wide routing and pathfinding configuration. Also, added default values for `SendingParameters` to ensure reasonable defaults when no custom settings are provided by the user.

Add SendingParameters to bolt11 send

d6e6ff7

Updated `Bolt11Payment` `send` method to accept `SendingParameters`, as a parameter. If the user provided sending params the default values are overridden.

Add SendingParameters to bolt11 send_using_amount

2b85ba9

Added the optional `SendingParameters` to `send_using_amount` in `Bolt11Payment`. If the user provides sending params the values will be overridden otherwise they'll use the default values.

Add SendingParameters to spontaneous send

4822336

Added optional SendingParameters to the send method in SpontaneousPayment. If the user provides sending params the values will be overridden otherwise they remain the same.

slanesuke force-pushed the 2024-07-introduce-PaymentParameters branch from 0b9f3cd to 4822336 Compare August 27, 2024 00:59

tnull approved these changes Aug 27, 2024

View reviewed changes

tnull merged commit 398ece5 into lightningdevkit:main Aug 27, 2024
11 of 13 checks passed

tnull mentioned this pull request Aug 27, 2024

#336 followups #351

Merged

tnull added a commit that referenced this pull request Aug 28, 2024

Merge pull request #351 from tnull/2024-08-336-followup

ab7cc70

#336 followups

tnull mentioned this pull request Sep 12, 2024

Allow to set max_total_routing_fee_msat #166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SendingParameters` struct for customizable payments #336

Add `SendingParameters` struct for customizable payments #336

slanesuke commented Aug 2, 2024 •

edited

Loading

tnull left a comment

tnull left a comment •

edited

Loading

joschisan commented Aug 9, 2024

tnull commented Aug 12, 2024

tnull commented Aug 13, 2024

slanesuke commented Aug 13, 2024

slanesuke commented Aug 13, 2024

tnull commented Aug 13, 2024

slanesuke commented Aug 19, 2024

slanesuke commented Aug 19, 2024

tnull commented Aug 21, 2024

tnull commented Aug 21, 2024 •

edited

Loading

slanesuke commented Aug 21, 2024

tnull Aug 22, 2024

slanesuke Aug 22, 2024

tnull left a comment

Add SendingParameters struct for customizable payments #336

Add SendingParameters struct for customizable payments #336

Conversation

slanesuke commented Aug 2, 2024 • edited Loading

tnull left a comment

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

joschisan commented Aug 9, 2024

tnull commented Aug 12, 2024

tnull commented Aug 13, 2024

slanesuke commented Aug 13, 2024

slanesuke commented Aug 13, 2024

tnull commented Aug 13, 2024

slanesuke commented Aug 19, 2024

slanesuke commented Aug 19, 2024

tnull commented Aug 21, 2024

tnull commented Aug 21, 2024 • edited Loading

slanesuke commented Aug 21, 2024

tnull Aug 22, 2024

Choose a reason for hiding this comment

slanesuke Aug 22, 2024

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Add `SendingParameters` struct for customizable payments #336

Add `SendingParameters` struct for customizable payments #336

slanesuke commented Aug 2, 2024 •

edited

Loading

tnull left a comment •

edited

Loading

tnull commented Aug 21, 2024 •

edited

Loading