Code refactoring removing old actions #409
Conversation
|
Warning Rate limit exceeded@grunch has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a comprehensive update to the Mostro application, focusing on refactoring error handling and messaging across multiple files. The changes primarily involve replacing Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/app/cancel.rs (1)
60-63: Consider adding a log statement for clarityWhen invoking
send_cant_do_msgwithCantDoReason::IsNotYourOrder, adding a brief log statement prior to sending the message could help with debugging or usage metrics. For example:+ tracing::warn!("User {} attempted to cancel an order they do not own (order_id: {}).", user_pubkey, order.id); send_cant_do_msg( request_id, Some(order.id), Some(CantDoReason::IsNotYourOrder), &event.rumor.pubkey, ) .await;src/app/release.rs (5)
274-292: Consider making the 5-second delay configurableWhile the delay may be necessary for Nostr event propagation or coordination, making it configurable in a setting or passing it as a parameter can improve flexibility and facilitate testing:
- tokio::time::sleep(tokio::time::Duration::from_secs(5)).await; + let wait_duration = std::time::Duration::from_secs(5); // or configurable + tokio::time::sleep(wait_duration).await;
303-336: Return early after notifying invalid range amountsWhen
notify_invalid_amountis called (lines 330-331), the function continues and eventually returns(false, order.clone()). To avoid confusion, you may prefer an early return:Ordering::Less => { notify_invalid_amount(order, request_id).await; + return Ok((false, order.clone())); }
338-359: Reset the status for newly created orderWhen cloning an existing order, consider explicitly setting
new_order.statustoPending(or another default) to avoid inheriting a stale status:fn create_base_order(order: &Order) -> Order { let mut new_order = order.clone(); + new_order.status = Status::Pending.to_string(); new_order.amount = 0; ...
361-381: Use a shared DB pool rather than connecting anewThe function calls
db::connect()internally, potentially opening a new connection each time. Consider passing a&Pool<Sqlite>to reuse established connections and improve testability:- async fn update_order_for_equal(new_max: i64, new_order: &mut Order, my_keys: &Keys) -> Result<()> { - let pool = db::connect().await?; + async fn update_order_for_equal( + new_max: i64, + new_order: &mut Order, + my_keys: &Keys, + pool: &Pool<Sqlite> + ) -> Result<()> {
383-406: Reuse DB pool inupdate_order_for_greaterSimilar to
update_order_for_equal, passing in the existing pool can improve performance and code clarity:- async fn update_order_for_greater( - new_max: i64, - new_order: &mut Order, - my_keys: &Keys, - ) -> Result<()> { - let pool = db::connect().await?; + async fn update_order_for_greater( + new_max: i64, + new_order: &mut Order, + my_keys: &Keys, + pool: &Pool<Sqlite>, + ) -> Result<()> {src/app/order.rs (2)
61-68: Avoid mixing partial ID usageHere,
send_cant_do_msgusesNonefor the order ID but logs anInvalidAmount. Check if you still want to store the actualorder.idin the message to help the client or logs correlate the error with the correct order.
107-114: Check negative or zero invoice amounts earlierThis block ensures no negative amounts slip through. You may also consider verifying zero amounts strictly to avoid unexpected edge cases.
📜 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 (12)
Cargo.toml(1 hunks)src/app/add_invoice.rs(2 hunks)src/app/admin_cancel.rs(3 hunks)src/app/admin_settle.rs(3 hunks)src/app/admin_take_dispute.rs(2 hunks)src/app/cancel.rs(1 hunks)src/app/dispute.rs(1 hunks)src/app/fiat_sent.rs(1 hunks)src/app/order.rs(4 hunks)src/app/release.rs(4 hunks)src/app/take_buy.rs(3 hunks)src/app/take_sell.rs(3 hunks)
🔇 Additional comments (28)
src/app/cancel.rs (1)
Line range hint 104-111: Validate “NotAllowedByStatus” usage
The change to use CantDoReason::NotAllowedByStatus is consistent with the new messaging approach and clearly conveys the reason to the user. Consider verifying that all relevant statuses are handled elsewhere to ensure consistency.
src/app/release.rs (2)
2-2: No issues with the newly introduced imports
The added imports for db and enhanced message structures appear to be correctly referenced in the code.
Also applies to: 13-13
104-111: Consistent “NotAllowedByStatus” message usage
This addition aligns with the broader refactoring to use send_cant_do_msg. The code is straightforward and helps unify your error messaging strategy.
src/app/fiat_sent.rs (1)
36-43: Informative error message
Using CantDoReason::NotAllowedByStatus is consistent and clear. This helps to unify your error-handling approach and makes the code more maintainable.
src/app/take_buy.rs (3)
2-2: Import usage looks aligned with code changes
Imports for send_cant_do_msg and CantDoReason match the new error messaging approach. No issues found here.
Also applies to: 6-6
71-78: Concise status validation
Using CantDoReason::NotAllowedByStatus for an invalid order status scenario is consistent across the refactor. This keeps logic straightforward and standardized.
87-94: Out-of-range fiat amount check is properly handled
Returning early with a CantDoReason::OutOfRangeFiatAmount message ensures the user is quickly informed of the issue, maintaining a robust flow.
src/app/order.rs (3)
3-3: No issues with revised imports
All referenced functions and enums align with the updated messaging scheme. Good job removing unused references.
Also applies to: 5-5
31-38: Proper invoice validation messaging
If an invalid invoice is detected, sending CantDoReason::InvalidAmount is clear. Consider verifying that borderline cases (e.g., zero or near-zero amounts) are also caught upstream.
121-124: Clearer rejection for out-of-range Sats amounts
Providing CantDoReason::OutOfRangeSatsAmount standardizes your error response. Ensure your client UI also interprets this reason properly for user feedback.
src/app/admin_settle.rs (4)
5-5: Refined import logic is clear.
The addition of send_cant_do_msg in the import list aligns with the updated error-handling approach.
10-10: Inclusion of new CantDoReason enum.
Bringing CantDoReason into scope is consistent with the refactored messaging strategy across the codebase.
40-47: Appropriate error message for unassigned solver scenario.
Using CantDoReason::IsNotYourDispute clarifies why the action cannot proceed if the solver is not properly assigned. The process flow and error feedback appear correct.
82-89: Clearer handling for invalid order status.
Sending CantDoReason::NotAllowedByStatus is an effective way to explain why the transaction can’t be processed when the status is not set to Dispute. This aligns well with the new messaging design.
src/app/take_sell.rs (4)
3-3: Consolidated utility imports.
Pulling in send_cant_do_msg and others from util is consistent with the new approach for fail-fast messaging.
8-8: Importing CantDoReason for more descriptive error feedback.
This improves clarity in error handling by enumerating specific scenarios where the action cannot be completed.
105-112: Disallowing action by order status.
Emitting CantDoReason::NotAllowedByStatus provides a concise explanation to the user when the order is not Pending. This ensures coherent user feedback.
121-128: More specific out-of-range error.
CantDoReason::OutOfRangeFiatAmount clarifies the cause of the failure, improving user comprehension about fiat amount restrictions.
src/app/admin_cancel.rs (4)
7-7: Refined imports for updated messaging flow.
Including send_cant_do_msg closely mirrors the consistent approach used across the codebase to return descriptive failure reasons.
11-11: Bringing CantDoReason into scope for clarity.
This import ensures that explicit reasoning (e.g., IsNotYourDispute, NotAllowedByStatus) is communicated when actions are blocked.
38-45: Explicit dispute ownership check.
Using CantDoReason::IsNotYourDispute is an elegant way to handle attempts by users who are not assigned solvers.
80-87: Status-based cancellation restriction.
CantDoReason::NotAllowedByStatus ensures the system provides straightforward feedback if the solver tries to cancel an order not in dispute.
src/app/add_invoice.rs (2)
86-89: Handling invalid invoice amounts gracefully.
Replacing a vague “invalid invoice” action with CantDoReason::InvalidAmount makes the nature of the failure explicit to the user.
121-124: Status validation before adding invoice.
Sending CantDoReason::NotAllowedByStatus clearly communicates that the order status prevents adding an invoice, aligning with the improved error-handling convention.
src/app/admin_take_dispute.rs (2)
3-3: Refactored imports to replace old messaging function with send_cant_do_msg.
This aligns well with the PR’s objective of introducing reason-based error handling.
59-66: Correct application of send_cant_do_msg with CantDoReason::NotFound.
Replacing the old messaging approach with reason-based error messaging enhances clarity and consistency in dispute handling.
src/app/dispute.rs (1)
118-125: Appropriate switch to send_cant_do_msg with CantDoReason::NotAllowedByStatus.
The usage is consistent with the new reason-based error messaging and correctly passes the order’s ID and request ID for clarity.
Cargo.toml (1)
41-41: Version bump for mostro-core to 0.6.22.
Please verify that downstream dependencies remain compatible with this updated version.
Here is a script to check for references to mostro-core usage and review any potential compatibility concerns:
✅ Verification successful
Let me gather more information about the usage of mostro-core in the codebase.
Let me check one of the modified files to see if there are any breaking changes in the mostro-core usage.
Version bump for mostro-core appears compatible with codebase changes
The changes in the codebase show refactoring work that aligns with the mostro-core API. The modifications in take_sell.rs demonstrate proper usage of mostro-core types and no breaking changes:
- Still using the same types from mostro-core (
CantDoReason,Message,Order,Status) - Changes are refactoring-related, replacing
send_new_order_msgwithsend_cant_do_msgbut maintaining type compatibility - No compiler errors were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search the codebase for references to `mostro-core` to ensure no unresolved compatibility issues
rg -A 5 "mostro-core"
Length of output: 752
Script:
#!/bin/bash
# Search for direct usage of mostro-core types and functions
rg -l "use mostro_core::"
# Check if there are any git changes to source files that might indicate breaking changes
git show --name-status | grep -v "Cargo"
Length of output: 892
Script:
#!/bin/bash
# Check the diff of one of the modified files to see the nature of changes
git show src/app/take_sell.rs
# Also check if there are any compiler errors or warnings in the build log
git show | grep -A 5 "error\["
Length of output: 2014
Using unwrap() can cause panics if the public keys are malformed. Handle the error gracefully or log a warning Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/app/release.rs (4)
274-292: Consider using a configurable delay and add error handlingThe 5-second sleep before publishing the new event could be problematic:
- The delay duration should be configurable via settings
- Consider using a more robust retry mechanism instead of a fixed delay
- Add error handling for the event publishing
Consider this improvement:
- tokio::time::sleep(tokio::time::Duration::from_secs(5)).await; + let delay = Settings::get_event_publish_delay(); + tokio::time::sleep(tokio::time::Duration::from_secs(delay)).await; + // Add retry mechanism with exponential backoff if needed + match NOSTR_CLIENT.get()?.send_event(event).await { + Ok(_) => info!("New order event published successfully"), + Err(e) => error!("Failed to publish new order event: {}", e), + }
308-336: Add documentation for the range order logicThe function handles complex range order logic but lacks detailed documentation. Consider adding:
- Examples of valid/invalid scenarios
- Explanation of the relationship between max_amount, min_amount, and fiat_amount
- Documentation of the return tuple's meaning
Add this documentation:
/// Check if order is range type /// Add parent range id and update max amount /// publish a new replaceable kind nostr event with the status updated /// and update on local database the status and new event id +/// +/// # Arguments +/// * `order` - The original order to process +/// * `request_id` - Optional request ID for tracking +/// * `my_keys` - Keys for signing the new event +/// +/// # Returns +/// A tuple containing: +/// * `bool` - Whether a new range order was created +/// * `Order` - The new order if created, or the original order if not +/// +/// # Examples +/// ``` +/// // When max_amount is 1000 and min_amount is 500: +/// // If fiat_amount is 300, creates new order with max_amount = 700 +/// // If fiat_amount is 500, creates new order with exact amount +/// // If fiat_amount is 600, notifies invalid amount +/// ```
361-406: Reduce code duplication in order update functionsThe
update_order_for_equalandupdate_order_for_greaterfunctions share significant code. Consider extracting the common functionality.Create a helper function:
+ async fn publish_order_event(new_order: &mut Order, my_keys: &Keys) -> Result<()> { + let pool = db::connect().await?; + let tags = crate::nip33::order_to_tags(new_order, None); + let event = crate::nip33::new_event(my_keys, "", new_order.id.to_string(), tags)?; + new_order.event_id = event.id.to_string(); + new_order.clone().create(&pool).await?; + NOSTR_CLIENT + .get() + .ok_or_else(|| anyhow::Error::msg("NOSTR_CLIENT not initialized"))? + .send_event(event) + .await + .map_err(|err| anyhow::Error::msg(err.to_string()))?; + Ok(()) + }Then use it in both functions:
async fn update_order_for_equal(new_max: i64, new_order: &mut Order, my_keys: &Keys) -> Result<()> { new_order.fiat_amount = new_max; new_order.max_amount = None; new_order.min_amount = None; new_order.status = Status::Pending.to_string(); new_order.id = uuid::Uuid::new_v4(); - // ... remove duplicated event publishing code + publish_order_event(new_order, my_keys).await }
408-443: Improve error handling in notify_invalid_amountThe function has complex error handling for parsing public keys. Consider using the
?operator with a custom error type or moving the parsing logic to a helper function.Consider this improvement:
+ fn parse_pubkey(key: &str) -> Result<PublicKey> { + PublicKey::from_str(key).map_err(|e| anyhow::Error::msg(format!("Failed to parse pubkey: {}", e))) + } async fn notify_invalid_amount(order: &Order, request_id: Option<u64>) { if let (Some(buyer_pubkey), Some(seller_pubkey)) = (order.buyer_pubkey.as_ref(), order.seller_pubkey.as_ref()) { - let buyer_pubkey = match PublicKey::from_str(buyer_pubkey) { - Ok(pk) => pk, - Err(e) => { - error!("Failed to parse buyer pubkey: {:?}", e); - return; - } - }; - let seller_pubkey = match PublicKey::from_str(seller_pubkey) { - Ok(pk) => pk, - Err(e) => { - error!("Failed to parse seller pubkey: {:?}", e); - return; - } - }; + let buyer_pk = match parse_pubkey(buyer_pubkey) { + Ok(pk) => pk, + Err(e) => { + error!("{}", e); + return; + } + }; + let seller_pk = match parse_pubkey(seller_pubkey) { + Ok(pk) => pk, + Err(e) => { + error!("{}", e); + return; + } + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/release.rs(4 hunks)
🔇 Additional comments (1)
src/app/release.rs (1)
104-111: LGTM! Improved error messaging
The change from send_new_order_msg to send_cant_do_msg with CantDoReason::NotAllowedByStatus provides clearer feedback about why the action failed.
Summary by CodeRabbit
Release Notes
Dependency Update
mostro-corelibrary to version 0.6.22New Features
get_child_orderfunction for enhanced order processingBug Fixes
Refactoring
send_cant_do_msg