refactor: migrate gift-wrap to mostro-core 0.9.1 nip59 module#164
Merged
refactor: migrate gift-wrap to mostro-core 0.9.1 nip59 module#164
Conversation
Bump mostro-core to 0.9.1 and nostr-sdk to 0.44.1, then replace the
local NIP-59 implementation with the centralized
`wrap_message` / `unwrap_message` / `validate_response` API exposed
by `mostro_core::nip59`.
- send_dm now delegates to wrap_message; identity_keys parameter is
dropped because nostr-sdk 0.44 enforces rumor.pubkey == seal.pubkey
(SenderMismatch). The single signer drives rumor authorship, seal
signing, and the inner tuple signature.
- Admin flows (storage::admin_send_dm, take_dispute) now sign the
whole pipeline with admin_keys, aligning with how
send_admin_gift_wrap_dm already worked for plain DMs.
- parse_dm_events uses unwrap_message; print_dm_events uses
validate_response, with one documented exception for `NewOrder`
pushes that arrive without a request_id (range-trade child).
- Drops gift_wrap_from_seal_with_pow, create_gift_wrap_event,
send_gift_wrap_dm{,_internal}, send_admin_gift_wrap_dm,
determine_message_type, create_expiration_tags, and the
MessageType enum, all subsumed by the new API.
- NIP-17 `PrivateDirectMessage` (kind 14) and the per-dispute
shared-key chat transport are out of scope and left untouched.
Knock-on changes from the dependency bump: rename
NOSTR_REPLACEABLE_EVENT_KIND to NOSTR_ORDER_EVENT_KIND (same value
38383, preserves derived BIP32 keys), Timestamp::as_u64 →
as_secs, and add the new `initiator` / `solver_pubkey` fields
to RestoredDisputesInfo in tests.
Tests: 4 new wiring unit tests in messaging.rs (wrap→unwrap
roundtrip, SECRET semantics, PoW, wrong-keys → None) and 2 new
integration tests in tests/parser_dms.rs covering the
wrap_message → parse_dm_events handshake.
Closes #163
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThe PR migrates gift-wrap DM handling to use mostro-core 0.9.1's centralized wrap_message/unwrap_message/validate_response APIs. Dependencies are bumped (mostro-core 0.6.56→0.9.1, nostr-sdk 0.43.0→0.44.1), and local gift-wrap construction/unwrapping logic is replaced. Key derivation paths updated to use NOSTR_ORDER_EVENT_KIND instead of NOSTR_REPLACEABLE_EVENT_KIND. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Client as nostr-sdk Client
participant MostroCore as mostro-core 0.9.1
participant Mostro as Mostro Node
participant Receiver as Receiver
Note over CLI,Receiver: Send Flow (New)
CLI->>CLI: Prepare message payload
CLI->>MostroCore: wrap_message(payload, WrapOptions)
MostroCore->>MostroCore: Create rumor, seal, gift-wrap
MostroCore-->>CLI: Wrapped GiftWrap event
CLI->>Client: Publish GiftWrap
Client->>Mostro: Send to relay
Note over CLI,Receiver: Receive Flow (New)
Mostro-->>Client: GiftWrap event
Client-->>CLI: Received event
CLI->>MostroCore: unwrap_message(event, receiver_keys)
MostroCore->>MostroCore: Decrypt & extract rumor
MostroCore-->>CLI: UnwrappedMessage
CLI->>MostroCore: validate_response(message, request_id)
MostroCore-->>CLI: Validation result
CLI->>CLI: Parse & handle response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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: 0c4551d6db
ℹ️ 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".
Comment on lines
330
to
333
| pub async fn send_dm( | ||
| client: &Client, | ||
| identity_keys: Option<&Keys>, | ||
| trade_keys: &Keys, | ||
| signer_keys: &Keys, | ||
| receiver_pubkey: &PublicKey, |
There was a problem hiding this comment.
Collapsing send_dm to a single signer_keys identity removes the old ability to publish account-scoped requests with an identity sender while still using trade keys for the message flow, and the call sites for execute_restore/execute_last_trade_index now send with ctx.trade_keys. On Mostro’s request handlers, user lookup is keyed by the sender pubkey, so these requests can be treated as unknown users (or invalid trade-index state) when the trade key is not the account identity key, which breaks restore/last-trade-index recovery flows.
Useful? React with 👍 / 👎.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Restore and LastTradeIndex are account-scoped queries: Mostro looks up the user by the sender pubkey on the gift wrap, so signing with a per-trade key leaves the request either resolving to an unknown user or to stale trade-index state — either way, recovery flows silently return nothing. Revert these two call sites to `identity_keys` as the signer, and route `wait_for_dm` / `parse_dm_events` on the same key so the response (which Mostro addresses back to the sender pubkey) is the one we subscribe to and can decrypt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/util/messaging.rs (1)
100-104: Reuseparse_pow_env()here to avoid duplicated POW parsing.
build_custom_wrap_eventmanually re-implements the same POW env parsing that now lives inparse_pow_env()(lines 288–293). Since the whole point of the new helpers is to centralize this, please call the helper to prevent future drift (e.g., differing defaults or error messages).♻️ Proposed diff
- // Reuse POW behaviour from existing DM helpers, but fail on invalid values - let pow: u8 = var("POW") - .unwrap_or_else(|_| "0".to_string()) - .parse() - .map_err(|e| anyhow::anyhow!("Failed to parse POW: {}", e))?; + // Reuse POW behaviour from existing DM helpers, but fail on invalid values + let pow: u8 = parse_pow_env()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/messaging.rs` around lines 100 - 104, In build_custom_wrap_event replace the manual POW env parsing with a call to the existing parse_pow_env() helper: remove the var("POW")...parse() block and call parse_pow_env() to obtain the u8 POW value, propagating its Result/anyhow error as before; ensure you reference parse_pow_env() return (u8) where the previous local pow variable was used so defaults and error messages remain centralized and consistent.tests/parser_dms.rs (1)
235-292: Solid new coverage — consider adding asigned: falsevariant.The new wrap→parse handshake tests both use
WrapOptions::default()(i.e.signed: true), yet the production paths that matter most (send_plain_text_dm, admin DM flows,execute_restore,execute_last_trade_index) all usesigned: false. Adding one assertion exercising that knob would catch any futureunwrap_messageregression on the tuple-without-signature branch.🧪 Proposed follow-up test
+#[tokio::test] +async fn parse_dm_events_accepts_unsigned_wrap_message_output() { + let sender_trade_keys = Keys::generate(); + let receiver_keys = Keys::generate(); + + let inner = Message::new_order(None, Some(1), Some(1), Action::NewOrder, None); + let wrapped = wrap_message( + &inner, + &sender_trade_keys, + receiver_keys.public_key(), + WrapOptions { signed: false, ..Default::default() }, + ) + .await + .expect("wrap"); + + let mut events = Events::new(&Filter::new()); + events.insert(wrapped); + + let parsed = parse_dm_events(events, &receiver_keys, None).await; + assert_eq!(parsed.len(), 1); + assert_eq!(parsed[0].2, sender_trade_keys.public_key()); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/parser_dms.rs` around lines 235 - 292, Add a companion test that mirrors parse_dm_events_accepts_wrap_message_output but creates the wrapped message with unsigned wrap options (use WrapOptions { signed: false } or the equivalent constructor instead of WrapOptions::default()) so parse_dm_events is exercised for the unsigned branch; locate the test using wrap_message and WrapOptions in parse_dm_events_accepts_wrap_message_output and replicate the same assertions (message roundtrip and sender public key) while passing the receiver keys to parse_dm_events as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/util/messaging.rs`:
- Around line 100-104: In build_custom_wrap_event replace the manual POW env
parsing with a call to the existing parse_pow_env() helper: remove the
var("POW")...parse() block and call parse_pow_env() to obtain the u8 POW value,
propagating its Result/anyhow error as before; ensure you reference
parse_pow_env() return (u8) where the previous local pow variable was used so
defaults and error messages remain centralized and consistent.
In `@tests/parser_dms.rs`:
- Around line 235-292: Add a companion test that mirrors
parse_dm_events_accepts_wrap_message_output but creates the wrapped message with
unsigned wrap options (use WrapOptions { signed: false } or the equivalent
constructor instead of WrapOptions::default()) so parse_dm_events is exercised
for the unsigned branch; locate the test using wrap_message and WrapOptions in
parse_dm_events_accepts_wrap_message_output and replicate the same assertions
(message roundtrip and sender public key) while passing the receiver keys to
parse_dm_events as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b48da61e-a8a6-4f0f-9a29-fd12f1d8e6e9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomlsrc/cli/add_invoice.rssrc/cli/adm_send_dm.rssrc/cli/last_trade_index.rssrc/cli/new_order.rssrc/cli/orders_info.rssrc/cli/rate_user.rssrc/cli/restore.rssrc/cli/send_admin_dm_attach.rssrc/cli/send_dm.rssrc/cli/send_msg.rssrc/cli/take_dispute.rssrc/cli/take_order.rssrc/db.rssrc/parser/disputes.rssrc/parser/dms.rssrc/parser/orders.rssrc/util/events.rssrc/util/messaging.rssrc/util/mod.rssrc/util/storage.rssrc/util/types.rstests/parser_dms.rs
💤 Files with no reviewable changes (7)
- src/cli/rate_user.rs
- src/cli/orders_info.rs
- src/cli/take_order.rs
- src/cli/send_msg.rs
- src/util/types.rs
- src/cli/new_order.rs
- src/cli/add_invoice.rs
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
mostro-corefrom 0.6.56 → 0.9.1 andnostr-sdkfrom 0.43.0 → 0.44.1.wrap_message/unwrap_message/validate_responseAPI frommostro_core::nip59.admin_keys. The previous hybrid (admin seal + trade-keys rumor) is not valid undernostr-sdk0.44, which enforcesrumor.pubkey == seal.pubkey(SenderMismatch).Key changes
src/util/messaging.rs— deletesgift_wrap_from_seal_with_pow,create_gift_wrap_event,send_gift_wrap_dm{,_internal},send_admin_gift_wrap_dm,determine_message_type,create_expiration_tags, and the now-obsoleteMessageTypeenum.send_dmbecomes a thin wrapper aroundwrap_message, with a singlesigner_keysparameter (identity_keys dropped).print_dm_eventsusesvalidate_responsewith a single documented exception:NewOrderpushes without arequest_id(child orders from range trades) are still printed becausemostro-coreintentionally leavesNewOrderout of the unsolicited-push allow-list.src/parser/dms.rs— theKind::GiftWrapbranch ofparse_dm_eventsnow callsunwrap_message, distinguishingOk(None)("not addressed to us") fromErr(protocol violation). No more direct calls tonip59::extract_rumoror(Message, Option<String>)tuple deserialization.Admin signer change —
storage::admin_send_dmandtake_dispute::execute_take_disputenow passadmin_keys(notctx.trade_keys) as the signer. This is the semantically correct migration: Mostro identifies admin viarumor.pubkey = admin_keys.pubkey, matching howsend_admin_gift_wrap_dmalready worked for plain admin DMs.Out of scope (unchanged):
PrivateDirectMessage(kind 14, SECRET/to_user=truebranch).send_admin_chat_message_via_shared_key,build_custom_wrap_event, …) — not NIP-59 proper.Knock-on changes from the dependency bump
NOSTR_REPLACEABLE_EVENT_KIND→NOSTR_ORDER_EVENT_KIND(same value38383; preserves BIP32-derived keys).Timestamp::as_u64→as_secs(6 sites).RestoredDisputesInfogainsinitiator/solver_pubkeyfields — test fixture updated.Tests
Added (6):
messaging::tests:send_dm_gift_wrap_roundtrips_via_unwrap_message,secret_env_semantics_drop_inner_signature,wrap_message_respects_pow_option,wrong_keys_yield_none_on_unwrap.tests/parser_dms.rs:parse_dm_events_accepts_wrap_message_output,parse_dm_events_skips_events_for_other_keys.Total: 83/83 passing.
cargo fmt --allclean.cargo clippy --all-targets --all-features -- -D warningsclean (matches the release pre-hook).Test plan
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningscargo test --all-targets(83 passing)new_order→take_sell/take_buy→add_invoice→fiat_sent→release→rate.NewOrderstill prints.ADMIN_NSECset):adm_send_dm,adm_add_solver,adm_take_dispute,adm_cancel,adm_settle.restoreandget_last_trade_indexagainst a user with prior history.Closes #163
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
nostr-sdkto version 0.44.1 andmostro-coreto version 0.9.1.Tests