feat(nip59): migrate gift-wrap transport to mostro-core 0.9 - #102
Conversation
Replace the hand-rolled NIP-59 wrap/unwrap in `rust/src/nostr/gift_wrap.rs`
with thin shims over `mostro_core::nip59::{wrap_message, unwrap_message,
validate_response}`, so every Mostro client shares one implementation of
seal construction, ephemeral keys, timestamp tweak, PoW, and inner-tuple
signing/verification.
Scope is limited to typed `Message` traffic with the Mostro daemon. The
Kind 14 text DM paths in `messages.rs` (P2P chat) and `disputes.rs` (admin
escalation) wrap raw `{"text": …}` JSON and stay on the legacy local
helper until `mostro-core` grows a DM variant.
Notable behavior changes on the inbound path:
- Gift-wraps now authenticate the sender: responses whose `sender` is not
the configured active Mostro pubkey are rejected and logged. Previously
the only check was "it decrypted under one of our trade keys".
- `validate_response(&msg, None)` runs on every unwrapped message,
short-circuiting `CantDo` responses centrally. `request_id` tracking is
a follow-up (see issue #101 §5).
- The per-trade and global subscriptions can now distinguish "wrap was
not addressed to this key" (`Ok(None)`) from "corrupted wrap" (`Err`).
Outbound `actions.rs` builders keep their `Result<String>` signature via
`event.as_json()` so call sites in `orders.rs` / `disputes.rs` are
unchanged; the `(Message, Option<Peer>)` tuple (de)serialization that
used to straddle the wrap and unwrap paths is gone.
Adds unit tests covering round-trip, `Ok(None)` on wrong recipient, and
PoW difficulty propagation to the outer 1059 event.
Refs: #101
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 21 seconds. ⌛ 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 (5)
WalkthroughThis PR migrates from mostro-core 0.8.0 to 0.9.0 and refactors NIP-59 gift-wrap message handling across multiple Rust modules. Changes introduce new typed Mostro message wrapping/unwrapping APIs, add daemon sender authentication checks, centralize message validation, and replace the old serialization-based flow with strongly-typed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 208294c80c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Returning early on any `validate_response` error meant `MostroCantDo` responses never reached the `Action::CantDo` arm in `dispatch_mostro_message`, so `create_order` callers waiting on a `pending_confirmations` oneshot were never unblocked — rejected orders timed out and fell back to the optimistic local-ID path, leaving phantom pending orders in the book. Only `MostroInternalErr` (malformed `request_id`, etc.) warrants a drop. `MostroCantDo` falls through so the existing CantDo handler can notify the waiting caller with the daemon's reason.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
rust/Cargo.toml (1)
14-14: Consider"0.9"for caret-compatible patch updates.
mostro-core = "0.9.0"pins to exactly>=0.9.0, <0.10.0under Cargo's default caret semantics, which is fine — but if you want to automatically pick up0.9.xbug-fix releases without editing this file, drop the patch and use"0.9". Purely a dependency-hygiene preference; no functional impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/Cargo.toml` at line 14, Update the dependency spec for mostro-core in Cargo.toml: replace the exact version string mostro-core = "0.9.0" with the caret-compatible shorthand mostro-core = "0.9" so Cargo will accept 0.9.x patch updates automatically.rust/src/nostr/gift_wrap.rs (2)
43-49: Doc overstates whatunwrap_mostro_messageguarantees.The comment claims that "sender mismatch" surfaces as
Err, but this shim just delegates tomostro_core::nip59::unwrap_message— which verifies the inner-tuple signature against the seal's sender, not against the configured Mostro pubkey. The daemon-authentication check ("sender == active mostro") is actually performed upstream inorders.rs::dispatch_mostro_message, so a wrap authored by a random key that correctly signs its own seal will still returnOk(Some(_))here. Consider tightening the wording so readers don't assume this layer enforces daemon identity.📝 Proposed doc tweak
/// Try to open an incoming Kind 1059 event using `trade_keys`. /// /// Returns `Ok(None)` only when the outer NIP-44 layer cannot be decrypted /// with the given key — the canonical "not addressed to me" signal, used by /// the global subscription to trial-decrypt across all derived trade keys. -/// Every other failure (corrupted seal, malformed rumor, bad signature, -/// sender mismatch) surfaces as `Err`. +/// Every other failure (corrupted seal, malformed rumor, bad inner-tuple +/// signature) surfaces as `Err`. Daemon-identity authentication (rejecting +/// wraps whose decrypted sender is not the active Mostro pubkey) is the +/// caller's responsibility — see `orders::dispatch_mostro_message`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/nostr/gift_wrap.rs` around lines 43 - 49, Docstring for unwrap_mostro_message overclaims sender verification: it does not enforce that the seal's sender matches the configured/active Mostro key but simply delegates to mostro_core::nip59::unwrap_message which only verifies the inner signature against the seal's sender. Update the comment for unwrap_mostro_message to remove or soften the claim that "sender mismatch surfaces as Err" and instead state that only the inner signature vs. seal sender is checked here and that daemon-authentication (ensuring sender == active Mostro) is performed upstream in orders.rs::dispatch_mostro_message.
185-213: PoW test is mildly flaky and over-asserts leading zeros.With
difficulty = 4this is usually fast, but PoW mining time is probabilistic and the test has no timeout — on a slow CI runner a bad random seed could occasionally stretch wall time. Also, the leading-zero counter short-circuits thecontinuelogic correctly, but you don't need to hand-roll it:event.id.as_bytes()(or the existingto_bytes()) fed through a standardleading_zeroshelper is sufficient. Not blocking; just something to revisit if CI ever goes flaky here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/nostr/gift_wrap.rs` around lines 185 - 213, The test pow_is_applied_to_outer_event can hang on slow CI and uses a hand-rolled leading-zero byte loop; wrap the async mining call (wrap_mostro_message) in a tokio::time::timeout to bound wall time and fail fast, and replace the manual byte loop over event.id.to_bytes() with a standard aggregation of byte.leading_zeros() (e.g., sum the leading_zeros for the byte slice until a non-zero byte) to compute leading_zero_bits before asserting it >= difficulty; update the test function pow_is_applied_to_outer_event to use timeout and the new leading-zero computation using event.id.to_bytes() or event.id.as_bytes().rust/src/api/orders.rs (3)
1063-1066: Unused bindingidinPaymentRequestdestructure.
idis bound but never read in this debug string — silences a clippyunused_variableswarning. Consider_id(or just useprwithout destructuring).📝 Proposed tweak
- Some(mostro_core::message::Payload::PaymentRequest(id, pr, amt)) => format!( - "PaymentRequest(id={id:?}, invoice_len={}, amount={amt:?})", + Some(mostro_core::message::Payload::PaymentRequest(_id, pr, amt)) => format!( + "PaymentRequest(invoice_len={}, amount={amt:?})", pr.len() ),🤖 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 1063 - 1066, The match arm for mostro_core::message::Payload::PaymentRequest binds `id` but never uses it, causing an unused binding warning; change the pattern to ignore the id (e.g., use `_id` or `_`) in the destructure for PaymentRequest(id, pr, amt) and keep the existing format string that uses pr.len() and amt, or alternatively stop destructuring `id` entirely (PaymentRequest(_, pr, amt)) so the compiler warning is silenced while leaving `pr` and `amt` intact.
975-986: Ok(None) on the per-trade path warrantsdebug, notwarn.The per-trade relay filter already matches on this
p-tag, soOk(None)here means the outer NIP-44 layer decrypted "not for us" despite the tag match — most commonly a stale relay echo for a recycled key, or ap-tag collision from another client. Logging every occurrence atwarnwill produce noisy client logs during normal reconnects/replays. Suggest dropping todebugand keepingwarn/errorfor the decrypt-error branch below.🤖 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 975 - 986, Change the OK(None) branch to log at debug rather than warn: replace the crate::api::logging::blog_warn call in the Ok(None) arm with crate::api::logging::blog_debug (keeping the same message and trade_pubkey_hex formatting), while leaving the Err(e) branch as a warn; this reduces noisy warnings when the per-trade filter already filtered "not for us" cases.
1020-1040: Sender-auth gate: good addition; consider logging sender hex atdebuginstead ofwarnfor UX.Rejecting wraps whose decrypted seal author is not the active Mostro pubkey is the right call and closes a real attack surface (previously anything decrypting under the trade key was trusted). Two small notes:
- If the user switches the active Mostro pubkey override (
set_active_mostro_pubkey) mid-session, in-flight responses from the previous daemon will now be dropped with awarn!— expected, but worth documenting near the config's override setter so operators don't chase it as a bug.- The
Err(e)branch at 1034-1039 covers a misconfigured active pubkey — this should probably be louder thanblog_warn(anerror!or surfaced through the UI), since every inbound response will silently fail until the user fixes their settings.🤖 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 1020 - 1040, Change the sender-mismatch log from warn to a debug-level message and include the sender hex there (reference: the match on nostr_sdk::PublicKey::from_hex(&crate::config::active_mostro_pubkey()) comparing expected == sender and the blog_warn call that logs sender/expected/trade_pubkey_hex); also make the Err(e) branch louder by replacing blog_warn with a blog_error (or equivalent error-level reporting) and include the error detail e in the message so a misconfigured active_mostro_pubkey() is prominently reported; additionally add a short comment near the set_active_mostro_pubkey override setter documenting that in-flight responses from the prior daemon will be dropped when the override changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rust/Cargo.toml`:
- Line 14: Update the dependency spec for mostro-core in Cargo.toml: replace the
exact version string mostro-core = "0.9.0" with the caret-compatible shorthand
mostro-core = "0.9" so Cargo will accept 0.9.x patch updates automatically.
In `@rust/src/api/orders.rs`:
- Around line 1063-1066: The match arm for
mostro_core::message::Payload::PaymentRequest binds `id` but never uses it,
causing an unused binding warning; change the pattern to ignore the id (e.g.,
use `_id` or `_`) in the destructure for PaymentRequest(id, pr, amt) and keep
the existing format string that uses pr.len() and amt, or alternatively stop
destructuring `id` entirely (PaymentRequest(_, pr, amt)) so the compiler warning
is silenced while leaving `pr` and `amt` intact.
- Around line 975-986: Change the OK(None) branch to log at debug rather than
warn: replace the crate::api::logging::blog_warn call in the Ok(None) arm with
crate::api::logging::blog_debug (keeping the same message and trade_pubkey_hex
formatting), while leaving the Err(e) branch as a warn; this reduces noisy
warnings when the per-trade filter already filtered "not for us" cases.
- Around line 1020-1040: Change the sender-mismatch log from warn to a
debug-level message and include the sender hex there (reference: the match on
nostr_sdk::PublicKey::from_hex(&crate::config::active_mostro_pubkey()) comparing
expected == sender and the blog_warn call that logs
sender/expected/trade_pubkey_hex); also make the Err(e) branch louder by
replacing blog_warn with a blog_error (or equivalent error-level reporting) and
include the error detail e in the message so a misconfigured
active_mostro_pubkey() is prominently reported; additionally add a short comment
near the set_active_mostro_pubkey override setter documenting that in-flight
responses from the prior daemon will be dropped when the override changes.
In `@rust/src/nostr/gift_wrap.rs`:
- Around line 43-49: Docstring for unwrap_mostro_message overclaims sender
verification: it does not enforce that the seal's sender matches the
configured/active Mostro key but simply delegates to
mostro_core::nip59::unwrap_message which only verifies the inner signature
against the seal's sender. Update the comment for unwrap_mostro_message to
remove or soften the claim that "sender mismatch surfaces as Err" and instead
state that only the inner signature vs. seal sender is checked here and that
daemon-authentication (ensuring sender == active Mostro) is performed upstream
in orders.rs::dispatch_mostro_message.
- Around line 185-213: The test pow_is_applied_to_outer_event can hang on slow
CI and uses a hand-rolled leading-zero byte loop; wrap the async mining call
(wrap_mostro_message) in a tokio::time::timeout to bound wall time and fail
fast, and replace the manual byte loop over event.id.to_bytes() with a standard
aggregation of byte.leading_zeros() (e.g., sum the leading_zeros for the byte
slice until a non-zero byte) to compute leading_zero_bits before asserting it >=
difficulty; update the test function pow_is_applied_to_outer_event to use
timeout and the new leading-zero computation using event.id.to_bytes() or
event.id.as_bytes().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a360266-59c0-46dd-a248-caaf000620b5
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
rust/Cargo.tomlrust/src/api/orders.rsrust/src/mostro/actions.rsrust/src/nostr/gift_wrap.rs
- Cargo.toml: use short `mostro-core = "0.9"` for consistency with the
rest of the deps (`serde = "1"`, `bip32 = "0.5"`, etc.); semantics
unchanged (`^0.9` == `^0.9.0`).
- orders.rs: demote per-trade `Ok(None)` from `warn` to `debug` — the
p-tag filter has already narrowed to our key, so this only fires on
unsolicited wraps that don't decrypt, which a hostile relay could
cheaply spam.
- config.rs: document that `set_active_mostro_pubkey` override drops
in-flight responses from the previous daemon (they fail the sender
check in `dispatch_mostro_message`).
- gift_wrap.rs: tighten the `unwrap_mostro_message` docstring — the
helper verifies inner-tuple signature against the seal author, not
against the active Mostro pubkey; daemon authentication is upstream.
- gift_wrap.rs: wrap the PoW mining test in `tokio::time::timeout(30s)`
and use `u32` for the leading-zero count so a regression can't hang
CI or silently overflow.
Intentionally skipped:
- "unused `id` binding in `PaymentRequest(id, pr, amt)` match" — `id`
is consumed via named-capture in the `{id:?}` format string; compiler
emits no warning.
- "sender-mismatch warn → debug" — any peer can wrap a message to our
trade key, so the check catches spoofed daemon responses, not just
override races. Keeping `warn` as a security signal.
The local helper was renamed to wrap_for_mostro during the migration to avoid an imagined collision with mostro_core::nip59::wrap_message. Since that symbol is not imported into this module (only mostro_core::message types and the crate::nostr::gift_wrap module are), there is no collision — restore the shorter name.
1 participant
Replace the hand-rolled NIP-59 wrap/unwrap in
rust/src/nostr/gift_wrap.rswith thin shims overmostro_core::nip59::{wrap_message, unwrap_message, validate_response}, so every Mostro client shares one implementation of seal construction, ephemeral keys, timestamp tweak, PoW, and inner-tuple signing/verification.Scope is limited to typed
Messagetraffic with the Mostro daemon. The Kind 14 text DM paths inmessages.rs(P2P chat) anddisputes.rs(admin escalation) wrap raw{"text": …}JSON and stay on the legacy local helper untilmostro-coregrows a DM variant.Notable behavior changes on the inbound path:
senderis not the configured active Mostro pubkey are rejected and logged. Previously the only check was "it decrypted under one of our trade keys".validate_response(&msg, None)runs on every unwrapped message, short-circuitingCantDoresponses centrally.request_idtracking is a follow-up (see issue Migrate NIP-59 gift-wrap transport to mostro-core 0.9 (wrap_message / unwrap_message / validate_response) #101 §5).Ok(None)) from "corrupted wrap" (Err).Outbound
actions.rsbuilders keep theirResult<String>signature viaevent.as_json()so call sites inorders.rs/disputes.rsare unchanged; the(Message, Option<Peer>)tuple (de)serialization that used to straddle the wrap and unwrap paths is gone.Adds unit tests covering round-trip,
Ok(None)on wrong recipient, and PoW difficulty propagation to the outer 1059 event.Refs: #101
Summary by CodeRabbit
Chores
mostro-coredependency from 0.8.0 to 0.9.0.Bug Fixes