feat(nip59): migrate gift-wrap transport to mostro-core 0.10 - #103
Conversation
mostro-core 0.10 reshapes `nip59::wrap_message` to take the identity and trade keys as separate arguments, matching the key-management protocol at https://mostro.network/protocol/key_management.html: - identity_keys (BIP-32 index 0, long-lived) sign the Seal (Kind 13) so the Mostro node can accumulate reputation under a stable pubkey. - trade_keys (BIP-32 index ≥1, per-order) author the rumor (Kind 1) and produce the inner tuple signature. Callers that opt out of reputation (privacy mode) pass `trade_keys` for both parameters — the "full-privacy" variant described in the protocol doc. Changes - Bump rust/Cargo.toml: mostro-core 0.9 → 0.10. - Update `wrap_mostro_message` and every action builder in `rust/src/mostro/actions.rs` to accept `identity_keys` + `trade_keys`. - Add `api::identity::get_transport_identity_keys(&trade_keys)` — returns the master identity keys in reputation mode, or a clone of `trade_keys` when `api::reputation::get_privacy_mode()` is on. This is the single entry point callers use to apply the runtime privacy toggle. - Wire the helper through every dispatcher: `create_order`, `take_order`, `send_invoice`, `send_fiat_sent`, `release_order`, `cancel_order`, `open_dispute`, and `submit_rating`. - Destructure the new `identity` field on `UnwrappedMessage` in the daemon-response dispatcher; authentication continues to match on `sender` because the current daemon reuses its pubkey for both. - Refresh gift-wrap unit tests: roundtrip asserts both `sender` and `identity`, and a new `full_privacy_mode_reuses_trade_key_as_identity` case covers the one-key path.
WalkthroughThis PR implements a dual-key model for NIP-59 gift-wrapping in Mostro transactions. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Order/Dispute Caller
participant KeyResolver as get_transport_identity_keys
participant PrivacyMode as Privacy Mode Check
participant ActionBuilder as Mostro Action Builder
participant GiftWrap as wrap_mostro_message
Caller->>KeyResolver: resolve identity keys from sender_keys
KeyResolver->>PrivacyMode: check privacy mode enabled?
alt Privacy Mode ON
PrivacyMode-->>KeyResolver: return trade_keys (cloned)
else Privacy Mode OFF
PrivacyMode-->>KeyResolver: return active identity keys
end
KeyResolver-->>Caller: identity_keys
Caller->>ActionBuilder: call action with identity_keys + trade_keys
ActionBuilder->>GiftWrap: pass both identity_keys and trade_keys
GiftWrap->>GiftWrap: identity_keys sign seal (Kind 1059)
GiftWrap->>GiftWrap: trade_keys author rumor (Kind 1)
GiftWrap-->>ActionBuilder: wrapped message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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: c7eb902427
ℹ️ 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".
| sender, | ||
| identity: _, |
There was a problem hiding this comment.
dispatch_mostro_message discards UnwrappedMessage.identity and continues trusting sender, but after the 0.10 transport split sender is just the rumor pubkey and may differ from the seal signer. Because Mostro responses commonly have no inner signature, an attacker can send a validly sealed wrap from their own key while setting rumor sender to the configured Mostro pubkey; this path will pass the current auth check and route a forged protocol message. Compare active_mostro_pubkey against identity (and only treat sender as authoritative when a verified inner signature is present).
Useful? React with 👍 / 👎.
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
rust/src/api/orders.rs (1)
686-734:⚠️ Potential issue | 🟡 Minor
take_order: identity-key failure returnsOk(trade)while other failures log-and-continue.The new branch at Lines 697–709 returns
Ok(trade)early whenget_transport_identity_keysfails, but every other failure mode in thistake_orderbody (invalid mostro pubkey, action build failure, publish failure, sender-key fetch failure) is handled withlog::warn!+ fall-through, ending inOk(trade)at Line 785 anyway. Two side-effects to note:
- The early return skips the
subscribe_single_order/subscribe_gift_wraps/create_sessionsetup that happens after a successful publish — fine, since dispatch failed — but it's also reachable when no dispatch was attempted, so the caller can't distinguish "dispatched and may eventually succeed" from "never dispatched".derive_trade_key()at Line 664 already incremented the persistedtrade_key_indexbefore this point; on this failure path that index is consumed without a Mostro message ever being signed under it. That's not a correctness bug (the next derivation will still be unique), just minor key-index waste under repeated transient failures.Consider unifying with the surrounding pattern (
log::error!+ fall-through, allowing the emptymatcharm to drop through toOk(trade)), or — better — convertingtake_orderto surface aResult::Errfor actual dispatch failures so callers can distinguish "took locally only" from "took and dispatched". The latter is a larger change and can be deferred.🤖 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 686 - 734, In take_order, the get_transport_identity_keys failure branch returns early with return Ok(trade) which diverges from the surrounding fall-through pattern and can hide dispatch-vs-non-dispatch outcomes and waste a derived trade key; instead, change the Err(e) arm in the get_transport_identity_keys match to log::error! (matching the existing style used elsewhere) and drop through without returning so execution continues to the common end (allowing subscribe_single_order / subscribe_gift_wraps / create_session behavior to remain consistent); locate the match on crate::api::identity::get_transport_identity_keys and replace the early return with a logged error only.
🧹 Nitpick comments (2)
rust/src/mostro/actions.rs (1)
254-303: Consider grouping the action-builder parameters into a smallWrapCtxstruct.Every builder now threads
(identity_keys, trade_keys, mostro_pubkey, order_id, trade_index, …)through, andtake_order_implalready needed#[allow(clippy::too_many_arguments)]for the same reason. A tiny context type — for example:pub struct WrapCtx<'a> { pub identity_keys: &'a Keys, pub trade_keys: &'a Keys, pub mostro_pubkey: &'a PublicKey, pub trade_index: u32, }would (a) drop the
too_many_argumentsallow, (b) make call sites at the API boundary read more naturally, and (c) make it harder to accidentally swapidentity_keys/trade_keyswhen both are&Keys. Optional and can be deferred — the current code is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/mostro/actions.rs` around lines 254 - 303, The functions take_order_impl and simple_action currently accept many positional parameters (identity_keys, trade_keys, mostro_pubkey, order_id, trade_index, …) which is error-prone and required a clippy allow; introduce a small context struct (e.g., WrapCtx with fields identity_keys: &Keys, trade_keys: &Keys, mostro_pubkey: &PublicKey, trade_index: u32) and refactor these functions to take &WrapCtx plus the remaining per-call args (order_id, amount/ln_address, action) so you can remove #[allow(clippy::too_many_arguments)], simplify call sites, and avoid accidental parameter swaps when calling Message::new_order / wrap_message.rust/Cargo.toml (1)
14-14: Patch version pinning consideration for mostro-core.
mostro-core = "0.10"currently resolves to^0.10.0. SinceCargo.lockis already committed, immediate reproducibility is ensured. However, consider that this is a library: downstream consumers will use their ownCargo.lock, so pinning to a specific patch (e.g.,"=0.10.0") would protect this project's builds against hypothetical breaking changes in future 0.10.x releases. Currently, only0.10.0exists in the 0.10.x series, so the risk is speculative at present. If adapting to breaking APIs in 0.10 was significant work for this PR, pinning to"0.10.0"is reasonable as a safeguard; otherwise, the current^0.10constraint is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/Cargo.toml` at line 14, The dependency declaration mostro-core = "0.10" in Cargo.toml should be changed to a patch-pinned constraint if you want to protect library users from future 0.10.x regressions; update the entry for mostro-core to use an exact patch pin (e.g., mostro-core = "=0.10.0") to lock to the current patch, or leave it as-is if you prefer permissive semver; modify the Cargo.toml dependency line for mostro-core accordingly and commit the change.
🤖 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 `@rust/src/api/orders.rs`:
- Around line 686-734: In take_order, the get_transport_identity_keys failure
branch returns early with return Ok(trade) which diverges from the surrounding
fall-through pattern and can hide dispatch-vs-non-dispatch outcomes and waste a
derived trade key; instead, change the Err(e) arm in the
get_transport_identity_keys match to log::error! (matching the existing style
used elsewhere) and drop through without returning so execution continues to the
common end (allowing subscribe_single_order / subscribe_gift_wraps /
create_session behavior to remain consistent); locate the match on
crate::api::identity::get_transport_identity_keys and replace the early return
with a logged error only.
---
Nitpick comments:
In `@rust/Cargo.toml`:
- Line 14: The dependency declaration mostro-core = "0.10" in Cargo.toml should
be changed to a patch-pinned constraint if you want to protect library users
from future 0.10.x regressions; update the entry for mostro-core to use an exact
patch pin (e.g., mostro-core = "=0.10.0") to lock to the current patch, or leave
it as-is if you prefer permissive semver; modify the Cargo.toml dependency line
for mostro-core accordingly and commit the change.
In `@rust/src/mostro/actions.rs`:
- Around line 254-303: The functions take_order_impl and simple_action currently
accept many positional parameters (identity_keys, trade_keys, mostro_pubkey,
order_id, trade_index, …) which is error-prone and required a clippy allow;
introduce a small context struct (e.g., WrapCtx with fields identity_keys:
&Keys, trade_keys: &Keys, mostro_pubkey: &PublicKey, trade_index: u32) and
refactor these functions to take &WrapCtx plus the remaining per-call args
(order_id, amount/ln_address, action) so you can remove
#[allow(clippy::too_many_arguments)], simplify call sites, and avoid accidental
parameter swaps when calling Message::new_order / wrap_message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e6e9336-0b7d-404a-bfeb-1ead8f48b506
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
rust/Cargo.tomlrust/src/api/disputes.rsrust/src/api/identity.rsrust/src/api/orders.rsrust/src/api/reputation.rsrust/src/mostro/actions.rsrust/src/nostr/gift_wrap.rs
1 participant
Summary
mostro-core 0.10 reshapes
nip59::wrap_messageto take the identity key and the trade key as separate arguments, matching the key-management protocol at https://mostro.network/protocol/key_management.html:identity_keys(BIP-32 index 0, long-lived) sign the Seal (Kind 13) so the node can accumulate reputation under a stable pubkey.trade_keys(BIP-32 index ≥1, per-order) author the rumor (Kind 1) and produce the inner tuple signature.Callers that opt out of reputation (privacy mode) pass
trade_keysfor both parameters — the "full-privacy" variant described in the protocol doc.Changes
rust/Cargo.toml:mostro-core0.9 → 0.10.rust/src/nostr/gift_wrap.rs—wrap_mostro_messagenow takesidentity_keysandtrade_keysseparately, forwarding both tomostro_core::nip59::wrap_message. Roundtrip test asserts bothsenderandidentity; newfull_privacy_mode_reuses_trade_key_as_identitycase covers the one-key path.rust/src/mostro/actions.rs— every public action builder (new_order,take_buy,take_sell,fiat_sent,release,cancel,dispute,rate_user,add_invoice) plus the internalwrap_messagehelper gain theidentity_keys: &Keysparameter.rust/src/api/identity.rs— newget_transport_identity_keys(&trade_keys)helper returns the master identity keys in reputation mode, or a clone oftrade_keyswhenapi::reputation::get_privacy_mode()is on. This is the single entry point every dispatcher uses to apply the runtime privacy toggle.rust/src/api/orders.rs,rust/src/api/disputes.rs,rust/src/api/reputation.rs) — resolve identity keys via the new helper before building each action.rust/src/api/orders.rs(receive path) — destructure the newidentityfield onUnwrappedMessage; authentication continues to match onsenderbecause the current daemon reuses its pubkey for both, but the change is ready for a node that later splits them.Test plan
cargo test --lib— 81 passed, 0 failed (newfull_privacy_mode_reuses_trade_key_as_identitypasses; existing roundtrip and PoW tests updated).cargo build— clean.cargo clippy --lib --all-targets— no new warnings introduced by this PR (remaining warnings inapi/settings.rsare pre-existing onmain).privacy_mode = false) and verify the daemon links the trade to the identity key.set_privacy_mode(true)) and verify the seal pubkey matches the rumor pubkey.References
Summary by CodeRabbit
Chores
New Features