Refactor with sdk and removal of nip59 mod #100
Conversation
…i, need to check if I can manage also user messages. Removed lot of code also for events fetching
WalkthroughThis pull request introduces several significant changes to the Mostro CLI project, focusing on dependency management, code refactoring, and module restructuring. The modifications include updating dependencies in Changes
Sequence DiagramsequenceDiagram
participant Client
participant SendDM
participant EventBuilder
participant Message
Client->>SendDM: send_dm(payload, keys, expiration)
SendDM->>Message: Create message
Message-->>EventBuilder: Build event
EventBuilder->>Client: Return signed event
Possibly Related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/util.rs (1)
Line range hint
161-222: Potential unwrapping vulnerabilities.
Multiple unwraps occur in the decryption and unwrapping processes. Attacks on manipulated or invalid data could cause panics. Replace these unwraps with structured error handling or the “? operator” to gracefully handle malformed data.- let unwrapped_gift = match nip59::extract_rumor(my_key, dm).await { - Ok(u) => u, - Err(_) => { - println!("Error unwrapping gift"); - continue; - } - }; - let (mmessage, sig): (Message, nostr_sdk::secp256k1::schnorr::Signature) = - serde_json::from_str(&unwrapped_gift.rumor.content).unwrap(); + let unwrapped_gift = nip59::extract_rumor(my_key, dm).await; + if unwrapped_gift.is_err() { + println!("Error unwrapping gift"); + continue; + } + let unwrapped_gift = unwrapped_gift.unwrap(); // or match to handle error + + let deser_result = serde_json::from_str(&unwrapped_gift.rumor.content); + if deser_result.is_err() { + println!("Error parsing unwrapped rumor content"); + continue; + } + let (mmessage, sig): (Message, nostr_sdk::secp256k1::schnorr::Signature) = deser_result.unwrap();
🧹 Nitpick comments (5)
src/util.rs (3)
Line range hint
33-38: Handle encryption errors with more descriptive feedback.
Although you’re correctly checking for an error, consider returning a more explicit error message or logging additional context (like the receiver’s pubkey or the payload length) to improve troubleshooting.
110-110: Double-check the "None" expiration usage in send_message_sync.
Here, “expiration” is explicitly set to None. If that’s intended, confirm that the broader system logic does not rely on an expiration for short-lived messages.
Line range hint
242-265: Check for concurrency or pagination issues in fetch_events.
When retrieving multiple events from multiple relays, ensure any concurrency or pagination edge cases are handled. If events come in out of sequence or partial sets, data might be incomplete, especially for large volumes.Cargo.toml (1)
52-53: Local path override for "mostro-core".
Using a local path may ease development but could complicate CI/CD if the environment lacks the local version. Consider using a dedicated branch or Git reference for better integration in multi-dev workflows.src/cli/send_msg.rs (1)
55-55: LGTM! Consistent with the refactoring pattern.The change to use reference passing with
Keys::parsematches the pattern in add_invoice.rs and maintains proper error handling.Consider adding a brief comment explaining the Keys parsing pattern, as it's part of a larger SDK integration effort:
+ // Parse trade keys using SDK's reference-based parsing let trade_keys = Keys::parse(&trade_keys_str)?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(2 hunks)src/cli/add_invoice.rs(1 hunks)src/cli/send_msg.rs(1 hunks)src/lib.rs(0 hunks)src/nip59.rs(0 hunks)src/util.rs(10 hunks)
💤 Files with no reviewable changes (2)
- src/lib.rs
- src/nip59.rs
🔇 Additional comments (5)
src/util.rs (2)
25-25: Ensure proper handling of the new "expiration" parameter.
If the event content is time-sensitive, consider validating its timestamp before passing it to this function. Also, verify that relays reliably honor the new "expiration" tag.
44-63: Strengthen error checks around message signing & rumor creation.
You’re assuming successful JSON parsing and signature creation without fallback or additional logging in case of errors. Consider adding checks to handle partial or incorrect data in the content.
Cargo.toml (2)
25-26: Confirm all Nostr SDK features are still needed.
You continue to include "nip59" in the features, even though the PR’s summary indicates a removal of the nip59 module. Confirm that the Nostr SDK’s nip59 capability is still required.
43-43: Upgrade verification for "mostro-core".
Version 0.6.18 might introduce changes that require code updates. Confirm that these changes are minimal and tested thoroughly, especially if they introduce new message/event handling logic.
src/cli/add_invoice.rs (1)
24-24: LGTM! Consistent with SDK integration objectives.
The change to pass a reference to Keys::parse is a good practice for memory efficiency and aligns with the PR's goal of SDK integration.
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
Reference passing to Keys::parse is consistently implemented
Based on the comprehensive search results, the pattern of passing references to Keys::parse is consistently applied across the codebase:
src/cli/add_invoice.rs:Keys::parse(&trade_keys)src/cli/get_dm.rs:Keys::parse(keys)src/cli/send_msg.rs:Keys::parse(&trade_keys_str)
The usage pattern shows that Keys is a core type used throughout the application for identity and trade key management, and the reference passing approach is uniformly implemented in all instances where Keys::parse is called.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct Keys::parse calls that might need similar updates
ast-grep --pattern 'Keys::parse($x)'
Length of output: 38
Script:
#!/bin/bash
# Let's try a more comprehensive search for Keys::parse usage
rg "Keys::parse" -A 2
Length of output: 623
Script:
#!/bin/bash
# Also search for the Keys type usage to understand the context
rg "use.*Keys" --type rust
Length of output: 28
Script:
#!/bin/bash
# Look for the Keys struct/type definition
fd -e rs -x ast-grep --pattern 'struct Keys {$$$}'
Length of output: 52
Script:
#!/bin/bash
# Let's look for Keys imports to find its module
rg "Keys" --type rust -B 2
Length of output: 3860
Script:
#!/bin/bash
# Check the actual implementation of Keys::parse
fd Keys.rs -x cat {} \;
Length of output: 28
| if let Ok(mostro_req) = client | ||
| .fetch_events(vec![filter], Duration::from_secs(15)) | ||
| .await | ||
| { | ||
| // Scan events to extract all disputes | ||
| for d in mostro_req.iter() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine error handling for dispute extraction.
Similar to orders, the dispute extraction loop silently discards errors. Investigate adding logging or returning partial results to facilitate debugging and error recovery in production.
|
Check #102 that wraps also this pr. |
Remove of
nip59.rsmodtesting to replace lot of lines of code with internal Sdk functions:
Temporary note:
At the moment to make this compile we need to point to Nost-sdk in
cargo.tomlofmostro-cliandmostro-corelike this:and also use a local override of
mostro-coreto point to same nostr-sdk like, this:these 2 patches can be removed as soon as new release of sdk has been shipped.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores