feat(phase14): NWC wallet integration — connect, settings, auto-pa - #65
Merged
Conversation
…rade flows - rust/src/nwc/client.rs: NwcUri parser + NwcClient (get_info, get_balance, pay_invoice) - rust/src/api/nwc.rs: connect_wallet, disconnect_wallet, get_wallet, get_balance, pay_invoice + WalletStatusStream - rust/src/api/types.rs: NwcWalletInfo, PaymentResult types - lib/features/settings/providers/nwc_provider.dart: Riverpod NwcNotifier + isWalletConnectedProvider - lib/features/settings/screens/connect_wallet_screen.dart: NWC URI input + QR scan + Connect button - lib/features/settings/screens/wallet_settings_screen.dart: connected info card + disconnect - Wire /connect_wallet and /wallet_settings routes away from _Stub - add_lightning_invoice_screen: show NwcInvoiceWidget when wallet connected; _manualMode fallback - pay_lightning_invoice_screen: show NwcPaymentWidget when wallet connected; _manualMode fallback
… parsing, validation - NwcUri: replace #[derive(Debug)] with manual impl that redacts secret_hex - urlencoding_decode: preserve consumed chars on malformed %XX sequence - connect_wallet_screen: parse relay URLs from NWC URI instead of hardcoded stub - connect_wallet_screen: strengthen _isValid to check 64-char hex pubkey after prefix - connect_wallet_screen: replace raw $e SnackBar with generic message + debugPrint - AddLightningInvoiceScreen: accept optional amountSats param; gate NWC path on sats > 0
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds Nostr Wallet Connect (NWC) end-to-end: Rust NWC client, API types and global store with broadcast stream; Dart Riverpod providers for wallet state; Connect Wallet and Wallet Settings screens; route wiring; and conditional NWC auto-pay integration with manual fallback in invoice screens. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Flutter as Flutter UI
participant Riverpod as Riverpod Provider
participant RustAPI as Rust API Layer
participant NwcClient as NWC Client
User->>Flutter: Open Connect Wallet screen / Scan or paste URI
Flutter->>Flutter: Validate URI
User->>Flutter: Tap "Connect"
Flutter->>RustAPI: connect_wallet(nwc_uri)
RustAPI->>NwcClient: parse(uri) & get_info()
NwcClient-->>RustAPI: NwcWalletInfo
RustAPI->>RustAPI: store client & broadcast status
RustAPI-->>Flutter: success
Flutter->>Riverpod: nwcProvider.notifier.setConnected(info)
Riverpod-->>Flutter: isWalletConnected = true
Flutter->>Flutter: Navigate to Wallet Settings
sequenceDiagram
participant User as User
participant Invoice as Invoice Screen
participant Riverpod as isWalletConnectedProvider
participant NwcWidget as NWC Widget
participant ManualUI as Manual Invoice UI
participant RustAPI as Rust API Layer
User->>Invoice: Open invoice/payment screen
Invoice->>Riverpod: watch connection status
alt Wallet connected & amount known
Riverpod-->>Invoice: true
Invoice->>NwcWidget: render auto-pay widget
NwcWidget->>RustAPI: request/generate invoice or pay
alt NWC success
RustAPI-->>NwcWidget: success
NwcWidget->>Invoice: onConfirmed/onPaymentSuccess
Invoice->>Invoice: submit/complete
else NWC failure
NwcWidget->>Invoice: onFallbackToManual
Invoice->>Invoice: set _manualMode = true
Invoice->>ManualUI: render manual flow
end
else Wallet not connected
Invoice->>ManualUI: render manual flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
rust/src/nwc/client.rs (2)
175-192: Duplicate empty-string validation.The
bolt11.trim().is_empty()check is also performed inrust/src/api/nwc.rs:114-116. While defense-in-depth is fine, consider whether both layers need this check or if one should be authoritative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/nwc/client.rs` around lines 175 - 192, The bolt11 empty-string validation is duplicated: remove the redundant check from the pay_invoice method in rust/src/nwc/client.rs and let the API layer (rust/src/api/nwc.rs) remain authoritative; specifically, delete the bolt11.trim().is_empty() guard and its bail!() in the pay_invoice function so the method only performs wallet-status validation and the NotImplemented response, ensuring input validation is centralized in the API layer.
92-121: Consider usingpercent-encodingcrate for robustness and UTF-8 correctness.The custom
urlencoding_decodehandles basic cases, but line 104'schar::from_u32(h1 * 16 + h2)treats each percent-encoded byte as a separate character. For multi-byte UTF-8 sequences (e.g.,%C3%A9foré), this produces incorrect results.The
percent-encodingcrate handles UTF-8 sequences correctly and is the standard for this task. While relay URLs are ASCII-only in practice (RFC 3986), using an established crate improves clarity and prevents issues if this function is ever repurposed.♻️ Alternative using established crate
+use percent_encoding::percent_decode_str; + -fn urlencoding_decode(s: &str) -> String { - let mut out = String::with_capacity(s.len()); - let mut chars = s.chars().peekable(); - while let Some(c) = chars.next() { - if c == '%' { - // ... implementation ... - } else { - out.push(c); - } - } - out -} +fn urlencoding_decode(s: &str) -> String { + percent_decode_str(s).decode_utf8_lossy().into_owned() +}Add to
Cargo.toml:percent-encoding = "2.3"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/nwc/client.rs` around lines 92 - 121, Replace the custom byte-by-byte decoder in urlencoding_decode with the well-tested percent-encoding crate: add percent-encoding = "2.3" to Cargo.toml, import percent_encoding::percent_decode_str, and have urlencoding_decode return percent_decode_str(s).decode_utf8_lossy().into_owned() (or decode_utf8() with proper error handling) so percent-encoded multi-byte UTF-8 sequences (e.g. %C3%A9) are decoded correctly and robustly.rust/src/api/nwc.rs (2)
57-75: Silent wallet replacement when already connected.If a wallet is already connected,
connect_walletsilently replaces it without disconnecting first. Consider whether this is intentional behavior or if you should:
- Return an error if already connected, requiring explicit disconnect first
- Explicitly disconnect the old wallet before connecting the new one (for cleanup)
Current behavior may be fine for the stub, but when NIP-47 is wired, you may need cleanup logic for the previous connection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/nwc.rs` around lines 57 - 75, connect_wallet currently overwrites an existing connection in wallet_store without cleanup; update it to handle an existing client before replacing: when you obtain the write guard on store.client (the same spot around store.client.write().await in connect_wallet), check if guard.is_some() and then either return an error (explicitly Err for "AlreadyConnected") or call the old client's cleanup/disconnect routine (e.g., NwcClient::disconnect or the appropriate shutdown/close method) and await it before replacing the value, then proceed to set Some(new_client) and notify; ensure you propagate errors from the disconnect/cleanup step if they occur.
102-108: Read lock held across.await— safe for stubs, revisit for Phase 15+.Both
get_balance()andpay_invoice()hold theRwLockread guard while awaiting the client methods. Currently the client methods are effectively synchronous stubs, so this is safe. However, when NIP-47 relay I/O is wired in Phase 15+:
- Holding a
tokio::sync::RwLockacross.awaitis allowed but can cause contention if the await takes time.- Consider cloning necessary data from the client and releasing the lock before the async operation.
This is a heads-up for future implementation rather than a blocking issue now.
Also applies to: 113-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/nwc.rs` around lines 102 - 108, The read RwLock guard from wallet_store().client is held across an .await in get_balance() (and pay_invoice()), which is OK for current stubs but will cause contention once client methods perform I/O; fix by cloning or extracting the minimal handle needed from the guarded client (e.g., clone the inner client or required data from the Option returned by wallet_store().client.read().await), drop the guard immediately, then call client.get_balance().await (and similarly for pay_invoice()) so no RwLock read guard is held during the async operation.lib/features/settings/screens/wallet_settings_screen.dart (1)
147-153: Consider showing all relay URLs or indicating count.Currently only the first relay URL is displayed (
wallet.relayUrls.firstOrNull). If the NWC URI contains multiple relays, users won't see them. Consider showing all relays or indicating "+N more".♻️ Example: show relay count
// Relays _InfoRow( label: 'Relay', - value: wallet.relayUrls.firstOrNull ?? '—', + value: wallet.relayUrls.isEmpty + ? '—' + : wallet.relayUrls.length == 1 + ? wallet.relayUrls.first + : '${wallet.relayUrls.first} (+${wallet.relayUrls.length - 1})', colors: colors, theme: theme, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/wallet_settings_screen.dart` around lines 147 - 153, The UI currently shows only the first relay via wallet.relayUrls.firstOrNull in _InfoRow, which hides additional relays; update the display logic in wallet_settings_screen.dart to surface all relays or indicate there are more by: compute a display string from wallet.relayUrls (e.g., empty -> '—', single -> the URL, multiple -> either join all URLs or show first + " (+N more)" where N = length - 1) and pass that string into the existing _InfoRow value parameter so users can see all relays or the count of hidden relays.lib/features/settings/screens/connect_wallet_screen.dart (2)
182-189: Paste button silently ignores invalid clipboard content.When clipboard doesn't contain a valid NWC URI, nothing happens and the user gets no feedback. Consider showing a brief message.
♻️ Optional: add user feedback on invalid paste
TextButton.icon( onPressed: () async { final data = await Clipboard.getData(Clipboard.kTextPlain); final text = data?.text ?? ''; if (text.startsWith('nostr+walletconnect://')) { _uriController.text = text; setState(() {}); + } else if (text.isNotEmpty) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('Clipboard does not contain a valid NWC URI'), + duration: Duration(seconds: 2), + ), + ); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/connect_wallet_screen.dart` around lines 182 - 189, The paste handler currently only sets _uriController.text and calls setState() when the clipboard text startsWith('nostr+walletconnect://') and silently does nothing otherwise; update the onPressed closure in connect_wallet_screen.dart to handle the else branch by showing brief user feedback (e.g., via ScaffoldMessenger.of(context).showSnackBar or similar) when the clipboard content is not a valid NWC URI, while preserving the existing behavior of assigning to _uriController and calling setState() for valid URIs; ensure you reference the same Clipboard.getData(...) result and use the current BuildContext so the Snackbar displays correctly.
56-67: Manual URI parsing could be simplified withUri.parse().The current manual string splitting works but is fragile. Using Dart's
Uriclass would handle edge cases (URL encoding, multiple?characters, etc.) more robustly.♻️ Suggested improvement using Uri
// Stub: store minimal wallet state from the parsed URI. final uri = _uriController.text.trim(); - final rest = uri.replaceFirst('nostr+walletconnect://', ''); - final parts = rest.split('?'); - final pubkey = parts.first; - final query = parts.length > 1 ? parts[1] : ''; - final relayUrls = query - .split('&') - .where((p) => p.startsWith('relay=')) - .map((p) => Uri.decodeComponent(p.substring('relay='.length))) - .where((r) => r.startsWith('wss://') || r.startsWith('ws://')) - .toList(); + // Parse as URI (replace custom scheme for Uri.parse compatibility) + final parsed = Uri.parse(uri.replaceFirst('nostr+walletconnect://', 'https://')); + final pubkey = parsed.host.isNotEmpty ? parsed.host : parsed.path.split('/').first; + final relayUrls = parsed.queryParametersAll['relay'] + ?.where((r) => r.startsWith('wss://') || r.startsWith('ws://')) + .toList() ?? + [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/connect_wallet_screen.dart` around lines 56 - 67, Replace the manual string-splitting with Dart's Uri parsing: read the raw input from _uriController.text.trim(), call Uri.parse(...) to get a parsedUri, derive pubkey from parsedUri.authority (fallback to parsedUri.path or parsedUri.pathSegments.join('/') if authority is empty), and build relayUrls from parsedUri.queryParametersAll['relay'] (or an empty list) then filter for entries starting with 'wss://' or 'ws://'; update references to pubkey and relayUrls accordingly and remove the existing replaceFirst/split logic.lib/features/settings/providers/nwc_provider.dart (2)
7-19: Consider addingcopyWithand makingrelayUrlsimmutable.
NwcWalletStateis a data class that benefits from thecopyWithpattern for cleaner state updates. Additionally,relayUrlsas a mutableList<String>can be modified externally after construction.♻️ Suggested improvement
class NwcWalletState { const NwcWalletState({ required this.walletPubkey, required this.relayUrls, this.walletName, this.balanceSats, - }); + }) : relayUrls = relayUrls is List<String> ? List.unmodifiable(relayUrls) : relayUrls; final String walletPubkey; final List<String> relayUrls; final String? walletName; final int? balanceSats; + + NwcWalletState copyWith({ + String? walletPubkey, + List<String>? relayUrls, + String? walletName, + int? balanceSats, + }) { + return NwcWalletState( + walletPubkey: walletPubkey ?? this.walletPubkey, + relayUrls: relayUrls ?? this.relayUrls, + walletName: walletName ?? this.walletName, + balanceSats: balanceSats ?? this.balanceSats, + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/providers/nwc_provider.dart` around lines 7 - 19, NwcWalletState exposes a mutable List relayUrls and lacks a copyWith helper; make relayUrls immutable by storing List.unmodifiable(relayUrls) (or wrapping with UnmodifiableListView) in the constructor and expose it as List<String>, and add a copyWith({String? walletPubkey, List<String>? relayUrls, String? walletName, int? balanceSats}) on NwcWalletState that returns a new instance using existing values when params are null and ensures the new relayUrls are also stored as an unmodifiable list; update any places that construct NwcWalletState to keep behavior unchanged.
33-42:updateBalancemethod is defined but never invoked.Per the context snippet from
wallet_settings_screen.dart(lines 127-129), the balance display reads directly fromwallet.balanceSatswhich is only set during initial connection. SinceupdateBalance()is never called, the displayed balance will be stale.This appears intentional for the current stub implementation, but when the Rust bridge is wired, ensure
get_balance()calls triggerupdateBalance().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/providers/nwc_provider.dart` around lines 33 - 42, The updateBalance method on the provider (updateBalance) is never invoked so wallet balance stays stale; wire the Rust bridge balance fetch (get_balance) to call provider.updateBalance(int sats) when a new balance is received (or null-safe convert), and ensure the NwcWalletState.balanceSats is updated via updateBalance rather than only on initial connect; locate the bridge callback or async function that calls get_balance and invoke updateBalance with the fetched sats (or schedule it on the provider instance managing NwcWalletState) so UI reads the fresh value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/settings/screens/connect_wallet_screen.dart`:
- Around line 34-45: The hex validation in the _isValid getter rejects uppercase
hex; update the logic in _isValid (using _uriController, prefix, afterPrefix) to
accept both cases by either normalizing the extracted afterPrefix to lowercase
before validation (call toLowerCase() on the substring before splitting) or
expand the character check to include ASCII ranges for 'A'-'F' (65-70) in the
codeUnits condition so uppercase A-F pass; keep the 64-length check unchanged.
---
Nitpick comments:
In `@lib/features/settings/providers/nwc_provider.dart`:
- Around line 7-19: NwcWalletState exposes a mutable List relayUrls and lacks a
copyWith helper; make relayUrls immutable by storing
List.unmodifiable(relayUrls) (or wrapping with UnmodifiableListView) in the
constructor and expose it as List<String>, and add a copyWith({String?
walletPubkey, List<String>? relayUrls, String? walletName, int? balanceSats}) on
NwcWalletState that returns a new instance using existing values when params are
null and ensures the new relayUrls are also stored as an unmodifiable list;
update any places that construct NwcWalletState to keep behavior unchanged.
- Around line 33-42: The updateBalance method on the provider (updateBalance) is
never invoked so wallet balance stays stale; wire the Rust bridge balance fetch
(get_balance) to call provider.updateBalance(int sats) when a new balance is
received (or null-safe convert), and ensure the NwcWalletState.balanceSats is
updated via updateBalance rather than only on initial connect; locate the bridge
callback or async function that calls get_balance and invoke updateBalance with
the fetched sats (or schedule it on the provider instance managing
NwcWalletState) so UI reads the fresh value.
In `@lib/features/settings/screens/connect_wallet_screen.dart`:
- Around line 182-189: The paste handler currently only sets _uriController.text
and calls setState() when the clipboard text
startsWith('nostr+walletconnect://') and silently does nothing otherwise; update
the onPressed closure in connect_wallet_screen.dart to handle the else branch by
showing brief user feedback (e.g., via
ScaffoldMessenger.of(context).showSnackBar or similar) when the clipboard
content is not a valid NWC URI, while preserving the existing behavior of
assigning to _uriController and calling setState() for valid URIs; ensure you
reference the same Clipboard.getData(...) result and use the current
BuildContext so the Snackbar displays correctly.
- Around line 56-67: Replace the manual string-splitting with Dart's Uri
parsing: read the raw input from _uriController.text.trim(), call Uri.parse(...)
to get a parsedUri, derive pubkey from parsedUri.authority (fallback to
parsedUri.path or parsedUri.pathSegments.join('/') if authority is empty), and
build relayUrls from parsedUri.queryParametersAll['relay'] (or an empty list)
then filter for entries starting with 'wss://' or 'ws://'; update references to
pubkey and relayUrls accordingly and remove the existing replaceFirst/split
logic.
In `@lib/features/settings/screens/wallet_settings_screen.dart`:
- Around line 147-153: The UI currently shows only the first relay via
wallet.relayUrls.firstOrNull in _InfoRow, which hides additional relays; update
the display logic in wallet_settings_screen.dart to surface all relays or
indicate there are more by: compute a display string from wallet.relayUrls
(e.g., empty -> '—', single -> the URL, multiple -> either join all URLs or show
first + " (+N more)" where N = length - 1) and pass that string into the
existing _InfoRow value parameter so users can see all relays or the count of
hidden relays.
In `@rust/src/api/nwc.rs`:
- Around line 57-75: connect_wallet currently overwrites an existing connection
in wallet_store without cleanup; update it to handle an existing client before
replacing: when you obtain the write guard on store.client (the same spot around
store.client.write().await in connect_wallet), check if guard.is_some() and then
either return an error (explicitly Err for "AlreadyConnected") or call the old
client's cleanup/disconnect routine (e.g., NwcClient::disconnect or the
appropriate shutdown/close method) and await it before replacing the value, then
proceed to set Some(new_client) and notify; ensure you propagate errors from the
disconnect/cleanup step if they occur.
- Around line 102-108: The read RwLock guard from wallet_store().client is held
across an .await in get_balance() (and pay_invoice()), which is OK for current
stubs but will cause contention once client methods perform I/O; fix by cloning
or extracting the minimal handle needed from the guarded client (e.g., clone the
inner client or required data from the Option returned by
wallet_store().client.read().await), drop the guard immediately, then call
client.get_balance().await (and similarly for pay_invoice()) so no RwLock read
guard is held during the async operation.
In `@rust/src/nwc/client.rs`:
- Around line 175-192: The bolt11 empty-string validation is duplicated: remove
the redundant check from the pay_invoice method in rust/src/nwc/client.rs and
let the API layer (rust/src/api/nwc.rs) remain authoritative; specifically,
delete the bolt11.trim().is_empty() guard and its bail!() in the pay_invoice
function so the method only performs wallet-status validation and the
NotImplemented response, ensuring input validation is centralized in the API
layer.
- Around line 92-121: Replace the custom byte-by-byte decoder in
urlencoding_decode with the well-tested percent-encoding crate: add
percent-encoding = "2.3" to Cargo.toml, import
percent_encoding::percent_decode_str, and have urlencoding_decode return
percent_decode_str(s).decode_utf8_lossy().into_owned() (or decode_utf8() with
proper error handling) so percent-encoded multi-byte UTF-8 sequences (e.g.
%C3%A9) are decoded correctly and robustly.
🪄 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: 629c1157-7703-43e3-969a-96770ce9610c
📒 Files selected for processing (12)
lib/core/app_routes.dartlib/features/order/screens/add_lightning_invoice_screen.dartlib/features/order/screens/pay_lightning_invoice_screen.dartlib/features/settings/providers/nwc_provider.dartlib/features/settings/screens/connect_wallet_screen.dartlib/features/settings/screens/wallet_settings_screen.dartrust/src/api/mod.rsrust/src/api/nwc.rsrust/src/api/types.rsrust/src/nwc/client.rsrust/src/nwc/mod.rsspecs/004-mostro-p2p-client/tasks.md
…lay, validation - client.rs: remove duplicate bolt11 empty-check (API layer is authoritative) - api/nwc.rs: drop RwLock read guard before awaiting get_balance/pay_invoice - api/nwc.rs: notify disconnect before replacing existing connection in connect_wallet - nwc_provider.dart: store relayUrls as List.unmodifiable; add copyWith helper - nwc_provider.dart: updateBalance uses copyWith instead of manual reconstruction - connect_wallet_screen: accept uppercase hex pubkeys in _isValid (toLowerCase) - connect_wallet_screen: replace manual URI splitting with Uri.parse - connect_wallet_screen: show SnackBar when clipboard has no valid NWC URI - wallet_settings_screen: show all relays (first + N more) instead of firstOrNull
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/features/settings/screens/connect_wallet_screen.dart (1)
49-84: Consider handling empty relay URLs as a validation error.The
_connectmethod accepts URIs even when no valid relay URLs are present after filtering. An NWC connection without relays would be non-functional once the Rust bridge is implemented.♻️ Proposed validation enhancement
final relayUrls = (parsed.queryParametersAll['relay'] ?? const []) .where((r) => r.startsWith('wss://') || r.startsWith('ws://')) .toList(); + if (relayUrls.isEmpty) { + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar( + content: Text('NWC URI must contain at least one relay URL.'), + ), + ); + return; + } + if (!mounted) return; ref.read(nwcProvider.notifier).setConnected(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/connect_wallet_screen.dart` around lines 49 - 84, In _connect, after parsing the URI and computing relayUrls (variable relayUrls in the _connect method), validate that relayUrls.isNotEmpty and treat empty relays as a failure: if empty, show a SnackBar (similar to the existing catch block) with a clear validation message, reset _connecting (via setState) and return early instead of calling ref.read(nwcProvider.notifier).setConnected or navigating to AppRoute.walletSettings; this prevents storing a non-functional NWC state when no valid relay URLs are present.rust/src/api/nwc.rs (1)
137-143: Inconsistent error handling betweenget_balanceandpay_invoice.When the wallet status is not
Connected,get_balancereturnsErr(line 118), butpay_invoicereturnsOk(PaymentResult { success: false, ... }). This forces callers to handle two different error paths for the same condition.The docstring also declares
NoWalletConnectedas an error, but this branch returns it insideOk().For consistency with
get_balanceand clearer caller semantics, consider returningErrhere as well.♻️ Proposed fix for consistent error handling
if status != WalletStatus::Connected { - return Ok(PaymentResult { - success: false, - preimage: None, - error: Some("NoWalletConnected: wallet is not connected".into()), - }); + bail!("NoWalletConnected: wallet is not connected"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/nwc.rs` around lines 137 - 143, The branch in pay_invoice that checks WalletStatus::Connected currently returns Ok(PaymentResult { success: false, ... }) causing inconsistent error handling with get_balance; change pay_invoice to return Err with the same error variant used by get_balance (e.g., NoWalletConnected) when status != WalletStatus::Connected so callers receive a consistent Result::Err for a disconnected wallet; update the function's signature/error type handling around pay_invoice and any callers if necessary to propagate the Err variant instead of embedding the error in PaymentResult.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/features/settings/screens/connect_wallet_screen.dart`:
- Around line 49-84: In _connect, after parsing the URI and computing relayUrls
(variable relayUrls in the _connect method), validate that relayUrls.isNotEmpty
and treat empty relays as a failure: if empty, show a SnackBar (similar to the
existing catch block) with a clear validation message, reset _connecting (via
setState) and return early instead of calling
ref.read(nwcProvider.notifier).setConnected or navigating to
AppRoute.walletSettings; this prevents storing a non-functional NWC state when
no valid relay URLs are present.
In `@rust/src/api/nwc.rs`:
- Around line 137-143: The branch in pay_invoice that checks
WalletStatus::Connected currently returns Ok(PaymentResult { success: false, ...
}) causing inconsistent error handling with get_balance; change pay_invoice to
return Err with the same error variant used by get_balance (e.g.,
NoWalletConnected) when status != WalletStatus::Connected so callers receive a
consistent Result::Err for a disconnected wallet; update the function's
signature/error type handling around pay_invoice and any callers if necessary to
propagate the Err variant instead of embedding the error in PaymentResult.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ef724f7-3ede-46fa-aba4-88dbb230a1be
📒 Files selected for processing (5)
lib/features/settings/providers/nwc_provider.dartlib/features/settings/screens/connect_wallet_screen.dartlib/features/settings/screens/wallet_settings_screen.dartrust/src/api/nwc.rsrust/src/nwc/client.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/nwc/client.rs
…ror consistency - connect_wallet_screen: guard against empty relayUrls after URI parse; show SnackBar and return early instead of storing non-functional state - api/nwc.rs: pay_invoice returns Err (bail!) for disconnected wallet, consistent with get_balance
This was referenced Mar 31, 2026
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.
Summary by CodeRabbit
New Features
Chores