LNURL-pay resolution in max_sendable#23
Open
amackillop wants to merge 4 commits into
Open
Conversation
ldk-node: 5616db4e -> 6d2502bd. The new rev brings in the bitcoin-payment-instructions bump transitively, so the direct pin moves to bdcef061 to match and dedupe (verify with `cargo tree -d | grep bitcoin-payment-instructions`). bitcoin-payment-instructions on bdcef061 pins reqwest 0.13, so mdkd's reqwest is bumped 0.12 -> 0.13 to share the type across the crate boundary. The `rustls-tls` feature was renamed to `rustls` in 0.13. Sharing the same reqwest version is what unlocks passing MdkClient's shared `reqwest::Client` into `HTTPHrnResolver::with_client(...)`. Before the bump, the two crates pinned incompatible reqwest majors and the resolver had to be built with `::new()`, which bypassed the SOCKS proxy. See the follow-up commits that route LNURL fetches through the shared client. The dev-dep ldk-node-lsp (rev e5fcce06) still pulls the older bpi rev (6796e87) transitively; that duplication only affects test builds and is left for a separate bump.
Pull the build-route-params / find-route / fold-into-estimate tail out of compute_estimate's FromRoute branch into a private helper. No behaviour change. The LNURL-pay branch added in the follow-up commit fetches a BOLT11 invoice asynchronously and then needs the exact same routing tail to turn the resulting PaymentParameters into an estimate. Lifting it here lets that branch reuse a single tested code path instead of duplicating the find_route + estimate_from_route sequence.
Wire an HrnResolver through compute_estimate so the LNURL-pay arm
fetches a concrete BOLT11 invoice from the callback before pricing.
The estimate is driven by find_route over a real path now, not a
flat percentage buffer.
Target amount is min(balance, max_value): the fee at the largest
amount we could plausibly send. If balance falls below min_value
the destination is unreachable; surface that as
MaxSendableError::BelowLnurlMin { min_msat, balance_msat } so consumers
can render a recipient-specific message ("minimum send is N
sats") instead of conflating it with a dust balance. LNURL fetch
or validation failures surface as a new
MaxSendableError::LnurlResolutionFailed variant.
compute_estimate becomes async; MdkClient::max_sendable likewise.
The sole caller (/getbalance) is already inside an async fn, so
the change is a single .await.
The resolver is built with `HTTPHrnResolver::with_client(self.http_client.clone())`
rather than `::new()` so LNURL callbacks go through the configured
SOCKS proxy. `MdkClient` now holds the shared `reqwest::Client`
that was previously passed only to `MdkApiClient`; pay_any.rs
should be switched over in a follow-up so it stops leaking IP
on LN-Address payments too.
HRNs are already resolved by PaymentInstructions::parse. BIP 353
names land here as whatever reusable methods the recipient
publishes in their BIP 321 URI (most commonly a BOLT12 offer);
LN-Address names land as LNURL-pay. No extra branch needed.
Existing dispatch tests are converted to #[tokio::test]. The
LNURL fetch path itself is exercised manually via /getbalance
against a real LN-Address endpoint; producing a signed BOLT11 for
an arbitrary amount in a unit test would need its own keying
harness, which isn't worth the surface area.
`HTTPHrnResolver::new()` builds its own `reqwest::Client` and ignores any proxy config, so LN-Address pays via /pay were reaching the LNURL callback host directly even when `socks_proxy` was set. /getbalance's max_sendable path was fixed in the previous commit; this is the matching fix for the actual send path. Expose `MdkClient::http_client()` and pass that shared client to `HTTPHrnResolver::with_client`. No new dependency; the client was already built in `MdkClient::new()` and handed to `MdkApiClient`. Possible only because the reqwest 0.13 bump aligned mdkd with bitcoin-payment-instructions; pre-bump the two crates pinned incompatible reqwest majors and `with_client` rejected ours.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What it does
HrnResolverthroughcompute_estimateand resolvesLNURL-pay destinations to a concrete BOLT11 invoice before pricing.
Target amount is
min(balance, max_value): the fee at the largestamount we could plausibly send.
reqwest 0.12 → 0.13 bump aligns mdkd's reqwest with the bpi rev's,
which is what makes
HTTPHrnResolver::with_client(...)acceptMdkClient's sharedreqwest::Client./payLN-Address sends.pay_anywasbuilding
HTTPHrnResolver::new(), which constructs a freshreqwest client that ignores
socks_proxy. Bothmax_sendableandpay_anynow reuse the shared client.Error domain
MaxSendableErrorgains two variants for the API layer:BelowLnurlMin { min_msat, balance_msat }— distinct from a dustbalance. The destination has a hard floor we can't meet, and the
UI should say so rather than show "0 sat sendable".
LnurlResolutionFailed(String)— transport or validation failurefrom the LNURL callback. Surfaced rather than buffer-falling-back
so intermittent issues don't masquerade as a successful estimate.
Commits
Four atomic commits, each passes
just check:estimate_via_payment_paramshelper (pure refactor)max_sendablepay_anyLNURL fetches through the shared HTTP clientOut of scope
/getbalanceaccepting adest=query param. Today the daemoncalls
max_sendable(None); the destination-aware path isreachable only from inside the library.
hits the callback. Revisit if telemetry shows it matters.
ldk-node-lsp, which still pulls the olderbpi rev transitively. Test-build dup only.