feat: shortened time interval to update keys list of subscription#42
feat: shortened time interval to update keys list of subscription#42arkanoider wants to merge 12 commits intomainfrom
Conversation
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or 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:
WalkthroughRefactors DM flow from polling to subscription-driven. Adds Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Order as OrderService
participant SubMgr as SubManager
participant Relay as Relay
participant Parser as DMParser
participant Notif as Notifier
rect rgba(100,200,100,0.5)
UI->>Order: send subscribe command (OrderDmSubscriptionCmd::Subscribe)
Order->>SubMgr: derive trade pubkey, subscribe GiftWrap (limit 0)
SubMgr->>Relay: subscribe
Relay-->>SubMgr: subscription confirmed (subscription_id)
SubMgr->>SubMgr: map subscription_id -> (order_id, trade_index)
end
rect rgba(100,150,200,0.5)
Relay->>SubMgr: GiftWrap event (subscription_id, payload)
SubMgr->>Parser: decrypt/parse DM for associated trade key
Parser-->>SubMgr: parsed DM (action, timestamp, payload)
SubMgr->>Order: handle_trade_dm_for_order(parsed DM)
Order->>Notif: send MessageNotification / increment pending
alt terminal status detected
Order->>SubMgr: unsubscribe(subscription_id) and cleanup
SubMgr->>Relay: unsubscribe
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/util/dm_utils/mod.rs`:
- Around line 256-264: The logic marks a same-second different-action event as
new via is_new_message (involving existing_message and
existing.message.get_inner_message_kind().action), but later sort_by(timestamp)
followed by dedup_by_key(order_id) can collapse distinct actions into one row
and drop the new message before it reaches messages; fix by making the de-dup
step consider action (or a composite key) when sorting/deduping so rows with the
same order_id but different action at the same timestamp are preserved—update
the sort/dedup logic that currently uses sort_by(timestamp) +
dedup_by_key(order_id) to use a key that includes action (and timestamp if
needed) or filter earlier to ensure messages retains the new-action row.
- Around line 248-269: The code currently holds messages.lock() while later
locking pending_notifications, which can deadlock with other code that locks
them in the opposite order; to fix, avoid nested locking by acquiring the
minimal data needed from messages while holding messages_lock (e.g., clone
existing_message.timestamp and existing_action via
existing.message.get_inner_message_kind().action.clone()), then drop
messages_lock and compute is_new_message, and only after that lock
pending_notifications to increment it when needed; reference symbols: messages,
pending_notifications, existing_message, get_inner_message_kind(), timestamp,
and action.
- Around line 333-336: The background task and wait_for_dm() both consume the
same client.notifications() stream and currently accept the first
RelayPoolNotification::Event without verifying which subscription it belongs to,
causing cross-order interference; update wait_for_dm() to filter notifications
by matching either the subscription_id from RelayPoolNotification::Event against
the subscription ID returned by client.subscribe() or by checking the event's
pubkey against trade_keys.public_key(), and ensure the background task when
storing/using subscribed_pubkeys and pubkey_to_order consistently uses the same
subscription IDs so wait_for_dm() only returns events for the expected
subscription (reference Client::subscribe(), client.notifications(),
wait_for_dm(), RelayPoolNotification::Event, subscription_id, and
trade_keys.public_key()).
- Around line 365-382: The code is using event.pubkey to correlate incoming Gift
Wrap events but NIP-59 uses a one-time pubkey so that will never match the
stored trade pubkey; change the subscription correlation to use the
subscription_id returned by client.subscribe() instead: capture the
subscription_id from client.subscribe(...) where you currently call it (near the
block that inserts into subscribed_pubkeys and pubkey_to_order), store the
mapping from subscription_id -> (order_id, trade_index) (replacing or augmenting
pubkey_to_order), and update the RelayPoolNotification::Event handler to extract
and use subscription_id (instead of event.pubkey) to look up the trade tuple;
also stop using the temporary one-time pubkey as the lookup key and remove its
use in the incoming-event path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43f37fd7-3a8c-4837-a8d1-ecb0da8596b0
📒 Files selected for processing (1)
src/util/dm_utils/mod.rs
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Concept
Good architectural change: moving from polling (fetch every 2s) to push-based subscriptions via client.notifications(). This is more efficient and reduces latency for incoming DMs.
CI Status
- Latest commit (3d99983): Passing
Issues Found by CodeRabbit (Still Unresolved)
1. Lock Inversion Deadlock Risk
Lines 248-268: Code holds messages.lock() while acquiring pending_notifications.lock(). If any other code path acquires these in reverse order, deadlock occurs.
Fix: Drop messages_lock before taking pending_notifications:
let is_new_message = {
let messages_lock = messages.lock().unwrap();
let existing_message = messages_lock.iter()...
// compute is_new_message
}; // messages_lock dropped here
if is_new_message {
let mut pending = pending_notifications.lock().unwrap();
*pending += 1;
}
let mut messages_lock = messages.lock().unwrap();
messages_lock.push(order_message);2. Dedup Logic Inconsistency
Lines 257-263 + 283-285:
is_new_messagetreatstimestamp == existing && action != existing_actionas new- But
dedup_by_key(order_id)ignores action, potentially removing the newly-added message
Fix:
if is_new_message {
messages_lock.retain(|m| m.order_id != Some(order_id));
messages_lock.push(order_message);
messages_lock.sort_by(|a, b| b.timestamp.cmp(&a.timestamp));
}Minor Notes
-
Subscription cleanup: When an order is removed from
active_order_trade_indices, its pubkey remains insubscribed_pubkeysandpubkey_to_order. Consider periodic cleanup or removal when order completes. -
3s interval: The PR title says "shortened time interval" but 3s for subscription housekeeping seems reasonable. Document why this value was chosen.
Summary
| Item | Status |
|---|---|
| Architecture (push vs poll) | Good |
| CI | Passing |
| Lock inversion risk | Needs fix |
| Dedup logic | Needs fix |
| Subscription cleanup | Nice-to-have |
Verdict: Request Changes — Fix the two concurrency issues flagged by CodeRabbit before merge.
arkanoider
force-pushed
the
subscription-messages
branch
from
067e407 to
939da2b
Compare
Made-with: Cursor # Conflicts: # .gitignore # src/ui/key_handler/confirmation.rs # src/ui/key_handler/mod.rs # src/util/dm_utils/mod.rs
1 participant
Summary
Replace timed polling for order trade DMs with Nostr relay subscriptions so incoming Gift Wrap messages are pushed via
client.notifications()instead of being fetched on an interval.What changed
Background listener (
listen_for_order_messages)notifications.recv()loop forRelayPoolNotification::Event.nostr_sdk::Kind::GiftWrap), matched to the trade pubkey.active_order_trade_indicesand callsclient.subscribe()for any trade pubkey not yet insubscribed_pubkeys.pubkey_to_ordermaps relay event pubkey →(order_id, trade_index)for decryption and UI handling (parse_dm_events,handle_trade_dm_for_order).The interval is subscription housekeeping only (discover new active trade keys), not message polling.
Request/response flows (
wait_for_dm)Unchanged contract, documented in code:
client.notifications().send_dmfuture (e.g. new order, take order, add invoice).So immediate confirmations (e.g.
send_new_order) are not subject to the background listener’s tick delay.Technical notes
pubkey(trade_pubkey) + kind(GiftWrap)withlimit(0)→ effectively live traffic after subscribe; no historical backfill on that filter.subscribeattempts roll backsubscribed_pubkeysso the next tick can retry.Risks / limitations
active_order_trade_indicesand the next subscription tick where a background Gift Wrap could be missed (same as any “subscribe after activation” design without backfill).wait_for_dmpaths explicitly avoid that for the paired request/response.How to test
wait_for_dm).cargo fmt --all,cargo clippy --all-targets --all-features,cargo test,cargo build.Docs
docs/MESSAGE_FLOW_AND_PROTOCOL.md(section on waiting for response /wait_for_dm) for the request/response side of the flow.Summary by CodeRabbit
New Features
Bug Fixes
Performance