feat(bolt12): add BOLT12 offer payout support via LNDK#720
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (13)
WalkthroughAdds experimental BOLT12 offer payouts via an LNDK gRPC integration: proto schema, build change, new crates/config, offer classification and validation, LndkConnector client, payment routing and refactor, context/RPC wiring, startup init/feature checks, docs, and tests. ChangesBOLT12 Offer Payouts via LNDK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util.rs (1)
1244-1286:⚠️ Potential issue | 🔴 CriticalStore the buyer's original address in
buyer_invoice, not the resolved BIP-353 offer; re-resolve at payout time.The code resolves BIP-353 addresses to BOLT12 offers and stores the resolved offer in
order.buyer_invoice(seeorder.rs:96-100), then uses that stored offer directly at payout time indo_payment_bolt12(release.rs:476,do_payment_bolt12atrelease.rs:579) without re-resolution. This defeats BIP-353's offer rotation: if the issuer rotates or expires the offer between order creation and payout, the stored offer becomes stale and payment fails.Instead, preserve the original
user@domainaddress inbuyer_invoiceand callresolve_bip353again at payout time (indo_paymentbefore routing todo_payment_bolt12) to fetch the current offer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util.rs` around lines 1244 - 1286, The current logic in util.rs resolves BIP-353 addresses via crate::lightning::bip353::resolve_bip353 and then stores the resolved BOLT12 offer in order.buyer_invoice; instead, keep the original user@domain string in order.buyer_invoice (do not replace it with the resolved offer) and only use resolve_bip353 for validation (is_valid_invoice) or temporary checks here; then, remove usage of the stored offer in do_payment_bolt12 and instead call resolve_bip353 again from do_payment (before routing to do_payment_bolt12) to fetch the live BOLT12 offer at payout time so offer rotation/expiration is respected. Ensure resolve_bip353, is_valid_invoice, order.buyer_invoice, do_payment, and do_payment_bolt12 are the referenced symbols to locate changes.
🧹 Nitpick comments (15)
build.rs (1)
4-7: Optional: emitcargo:rerun-if-changedfor the proto inputs.Currently
build.rshas norerun-if-changeddirective for the.protofiles, so edits toproto/lndkrpc.proto(orproto/admin.proto) won't trigger a rebuild during local development. Adding directives keeps generated Tonic types in sync with the schema.♻️ Suggested addition
tonic_prost_build::configure() .protoc_arg("--experimental_allow_proto3_optional") .compile_protos(&["proto/admin.proto", "proto/lndkrpc.proto"], &["proto"]) .unwrap_or_else(|e| panic!("Failed to compile protos {:?}", e)); + + println!("cargo:rerun-if-changed=proto/admin.proto"); + println!("cargo:rerun-if-changed=proto/lndkrpc.proto");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.rs` around lines 4 - 7, Add cargo rebuild triggers for the proto inputs so Cargo reruns the build script when proto files change: in the build script where tonic_prost_build::configure() and .compile_protos(...) are invoked, emit println!("cargo:rerun-if-changed=proto/lndkrpc.proto") and println!("cargo:rerun-if-changed=proto/admin.proto") (optionally also for the proto directory) before calling compile_protos to ensure generated Tonic code is regenerated on proto edits.src/util.rs (1)
1251-1251: Minor: tighten the address heuristic.
pr.contains('@') && !pr.contains(' ')will also match degenerate inputs like"@","user@", or"@domain"and forward them toresolve_bip353. Resolution will fail and the value will fall through to LNURL — functional, but it adds a wasted DNS round-trip per malformed submission. A cheapLightningAddress::from_str(&pr).is_ok()(already used elsewhere inis_valid_invoice) or a tighter regex would short-circuit clearly invalid inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util.rs` at line 1251, The current heuristic `pr.contains('@') && !pr.contains(' ')` is too permissive and forwards degenerate inputs to resolve_bip353; replace it by attempting to parse as a LightningAddress first (e.g. call LightningAddress::from_str(&pr).is_ok()) or use the stricter validation used in is_valid_invoice so only well-formed addresses are sent to resolve_bip353; update the branch that sets `pr` to call LightningAddress::from_str(&pr).is_ok() (or an equivalent regex) before invoking resolve_bip353 to avoid unnecessary DNS lookups on invalid inputs.settings.tpl.toml (1)
28-30: Use neutral / non-mainnetexample paths in the template.The macaroon path defaults to
…/chain/bitcoin/mainnet/admin.macaroonwhile every other LND path in the template (lnd_cert_file,lnd_macaroon_file) uses Polar regtest paths. An operator who copies the template wholesale could end up pointing LNDK at mainnet credentials while LND itself is on regtest, mixing networks. Recommend using a clearly placeholder path or aligning with the existing Polarregtestconvention used above.♻️ Proposed alignment
-lndk_cert_file = "/home/user/.lndk/data/tls-cert.pem" +lndk_cert_file = "/home/user/.lndk/tls-cert.pem" # LND macaroon used by LNDK for payment authorization -lndk_macaroon_file = "/home/user/.lnd/data/chain/bitcoin/mainnet/admin.macaroon" +lndk_macaroon_file = "/home/user/.polar/networks/1/volumes/lnd/alice/data/chain/bitcoin/regtest/admin.macaroon"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@settings.tpl.toml` around lines 28 - 30, The template uses a mainnet macaroon path for lndk_macaroon_file which is inconsistent with the other Polar/regtest example paths and could cause accidental mainnet usage; update the lndk_macaroon_file value to a neutral placeholder or align it with the Polar regtest convention used by lnd_cert_file and lnd_macaroon_file (e.g., replace the /chain/bitcoin/mainnet/... path with a /chain/bitcoin/regtest/... or a clearly marked placeholder like /path/to/admin.macaroon) so lndk_cert_file, lndk_macaroon_file and lnd_macaroon_file consistently reference non-mainnet example locations.src/main.rs (1)
126-146: Optional: avoid the secondget_node_info()round-trip.
get_node_info()is already called at line 127 to populateLN_STATUS(whose response is moved intoLnStatus::from_get_info_response). Calling it again at line 146 just to readfeaturesis an extra gRPC round-trip on the boot path. You could either persist the originalGetInfoResponsefor one-shot reuse or extendLnStatuswith afeatures: HashMap<u32, Feature>field so the second call is unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 126 - 146, The code makes a second gRPC round-trip by calling ln_client.get_node_info() again to check features for LNDK; reuse the initial GetInfoResponse instead: modify LnStatus::from_get_info_response or LnStatus to retain the original GetInfoResponse (or a features map) when you create LN_STATUS from the first ln_client.get_node_info() call, then replace the second ln_client.get_node_info().await.ok() usage in the lndk_client branch with a lookup from LN_STATUS (or LnStatus.features) so the extra get_node_info() call is removed; update references to LnStatus and LN_STATUS accessors accordingly (keep LndkConnector::new_from_settings, lndk_client logic unchanged).src/app/context.rs (1)
42-105: LGTM!
Option<LndkConnector>is the right shape —Nonecleanly disables BOLT12 payouts at every consumer site (do_payment_bolt12already handles the runtime-Nonecase explicitly).Optional follow-up: when BOLT12 unit tests land, consider adding a
with_lndk(...)method onTestContextBuilderso handler tests can inject a mock connector instead of going through the always-Nonepath.Also applies to: 250-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/context.rs` around lines 42 - 105, Add a with_lndk(...) builder method to TestContextBuilder so tests can inject a mock LndkConnector instead of relying on the always-None path; implement TestContextBuilder::with_lndk(self, lndk: Option<LndkConnector>) -> Self (or accept LndkConnector and wrap in Some) to set the builder's lndk field and return Self, and update any test setup that constructs TestContextBuilder to allow passing a mock connector into AppContext::new via the builder.src/config/types.rs (1)
205-262: Optional: alignDefaultwith#[serde(default = "...")]helpers.
LightningSettingsderivesDefault, so direct Rust construction (LightningSettings::default()— used intest_settings()and elsewhere) yieldslndk_grpc_host = "",lndk_fetch_invoice_timeout = 0, andbip353_doh_resolver = "". TOML deserialization with missing fields, on the other hand, uses thedefault_*helpers and gets the documented values. Today this is benign becauselndk_enabled/bip353_enableddefault tofalse, but anyone toggling those flags in a hand-built settings struct will get a 0-second timeout silently.Consider implementing
Defaultmanually (asMostroSettingsandRpcSettingsalready do) so both paths agree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/types.rs` around lines 205 - 262, LightningSettings derives Default but that Default yields empty/zero values for fields that have serde default helpers (lndk_grpc_host, lndk_fetch_invoice_timeout, bip353_doh_resolver), causing mismatch between LightningSettings::default() and TOML deserialization; implement a manual impl Default for LightningSettings that constructs the struct with the same default values used by the serde helpers (call default_lndk_grpc_host(), default_lndk_fetch_timeout(), default_bip353_doh_resolver() for the respective fields) and sensible defaults for the remaining fields (empty strings, zeros, false as appropriate) so tests like test_settings() and any hand-built settings match deserialized defaults.src/lightning/bip353.rs (2)
95-122: Minor:parse_bitcoin_uridoesn't enforce thebitcoin:scheme.The parser only looks for
?and anlno=parameter, so it would happily extractlno1…from any TXT payload that happens to embed a query string (e.g. a strayhttps://…?lno=…). For BIP-353 the lookup name is targeted, so the blast radius is small, but a cheaptxt.starts_with("bitcoin:")(after dechunking) would make the contract explicit and reject malformed records earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightning/bip353.rs` around lines 95 - 122, The parse_bitcoin_uri function currently accepts any string with a query containing lno=; after dechunking/trim you should enforce the BIP-353 scheme by verifying the dechunked txt starts with "bitcoin:" (e.g., let txt = ...; if !txt.starts_with("bitcoin:") { return None; }) before splitting on '?', so malformed or unrelated query strings are rejected; keep the rest of the logic (query extraction and lno lookup) unchanged.
27-84: Doc/return-type mismatch:Erris never returned.The doc comment says "Only returns
Errfor truly malformed input", but every failure path in the function body returnsOk(None)— there is no path that constructs anErr. Either tighten the signature toOption<String>or drop theErrclause from the docstring (and decide which malformed-input cases should genuinely error out).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightning/bip353.rs` around lines 27 - 84, The function resolve_bip353 currently has signature pub async fn resolve_bip353(address: &str) -> Result<Option<String>, MostroError> but never returns Err, so change it to return Option<String> (pub async fn resolve_bip353(address: &str) -> Option<String>), update its body to return Some(...) / None as it already does, remove handling of MostroError, update any call sites to handle Option instead of Result, and update the doc comment to remove the "Err" clause; alternatively, if you prefer real error returns, add specific Err returns in resolve_bip353 for truly malformed input (e.g., when Settings::get_ln() or query_doh fails) and construct MostroError accordingly—pick one approach and make callers and the docstring consistent with resolve_bip353 and Settings::get_ln/query_doh usage.src/app/order.rs (1)
96-101: Minor: avoid double-clone oforder.The block clones
ordertwice — once to materialize anOrderforvalidate_invoice, and again to constructorder_with_resolved. You can fold these into a single clone by validating off the already-cloned value.♻️ Suggested tweak
- let resolved_invoice = validate_invoice(&msg, &Order::from(order.clone())).await?; - let mut order_with_resolved = order.clone(); - if resolved_invoice.is_some() { - order_with_resolved.buyer_invoice = resolved_invoice; - } - let order = &order_with_resolved; + let mut order_with_resolved = order.clone(); + let resolved_invoice = + validate_invoice(&msg, &Order::from(order_with_resolved.clone())).await?; + if resolved_invoice.is_some() { + order_with_resolved.buyer_invoice = resolved_invoice; + } + let order = &order_with_resolved;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/order.rs` around lines 96 - 101, Clone `order` only once by creating a single mutable clone (e.g., let mut order_with_resolved = order.clone()), then call validate_invoice(&msg, &Order::from(...)) using that cloned value (either by borrowing or moving into Order::from as the type permits) instead of cloning again, update order_with_resolved.buyer_invoice with the resolved invoice when present, and then use &order_with_resolved for the rest; adjust the call sites around validate_invoice, Order::from, and order_with_resolved to avoid the second clone.src/lightning/lndk.rs (3)
163-168: RedundantamountonPayInvoiceRequestfor an amount-bearing invoice.The fetched invoice already pins
amount_msats, andvalidate_fetched_invoice(lines 155–161) has just confirmed it matchesamount_msats. Sendingamount: Some(amount_msats)again is harmless today but duplicates the source of truth — if LNDK ever treats this field as an override (for amountless invoices) it could mask a desync. Setting it toNonehere would make the intent clearer and rely solely on the validated invoice contents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightning/lndk.rs` around lines 163 - 168, The PayInvoiceRequest currently sets amount: Some(amount_msats) even though validate_fetched_invoice already confirmed the fetched invoice (fetched.invoice_hex_str) contains the expected amount_msats; remove the redundant amount field by setting amount to None in the PayInvoiceRequest construction so the request relies solely on the validated invoice contents (update the PayInvoiceRequest used when building pay_req and ensure validate_fetched_invoice remains the source of truth).
82-87: HardcodedlocalhostSNI may fail for non-sidecar LNDK deployments.This works when LNDK runs on the same host as Mostro (the documented common case), but if an operator points
lndk_grpc_hostat a remote LNDK whose TLS cert was generated for a different SAN (e.g. its real hostname), the TLS handshake will fail with a confusing "invalid certificate: name not in cert" error. Consider deriving the SNI fromlndk_grpc_host(stripping scheme/port) and only falling back to"localhost", or exposing it as a setting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightning/lndk.rs` around lines 82 - 87, The TLS SNI is currently hardcoded to "localhost" which breaks remote LNDK deployments; change the ClientTlsConfig::domain_name call to derive the name from the configured lndk_grpc_host (parse/strip any scheme and port to get the hostname) and pass that hostname to domain_name, falling back to "localhost" only if parsing yields nothing or the host is empty; update the code around Certificate::from_pem(&cert) / ClientTlsConfig::new() to use the derived SNI or expose it as a config value.
107-114: The field namelndk_fee_limit_percentis misleading — it stores a fraction, not a percent.The field is documented in
src/config/types.rsas "Fee limit for BOLT12 payments as percent", but the code at line 112–114 treats it as a fraction (multiplying by 100.0 and clamping). The template examplelndk_fee_limit_percent = 0.2and fallback tomostro_settings.max_routing_fee(which is also a fraction) both confirm the actual units are fractional. An operator reading the config name or the field's doc comment would reasonably setlndk_fee_limit_percent = 2expecting "2%", but this would be interpreted as 200%. Rename tolndk_fee_limit_fraction(matchingmax_routing_feenaming), or accept a true percent and divide by 100 at the config layer. Also update the doc comment insrc/config/types.rs:236to say "fraction" instead of "percent".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightning/lndk.rs` around lines 107 - 114, The config field lndk_fee_limit_percent is misnamed — it stores a fractional value (like max_routing_fee) but is documented/ named as a percent; rename it to lndk_fee_limit_fraction in the config type and update its doc comment to say "fraction" (and update any example/template values to use fractional form), then update usages in lightning/lndk.rs (replace lndk_fee_limit_percent with lndk_fee_limit_fraction when building fee_fraction for fee_limit_percent) and any other places that reference the old symbol; to avoid breaking existing configs, add a serde alias/rename for backward compatibility (e.g. #[serde(alias = "lndk_fee_limit_percent")] or serde rename) on the new field.src/lightning/offers.rs (3)
99-132: Consider rejecting offers whose chain set excludes the active network.
validate_offerchecks denomination but notoffer.chains(). A buyer can submit a perfectly-parseable BOLT12 offer that only advertises e.g. signet while Mostro/LND/LNDK are on mainnet; that offer will pass validation here and fail later at fetch/pay time, leaving the order in a degraded state. A defensive check against the configured network's chain hash up front would surface the mismatch synchronously, with the sameInvoiceInvalidErrorsemantics already used for currency mismatches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightning/offers.rs` around lines 99 - 132, validate_offer currently skips checking offer.chains(), so add a defensive check after parsing the Offer (after Offer::from_str and before the amount match) that the parsed offer.chains() includes the active network's chain hash and return MostroInternalErr(ServiceError::InvoiceInvalidError) if it does not; obtain the active network chain hash from the same config/source used elsewhere in the module (or add an active_chain_hash parameter to validate_offer if no accessor exists) so a non-matching offer (e.g., signet vs mainnet) is rejected synchronously.
51-57: Redundant prefix checks:lntbsandlnbcrtare shadowed.
starts_with("lntb")already matcheslntbs…, andstarts_with("lnbc")already matcheslnbcrt…, so the explicitlntbs/lnbcrtarms are unreachable. All four currently map toBolt11so there is no functional bug, but the redundancy is misleading — readers may infer signet/regtest are being treated specially.♻️ Cleanup
- if lower.starts_with("lnbc") - || lower.starts_with("lntb") - || lower.starts_with("lntbs") - || lower.starts_with("lnbcrt") - { + if lower.starts_with("lnbc") || lower.starts_with("lntb") { return InvoiceFormat::Bolt11; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightning/offers.rs` around lines 51 - 57, The prefix checks in the block that returns InvoiceFormat::Bolt11 are redundant: remove the explicit starts_with("lntbs") and starts_with("lnbcrt") checks and keep only starts_with("lnbc") and starts_with("lntb") (the code that builds/uses the lower variable and returns InvoiceFormat::Bolt11 should remain), so behavior is unchanged but the unreachable arms are eliminated and readability improved; update any nearby comment if it suggests special-casing signet/regtest to avoid confusion.
144-151:Quantity::Bounded(n).get() < 1is unreachable code.The
lightningcrate'sQuantity::Boundedvariant wraps aNonZeroU64, son.get()is guaranteed to be ≥ 1. The check will never trigger and can be removed. Simplify the match arm to accept any bounded quantity:Suggested cleanup
match offer.supported_quantity() { - Quantity::One | Quantity::Unbounded => {} - Quantity::Bounded(n) => { - if n.get() < 1 { - return Err(MostroInternalErr(ServiceError::InvoiceInvalidError)); - } - } + Quantity::One | Quantity::Unbounded | Quantity::Bounded(_) => {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightning/offers.rs` around lines 144 - 151, The match on offer.supported_quantity() in offers.rs contains an unreachable check: inside the Quantity::Bounded(n) arm the code tests if n.get() < 1 and returns an error, but Quantity::Bounded holds a NonZeroU64 so n.get() is always ≥ 1; remove that conditional and simply accept the Bounded(n) arm (i.e., drop the n.get() < 1 check and the MostroInternalErr return) so the match becomes a no-op for One, Unbounded, and Bounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/LNDK_SETUP.md`:
- Line 13: Add explicit language specifiers to the two fenced code blocks that
currently have none: change the block containing the ASCII diagram "Buyer ----
Nostr ----> Mostro ----gRPC----> LNDK ----gRPC----> LND …" to start with
```text and change the block containing the configuration snippet beginning with
"[protocol]" to start with ```ini so both blocks satisfy markdownlint MD040.
In `@settings.tpl.toml`:
- Around line 21-46: The template added required-looking keys that will break
existing configs; update LightningSettings in src/config/types.rs to make the
new fields optional by adding #[serde(default)] to lndk_enabled, lndk_cert_file,
lndk_macaroon_file, bip353_enabled, and bip353_skip_dnssec (or alternatively
revert these entries in settings.tpl.toml to commented form like the
[anti_abuse_bond] pattern); ensure the struct field names exactly match those
keys and add sensible Rust defaults if needed, and document the chosen approach
in the changelog.
In `@src/lightning/bip353.rs`:
- Around line 27-84: The logs in resolve_bip353 currently emit the full BIP-353
address (user@domain); change all tracing calls to avoid logging the raw
local-part by only including the domain (domain) and either a short
deterministic hash or redact of the user (e.g. hash of user or
"[redacted_user]") instead of address. Update the tracing::warn/info/debug calls
that currently reference {address} (the DoH failure, DNS status, DNSSEC warning,
resolved bolt12, and no-offer debug messages) to include domain=%domain and
user_hash=%user_hash (or user="[redacted]") so diagnostics keep domain context
but do not expose the plain user identifier; compute user_hash from the local
variable user before logging.
In `@src/util.rs`:
- Around line 1251-1265: The logs in the BIP-353 resolution branch leak the full
user@domain payout address via tracing::info! and tracing::warn!; update the
logging in the block that calls
crate::lightning::bip353::resolve_bip353(&pr).await so it does not emit the raw
pr value—either redact the local-part (log only "@domain"), or log a stable
short hash (e.g., SHA-256 first 8 bytes) of pr, or lower the level to debug;
change the tracing::info! and tracing::warn! calls that reference {pr} to use
the chosen non-identifying representation instead while leaving the rest of the
control flow and returned value (offer or pr) unchanged.
---
Outside diff comments:
In `@src/util.rs`:
- Around line 1244-1286: The current logic in util.rs resolves BIP-353 addresses
via crate::lightning::bip353::resolve_bip353 and then stores the resolved BOLT12
offer in order.buyer_invoice; instead, keep the original user@domain string in
order.buyer_invoice (do not replace it with the resolved offer) and only use
resolve_bip353 for validation (is_valid_invoice) or temporary checks here; then,
remove usage of the stored offer in do_payment_bolt12 and instead call
resolve_bip353 again from do_payment (before routing to do_payment_bolt12) to
fetch the live BOLT12 offer at payout time so offer rotation/expiration is
respected. Ensure resolve_bip353, is_valid_invoice, order.buyer_invoice,
do_payment, and do_payment_bolt12 are the referenced symbols to locate changes.
---
Nitpick comments:
In `@build.rs`:
- Around line 4-7: Add cargo rebuild triggers for the proto inputs so Cargo
reruns the build script when proto files change: in the build script where
tonic_prost_build::configure() and .compile_protos(...) are invoked, emit
println!("cargo:rerun-if-changed=proto/lndkrpc.proto") and
println!("cargo:rerun-if-changed=proto/admin.proto") (optionally also for the
proto directory) before calling compile_protos to ensure generated Tonic code is
regenerated on proto edits.
In `@settings.tpl.toml`:
- Around line 28-30: The template uses a mainnet macaroon path for
lndk_macaroon_file which is inconsistent with the other Polar/regtest example
paths and could cause accidental mainnet usage; update the lndk_macaroon_file
value to a neutral placeholder or align it with the Polar regtest convention
used by lnd_cert_file and lnd_macaroon_file (e.g., replace the
/chain/bitcoin/mainnet/... path with a /chain/bitcoin/regtest/... or a clearly
marked placeholder like /path/to/admin.macaroon) so lndk_cert_file,
lndk_macaroon_file and lnd_macaroon_file consistently reference non-mainnet
example locations.
In `@src/app/context.rs`:
- Around line 42-105: Add a with_lndk(...) builder method to TestContextBuilder
so tests can inject a mock LndkConnector instead of relying on the always-None
path; implement TestContextBuilder::with_lndk(self, lndk: Option<LndkConnector>)
-> Self (or accept LndkConnector and wrap in Some) to set the builder's lndk
field and return Self, and update any test setup that constructs
TestContextBuilder to allow passing a mock connector into AppContext::new via
the builder.
In `@src/app/order.rs`:
- Around line 96-101: Clone `order` only once by creating a single mutable clone
(e.g., let mut order_with_resolved = order.clone()), then call
validate_invoice(&msg, &Order::from(...)) using that cloned value (either by
borrowing or moving into Order::from as the type permits) instead of cloning
again, update order_with_resolved.buyer_invoice with the resolved invoice when
present, and then use &order_with_resolved for the rest; adjust the call sites
around validate_invoice, Order::from, and order_with_resolved to avoid the
second clone.
In `@src/config/types.rs`:
- Around line 205-262: LightningSettings derives Default but that Default yields
empty/zero values for fields that have serde default helpers (lndk_grpc_host,
lndk_fetch_invoice_timeout, bip353_doh_resolver), causing mismatch between
LightningSettings::default() and TOML deserialization; implement a manual impl
Default for LightningSettings that constructs the struct with the same default
values used by the serde helpers (call default_lndk_grpc_host(),
default_lndk_fetch_timeout(), default_bip353_doh_resolver() for the respective
fields) and sensible defaults for the remaining fields (empty strings, zeros,
false as appropriate) so tests like test_settings() and any hand-built settings
match deserialized defaults.
In `@src/lightning/bip353.rs`:
- Around line 95-122: The parse_bitcoin_uri function currently accepts any
string with a query containing lno=; after dechunking/trim you should enforce
the BIP-353 scheme by verifying the dechunked txt starts with "bitcoin:" (e.g.,
let txt = ...; if !txt.starts_with("bitcoin:") { return None; }) before
splitting on '?', so malformed or unrelated query strings are rejected; keep the
rest of the logic (query extraction and lno lookup) unchanged.
- Around line 27-84: The function resolve_bip353 currently has signature pub
async fn resolve_bip353(address: &str) -> Result<Option<String>, MostroError>
but never returns Err, so change it to return Option<String> (pub async fn
resolve_bip353(address: &str) -> Option<String>), update its body to return
Some(...) / None as it already does, remove handling of MostroError, update any
call sites to handle Option instead of Result, and update the doc comment to
remove the "Err" clause; alternatively, if you prefer real error returns, add
specific Err returns in resolve_bip353 for truly malformed input (e.g., when
Settings::get_ln() or query_doh fails) and construct MostroError
accordingly—pick one approach and make callers and the docstring consistent with
resolve_bip353 and Settings::get_ln/query_doh usage.
In `@src/lightning/lndk.rs`:
- Around line 163-168: The PayInvoiceRequest currently sets amount:
Some(amount_msats) even though validate_fetched_invoice already confirmed the
fetched invoice (fetched.invoice_hex_str) contains the expected amount_msats;
remove the redundant amount field by setting amount to None in the
PayInvoiceRequest construction so the request relies solely on the validated
invoice contents (update the PayInvoiceRequest used when building pay_req and
ensure validate_fetched_invoice remains the source of truth).
- Around line 82-87: The TLS SNI is currently hardcoded to "localhost" which
breaks remote LNDK deployments; change the ClientTlsConfig::domain_name call to
derive the name from the configured lndk_grpc_host (parse/strip any scheme and
port to get the hostname) and pass that hostname to domain_name, falling back to
"localhost" only if parsing yields nothing or the host is empty; update the code
around Certificate::from_pem(&cert) / ClientTlsConfig::new() to use the derived
SNI or expose it as a config value.
- Around line 107-114: The config field lndk_fee_limit_percent is misnamed — it
stores a fractional value (like max_routing_fee) but is documented/ named as a
percent; rename it to lndk_fee_limit_fraction in the config type and update its
doc comment to say "fraction" (and update any example/template values to use
fractional form), then update usages in lightning/lndk.rs (replace
lndk_fee_limit_percent with lndk_fee_limit_fraction when building fee_fraction
for fee_limit_percent) and any other places that reference the old symbol; to
avoid breaking existing configs, add a serde alias/rename for backward
compatibility (e.g. #[serde(alias = "lndk_fee_limit_percent")] or serde rename)
on the new field.
In `@src/lightning/offers.rs`:
- Around line 99-132: validate_offer currently skips checking offer.chains(), so
add a defensive check after parsing the Offer (after Offer::from_str and before
the amount match) that the parsed offer.chains() includes the active network's
chain hash and return MostroInternalErr(ServiceError::InvoiceInvalidError) if it
does not; obtain the active network chain hash from the same config/source used
elsewhere in the module (or add an active_chain_hash parameter to validate_offer
if no accessor exists) so a non-matching offer (e.g., signet vs mainnet) is
rejected synchronously.
- Around line 51-57: The prefix checks in the block that returns
InvoiceFormat::Bolt11 are redundant: remove the explicit starts_with("lntbs")
and starts_with("lnbcrt") checks and keep only starts_with("lnbc") and
starts_with("lntb") (the code that builds/uses the lower variable and returns
InvoiceFormat::Bolt11 should remain), so behavior is unchanged but the
unreachable arms are eliminated and readability improved; update any nearby
comment if it suggests special-casing signet/regtest to avoid confusion.
- Around line 144-151: The match on offer.supported_quantity() in offers.rs
contains an unreachable check: inside the Quantity::Bounded(n) arm the code
tests if n.get() < 1 and returns an error, but Quantity::Bounded holds a
NonZeroU64 so n.get() is always ≥ 1; remove that conditional and simply accept
the Bounded(n) arm (i.e., drop the n.get() < 1 check and the MostroInternalErr
return) so the match becomes a no-op for One, Unbounded, and Bounded.
In `@src/main.rs`:
- Around line 126-146: The code makes a second gRPC round-trip by calling
ln_client.get_node_info() again to check features for LNDK; reuse the initial
GetInfoResponse instead: modify LnStatus::from_get_info_response or LnStatus to
retain the original GetInfoResponse (or a features map) when you create
LN_STATUS from the first ln_client.get_node_info() call, then replace the second
ln_client.get_node_info().await.ok() usage in the lndk_client branch with a
lookup from LN_STATUS (or LnStatus.features) so the extra get_node_info() call
is removed; update references to LnStatus and LN_STATUS accessors accordingly
(keep LndkConnector::new_from_settings, lndk_client logic unchanged).
In `@src/util.rs`:
- Line 1251: The current heuristic `pr.contains('@') && !pr.contains(' ')` is
too permissive and forwards degenerate inputs to resolve_bip353; replace it by
attempting to parse as a LightningAddress first (e.g. call
LightningAddress::from_str(&pr).is_ok()) or use the stricter validation used in
is_valid_invoice so only well-formed addresses are sent to resolve_bip353;
update the branch that sets `pr` to call LightningAddress::from_str(&pr).is_ok()
(or an equivalent regex) before invoking resolve_bip353 to avoid unnecessary DNS
lookups on invalid inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03f4e435-f884-4f07-bae3-2cf5d320eb9b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlbuild.rsdocs/LNDK_SETUP.mddocs/STARTUP_AND_CONFIG.mdproto/lndkrpc.protosettings.tpl.tomlsrc/app/context.rssrc/app/order.rssrc/app/release.rssrc/config/types.rssrc/config/wizard.rssrc/lightning/bip353.rssrc/lightning/invoice.rssrc/lightning/lndk.rssrc/lightning/mod.rssrc/lightning/offers.rssrc/main.rssrc/rpc/service.rssrc/util.rs
grunch
requested review from
AndreaDiazCorreia,
BraCR10,
Catrya,
arkanoider and
grunch
There was a problem hiding this comment.
Hi @QaidVoid Thanks for the work on this PR, it’s a great feature. Before moving forward, it should be split into two. This PR touches 20 files, we recommend keeping PRs limited to fewer files and focused on a single feature at a time.
It mixes BOLT12 (via LNDK) and BIP-353 (DoH resolver), which have different dependencies, risks, and operational implications. They shouldn’t be reviewed or shipped as a single unit.
BIP-353 depends on BOLT12, but not the other way around. It makes more sense to land BOLT12 first, and then in other PR add BIP-353.
|
@QaidVoid I also asked Claude Code (Opus 4.7) to do a deep review, and here’s its feedback, please take this along with CodeRabbit’s review into account when working on the other PRs. Happy to discuss any of these. Thanks again for the work, splitting and addressing the issues above will get this landed cleanly. Issues found in BOLT12 (PR 1 scope) Blocking
Serious
Minor
Issues to address in BIP-353 (PR 2) Please don't ship these as-is in any future PR; flagging now so they're on the radar:
|
QaidVoid
force-pushed
the
feat/bolt12-lndk
branch
from
0106b07 to
d37ac14
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@settings.tpl.toml`:
- Line 34: The sample setting lndk_macaroon_file uses a mainnet path which is
inconsistent with the surrounding regtest-oriented examples; update the
lndk_macaroon_file example value to a regtest-consistent path (e.g., replace any
".../chain/bitcoin/mainnet/..." with the regtest equivalent) so the template
aligns with other sample values and avoids copy/paste startup errors when using
regtest; locate the lndk_macaroon_file entry in settings.tpl.toml to make this
change.
In `@src/config/types.rs`:
- Around line 206-207: The struct-level #[serde(default)] on LightningSettings
is too broad and lets legacy required fields (lnd_cert_file, lnd_macaroon_file,
lnd_grpc_host) deserialize as empty strings; remove the #[serde(default)] from
the LightningSettings struct and instead add field-level #[serde(default)] only
to the new optional LNDK fields (identify them by name in the struct) so legacy
fields remain required during deserialization while new fields still get
defaults; update the LightningSettings definition to remove the top-level
attribute and annotate only the new optional fields with #[serde(default)].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2da84603-0836-4959-93ce-27954f53c4ac
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlbuild.rsdocs/LNDK_SETUP.mddocs/STARTUP_AND_CONFIG.mdproto/lndkrpc.protosettings.tpl.tomlsrc/app/context.rssrc/app/release.rssrc/config/types.rssrc/config/wizard.rssrc/lightning/invoice.rssrc/lightning/lndk.rssrc/lightning/mod.rssrc/lightning/offers.rssrc/main.rssrc/rpc/server.rssrc/rpc/service.rs
✅ Files skipped from review due to trivial changes (3)
- src/lightning/mod.rs
- docs/STARTUP_AND_CONFIG.md
- docs/LNDK_SETUP.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main.rs
- src/lightning/invoice.rs
- src/app/context.rs
- build.rs
- src/lightning/lndk.rs
- src/lightning/offers.rs
- src/app/release.rs
- proto/lndkrpc.proto
QaidVoid
force-pushed
the
feat/bolt12-lndk
branch
from
d37ac14 to
c807bd1
Compare
Accept BOLT12 offers (lno1...) as buyer payout destinations by routing through an optional LNDK daemon alongside LND. LNDK implements BOLT12 (invoice_request onion messaging, LDK offer parsing); LND continues to handle routing. Opt-in via lightning.lndk_enabled (default false). When disabled, BOLT12 offers are rejected at validation time. Setup requirements are in docs/LNDK_SETUP.md. The BOLT11 path is unchanged. BOLT12 lives in do_payment_bolt12 so LND's streaming payment API and LNDK's synchronous RPC don't share plumbing. Uses two-step GetInvoice -> validate -> PayInvoice rather than PayOffer, which does not verify amount or expiry.
QaidVoid
force-pushed
the
feat/bolt12-lndk
branch
from
c807bd1 to
55dc7e9
Compare
QaidVoid
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Hi @Catrya, thanks for the thorough review. I've pushed fixes addressing all the feedback. This PR is now focused only on BOLT12. Ready for another look when you have a moment. |
|
@QaidVoid thank you for your valuable contribution, this is a great new feature that Mostro users would appreciate. Unfortunately this PR is getting too big, we are already in production with mostros around the world and we can't afford to introduce a bug. When we need to implement features like this one we split it on smaller phases, you can see this example on closed issues on recently we are doing a big and sensitive multiple phases PR to add anti abuse bond (#731) The spec must indicate all phases and what do each one in detail and update the spec indicating when each one is completed. Every phase must be backwards compatible if possible, if not we should indicate it in the spec at the beginning to prepare a minor release in the future. If you have doubts let me know |
3 participants
Related Issue
Partial fix for #703. BIP-353 DNS resolution will follow as a separate PR.
Summary
Lets buyers submit a BOLT12 offer (
lno1…) as their payout destination. Mostro routes the payout through an optional LNDK daemon running alongside LND. Opt-in; default-off behavior is byte-identical to current Mostro.Motivation
BOLT12 closes UX gaps in BOLT11: offers are reusable (no per-retry invoice round-trip) and receiver identity is hidden via blinded paths. Mostro's escrow flow keeps using BOLT11 hold invoices (BOLT12 has no hold-invoice equivalent), so this PR only touches the buyer-payout step — the one place where the destination is chosen by an external party.
LNDK is the standard sidecar for BOLT12 with LND today: LDK implements offers, LND provides routing and onion-message forwarding, LNDK glues them together. Avoids forking LND or maintaining a parallel routing stack.
Architecture
do_paymentbecomes a thin dispatch over a newclassify()that routes BOLT12 offers todo_payment_bolt12(LNDK) and everything else to a renameddo_payment_lndwhose body is byte-for-byte identical to the priordo_payment.pay_offer_validated()does a defensiveGetInvoice → validate → PayInvoicebecause LNDK's one-shotPayOfferdoesn't verify the fetched invoice's amount or expiry. The LNDK client is threaded throughAppContextandAdminServiceImplso dispute resolution and admin settle/cancel can pay BOLT12 buyers as well.Defensive checks in
validate_offerreject non-BTC currency, mismatched fixed amounts, elapsed expiry,Quantity::Bounded(n > 1), zero-amount orders, and any offer received whilelndk_enabled = false. Wire-level fee tolerance is sent as absolute msats (PayInvoiceRequest.fee_limit), not LNDK's whole-percent field, so a smallmax_routing_feedoesn't get rounded up by ~10×.Configuration
All new fields live in the existing
[lightning]block. Oldersettings.tomlfiles parse unchanged via struct-level#[serde(default)]. Operator setup (LND build tags, custom-message flags, LNDK install, baked-macaroon recipe, troubleshooting) lives indocs/LNDK_SETUP.md.lndk_tls_domainlets remote LNDK deployments override the default localhost SNI;lndk_min_invoice_expiry(default 60s) keeps the freshness floor appropriate for short-lived BOLT12 invoices.Backward compatibility
lni1…) rejected at validation rather than failing later.settings.tomlfiles parse unchanged.Scope and limitations
Deferred to follow-ups: BIP-353 (separate PR), BOLT12 offer creation (dev-fee receipt still uses BOLT11), BOLT12-aware retries (today shares the BOLT11 retry loop), seller→Mostro escrow leg (BOLT12 has no hold-invoice equivalent).
Test plan
Automated
cargo test: 251 passed, 0 failed.cargo clippy --all-targets --all-features: clean.cargo fmt --check: clean.offers(classify shapes,validate_offerrejection cases includingQuantity::Bounded(n>1)and zero-amount orders,validate_fetched_invoiceamount / expiry / sanity bounds).Regtest topology
bitcoindregtest.lnd,lnd2) built withpeersrpc signrpc walletrpcand started with--protocol.custom-message=513 --protocol.custom-nodeann=39 --protocol.custom-init=39.:7000,:7001).lnd2 → lnd500k sats,cln → lnd500k sats,lnd2 ↔ clnpeered for the BOLT12 reply path.LNDK wire-format check
LNDK v0.3.0 was triggered with the exact wire format Mostro now sends — absolute msat
fee_limitandpayer_note: None. From the LNDK server log:LNDK parsed the request, looked up the introduction node, and emitted the onion message — the new
PayInvoiceRequest.fee_limit(msat) +payer_note: Noneencoding is wire-compatible with LNDK v0.3.0. mostrod boots cleanly against the same LNDK (LNDK connector initialized for BOLT12 offer payouts) with no panics, and resubscribes pending orders without errors.End-to-end Mostro flow on the same topology
buyer_invoice = lno1...issued by CLN. Hold-invoice flow runs unchanged. Onrelease, mostrod callsLNDK::GetInvoice → validate → PayInvoice. Afterlnd2was peered with CLN (so the BOLT12 reply path through it works), the retry loop converged withBOLT12 offer paid, preimage=0cad817d…. Channel-level proof:lnd → clnshifted by exactly 100,000 sats.lnbcrt500u…invoice on the seller LND.releasetriggersdo_payment_lnd. mostrod logsOrder…: Invoice with hash: b21a252e… paid!and transitions tosuccess. No BOLT12 path touched.Summary by CodeRabbit
New Features
Documentation