Add dm message #97
Add dm message #97
Conversation
* Verify request_id in mostrod response * Update mostro core to version 0.6.17 * Add new message kind for DM * Code refactoring
WalkthroughThis pull request introduces a series of modifications across multiple files in the Mostro CLI project. The changes primarily focus on updating the package version, modifying the Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Util as send_message_sync
participant Client as Client
participant DB as Database
CLI->>Util: Send message
Util->>Client: Convert message to JSON
Client->>Client: Send message
Client-->>Util: Receive messages
Util->>Util: Process received messages
Util->>DB: Save order ID (if applicable)
Util-->>CLI: Return messages
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 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: 3
🧹 Outside diff range and nitpick comments (8)
src/cli/send_dm.rs (1)
18-18: Verify the boolean flag parameters in send_message_syncThe
send_message_synccall includes two boolean flags (true, true). Consider making these named parameters or constants for better clarity.- send_message_sync(client, None, trade_keys, receiver, message, true, true).await?; + const SAVE_MESSAGE: bool = true; + const PRINT_MESSAGE: bool = true; + send_message_sync(client, None, trade_keys, receiver, message, + SAVE_MESSAGE, PRINT_MESSAGE).await?;src/cli/take_dispute.rs (1)
Boolean flags in send_message_sync are used inconsistently across the codebase
- In
src/cli/send_dm.rs, both flags are set totrue(wait_for_dm_ans=true, to_user=true)- In
src/cli/take_dispute.rs, the flags are missing from the call, which means the code won't compile- Other calls in the codebase are also missing these required boolean parameters
This is a real issue as the function signature requires two boolean parameters (
wait_for_dm_ansandto_user) but most calls in the codebase don't provide them, which would result in compilation errors.🔗 Analysis chain
Line range hint
29-35: Consider standardizing boolean flag usage across commandsThe boolean flags (true, false) differ from send_dm.rs (true, true). Consider standardizing these flags across different commands or documenting why they differ.
Let me gather more information about the actual flag values used in these calls.
Let me check the function definition and all its usages with more context to see the complete parameter list and boolean flags.
Let me check the complete function signature to understand all parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all send_message_sync calls to analyze flag patterns rg -A 1 "send_message_sync\("Length of output: 864
Script:
#!/bin/bash # Search for complete send_message_sync calls with context to see the flag values rg -B 3 -A 3 "send_message_sync\(" --multilineLength of output: 2832
Script:
#!/bin/bash # Get the function definition from util.rs ast-grep --pattern 'pub async fn send_message_sync($$$)' # Get complete function calls with more context rg "send_message_sync\(" -B 5 -A 5Length of output: 4387
Script:
#!/bin/bash # Get the complete function definition with all parameters cat src/util.rs | rg -A 10 "^pub async fn send_message_sync"Length of output: 400
src/cli/rate_user.rs (2)
Line range hint
19-23: Consider improving error handling instead of direct process::exitUsing
std::process::exit(0)for validation failures bypasses proper error handling. Consider returning a Result with a custom error type instead.- if let 1..=5 = *rating { - rating_content = Payload::RatingUser(*rating); - } else { - println!("Rating must be in the range 1 - 5"); - std::process::exit(0); - } + if !(1..=5).contains(rating) { + return Err(anyhow::anyhow!("Rating must be in the range 1 - 5")); + } + rating_content = Payload::RatingUser(*rating);
Line range hint
44-44: Remove unnecessary process::exit at function endThe function already returns
Result<()>, making thestd::process::exit(0)redundant and potentially masking errors.- std::process::exit(0); + Ok(())src/cli/take_buy.rs (1)
Line range hint
1-52: Consider improving error handling and reducing function complexity.
- Replace
unwrap()calls with proper error handling to prevent potential panics:- let mut user = User::get(&pool).await.unwrap(); - user.save(&pool).await.unwrap(); + let mut user = User::get(&pool).await?; + user.save(&pool).await?;
- Consider introducing a struct to encapsulate the numerous function parameters:
struct TakeBuyParams { order_id: Uuid, amount: Option<u32>, identity_keys: Keys, trade_keys: Keys, trade_index: i64, mostro_key: PublicKey, client: Client, }src/cli/add_invoice.rs (1)
Line range hint
19-25: Consider improving error handling for database operations.Replace
unwrap()calls with proper error handling to prevent potential panics:- let order = Order::get_by_id(&pool, &order_id.to_string()) - .await - .unwrap(); - let trade_keys = order.trade_keys.unwrap(); - let trade_keys = Keys::parse(trade_keys).unwrap(); + let order = Order::get_by_id(&pool, &order_id.to_string()) + .await?; + let trade_keys = order.trade_keys + .ok_or_else(|| anyhow::anyhow!("Trade keys not found"))?; + let trade_keys = Keys::parse(trade_keys) + .map_err(|e| anyhow::anyhow!("Failed to parse trade keys: {}", e))?;src/cli/take_sell.rs (1)
Line range hint
12-71: Consider improving error handling and reducing function complexity.
- Replace
unwrap()calls with proper error handling:- let mut user = User::get(&pool).await.unwrap(); - user.save(&pool).await.unwrap(); + let mut user = User::get(&pool).await?; + user.save(&pool).await?;
- Consider breaking down the complex payload construction logic into a separate function:
fn construct_payload(invoice: &Option<String>, amount: Option<u32>) -> Option<Payload> { let mut payload = None; if let Some(invoice) = invoice { payload = construct_invoice_payload(invoice); } if let Some(amount) = amount { payload = add_amount_to_payload(payload, amount); } payload }
- Consider introducing a struct to encapsulate the numerous function parameters (similar to the suggestion in take_buy.rs):
struct TakeSellParams { order_id: Uuid, invoice: Option<String>, amount: Option<u32>, identity_keys: Keys, trade_keys: Keys, trade_index: i64, mostro_key: PublicKey, client: Client, }src/util.rs (1)
104-104: Consider making sleep duration configurableThe hardcoded 1-second sleep duration should be configurable to accommodate different network conditions and relay responsiveness.
+ const DEFAULT_MESSAGE_WAIT_DURATION: u64 = 1; + pub async fn send_message_sync( client: &Client, identity_keys: Option<&Keys>, trade_keys: &Keys, receiver_pubkey: PublicKey, message: Message, wait_for_dm_ans: bool, to_user: bool, + wait_duration: Option<u64>, ) -> Result<Vec<(Message, u64)>> { // ... - sleep(Duration::from_secs(1)); + sleep(Duration::from_secs(wait_duration.unwrap_or(DEFAULT_MESSAGE_WAIT_DURATION)));
📜 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 (11)
Cargo.toml(2 hunks)src/cli/add_invoice.rs(1 hunks)src/cli/new_order.rs(2 hunks)src/cli/rate_user.rs(1 hunks)src/cli/send_dm.rs(2 hunks)src/cli/send_msg.rs(1 hunks)src/cli/take_buy.rs(1 hunks)src/cli/take_dispute.rs(1 hunks)src/cli/take_sell.rs(1 hunks)src/db.rs(1 hunks)src/util.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🔇 Additional comments (11)
src/cli/send_dm.rs (1)
12-17: Verify the usage of None parameters in Message::new_dm
The message construction looks correct, but let's verify the purpose of the two None parameters. Consider documenting their purpose if they represent optional fields that might be used in future extensions.
✅ Verification successful
Let me search for the Message struct definition to understand these parameters better.
Let me try a different approach to find the Message type definition and its documentation.
Based on the search results, I can now provide a final response about the None parameters in Message::new_dm.
The None parameters in Message::new_dm are correctly used
Looking at the codebase usage patterns, the None parameters are consistently used this way for direct messages. The first two None parameters appear to be for optional order ID and request ID, which aren't needed for simple direct messages. This is evidenced by comparing with other message constructors like Message::new_order where these fields are used for order-related messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Message::new_dm usages to understand parameter patterns
ast-grep --pattern 'Message::new_dm($$$)'
Length of output: 326
Script:
#!/bin/bash
# Search for Message struct definition
ast-grep --pattern 'struct Message {
$$$
}'
# Also search for new_dm implementation
ast-grep --pattern 'impl Message {
$$$
fn new_dm($$$) {
$$$
}
$$$
}'
Length of output: 130
Script:
#!/bin/bash
# Search for Message type in the codebase using ripgrep
rg "Message" -A 5 -B 5
# Also search specifically for new_dm documentation or implementation
rg "new_dm" -A 5 -B 5
Length of output: 18205
src/cli/take_dispute.rs (1)
27-27: LGTM! Message construction simplified
The removal of .as_json().unwrap() aligns with the broader refactoring effort and improves code robustness by removing unnecessary JSON serialization.
src/cli/rate_user.rs (1)
34-34: LGTM! Message construction simplified
The removal of .as_json().unwrap() aligns with the broader refactoring effort and improves code robustness.
src/cli/take_buy.rs (1)
33-33: LGTM! Message handling refactor looks good.
The removal of .as_json().unwrap() aligns with the broader refactoring to handle Message types directly in send_message_sync.
src/cli/add_invoice.rs (1)
42-42: LGTM! Message handling refactor looks good.
The removal of .as_json().unwrap() aligns with the broader refactoring to handle Message types directly in send_message_sync.
src/cli/take_sell.rs (1)
59-59: LGTM! Message handling refactor looks good.
The removal of .as_json().unwrap() aligns with the broader refactoring to handle Message types directly in send_message_sync.
src/cli/send_msg.rs (1)
47-47: LGTM! Message creation simplified.
The removal of .as_json().unwrap() aligns with the broader refactoring to simplify message handling by passing Message objects directly.
src/cli/new_order.rs (1)
124-124: LGTM! Message creation simplified.
The removal of JSON conversion aligns with the broader refactoring to simplify message handling.
src/db.rs (1)
390-409: LGTM! Well-implemented database update method.
The new save_new_id method follows best practices:
- Uses parameterized queries preventing SQL injection
- Returns meaningful success indicator
- Proper error handling with anyhow::Result
src/util.rs (2)
16-16: LGTM: Import added for message handling delay
The addition of the sleep import is appropriate for managing message synchronization timing.
83-87: Function signature change improves type safety
The change from String to Message type and returning Vec<(Message, u64)> provides better type safety and more useful return data.
| if wait_for_dm_ans { | ||
| dm = get_direct_messages(client, trade_keys, 1, to_user).await; | ||
| for el in dm.iter() { | ||
| if let Some(Payload::PaymentRequest(ord, inv, _)) = | ||
| &el.0.get_inner_message_kind().payload | ||
| { | ||
| println!("NEW MESSAGE:"); | ||
| println!( | ||
| "Mostro sent you this hold invoice for order id: {}", | ||
| ord.as_ref().unwrap().id.unwrap() | ||
| ); | ||
| println!(); | ||
| println!("Pay this invoice to continue --> {}", inv); | ||
| println!(); | ||
| } | ||
| break; | ||
| } else if let RelayPoolNotification::Message { | ||
| message: | ||
| RelayMessage::Ok { | ||
| event_id: _, | ||
| status: _, | ||
| message: _, | ||
| }, | ||
| .. | ||
| } = notification | ||
| { | ||
| println!( | ||
| "Message correctly sent to Mostro! Check messages with getdm or listorders command" | ||
| ); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and async operations
The current implementation has several areas for improvement:
- The message processing loop could benefit from error handling
- Using synchronous sleep in an async function can block the executor
Consider this improved implementation:
if wait_for_dm_ans {
- dm = get_direct_messages(client, trade_keys, 1, to_user).await;
- for el in dm.iter() {
+ match get_direct_messages(client, trade_keys, 1, to_user).await {
+ messages => {
+ dm = messages;
+ for el in dm.iter() {
if let Some(Payload::PaymentRequest(ord, inv, _)) =
&el.0.get_inner_message_kind().payload
{
- println!("NEW MESSAGE:");
- println!(
- "Mostro sent you this hold invoice for order id: {}",
- ord.as_ref().unwrap().id.unwrap()
- );
- println!();
- println!("Pay this invoice to continue --> {}", inv);
- println!();
+ if let Some(order) = ord.as_ref() {
+ if let Some(order_id) = order.id {
+ println!("NEW MESSAGE:");
+ println!(
+ "Mostro sent you this hold invoice for order id: {}",
+ order_id
+ );
+ println!();
+ println!("Pay this invoice to continue --> {}", inv);
+ println!();
+ }
+ }
}
+ }
}
}
}Also, consider using tokio::time::sleep instead of std::thread::sleep to avoid blocking:
- use std::thread::sleep;
+ use tokio::time::sleep;Committable suggestion skipped: line range outside the PR's diff.
| receiver_pubkey: PublicKey, | ||
| message: String, | ||
| message: Message, | ||
| wait_for_dm_ans: bool, | ||
| to_user: bool, | ||
| ) -> Result<()> { | ||
| ) -> Result<Vec<(Message, u64)>> { | ||
| let mut dm: Vec<(Message, u64)> = Vec::new(); | ||
| let message_json = message.as_json()?; | ||
| // Send dm to receiver pubkey | ||
| println!( | ||
| "SENDING DM with trade keys: {:?}", | ||
| trade_keys.public_key().to_hex() | ||
| ); | ||
| send_dm( | ||
| client, | ||
| identity_keys, | ||
| trade_keys, | ||
| &receiver_pubkey, | ||
| message, | ||
| message_json, | ||
| to_user, | ||
| ) | ||
| .await?; | ||
|
|
||
| let mut notifications = client.notifications(); | ||
| while let Ok(notification) = notifications.recv().await { | ||
| if wait_for_dm_ans { | ||
| let dm = get_direct_messages(client, trade_keys, 1, to_user).await; | ||
| println!("DM: {:#?}", dm); | ||
| for el in dm.iter() { | ||
| if let Some(Payload::PaymentRequest(ord, inv, _)) = | ||
| &el.0.get_inner_message_kind().payload | ||
| { | ||
| println!("NEW MESSAGE:"); | ||
| println!( | ||
| "Mostro sent you this hold invoice for order id: {}", | ||
| ord.as_ref().unwrap().id.unwrap() | ||
| ); | ||
| println!(); | ||
| println!("Pay this invoice to continue --> {}", inv); | ||
| println!(); | ||
| } | ||
| sleep(Duration::from_secs(1)); | ||
|
|
||
| if wait_for_dm_ans { | ||
| dm = get_direct_messages(client, trade_keys, 1, to_user).await; | ||
| for el in dm.iter() { | ||
| if let Some(Payload::PaymentRequest(ord, inv, _)) = | ||
| &el.0.get_inner_message_kind().payload | ||
| { | ||
| println!("NEW MESSAGE:"); | ||
| println!( | ||
| "Mostro sent you this hold invoice for order id: {}", | ||
| ord.as_ref().unwrap().id.unwrap() | ||
| ); | ||
| println!(); | ||
| println!("Pay this invoice to continue --> {}", inv); | ||
| println!(); | ||
| } | ||
| break; | ||
| } else if let RelayPoolNotification::Message { | ||
| message: | ||
| RelayMessage::Ok { | ||
| event_id: _, | ||
| status: _, | ||
| message: _, | ||
| }, | ||
| .. | ||
| } = notification | ||
| { | ||
| println!( | ||
| "Message correctly sent to Mostro! Check messages with getdm or listorders command" | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
| Ok(()) | ||
| Ok(dm) |
There was a problem hiding this comment.
💡 Codebase verification
Update required: Inconsistent handling of return values
Based on the verification results, there are issues with how the return value is handled across different callers:
- In
src/cli/new_order.rs: Correctly assigns the return value todmvariable - In
src/cli/send_dm.rs: Ignores the return value and returnsOk(()) - Other callers (take_sell, take_dispute, rate_user, send_msg, take_buy, add_invoice) don't handle the return value properly
The change from Result<()> to Result<Vec<(Message, u64)>> requires updates in all calling functions to properly handle the returned messages.
🔗 Analysis chain
Verify updates in all caller functions
The change in return type from Result<()> to Result<Vec<(Message, u64)>> requires verification of all calling functions.
Let me analyze the callers to verify if they handle the return value correctly.
Let me try a different approach to check the usage patterns.
Let me try one more search to see how the function calls are handled in other files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to send_message_sync
rg "send_message_sync\(" --type rust -A 5 -B 5
Length of output: 4340
Script:
#!/bin/bash
# Check how callers handle the return value
ast-grep --pattern 'send_message_sync($$$).await$_'
Length of output: 53
Script:
#!/bin/bash
# Search for await patterns with send_message_sync
rg "send_message_sync\(.*\)\.await" --type rust -A 2 -B 2
Length of output: 317
Script:
#!/bin/bash
# Search for let patterns with send_message_sync to see variable assignments
rg "let.*=.*send_message_sync\(" --type rust -A 2 -B 2
Length of output: 319
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
mostro-clito 0.10.6.Bug Fixes
Documentation
Refactor
Chores
mostro-core.