feat: handles cancel request for already cancelled order & removes redundant check #393
Conversation
WalkthroughThe changes in this pull request focus on enhancing the cancellation process within the application. The Changes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (1)
src/app/cancel.rs (1)
Line range hint
208-324: Consider refactoring common patterns in cancellation functionsThe
cancel_add_invoiceandcancel_pay_hold_invoicefunctions share similar patterns for:
- Handling hold invoices
- Checking pubkeys and permissions
- Updating order status
- Sending messages
- Logging
Consider extracting these common operations into helper functions to improve maintainability and reduce duplication.
Here's a suggested approach:
// Helper for handling hold invoices async fn handle_hold_invoice(ln_client: &mut LndConnector, order: &Order) -> Result<()> { if let Some(hash) = &order.hash { ln_client.cancel_hold_invoice(hash).await?; info!("Order Id {}: Funds returned to seller", &order.id); } Ok(()) } // Helper for checking permissions async fn check_cancellation_permission( order: &Order, user_pubkey: &str, expected_role_pubkey: &str, request_id: Option<u64>, event_sender: &PublicKey, ) -> Result<()> { if user_pubkey != expected_role_pubkey { send_cant_do_msg(request_id, Some(order.id), None, event_sender).await; return Err(Error::msg("Unauthorized cancellation attempt")); } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app/cancel.rs(2 hunks)
🔇 Additional comments (2)
src/app/cancel.rs (2)
43-56: LGTM: Well-implemented early return for cancelled orders
The new check properly handles already cancelled orders by checking all possible cancellation states and returning a clear error message to the user. This aligns well with the PR objective of improving the user experience.
91-91: Verify the safety of removing the redundant check
The condition has been simplified, but let's verify that removing the check doesn't introduce any edge cases.
✅ Verification successful
Based on the search results, I can now generate the final response:
The simplified condition check is safe and correctly implemented
The codebase analysis confirms that WaitingBuyerInvoice status is consistently used in the expected flow:
- It's set during sell order flow (src/flow.rs)
- It's properly checked in add_invoice handling (src/app/add_invoice.rs)
- It's monitored by the scheduler for timeouts (src/scheduler.rs)
- The simplified condition in cancel.rs aligns with these usages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other occurrences of WaitingBuyerInvoice status checks
# to ensure we're not missing any important conditions
# Look for WaitingBuyerInvoice status checks in the codebase
rg "WaitingBuyerInvoice" -B 2 -A 2
# Look for status transitions to/from WaitingBuyerInvoice
ast-grep --pattern 'Status::WaitingBuyerInvoice'
Length of output: 3356
| send_cant_do_msg( | ||
| request_id, | ||
| Some(order_id), | ||
| Some("Order already canceled".to_string()), |
There was a problem hiding this comment.
I think the idea is to send messages with just the action, without a text string inside, so that clients can interpret that action and show a message to users with the text they want and in the language they want.
There was a problem hiding this comment.
Oh, sorry, I somehow missed this.
Yes, the idea is to replace custom messages with static codes that clients can map to localized, human-readable messages. However, I don’t think we have those static codes implemented yet, so for now, I decided to stick with the legacy method of using hard-coded, human-readable error messages.
There was a problem hiding this comment.
Actually there are, there are no longer any actions with text strings in the code, all the ones that had them were replaced by one of these: https://github.com/MostroP2P/mostro-core/blob/c0cd7bbd263a303e7aab46588362839e836f0ac0/src/message.rs#L37C1-L84C2
Perhaps in this case just send the cant do msg, but if is needit something more descriptive, create it as a new action
But if don't do it for now, at least keep it pending to don't forget later, because since there are no more actions with text string we could forget that was added one recently and not change it later. My 2 cents
There was a problem hiding this comment.
Oh I see. The thing is, we can have many many different reasons for errors like CantDo. Expanding this set for every new specific error we want to convey to clients would result in an unbounded number of actions, making CantDo itself too vague in the process.
Instead I'd suggest keeping the action list concise and grouping all specific error messages into a new enum, which we could call ErrorMessage. What do you say @grunch?
There was a problem hiding this comment.
Sorry I missed this response, yes I think it makes sense @bilthon, if you want you want to do it please go first to mostro-core and then change it here
There was a problem hiding this comment.
Here the proposal for CantDoReason error handling
And here the merged PR
It is already working on mostro-core 0.6.15
When receiving a cancel request for an already cancelled order mostro just fails silently. This adds a check for that and makes mostro reply with a
cant-doaction response.There's also a small redundant check that was removed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation