Skip to content

Accept lightning-prefixed LNURLs - #2498

Draft
BullishNode wants to merge 2 commits into
developfrom
fix/develop-lightning-lnurl-prefix
Draft

Accept lightning-prefixed LNURLs#2498
BullishNode wants to merge 2 commits into
developfrom
fix/develop-lightning-lnurl-prefix

Conversation

@BullishNode

@BullishNode BullishNode commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Handle the lightning: scheme explicitly before the existing payment-request heuristics.
  • Delegate LNURL normalization and validation to Satoshifier.
  • Preserve the BOLT11 path for lightning:-prefixed invoices without first treating them as invalid LNURLs.
  • Pin the Bull SDK monorepo dependencies together to the companion parser fix.

The existing parser could extract some prefixed LNURLs through its broad embedded-LNURL heuristic. This change makes ownership explicit: Satoshifier normalizes LNURLs, while the mobile parser only classifies the payload and routes it to the correct parser.

Testing

  • TZ=UTC make checks
    • analyzer and formatting checks
    • 572 application tests
    • 17 bull_ui tests
    • bull_ui_catalogue test
  • Focused Linux native integration tests: 7/7 passed, covering BIP21 LNURL, BIP21 BOLT11, lowercase and uppercase lightning: schemes, prefixed LNURLs, prefixed Lightning Addresses, prefixed BOLT11 invoices, and encoded Lightning Address URLs.
  • Android production-debug APK built and installed successfully. The focused Android test was not run because the existing emulator data uses a newer database schema than develop; its data was deliberately left intact.

Dependency

Companion Bull SDK PR: SatoshiPortal/bull_sdk#15

Merge the SDK PR first, then repin this PR to the resulting durable SDK commit before merging it.

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown

Claude finished @BullishNode's task in 5m 1s —— View job


Code review — Accept lightning-prefixed LNURLs

  • Gather context (diff, related code, downstream consumers)
  • Analyze payment_request.dart parsing changes
  • Review test and dependency changes
  • Post findings

Note: I couldn't post these as inline review comments — this environment doesn't allow formal PR reviews or unapproved gh api calls — so findings are listed here with file/line references. I also couldn't inspect the companion bull_sdk commit (2140170) since fetching the SDK repo requires approvals I don't have; items depending on Satoshifier internals are flagged as questions.

No hard bugs found in the mobile-side changes. The main risks are the unmerged SDK dependency and two untested input shapes that currently rely on fallback heuristics.

Findings

1. Dependency pins point to an unmerged PR-branch commit — merge ordering risk (pubspec.yaml:50, pubspec.lock)
All four bull_sdk packages are pinned to 2140170ee..., which per the description lives on the open bull_sdk PR #15 branch. If that PR is squash-merged and its branch deleted, the pinned commit becomes unreachable from any ref and GitHub may eventually garbage-collect it, breaking flutter pub get on fresh clones and CI. Recommend merging bull_sdk#15 first and repinning to the resulting commit on the SDK's main branch before merging this PR. (The pinning itself is consistent — I verified all 14 ref occurrences across pubspec.yaml/pubspec.lock were updated and no stale 3dcd008d... remains.)

2. Uppercase LIGHTNING: prefix is matched locally but forwarded with original case to Satoshifier (lib/core/utils/payment_request.dart:82-87)
The prefix check is case-insensitive, but the LNURL branch passes the full original-case trimmed (prefix included) to satoshifier.LnurlParser.parse, while the BOLT11 branch passes the stripped lightningPayload. All-uppercase QR payloads (LIGHTNING:LNURL1...) are common because of QR alphanumeric-mode encoding, and the new test only covers a lowercase prefix with an uppercase payload. If the SDK's prefix stripping isn't case-insensitive, such input fails the explicit branch and only parses via the broad lnurl[0-9a-z]+ regex fallback at line 94 — the exact heuristic this PR is trying to make non-load-bearing. Same question for lightning:user@domain.com (email-style payload with prefix), which is routed to _tryParseLnAddress(trimmed) but has no test. Suggest either passing lightningPayload for symmetry with the BOLT11 branch, or adding tests for LIGHTNING:-uppercase and lightning:+lightning-address inputs. Fix this →

3. Parse-time LNURL validation removed — confirm intended for non-send entry points (lib/core/utils/payment_request.dart:300-311)
The old _tryParseLnAddress called boltz.Lnurl.validate(); the new code only asks Satoshifier to classify/normalize. For the send flow this is well mitigated — send_cubit.dart:539-545 validates by creating a dummy invoice before proceeding. But other callers (request_identifier_cubit.dart, the QR scanner pages) previously got upfront validation and will now redirect to Send for LNURLs that only fail later. If LnurlParser.parse at least verifies the bech32 checksum this is a reasonable UX trade-off; if it's purely pattern-based, a typo'd LNURL string now classifies as valid. Worth confirming what the SDK actually checks.

4. Test invoice replacement — a short comment would help (integration_test/payment_request_test.dart:20)
The BOLT11 constant changed from the well-known spec example to a real mainnet invoice, presumably because the updated decoder now requires fields the spec example lacks (e.g. a payment secret — the new invoice carries an sp5 tag). A one-line comment on why the spec example no longer decodes would prevent someone from "simplifying" it back. Also worth confirming the decoder doesn't reject expired invoices, so this test doesn't rot as the invoice ages.

5. Pre-existing, out of scope but adjacent: missing await defeats error handling (lib/features/send/request_identifier/request_identifier_cubit.dart:12 and :27)
PaymentRequest.parse(...) is async but called without await inside try/catch, so the catch never fires, parse failures become unhandled async errors, and redirect: toSend is emitted even for invalid input. Not introduced by this PR, but since this PR changes the parse contract it's a good follow-up candidate. Fix this →

What looks good

  • Moving the explicit lightning: branch ahead of the embedded-LNURL regex fixes a real misrouting hazard: bech32 invoice data can legitimately contain the substring lnurl, and previously a prefixed BOLT11 could be captured by the LNURL heuristic first.
  • The old replaceAll("lightning:", "").replaceAll("LIGHTNING:", "") was doubly buggy (removed all occurrences anywhere in the string, and missed mixed-case prefixes like Lightning:); substring('lightning:'.length) after a case-insensitive startsWith is correct.
  • The test harness change from Bull.init() to initLogs() + initFlutterRustBridgeDependencies() is a sensible minimal subset — it avoids the locator/Sentry/migration setup the parser test doesn't need.
  • Fallback behavior is preserved: if the explicit lightning: branch fails to parse, execution still falls through to the regex/email heuristics, so nothing that parsed before should hard-fail now.

@BullishNode
BullishNode marked this pull request as draft July 23, 2026 14:06
@BullishNode

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed as follows:

  1. The mobile PR is now draft. Bull SDK Unsigned PSBT download button not working #15 must merge first; this PR will then be repinned to the resulting durable SDK commit before it is made ready.
  2. Added native integration coverage for both LIGHTNING:LNURL1... and lightning:user@domain.com. Both pass. Satoshifier deliberately receives the complete URI and removes the scheme with an anchored, case-insensitive expression, keeping normalization in one place.
  3. Confirmed that LnurlParser.parse constructs boltz.Lnurl and awaits lnurl.validate() before returning, so parse-time LNURL validation was not removed.
  4. Documented why the BOLT11 vector differs and that parsing extracts, rather than enforces, its expiry timestamp.
  5. The missing await predates and is unrelated to this parser change, so it is intentionally excluded from this focused PR.

The expanded focused native suite passes 7/7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants