feat: wire P2P chat bridge (send, receive, chat rooms list) - #95
Merged
Conversation
## Summary Connects the P2P trade chat end-to-end — from NIP-59 gift-wrap publishing to real-time incoming message delivery and chat room population from trades. ## Root cause analysis The chat screen had optimistic local state only; the Rust bridge had all the building blocks (send/receive/mark-as-read) but was missing three wiring points: 1. `take_order` did not create a session or subscribe gift-wraps. 2. `process_gift_wrap_rumor` did not handle `BuyerTookOrder` / `HoldInvoicePaymentAccepted` — the actions that carry the peer's trade pubkey. 3. No background task subscribed to NIP-59 kind-1059 events addressed to the shared-key pubkey for incoming P2P messages. ## Changes ### Rust (`rust/src/api/orders.rs`) - `take_order`: after successful publish, now calls `subscribe_gift_wraps` + `session_manager().create_session()` so the session exists before the first daemon response arrives. - `process_gift_wrap_rumor`: handles `Action::BuyerTookOrder` and `Action::HoldInvoicePaymentAccepted`. Both carry the counterpart's trade pubkey in the SmallOrder payload. On receipt, calls `on_peer_pubkey_received`. - New `on_peer_pubkey_received(order_id, trade_pubkey_hex, peer_pubkey_hex)`: derives the NIP-04 ECDH shared secret, computes the shared-key pubkey (shared_secret used as a private scalar), stores both in the session, and spawns `subscribe_incoming_chat`. - Tests: session idempotency, initial state, graceful no-op on missing session. ### Rust (`rust/src/api/messages.rs`) - `subscribe_incoming_chat`: new `pub(crate)` function. Subscribes to kind-1059 gift-wrap events with p-tag == shared-key pubkey. Decrypts each rumor, ignores own echoes (sender == trade key), constructs a `ChatMessage`, appends it to `message_store()`, which fires the `on_new_message` stream. Exits after 30 min of inactivity. - `send_message`: now wraps to `shared_pubkey` (ECDH shared-key pubkey) instead of `peer_pubkey` directly, per the Mostro P2P chat protocol spec. - `send_file`: same fix — wraps to shared-key pubkey. - Tests: deduplication contract, `on_new_message` trade-id filtering. ### Dart (`lib/features/chat/providers/chat_providers.dart`) - `chatRoomsFromTradesProvider`: new `FutureProvider` that converts `rawTradesProvider` entries with a known `counterpartyPubkey` into `ChatRoomState`s (resolves NymIdentity, loads last message preview, counts unread). - `incomingMessageProvider`: new `StreamProvider.family` that wraps `messages_api.onNewMessage` for a given trade ID. Consumed by `ChatRoomScreen` via `ref.listen`. - `messageHistoryProvider`: `FutureProvider.family` over `messages_api.getMessages` (kept as reference; screen uses it directly). ### Dart (`lib/features/chat/screens/chat_room_screen.dart`) - Replaces the hardcoded optimistic `_messages` list with bridge-backed state: - `_loadHistory`: seeds from `messages_api.getMessages` on `initState`. - `_markRead`: calls `messages_api.markAsRead` and resets unread badge. - `_onSend`: calls `messages_api.sendMessage`, appends returned message. - `ref.listen(incomingMessageProvider)`: appends live incoming messages. - Adds empty-state placeholder and loading indicator. - Adapts FRB `rust_types.ChatMessage` → Dart `ChatMessage` for `MessageBubble`. ### Dart (`lib/features/chat/screens/chat_rooms_screen.dart`) - `ChatRoomsScreen` → `ConsumerStatefulWidget`: calls `_syncRoomsFromTrades` on init, populating `chatRoomsNotifierProvider` from `chatRoomsFromTradesProvider`.
Member
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR connects Rust-backed NIP-59 gift-wrap messaging to the Flutter chat UI: it adds bridge providers mapping trades to chat rooms, streaming incoming messages and history, updates UI screens for lifecycle and realtime updates, and extends Rust order/session logic to derive shared keys and spawn incoming-chat subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Flutter as Flutter App
participant Riverpod as Riverpod Provider<br/>(incomingMessageProvider)
participant RustAPI as Rust Messages API<br/>(subscribe_incoming_chat)
participant Nostr as Nostr Daemon<br/>(Gift-wrap Events)
participant MsgStore as In-Memory<br/>Message Store
Flutter->>Riverpod: ref.listen(incomingMessageProvider(tradeId))
Riverpod->>RustAPI: subscribe_incoming_chat(shared_pubkey, trade_keys)
RustAPI->>Nostr: subscribe to Kind 1059 (gift-wrap)
Nostr-->>RustAPI: gift-wrap event (encrypted)
RustAPI->>RustAPI: decrypt with recipient_keys
RustAPI->>RustAPI: parse rumor JSON → ChatMessage
RustAPI->>RustAPI: filter echo (by trade_pubkey)
RustAPI->>MsgStore: insert ChatMessage
MsgStore-->>Riverpod: emit new message on stream
Riverpod-->>Flutter: deliver AsyncValue.data(msg)
Flutter->>Flutter: dedupe, append, scroll, mark read, update preview
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/chat/screens/chat_room_screen.dart (1)
164-177:⚠️ Potential issue | 🟡 MinorFallback
ChatRoomStateuses emptyorderIdinstead ofwidget.orderId.When
_resolveRoom()doesn't find a matching room, the fallback hasorderId: ''. This could cause issues in_buildRoomPreview()which usesroom.copyWith()— the upserted room would have an empty orderId and not match the current trade.🐛 Suggested fix
orElse: () => ChatRoomState( - orderId: '', + orderId: widget.orderId, peerPubkey: '', peerHandle: 'Unknown', peerIconIndex: 0, peerColorHue: 180, isSelling: false, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/screens/chat_room_screen.dart` around lines 164 - 177, The fallback ChatRoomState returned by _resolveRoom() uses orderId: '' which will break matching when _buildRoomPreview() later calls room.copyWith(); update the fallback to use the current widget.orderId instead (i.e. set orderId: widget.orderId) so the upserted room retains the correct orderId; change only the fallback ChatRoomState in _resolveRoom() to use widget.orderId while keeping other default fields intact.
🧹 Nitpick comments (5)
rust/src/api/orders.rs (2)
992-994: Extraneous whitespace in log message.The log message has inconsistent spacing due to string literal continuation across lines. This could make log parsing harder.
📝 Clean up log formatting
log::warn!( - "[orders] on_peer_pubkey_received: session not found for order={order_id}, skipping session update — incoming subscription still spawned" + "[orders] on_peer_pubkey_received: session not found for order={order_id}, \ + skipping session update — incoming subscription still spawned" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/orders.rs` around lines 992 - 994, The log::warn! call inside on_peer_pubkey_received contains extraneous whitespace from a broken string literal; replace the multi-line/space-padded message with a single cleanly formatted string (keeping the {order_id} interpolation) so it reads e.g. "[orders] on_peer_pubkey_received: session not found for order={order_id}, skipping session update — incoming subscription still spawned" to ensure consistent log formatting and easier parsing.
899-912: Verify peer pubkey extraction logic for each action type.The logic extracts
buyer_trade_pubkeyforBuyerTookOrderandseller_trade_pubkeyfor all other cases (line 901). However, the match only coversBuyerTookOrderandHoldInvoicePaymentAccepted. ForHoldInvoicePaymentAccepted, the buyer receives the seller's pubkey, which is correct. The wildcard_on line 901 could be more explicit.📝 Make the match exhaustive for clarity
let peer_pubkey_hex = match kind.action { Action::BuyerTookOrder => small_order.buyer_trade_pubkey.clone(), - _ => small_order.seller_trade_pubkey.clone(), + Action::HoldInvoicePaymentAccepted => small_order.seller_trade_pubkey.clone(), + _ => unreachable!("only BuyerTookOrder and HoldInvoicePaymentAccepted reach here"), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/orders.rs` around lines 899 - 912, The peer pubkey selection currently uses a wildcard for all non-BuyerTookOrder cases which is ambiguous; update the match on kind.action so it explicitly lists the Action variants that should use small_order.seller_trade_pubkey (e.g., Action::HoldInvoicePaymentAccepted and any other specific seller-side actions) and keep Action::BuyerTookOrder mapped to small_order.buyer_trade_pubkey; use an explicit catch-all arm only if you add a clear comment explaining why it falls back to seller_trade_pubkey, referencing the symbols kind.action, Action::BuyerTookOrder, Action::HoldInvoicePaymentAccepted, small_order.buyer_trade_pubkey, and small_order.seller_trade_pubkey so the intent is unambiguous and future additions won’t accidentally change behavior.rust/src/api/messages.rs (2)
926-959: Test name is misleading — it documents that duplicates are not ignored.The test name
add_duplicate_message_is_ignored_in_countsuggests duplicates are ignored, but the assertionassert_eq!(msgs.len(), 2)and the comments explicitly state that both messages are stored. Consider renaming to better reflect the documented behavior.📝 Suggested test rename
- /// Verify that the message store deduplicates by id. - /// subscribe_incoming_chat relies on this to ignore echo messages. + /// Documents that the message store does NOT deduplicate by id. + /// The Dart UI layer must deduplicate incoming messages. #[tokio::test] - async fn add_duplicate_message_is_ignored_in_count() { + async fn message_store_does_not_deduplicate_by_id() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/messages.rs` around lines 926 - 959, Rename the test function add_duplicate_message_is_ignored_in_count to reflect that the Rust store does NOT deduplicate (e.g., add_duplicate_message_is_stored_twice or add_duplicate_message_is_not_deduplicated_in_store) and update the test doc comment accordingly; locate the async test function named add_duplicate_message_is_ignored_in_count (which uses message_store(), ChatMessage, and get_messages()) and change the function name and top comment so they document that two messages with the same id are both persisted and the Dart layer is responsible for deduplication.
654-665: Misleading comment contradicts the actual (correct) code pattern.The comment on lines 654-655 says "Subscribe BEFORE obtaining the receiver" but the actual code correctly obtains the receiver first (line 660), then subscribes (line 662). This matches the pattern in
subscribe_gift_wrapsand is the correct approach to avoid missing events. The comment should be updated to match reality.📝 Suggested comment fix
- // Subscribe BEFORE obtaining the receiver to avoid missing events that - // arrive between the two calls. + // Obtain the receiver BEFORE subscribing to avoid missing events that + // arrive between the two calls — same pattern as subscribe_gift_wraps. let filter = nostr_sdk::Filter::new()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/messages.rs` around lines 654 - 665, The inline comment above the receiver/subscription sequence is incorrect: update the comment near client.notifications() / client.subscribe(...) in messages.rs to state that we obtain the receiver first and then subscribe (matching the pattern used in subscribe_gift_wraps and the actual code) so the comment reflects the correct order and intent to avoid missing events.lib/features/chat/providers/chat_providers.dart (1)
231-236: Consider parallelizing trade-to-room conversion for better performance.The loop awaits each
tradeInfoToChatRoomcall sequentially. With many trades, this could be slow since each call may hit the Rust bridge and message store. UsingFuture.waitwould parallelize the conversions.⚡ Suggested parallel conversion
final chatRoomsFromTradesProvider = FutureProvider<List<ChatRoomState>>((ref) async { final trades = await ref.watch(rawTradesProvider.future); - final rooms = <ChatRoomState>[]; - for (final trade in trades) { - final room = await tradeInfoToChatRoom(trade); - if (room != null) rooms.add(room); - } + final roomFutures = trades.map(tradeInfoToChatRoom); + final results = await Future.wait(roomFutures); + final rooms = results.whereType<ChatRoomState>().toList(); rooms.sort((a, b) => b.lastMessageAt.compareTo(a.lastMessageAt)); return rooms; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/providers/chat_providers.dart` around lines 231 - 236, The current sequential loop awaiting tradeInfoToChatRoom for each element (trades -> rooms) is slow; convert it to parallel by mapping trades to a list of futures and using Future.wait to await them concurrently, then filter out nulls to produce a List<ChatRoomState>. Update the block that builds rooms (references: trades, tradeInfoToChatRoom, rooms, ChatRoomState) to collect the results of Future.wait(trades.map(...)) and then use whereType or remove nulls to get the final rooms list.
🤖 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/chat/screens/chat_room_screen.dart`:
- Around line 68-83: _loadHistory clears and replaces the _messages list which
can drop or duplicate messages that arrive via the stream; instead, fetch
history with messages_api.getMessages(tradeId: widget.orderId) and merge the
returned msgs into the existing _messages while holding mounted checks,
performing deduplication (use the same identity check as _onIncomingMessage) and
preserving message order, then set _historyLoaded = true and call
_scrollToBottom(); alternatively, start the stream listener only after merge
completes to avoid race; update the _loadHistory function and its interaction
with _onIncomingMessage to merge rather than clear-and-replace.
In `@lib/features/chat/screens/chat_rooms_screen.dart`:
- Around line 39-48: _syncRoomsFromTrades currently replaces the notifier state
with the fetched rooms which can overwrite rooms added concurrently (e.g., via
ChatRoomScreen calling upsertRoom); change _syncRoomsFromTrades to merge the
fetched list into the existing notifier state instead of calling setRooms with a
full replacement: read the current state from chatRoomsNotifierProvider, for
each room from chatRoomsFromTradesProvider.future call the notifier's upsertRoom
(or perform a dedupe-by-id merge) so new incoming rooms are preserved and
duplicates are replaced/updated rather than lost.
---
Outside diff comments:
In `@lib/features/chat/screens/chat_room_screen.dart`:
- Around line 164-177: The fallback ChatRoomState returned by _resolveRoom()
uses orderId: '' which will break matching when _buildRoomPreview() later calls
room.copyWith(); update the fallback to use the current widget.orderId instead
(i.e. set orderId: widget.orderId) so the upserted room retains the correct
orderId; change only the fallback ChatRoomState in _resolveRoom() to use
widget.orderId while keeping other default fields intact.
---
Nitpick comments:
In `@lib/features/chat/providers/chat_providers.dart`:
- Around line 231-236: The current sequential loop awaiting tradeInfoToChatRoom
for each element (trades -> rooms) is slow; convert it to parallel by mapping
trades to a list of futures and using Future.wait to await them concurrently,
then filter out nulls to produce a List<ChatRoomState>. Update the block that
builds rooms (references: trades, tradeInfoToChatRoom, rooms, ChatRoomState) to
collect the results of Future.wait(trades.map(...)) and then use whereType or
remove nulls to get the final rooms list.
In `@rust/src/api/messages.rs`:
- Around line 926-959: Rename the test function
add_duplicate_message_is_ignored_in_count to reflect that the Rust store does
NOT deduplicate (e.g., add_duplicate_message_is_stored_twice or
add_duplicate_message_is_not_deduplicated_in_store) and update the test doc
comment accordingly; locate the async test function named
add_duplicate_message_is_ignored_in_count (which uses message_store(),
ChatMessage, and get_messages()) and change the function name and top comment so
they document that two messages with the same id are both persisted and the Dart
layer is responsible for deduplication.
- Around line 654-665: The inline comment above the receiver/subscription
sequence is incorrect: update the comment near client.notifications() /
client.subscribe(...) in messages.rs to state that we obtain the receiver first
and then subscribe (matching the pattern used in subscribe_gift_wraps and the
actual code) so the comment reflects the correct order and intent to avoid
missing events.
In `@rust/src/api/orders.rs`:
- Around line 992-994: The log::warn! call inside on_peer_pubkey_received
contains extraneous whitespace from a broken string literal; replace the
multi-line/space-padded message with a single cleanly formatted string (keeping
the {order_id} interpolation) so it reads e.g. "[orders]
on_peer_pubkey_received: session not found for order={order_id}, skipping
session update — incoming subscription still spawned" to ensure consistent log
formatting and easier parsing.
- Around line 899-912: The peer pubkey selection currently uses a wildcard for
all non-BuyerTookOrder cases which is ambiguous; update the match on kind.action
so it explicitly lists the Action variants that should use
small_order.seller_trade_pubkey (e.g., Action::HoldInvoicePaymentAccepted and
any other specific seller-side actions) and keep Action::BuyerTookOrder mapped
to small_order.buyer_trade_pubkey; use an explicit catch-all arm only if you add
a clear comment explaining why it falls back to seller_trade_pubkey, referencing
the symbols kind.action, Action::BuyerTookOrder,
Action::HoldInvoicePaymentAccepted, small_order.buyer_trade_pubkey, and
small_order.seller_trade_pubkey so the intent is unambiguous and future
additions won’t accidentally change behavior.
🪄 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: 0078d78b-aa81-47e7-98ed-592162c0ddd2
📒 Files selected for processing (5)
lib/features/chat/providers/chat_providers.dartlib/features/chat/screens/chat_room_screen.dartlib/features/chat/screens/chat_rooms_screen.dartrust/src/api/messages.rsrust/src/api/orders.rs
- messages.rs: fix comment order near notifications()/subscribe() to match actual code; rename test to add_duplicate_message_is_not_deduplicated_in_store to accurately document that the Rust store does not deduplicate; fix extra whitespace in subscribe_incoming_chat log::info string - orders.rs: fix extraneous whitespace in log::warn string literal inside on_peer_pubkey_received - chat_room_screen.dart: _loadHistory now merges history into _messages with dedup-by-id + sort instead of clear-and-replace, eliminating the race where stream messages arriving during the await could be lost or duplicated; _resolveRoom fallback now uses widget.orderId instead of '' so _buildRoomPreview produces a room with the correct orderId - chat_rooms_screen.dart: _syncRoomsFromTrades now calls upsertRoom for each fetched room instead of setRooms, preserving concurrent upserts from ChatRoomScreen - chat_providers.dart: chatRoomsFromTradesProvider now uses Future.wait to resolve all rooms in parallel instead of sequentially
…eceived - Replace wildcard arm in peer-pubkey selection with explicit Action::HoldInvoicePaymentAccepted, add unreachable!() for the catch-all so future action additions don't silently pick the wrong pubkey field - Fix log::warn string: remove embedded spaces from multi-line literal so the message renders cleanly in structured log parsers
Member
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rust/src/api/messages.rs (1)
724-735: Verify the echo-filtering logic covers all scenarios.The echo filter compares
sender_pubkey(from the inner rumor'spubkeyfield) againsttrade_pubkey_hex. This assumes the sender always signs with their trade pubkey. If the sender ever signs with a different key (e.g., due to key rotation or an edge case), echoes could leak through.Consider adding a comment clarifying this assumption, or verify that the protocol guarantees the inner rumor pubkey always matches the trade pubkey.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/messages.rs` around lines 724 - 735, The echo filter currently compares the inner rumor's sender_pubkey to trade_pubkey_hex which assumes the sender always signs with their trade key; update the code to either (1) document this assumption with a clear comment near sender_pubkey/trade_pubkey_hex explaining that protocol guarantees inner.pubkey == trade pubkey (or note if key rotation is possible), and/or (2) strengthen the check by also comparing the outer/top-level event pubkey (if available) or other authoritative author identity (e.g., event.pubkey or signature-verified author) against trade_pubkey_hex so echoes signed by a different key don't bypass the filter; reference the sender_pubkey, inner, and trade_pubkey_hex symbols when making the change.lib/features/chat/screens/chat_room_screen.dart (1)
187-196: Consider using the Rust-sideisReadflag instead of local recomputation.
_buildRoomPreviewrecomputesunreadCountfrom the local_messageslist. Since messages arrive withisRead: falsefrom the Rust layer and_markRead()calls the bridge to update read status, the local recomputation may drift from the bridge's state if there's any timing mismatch.However, since
_markRead()is called immediately after receiving messages and the bridge call is awaited before any UI update, this is likely fine in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/screens/chat_room_screen.dart` around lines 187 - 196, _buildRoomPreview currently recomputes unreadCount from the local _messages list which can drift from the Rust bridge state; instead, use the Rust-provided unread/read state (e.g., the room model returned by _resolveRoom or the message's isRead flag) to populate unreadCount. Replace the local recompute (the unread variable using _messages.where(...).length) and set unreadCount to the value provided by the resolved room (room.unreadCount) or, if the room model lacks it, derive from the incoming rust_types.ChatMessage.isRead values provided by the bridge, and ensure this aligns with _markRead bridge calls.rust/src/api/orders.rs (1)
996-1001: Fix malformed log message with embedded whitespace.The log message spans multiple lines with irregular indentation that will appear in the output. Consider consolidating to a single line or using proper formatting.
✨ Suggested fix
- log::warn!( - "[orders] on_peer_pubkey_received: session not found for order={order_id}, skipping session update — incoming subscription still spawned" - ); + log::warn!( + "[orders] on_peer_pubkey_received: session not found for order={order_id}, \ + skipping session update — incoming subscription still spawned" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/orders.rs` around lines 996 - 1001, The log::warn! invocation inside on_peer_pubkey_received currently contains embedded newlines and irregular whitespace; consolidate it into a single-line message and use proper formatting placeholders instead of inline braces so it prints cleanly, e.g. replace the multi-line string with a single line like "session not found for order={}, skipping session update — incoming subscription still spawned" and pass order_id as the argument to log::warn!(...) (or use a named parameter like order_id = order_id) to ensure correct formatting.
🤖 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/chat/screens/chat_room_screen.dart`:
- Around line 187-196: _buildRoomPreview currently recomputes unreadCount from
the local _messages list which can drift from the Rust bridge state; instead,
use the Rust-provided unread/read state (e.g., the room model returned by
_resolveRoom or the message's isRead flag) to populate unreadCount. Replace the
local recompute (the unread variable using _messages.where(...).length) and set
unreadCount to the value provided by the resolved room (room.unreadCount) or, if
the room model lacks it, derive from the incoming rust_types.ChatMessage.isRead
values provided by the bridge, and ensure this aligns with _markRead bridge
calls.
In `@rust/src/api/messages.rs`:
- Around line 724-735: The echo filter currently compares the inner rumor's
sender_pubkey to trade_pubkey_hex which assumes the sender always signs with
their trade key; update the code to either (1) document this assumption with a
clear comment near sender_pubkey/trade_pubkey_hex explaining that protocol
guarantees inner.pubkey == trade pubkey (or note if key rotation is possible),
and/or (2) strengthen the check by also comparing the outer/top-level event
pubkey (if available) or other authoritative author identity (e.g., event.pubkey
or signature-verified author) against trade_pubkey_hex so echoes signed by a
different key don't bypass the filter; reference the sender_pubkey, inner, and
trade_pubkey_hex symbols when making the change.
In `@rust/src/api/orders.rs`:
- Around line 996-1001: The log::warn! invocation inside on_peer_pubkey_received
currently contains embedded newlines and irregular whitespace; consolidate it
into a single-line message and use proper formatting placeholders instead of
inline braces so it prints cleanly, e.g. replace the multi-line string with a
single line like "session not found for order={}, skipping session update —
incoming subscription still spawned" and pass order_id as the argument to
log::warn!(...) (or use a named parameter like order_id = order_id) to ensure
correct formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc3ddc09-167b-40f7-bfb4-b75e78f5587d
📒 Files selected for processing (5)
lib/features/chat/providers/chat_providers.dartlib/features/chat/screens/chat_room_screen.dartlib/features/chat/screens/chat_rooms_screen.dartrust/src/api/messages.rsrust/src/api/orders.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/chat/providers/chat_providers.dart
- orders.rs: consolidate log::warn into single clean line (no embedded spaces) - messages.rs: document echo-filter assumption — inner.pubkey is the sender's trade key per protocol; outer event pubkey is ephemeral and non-authoritative; key rotation is not supported within a session; comment explains why comparing inner sender_pubkey against trade_pubkey_hex is the correct dedup strategy - chat_room_screen.dart: _buildRoomPreview now uses room.unreadCount (bridge state) + increments by 1 for new unread peer messages instead of recomputing from local _messages list which could drift from the async markAsRead call
This was referenced Apr 8, 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
Connects the P2P trade chat end-to-end — from NIP-59 gift-wrap publishing to real-time incoming message delivery and chat room population from trades.
Previously the chat screen had optimistic local state only; the Rust bridge had all the building blocks (
send_message,get_messages,mark_as_read,on_new_message) but no wiring connected them to the live Nostr layer.Root cause
Three gaps prevented chat from working:
take_orderdid not create a session or subscribe gift-wraps — the session never existed, sosend_messagealways failed with "session not found".process_gift_wrap_rumordid not handleBuyerTookOrder/HoldInvoicePaymentAccepted— these are the protocol actions that carry the peer's trade pubkey. Without them,session.peer_pubkeywas alwaysNoneandsend_messagereturned an error.Additionally,
send_messagewas wrapping to the raw peer pubkey instead of the ECDH shared-key pubkey, which violates the Mostro P2P chat protocol spec.Changes
Rust
rust/src/api/orders.rstake_order: after a successful publish, now callssubscribe_gift_wraps+session_manager().create_session()so the session exists before the first daemon response arrives.process_gift_wrap_rumor: new arms forAction::BuyerTookOrderandAction::HoldInvoicePaymentAccepted. Both carry the counterpart's trade pubkey inSmallOrder.{buyer,seller}_trade_pubkey. On receipt, callson_peer_pubkey_received.on_peer_pubkey_received: derives the NIP-04 ECDH shared secret from(our_trade_key, peer_trade_pubkey), computes the shared-key pubkey (treating the 32-byte secret as a private scalar), stores both in the session, and spawnssubscribe_incoming_chat.Rust
rust/src/api/messages.rssubscribe_incoming_chat(pub(crate)): subscribes to kind-1059 gift-wrap events withptag == shared-key pubkey. Decrypts each rumor, ignores own echoes (sender == trade key), constructs aChatMessage, appends tomessage_store(), which fires theon_new_messagestream. Exits after 30 min of inactivity.send_message: now wraps toshared_pubkey(ECDH shared-key pubkey) instead ofpeer_pubkeydirectly, per the protocol spec.send_file: same fix.on_new_messagetrade-id isolation.Dart
lib/features/chat/providers/chat_providers.dartchatRoomsFromTradesProvider: newFutureProviderthat convertsrawTradesProviderentries with a non-emptycounterpartyPubkeyintoChatRoomStates — resolvesNymIdentity, loads last-message preview and unread count.incomingMessageProvider:StreamProvider.familywrappingmessages_api.onNewMessage. Consumed byChatRoomScreenviaref.listen.messageHistoryProvider:FutureProvider.familyovermessages_api.getMessages.Dart
lib/features/chat/screens/chat_room_screen.dart_loadHistoryseeds frommessages_api.getMessagesoninitState._markReadcallsmessages_api.markAsReadand resets unread badge._onSendcallsmessages_api.sendMessage, appends returned message.ref.listen(incomingMessageProvider)appends live incoming messages.rust_types.ChatMessageto DartChatMessageforMessageBubble.Dart
lib/features/chat/screens/chat_rooms_screen.dartChatRoomsScreen->ConsumerStatefulWidget: calls_syncRoomsFromTradeson init, populatingchatRoomsNotifierProviderfromchatRoomsFromTradesProvider.Tests
cargo test --libpasses 77/77 (0 failures, 6 ignored integration tests).New test coverage:
orders::tests::create_session_is_idempotentorders::tests::new_session_has_no_peer_keysorders::tests::peer_pubkey_with_no_session_does_not_panicapi::messages::tests::add_duplicate_message_is_ignored_in_countapi::messages::tests::on_new_message_stream_fires_for_correct_tradeProtocol compliance
This implementation follows the Mostro P2P chat spec:
Note on FRB bindings
subscribe_incoming_chatispub(crate)— it is not exposed to Dart via FRB. The Dart side drives the UI viaon_new_message(already in the generated bindings). No binding regeneration is required for the Dart changes to work.What is still deferred
ChatRoomScreen(Rustsend_fileis wired; tap handler is a stub).Closes the
Bridge no cableadoandSiempre vacioitems in the v2 feature matrix.Summary by CodeRabbit
New Features
Improvements