feat: migrate dispute chat to shared key encryption Phase 1#495
Conversation
- Add adminSharedKey field and setAdminPeer() to Session model with serialization - Add adminTookDispute handler in abstract_mostro_notifier to compute/persist admin shared key - Switch DisputeChatNotifier from mostroWrap/mostroUnWrap to p2pWrap/p2pUnwrap - Route dispute chat subscription via adminSharedKey.public instead of tradeKey.public - Use plain text content instead of MostroMessage JSON wrapper - Remove dm skip logic from MostroService (no longer needed with shared key routing) - Add dispute shared key tests (7 tests) and p2pWrap/p2pUnwrap round-trip tests (4 tests)
|
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:
WalkthroughAdds admin peer/pubkey and ECDH-derived adminSharedKey to Session; migrates dispute chat to p2pWrap/p2pUnwrap routed by adminSharedKey; adds Action.adminTookDispute handling to notifier; removes a prior dm guard in MostroService; adds tests for wrap/unwrap and shared-key behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ChatUI as Chat UI
participant Notifier as Dispute Notifier
participant Session
participant Relay as Relay
actor Admin
User->>ChatUI: compose/send message
ChatUI->>Notifier: send outbound
Notifier->>Session: request adminSharedKey
alt adminSharedKey available
Notifier->>Notifier: create plain-text event
Notifier->>Notifier: p2pWrap(event, adminSharedKey)
Notifier->>Relay: publish wrapped event (kind 1059, p-tag -> shared-key pub)
Relay->>Admin: deliver wrapped event
Admin->>Admin: p2pUnwrap(wrapped, adminSharedKey) and read content
else waiting for adminSharedKey
Notifier->>Session: subscribe/listen for adminSharedKey
Note right of Notifier: subscription gated until key ready
end
Admin->>Relay: publish wrapped reply
Relay->>Notifier: deliver wrapped event
Notifier->>Notifier: verify p-tag and p2pUnwrap using adminSharedKey
Notifier->>Session: store message (isFromAdmin/isFromUser, pending/error)
Notifier->>ChatUI: surface admin reply to user
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
502-528:this.sessionnot refreshed after updating admin peer.Other handlers that mutate the session (e.g.,
buyerTookOrderat Line 221–226 andholdInvoicePaymentAcceptedat Line 269–275) explicitly re-fetchsessionfrom the session notifier after the update. Here,this.sessionretains the stale copy withoutadminSharedKey. If any future code in this notifier accessessession.adminSharedKey, it will benull.♻️ Re-fetch session after mutation
if (adminPubkey != null && adminPubkey.isNotEmpty) { final sessionNotifier = ref.read(sessionNotifierProvider.notifier); + final fetchedSession = sessionNotifier.getSessionByOrderId(orderId); + if (fetchedSession == null) { + logger.e('Session not found for order $orderId in adminTookDispute'); + break; + } + session = fetchedSession; await sessionNotifier.updateSession( orderId, (s) => s.setAdminPeer(adminPubkey!)); + // Re-fetch to get the updated session with admin shared key + final updatedSession = sessionNotifier.getSessionByOrderId(orderId); + if (updatedSession != null) { + session = updatedSession; + } logger.i( 'Admin shared key computed and persisted for order $orderId');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/order/notfiers/abstract_mostro_notifier.dart` around lines 502 - 528, The handler updates the session via sessionNotifierProvider.notifier.updateSession(...) but does not refresh this.session, leaving this.session.adminSharedKey stale; after the await sessionNotifier.updateSession(orderId, (s) => s.setAdminPeer(adminPubkey!)), re-fetch and assign the updated session into this.session (e.g., read the session state from sessionNotifierProvider) so subsequent access to this.session.adminSharedKey reflects the newly persisted adminPubkey.lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
480-487: Fire-and-forgetinitialize()in provider factory — unhandled-future risk.
notifier.initialize()isasyncbut the returnedFutureis neither awaited nor wrapped inunawaited(). If the future rejects before the internal try-catch (e.g., a thrown error in_loadHistoricalMessagespropagating past the catch), it becomes an unhandled future. Dart's linter (unawaited_futures/discarded_futures) will flag this.♻️ Wrap in unawaited to silence the lint
(ref, disputeId) { final notifier = DisputeChatNotifier(disputeId, ref); - notifier.initialize(); + unawaited(notifier.initialize()); return notifier; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 480 - 487, The provider factory is calling the async method DisputeChatNotifier.initialize() in a fire-and-forget manner which can create unhandled futures; wrap that call with unawaited(...) to explicitly discard the Future and satisfy the linter (or make the factory async and await the call if you need initialization to complete before returning). Specifically, update the disputeChatNotifierProvider factory to call unawaited(notifier.initialize()) and add the appropriate import for unawaited (e.g., package:pedantic or package:async) so the linter no longer flags the discarded Future; reference DisputeChatNotifier.initialize and any internal helpers like _loadHistoricalMessages when making the change.lib/data/models/session.dart (1)
156-163: Missing type validation foradmin_peerinfromJson.Other optional fields like
peer(Line 138) andparent_order_id(Line 149) include anelse if (value is! String)branch that throws aFormatExceptionon invalid types. Theadmin_peerparsing silently ignores non-string values, which is inconsistent.♻️ Suggested fix for consistency
String? adminPeer; final adminPeerValue = json['admin_peer']; if (adminPeerValue != null) { if (adminPeerValue is String && adminPeerValue.isNotEmpty) { adminPeer = adminPeerValue; } + else if (adminPeerValue is! String) { + throw FormatException( + 'Invalid admin_peer type: ${adminPeerValue.runtimeType}', + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/models/session.dart` around lines 156 - 163, The fromJson parsing for admin_peer is currently skipping non-string values; update the fromJson (in the Session model) to validate adminPeerValue like the other fields: if adminPeerValue is a non-empty String assign it to adminPeer, else if adminPeerValue is not null and not a String throw a FormatException (mentioning 'admin_peer' and the offending type) so behavior matches peer and parent_order_id validation; adjust the adminPeer/adminPeerValue handling in the Session.fromJson method accordingly.
🤖 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/data/models/session.dart`:
- Around line 156-163: The fromJson parsing for admin_peer is currently skipping
non-string values; update the fromJson (in the Session model) to validate
adminPeerValue like the other fields: if adminPeerValue is a non-empty String
assign it to adminPeer, else if adminPeerValue is not null and not a String
throw a FormatException (mentioning 'admin_peer' and the offending type) so
behavior matches peer and parent_order_id validation; adjust the
adminPeer/adminPeerValue handling in the Session.fromJson method accordingly.
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 480-487: The provider factory is calling the async method
DisputeChatNotifier.initialize() in a fire-and-forget manner which can create
unhandled futures; wrap that call with unawaited(...) to explicitly discard the
Future and satisfy the linter (or make the factory async and await the call if
you need initialization to complete before returning). Specifically, update the
disputeChatNotifierProvider factory to call unawaited(notifier.initialize()) and
add the appropriate import for unawaited (e.g., package:pedantic or
package:async) so the linter no longer flags the discarded Future; reference
DisputeChatNotifier.initialize and any internal helpers like
_loadHistoricalMessages when making the change.
In `@lib/features/order/notfiers/abstract_mostro_notifier.dart`:
- Around line 502-528: The handler updates the session via
sessionNotifierProvider.notifier.updateSession(...) but does not refresh
this.session, leaving this.session.adminSharedKey stale; after the await
sessionNotifier.updateSession(orderId, (s) => s.setAdminPeer(adminPubkey!)),
re-fetch and assign the updated session into this.session (e.g., read the
session state from sessionNotifierProvider) so subsequent access to
this.session.adminSharedKey reflects the newly persisted adminPubkey.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/DISPUTE_CHAT_MULTIMEDIA_PLAN.mdlib/data/models/session.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/services/mostro_service.darttest/data/models/nostr_event_wrap_test.darttest/features/disputes/dispute_shared_key_test.dart
💤 Files with no reviewable changes (1)
- lib/services/mostro_service.dart
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (2)
179-193:⚠️ Potential issue | 🟡 Minor
admin_pubkeyis never persisted, soDisputeChat.adminPubkeyis alwaysnullafter reload.
_onChatEventstores the sender's pubkey under the key'pubkey'(line 186) but never writes'admin_pubkey'. However,_loadHistoricalMessagesreadseventData['admin_pubkey'] as String?(line 266), which is alwaysnullfrom storage.DisputeChat.adminPubkeyis correctly populated in memory (line 202) but silently lost after an app restart.🐛 Proposed fix — persist the field when it's from the admin
'is_from_user': !isFromAdmin, 'isPending': false, + if (isFromAdmin) 'admin_pubkey': senderPubkey, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 179 - 193, The stored event map written in _onChatEvent via eventStore.putItem omits the 'admin_pubkey' field, so DisputeChat.adminPubkey (read in _loadHistoricalMessages) is null after reload; update the payload passed to eventStore.putItem (in the _onChatEvent code path where you build the map and call eventStore.putItem/eventId) to include 'admin_pubkey': isFromAdmin ? senderPubkey : null (or omit when null), ensuring that the persisted key name matches what _loadHistoricalMessages expects, so DisputeChat.adminPubkey is restored on reload.
480-487:⚠️ Potential issue | 🟡 MinorErrors from
_subscribe()insideinitialize()are silently swallowed byunawaited.
_loadHistoricalMessagesproperly routes failures intostate.copyWith(error:), but_subscribe()has no equivalent guard. IfsubscribeToEventsthrows, the error escapes_subscribe(), propagates throughinitialize(), and is discarded byunawaited. The user sees a blank chat with no error message and_isInitializedstaysfalse.🛡️ Proposed fix — catch subscribe errors inside `initialize()`
Future<void> initialize() async { if (_isInitialized) return; logger.i('Initializing dispute chat for disputeId: $disputeId'); await _loadHistoricalMessages(); - await _subscribe(); + try { + await _subscribe(); + } catch (e, st) { + logger.e('Failed to subscribe for dispute $disputeId: $e', stackTrace: st); + state = state.copyWith(error: 'Subscription failed: $e'); + } _isInitialized = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 480 - 487, The initialize() flow in DisputeChatNotifier (created by disputeChatNotifierProvider) currently unawaits the async initialize so exceptions from _subscribe() are swallowed; wrap the call to _subscribe()/subscribeToEvents inside initialize() with a try/catch that captures any thrown error and updates state via state.copyWith(error: <wrapped error>) (similar to _loadHistoricalMessages), optionally log the error, and ensure _isInitialized remains false (or set to false explicitly) so the UI shows an error instead of a silent blank chat; reference DisputeChatNotifier.initialize(), DisputeChatNotifier._subscribe(), subscribeToEvents, and state.copyWith(error:) when making the change.
🧹 Nitpick comments (1)
lib/data/models/session.dart (1)
35-35: RenameadminPeerconstructor parameter toadminPubkeyfor consistency.The sibling
peerparameter takes aPeer?object, soadminPeerimplies the same type. Everywhere else — the private field (_adminPubkey), the getter (adminPubkey), and the method (setAdminPeer(String adminPubkey)) — the value is correctly named as a pubkey. The constructor parameter should match.♻️ Proposed rename
- String? adminPeer, + String? adminPubkey,- if (adminPeer != null) { - setAdminPeer(adminPeer); + if (adminPubkey != null) { + setAdminPeer(adminPubkey); }Also update the
fromJsoncall site:- adminPeer: adminPeer, + adminPubkey: adminPeer,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/models/session.dart` at line 35, Rename the Session constructor parameter adminPeer to adminPubkey to match the existing private field _adminPubkey, the getter adminPubkey, and the setter setAdminPeer(String adminPubkey); update all constructor callers including the fromJson call site to pass adminPubkey instead of adminPeer so naming is consistent across Session, _adminPubkey, adminPubkey, and setAdminPeer.
🤖 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/data/models/session.dart`:
- Around line 192-198: The setAdminPeer method currently passes whatever string
it receives into NostrUtils.computeSharedKey which will throw on empty or
malformed keys; update setAdminPeer to validate adminPubkey before using it
(e.g., check adminPubkey.isNotEmpty and that it matches your expected pubkey
format/length) and return early or throw a clear error if invalid, ensuring
_adminPubkey and _adminSharedKey are only set when adminPubkey is valid;
reference setAdminPeer, _adminPubkey, _adminSharedKey,
NostrUtils.computeSharedKey and tradeKey.private when making the guard so
callers like the adminTookDispute handler no longer cause uncaught exceptions.
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 163-173: The current admin-auth block in
dispute_chat_notifier.dart uses await
ref.read(disputeDetailsProvider(disputeId).future) and returns early if
dispute?.adminPubkey == null, which can silently drop legitimate admin messages
during a loading race; change the logic in the isFromAdmin branch (the
disputeDetailsProvider(disputeId) check around disputeId, senderPubkey,
adminPubkey) to either (a) defer the security decision until the provider
settles by waiting/retrying a short period for a non-null dispute.adminPubkey,
or (b) accept the message temporarily and queue/mark it for re-verification once
disputeDetailsProvider resolves, and in the meantime emit a structured
logger.warn that includes disputeId and senderPubkey so drops are diagnosable;
ensure you keep the existing unauthorized check (senderPubkey !=
dispute.adminPubkey) once dispute data is confirmed.
---
Outside diff comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 179-193: The stored event map written in _onChatEvent via
eventStore.putItem omits the 'admin_pubkey' field, so DisputeChat.adminPubkey
(read in _loadHistoricalMessages) is null after reload; update the payload
passed to eventStore.putItem (in the _onChatEvent code path where you build the
map and call eventStore.putItem/eventId) to include 'admin_pubkey': isFromAdmin
? senderPubkey : null (or omit when null), ensuring that the persisted key name
matches what _loadHistoricalMessages expects, so DisputeChat.adminPubkey is
restored on reload.
- Around line 480-487: The initialize() flow in DisputeChatNotifier (created by
disputeChatNotifierProvider) currently unawaits the async initialize so
exceptions from _subscribe() are swallowed; wrap the call to
_subscribe()/subscribeToEvents inside initialize() with a try/catch that
captures any thrown error and updates state via state.copyWith(error: <wrapped
error>) (similar to _loadHistoricalMessages), optionally log the error, and
ensure _isInitialized remains false (or set to false explicitly) so the UI shows
an error instead of a silent blank chat; reference
DisputeChatNotifier.initialize(), DisputeChatNotifier._subscribe(),
subscribeToEvents, and state.copyWith(error:) when making the change.
---
Nitpick comments:
In `@lib/data/models/session.dart`:
- Line 35: Rename the Session constructor parameter adminPeer to adminPubkey to
match the existing private field _adminPubkey, the getter adminPubkey, and the
setter setAdminPeer(String adminPubkey); update all constructor callers
including the fromJson call site to pass adminPubkey instead of adminPeer so
naming is consistent across Session, _adminPubkey, adminPubkey, and
setAdminPeer.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/data/models/session.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/order/notfiers/abstract_mostro_notifier.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/order/notfiers/abstract_mostro_notifier.dart
- Rename adminPeer param to adminPubkey for naming consistency - Add validation guard in setAdminPeer() for empty/invalid pubkeys - Re-fetch session after updateSession in adminTookDispute handler - Wrap initialize() with unawaited() in dispute chat provider - Improve admin message rejection logs with eventId context - Add type validation for admin_peer in Session.fromJson
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
143-180: Deduplicate using the unwrapped event id.You store events under the unwrapped id, but the pre-check uses the wrapper id. This makes the duplicate guard ineffective when the same inner event arrives from multiple relays.
♻️ Suggested tweak
- // Check for duplicate events - final eventStore = ref.read(eventStorageProvider); - if (await eventStore.hasItem(event.id!)) { - return; - } - - // Unwrap using admin shared key (1-layer p2p decryption) - final unwrappedEvent = await event.p2pUnwrap(session.adminSharedKey!); + final eventStore = ref.read(eventStorageProvider); + + // Unwrap using admin shared key (1-layer p2p decryption) + final unwrappedEvent = await event.p2pUnwrap(session.adminSharedKey!); @@ final messageText = unwrappedEvent.content ?? ''; if (messageText.isEmpty) { logger.w('Received empty message, skipping'); return; } + + final eventId = unwrappedEvent.id ?? + event.id ?? + 'chat_${DateTime.now().millisecondsSinceEpoch}_${messageText.hashCode}'; + if (await eventStore.hasItem(eventId)) { + return; + } @@ - final eventId = unwrappedEvent.id ?? event.id ?? 'chat_${DateTime.now().millisecondsSinceEpoch}_${messageText.hashCode}';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 143 - 180, The duplicate-check currently uses the wrapper id (event.id) so duplicates slip through when the same inner event arrives from other relays; move or change the dedupe to use the unwrapped id: after calling event.p2pUnwrap(session.adminSharedKey!) (p2pUnwrap) compute the canonical id (use unwrappedEvent.id if non-null, else fall back to event.id or the existing generated eventId) and call eventStore.hasItem(...) (eventStore from eventStorageProvider) with that canonical id; ensure the same canonical id is used later when storing the event so the guard is effective.
🤖 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/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 143-180: The duplicate-check currently uses the wrapper id
(event.id) so duplicates slip through when the same inner event arrives from
other relays; move or change the dedupe to use the unwrapped id: after calling
event.p2pUnwrap(session.adminSharedKey!) (p2pUnwrap) compute the canonical id
(use unwrappedEvent.id if non-null, else fall back to event.id or the existing
generated eventId) and call eventStore.hasItem(...) (eventStore from
eventStorageProvider) with that canonical id; ensure the same canonical id is
used later when storing the event so the guard is effective.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/data/models/session.dartlib/features/disputes/notifiers/dispute_chat_notifier.darttest/features/disputes/dispute_shared_key_test.dart
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
95-96: Add anonErrorhandler to the stream subscription.Without an
onErrorcallback, any error emitted by the underlying relay stream (e.g., relay disconnect, malformed frame) propagates to the ambient Zone unhandled, bypassing the_onChatEventtry/catch entirely.♻️ Proposed fix
- _subscription = nostrService.subscribeToEvents(request).listen(_onChatEvent); + _subscription = nostrService.subscribeToEvents(request).listen( + _onChatEvent, + onError: (Object e, StackTrace st) => + logger.e('Stream error in dispute chat subscription: $e', stackTrace: st), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 95 - 96, The stream subscription created by calling nostrService.subscribeToEvents(request).listen(_onChatEvent) needs an onError handler so relay errors don't escape the Zone; update the listen call on the _subscription assignment to provide an onError (and optional onDone) callback that catches/logs the error (use logger.e with context "disputeId" and the error/stackTrace), and perform appropriate cleanup or reconnection (e.g., cancel _subscription or invoke your reconnect logic) so errors are handled inside the DisputeChatNotifier rather than propagating unhandled; reference the existing symbols _subscription, nostrService.subscribeToEvents, and _onChatEvent when making this change.
🤖 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/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 250-261: The security check in dispute_chat_notifier.dart
incorrectly allows null messagePubkey to bypass admin authorization; update the
branch inside the if (!isFromUser) block so that messages with a null pubkey are
treated as unauthorized by changing the check to reject when messagePubkey is
null OR does not equal dispute!.adminPubkey (i.e., if (messagePubkey == null ||
messagePubkey != dispute!.adminPubkey) { logger.w(...); filteredCount++;
continue; }), keeping the existing early null-dispute check
(dispute?.adminPubkey == null) intact.
- Around line 309-322: Move the NostrEvent.fromPartialData call and the
rumor.id/rumorTimestamp extraction inside the existing try block so any
exceptions are caught and handled; specifically, wrap the creation of the
`rumor` (NostrEvent.fromPartialData(...)), the assignment `final rumorId =
rumor.id`, and `final rumorTimestamp = rumor.createdAt ?? DateTime.now()` inside
the try, and add a null-check for `rumor.id` (avoid `!`) to set `state.error`
and add the pending message on failure; update code paths that reference
`rumorId` to account for the null-check so the catch block reliably sets
`state.error` and updates pending state when event construction fails.
- Around line 144-147: Replace the force-unwrap usage of event.id in the hasItem
call with an explicit null guard: compute or read the nullable id first (e.g.
local variable referencing event.id), if id is null return/skip early, then call
eventStore.hasItem(id); this keeps behavior consistent with the later null-aware
usage (unwrappedEvent.id ?? event.id ?? 'chat_...') and avoids relying on
exceptions for control flow; look for the hasItem call and the unwrappedEvent.id
usage to update both places accordingly.
---
Nitpick comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 95-96: The stream subscription created by calling
nostrService.subscribeToEvents(request).listen(_onChatEvent) needs an onError
handler so relay errors don't escape the Zone; update the listen call on the
_subscription assignment to provide an onError (and optional onDone) callback
that catches/logs the error (use logger.e with context "disputeId" and the
error/stackTrace), and perform appropriate cleanup or reconnection (e.g., cancel
_subscription or invoke your reconnect logic) so errors are handled inside the
DisputeChatNotifier rather than propagating unhandled; reference the existing
symbols _subscription, nostrService.subscribeToEvents, and _onChatEvent when
making this change.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (4)
265-275:⚠️ Potential issue | 🟡 MinorStorage key mismatch:
admin_pubkeyis read but never written.Line 272 reads
eventData['admin_pubkey'], but the storage at line 192 writes the admin's pubkey under the key'pubkey'. This meansDisputeChat.adminPubkeywill always benullfor historically loaded messages, even though the security check at line 250 correctly reads'pubkey'.Proposed fix
messages.add(DisputeChat( id: eventData['id'] as String, message: eventData['content'] as String? ?? '', timestamp: DateTime.fromMillisecondsSinceEpoch( (eventData['created_at'] as int) * 1000, ), isFromUser: isFromUser, - adminPubkey: eventData['admin_pubkey'] as String?, + adminPubkey: !isFromUser ? (eventData['pubkey'] as String?) : null, isPending: eventData['isPending'] as bool? ?? false, error: eventData['error'] as String?, ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 265 - 275, The code constructs DisputeChat entries using eventData['admin_pubkey'] but the stored admin key is written under 'pubkey', causing adminPubkey to be null for historical messages; update the reader in the message assembly (where DisputeChat is created) to use eventData['pubkey'] (or make the writer use 'admin_pubkey' consistently) so that DisputeChat.adminPubkey and the security check that reads 'pubkey' reference the same storage key.
367-370:⚠️ Potential issue | 🟡 MinorRaw exception details leak into
state.errorwhich may be user-facing.Lines 369, 433 set
state.errorto strings like'Failed to send message: $publishError'and'Failed to send message: $e'. If this is rendered in the UI, raw exception details (stack info, internal errors) are exposed. Per project convention, keep technical details inlogger.e()and use a generic user-facing message.Proposed fix (apply similarly at line 433)
state = state.copyWith( messages: updatedMessages, - error: 'Failed to send message: $publishError', + error: 'Failed to send message', );Based on learnings: "do not expose raw exception messages (e.g.,
e.toString()) in the UI viashowError()calls. The team prefers to keep technical error details out of the user interface for security and UX reasons."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 367 - 370, The state currently embeds raw exception details (e.g., using publishError or e) into state.error (see the state.copyWith that sets messages: updatedMessages, error: 'Failed to send message: $publishError' and the similar assignment at line 433) which may be surfaced to users; instead log the full error via the notifier's logger (e.g., logger.e(..., error: publishError)) and set state.error to a generic, user-facing string like "Failed to send message" (no exception text); apply the same change at both occurrences (the state.copyWith that uses publishError and the later one that uses e) so UI gets only the generic message while technical details remain in logs.
143-200:⚠️ Potential issue | 🔴 CriticalDedup check is broken: wrapper event ID vs. unwrapped event ID mismatch.
Line 147 checks
eventStore.hasItem(wrapperEventId)wherewrapperEventIdisevent.id(the kind-1059 Gift Wrap ID). But line 186 stores the record undereventId(the unwrapped rumor ID from line 181). These are different IDs — the wrapper is never stored under its own ID, so the dedup check will never match and duplicate deliveries from relays will produce duplicate chat messages.Proposed fix — also store the wrapper ID for dedup
// Store the event + // Store wrapper event ID for deduplication + await eventStore.putItem(wrapperEventId, {'wrapper_for': eventId}); + await eventStore.putItem( eventId, { 'id': eventId,Alternatively, you could use
wrapperEventIdas the primary storage key if the unwrapped event ID isn't needed for other lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 143 - 200, The dedup check uses wrapperEventId (event.id) but you store the unwrapped event under eventId (unwrappedEvent.id), so duplicates slip through; fix by ensuring the wrapper ID is recorded for dedup: after computing eventId (or instead of it) write the same stored record under wrapperEventId (or store a separate mapping from wrapperEventId -> eventId) so eventStore.hasItem(wrapperEventId) will hit; update the eventStore.putItem call (and any metadata) to include wrapperEventId, referencing wrapperEventId, eventId, eventStore.hasItem and eventStore.putItem (or alternatively switch to always using wrapperEventId as the primary storage key if unwrappedEvent.id is not required).
27-37:⚠️ Potential issue | 🟠 MajorBug:
copyWithcannot clearerrorback tonull.
error: error ?? this.errormeans callingcopyWith(error: null)preserves the previous error. Line 345 relies oncopyWith(error: null)to clear errors when the user sends a new message — this silently fails, leaving stale error text in state.A common Dart pattern for nullable
copyWithfields uses a sentinel or a wrapper:Proposed fix
DisputeChatState copyWith({ List<DisputeChat>? messages, bool? isLoading, - String? error, + Object? error = _sentinel, }) { return DisputeChatState( messages: messages ?? this.messages, isLoading: isLoading ?? this.isLoading, - error: error ?? this.error, + error: error == _sentinel ? this.error : error as String?, ); } } + +const Object _sentinel = Object();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 27 - 37, The copyWith currently uses `error: error ?? this.error` so passing `copyWith(error: null)` cannot clear the error; modify DisputeChatState.copyWith to accept a sentinel for nullable fields (e.g., change the parameter signature to use Object? error = _noChange with a private const _noChange = Object()) and then set the returned `error` to `error == _noChange ? this.error : error as String?` so callers can pass `error: null` to clear it; apply the same pattern for any other nullable fields you need to explicitly clear and keep referencing DisputeChatState.copyWith, the `error` parameter, and the `_noChange` sentinel in your change.
♻️ Duplicate comments (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
311-329: Rumor construction still outsidetry— unhandled throw fromfromPartialData.The null-check on
rumorId(lines 321-326) addresses the NPE risk from the prior review, butNostrEvent.fromPartialDataitself can throw (e.g., invalid key material), and that exception escapes thetryblock starting at line 329. The user gets no error feedback and the message silently disappears.Move the rumor construction inside the
tryblock:Proposed fix
+ try { // Create rumor (kind 1) with plain text content FIRST to get real event ID final rumor = NostrEvent.fromPartialData( keyPairs: session.tradeKey, content: text, kind: 1, tags: [ ["p", session.adminSharedKey!.public], ], ); final rumorId = rumor.id; if (rumorId == null) { logger.e('Failed to compute rumor ID for dispute: $disputeId'); state = state.copyWith(error: 'Failed to prepare message'); return; } final rumorTimestamp = rumor.createdAt ?? DateTime.now(); - try { logger.i('Sending p2pWrap DM to admin via shared key for dispute: $disputeId');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 311 - 329, The construction of rumor via NostrEvent.fromPartialData is currently outside the try block and can throw (e.g., invalid key material), so move the entire rumor creation (the call to NostrEvent.fromPartialData and the subsequent checks for rumor.id and rumor.createdAt) inside the existing try block that begins after rumorTimestamp; ensure error handling uses the same logger.e('Failed to compute rumor ID for dispute: $disputeId') and state = state.copyWith(error: 'Failed to prepare message') path when fromPartialData throws or when rumor.id is null so exceptions are caught and the user gets proper feedback.
🧹 Nitpick comments (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
356-456: Consider extracting the duplicated error-handling logic.The inner publish-failure handler (lines 362-392) and the outer catch (lines 418-456) follow nearly identical patterns: mark the pending message as failed, update state, and persist error metadata. A small helper like
_handleSendFailure(rumorId, text, rumorTimestamp, error)would eliminate the duplication and make both paths easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 356 - 456, The publish-failure block and the outer catch in dispute_chat_notifier.dart duplicate logic for marking a message failed, updating state, and persisting error metadata; extract this into a helper (e.g., _handleSendFailure) that takes (String rumorId, String text, DateTime rumorTimestamp, Object error, {String? pubkey, int? kind, DisputeChat? fallbackMessage}) and centralizes: build the failed DisputeChat (using DisputeChat fallback when needed), update state.messages, set state.error, and write the same eventStore.putItem payload (including error, isPending=false, dispute_id, etc.); then replace both the inner catch (publishError) and the outer catch (e) to call this helper and remove the duplicated code paths, preserving existing fields like 'sig', 'tags', 'kind', and 'pubkey' via optional args.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 265-275: The code constructs DisputeChat entries using
eventData['admin_pubkey'] but the stored admin key is written under 'pubkey',
causing adminPubkey to be null for historical messages; update the reader in the
message assembly (where DisputeChat is created) to use eventData['pubkey'] (or
make the writer use 'admin_pubkey' consistently) so that DisputeChat.adminPubkey
and the security check that reads 'pubkey' reference the same storage key.
- Around line 367-370: The state currently embeds raw exception details (e.g.,
using publishError or e) into state.error (see the state.copyWith that sets
messages: updatedMessages, error: 'Failed to send message: $publishError' and
the similar assignment at line 433) which may be surfaced to users; instead log
the full error via the notifier's logger (e.g., logger.e(..., error:
publishError)) and set state.error to a generic, user-facing string like "Failed
to send message" (no exception text); apply the same change at both occurrences
(the state.copyWith that uses publishError and the later one that uses e) so UI
gets only the generic message while technical details remain in logs.
- Around line 143-200: The dedup check uses wrapperEventId (event.id) but you
store the unwrapped event under eventId (unwrappedEvent.id), so duplicates slip
through; fix by ensuring the wrapper ID is recorded for dedup: after computing
eventId (or instead of it) write the same stored record under wrapperEventId (or
store a separate mapping from wrapperEventId -> eventId) so
eventStore.hasItem(wrapperEventId) will hit; update the eventStore.putItem call
(and any metadata) to include wrapperEventId, referencing wrapperEventId,
eventId, eventStore.hasItem and eventStore.putItem (or alternatively switch to
always using wrapperEventId as the primary storage key if unwrappedEvent.id is
not required).
- Around line 27-37: The copyWith currently uses `error: error ?? this.error` so
passing `copyWith(error: null)` cannot clear the error; modify
DisputeChatState.copyWith to accept a sentinel for nullable fields (e.g., change
the parameter signature to use Object? error = _noChange with a private const
_noChange = Object()) and then set the returned `error` to `error == _noChange ?
this.error : error as String?` so callers can pass `error: null` to clear it;
apply the same pattern for any other nullable fields you need to explicitly
clear and keep referencing DisputeChatState.copyWith, the `error` parameter, and
the `_noChange` sentinel in your change.
---
Duplicate comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 311-329: The construction of rumor via NostrEvent.fromPartialData
is currently outside the try block and can throw (e.g., invalid key material),
so move the entire rumor creation (the call to NostrEvent.fromPartialData and
the subsequent checks for rumor.id and rumor.createdAt) inside the existing try
block that begins after rumorTimestamp; ensure error handling uses the same
logger.e('Failed to compute rumor ID for dispute: $disputeId') and state =
state.copyWith(error: 'Failed to prepare message') path when fromPartialData
throws or when rumor.id is null so exceptions are caught and the user gets
proper feedback.
---
Nitpick comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 356-456: The publish-failure block and the outer catch in
dispute_chat_notifier.dart duplicate logic for marking a message failed,
updating state, and persisting error metadata; extract this into a helper (e.g.,
_handleSendFailure) that takes (String rumorId, String text, DateTime
rumorTimestamp, Object error, {String? pubkey, int? kind, DisputeChat?
fallbackMessage}) and centralizes: build the failed DisputeChat (using
DisputeChat fallback when needed), update state.messages, set state.error, and
write the same eventStore.putItem payload (including error, isPending=false,
dispute_id, etc.); then replace both the inner catch (publishError) and the
outer catch (e) to call this helper and remove the duplicated code paths,
preserving existing fields like 'sig', 'tags', 'kind', and 'pubkey' via optional
args.
Catrya
changed the title
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid architectural change — migrating from mostroWrap/mostroUnWrap (2-layer NIP-59) to p2pWrap/p2pUnwrap (1-layer ECDH shared key) for dispute chat is the right call. The code is significantly cleaner than the old JSON parsing mess. Tests are good. CodeRabbit issues were addressed. But I found some issues that need fixing:
Must Fix
1. Race condition: admin messages permanently lost (SECURITY)
In _onChatEvent (line ~170), when an admin message arrives but disputeDetailsProvider hasn't resolved yet (race with adminTookDispute), the message is silently dropped and never re-delivered. The Nostr subscription won't replay it.
CodeRabbit flagged this too and it was marked "addressed" in commit e619f87, but looking at the current code, it only improved the log message — the message is still permanently dropped. This is a real bug: adminTookDispute computes the shared key → subscription starts → first admin message arrives → but disputeDetailsProvider cache still has adminPubkey: null → message dropped.
Fix options:
- Queue the message and re-process after
disputeDetailsProviderrefreshes - Accept the message when
adminSharedKeyis set (the ECDH itself is the auth — if you can decrypt it, it came from the admin) - At minimum: store the event and replay on next
_loadHistoricalMessages
This is the most critical issue.
2. setAdminPeer throws but adminTookDispute handler doesn't catch
In abstract_mostro_notifier.dart line ~510, setAdminPeer is called inside updateSession callback. setAdminPeer now throws ArgumentError on invalid input (good validation added in the fix commit). But the adminTookDispute handler has no try/catch around it — an invalid admin pubkey from the protocol would crash the entire order processing flow.
// This can throw ArgumentError:
await sessionNotifier.updateSession(
orderId, (s) => s.setAdminPeer(adminPubkey!));Wrap it in try/catch and log the error gracefully.
3. Commit messages: typo and low quality
Two commits are just "coderabbit suggestions" and "coderabbit suggetsions" (with typo). Per repo AGENTS.md: "Write concise, imperative commits, optionally prefixed with a scope."
Not a blocker but worth noting for future PRs.
Minor / Nice to have
4. _isNearBottom threshold is arbitrary
_nearBottomThreshold = 150.0 pixels — this varies dramatically across devices. On a small phone 150px could be half the chat. Consider making it a percentage of viewport height, or at least document why 150.
5. _getSessionForDispute swallows all info
The method was simplified (removed logging), but now when session lookup fails there's zero diagnostic info. The old code had useful logs. Consider keeping at least one logger.w for the not-found case — disputes are a critical flow and debugging blind is painful.
What looks good
- ✅ CI passes, no conflicts
- ✅ 11 tests covering shared key computation, serialization round-trip, p2pWrap/p2pUnwrap, and wrong-key rejection
- ✅ Security: p-tag verification, sender pubkey validation, dedup via
eventStore.hasItem - ✅ Optimistic UI with real rumor ID for dedup
- ✅ Clean removal of the old JSON parsing logic (
dm/MostroMessageformat) - ✅
_normalizeInitiator-style validation insetAdminPeer(length check) - ✅ The
unawaited(notifier.initialize())fix for the provider
- Remove redundant disputeDetailsProvider check — ECDH is sufficient auth - Wrap setAdminPeer() in try/catch to handle malformed admin pubkeys - Add diagnostic logging when session lookup fails for dispute
|
@mostronatorcoder review |
There was a problem hiding this comment.
All three issues from my previous review are resolved in f18c040:
- Race condition fixed — Removed the redundant
disputeDetailsProvidercheck. The ECDH shared key IS the authentication: ifp2pUnwrapsucceeds, the sender holds the admin's private key. Clean and correct. setAdminPeernow wrapped in try/catch — Malformed admin pubkeys are logged gracefully instead of crashing the order flow.- Diagnostic logging restored —
logger.wadded back for failed session lookups.
Also good: historical message loading no longer re-filters by admin pubkey (they were already authenticated at receive time via ECDH). Simplifies the code and eliminates another race window.
CI ✅ | No conflicts ✅
Approved.
|
@Catrya All three issues resolved ✅ — approved. |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation