Dispute readability #390
Dispute readability #390
Conversation
WalkthroughThe pull request introduces several modifications across multiple files to enhance action handling and logging within the application. Key changes include the addition of new helper functions for logging and processing message actions, as well as updates to existing functions to streamline the handling of parameters, particularly the Changes
Possibly related PRs
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (9)
src/app/fiat_sent.rs (2)
Line range hint
13-91: Consider breaking down this function for better maintainability.The function handles multiple responsibilities including validation, state updates, and notifications. Consider refactoring into smaller, focused functions:
+ async fn validate_order(order: &Order, sender: &XOnlyPublicKey) -> Result<()> { + if order.status != Status::Active.to_string() { + return Err(Error::msg("Invalid order status")); + } + if Some(sender.to_string()) != order.buyer_pubkey { + return Err(Error::msg("Unauthorized sender")); + } + Ok(()) + } + async fn notify_participants( + order: &Order, + request_id: Option<String>, + buyer: &XOnlyPublicKey, + seller: &PublicKey, + ) -> Result<()> { + // Notify seller + send_new_order_msg( + None, + Some(order.id), + Action::FiatSentOk, + Some(Content::Peer(Peer::new(buyer.to_string()))), + seller, + ).await; + + // Notify buyer + send_new_order_msg( + request_id, + Some(order.id), + Action::FiatSentOk, + Some(Content::Peer(Peer::new(seller.to_string()))), + buyer, + ).await; + + Ok(()) + } pub async fn fiat_sent_action( msg: Message, event: &UnwrappedGift, my_keys: &Keys, pool: &Pool<Sqlite>, ) -> Result<()> { let request_id = msg.get_inner_message_kind().request_id; let order_id = msg.get_inner_message_kind().id .ok_or_else(|| Error::msg("No order id"))?; let order = Order::by_id(pool, order_id).await? .ok_or_else(|| Error::msg(format!("Order Id {order_id} not found!")))?; validate_order(&order, &event.sender).await?; if let Ok(order_updated) = update_order_event(my_keys, Status::FiatSent, &order).await { let _ = order_updated.update(pool).await; } let seller_pubkey = order.seller_pubkey .as_ref() .ok_or_else(|| Error::msg(format!("Seller pubkey not found for order {}!", order.id))) .and_then(|pk| Ok(PublicKey::from_str(pk)?))?; notify_participants(&order, request_id, &event.sender, &seller_pubkey).await?; Ok(()) }
Line range hint
13-91: Improve error handling consistency.The error handling could be more consistent and informative:
- Use custom error types instead of generic error messages
- Add context to errors using
.context()or.with_context()- Consider logging errors before converting them to Results
#[derive(thiserror::Error, Debug)] pub enum FiatSentError { #[error("Order {0} not found")] OrderNotFound(i64), #[error("Invalid order status: expected Active, got {0}")] InvalidStatus(String), #[error("Unauthorized: sender {sender} is not the buyer {buyer}")] Unauthorized { sender: String, buyer: String }, #[error("Seller public key not found for order {0}")] SellerNotFound(i64), }src/app/admin_take_dispute.rs (2)
Line range hint
129-134: Consider broader refactoring for improved readability.While the
request_idchanges maintain consistency, this function could benefit from further refactoring to align with the PR's readability objectives:
- Extract message creation logic into a separate helper function
- Split dispute status update and notification logic
Example helper function:
async fn create_dispute_notification( order_id: i64, solver_pubkey: Peer, ) -> Result<(Message, Message)> { let buyer_msg = Message::new_order( None, Some(order_id), Action::AdminTookDispute, Some(Content::Peer(solver_pubkey.clone())), ); let seller_msg = Message::new_order( None, Some(order_id), Action::AdminTookDispute, Some(Content::Peer(solver_pubkey)), ); Ok((buyer_msg, seller_msg)) }
Line range hint
38-184: Consider decomposing the function for better maintainability.The
admin_take_dispute_actionfunction handles multiple responsibilities and could be broken down into smaller, focused functions:
- Dispute validation and retrieval
- Status update logic
- Notification handling
- Event publishing
This would improve:
- Testability: Each component can be tested in isolation
- Maintainability: Changes can be made to specific components
- Readability: Each function would have a clear, single purpose
Example structure:
async fn validate_dispute_request(pool: &Pool<Sqlite>, msg: &Message, event: &UnwrappedGift) -> Result<(Dispute, Order)> async fn update_dispute_status(pool: &Pool<Sqlite>, dispute: &mut Dispute, solver: &PublicKey) -> Result<()> async fn notify_participants(order: &Order, solver: &PublicKey, sender_keys: Keys) -> Result<()> async fn publish_dispute_event(dispute_id: i64) -> Result<()> async fn admin_take_dispute_action(msg: Message, event: &UnwrappedGift, pool: &Pool<Sqlite>) -> Result<()> { let (mut dispute, order) = validate_dispute_request(pool, &msg, event).await?; update_dispute_status(pool, &mut dispute, &event.sender).await?; notify_participants(&order, &event.sender, get_keys()?).await?; publish_dispute_event(dispute.id).await?; Ok(()) }src/app.rs (3)
39-42: LGTM! Consider adding more detailed documentation.The helper function effectively centralizes warning message logging. Consider adding documentation about the logging format and when this helper should be used.
/// Helper function to log warning messages for action errors +/// +/// # Arguments +/// * `action` - The action that caused the error +/// * `e` - The error that occurred during action processing +/// +/// # Example +/// ``` +/// warning_msg(&Action::NewOrder, anyhow::anyhow!("Invalid order data")); +/// ``` fn warning_msg(action: &Action, e: anyhow::Error) {
44-84: LGTM! Consider enhancing error context.The function effectively centralizes action handling with a clean organization. Consider wrapping the Result with context for better error tracing:
async fn handle_message_action( // ... parameters ... ) -> Result<()> { match action { - Action::NewOrder => order_action(msg, event, my_keys, pool).await, + Action::NewOrder => order_action(msg, event, my_keys, pool) + .await + .context("failed to process new order action"), // Apply similar pattern to other actions } }
Line range hint
111-153: LGTM! Consider enhancing error handling for event verification.The error handling and logging improvements look good. Consider adding more context to the event verification error:
if let Kind::GiftWrap = event.kind { // We validates if the event is correctly signed - if event.verify().is_err() { - tracing::warn!("Error in event verification") + if let Err(e) = event.verify() { + tracing::warn!("Failed to verify event {}: {}", event.id, e); };src/app/dispute.rs (2)
136-136: Correct the log message for clarity.The log message "Dispute yet opened for this order id: {order_id}" can be rephrased for better clarity. Consider changing it to "Dispute already opened for this order id: {order_id}".
Apply this diff to update the log message:
- tracing::error!("Dispute yet opened for this order id: {order_id}"); + tracing::error!("Dispute already opened for this order id: {order_id}");
19-66: Simplify error handling inpublish_dispute_eventfunction.The nested
matchstatements for error handling inpublish_dispute_eventcan be simplified using the?operator, which will make the code cleaner and more readable.Apply this diff to refactor the error handling:
- // Get nostr client and publish the event - match get_nostr_client() { - Ok(client) => match client.send_event(event).await { - Ok(_) => { - tracing::info!( - "Successfully published dispute event for dispute ID: {}", - dispute.id - ); - Ok(()) - } - Err(e) => { - tracing::error!("Failed to send dispute event: {}", e); - Err(Error::msg("Failed to send dispute event")) - } - }, - Err(e) => { - tracing::error!("Failed to get Nostr client: {}", e); - Err(Error::msg("Failed to get Nostr client")) - } - } + // Get nostr client and publish the event + let client = get_nostr_client()?; + client.send_event(event).await?; + tracing::info!( + "Successfully published dispute event for dispute ID: {}", + dispute.id + ); + Ok(())This refactor reduces nesting and leverages the
?operator for error propagation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app.rs(3 hunks)src/app/admin_take_dispute.rs(1 hunks)src/app/cancel.rs(2 hunks)src/app/dispute.rs(7 hunks)src/app/fiat_sent.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/cancel.rs
🔇 Additional comments (3)
src/app/fiat_sent.rs (1)
69-69: Verify the impact of removing request_id tracking for seller notifications.
While this change aligns with similar modifications in other files, removing the request_id for seller notifications could impact request-response correlation and debugging capabilities.
Let's check if there are any dependencies on this request_id in seller-side handling:
✅ Verification successful
Removing request_id for seller notifications is safe and consistent with the codebase design
After analyzing the codebase, the change to pass None instead of request_id for seller notifications is safe because:
- The
request_idis primarily used for correlating buyer-initiated actions - In the release flow and other similar flows, seller notifications already use
Noneforrequest_id - No error handling or logging functionality depends on seller-side
request_idtracking - The pattern of using
Nonefor counterparty notifications is consistently applied throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any code that might expect or handle seller-side request_ids
rg -l "request_id.*seller"
rg -l "seller.*request_id"
# Check if there are any logging or error handling that might use this request_id
ast-grep --pattern 'send_new_order_msg($$$request_id$$$, $$$)'
Length of output: 160
Script:
#!/bin/bash
# Let's examine the release.rs file to understand the seller-request_id relationship
cat src/app/release.rs
# Also check for any other notifications or message sending patterns
rg "send.*seller" -A 3
rg "notify.*seller" -A 3
# Look for any error handling or logging related to seller notifications
rg "error.*seller" -A 3
Length of output: 19396
src/app/admin_take_dispute.rs (1)
122-127: Verify and document the intentional removal of request_id.
The change from using request_id to None might affect message tracking. While this aligns with similar changes in other files, it would be helpful to:
- Document why
request_idis intentionally set toNonefor buyer notifications - Verify that this doesn't break message correlation in the system
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough context about the Message::new_order usage patterns across the codebase.
Setting request_id to None is consistent with the codebase patterns
The change from using request_id to None is actually correct and consistent with the codebase's message handling patterns. Here's why:
- In
admin_take_dispute.rs, both messages (msg_to_buyerandmsg_to_seller) useNonefor request_id since they are notifications about an admin action - In contrast,
admin_cancel.rsandadmin_settle.rsuse therequest_idbecause they are direct responses to admin requests - The utility function in
util.rsacceptsrequest_idas a parameter, allowing both patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other message creation patterns to verify consistency
rg "Message::new_order" -A 3 -B 3
Length of output: 2230
src/app/dispute.rs (1)
230-232: Consider handling the error returned by publish_dispute_event.
In the dispute_action function, if publish_dispute_event fails, the error is logged but not propagated. Depending on the importance of this event, consider whether the function should return an error to indicate the failure.
Ensure that it's acceptable for the function to continue execution even if publishing the dispute event fails. If the failure should stop further processing, modify the code to return the error.
Ok rabbit! Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Ok Rabbit Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/app/dispute.rs (4)
1-4: Enhance module documentation with workflow details.While the documentation provides a good overview, consider adding:
- The dispute lifecycle stages
- Valid state transitions
- Security token usage
- Required permissions/roles
23-74: Consider enhancing event publishing robustness.The function is well-structured, but consider these improvements:
- Extract tag names ("s", "y", "z") as module-level constants
- Make error messages more specific by including context
- Add retry logic for transient network failures
+ const TAG_STATUS: &str = "s"; + const TAG_APP_ID: &str = "y"; + const TAG_EVENT_TYPE: &str = "z"; + const APP_IDENTIFIER: &str = "mostrop2p"; async fn publish_dispute_event(dispute: &Dispute, my_keys: &Keys) -> Result<()> { let tags = Tags::new(vec![ Tag::custom( - TagKind::Custom(Cow::Borrowed("s")), + TagKind::Custom(Cow::Borrowed(TAG_STATUS)), vec![dispute.status.to_string()], ), Tag::custom( - TagKind::Custom(Cow::Borrowed("y")), - vec!["mostrop2p".to_string()], + TagKind::Custom(Cow::Borrowed(TAG_APP_ID)), + vec![APP_IDENTIFIER.to_string()], ),
210-220: Extract token generation to a separate function.The token generation logic could be encapsulated in a helper function for better maintainability and testability.
+ fn generate_security_tokens() -> (Option<i32>, Option<i32>) { + let mut rng = rand::thread_rng(); + ( + Some(rng.gen_range(100..=999)), + Some(rng.gen_range(100..=999)) + ) + } // Create new dispute record and generate security tokens let mut dispute = Dispute::new(order_id); - let mut rng = rand::thread_rng(); - dispute.buyer_token = Some(rng.gen_range(100..=999)); - dispute.seller_token = Some(rng.gen_range(100..=999)); + (dispute.buyer_token, dispute.seller_token) = generate_security_tokens();
Line range hint
189-208: Simplify dispute status update logic.The current implementation using multiple boolean flags could be simplified.
- let mut update_seller_dispute = false; - let mut update_buyer_dispute = false; - if is_seller_dispute && !order.seller_dispute { - update_seller_dispute = true; - order.seller_dispute = update_seller_dispute; - } else if is_buyer_dispute && !order.buyer_dispute { - update_buyer_dispute = true; - order.buyer_dispute = update_buyer_dispute; - }; - order.status = Status::Dispute.to_string(); - - if !update_buyer_dispute && !update_seller_dispute { - return Ok(()); - } else { - order.update(pool).await?; - } + let should_update = match is_seller_dispute { + true if !order.seller_dispute => { + order.seller_dispute = true; + true + } + false if !order.buyer_dispute => { + order.buyer_dispute = true; + true + } + _ => false + }; + + if should_update { + order.status = Status::Dispute.to_string(); + order.update(pool).await?; + } else { + return Ok(()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/dispute.rs(7 hunks)
🔇 Additional comments (1)
src/app/dispute.rs (1)
76-90: LGTM! Well-structured helper function.
The function is well-documented, has proper error handling, and effectively encapsulates the counterpart determination logic.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…stro into dispute-readability
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/app/dispute.rs (1)
23-74: Enhance error messages with more context.The function is well-structured with good error handling, but error messages could be more specific.
Consider this improvement:
- Err(Error::msg("Failed to send dispute event")) + Err(Error::msg(format!("Failed to send dispute event for dispute ID {}: {}", dispute.id, e)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/dispute.rs(7 hunks)
🔇 Additional comments (4)
src/app/dispute.rs (4)
1-4: LGTM! Clear and informative module documentation.
The added documentation header clearly describes the module's purpose and responsibilities.
76-90: LGTM! Well-designed helper function.
The function effectively encapsulates counterparty determination logic with proper error handling and clear return values.
92-138: LGTM! Robust order validation implementation.
The function properly implements order validation with comprehensive error handling and appropriate status checks. Previous review suggestions about error handling have been incorporated well.
166-167: Revisit duplicate dispute handling.
The current implementation still returns Ok(()) for duplicate disputes, which was previously flagged as an issue. Consider implementing the previously suggested error handling approach.
| // Create new dispute record and generate security tokens | ||
| let mut dispute = Dispute::new(order_id); | ||
| // Generate tokens for the users to avoid fake resolver | ||
| let mut rng = rand::thread_rng(); | ||
| dispute.buyer_token = Some(rng.gen_range(100..=999)); | ||
| dispute.seller_token = Some(rng.gen_range(100..=999)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider increasing the token range for better security.
The current token range (100-999) provides only 900 possible values. Consider increasing this range to provide better security against brute force attempts.
- dispute.buyer_token = Some(rng.gen_range(100..=999));
- dispute.seller_token = Some(rng.gen_range(100..=999));
+ dispute.buyer_token = Some(rng.gen_range(100000..=999999));
+ dispute.seller_token = Some(rng.gen_range(100000..=999999));📝 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.
| // Create new dispute record and generate security tokens | |
| let mut dispute = Dispute::new(order_id); | |
| // Generate tokens for the users to avoid fake resolver | |
| let mut rng = rand::thread_rng(); | |
| dispute.buyer_token = Some(rng.gen_range(100..=999)); | |
| dispute.seller_token = Some(rng.gen_range(100..=999)); | |
| // Create new dispute record and generate security tokens | |
| let mut dispute = Dispute::new(order_id); | |
| let mut rng = rand::thread_rng(); | |
| dispute.buyer_token = Some(rng.gen_range(100000..=999999)); | |
| dispute.seller_token = Some(rng.gen_range(100000..=999999)); |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Hey buddy tests are failing |
|
Yep! Too many rabbit advices...will fix! |
|
let's see now... |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/app/dispute.rs (2)
27-74: Enhance error handling specificityWhile the error handling is comprehensive, the error messages could be more specific to help with debugging.
Consider this improvement:
- .map_err(|_| Error::msg("Failed to create dispute event"))?; + .map_err(|e| Error::msg(format!("Failed to create dispute event: {}", e)))?; match client.send_event(event).await { Ok(_) => { tracing::info!( "Successfully published dispute event for dispute ID: {}", dispute.id ); Ok(()) } Err(e) => { tracing::error!("Failed to send dispute event: {}", e); - Err(Error::msg("Failed to send dispute event")) + Err(Error::msg(format!("Failed to send dispute event: {}", e))) } }, Err(e) => { tracing::error!("Failed to get Nostr client: {}", e); - Err(Error::msg("Failed to get Nostr client")) + Err(Error::msg(format!("Failed to get Nostr client: {}", e))) }
92-138: Consider defining status constantsTo improve maintainability and reduce the risk of typos, consider extracting the valid statuses into constants.
Add these constants at the module level:
const VALID_DISPUTE_STATUSES: [Status; 2] = [Status::Active, Status::FiatSent];Then modify the status check:
- if !matches!(status, Status::Active | Status::FiatSent) { + if !VALID_DISPUTE_STATUSES.contains(&status) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/app.rs(4 hunks)src/app/dispute.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.rs
🔇 Additional comments (4)
src/app/dispute.rs (4)
1-4: Great addition of module documentation!
The added documentation clearly describes the module's purpose and responsibilities.
76-90: Well-structured helper function!
The function effectively encapsulates the counterparty determination logic with proper error handling.
Line range hint 140-267: Excellent refactoring of dispute handling!
The dispute_action function has been well-modularized with improved error handling and clear separation of concerns. The integration with the new helper functions makes the code more maintainable and easier to understand.
215-219:
Increase token range for better security
The current token range (100-999) provides only 900 possible values, which might be insufficient for security purposes.
Consider increasing the range:
- dispute.buyer_token = Some(rng.gen_range(100..=999));
- dispute.seller_token = Some(rng.gen_range(100..=999));
+ dispute.buyer_token = Some(rng.gen_range(100000..=999999));
+ dispute.seller_token = Some(rng.gen_range(100000..=999999));Likely invalid or redundant comment.
@grunch
playing a bit with some refactoring insipired a bit by
cursorAI, he did quite well inApp.rsimo and I tried to chunck big functions indispute.rsto start.Take a look with a grain of saly, just compiled not tested.
Summary by CodeRabbit