Merged
Conversation
Implement the core NWC (Nostr Wallet Connect / NIP-47) client library: - nwc_connection.dart: Parse nostr+walletconnect:// URIs with validation - nwc_models.dart: Request/response models for all NIP-47 commands - nwc_client.dart: Core client with NIP-44 encryption via existing deps - nwc_exceptions.dart: Typed exceptions mapping all NWC error codes - Unit tests: 41 tests covering URI parsing, model serialization, errors - Documentation: docs/nwc-phase1-implementation.md No existing files modified. Uses dart_nostr and nip44 packages already in the project. Refs #456
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements Phase 1 of Nostr Wallet Connect (NWC/NIP-47) support for Mostro Mobile. Adds a complete NWC client library with URI parsing, request/response models, encrypted communication via NIP-44, relay connection management, and comprehensive test coverage plus documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Mostro Mobile<br/>(App)
participant Client as NwcClient
participant NostrService
participant Relay as Relay
participant Wallet as Wallet Service
App->>Client: payInvoice(params)
Client->>Client: Generate ephemeral keypair
Client->>Client: Encrypt payload (NIP-44)
Client->>Client: Create kind 23194 event, sign
Client->>NostrService: Send request event
NostrService->>Relay: Publish kind 23194
Relay->>Wallet: Event received
Client->>NostrService: Subscribe to kind 23195<br/>(response events)
Wallet->>Relay: Publish kind 23195<br/>(encrypted response)
Relay->>Client: Response event
Client->>Client: Validate event signature<br/>Decrypt payload (NIP-44)
Client->>Client: Parse & type-cast response
Client->>Client: Cleanup subscription
Client->>App: PayInvoiceResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 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: 4
🤖 Fix all issues with AI agents
In `@docs/nwc-phase1-implementation.md`:
- Around line 17-23: Add a language specifier to the fenced code block that
contains the directory tree starting with "lib/services/nwc/": change the
opening triple-backticks to include a language like text or plaintext (e.g.,
```text) so markdownlint MD040 is satisfied; update the fenced block in
docs/nwc-phase1-implementation.md that lists nwc_connection.dart,
nwc_models.dart, nwc_client.dart, and nwc_exceptions.dart accordingly.
In `@lib/services/nwc/nwc_client.dart`:
- Around line 86-93: disconnect() currently only cancels subscriptions and flips
_isConnected; also explicitly close and remove any Nostr relay/websocket
connections opened in connect(). Locate the code that opens relays (e.g., in
connect() where Nostr or RelayPool/relays are instantiated) and in disconnect()
call the appropriate relay close/disconnect method(s) (for example
Relay.close(), RelayPool.close(), or Nostr.instance.closeRelays()) and clear
those relay references, in addition to cancelling _subscriptions and clearing
state, so no websocket connections remain.
- Around line 247-274: The subscription cleanup is skipped on timeout or errors;
move the cancellation and removal of the subscription into a finally block so it
always runs: ensure the code that cancels _subscriptions[subId] and calls
_nostr.services.relays.closeEventsSubscription(subId) (the cleanup currently at
lines referencing subId and _subscriptions) is executed in a finally after
awaiting completer.future.timeout (and before any rethrow of
NwcException/NwcTimeoutException) so subscriptions are always cancelled
regardless of exceptions thrown from completer.future.timeout, encryption/event
creation, or the catch that rethrows NwcException.
- Around line 32-34: The _sendRequest() path in NwcClient leaks subscriptions
because the cleanup that removes entries from the _subscriptions map only runs
on the successful await path; wrap the entire request/response flow inside
_sendRequest() (the block that sets up the subscription, waits for
response/timeout, and notifies listeners) in a try-finally so that removal from
_subscriptions and cancellation of the subscription always happens in the
finally clause, ensuring cleanup on timeout or exceptions; also remove the
internal final Nostr _nostr = Nostr.instance field and refactor the class to
accept and use a NostrService dependency (inject NostrService into the NwcClient
constructor and replace direct uses of _nostr/Nostr.instance with the injected
service) so all Nostr interactions go through NostrService.
🧹 Nitpick comments (3)
lib/services/nwc/nwc_connection.dart (2)
52-63:Uri.hostlowercases the pubkey — verify this is acceptable.Dart's
Uri.parsenormalizes the host component to lowercase per RFC 3986. If a NWC URI contains uppercase hex characters in the pubkey (e.g.,...://A1B2C3D4...),uri.hostwill silently convert it toa1b2c3d4.... In practice Nostr pubkeys are lowercase hex, so this should be fine, but worth noting for awareness.
119-122: Minor:RegExpis re-instantiated on every call.Consider hoisting the regex to a
static finalto avoid recompilation on each invocation.Proposed fix
+ static final RegExp _hexKeyPattern = RegExp(r'^[0-9a-fA-F]{64}$'); + static bool _isValidHexKey(String key) { if (key.length != 64) return false; - return RegExp(r'^[0-9a-fA-F]{64}$').hasMatch(key); + return _hexKeyPattern.hasMatch(key); }lib/services/nwc/nwc_client.dart (1)
186-195: Usee: [requestId]to filter server-side instead of client-side.The
NostrFilterclass supports aneparameter for#etag filtering. Replace the emptytarray withe: [requestId]to have the relay filter responses server-side, avoiding unnecessary bandwidth and client-side processing:Suggested change
final filter = NostrFilter( kinds: const [23195], authors: [connection.walletPubkey], - t: [ - // The 'e' tag references the request event - ], - // Filter by p tag matching our pubkey + e: [requestId], p: [_clientPubkey], );
- Inject NostrService dependency instead of accessing Nostr.instance directly - Fix subscription leak: wrap completer.future.timeout in try-finally to ensure subscriptions are always cleaned up on timeout or error - Improve disconnect() to close subscriptions on relay side - Remove unused dart:convert import - Add documentation about Nostr.instance singleton limitation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/services/nwc/nwc_client.dart`:
- Around line 127-133: The public methods payInvoice, makeInvoice, getBalance,
getInfo, and lookupInvoice currently force-unwrap response.result! which can
cause a TypeError if a successful response has a null result; update
_sendRequest to, after checking response.isSuccess, verify response.result !=
null and throw a meaningful NwcException (with context like method name and
response) when result is null so callers can safely call
X.fromMap(response.result); alternatively, add the same null-check and
NwcException throw in each caller before invoking *.fromMap, referencing the
_sendRequest method and the public methods named above.
- Around line 216-224: The NostrFilter is using t: [] (topic tags) instead of
filtering by event tags, causing the relay to return all kind 23195 events;
update the NostrFilter constructed in nwc_client.dart (the NostrFilter instance
near where connection.walletPubkey and _clientPubkey are used) to include e:
[requestId] (use the requestId variable) instead of t: [], and then remove or
simplify the manual eTag verification in the listener callback that currently
checks tags (the logic around lines 237–244) since server-side filtering will
ensure only responses for that requestId arrive. Ensure you keep authors and p
filters as-is and pass the correct requestId string into the e array.
🧹 Nitpick comments (2)
lib/services/nwc/nwc_client.dart (2)
109-122:disconnect()is synchronous but callscancel()which returns aFuture.
StreamSubscription.cancel()returns aFuturethat should be awaited to ensure clean teardown. The current synchronousvoid disconnect()fires and forgets the cancellations, meaning the subscriptions may not be fully cancelled before subsequent operations.♻️ Make disconnect async
- void disconnect() { + Future<void> disconnect() async { for (final entry in _subscriptions.entries) { - entry.value.cancel(); + await entry.value.cancel(); try {
269-273:sendEventToRelaysreturn value is ignored — no confirmation the request was published.If the relay rejects or fails to receive the event, the client will silently wait until timeout. Consider checking the result or at minimum logging a warning on failure, so debugging intermittent timeouts is easier.
- Add // ignore: unused_field for _nostrService (kept for future refactor) - Use stream.stream.listen() for NostrEventsStream subscription - Use sendEventToRelaysAsync instead of sendEventToRelays (correct API) - Wrap angle brackets in backticks in doc comment (nwc_connection.dart) - Use super parameters in NwcTimeoutException and NwcNotConnectedException
- Add null checks on response.result in all public methods to prevent null dereference on malformed wallet responses - Fix NostrFilter: use 'e' parameter instead of 't' to properly filter response events by the request event ID tag - Improve disconnect() doc comment documenting the known limitation that dart_nostr's shared singleton cannot selectively close relay connections - Remove misleading empty t:[] filter parameter
Member
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
- Rename nwc-phase1-implementation.md → NWC_PHASE1_IMPLEMENTATION.md to match uppercase convention used by other docs/ files - Update NwcClient constructor to reflect required nostrService parameter - Document NostrService dependency injection pattern and rationale - Add subscription lifecycle section (try/finally cleanup) - Document known limitation: shared Nostr singleton prevents selective relay disconnection - Add null-safe result handling documentation - Update test count (41 unit tests) - Add model serialization details and round-trip support
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
2 participants
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
Implements the core NWC (Nostr Wallet Connect / NIP-47) client library as described in #456.
What's included
New files under
lib/services/nwc/:nwc_connection.dart— Parsesnostr+walletconnect://URIs, extracts wallet pubkey, relay URLs, secret key, and optional lud16. Full validation.nwc_models.dart— Request/response models for NIP-47 JSON-RPC protocol. Includes typed models forpay_invoice,make_invoice,get_balance,get_info, andlookup_invoicecommands.nwc_client.dart— Core client that connects to wallet relays, signs kind 23194 request events with the connection secret, encrypts/decrypts payloads using NIP-44 (via the existingnip44package), and handles timeouts.nwc_exceptions.dart— Exception hierarchy mapping all NIP-47 error codes (RATE_LIMITED, INSUFFICIENT_BALANCE, UNAUTHORIZED, PAYMENT_FAILED, etc.) to typed exceptions.Tests under
test/services/nwc/:Documentation:
docs/nwc-phase1-implementation.md— Detailed implementation guide.Design decisions
dart_nostrfor relay connections,nip44package for NIP-44 encryption,equatablefor model equality — all already inpubspec.yaml.Next steps (Phase 2+)
Closes #456
Summary by CodeRabbit
Release Notes
New Features
Documentation