Aligning message management #144
Conversation
… is yet present in db we do an UPDATE sql and not INSERT
|
Caution Review failedThe pull request is closed. WalkthroughAdded a GetLastTradeIndex CLI command and execute_last_trade_index; refactored DM flows to synchronous send → wait → parse with new print_dm_events/print_commands_results; changed several function signatures (wait_for_dm, fetch_events_list, execute_get_dm, save_order, run_simple_order_msg); implemented DB insert-then-update on unique constraint; bumped mostro-core. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI Command
participant SDK as nostr send_dm
participant Wait as wait_for_dm(ctx)
participant Parser as parse_dm_events
participant Printer as print_commands_results
participant DB as DB (save_order)
rect rgb(250,250,255)
CLI->>SDK: send_dm(ctx.identity_keys, payload with request_id)
SDK-->>CLI: ack/ok
end
rect rgb(245,255,245)
CLI->>Wait: wait_for_dm(ctx, order_trade_keys?)
Wait-->>CLI: Events
CLI->>Parser: parse_dm_events(Events, pubkey, since?)
Parser-->>CLI: [(MessageKind, ts, sender_pubkey)]
end
alt request_id matches & action valid
CLI->>Printer: print_commands_results(MessageKind, order?, ctx)
Printer->>DB: save_order(...) → insert_db / update_db
DB-->>Printer: ok/err
Printer-->>CLI: printed output
else no match / wrong action
CLI-->>CLI: error / ignore
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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(1 hunks)src/cli.rs(1 hunks)src/cli/add_invoice.rs(2 hunks)src/cli/get_dm.rs(2 hunks)src/cli/new_order.rs(2 hunks)src/cli/send_msg.rs(3 hunks)src/cli/take_order.rs(3 hunks)src/db.rs(2 hunks)src/parser/dms.rs(2 hunks)src/util.rs(7 hunks)tests/parser_dms.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter properly by creating filters with `since(Timestamp::from(since))`, so no additional local filtering is needed in CLI functions that call it.
Applied to files:
src/cli/get_dm.rssrc/util.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter through manual filtering after fetching events: it uses a hardcoded 2880-minute window in the Nostr filter, then manually filters results with `if event.created_at.as_u64() < since as u64 { continue; }`.
Applied to files:
src/parser/dms.rssrc/util.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/take_order.rssrc/cli/new_order.rs
📚 Learning: 2025-09-09T19:58:58.506Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/add_invoice.rs:80-82
Timestamp: 2025-09-09T19:58:58.506Z
Learning: In the mostro-cli codebase, when the trade_index parameter isn't needed inside a function like wait_for_dm, it should be made Optional<i64> instead of requiring callers to pass hardcoded values like 0. This makes the API clearer and avoids unnecessary computation for resolving indices when they're not used.
Applied to files:
src/util.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/new_order.rs
🧬 Code graph analysis (9)
src/cli/get_dm.rs (1)
src/util.rs (2)
fetch_events_list(440-531)None(58-58)
src/parser/dms.rs (2)
src/util.rs (1)
save_order(96-122)src/db.rs (7)
sqlx(79-79)sqlx(86-86)sqlx(196-196)sqlx(508-508)sqlx(526-526)get_by_id(507-523)delete_by_id(538-551)
src/cli/send_msg.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-267)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
src/cli/take_order.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-267)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
src/cli.rs (2)
src/cli/get_dm.rs (1)
execute_get_dm(10-38)src/cli/get_dm_user.rs (1)
execute_get_dm_user(9-73)
src/cli/add_invoice.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-267)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
tests/parser_dms.rs (1)
src/parser/dms.rs (1)
parse_dm_events(189-267)
src/util.rs (2)
src/db.rs (2)
new(139-160)new(278-384)src/parser/dms.rs (1)
parse_dm_events(189-267)
src/cli/new_order.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-267)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
| // DM retrieval commands | ||
| Commands::GetDm { since, from_user } => { | ||
| execute_get_dm(Some(since), false, from_user, ctx).await | ||
| execute_get_dm(since, false, from_user, ctx).await | ||
| } | ||
| Commands::GetDmUser { since } => execute_get_dm_user(since, ctx).await, | ||
| Commands::GetAdminDm { since, from_user } => { | ||
| execute_get_dm(Some(since), true, from_user, ctx).await | ||
| execute_get_dm(since, true, from_user, ctx).await | ||
| } |
There was a problem hiding this comment.
Fix borrow when calling execute_get_dm
execute_get_dm now takes &i64. Passing since by value won’t compile (expected &i64, found i64). Borrow it at both call sites.
- execute_get_dm(since, false, from_user, ctx).await
+ execute_get_dm(&since, false, from_user, ctx).await
…
- execute_get_dm(since, true, from_user, ctx).await
+ execute_get_dm(&since, true, from_user, ctx).await📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DM retrieval commands | |
| Commands::GetDm { since, from_user } => { | |
| execute_get_dm(Some(since), false, from_user, ctx).await | |
| execute_get_dm(since, false, from_user, ctx).await | |
| } | |
| Commands::GetDmUser { since } => execute_get_dm_user(since, ctx).await, | |
| Commands::GetAdminDm { since, from_user } => { | |
| execute_get_dm(Some(since), true, from_user, ctx).await | |
| execute_get_dm(since, true, from_user, ctx).await | |
| } | |
| // DM retrieval commands | |
| Commands::GetDm { since, from_user } => { | |
| execute_get_dm(&since, false, from_user, ctx).await | |
| } | |
| Commands::GetDmUser { since } => execute_get_dm_user(since, ctx).await, | |
| Commands::GetAdminDm { since, from_user } => { | |
| execute_get_dm(&since, true, from_user, ctx).await | |
| } |
🤖 Prompt for AI Agents
In src/cli.rs around lines 490 to 497, the calls to execute_get_dm pass since by
value but the function now expects &i64; change the two call sites to pass a
reference (e.g. &since) instead of since so the types match (update both the
Commands::GetDm and Commands::GetAdminDm invocations).
There was a problem hiding this comment.
Since is passed correctly also the:
cargo clippy --all-targets --all-features -- -D warnings
command is happy!
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual signature of execute_get_dm function
echo "=== Checking execute_get_dm signature ==="
rg -n "pub async fn execute_get_dm" src/cli/get_dm.rs -A 3
echo ""
echo "=== Checking call sites in src/cli.rs ==="
rg -n "execute_get_dm\(" src/cli.rs -B 1 -A 1
Length of output: 482
@arkanoider, thank you for confirming! Just to be clear: the diff shown above passes since by value (without the &), but execute_get_dm expects &i64. If you've already added the borrow operator (&since) in your local code and clippy is happy, that's perfect! ✅
If you haven't applied the fix yet, you'll need to change both call sites to:
execute_get_dm(&since, false, from_user, ctx).awaitexecute_get_dm(&since, true, from_user, ctx).await
Once that's done, the types will align correctly and clippy will pass.
🐰
🧬 Code Graph Analysis Results
[src/cli/adm_send_dm.rs:6-17]
pub async fn execute_adm_send_dm(receiver: PublicKey, ctx: &Context, message: &str) -> Result<()> {
println!(
"SENDING DM with admin keys: {}",
ctx.context_keys.public_key().to_hex()
);
send_admin_gift_wrap_dm(&ctx.client, &ctx.context_keys, &receiver, message).await?;
println!("Admin gift wrap message sent to {}", receiver);
Ok(())
}
[src/cli/conversation_key.rs:5-17]
pub async fn execute_conversation_key(trade_keys: &Keys, receiver: PublicKey) -> Result<()> {
// Derive conversation key
let ck = ConversationKey::derive(trade_keys.secret_key(), &receiver)?;
let key = ck.as_bytes();
let mut ck_hex = vec![];
for i in key {
ck_hex.push(format!("{:02x}", i));
}
let ck_hex = ck_hex.join("");
println!("Conversation key: {:?}", ck_hex);
Ok(())
}
[src/cli/list_disputes.rs:7-21]
pub async fn execute_list_disputes(ctx: &Context) -> Result<()> {
// Print mostro pubkey
println!(
"Requesting disputes from mostro pubId - {}",
&ctx.mostro_pubkey
);
// Get orders from relays
let table_of_disputes =
fetch_events_list(ListKind::Disputes, None, None, None, ctx, None).await?;
let table = print_disputes_table(table_of_disputes)?;
println!("{table}");
Ok(())
}
[src/cli/restore.rs:7-32]
pub async fn execute_restore(
identity_keys: &Keys,
mostro_key: PublicKey,
client: &Client,
) -> Result<()> {
let restore_message = Message::new_restore(None);
let message_json = restore_message
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
// Send the restore message to Mostro server
send_dm(
client,
Some(identity_keys),
identity_keys,
&mostro_key,
message_json,
None,
false,
)
.await?;
println!("Restore message sent successfully. Recovering pending orders and disputes...");
Ok(())
}
[src/cli/dm_to_user.rs:7-33]
pub async fn execute_dm_to_user(
receiver: PublicKey,
client: &Client,
order_id: &Uuid,
message: &str,
pool: &SqlitePool,
) -> Result<()> {
// Get the order
let order = Order::get_by_id(pool, &order_id.to_string())
.await
.map_err(|_| anyhow::anyhow!("order {} not found", order_id))?;
// Get the trade keys
let trade_keys = match order.trade_keys.as_ref() {
Some(trade_keys) => Keys::parse(trade_keys)?,
None => anyhow::bail!("No trade_keys found for this order"),
};
// Send the DM
println!(
"SENDING DM with trade keys: {}",
trade_keys.public_key().to_hex()
);
send_gift_wrap_dm(client, &trade_keys, &receiver, message).await?;
Ok(())
}
[src/cli/get_dm.rs:10-38]
pub async fn execute_get_dm(
since: &i64,
admin: bool,
from_user: &bool,
ctx: &Context,
) -> Result<()> {
// Get the list kind
let list_kind = match (admin, from_user) {
(true, true) => ListKind::PrivateDirectMessagesUser,
(true, false) => ListKind::DirectMessagesAdmin,
(false, true) => ListKind::PrivateDirectMessagesUser,
(false, false) => ListKind::DirectMessagesUser,
};
// Fetch the requested events
let all_fetched_events =
fetch_events_list(list_kind, None, None, None, ctx, Some(since)).await?;
// Extract (Message, u64) tuples from Event::MessageTuple variants
let mut dm_events: Vec<(Message, u64)> = Vec::new();
for event in all_fetched_events {
if let Event::MessageTuple(tuple) = event {
dm_events.push(*tuple);
}
}
print_direct_messages(&dm_events, &ctx.pool).await?;
Ok(())
}
[src/cli/send_dm.rs:8-47]
pub async fn execute_send_dm(
receiver: PublicKey,
ctx: &Context,
order_id: &Uuid,
message: &str,
) -> Result<()> {
let message = Message::new_dm(
None,
None,
Action::SendDm,
Some(Payload::TextMessage(message.to_string())),
)
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
let trade_keys =
if let Ok(order_to_vote) = Order::get_by_id(&ctx.pool, &order_id.to_string()).await {
match order_to_vote.trade_keys.as_ref() {
Some(trade_keys) => Keys::parse(trade_keys)?,
None => {
anyhow::bail!("No trade_keys found for this order");
}
}
} else {
return Err(anyhow::anyhow!("order {} not found", order_id));
};
send_dm(
&ctx.client,
None,
&trade_keys,
&receiver,
message,
None,
false,
)
.await?;
Ok(())
}
[src/cli/rate_user.rs:19-59]
pub async fn execute_rate_user(order_id: &Uuid, rating: &u8, ctx: &Context) -> Result<()> {
// Check boundaries
let rating_content = get_user_rate(rating)?;
// Get the trade keys
let trade_keys =
if let Ok(order_to_vote) = Order::get_by_id(&ctx.pool, &order_id.to_string()).await {
match order_to_vote.trade_keys.as_ref() {
Some(trade_keys) => Keys::parse(trade_keys)?,
None => {
return Err(anyhow::anyhow!("No trade_keys found for this order"));
}
}
} else {
return Err(anyhow::anyhow!("order {} not found", order_id));
};
// Create rating message of counterpart
let rate_message = Message::new_order(
Some(*order_id),
None,
None,
Action::RateUser,
Some(rating_content),
)
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
send_dm(
&ctx.client,
Some(&ctx.identity_keys),
&trade_keys,
&ctx.mostro_pubkey,
rate_message,
None,
false,
)
.await?;
Ok(())
}
[src/cli/list_orders.rs:9-72]
pub async fn execute_list_orders(
kind: &Option<String>,
currency: &Option<String>,
status: &Option<String>,
ctx: &Context,
) -> Result<()> {
// Used to get upper currency string to check against a list of tickers
let mut upper_currency: Option<String> = None;
// Default status is pending
let mut status_checked: Option<Status> = Some(Status::Pending);
// Default kind is none
let mut kind_checked: Option<mostro_core::order::Kind> = None;
// New check against strings
if let Some(s) = status {
status_checked = Some(
Status::from_str(s)
.map_err(|e| anyhow::anyhow!("Not valid status '{}': {:?}", s, e))?,
);
}
// Print status requested
if let Some(status) = &status_checked {
println!("You are searching orders with status {:?}", status);
}
// New check against strings for kind
if let Some(k) = kind {
kind_checked = Some(
mostro_core::order::Kind::from_str(k)
.map_err(|e| anyhow::anyhow!("Not valid order kind '{}': {:?}", k, e))?,
);
if let Some(kind) = &kind_checked {
println!("You are searching {} orders", kind);
}
}
// Uppercase currency
if let Some(curr) = currency {
upper_currency = Some(curr.to_uppercase());
if let Some(currency) = &upper_currency {
println!("You are searching orders with currency {}", currency);
}
}
println!(
"Requesting orders from mostro pubId - {}",
&ctx.mostro_pubkey
);
// Get orders from relays
let table_of_orders = fetch_events_list(
ListKind::Orders,
status_checked,
upper_currency,
kind_checked,
ctx,
None,
)
.await?;
let table = print_orders_table(table_of_orders)?;
println!("{table}");
Ok(())
}
[src/cli/take_order.rs:54-124]
pub async fn execute_take_order(
order_id: &Uuid,
action: Action,
invoice: &Option<String>,
amount: Option<u32>,
ctx: &Context,
) -> Result<()> {
let action_name = match action {
Action::TakeBuy => "take buy",
Action::TakeSell => "take sell",
_ => return Err(anyhow::anyhow!("Invalid action for take order")),
};
println!(
"Request of {} order {} from mostro pubId {}",
action_name, order_id, ctx.mostro_pubkey
);
// Create payload based on action type
let payload = create_take_order_payload(action.clone(), invoice, amount)?;
// Create request id
let request_id = Uuid::new_v4().as_u128() as u64;
// Create message
let take_order_message = Message::new_order(
Some(*order_id),
Some(request_id),
Some(ctx.trade_index),
action.clone(),
payload,
);
// Send dm to receiver pubkey
println!(
"SENDING DM with trade keys: {:?}",
ctx.trade_keys.public_key().to_hex()
);
let message_json = take_order_message
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
// Send the DM
// This is so we can wait for the gift wrap event in the main thread
let _ = send_dm(
&ctx.client,
Some(&ctx.identity_keys),
&ctx.trade_keys,
&ctx.mostro_pubkey,
message_json,
None,
false,
)
.await;
// Wait for the DM to be sent from mostro
let recv_event = wait_for_dm(ctx, None).await?;
// Parse the incoming DM
let messages = parse_dm_events(recv_event, &ctx.trade_keys, None).await;
if let Some(message) = messages.first() {
let message = message.0.get_inner_message_kind();
if message.request_id == Some(request_id) {
if let Err(e) = print_commands_results(message, None, ctx).await {
println!("Error in print_commands_results: {}", e);
}
}
}
Ok(())
}
[src/cli/add_invoice.rs:12-84]
pub async fn execute_add_invoice(order_id: &Uuid, invoice: &str, ctx: &Context) -> Result<()> {
let order = Order::get_by_id(&ctx.pool, &order_id.to_string()).await?;
let trade_keys = order
.trade_keys
.clone()
.ok_or(anyhow::anyhow!("Missing trade keys"))?;
let order_trade_keys = Keys::parse(&trade_keys)?;
println!(
"Order trade keys: {:?}",
order_trade_keys.public_key().to_hex()
);
println!(
"Sending a lightning invoice for order {} to mostro pubId {}",
order_id, ctx.mostro_pubkey
);
// Check invoice string
let ln_addr = LightningAddress::from_str(invoice);
let payload = if ln_addr.is_ok() {
Some(Payload::PaymentRequest(None, invoice.to_string(), None))
} else {
match is_valid_invoice(invoice) {
Ok(i) => Some(Payload::PaymentRequest(None, i.to_string(), None)),
Err(e) => {
return Err(anyhow::anyhow!("Invalid invoice: {}", e));
}
}
};
// Create request id
let request_id = Uuid::new_v4().as_u128() as u64;
// Create AddInvoice message
let add_invoice_message = Message::new_order(
Some(*order_id),
Some(request_id),
None,
Action::AddInvoice,
payload,
);
// Serialize the message
let message_json = add_invoice_message
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
// Send the DM
let _ = send_dm(
&ctx.client,
Some(&ctx.identity_keys),
&order_trade_keys,
&ctx.mostro_pubkey,
message_json,
None,
false,
)
.await;
// Wait for the DM to be sent from mostro
let recv_event = wait_for_dm(ctx, Some(&order_trade_keys)).await?;
// Parse the incoming DM
let messages = parse_dm_events(recv_event, &order_trade_keys, None).await;
if let Some(message) = messages.first() {
let message = message.0.get_inner_message_kind();
if message.request_id == Some(request_id) {
if let Err(e) = print_commands_results(message, Some(order.clone()), ctx).await {
println!("Error in print_commands_results: {}", e);
}
}
}
Ok(())
}
[src/cli/take_dispute.rs:64-85]
pub async fn execute_take_dispute(dispute_id: &Uuid, ctx: &Context) -> Result<()> {
println!(
"Request of take dispute {} from mostro pubId {}",
dispute_id,
ctx.mostro_pubkey.clone()
);
// Create takebuy message
let take_dispute_message = Message::new_dispute(
Some(*dispute_id),
None,
None,
Action::AdminTakeDispute,
None,
)
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
println!("Admin keys: {:?}", ctx.context_keys.public_key.to_string());
admin_send_dm(ctx, take_dispute_message).await?;
Ok(())
}
[src/cli/take_dispute.rs:7-26]
pub async fn execute_admin_add_solver(npubkey: &str, ctx: &Context) -> Result<()> {
println!(
"Request of add solver with pubkey {} from mostro pubId {}",
npubkey, &ctx.mostro_pubkey
);
// Create takebuy message
let take_dispute_message = Message::new_dispute(
Some(Uuid::new_v4()),
None,
None,
Action::AdminAddSolver,
Some(Payload::TextMessage(npubkey.to_string())),
)
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
admin_send_dm(ctx, take_dispute_message).await?;
Ok(())
}
[src/cli/take_dispute.rs:47-62]
pub async fn execute_admin_settle_dispute(dispute_id: &Uuid, ctx: &Context) -> Result<()> {
println!(
"Request of take dispute {} from mostro pubId {}",
dispute_id,
ctx.mostro_pubkey.clone()
);
// Create takebuy message
let take_dispute_message =
Message::new_dispute(Some(*dispute_id), None, None, Action::AdminSettle, None)
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
println!("Admin keys: {:?}", ctx.context_keys.public_key.to_string());
admin_send_dm(ctx, take_dispute_message).await?;
Ok(())
}
[src/cli/add_invoice.rs:12-84]
There was a problem hiding this comment.
yes but since is yet a reference coming from cli command no need to reference it again
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual pattern matching code in src/cli.rs
echo "=== Checking the actual pattern match in src/cli.rs ==="
rg -n "Commands::GetDm.*since.*from_user" src/cli.rs -A 2 -B 1
echo ""
echo "=== Checking the Commands enum definition ==="
rg -n "GetDm {" src/cli.rs -A 5
echo ""
echo "=== Checking execute_get_dm signature ==="
rg -n "pub async fn execute_get_dm" src/cli/get_dm.rs -A 3
Length of output: 508
🏁 Script executed:
#!/bin/bash
# Check the Commands enum GetDm variant definition
echo "=== Checking GetDm enum variant ==="
rg -n "GetDm \{" src/cli.rs -A 5
echo ""
echo "=== Alternative: check entire Commands enum around GetDm ==="
rg -n "/// Get the latest direct messages" src/cli.rs -A 10
echo ""
echo "=== Check if the code currently compiles ==="
cd /tmp && cargo check --manifest-path="${GITHUB_WORKSPACE}/Cargo.toml" 2>&1 | head -50
Length of output: 1710
@arkanoider, I understand the confusion, but there's actually a type mismatch here. Let me clarify:
When you pattern match on Commands::GetDm { since, from_user }, the since variable is an owned i64 value (not a reference), because the enum field is defined as:
GetDm {
since: i64, // ← owned value
from_user: bool,
}However, execute_get_dm expects a reference:
pub async fn execute_get_dm(
since: &i64, // ← expects a reference
...
)So when you call execute_get_dm(since, ...), you're passing an i64 where an &i64 is expected. This is a type error in Rust.
You need to borrow since at the call sites:
Commands::GetDm { since, from_user } => {
execute_get_dm(&since, false, from_user, ctx).await
}Commands::GetAdminDm { since, from_user } => {
execute_get_dm(&since, true, from_user, ctx).await
}If your local code compiles without the &, then you might have already made this fix locally, or perhaps you've modified the function signature to accept i64 instead of &i64. Could you double-check what you have locally?
🐰
🧬 Code Graph Analysis Results
- File: src/cli/adm_send_dm.rs, Lines 6-17
pub async fn execute_adm_send_dm(receiver: PublicKey, ctx: &Context, message: &str) -> Result<()> {
println!(
"SENDING DM with admin keys: {}",
ctx.context_keys.public_key().to_hex()
);
send_admin_gift_wrap_dm(&ctx.client, &ctx.context_keys, &receiver, message).await?;
println!("Admin gift wrap message sent to {}", receiver);
Ok(())
}
- File: src/cli/conversation_key.rs, Lines 5-17
pub async fn execute_conversation_key(trade_keys: &Keys, receiver: PublicKey) -> Result<()> {
// Derive conversation key
let ck = ConversationKey::derive(trade_keys.secret_key(), &receiver)?;
let key = ck.as_bytes();
let mut ck_hex = vec![];
for i in key {
ck_hex.push(format!("{:02x}", i));
}
let ck_hex = ck_hex.join("");
println!("Conversation key: {:?}", ck_hex);
Ok(())
}
- File: src/cli/list_disputes.rs, Lines 7-21
pub async fn execute_list_disputes(ctx: &Context) -> Result<()> {
// Print mostro pubkey
println!(
"Requesting disputes from mostro pubId - {}",
&ctx.mostro_pubkey
);
// Get orders from relays
let table_of_disputes =
fetch_events_list(ListKind::Disputes, None, None, None, ctx, None).await?;
let table = print_disputes_table(table_of_disputes)?;
println!("{table}");
Ok(())
}
- File: src/cli/restore.rs, Lines 7-32
pub async fn execute_restore(
identity_keys: &Keys,
mostro_key: PublicKey,
client: &Client,
) -> Result<()> {
let restore_message = Message::new_restore(None);
let message_json = restore_message
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
// Send the restore message to Mostro server
send_dm(
client,
Some(identity_keys),
identity_keys,
&mostro_key,
message_json,
None,
false,
)
.await?;
println!("Restore message sent successfully. Recovering pending orders and disputes...");
Ok(())
}
- File: src/cli/dm_to_user.rs, Lines 7-33
pub async fn execute_dm_to_user(
receiver: PublicKey,
client: &Client,
order_id: &Uuid,
message: &str,
pool: &SqlitePool,
) -> Result<()> {
// Get the order
let order = Order::get_by_id(pool, &order_id.to_string())
.await
.map_err(|_| anyhow::anyhow!("order {} not found", order_id))?;
// Get the trade keys
let trade_keys = match order.trade_keys.as_ref() {
Some(trade_keys) => Keys::parse(trade_keys)?,
None => anyhow::bail!("No trade_keys found for this order"),
};
// Send the DM
println!(
"SENDING DM with trade keys: {}",
trade_keys.public_key().to_hex()
);
send_gift_wrap_dm(client, &trade_keys, &receiver, message).await?;
Ok(())
}
- File: src/cli/get_dm.rs, Lines 10-38
pub async fn execute_get_dm(
since: &i64,
admin: bool,
from_user: &bool,
ctx: &Context,
) -> Result<()> {
// Get the list kind
let list_kind = match (admin, from_user) {
(true, true) => ListKind::PrivateDirectMessagesUser,
(true, false) => ListKind::DirectMessagesAdmin,
(false, true) => ListKind::PrivateDirectMessagesUser,
(false, false) => ListKind::DirectMessagesUser,
};
// Fetch the requested events
let all_fetched_events =
fetch_events_list(list_kind, None, None, None, ctx, Some(since)).await?;
// Extract (Message, u64) tuples from Event::MessageTuple variants
let mut dm_events: Vec<(Message, u64)> = Vec::new();
for event in all_fetched_events {
if let Event::MessageTuple(tuple) = event {
dm_events.push(*tuple);
}
}
print_direct_messages(&dm_events, &ctx.pool).await?;
Ok(())
}
- File: src/cli/send_dm.rs, Lines 8-47
pub async fn execute_send_dm(
receiver: PublicKey,
ctx: &Context,
order_id: &Uuid,
message: &str,
) -> Result<()> {
let message = Message::new_dm(
None,
None,
Action::SendDm,
Some(Payload::TextMessage(message.to_string())),
)
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
let trade_keys =
if let Ok(order_to_vote) = Order::get_by_id(&ctx.pool, &order_id.to_string()).await {
match order_to_vote.trade_keys.as_ref() {
Some(trade_keys) => Keys::parse(trade_keys)?,
None => {
anyhow::bail!("No trade_keys found for this order");
}
}
} else {
return Err(anyhow::anyhow!("order {} not found", order_id));
};
send_dm(
&ctx.client,
None,
&trade_keys,
&receiver,
message,
None,
false,
)
.await?;
Ok(())
}
- File: src/cli/rate_user.rs, Lines 19-59
pub async fn execute_rate_user(order_id: &Uuid, rating: &u8, ctx: &Context) -> Result<()> {
// Check boundaries
let rating_content = get_user_rate(rating)?;
// Get the trade keys
let trade_keys =
if let Ok(order_to_vote) = Order::get_by_id(&ctx.pool, &order_id.to_string()).await {
match order_to_vote.trade_keys.as_ref() {
Some(trade_keys) => Keys::parse(trade_keys)?,
None => {
return Err(anyhow::anyhow!("No trade_keys found for this order"));
}
}
} else {
return Err(anyhow::anyhow!("order {} not found", order_id));
};
// Create rating message of counterpart
let rate_message = Message::new_order(
Some(*order_id),
None,
None,
Action::RateUser,
Some(rating_content),
)
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
send_dm(
&ctx.client,
Some(&ctx.identity_keys),
&trade_keys,
&ctx.mostro_pubkey,
rate_message,
None,
false,
)
.await?;
Ok(())
}
- File: src/cli/list_orders.rs, Lines 9-72
pub async fn execute_list_orders(
kind: &Option<String>,
currency: &Option<String>,
status: &Option<String>,
ctx: &Context,
) -> Result<()> {
// Used to get upper currency string to check against a list of tickers
let mut upper_currency: Option<String> = None;
// Default status is pending
let mut status_checked: Option<Status> = Some(Status::Pending);
// Default kind is none
let mut kind_checked: Option<mostro_core::order::Kind> = None;
// New check against strings
if let Some(s) = status {
status_checked = Some(
Status::from_str(s)
.map_err(|e| anyhow::anyhow!("Not valid status '{}': {:?}", s, e))?,
);
}
// Print status requested
if let Some(status) = &status_checked {
println!("You are searching orders with status {:?}", status);
}
// New check against strings for kind
if let Some(k) = kind {
kind_checked = Some(
mostro_core::order::Kind::from_str(k)
.map_err(|e| anyhow::anyhow!("Not valid order kind '{}': {:?}", k, e))?,
);
if let Some(kind) = &kind_checked {
println!("You are searching {} orders", kind);
}
}
// Uppercase currency
if let Some(curr) = currency {
upper_currency = Some(curr.to_uppercase());
if let Some(currency) = &upper_currency {
println!("You are searching orders with currency {}", currency);
}
}
println!(
"Requesting orders from mostro pubId - {}",
&ctx.mostro_pubkey
);
// Get orders from relays
let table_of_orders = fetch_events_list(
ListKind::Orders,
status_checked,
upper_currency,
kind_checked,
ctx,
None,
)
.await?;
let table = print_orders_table(table_of_orders)?;
println!("{table}");
Ok(())
}
- File: src/cli/take_order.rs, Lines 54-124
pub async fn execute_take_order(
order_id: &Uuid,
action: Action,
invoice: &Option<String>,
amount: Option<u32>,
ctx: &Context,
) -> Result<()> {
let action_name = match action {
Action::TakeBuy => "take buy",
Action::TakeSell => "take sell",
_ => return Err(anyhow::anyhhow!("Invalid action for take order")),
};
println!(
"Request of {} order {} from mostro pubId {}",
action_name, order_id, ctx.mostro_pubkey
);
// Create payload based on action type
let payload = create_take_order_payload(action.clone(), invoice, amount)?;
// Create request id
let request_id = Uuid::new_v4().as_u128() as u64;
// Create message
let take_order_message = Message::new_order(
Some(*order_id),
Some(request_id),
Some(ctx.trade_index),
action.clone(),
payload,
);
// Send dm to receiver pubkey
println!(
"SENDING DM with trade keys: {:?}",
ctx.trade_keys.public_key().to_hex()
);
let message_json = take_order_message
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
// Send the DM
// This is so we can wait for the gift wrap event in the main thread
send_dm(
&ctx.client,
Some(&ctx.identity_keys),
&ctx.trade_keys,
&ctx.mostro_pubkey,
message_json,
None,
false,
)
.await?;
// Wait for the DM to be sent from mostro
let recv_event = wait_for_dm(ctx, None).await?;
// Parse the incoming DM
let messages = parse_dm_events(recv_event, &ctx.trade_keys, None).await;
if let Some(message) = messages.first() {
let message = message.0.get_inner_message_kind();
if message.request_id == Some(request_id) {
if let Err(e) = print_commands_results(message, None, ctx).await {
println!("Error in print_commands_results: {}", e);
}
}
}
Ok(())
}
- File: src/cli/add_invoice.rs, Lines 12-84
pub async fn execute_add_invoice(order_id: &Uuid, invoice: &str, ctx: &Context) -> Result<()> {
let order = Order::get_by_id(&ctx.pool, &order_id.to_string()).await?;
let trade_keys = order
.trade_keys
.clone()
.ok_or(anyhow::anyhow!("Missing trade keys"))?;
let order_trade_keys = Keys::parse(&trade_keys)?;
println!(
"Order trade keys: {:?}",
order_trade_keys.public_key().to_hex()
);
println!(
"Sending a lightning invoice for order {} to mostro pubId {}",
order_id, ctx.mostro_pubkey
);
// Check invoice string
let ln_addr = LightningAddress::from_str(invoice);
let payload = if ln_addr.is_ok() {
Some(Payload::PaymentRequest(None, invoice.to_string(), None))
} else {
match is_valid_invoice(invoice) {
Ok(i) => Some(Payload::PaymentRequest(None, i.to_string(), None)),
Err(e) => {
return Err(anyhow::anyhow!("Invalid invoice: {}", e));
}
}
};
// Create request id
let request_id = Uuid::new_v4().as_u128() as u64;
// Create AddInvoice message
let add_invoice_message = Message::new_order(
Some(*order_id),
Some(request_id),
None,
Action::AddInvoice,
payload,
);
// Serialize the message
let message_json = add_invoice_message
.as_json()
.map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
// Send the DM
send_dm(
&ctx.client,
Some(&ctx.identity_keys),
&order_trade_keys,
&ctx.mostro_pubkey,
message_json,
None,
false,
)
.await?;
// Wait for the DM to be sent from mostro
let recv_event = wait_for_dm(ctx, Some(&order_trade_keys)).await?;
// Parse the incoming DM
let messages = parse_dm_events(recv_event, &order_trade_keys, None).await;
if let Some(message) = messages.first() {
let message = message.0.get_inner_message_kind();
if message.request_id == Some(request_id) {
if let Some(order) = Order::get_by_id(&ctx.pool, &order_id.to_string()).await.ok() {
if let Err(e) = print_commands_results(message, Some(order.clone()), ctx).await {
println!("Error in print_commands_results: {}", e);
}
}
}
}
Ok(())
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/cli/send_msg.rs (2)
93-93: Consider removing debug println or converting to proper logging.This debug print differs from the other CLI commands (
add_invoice.rs,new_order.rs), which don't include such output. If this is intentional for debugging purposes, consider using a proper logging framework; otherwise, remove it for consistency.Apply this diff if the debug output is not needed:
- println!("Message: {:?}", message);
95-95: Make error handling consistent with other commands.Unlike
add_invoice.rs(lines 77-79) andnew_order.rs(lines 152-154), which log errors fromprint_commands_results, this line silently discards them. For consistency and better debuggability, log the error.Apply this diff to log errors consistently:
- let _ = print_commands_results(message, None, ctx).await; + if let Err(e) = print_commands_results(message, None, ctx).await { + println!("Error in print_commands_results: {}", e); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cli/add_invoice.rs(2 hunks)src/cli/new_order.rs(2 hunks)src/cli/send_msg.rs(3 hunks)src/cli/take_order.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/take_order.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/new_order.rssrc/cli/send_msg.rssrc/cli/add_invoice.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/new_order.rssrc/cli/send_msg.rssrc/cli/add_invoice.rs
📚 Learning: 2025-09-09T19:18:57.161Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/add_invoice.rs:57-79
Timestamp: 2025-09-09T19:18:57.161Z
Learning: arkanoider prefers to bubble up errors with anyhow::Result instead of using tokio::spawn with eprintln! error handling in Nostr DM sending scenarios, as the spawn is often overkill for simple send operations.
Applied to files:
src/cli/send_msg.rs
🧬 Code graph analysis (3)
src/cli/new_order.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-267)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
src/cli/send_msg.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-267)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
src/cli/add_invoice.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-267)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
🔇 Additional comments (3)
src/cli/add_invoice.rs (1)
57-81: LGTM! Error propagation properly implemented.The DM send-wait-parse-print flow is correctly implemented with proper error propagation from
send_dm. The conditional handling ofprint_commands_resultsbased on request_id matching is a good pattern.src/cli/new_order.rs (1)
132-156: LGTM! Consistent implementation.The refactored DM handling follows the same clean pattern as
add_invoice.rs: send with error propagation, wait for response, parse, and conditionally print results. Error handling is consistent across the codebase.src/cli/send_msg.rs (1)
32-38: LGTM! Clear error handling.The explicit check for missing
order_idprovides clear error feedback and prevents proceeding with invalid state.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
@arkanoider i dont understand because in the tests we did with your mostrod it worked fine, but now I tested with my mostro in commit 3e15ce3b823dc61c3d18ea26f59bf8a87675b829 and I have again the error with trade index. Before tests i deleted the DB from mostrod and mostro-cli
2025-10-08T01:55:18.331144Z INFO mostrod::app: Invalid trade index
2025-10-08T01:55:18.331211Z WARN mostrod::app: Error checking trade index: Sending cantDo message to user for InvalidTradeIndex
2025-10-08T01:55:18.573331Z INFO mostrod::util: sender key 00000018c1ae3147f9010b5d768ec2b121acf1a67f238be8e2bdd7c1f9eef705 - receiver key 8b21a31c7a021ca406284a2d927c9acbf5e0318350bc675797526722f654005a
2025-10-08T01:55:18.586215Z INFO mostrod::util: Sending DM, Event ID: 151ae67411a0f1f0d326df7855874b571e85aff03b923e6d7b35ca1eea3a0f7f to 8b21a31c7a021ca406284a2d927c9acbf5e0318350bc675797526722f654005a with payload: "{\"cant-do\":{\"version\":1,\"request_id\":11460206493788775782,\"trade_index\":null,\"id\":\"a5782bf7-01c8-4cc4-b8dc-94a9a47823a5\",\"action\":\"cant-do\",\"payload\":{\"cant_do\":\"invalid_trade_index\"}}}"
…ved code readability for new order management in db.rs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/db.rs (3)
333-363: Unused rows_affected result in insert_db.The method calls
.rows_affected()on line 361 but doesn't use the result. This value could be useful for logging or validation purposes, or the call should be removed if not needed.Consider one of these options:
- Remove the unused call if row count is not needed:
.execute(pool) - .await? - .rows_affected(); + .await?; Ok(())
- Or return and use the row count for validation:
- async fn insert_db(&self, pool: &SqlitePool) -> Result<(), sqlx::Error> { + async fn insert_db(&self, pool: &SqlitePool) -> Result<u64, sqlx::Error> { sqlx::query( r#" INSERT INTO orders (id, kind, status, amount, min_amount, max_amount, fiat_code, fiat_amount, payment_method, premium, trade_keys, counterparty_pubkey, is_mine, buyer_invoice, request_id, created_at, expires_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) "#, ) // ... bindings ... .execute(pool) - .await? - .rows_affected(); - Ok(()) + .await + .map(|result| result.rows_affected())
365-396: Unused rows_affected result in update_db.Similar to
insert_db, the method calls.rows_affected()on line 394 but doesn't use the result. Consider removing this call or using it to verify the update succeeded.Apply the same fix as suggested for
insert_db:.execute(pool) - .await? - .rows_affected(); + .await?; Ok(())
309-328: Clarify SQLite constraint error comment
Update the comment to reflect extended SQLite codes correctly: 1555 isSQLITE_CONSTRAINT_PRIMARYKEY(primary-key violation) and 2067 isSQLITE_CONSTRAINT_UNIQUE(unique-constraint violation).src/parser/dms.rs (1)
258-268: Potential panic from unwrap() in time filtering.Line 262 uses
unwrap()which will panic if the time subtraction fails. While this should be rare in practice (would require an extremely largesincevalue), it's safer to handle this explicitly.Replace the
unwrap()with proper error handling:// check if the message is older than the since time if it is, skip it if let Some(since_time) = since { // Calculate since time from now in minutes subtracting the since time - let since_time = chrono::Utc::now() - .checked_sub_signed(chrono::Duration::minutes(*since_time)) - .unwrap() - .timestamp() as u64; + let since_timestamp = match chrono::Utc::now() + .checked_sub_signed(chrono::Duration::minutes(*since_time)) + { + Some(dt) => dt.timestamp() as u64, + None => { + println!("Warning: Invalid since duration: {} minutes", since_time); + continue; + } + }; - if created_at.as_u64() < since_time { + if created_at.as_u64() < since_timestamp { continue; } }Note: Also consider renaming the variable to
since_timestamporcutoff_timestampfor clarity, assince_timeis confusing when it represents a timestamp rather than a duration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/db.rs(1 hunks)src/parser/dms.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter through manual filtering after fetching events: it uses a hardcoded 2880-minute window in the Nostr filter, then manually filters results with `if event.created_at.as_u64() < since as u64 { continue; }`.
Applied to files:
src/parser/dms.rs
🧬 Code graph analysis (1)
src/parser/dms.rs (2)
src/util.rs (1)
save_order(96-122)src/db.rs (7)
sqlx(79-79)sqlx(86-86)sqlx(196-196)sqlx(520-520)sqlx(538-538)get_by_id(519-535)delete_by_id(550-563)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/parser/dms.rs (1)
18-187: LGTM! Comprehensive command result handling.The
print_commands_resultsfunction properly handles various action types with appropriate validation, error handling, and database operations. The pattern of validating payloads and request IDs before persisting is consistent and correct.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cli/new_order.rs(3 hunks)src/cli/take_order.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/new_order.rssrc/cli/take_order.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/new_order.rssrc/cli/take_order.rs
🧬 Code graph analysis (2)
src/cli/new_order.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-273)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
src/cli/take_order.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-273)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
🔇 Additional comments (5)
src/cli/new_order.rs (2)
2-4: LGTM!The new imports align with the refactored synchronous DM handling flow. Both
print_commands_resultsandparse_dm_eventsare correctly imported and used in the response handling logic below.
123-126: LGTM!Enhanced logging with trade index and trade keys improves debuggability when troubleshooting DM sending issues.
src/cli/take_order.rs (3)
9-10: LGTM!The new imports support the refactored synchronous DM handling flow, consistent with the changes in
new_order.rs.
75-76: LGTM!Request ID creation enables proper correlation between the sent message and Mostro's response.
89-92: LGTM!Enhanced logging matches the pattern in
new_order.rsand aids in debugging DM flows.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/cli/new_order.rs (1)
145-165: Extract common response-handling logic into a helper function.The response-handling pattern (wait → parse → match request_id → print results) is duplicated in both
new_order.rsandtake_order.rs. A shared helper would reduce duplication and centralize error messages.Consider extracting into
src/util.rsor a new module:pub async fn handle_dm_response( ctx: &Context, expected_request_id: u64, operation: &str, ) -> Result<()> { let recv_event = wait_for_dm(ctx, None).await?; let messages = parse_dm_events(recv_event, &ctx.trade_keys, None).await; if let Some((message, _, _)) = messages.first() { let message = message.get_inner_message_kind(); if message.request_id == Some(expected_request_id) { print_commands_results(message, None, ctx).await?; } else { return Err(anyhow::anyhow!( "Received response with mismatched request_id for {}. Expected: {}, Got: {:?}", operation, expected_request_id, message.request_id )); } } else { return Err(anyhow::anyhow!( "No valid response received from Mostro after {}", operation )); } Ok(()) }Then call it as:
handle_dm_response(ctx, request_id, "sending new order").await?;src/cli/take_order.rs (1)
111-131: Refactor: use the shared helper from new_order.rs.This response-handling code is duplicated from
new_order.rs. Once the helper function suggested innew_order.rsis extracted, replace this block with:handle_dm_response(ctx, request_id, "taking order").await?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cli/new_order.rs(3 hunks)src/cli/take_order.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/take_order.rssrc/cli/new_order.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/take_order.rs
🧬 Code graph analysis (2)
src/cli/take_order.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-273)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
src/cli/new_order.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-273)src/util.rs (2)
send_dm(260-312)wait_for_dm(125-170)
🪛 GitHub Actions: CI Mostro-cli
src/cli/take_order.rs
[error] 117-117: no field 0 on type &mostro_core::message::Message
src/cli/new_order.rs
[error] 151-151: no field 0 on type &mostro_core::message::Message
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (5)
Cargo.toml(1 hunks)src/cli/add_invoice.rs(3 hunks)src/cli/new_order.rs(3 hunks)src/cli/take_order.rs(3 hunks)src/util.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- src/cli/take_order.rs
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/add_invoice.rs:80-82
Timestamp: 2025-09-09T19:58:58.506Z
Learning: In the mostro-cli codebase, when the trade_index parameter isn't needed inside a function like wait_for_dm, it should be made Optional<i64> instead of requiring callers to pass hardcoded values like 0. This makes the API clearer and avoids unnecessary computation for resolving indices when they're not used.
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/add_invoice.rssrc/cli/new_order.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/add_invoice.rs
📚 Learning: 2025-09-09T19:58:58.506Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/add_invoice.rs:80-82
Timestamp: 2025-09-09T19:58:58.506Z
Learning: In the mostro-cli codebase, when the trade_index parameter isn't needed inside a function like wait_for_dm, it should be made Optional<i64> instead of requiring callers to pass hardcoded values like 0. This makes the API clearer and avoids unnecessary computation for resolving indices when they're not used.
Applied to files:
src/util.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter through manual filtering after fetching events: it uses a hardcoded 2880-minute window in the Nostr filter, then manually filters results with `if event.created_at.as_u64() < since as u64 { continue; }`.
Applied to files:
src/util.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter properly by creating filters with `since(Timestamp::from(since))`, so no additional local filtering is needed in CLI functions that call it.
Applied to files:
src/util.rs
🧬 Code graph analysis (3)
src/cli/add_invoice.rs (3)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-273)src/db.rs (1)
get_by_id(519-535)src/util.rs (2)
send_dm(259-311)wait_for_dm(125-169)
src/util.rs (2)
src/db.rs (2)
new(139-160)new(278-331)src/parser/dms.rs (1)
parse_dm_events(189-273)
src/cli/new_order.rs (3)
src/parser/dms.rs (2)
print_commands_results(19-187)parse_dm_events(189-273)src/parser/orders.rs (1)
print_order_preview(76-148)src/util.rs (3)
send_dm(259-311)None(58-58)wait_for_dm(125-169)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: MSRV build (1.86.0)
- GitHub Check: clippy
src/cli/add_invoice.rs
Outdated
| if let Some((message, _, _)) = messages.first() { | ||
| let message = message.get_inner_message_kind(); | ||
| if message.request_id == Some(request_id) { | ||
| print_commands_results(message, None, ctx).await?; | ||
| } else { |
There was a problem hiding this comment.
Provide the fetched order to print_commands_results.
print_commands_results’s Action::WaitingSellerToPay arm (see src/parser/dms.rs) takes the Order from the supplied Option<Order> to mark it WaitingPayment. Passing None therefore always yields Err("No order found in message"), so this command fails right after Mostro confirms the invoice. Reuse the order you loaded at the top of the function (e.g. wrap it in Some(order)) before calling print_commands_results.
- print_commands_results(message, None, ctx).await?;
+ print_commands_results(message, Some(order), ctx).await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some((message, _, _)) = messages.first() { | |
| let message = message.get_inner_message_kind(); | |
| if message.request_id == Some(request_id) { | |
| print_commands_results(message, None, ctx).await?; | |
| } else { | |
| if let Some((message, _, _)) = messages.first() { | |
| let message = message.get_inner_message_kind(); | |
| if message.request_id == Some(request_id) { | |
| print_commands_results(message, Some(order), ctx).await?; | |
| } else { | |
| // … | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/cli/add_invoice.rs around lines 77 to 81, the code calls
print_commands_results(message, None, ctx) which causes the
Action::WaitingSellerToPay arm to receive None and fail; fix this by passing the
previously loaded order (wrap it as Some(order)) instead of None—if ownership
issues arise, clone or otherwise ensure the order value is available at this
call—so call print_commands_results(message, Some(order), ctx).
… to be tested - improved some part of code where messsage are received - tried to improve message reception with not responsive relays ( catrya issue )
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cli.rs (1)
498-504: Fix type mismatch: borrowsincewhen callingexecute_get_dm.In the pattern match,
sinceis bound as an ownedi64(from the enum fieldGetDm { since: i64, ... }), butexecute_get_dmexpects&i64. You must borrow it at both call sites.Apply this diff:
Commands::GetDm { since, from_user } => { - execute_get_dm(since, false, from_user, ctx).await + execute_get_dm(&since, false, from_user, ctx).await } Commands::GetDmUser { since } => execute_get_dm_user(since, ctx).await, Commands::GetAdminDm { since, from_user } => { - execute_get_dm(since, true, from_user, ctx).await + execute_get_dm(&since, true, from_user, ctx).await }
🧹 Nitpick comments (1)
src/util.rs (1)
576-594: Helper design limits extensibility for commands requiring order context.The
print_dm_eventshelper always passesNonefor the order parameter when callingprint_commands_results. This works for most commands but fails foradd_invoicewhen the response isAction::WaitingSellerToPay, which requires an order to update its status.Consider extending the helper to accept an optional order parameter:
-pub async fn print_dm_events(recv_event: Events, request_id: u64, ctx: &Context) -> Result<()> { +pub async fn print_dm_events( + recv_event: Events, + request_id: u64, + order: Option<Order>, + ctx: &Context, +) -> Result<()> { // Parse the incoming DM let messages = parse_dm_events(recv_event, &ctx.trade_keys, None).await; if let Some((message, _, _)) = messages.first() { let message = message.get_inner_message_kind(); if message.request_id == Some(request_id) { - print_commands_results(message, None, ctx).await?; + print_commands_results(message, order, ctx).await?; } else { return Err(anyhow::anyhow!( "Received response with mismatched request_id. Expected: {}, Got: {:?}", request_id, message.request_id )); } } else { return Err(anyhow::anyhow!("No response received from Mostro")); } Ok(()) }Then update call sites to pass
Nonefor commands that don't need an order, andSome(order)foradd_invoice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/cli.rs(5 hunks)src/cli/add_invoice.rs(3 hunks)src/cli/last_trade_index.rs(1 hunks)src/cli/new_order.rs(3 hunks)src/cli/send_msg.rs(3 hunks)src/cli/take_order.rs(3 hunks)src/parser/dms.rs(2 hunks)src/util.rs(10 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/take_order.rssrc/cli/add_invoice.rssrc/cli/new_order.rssrc/cli/send_msg.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/take_order.rssrc/cli/add_invoice.rssrc/cli/send_msg.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter through manual filtering after fetching events: it uses a hardcoded 2880-minute window in the Nostr filter, then manually filters results with `if event.created_at.as_u64() < since as u64 { continue; }`.
Applied to files:
src/parser/dms.rssrc/util.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter properly by creating filters with `since(Timestamp::from(since))`, so no additional local filtering is needed in CLI functions that call it.
Applied to files:
src/util.rs
📚 Learning: 2025-09-09T19:18:57.161Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/add_invoice.rs:57-79
Timestamp: 2025-09-09T19:18:57.161Z
Learning: arkanoider prefers to bubble up errors with anyhow::Result instead of using tokio::spawn with eprintln! error handling in Nostr DM sending scenarios, as the spawn is often overkill for simple send operations.
Applied to files:
src/cli/send_msg.rs
🧬 Code graph analysis (8)
src/cli/take_order.rs (1)
src/util.rs (4)
print_dm_events(576-594)send_dm(261-313)wait_for_dm(126-171)None(59-59)
src/cli/last_trade_index.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-204)parse_dm_events(206-290)src/util.rs (2)
send_dm(261-313)wait_for_dm(126-171)
src/cli.rs (3)
src/cli/last_trade_index.rs (1)
execute_last_trade_index(11-57)src/util.rs (1)
run_simple_order_msg(553-559)src/cli/get_dm.rs (1)
execute_get_dm(10-38)
src/parser/dms.rs (2)
src/util.rs (1)
save_order(97-123)src/db.rs (8)
sqlx(79-79)sqlx(86-86)sqlx(196-196)sqlx(520-520)sqlx(538-538)get_by_id(519-535)delete_by_id(550-563)get(195-207)
src/cli/add_invoice.rs (2)
src/util.rs (4)
print_dm_events(576-594)send_dm(261-313)wait_for_dm(126-171)None(59-59)src/db.rs (1)
get_by_id(519-535)
src/util.rs (3)
src/parser/dms.rs (2)
print_commands_results(19-204)parse_dm_events(206-290)src/db.rs (2)
new(139-160)new(278-331)src/cli/send_msg.rs (1)
execute_send_msg(10-94)
src/cli/new_order.rs (1)
src/util.rs (4)
print_dm_events(576-594)send_dm(261-313)wait_for_dm(126-171)None(59-59)
src/cli/send_msg.rs (1)
src/util.rs (4)
print_dm_events(576-594)send_dm(261-313)wait_for_dm(126-171)None(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (8)
src/cli/take_order.rs (1)
96-113: LGTM!The refactored DM flow is clean: synchronous send → wait → parse. The
print_dm_eventshelper works correctly here since none of the take-order response actions require a pre-existing order parameter.src/cli/send_msg.rs (1)
74-90: LGTM!The synchronous DM flow is correctly implemented. Using
print_dm_eventsis appropriate here since the command actions (FiatSent, Release, Cancel, etc.) don't require a pre-existing order parameter.src/cli/new_order.rs (1)
131-147: LGTM!The DM flow refactor is well-structured. For new orders,
print_dm_eventscorrectly passesNonesince the order is created from the response payload rather than requiring a pre-existing order.src/cli/last_trade_index.rs (1)
11-57: LGTM!The new
execute_last_trade_indexfunction implements the LastTradeIndex flow correctly:
- Sends the restore message with the appropriate action
- Waits for and parses the DM response
- Validates the action matches expectations
- Updates the user's trade index via
print_commands_resultsThe error handling for mismatched actions and missing responses is thorough.
src/cli.rs (1)
425-427: LGTM!The new
GetLastTradeIndexcommand is properly wired to callexecute_last_trade_indexwith the required parameters.src/parser/dms.rs (2)
18-204: LGTM!The new
print_commands_resultsfunction provides comprehensive handling for various Mostro response actions:
- Proper validation (request_id, order_id checks)
- Database persistence via
save_orderfor new orders- Status updates for existing orders
- Clear error messages for different
CantDoreasons- LastTradeIndex handling with user state updates
The implementation is well-structured and error propagation is correct.
206-290: LGTM!The updated
parse_dm_eventssignature now supports optional time-based filtering via thesinceparameter. The implementation correctly:
- Computes the cutoff timestamp from
now - since minutes- Skips messages older than the cutoff
- Maintains deduplication via
id_set- Preserves sorting by timestamp
src/util.rs (1)
125-171: LGTM!The
wait_for_dmrefactor is well-designed:
- Context-based API is cleaner than passing multiple parameters
- Optional
order_trade_keysparameter provides flexibility (defaults toctx.trade_keys)limit(0)ensures only new gift wraps are fetched- Exit policy
WaitForEventsAfterEOSE(1)auto-closes after receiving one event- Proper timeout handling with clear error messages
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/cli/add_invoice.rs (1)
58-73: Still need to pass the order and correct keypair when handling the DMThe reply for
AddInvoiceis encrypted to the order’s trade keys andprint_commands_results’sWaitingSellerToPayarm requires the loadedOrder. Feeding everything throughprint_dm_eventsmeans we decrypt withctx.trade_keys(often wrong for historical orders) and always handNonetoprint_commands_results, so the flow trips the"No order found in message"error that was already reported.Please switch back to parsing with
order_trade_keysand forwardSome(order)toprint_commands_results, mirroring the fix suggested earlier (or extend the helper to accept those parameters).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/cli/add_invoice.rs(3 hunks)src/cli/last_trade_index.rs(1 hunks)src/cli/new_order.rs(3 hunks)src/cli/send_msg.rs(3 hunks)src/cli/take_order.rs(3 hunks)src/util.rs(11 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/new_order.rssrc/cli/take_order.rssrc/cli/send_msg.rssrc/cli/add_invoice.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/new_order.rssrc/cli/take_order.rssrc/cli/send_msg.rssrc/cli/add_invoice.rs
📚 Learning: 2025-09-09T19:18:57.161Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/add_invoice.rs:57-79
Timestamp: 2025-09-09T19:18:57.161Z
Learning: arkanoider prefers to bubble up errors with anyhow::Result instead of using tokio::spawn with eprintln! error handling in Nostr DM sending scenarios, as the spawn is often overkill for simple send operations.
Applied to files:
src/cli/send_msg.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter through manual filtering after fetching events: it uses a hardcoded 2880-minute window in the Nostr filter, then manually filters results with `if event.created_at.as_u64() < since as u64 { continue; }`.
Applied to files:
src/util.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter properly by creating filters with `since(Timestamp::from(since))`, so no additional local filtering is needed in CLI functions that call it.
Applied to files:
src/util.rs
🧬 Code graph analysis (6)
src/cli/last_trade_index.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-204)parse_dm_events(206-290)src/util.rs (2)
send_dm(272-324)wait_for_dm(127-182)
src/cli/new_order.rs (1)
src/util.rs (5)
print_dm_events(587-605)send_dm(272-324)uppercase_first(546-552)wait_for_dm(127-182)None(60-60)
src/cli/take_order.rs (1)
src/util.rs (4)
print_dm_events(587-605)send_dm(272-324)wait_for_dm(127-182)None(60-60)
src/cli/send_msg.rs (1)
src/util.rs (4)
print_dm_events(587-605)send_dm(272-324)wait_for_dm(127-182)None(60-60)
src/cli/add_invoice.rs (2)
src/util.rs (4)
print_dm_events(587-605)send_dm(272-324)wait_for_dm(127-182)None(60-60)src/db.rs (1)
get_by_id(519-535)
src/util.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-204)parse_dm_events(206-290)src/cli/send_msg.rs (1)
execute_send_msg(10-93)
🪛 GitHub Actions: CI Mostro-cli
src/cli/last_trade_index.rs
[error] 34-34: Clippy: to_string applied to a type that implements Display in println! args. Remove this to_string in format arguments. (see: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args)
🔇 Additional comments (5)
src/util.rs (5)
4-4: LGTM: Import additions support the refactored DM handling.The new imports are correctly used in the refactored
wait_for_dmfunction and the newprint_dm_eventsfunction.Also applies to: 15-15
452-543: LGTM: Thesinceparameter is now properly utilized.The parameter was renamed from
_sincetosinceand is now correctly passed toparse_dm_eventsacross all list kinds that support time filtering. This enhances the ability to filter DM events by time.
564-570: All callers match the updated signature: the sole invocation insrc/cli.rsnow passesSome(*order_id), aligning with the newOption<Uuid>parameter.
98-124: Verify save_order callers include new parametersCallers in
src/parser/dms.rscurrently span only three args; ensure they now supplytrade_indexandpool. Run:#!/bin/bash rg -nP --type=rust -C10 'save_order\s*\(' src/parser/dms.rs
127-182: Approve wait_for_dm refactor
- All call sites now pass a
send_dm(…)future, whoseOutput = Result<()>satisfiesF: Future<Output = Result<()>> + Send.Filter::limit(0)withReqExitPolicy::WaitForEventsAfterEOSE(1)correctly subscribes only to new events and auto-closes once one event arrives.- No further changes required.
src/cli/send_msg.rs
Outdated
| // Send DM | ||
| let sent_message = send_dm( | ||
| &ctx.client, | ||
| Some(&idkey), | ||
| Some(&ctx.identity_keys), | ||
| &trade_keys, | ||
| &ctx.mostro_pubkey, | ||
| message_json, | ||
| None, | ||
| false, | ||
| ) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("Failed to send DM: {e}"))?; | ||
| ); | ||
|
|
||
| // Wait for the DM to be sent from mostro | ||
| wait_for_dm( | ||
| &ctx.client, | ||
| &trade_keys, | ||
| request_id, | ||
| None, | ||
| Some(order), | ||
| &ctx.pool, | ||
| ) | ||
| .await | ||
| .map_err(|e| anyhow::anyhow!("Failed to wait for DM: {e}"))?; | ||
| // Wait for incoming DM | ||
| let recv_event = wait_for_dm(ctx, Some(&trade_keys), sent_message).await?; | ||
|
|
||
| // Parse the incoming DM | ||
| print_dm_events(recv_event, request_id, ctx).await?; |
There was a problem hiding this comment.
Restore per-order keys and order context when processing the DM reply
print_dm_events always decrypts with ctx.trade_keys and passes None into print_commands_results. When these commands operate on an order stored with its own trade_keys, we’re handing the wrong keypair to parse_dm_events, so the reply fails to decrypt. Even if it does, actions like WaitingSellerToPay require the Order you just loaded; passing None reproduces the "No order found in message" failure that was previously flagged.
Handle the response with the order’s keys and forward the order to print_commands_results, e.g.:
-use crate::util::{print_dm_events, send_dm, wait_for_dm};
+use crate::{
+ parser::{dms::print_commands_results, parse_dm_events},
+ util::{send_dm, wait_for_dm},
+};
@@
- // Parse the incoming DM
- print_dm_events(recv_event, request_id, ctx).await?;
+ // Parse the incoming DM with the order’s trade keys
+ let messages = parse_dm_events(recv_event, &trade_keys, None).await;
+ if let Some((message, _, _)) = messages.first() {
+ let message = message.get_inner_message_kind();
+ if message.request_id == Some(request_id) {
+ print_commands_results(message, Some(order), ctx).await?;
+ } else {
+ return Err(anyhow::anyhow!(
+ "Received response with mismatched request_id. Expected: {}, Got: {:?}",
+ request_id,
+ message.request_id
+ ));
+ }
+ } else {
+ return Err(anyhow::anyhow!("No response received from Mostro"));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Send DM | |
| let sent_message = send_dm( | |
| &ctx.client, | |
| Some(&idkey), | |
| Some(&ctx.identity_keys), | |
| &trade_keys, | |
| &ctx.mostro_pubkey, | |
| message_json, | |
| None, | |
| false, | |
| ) | |
| .await | |
| .map_err(|e| anyhow::anyhow!("Failed to send DM: {e}"))?; | |
| ); | |
| // Wait for the DM to be sent from mostro | |
| wait_for_dm( | |
| &ctx.client, | |
| &trade_keys, | |
| request_id, | |
| None, | |
| Some(order), | |
| &ctx.pool, | |
| ) | |
| .await | |
| .map_err(|e| anyhow::anyhow!("Failed to wait for DM: {e}"))?; | |
| // Wait for incoming DM | |
| let recv_event = wait_for_dm(ctx, Some(&trade_keys), sent_message).await?; | |
| // Parse the incoming DM | |
| print_dm_events(recv_event, request_id, ctx).await?; | |
| // At the top of src/cli/send_msg.rs, replace the existing print_dm_events import: | |
| use crate::{ | |
| parser::{dms::print_commands_results, parse_dm_events}, | |
| util::{send_dm, wait_for_dm}, | |
| }; | |
| // … | |
| // Send DM | |
| let sent_message = send_dm( | |
| &ctx.client, | |
| Some(&ctx.identity_keys), | |
| &trade_keys, | |
| &ctx.mostro_pubkey, | |
| message_json, | |
| None, | |
| false, | |
| ); | |
| // Wait for incoming DM | |
| let recv_event = wait_for_dm(ctx, Some(&trade_keys), sent_message).await?; | |
| // Parse the incoming DM with the order’s trade keys | |
| let messages = parse_dm_events(recv_event, &trade_keys, None).await; | |
| if let Some((message, _, _)) = messages.first() { | |
| let message = message.get_inner_message_kind(); | |
| if message.request_id == Some(request_id) { | |
| print_commands_results(message, Some(order), ctx).await?; | |
| } else { | |
| return Err(anyhow::anyhow!( | |
| "Received response with mismatched request_id. Expected: {}, Got: {:?}", | |
| request_id, | |
| message.request_id | |
| )); | |
| } | |
| } else { | |
| return Err(anyhow::anyhow!("No response received from Mostro")); | |
| } |
🤖 Prompt for AI Agents
In src/cli/send_msg.rs around lines 74 to 89, restore and use the per-order
trade keys and order context when handling the DM reply: after receiving and
parsing the reply, load the Order and its specific trade_keys, call
print_dm_events (and/or parse_dm_events) using that order.trade_keys instead of
ctx.trade_keys, and pass Some(order) into print_commands_results so command
handlers receive the correct Order context; ensure decryption uses the order's
keypair and propagate the order object to downstream calls.
src/util.rs
Outdated
| pub async fn print_dm_events(recv_event: Events, request_id: u64, ctx: &Context) -> Result<()> { | ||
| // Parse the incoming DM | ||
| let messages = parse_dm_events(recv_event, &ctx.trade_keys, None).await; | ||
| if let Some((message, _, _)) = messages.first() { | ||
| let message = message.get_inner_message_kind(); | ||
| if message.request_id == Some(request_id) { | ||
| print_commands_results(message, None, ctx).await?; | ||
| } else { | ||
| return Err(anyhow::anyhow!( | ||
| "Received response with mismatched request_id. Expected: {}, Got: {:?}", | ||
| request_id, | ||
| message.request_id | ||
| )); | ||
| } | ||
| } else { | ||
| return Err(anyhow::anyhow!("No response received from Mostro")); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Potential issue: Passing None for order parameter may cause failures.
The print_dm_events function always passes None for the order parameter when calling print_commands_results. However, certain response actions like Action::WaitingSellerToPay expect an order to be present and will return an error if it's None:
// From print_commands_results
Action::WaitingSellerToPay => {
if let Some(mut order) = order.take() {
// ... update status
Ok(())
} else {
Err(anyhow::anyhow!("No order found in message"))
}
}If this action can be received in the flows using print_dm_events (e.g., new_order, take_order, send_msg, add_invoice), consider fetching the order from the database using the order_id from the message before calling print_commands_results.
Run this script to check if WaitingSellerToPay responses can occur in the identified flows:
🏁 Script executed:
#!/bin/bash
# Description: Check the Mostro protocol documentation or code to determine valid response actions
# Search for flows using print_dm_events
echo "=== Files calling print_dm_events ==="
rg -nP --type=rust -C3 '\bprint_dm_events\s*\('
echo -e "\n=== WaitingSellerToPay action usage ==="
rg -nP --type=rust -C3 '\bWaitingSellerToPay\b'Length of output: 2359
Propagate parsed order to print_commands_results instead of passing None
print_dm_events currently discards the Option<Order> from parse_dm_events, causing actions like WaitingSellerToPay to fail when no order is provided. Extract the order from the parse result and pass it into print_commands_results rather than using None.
🤖 Prompt for AI Agents
In src/util.rs around lines 587 to 605, the parsed order returned by
parse_dm_events is being discarded (pattern uses (_, _, _)) and None is passed
to print_commands_results; change the tuple pattern to capture the order (e.g.
Some((message, _, order))) and pass that captured order into
print_commands_results instead of None so the function receives the
Option<Order> produced by parse_dm_events.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
16-19: Add fenced code languages to satisfy markdownlint.Our lint (MD040) now fires on each new code fence because no language label is provided. Please tag them—e.g. use
```bashfor shell snippets and```text(or similar) for the commands listing—to clear the warnings.Also applies to: 26-29, 34-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli/last_trade_index.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cli/last_trade_index.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-204)parse_dm_events(206-290)src/util.rs (2)
send_dm(272-324)wait_for_dm(127-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/cli/last_trade_index.rs (2)
34-37: LGTM! Clippy lint fixed.The redundant
.to_string()call flagged in the previous review has been correctly removed.PublicKey'sDisplayimplementation is now used directly.
23-31: No changes required; usingidentity_keysfor both parameters is correct for LastTradeIndex.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/util.rs (1)
587-612: Fetch order from database before calling print_commands_results.The function passes
Nonefor the order parameter toprint_commands_results(line 600), but certain actions likeAction::WaitingSellerToPayinprint_commands_results(src/parser/dms.rs:49-64) expect an order to be present and return an error if it'sNone.Modify the function to fetch the order from the database when an order_id is present in the message:
pub async fn print_dm_events( recv_event: Events, request_id: u64, ctx: &Context, order_trade_keys: Option<&Keys>, ) -> Result<()> { // Get the trade keys let trade_keys = order_trade_keys.unwrap_or(&ctx.trade_keys); // Parse the incoming DM let messages = parse_dm_events(recv_event, trade_keys, None).await; if let Some((message, _, _)) = messages.first() { let message = message.get_inner_message_kind(); if message.request_id == Some(request_id) { + // Fetch order from database if order_id is present + let order = if let Some(order_id) = &message.id { + Order::get_by_id(&ctx.pool, &order_id.to_string()).await.ok() + } else { + None + }; - print_commands_results(message, None, ctx).await?; + print_commands_results(message, order, ctx).await?; } else { return Err(anyhow::anyhow!( "Received response with mismatched request_id. Expected: {}, Got: {:?}", request_id, message.request_id )); } } else { return Err(anyhow::anyhow!("No response received from Mostro")); } Ok(()) }
🧹 Nitpick comments (2)
README.md (1)
17-30: Add language hints to fenced code blocks.markdownlint (MD040) now flags these new fences because they lack a language identifier. Please tag them (e.g.,
bash,sh) so docs stay lint-clean.src/util.rs (1)
127-182: Consider making the timeout configurable.The function uses a hardcoded
FETCH_EVENTS_TIMEOUT(15 seconds) for waiting on DM responses. Depending on network conditions and relay responsiveness, this timeout might be too short or too long.Consider adding a timeout parameter to the function signature or making it configurable through environment variables:
pub async fn wait_for_dm<F>( ctx: &Context, order_trade_keys: Option<&Keys>, sent_message: F, timeout: Option<Duration>, ) -> anyhow::Result<Events> where F: Future<Output = Result<()>> + Send, { let timeout_duration = timeout.unwrap_or(FETCH_EVENTS_TIMEOUT); // ... rest of the function let event = tokio::time::timeout(timeout_duration, async move { // ... }) // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)src/cli/add_invoice.rs(3 hunks)src/cli/last_trade_index.rs(1 hunks)src/cli/new_order.rs(3 hunks)src/cli/send_msg.rs(3 hunks)src/cli/take_order.rs(3 hunks)src/parser/dms.rs(2 hunks)src/util.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/add_invoice.rs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter through manual filtering after fetching events: it uses a hardcoded 2880-minute window in the Nostr filter, then manually filters results with `if event.created_at.as_u64() < since as u64 { continue; }`.
Applied to files:
src/util.rssrc/parser/dms.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter properly by creating filters with `since(Timestamp::from(since))`, so no additional local filtering is needed in CLI functions that call it.
Applied to files:
src/util.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/send_msg.rssrc/cli/take_order.rssrc/cli/new_order.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/send_msg.rssrc/cli/take_order.rssrc/cli/new_order.rs
📚 Learning: 2025-09-09T19:18:57.161Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/add_invoice.rs:57-79
Timestamp: 2025-09-09T19:18:57.161Z
Learning: arkanoider prefers to bubble up errors with anyhow::Result instead of using tokio::spawn with eprintln! error handling in Nostr DM sending scenarios, as the spawn is often overkill for simple send operations.
Applied to files:
src/cli/send_msg.rs
🧬 Code graph analysis (6)
src/cli/last_trade_index.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-208)parse_dm_events(210-294)src/util.rs (2)
send_dm(272-324)wait_for_dm(127-182)
src/util.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-208)parse_dm_events(210-294)src/cli/send_msg.rs (1)
execute_send_msg(10-93)
src/parser/dms.rs (2)
src/util.rs (1)
save_order(98-124)src/db.rs (8)
sqlx(79-79)sqlx(86-86)sqlx(196-196)sqlx(520-520)sqlx(538-538)get_by_id(519-535)delete_by_id(550-563)get(195-207)
src/cli/send_msg.rs (1)
src/util.rs (4)
print_dm_events(587-612)send_dm(272-324)wait_for_dm(127-182)None(60-60)
src/cli/take_order.rs (1)
src/util.rs (4)
print_dm_events(587-612)send_dm(272-324)wait_for_dm(127-182)None(60-60)
src/cli/new_order.rs (1)
src/util.rs (4)
print_dm_events(587-612)send_dm(272-324)wait_for_dm(127-182)None(60-60)
🪛 markdownlint-cli2 (0.18.1)
README.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| // check if the message is older than the since time if it is, skip it | ||
| if let Some(since_time) = since { | ||
| // Calculate since time from now in minutes subtracting the since time | ||
| let since_time = chrono::Utc::now() | ||
| .checked_sub_signed(chrono::Duration::minutes(*since_time)) | ||
| .unwrap() | ||
| .timestamp() as u64; | ||
|
|
||
| let since_time = match chrono::Utc::now().checked_sub_signed(chrono::Duration::minutes(30)) | ||
| { | ||
| Some(dt) => dt.timestamp() as u64, | ||
| None => { | ||
| println!("Error: Unable to calculate time 30 minutes ago"); | ||
| if created_at.as_u64() < since_time { | ||
| continue; | ||
| } | ||
| }; | ||
| if created_at.as_u64() < since_time { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Replace .unwrap() with proper error handling.
Line 283 uses .unwrap() which will panic if the time calculation fails. This can occur in edge cases with system time changes or invalid since values.
Apply this diff to handle the error properly:
// check if the message is older than the since time if it is, skip it
if let Some(since_time) = since {
// Calculate since time from now in minutes subtracting the since time
- let since_time = chrono::Utc::now()
+ let cutoff_time = chrono::Utc::now()
.checked_sub_signed(chrono::Duration::minutes(*since_time))
- .unwrap()
- .timestamp() as u64;
+ .ok_or(anyhow::anyhow!("Invalid since time calculation"))?
+ .timestamp() as u64;
- if created_at.as_u64() < since_time {
+ if created_at.as_u64() < cutoff_time {
continue;
}
}Note: Also renamed the shadowed variable since_time to cutoff_time for clarity.
🤖 Prompt for AI Agents
In src/parser/dms.rs around lines 278 to 289, replace the .unwrap() on the
result of checked_sub_signed with proper handling and rename the shadowed
since_time to cutoff_time: compute cutoff_time by calling
checked_sub_signed(...).map(|dt| dt.timestamp() as u64) and handle the None case
(e.g., log the error and continue or return Err depending on surrounding
function signature) instead of unwrapping; then compare created_at.as_u64() with
cutoff_time. Ensure no panics and use a clear name cutoff_time.
@Catrya @grunch ,
this is another part of the refactoring of mostro-cli, we are testing with @Catrya and now seems in an usable state.
Tomorrow I will read @Catrya opinion about and in case we can merge all this things, so also @AndreaDiazCorreia pr can be merged with no conflicts.
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores