Expose full per-payment sending parameters#829
Expose full per-payment sending parameters#829benthecarman wants to merge 2 commits intolightningdevkit:mainfrom
Conversation
|
👋 I see @tankyleo was un-assigned. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
An alternative would be to expose the
Retrycompletely to allow for attempts based version as well. However, the timeout is generally simpler and more in line with what people expect.
If we do it, I'd lean towards exposing retry_strategy: Retry in the config. Should be even pretty easy to handle in bindings, as uniffi should now expose tuple enum variants.
benthecarman
force-pushed
the
config-retry-timeout
branch
from
5af55c9 to
4ddeb92
Compare
done |
benthecarman
changed the title
| /// See [`Retry`] for details. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| #[cfg_attr(feature = "uniffi", derive(uniffi::Enum))] | ||
| pub enum PaymentRetryStrategy { |
There was a problem hiding this comment.
Rather than duplicating the object here, can't we reuse LDK's Retry?
There was a problem hiding this comment.
No we can't re-export it to the bindings because of the Duration inside.
There was a problem hiding this comment.
Hmm, then it would be likely preferable to expose a remote type for Duration, too, rather than duplicating a lot of code?
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Would be nice if this were configurable per-payment rather than only globally, IMO.
Hmm, what would be the use case for having some payments diverge from the overall retry strategy? FWIW, I considered that before but decided somewhat that it's not worth complicating the API for, but happy to be convinced otherwise. |
Replace the `payment_retry_timeout_secs` field with a `PaymentRetryStrategy` enum that mirrors LDK's `Retry` type, allowing users to choose between timeout-based and attempt-count-based retry strategies. Defaults to a 10-second timeout to preserve existing behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
benthecarman
force-pushed
the
config-retry-timeout
branch
from
4ddeb92 to
9bb440f
Compare
Imo if we are gonna make it on per payment, would make sense to expose all of |
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
Replace per-payment `Option<RouteParametersConfig>` with dedicated `Bolt11SendingParameters` and `Bolt12SendingParameters` structs that expose the full LDK optional payment params (retry strategy, route parameters, custom TLVs, MPP override) on a per-payment basis. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
benthecarman
changed the title
|
Made it so we expose all the bolt 11/12 send params. It's about the same api complexity (a single optional struct) and will be useful for things like the mpp override that we need for orange |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Made it so we expose all the bolt 11/12 send params. It's about the same api complexity (a single optional struct) and will be useful for things like the mpp override that we need for orange
Hmm, I have to say I'm not loving this. In the past we spent quite a bit of time to clean up and align BOLT11 and BOLT12 APIs (also opened upstream PRs etc) around sending_parameters/RouteParametersConfig, so I'd prefer not to simply 'give up', expose everything under the sun and have the APIs diverge a lot just because AI tooling now makes it very easy to generate a bunch of additional structs. If we think we need additional parameters, can we please first at least try to iterate upstream to keep BOLT11 and BOLT12 interfaces aligned and maintain feature parity?
| /// See [`Retry`] for details. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| #[cfg_attr(feature = "uniffi", derive(uniffi::Enum))] | ||
| pub enum PaymentRetryStrategy { |
There was a problem hiding this comment.
Hmm, then it would be likely preferable to expose a remote type for Duration, too, rather than duplicating a lot of code?
4 participants
Previously only route parameters could be overridden on a per-payment basis. This PR makes the full set of LDK's optional payment parameters available to callers, allowing per-payment control over retry strategy, custom TLVs, and multi-wallet MPP splits in addition to route configuration. BOLT11 and BOLT12 get separate parameter types since they expose different fields. The node-wide retry strategy is also made configurable via Config.
This was something I needed when getting ldk-server working with loop. Also needed for orange for exposing the MPP override