feat(nwc): implement real NIP-47 Nostr Wallet Connect client - #88
Merged
Conversation
Replace stub NWC client with real implementation using nostr-sdk's NIP-47 types and Client for relay communication: - Parse NWC URI via NostrWalletConnectURI::parse (replaces hand-rolled parser) - NwcClient connects to wallet relay, sends encrypted NIP-47 requests, and awaits Kind 23195 response events - get_info(): sends get_info request, populates wallet name from alias - get_balance(): sends get_balance request, converts mSAT to sats - pay_invoice(): sends pay_invoice request, returns preimage on success - make_invoice(): sends make_invoice request, returns bolt11 string - connect_wallet() now fetches initial balance after get_info - disconnect_wallet() cleanly disconnects the nostr-sdk Client - WASM gate: all relay-dependent code behind cfg(not(wasm32)); WASM targets get clear 'NWC not supported on web' errors - Add nip47 feature to nostr-sdk dependency in Cargo.toml - Unit tests for mSAT to sats conversion; live relay tests marked ignore
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 36 seconds. ⌛ 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 (6)
WalkthroughReplaces the stub NWC implementation with a nostr-sdk NIP-47–backed native NwcClient, adds the Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant API as NWC API Layer
participant Client as NwcClient
participant SDK as nostr-sdk
participant Relay as Relay Server
participant Wallet as Remote Wallet
App->>API: connect_wallet(nwc_uri)
API->>Client: NwcClient::new(uri_str)
Client->>SDK: parse URI & send signed NIP-47 request
SDK->>Relay: publish request event
Relay->>Wallet: deliver request
Wallet->>Relay: send response event
Relay->>SDK: receive response
SDK->>Client: decode response (msat→sat)
Client->>API: return NwcWalletInfo (+balance)
API->>App: return result
App->>API: pay_invoice(bolt11)
API->>Client: pay_invoice(bolt11)
Client->>SDK: build & send NIP-47 pay request
SDK->>Relay: publish pay event
Relay->>Wallet: deliver pay request
Wallet->>Relay: send payment response
Relay->>SDK: receive response
SDK->>Client: decode PaymentResult
Client->>API: return PaymentResult
API->>App: return status
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. 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: 2
🧹 Nitpick comments (2)
rust/src/nwc/client.rs (2)
203-217: Potential integer overflow on large amounts.
amount_sats * 1000could overflow onu64::MAX / 1000 ≈ 18.4 quadrillion sats(way beyond any real Lightning amount), but defensive coding would usesaturating_mulorchecked_mul:♻️ Optional: Use saturating multiplication
let request = Request::make_invoice(MakeInvoiceRequest { - amount: amount_sats * 1000, // convert sats → msats + amount: amount_sats.saturating_mul(1000), // convert sats → msats description, description_hash: None, expiry: None, });🤖 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 203 - 217, In make_invoice, multiplying amount_sats by 1000 can overflow; replace the direct multiplication of amount_sats * 1000 with a safe multiply (e.g., use amount_sats.checked_mul(1000) or amount_sats.saturating_mul(1000)), handle the Result/Option accordingly (return a concrete error via bail! if checked_mul returns None, or use the saturating result if preferred) and propagate the msat value into Request::make_invoice; update any variable names (e.g., msats) and error message to reflect an overflow/invalid amount.
316-327: Consider using#[tokio::test]for consistency.The
parse_rejects_invalid_uritest manually creates a tokio runtime while other tests use#[tokio::test]. This works but is inconsistent with the rest of the test module.♻️ Use #[tokio::test] for consistency
- /// URI parsing is delegated to nostr-sdk's `NostrWalletConnectURI::parse`. - #[test] + #[tokio::test] fn parse_rejects_invalid_uri() { #[cfg(not(target_arch = "wasm32"))] { - let result = tokio::runtime::Runtime::new() - .unwrap() - .block_on(NwcClient::new("not-a-valid-uri")); + let result = NwcClient::new("not-a-valid-uri").await; let err = result.err().expect("should fail for invalid URI"); assert!(err.to_string().contains("InvalidNwcUri")); } }🤖 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 316 - 327, The test parse_rejects_invalid_uri creates its own tokio runtime; change it to an async test using #[tokio::test] (optionally keep #[cfg(not(target_arch = "wasm32"))] if needed), remove the manual tokio::runtime::Runtime::new() call, await NwcClient::new("not-a-valid-uri").await, capture the Err (e.g., let err = result.err().expect(...)) and assert the error contains "InvalidNwcUri"; this uses the existing NwcClient::new symbol and makes the test consistent with the rest of the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/api/nwc.rs`:
- Around line 132-142: The code holds the read lock from wallet_store() across
an await and also performs a redundant status check; instead, acquire the guard,
get the client reference via guard.as_ref().ok_or_else(...)? and clone or
otherwise take ownership (e.g. let client =
guard.as_ref().ok_or_else(...)?.clone();), drop the guard so the read lock is
released, remove the explicit WalletStatus::Connected check, and then call
client.pay_invoice(&bolt11).await (NwcClient::pay_invoice already validates
status).
- Around line 117-123: The read guard in get_balance() holds the wallet_store()
RwLock across the await on client.get_balance(), which can block other ops like
connect_wallet; fix by extracting or cloning the client reference before
awaiting: inside get_balance() acquire the read guard, clone or clone the
Arc-wrapped NwcClient (or change storage to Arc<NwcClient> and clone that) into
a local variable, drop the guard, then call cloned_client.get_balance().await so
the RwLock is not held during network I/O; ensure NwcClient implements Clone or
that wallet_store stores Arc<NwcClient>.
---
Nitpick comments:
In `@rust/src/nwc/client.rs`:
- Around line 203-217: In make_invoice, multiplying amount_sats by 1000 can
overflow; replace the direct multiplication of amount_sats * 1000 with a safe
multiply (e.g., use amount_sats.checked_mul(1000) or
amount_sats.saturating_mul(1000)), handle the Result/Option accordingly (return
a concrete error via bail! if checked_mul returns None, or use the saturating
result if preferred) and propagate the msat value into Request::make_invoice;
update any variable names (e.g., msats) and error message to reflect an
overflow/invalid amount.
- Around line 316-327: The test parse_rejects_invalid_uri creates its own tokio
runtime; change it to an async test using #[tokio::test] (optionally keep
#[cfg(not(target_arch = "wasm32"))] if needed), remove the manual
tokio::runtime::Runtime::new() call, await
NwcClient::new("not-a-valid-uri").await, capture the Err (e.g., let err =
result.err().expect(...)) and assert the error contains "InvalidNwcUri"; this
uses the existing NwcClient::new symbol and makes the test consistent with the
rest of the module.
🪄 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: b7fb523a-e3a1-4bdd-8d7f-a7229d016963
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rust/Cargo.tomlrust/src/api/nwc.rsrust/src/nwc/client.rs
- Store NwcClient in Arc so RwLock is released before network I/O: get_balance() and pay_invoice() clone the Arc and drop the guard before awaiting; connect/disconnect also release locks before await - Remove redundant WalletStatus check in pay_invoice (NwcClient already validates connection status internally) - Guard sats-to-msats conversion in make_invoice with checked_mul to prevent overflow on large amounts - Convert parse_rejects_invalid_uri test to #[tokio::test] for consistency with other async tests in the module
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rust/src/nwc/client.rs (2)
116-125: Permissive handling of missingGetInforesponse.If
response.resultdoesn't matchResponseResult::GetInfo, the method still succeeds and marks the wallet asConnected. This is likely intentional for compatibility with wallets that may return unexpected response types, but it could mask issues during debugging.Consider adding a debug log when the response doesn't match the expected variant:
if let Some(ResponseResult::GetInfo(GetInfoResponse { alias, .. })) = response.result { self.info.wallet_name = alias; } else { // Optional: log for debugging // tracing::debug!("get_info response did not contain GetInfo result"); }🤖 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 116 - 125, The method currently treats any non-GetInfo ResponseResult as a success and sets self.info.status = WalletStatus::Connected, which can hide unexpected responses; update the handling around ResponseResult::GetInfo (the block that inspects response.result and sets self.info.wallet_name from GetInfoResponse) to add an else branch that emits a debug/tracing log (e.g., tracing::debug!) indicating the result did not contain GetInfo, while preserving the existing compatibility behavior (still set status/last_connected_at and return self.info.clone()); reference ResponseResult::GetInfo, GetInfoResponse, self.info, and WalletStatus::Connected when locating the change.
86-95: Consider clock skew resilience for thesincefilter.The filter uses
event.created_atfrom the request event to filter responses. If there's clock drift between the client and the relay/wallet service, legitimate responses could be filtered out.A small buffer (e.g., subtracting a few seconds from
since) could improve robustness:let filter = Filter::new() .kind(Kind::WalletConnectResponse) .author(self.uri.public_key) - .since(event.created_at); + .since(Timestamp::from(event.created_at.as_u64().saturating_sub(5)));This is a minor edge case, but worth considering for production reliability.
🤖 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 86 - 95, The filter currently uses event.created_at directly which can drop valid responses if clocks differ; adjust the timestamp passed to Filter::since by subtracting a small buffer (e.g., 5–10 seconds) from event.created_at before building the Filter in the code around Filter::new().kind(Kind::WalletConnectResponse).author(self.uri.public_key).since(event.created_at) (and keep the rest of the flow using client.fetch_events as-is); ensure the adjusted timestamp cannot underflow (handle zero/negative) so the since value remains valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rust/src/nwc/client.rs`:
- Around line 116-125: The method currently treats any non-GetInfo
ResponseResult as a success and sets self.info.status = WalletStatus::Connected,
which can hide unexpected responses; update the handling around
ResponseResult::GetInfo (the block that inspects response.result and sets
self.info.wallet_name from GetInfoResponse) to add an else branch that emits a
debug/tracing log (e.g., tracing::debug!) indicating the result did not contain
GetInfo, while preserving the existing compatibility behavior (still set
status/last_connected_at and return self.info.clone()); reference
ResponseResult::GetInfo, GetInfoResponse, self.info, and WalletStatus::Connected
when locating the change.
- Around line 86-95: The filter currently uses event.created_at directly which
can drop valid responses if clocks differ; adjust the timestamp passed to
Filter::since by subtracting a small buffer (e.g., 5–10 seconds) from
event.created_at before building the Filter in the code around
Filter::new().kind(Kind::WalletConnectResponse).author(self.uri.public_key).since(event.created_at)
(and keep the rest of the flow using client.fetch_events as-is); ensure the
adjusted timestamp cannot underflow (handle zero/negative) so the since value
remains valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7f8cf18-44c4-4a0e-8a30-189149d2119c
📒 Files selected for processing (2)
rust/src/api/nwc.rsrust/src/nwc/client.rs
fetch_events exits on EOSE (end of stored events) which misses the wallet's response — it arrives as a NEW event after EOSE. Fix by: 1. Subscribe to Kind 23195 response events via notifications channel BEFORE sending the request (eliminates race condition) 2. Send the Kind 23194 request event 3. Listen on the notification channel for the matching response This fixes 'Connection failed' errors when connecting real NWC wallets like Alby, where the response arrives after the relay's EOSE marker.
Two root causes for NWC connection failure with Alby: 1. connect() only spawns background tasks — the relay wasn't ready when send_event was called. Fix: call wait_for_connection(10s) after connect() to block until at least one relay is connected. 2. nostr-sdk's Response::from_event uses strict deserialization that rejects unknown NIP-47 methods (Alby returns 'get_budget' which isn't in the spec). Fix: implement lenient parsing — decrypt NIP-04 manually and parse the JSON with serde_json, extracting only the fields we need. GetInfoResponse also parsed leniently since its methods field has the same strict enum issue. Add live integration tests against Alby relay: - connect_to_alby_relay: verifies URI parsing and relay connection - get_info_from_alby: verifies get_info round-trip (wallet name) - get_balance_from_alby: verifies balance fetch (mSAT to sats)
connect_wallet_screen uses context.go() which replaces the nav stack, so the default AppBar back button never appears. Add an explicit leading back button that pops if possible, otherwise navigates to settings.
Save the NWC URI to SharedPreferences on connect, remove on disconnect. On app startup, if a saved URI exists, reconnect in the background. - NwcNotifier now takes SharedPreferences and persists the URI via kNwcUriKey on setConnected, clears it on setDisconnected - main.dart creates ProviderContainer explicitly, overrides nwcProvider with prefs, and calls _restoreNwcConnection before runApp - connect_wallet_screen passes the URI string to setConnected
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.
Replace stub NWC client with real implementation using nostr-sdk's NIP-47 types and Client for relay communication:
Summary by CodeRabbit
New Features
Improvements
Platform Support
Tests