Step for new mostro-core gift wrap: send GiftWrap via mostro-core nip59#64
Conversation
Use mostro-core `wrap_message` in `send_dm` and remove the now-unused local NIP-59 GiftWrap builders. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR refactors gift-wrap message handling by removing ChangesGift Wrap Message Handling Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/util/dm_utils/mod.rs (3)
1529-1561: ⚖️ Poor tradeoffFallback path still double-decrypts:
rumor_cachedoesn't coverresolve_order_for_event.
resolve_order_for_eventcallsnip59::extract_rumorinternally to find the matching key, thenparse_dm_events_single(line 1532) calls it again. The newrumor_cacheis never consulted or populated by this path.One option is to have
resolve_order_for_eventreturn the already-decryptedUnwrappedGiftalongside the keys, soparse_dm_events_singlecan accept an optional pre-decrypted payload. Another is to insert the result intorumor_cachebefore callingparse_dm_events_single(though that still doesn't eliminate the second full decrypt insideparse_dm_events).Given that this is the O(n) fallback path and the redundancy pre-dates this PR, this is non-urgent, but worth a follow-up to keep caching gains consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/dm_utils/mod.rs` around lines 1529 - 1561, The fallback path double-decrypts because resolve_order_for_event(...) calls nip59::extract_rumor but parse_dm_events_single(...) re-decrypts and never consults rumor_cache; fix by either (A) extending resolve_order_for_event to also return the already-decrypted UnwrappedGift (e.g., return Option<(order_id, trade_index, trade_keys, unwrapped_gift)>) and update parse_dm_events_single to accept an optional pre-decrypted payload to skip decrypt, or (B) after resolve_order_for_event returns, insert the decrypted result into rumor_cache before calling parse_dm_events_single so parse_dm_events_single can read from the cache; update the call site in the shown block and the signatures of resolve_order_for_event and parse_dm_events_single accordingly (refer to resolve_order_for_event, parse_dm_events_single, rumor_cache, UnwrappedGift).
1426-1429: 💤 Low value
rumor_cachekey:EventIdcomponent is redundant.The cache is allocated fresh for every
RelayPoolNotification::Event, so theevent_idin the(EventId, PublicKey)key is always the same value within a given event's handling.HashMap<PublicKey, bool>is sufficient and removes the unnecessaryevent_idvariable.♻️ Proposed simplification
- let event_id = event.id; - // Cache decryptability for this event across both waiter and tracked paths. - // Keep it event-scoped to avoid unbounded growth over runtime. - let mut rumor_cache: HashMap<(EventId, PublicKey), bool> = HashMap::new(); + // Cache decryptability across both waiter and tracked paths for this event. + let mut rumor_cache: HashMap<PublicKey, bool> = HashMap::new();Then update both cache key sites:
- let key = (event_id, waiter.trade_keys.public_key()); + let key = waiter.trade_keys.public_key();- let key = (event_id, trade_keys.public_key()); + let key = trade_keys.public_key();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/dm_utils/mod.rs` around lines 1426 - 1429, The per-event cache `rumor_cache` is needlessly keyed by `(EventId, PublicKey)` because `rumor_cache` is created inside the `RelayPoolNotification::Event` handler and `event_id` is constant; change its type from `HashMap<(EventId, PublicKey), bool>` to `HashMap<PublicKey, bool>`, remove the `let event_id = event.id` if unused, and update all places that lookup/insert into `rumor_cache` to use just the `PublicKey` as the key (e.g., sites that previously used `(event_id.clone(), public_key.clone())` should use `public_key.clone()`). Ensure any variable names and imports remain consistent in functions that reference `rumor_cache`, `EventId`, and `PublicKey`.
192-225: ⚡ Quick winMerge the duplicate
PrivateGiftWrapandSignedGiftWraparms.Both arms are identical except for
signed: false/true. Consolidate them using pattern matching to eliminate ~16 lines of duplication:♻️ Proposed deduplication
- MessageType::PrivateGiftWrap => { - let message = Message::from_json(&payload) - .map_err(|e| anyhow::anyhow!("Failed to deserialize message: {e}"))?; - let identity_keys = identity_keys.unwrap_or(trade_keys); - wrap_message( - &message, - identity_keys, - trade_keys, - *receiver_pubkey, - WrapOptions { - pow, - expiration, - signed: false, - }, - ) - .await? - } - MessageType::SignedGiftWrap => { + MessageType::PrivateGiftWrap | MessageType::SignedGiftWrap => { let message = Message::from_json(&payload) .map_err(|e| anyhow::anyhow!("Failed to deserialize message: {e}"))?; let identity_keys = identity_keys.unwrap_or(trade_keys); wrap_message( &message, identity_keys, trade_keys, *receiver_pubkey, WrapOptions { pow, expiration, - signed: true, + signed: matches!(message_type, MessageType::SignedGiftWrap), }, ) .await? }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/dm_utils/mod.rs` around lines 192 - 225, Merge the duplicate arms for MessageType::PrivateGiftWrap and MessageType::SignedGiftWrap into one match arm using a combined pattern (MessageType::PrivateGiftWrap | MessageType::SignedGiftWrap) and compute the signed boolean based on which variant matched (e.g. let signed = matches!(msg_type, MessageType::SignedGiftWrap) or bind the variant and compare). Keep the existing deserialization via Message::from_json(&payload), the identity_keys = identity_keys.unwrap_or(trade_keys) line, and call wrap_message with WrapOptions { pow, expiration, signed } so only the signed flag differs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/util/dm_utils/mod.rs`:
- Around line 1529-1561: The fallback path double-decrypts because
resolve_order_for_event(...) calls nip59::extract_rumor but
parse_dm_events_single(...) re-decrypts and never consults rumor_cache; fix by
either (A) extending resolve_order_for_event to also return the
already-decrypted UnwrappedGift (e.g., return Option<(order_id, trade_index,
trade_keys, unwrapped_gift)>) and update parse_dm_events_single to accept an
optional pre-decrypted payload to skip decrypt, or (B) after
resolve_order_for_event returns, insert the decrypted result into rumor_cache
before calling parse_dm_events_single so parse_dm_events_single can read from
the cache; update the call site in the shown block and the signatures of
resolve_order_for_event and parse_dm_events_single accordingly (refer to
resolve_order_for_event, parse_dm_events_single, rumor_cache, UnwrappedGift).
- Around line 1426-1429: The per-event cache `rumor_cache` is needlessly keyed
by `(EventId, PublicKey)` because `rumor_cache` is created inside the
`RelayPoolNotification::Event` handler and `event_id` is constant; change its
type from `HashMap<(EventId, PublicKey), bool>` to `HashMap<PublicKey, bool>`,
remove the `let event_id = event.id` if unused, and update all places that
lookup/insert into `rumor_cache` to use just the `PublicKey` as the key (e.g.,
sites that previously used `(event_id.clone(), public_key.clone())` should use
`public_key.clone()`). Ensure any variable names and imports remain consistent
in functions that reference `rumor_cache`, `EventId`, and `PublicKey`.
- Around line 192-225: Merge the duplicate arms for MessageType::PrivateGiftWrap
and MessageType::SignedGiftWrap into one match arm using a combined pattern
(MessageType::PrivateGiftWrap | MessageType::SignedGiftWrap) and compute the
signed boolean based on which variant matched (e.g. let signed =
matches!(msg_type, MessageType::SignedGiftWrap) or bind the variant and
compare). Keep the existing deserialization via Message::from_json(&payload),
the identity_keys = identity_keys.unwrap_or(trade_keys) line, and call
wrap_message with WrapOptions { pow, expiration, signed } so only the signed
flag differs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0299a3f-29dd-4f23-953f-c904015f4d93
📒 Files selected for processing (3)
src/util/dm_utils/dm_helpers.rssrc/util/dm_utils/mod.rssrc/util/types.rs
💤 Files with no reviewable changes (1)
- src/util/types.rs
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this carefully and I do not see a blocker in the current diff.
What I checked:
- the
send_dmrefactor that now delegates GiftWrap creation tomostro-core::wrap_message - the remaining PDM path, to confirm user-directed private DMs still keep the existing NIP-44 flow
- the caller split between admin / trade messages, to verify the chosen key material still matches the intended sender identity
- the removal of the now-unused local GiftWrap builders and enum variants, to make sure no path was left half-migrated
- parsing/decrypt flow, to confirm this PR is only changing message construction, not the receive-side contract
The result looks coherent to me:
- signed GiftWrap messages to Mostro are now built by the shared core implementation, which is exactly where that logic should live
- the PDM branch remains separate and keeps using the local NIP-44 encryption path
identity_keys.unwrap_or(trade_keys)preserves the previous sender-key behavior for the existing callers- I do not see any leftover call site that still depends on the removed local GiftWrap helper code
Checks are green too, so this looks good from my side. Nice cleanup, and it reduces the risk of Mostrix drifting from mostro-core on NIP-59 details.
1 participant
Summary
Use mostro-core
wrap_messageinsend_dmand remove the now-unused local NIP-59 GiftWrap builders.Summary by CodeRabbit