feat(ln): verify LNURL-pay before saving ln_address and AddInvoice - Phase 2#60
Conversation
- Add util/ln_address.rs: resolve lnurlp URL, GET metadata, require tag payRequest - spawn_verify_and_save_ln_address_task: async verify then persist settings - Settings confirm and y-shortcut: show verifying toast, save only after success - execute_add_invoice: same check for Lightning address input before DM - Remove unused sync save_ln_address_to_settings - Docs: STARTUP_AND_CONFIG, TUI, SETTINGS_ANALYSIS, MESSAGE_FLOW, buy/sell flows, docs README Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR adds Lightning address (LNURL-pay) verification to Mostrix. Users can now save a Lightning address in settings with LNURL-pay metadata reachability validation before persistence, and the ChangesLightning Address Verification Feature
Sequence DiagramsequenceDiagram
participant User as User (TUI)
participant UI as Settings UI
participant AsyncTask as Async Verify Task
participant LNURL as LNURL Endpoint
participant Disk as Settings File
participant Modal as OperationResult Modal
User->>UI: Enter Lightning address (user@domain.com)
UI->>UI: validate_ln_address_format()
UI->>User: Show confirmation prompt
User->>UI: Confirm (YES)
UI->>AsyncTask: spawn_verify_and_save_ln_address_task()
UI->>Modal: Display "Verifying Lightning address..."
AsyncTask->>LNURL: ln_address_pay_request_reachable() <br/> (fetch metadata JSON)
alt LNURL Valid (tag: "payRequest")
LNURL-->>AsyncTask: Metadata OK
AsyncTask->>Disk: Load settings.toml
AsyncTask->>Disk: Update ln_address field
AsyncTask->>Disk: Persist to disk
AsyncTask-->>Modal: OperationResult::Info("Saved")
Modal->>User: Show success message
else LNURL Failed
LNURL-->>AsyncTask: HTTP error / JSON parse error / tag mismatch
AsyncTask-->>Modal: OperationResult::Error("Lightning address not verified: {e}")
Modal->>User: Show error (settings NOT updated)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 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)
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. Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 48 seconds.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/key_handler/enter_handlers.rs (1)
773-781: ⚖️ Poor tradeoff
order_result_txreused for a settings-domain task — consider a dedicated channel.
spawn_verify_and_save_ln_address_taskpassesctx.order_result_txto report a Lightning-address verification result, butorder_result_txis semantically scoped to order/dispute operation results. TheOperationResultvariants it sends will be processed by the main-loop'shandle_operation_result, which may have order-specific side effects for certain variants.Other settings-adjacent tasks already have dedicated channels (
key_rotation_tx,mostro_info_tx,seed_words_tx); introducing aln_address_result_tx(or a purpose-specificOperationResult::LnAddressVerifiedvariant with a matching handler) would keep channel semantics clean and avoid future cross-flow coupling.Based on learnings: "Do not reuse order_result_tx for flows unrelated to its semantic purpose (order/dispute operation results). This guidance applies across Rust files in the codebase to ensure consistent channel usage and avoid cross-flow coupling." (PR
#31,src/ui/key_handler/enter_handlers.rs)🤖 Prompt for 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. In `@src/ui/key_handler/enter_handlers.rs` around lines 773 - 781, Replace the reused ctx.order_result_tx with a purpose-specific channel: add a new ln_address_result_tx to the context and change spawn_verify_and_save_ln_address_task to accept that channel instead of ctx.order_result_tx; send a dedicated result type (either a new OperationResult::LnAddressVerified variant or a small enum specific to ln_address_result_tx) from the spawned task, and update the main-loop handler (handle_operation_result or add a new handler for ln_address_result_tx) to process the Lightning-address verification results; update the call site in enter_handlers.rs (where create_key_input_state and UiMode::OperationResult are set) to clone and pass ctx.ln_address_result_tx rather than ctx.order_result_tx so settings-domain flows are kept separate from order/dispute flows.
🤖 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.
Nitpick comments:
In `@src/ui/key_handler/enter_handlers.rs`:
- Around line 773-781: Replace the reused ctx.order_result_tx with a
purpose-specific channel: add a new ln_address_result_tx to the context and
change spawn_verify_and_save_ln_address_task to accept that channel instead of
ctx.order_result_tx; send a dedicated result type (either a new
OperationResult::LnAddressVerified variant or a small enum specific to
ln_address_result_tx) from the spawned task, and update the main-loop handler
(handle_operation_result or add a new handler for ln_address_result_tx) to
process the Lightning-address verification results; update the call site in
enter_handlers.rs (where create_key_input_state and UiMode::OperationResult are
set) to clone and pass ctx.ln_address_result_tx rather than ctx.order_result_tx
so settings-domain flows are kept separate from order/dispute flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 526189d8-9cd3-4817-a244-d461f31f8453
📒 Files selected for processing (16)
docs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/README.mddocs/SETTINGS_ANALYSIS.mddocs/STARTUP_AND_CONFIG.mddocs/TUI_INTERFACE.mddocs/buy order flow.mddocs/sell order flow.mdsrc/ui/draw.rssrc/ui/help_popup.rssrc/ui/key_handler/async_tasks.rssrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/settings.rssrc/util/ln_address.rssrc/util/mod.rssrc/util/order_utils/execute_add_invoice.rs
There was a problem hiding this comment.
I reviewed this one carefully and I do not see a blocker in the current diff.
What I checked:
- the new LNURL verification helper and its
tag == payRequestguard - the async settings flow for verifying an address before persisting it
- the updated AddInvoice path, to ensure Lightning addresses are verified before the DM is sent
- confirm / Esc /
yshortcut behavior around the new async verification step - docs and UI copy, to make sure they now describe verification instead of plain format validation
The implementation looks coherent to me:
- format validation still happens first in the UI
- persistence moved behind async LNURL verification, which is the right place for a network-dependent check
execute_add_invoicenow rejects Lightning addresses whose LNURL-pay endpoint cannot be verified- clear-address flow stayed simple and synchronous, which makes sense
Checks are green too, so this looks good from my side. Nice step forward, this closes the biggest hole left by phase 1.
Summary by CodeRabbit
New Features
Documentation