refactor: Split big utils.rs file in a module for being more usable #145
Conversation
WalkthroughReplaces the monolithic Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI Command
participant Net as util::net
participant Msg as util::messaging
participant Nostr as Nostr Relays
User->>CLI: trigger send DM / gift-wrap
CLI->>Net: connect_nostr()
Net->>Nostr: add relays + connect
Net-->>CLI: Client
CLI->>Msg: send_dm(payload, to_user/flags)
alt PrivateDirectMessage
Msg->>Msg: create_private_dm_event()
else GiftWrap
Msg->>Msg: create_gift_wrap_event(signed?, expiration)
end
Msg->>Nostr: publish event
CLI->>Msg: wait_for_dm(sent_future)
Msg->>Nostr: subscribe (timeout)
Nostr-->>Msg: incoming events
Msg-->>CLI: Events
CLI->>Msg: print_dm_events(request_id)
Msg-->>User: parsed output
sequenceDiagram
autonumber
participant CLI as CLI List
participant Events as util::events
participant Nostr as Nostr Relays
participant Types as util::types
CLI->>Events: fetch_events_list(ListKind, ..., since)
Events->>Events: create_filter(...)
Events->>Nostr: fetch events (with timeout)
Nostr-->>Events: events
Events->>Events: parse_orders / parse_dm_events / parse_disputes
Events-->>CLI: Vec<Types::Event> (SmallOrder / MessageTuple / Dispute)
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)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/parser/dms.rs (1)
219-222: Differentiate messages for Released vs HoldInvoicePaymentSettledUsing the same print for both can confuse users. Consider distinct outputs (e.g., “Sats released to buyer” vs “Hold invoice settled”).
- Action::HoldInvoicePaymentSettled | Action::Released => { - println!("Hold invoice payment settled"); + Action::HoldInvoicePaymentSettled => { + println!("Hold invoice payment settled"); + Ok(()) + } + Action::Released => { + println!("Sats released by seller"); Ok(()) }src/util/misc.rs (1)
11-19: Avoid panics, use PathBuf join, and create_dir_all in get_mcli_pathCurrent code panics on missing HOME and has a TOCTOU check with create_dir. Prefer robust, non-panicking behavior and idiomatic paths.
-pub fn get_mcli_path() -> String { - let home_dir = dirs::home_dir().expect("Couldn't get home directory"); - let mcli_path = format!("{}/.mcli", home_dir.display()); - if !Path::new(&mcli_path).exists() { - fs::create_dir(&mcli_path).expect("Couldn't create mostro-cli directory in HOME"); - println!("Directory {} created.", mcli_path); - } - mcli_path -} +pub fn get_mcli_path() -> String { + let home_dir = dirs::home_dir().unwrap_or_else(|| { + eprintln!("Warning: HOME not found; using current directory"); + std::env::current_dir().unwrap_or_default() + }); + let mcli_path = home_dir.join(".mcli"); + if let Err(e) = fs::create_dir_all(&mcli_path) { + eprintln!( + "Warning: couldn't ensure {} exists: {}", + mcli_path.display(), + e + ); + } + mcli_path.to_string_lossy().into_owned() +}src/util/net.rs (1)
5-16: Harden RELAYS parsing; avoid panics; consider passing Keys instead of generating
- var("RELAYS").expect panics; trim/filter relays; error if empty.
- Generating keys makes the client identity ephemeral and likely unusable for DM flows; prefer taking Keys as a parameter (or provide a separate function).
-pub async fn connect_nostr() -> Result<Client> { - let my_keys = Keys::generate(); - - let relays = var("RELAYS").expect("RELAYS is not set"); - let relays = relays.split(',').collect::<Vec<&str>>(); - let client = Client::new(my_keys); - for r in relays.into_iter() { - client.add_relay(r).await?; - } - client.connect().await; - Ok(client) -} +pub async fn connect_nostr() -> Result<Client> { + let keys = Keys::generate(); + let relays_env = var("RELAYS").map_err(|e| anyhow::anyhow!("RELAYS not set: {}", e))?; + let relays = relays_env + .split(',') + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + .collect::<Vec<_>>(); + if relays.is_empty() { + return Err(anyhow::anyhow!("RELAYS is empty")); + } + let client = Client::new(keys); + for r in relays { + client.add_relay(r).await?; + } + client.connect().await; + Ok(client) +}Additional: consider adding a connect_nostr_with_keys(keys: Keys) variant to bind the known identity instead of generating fresh keys.
src/util/messaging.rs (2)
193-201: POW/SECRET parsing: minor cleanupUsing '0'.to_string() is unusual. Prefer string literals and consistent error handling.
- let pow: u8 = var("POW") - .unwrap_or('0'.to_string()) + let pow: u8 = var("POW") + .unwrap_or_else(|_| "0".to_string()) .parse() .map_err(|e| anyhow::anyhow!("Failed to parse POW: {}", e))?; - let private = var("SECRET") - .unwrap_or("false".to_string()) + let private = var("SECRET") + .unwrap_or_else(|_| "false".to_string()) .parse::<bool>() .map_err(|e| anyhow::anyhow!("Failed to parse SECRET: {}", e))?;
69-110: wait_for_dm subscribes only to GiftWrapIf any flows use PrivateDirectMessage for responses, this subscription will miss them. If this function is only used for gift-wrap replies, ignore; otherwise consider subscribing to both GiftWrap and PrivateDirectMessage.
- let subscription = Filter::new() - .pubkey(trade_keys.public_key()) - .kind(nostr_sdk::Kind::GiftWrap) - .limit(0); + let subscription = Filter::new() + .pubkey(trade_keys.public_key()) + .kinds([nostr_sdk::Kind::GiftWrap, nostr_sdk::Kind::PrivateDirectMessage]) + .limit(0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/parser/dms.rs(2 hunks)src/util.rs(0 hunks)src/util/events.rs(1 hunks)src/util/messaging.rs(1 hunks)src/util/misc.rs(1 hunks)src/util/mod.rs(1 hunks)src/util/net.rs(1 hunks)src/util/storage.rs(1 hunks)src/util/types.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/util.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-13T10:31:42.313Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm.rs:24-29
Timestamp: 2025-09-13T10:31:42.313Z
Learning: In mostro-cli's get_dm.rs, the message filtering is intentional: non-admin users should only receive DirectMessage from users, while messages from Mostro are only fetched when the admin flag is true. Don't include GiftWrap messages for non-admin users.
Applied to files:
src/util/messaging.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/events.rs
🧬 Code graph analysis (5)
src/util/net.rs (1)
src/util/messaging.rs (1)
var(197-199)
src/util/mod.rs (5)
src/util/events.rs (3)
create_filter(69-102)fetch_events_list(105-195)get_direct_messages_from_trade_keys(13-45)src/util/messaging.rs (5)
print_dm_events(238-261)send_admin_gift_wrap_dm(14-21)send_dm(184-236)send_gift_wrap_dm(23-30)wait_for_dm(69-110)src/util/misc.rs (2)
get_mcli_path(11-19)uppercase_first(3-9)src/util/net.rs (1)
connect_nostr(5-16)src/util/storage.rs (3)
admin_send_dm(46-58)run_simple_order_msg(38-44)save_order(11-36)
src/util/messaging.rs (2)
src/parser/dms.rs (2)
print_commands_results(19-225)parse_dm_events(227-317)src/cli.rs (1)
std(389-391)
src/util/storage.rs (3)
src/cli/send_msg.rs (1)
execute_send_msg(10-93)src/db.rs (1)
get(195-207)src/util/messaging.rs (2)
None(51-51)send_dm(184-236)
src/util/events.rs (4)
src/parser/disputes.rs (1)
parse_dispute_events(13-43)src/parser/dms.rs (1)
parse_dm_events(227-317)src/parser/orders.rs (1)
parse_orders_events(15-74)src/db.rs (1)
get_trade_keys(238-253)
⏰ 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 (11)
src/util/misc.rs (1)
3-9: LGTM: uppercase_firstConcise and correct for Unicode scalar first character uppercasing.
src/util/types.rs (1)
19-24: Visibility of MessageTypepub(super) is appropriate for use within util::* (e.g., messaging). If you later need it outside util, switch to pub(crate). For now: looks good.
Ensure all usages import it from crate::util::types (not from siblings directly), which is required for pub(super) visibility.
src/util/storage.rs (2)
38-44: LGTM: run_simple_order_msg wrapperStraightforward delegation to execute_send_msg.
46-58: admin_send_dm: verify key usageYou pass identity via ctx.context_keys and trade via ctx.trade_keys. Confirm this aligns with your intended signed vs. private gift-wrap behavior in send_dm.
src/util/mod.rs (1)
1-18: LGTM: module split and re-exportsClean module boundary and stable re-export surface. Matches the PR goal.
src/util/messaging.rs (4)
157-167: Signature origin in signed gift wraps: confirm intentIn signed mode you include sig = Message::sign(payload, trade_keys), while the wrapper is signed with identity_keys. If the receiver is expected to verify the message with identity_keys, consider signing the payload with identity_keys as well. If the design requires trade_keys to author the inner rumor and identity_keys to wrap only, this is fine—just confirm.
Also applies to: 175-181
238-261: LGTM: print_dm_events flowMatches parser logic and validates request_id before dispatching.
10-13: Import path for parse_dm_events is correct parse_dm_events is re-exported in src/parser/mod.rs; no import change required.
1-8: DM filtering check belongs in events.rs/CLI, not messaging.rs
src/util/messaging.rs only constructs and sends DM/GiftWrap events; ListKind filtering and fetch_events_list are implemented in src/util/events.rs and CLI (e.g., get_dm.rs).Likely an incorrect or invalid review comment.
src/parser/dms.rs (1)
168-171: Confirm no remaining Action::Rate references
Search returned no hits; manually verify that the legacyRatevariant has been fully removed and no dead branches remain.src/util/events.rs (1)
33-39:sincefilter regressesWe compute
since_time(a Unix timestamp) but then compareevent.created_atagainst the rawsinceargument (minutes). For typical inputs (30, 60, …) this comparison always fails, so we never drop old events and the CLI ignores the requested window. Use the timestamp you just calculated (or mirror theparse_dm_eventslogic) so the filtering actually respectssince.⛔ Skipped due to learnings
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; }`.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.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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/util/storage.rs (1)
49-61: Verify field name:context_keysvsidentity_keys.Line 52 passes
&ctx.context_keystosend_dm, but the relevant code insrc/cli/send_msg.rs(line 70) uses&ctx.identity_keysfor the same parameter. This inconsistency could indicate:
- A typo where
context_keysshould beidentity_keys- An intentional difference for admin operations
- A field that was renamed but not updated consistently
Please verify this is correct.
Optional refactors:
- Line 49: The
anyhow::prefix is redundant sinceResultis imported fromanyhowon line 1.- Lines 59-60: The function can be simplified by directly returning the result instead of awaiting, unwrapping with
?, and returningOk(()).Apply this diff:
-pub async fn admin_send_dm(ctx: &Context, msg: String) -> anyhow::Result<()> { +pub async fn admin_send_dm(ctx: &Context, msg: String) -> Result<()> { super::messaging::send_dm( &ctx.client, Some(&ctx.context_keys), &ctx.trade_keys, &ctx.mostro_pubkey, msg, None, false, ) - .await?; - Ok(()) + .await }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util/storage.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/util/storage.rs (3)
src/cli/send_msg.rs (1)
execute_send_msg(10-93)src/db.rs (1)
get(195-207)src/util/messaging.rs (2)
None(51-51)send_dm(184-236)
⏰ 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/util/storage.rs (2)
18-20: Previous critical issues resolved.The unsafe cast and error swallowing flagged in the previous review have been properly addressed:
- Lines 18-19:
request_idnow uses checkedtry_fromconversion with a descriptive error message- Line 20:
Order::newerrors are now correctly propagated with?
41-47: LGTM!Clean delegation to
execute_send_msgwithNonefor the text parameter.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
src/util/misc.rs (1)
3-9: Consider Unicode edge cases and add documentation.The function correctly handles ASCII characters, but
to_uppercase()can produce multiple characters for some Unicode characters (e.g., German 'ß' becomes 'SS'). While the current implementation handles this correctly, consider documenting this behavior.Additionally, add doc comments for this public function to explain its purpose and any edge cases.
Example documentation:
+/// Capitalizes the first character of a string. +/// +/// Returns an empty string if the input is empty. +/// Note: For some Unicode characters, the uppercase form may be multiple characters. pub fn uppercase_first(s: &str) -> String { let mut c = s.chars(); match c.next() { None => String::new(), Some(f) => f.to_uppercase().collect::<String>() + c.as_str(), } }src/util/storage.rs (1)
41-47: Consider inlining or adding documentation.This wrapper function only delegates to
execute_send_msgwithNonefor the text parameter. If this is a common pattern, consider adding a doc comment explaining when to use this versus callingexecute_send_msgdirectly, or inline it at call sites if it's only used once or twice.src/cli/rate_user.rs (1)
51-67: Verify fake_request_id approach for rate operations.The code uses a
fake_request_idgenerated from a UUID becauseAction::RateReceivedresponses may not include arequest_id. This aligns with the special handling insrc/util/messaging.rs:254-256whereRateReceivedmessages are processed without matching request IDs.However, confirm this is the intended flow: should rate operations be fire-and-forget, or should they wait for confirmation? If waiting is intentional, consider adding a comment explaining why a fake ID is used.
Consider adding a clarifying comment:
// Wait for incoming DM let recv_event = wait_for_dm(ctx, Some(&trade_keys), sent_message).await?; // Parse the incoming DM - // use a fake request id + // RateReceived responses don't include request_id, so we use a placeholder let fake_request_id = Uuid::new_v4().as_u128() as u64; print_dm_events(recv_event, fake_request_id, ctx, Some(&trade_keys)).await?;src/util/messaging.rs (2)
238-268: Handle edge case: multiple matching messages.The function checks only
messages.first()(line 246) and returns an error if no messages are found (line 265). However, if multiple messages match, only the first is processed, and subsequent messages are silently ignored. Consider logging a warning ifmessages.len() > 1to aid debugging scenarios where duplicate or unexpected responses occur.let messages = parse_dm_events(recv_event, trade_keys, None).await; + if messages.len() > 1 { + log::warn!("Received {} messages, processing only the first", messages.len()); + } if let Some((message, _, _)) = messages.first() {
254-256: Document special handling for RateReceived.The special case for
Action::RateReceivedwithout arequest_id(lines 254-256) is crucial for understanding the fake_request_id pattern used insrc/cli/rate_user.rs:66. Consider adding a comment explaining whyRateReceivedresponses don't require matching request IDs.Some(id) => { if request_id == id { print_commands_results(message, ctx).await?; } } + // RateReceived responses don't include request_id; process them anyway None if message.action == Action::RateReceived => { print_commands_results(message, ctx).await?; }src/util/events.rs (2)
13-19: Consider renaming for clarity.The name
create_fake_timestampis misleading—it creates a real timestamp with a 2-day lookback. A name likecreate_default_lookback_timestamporcreate_two_day_lookback_timestampwould better convey its purpose.
127-146: Document or unifysincehandling for DirectMessages filters
Increate_filter(src/util/events.rs), the DirectMessagesUser/Admin arm ignores thesinceargument and uses a fixed 2880-minute window (create_fake_timestamp), while the caller’ssinceis only applied later inparse_dm_events. Add a doc comment or updatecreate_filterto accept and apply the user-providedsincefor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/cli/get_dm.rs(2 hunks)src/cli/get_dm_user.rs(3 hunks)src/cli/rate_user.rs(2 hunks)src/parser/dms.rs(3 hunks)src/util/events.rs(1 hunks)src/util/messaging.rs(1 hunks)src/util/misc.rs(1 hunks)src/util/mod.rs(1 hunks)src/util/storage.rs(1 hunks)src/util/types.rs(1 hunks)tests/parser_dms.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/util/mod.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:
tests/parser_dms.rs
📚 Learning: 2025-09-13T10:31:42.313Z
Learnt from: arkanoider
PR: MostroP2P/mostro-cli#135
File: src/cli/get_dm.rs:24-29
Timestamp: 2025-09-13T10:31:42.313Z
Learning: In mostro-cli's get_dm.rs, the message filtering is intentional: non-admin users should only receive DirectMessage from users, while messages from Mostro are only fetched when the admin flag is true. Don't include GiftWrap messages for non-admin users.
Applied to files:
src/util/events.rssrc/util/messaging.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/cli/get_dm_user.rs
🧬 Code graph analysis (5)
src/util/events.rs (4)
src/parser/disputes.rs (1)
parse_dispute_events(13-43)src/parser/dms.rs (1)
parse_dm_events(227-317)src/parser/orders.rs (1)
parse_orders_events(15-74)src/db.rs (1)
get_trade_keys(238-253)
src/util/storage.rs (3)
src/cli/send_msg.rs (1)
execute_send_msg(10-93)src/db.rs (1)
get(195-207)src/util/messaging.rs (2)
None(51-51)send_dm(184-236)
src/util/messaging.rs (1)
src/parser/dms.rs (2)
print_commands_results(19-225)parse_dm_events(227-317)
src/cli/get_dm_user.rs (1)
src/util/events.rs (1)
fetch_events_list(71-157)
src/cli/rate_user.rs (1)
src/util/messaging.rs (4)
print_dm_events(238-268)send_dm(184-236)wait_for_dm(69-110)None(51-51)
⏰ 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 (15)
src/util/storage.rs (2)
11-39: LGTM! Past critical issue properly addressed.The u64→i64 conversion now uses
try_fromwith clear error handling, andOrder::newerrors are correctly propagated. The user update logic appropriately logs but doesn't fail the entire operation if user save fails, which is acceptable for this workflow.
49-61: Use ofcontext_keysis correct for admin operations. All admin DM calls usecontext_keys; no changes needed.src/cli/get_dm.rs (1)
30-35: LGTM! Consistent with DM data shape refactor.The update to handle
(Message, u64, PublicKey)triples aligns with the broader PR changes that add PublicKey context to direct messages. The extraction pattern fromEvent::MessageTupleis consistent with similar updates insrc/cli/get_dm_user.rs.tests/parser_dms.rs (1)
16-18: LGTM! Test updated to match new signature.The test correctly reflects the updated
print_direct_messagessignature that now accepts(Message, u64, PublicKey)triples.src/cli/get_dm_user.rs (2)
36-58: LGTM! Consistent refactor to use fetch_events_list.The replacement of direct DB calls with
fetch_events_listimproves consistency and aligns with the modularization effort. The early returns for empty trade keys and messages improve code clarity. The extraction ofMessageTuplevariants is consistent withsrc/cli/get_dm.rs.Based on learnings, the
sinceparameter is handled appropriately byfetch_events_list, which performs manual filtering after fetching events.
67-83: LGTM! Proper handling of new triple data shape.The iteration correctly destructures the
(message, created_at, sender_pubkey)triple and usessender_pubkey.to_hex()for display. This aligns with the updated DM event structure.src/util/types.rs (1)
1-25: LGTM! Well-designed foundational types.The type definitions are clean and appropriate for the modularization:
MessageTupleusesBoxto optimize memory layout for the large tupleMessageTypehas appropriate visibility (pub(super)) as an internal helper- All types derive
CloneandDebugfor usability- The
ListKindvariants clearly categorize different event fetching scenariossrc/parser/dms.rs (3)
219-222: LGTM! Good refactor to combine duplicate handlers.Combining
HoldInvoicePaymentSettledandReleasedactions into a single pattern reduces code duplication since they have identical behavior.
319-322: LGTM! Signature updated consistently.The
print_direct_messagessignature now accepts&[(Message, u64, PublicKey)], consistent with the broader DM data shape refactor. The function body correctly handles the new triple format.
168-170: ConfirmAction::RateReceivedexists in theActionenum
The code replacesAction::RatewithAction::RateReceived; ensure themostro_coredependency defines this variant to avoid mismatches.src/util/messaging.rs (4)
14-67: LGTM! Clean gift-wrap DM helpers with proper factoring.The public
send_admin_gift_wrap_dmandsend_gift_wrap_dmfunctions provide clear interfaces, delegating to a shared internal implementation. The POW parsing with fallback to 0 on error (lines 39-42) is appropriate, and the logging at lines 60-63 aids debugging.
89-105: Good timeout handling for DM waiting.The
tokio::time::timeoutwrapper (lines 89-105) ensures the operation doesn't hang indefinitely. The timeout duration referencesFETCH_EVENTS_TIMEOUTfrom the events module, maintaining consistency across the codebase.
128-182: LGTM! Robust event creation helpers.The internal helper functions properly handle:
- Encryption/encoding in
create_private_dm_event(lines 128-143)- Conditional signing in
create_gift_wrap_event(lines 145-182)- Identity key validation before use (lines 158-159, 176-177)
- Proper error propagation with context
69-110: Subscription.limit(0)is correct – settinglimit(0)suppresses past events and, withWaitForEventsAfterEOSE(1), delivers only the first new event.src/util/events.rs (1)
29-29: Verify if 50-event limit is sufficient.The hardcoded
limit(50)might miss events if more than 50 orders or disputes are created within the 7-day window. Consider whether this limit should be configurable, removed, or increased based on expected event volume.
| ListKind::PrivateDirectMessagesUser => { | ||
| let since = if let Some(mins) = since { | ||
| chrono::Utc::now() | ||
| .checked_sub_signed(chrono::Duration::minutes(*mins)) | ||
| .unwrap() | ||
| .timestamp() | ||
| } else { | ||
| chrono::Utc::now() | ||
| .checked_sub_signed(chrono::Duration::minutes(30)) | ||
| .unwrap() | ||
| .timestamp() | ||
| } as u64; | ||
| Ok(Filter::new() | ||
| .kind(nostr_sdk::Kind::PrivateDirectMessage) | ||
| .pubkey(pubkey) | ||
| .since(Timestamp::from(since))) | ||
| } |
There was a problem hiding this comment.
Replace unwrap() with proper error handling.
Lines 54 and 60 use unwrap() on checked_sub_signed results, which will panic if the arithmetic operation fails (e.g., due to overflow/underflow). Since this function returns Result, use ok_or_else or a similar pattern to propagate the error gracefully, as done in create_fake_timestamp.
Apply this diff to handle errors safely:
ListKind::PrivateDirectMessagesUser => {
let since = if let Some(mins) = since {
chrono::Utc::now()
.checked_sub_signed(chrono::Duration::minutes(*mins))
- .unwrap()
+ .ok_or_else(|| anyhow::anyhow!("Failed to compute since time"))?
.timestamp()
} else {
chrono::Utc::now()
.checked_sub_signed(chrono::Duration::minutes(30))
- .unwrap()
+ .ok_or_else(|| anyhow::anyhow!("Failed to compute default since time"))?
.timestamp()
} as u64;
Ok(Filter::new()
.kind(nostr_sdk::Kind::PrivateDirectMessage)
.pubkey(pubkey)
.since(Timestamp::from(since)))
}📝 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.
| ListKind::PrivateDirectMessagesUser => { | |
| let since = if let Some(mins) = since { | |
| chrono::Utc::now() | |
| .checked_sub_signed(chrono::Duration::minutes(*mins)) | |
| .unwrap() | |
| .timestamp() | |
| } else { | |
| chrono::Utc::now() | |
| .checked_sub_signed(chrono::Duration::minutes(30)) | |
| .unwrap() | |
| .timestamp() | |
| } as u64; | |
| Ok(Filter::new() | |
| .kind(nostr_sdk::Kind::PrivateDirectMessage) | |
| .pubkey(pubkey) | |
| .since(Timestamp::from(since))) | |
| } | |
| ListKind::PrivateDirectMessagesUser => { | |
| let since = if let Some(mins) = since { | |
| chrono::Utc::now() | |
| .checked_sub_signed(chrono::Duration::minutes(*mins)) | |
| .ok_or_else(|| anyhow::anyhow!("Failed to compute since time"))? | |
| .timestamp() | |
| } else { | |
| chrono::Utc::now() | |
| .checked_sub_signed(chrono::Duration::minutes(30)) | |
| .ok_or_else(|| anyhow::anyhow!("Failed to compute default since time"))? | |
| .timestamp() | |
| } as u64; | |
| Ok(Filter::new() | |
| .kind(nostr_sdk::Kind::PrivateDirectMessage) | |
| .pubkey(pubkey) | |
| .since(Timestamp::from(since))) | |
| } |
🤖 Prompt for AI Agents
In src/util/events.rs around lines 50 to 66, the two calls to checked_sub_signed
currently end with unwrap(), which can panic; replace those unwraps with safe
error propagation similar to create_fake_timestamp: convert the surrounding
function to return Result if it does not already, call
checked_sub_signed(...).ok_or_else(|| YourError::TimestampArithmetic)? (or map
to the crate's existing error type) and propagate with ? so the function returns
an Err instead of panicking; ensure the created timestamp is then converted to
u64 after successful unwrapping.
| ListKind::PrivateDirectMessagesUser => { | ||
| let mut direct_messages: Vec<(Message, u64, PublicKey)> = Vec::new(); | ||
| for index in 1..=ctx.trade_index { | ||
| let trade_key = User::get_trade_keys(&ctx.pool, index).await?; | ||
| let filter = create_filter( | ||
| ListKind::PrivateDirectMessagesUser, | ||
| trade_key.public_key(), | ||
| None, | ||
| )?; | ||
| let fetched_user_messages = ctx | ||
| .client | ||
| .fetch_events(filter, FETCH_EVENTS_TIMEOUT) | ||
| .await?; | ||
| let direct_messages_for_trade_key = | ||
| parse_dm_events(fetched_user_messages, &trade_key, since).await; | ||
| // Extend the direct messages | ||
| direct_messages.extend(direct_messages_for_trade_key); | ||
| } | ||
| Ok(direct_messages | ||
| .into_iter() | ||
| .map(|t| Event::MessageTuple(Box::new(t))) | ||
| .collect()) | ||
| } |
There was a problem hiding this comment.
since parameter ignored when creating filter.
Line 111 passes None as the since parameter to create_filter, which defaults to a 30-minute lookback. However, fetch_events_list receives a since parameter from the caller, and you should pass it through:
let filter = create_filter(
ListKind::PrivateDirectMessagesUser,
trade_key.public_key(),
since, // ← pass the parameter through
)?;While parse_dm_events performs manual filtering based on since after fetching, using a tighter Nostr filter window from the start can reduce unnecessary network traffic and processing overhead.
🤖 Prompt for AI Agents
In src/util/events.rs around lines 104 to 126, the create_filter call inside the
PrivateDirectMessagesUser branch ignores the passed-in since parameter (it uses
None), causing unnecessary wide fetches; change the create_filter invocation to
pass the function's since parameter instead of None so the Nostr filter window
is tightened, thereby reducing network/processing cost, and keep the subsequent
parse_dm_events filtering intact.
Last pr before bumping new version:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor