Feat: issue #427 avoid multiple pending orders#444
Conversation
…ayment or waiting-buyer-invoice order pending
WalkthroughThe changes introduce new checks for pending orders in both the buy and sell actions of the application. In the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/take_sell.rs (1)
54-56: 🛠️ Refactor suggestionEnhance error message to provide more meaningful feedback.
The current error message
InvalidActionis too generic and doesn't help users understand why their action was rejected. Consider providing a more descriptive error message that explains the pending order restriction.
🧹 Nitpick comments (2)
src/app/take_sell.rs (1)
53-53: Fix incorrect comment.The comment says "Check if the seller has a pending order" but the function name is
buyer_has_pending_order. This is confusing and should be corrected.Apply this diff to fix the comment:
- // Check if the seller has a pending order + // Check if the buyer has a pending ordersrc/db.rs (1)
456-508: Reduce code duplication by extracting common logic.The
seller_has_pending_orderandbuyer_has_pending_orderfunctions share identical structure and error handling. Consider refactoring to reduce duplication.Apply this diff to extract common logic:
+/// Check if a user has a pending order in the database with status waiting-payment or waiting-buyer-invoice +async fn has_pending_order( + pool: &SqlitePool, + pubkey: String, + column_name: &str, +) -> anyhow::Result<bool, MostroError> { + // Validate public key format (32-bytes hex) + if !pubkey.chars().all(|c| c.is_ascii_hexdigit()) || pubkey.len() != 64 { + return Err(MostroCantDo(CantDoReason::InvalidPubkey)); + } + + let mut conn = pool + .acquire() + .await + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + let rows_affected = sqlx::query(&format!( + r#" + SELECT EXISTS (SELECT 1 FROM orders WHERE {} = ?1 AND (status = 'waiting-payment' OR status = 'waiting-buyer-invoice')) + "#, + column_name + )) + .bind(pubkey) + .map(|row: SqliteRow| row.get(0)) + .fetch_one(&mut conn) + .await.map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; + + Ok(rows_affected) +} + /// Check if the seller has a pending order in the database with status waiting-payment or waiting-buyer-invoice pub async fn seller_has_pending_order( pool: &SqlitePool, pubkey: String, ) -> anyhow::Result<bool, MostroError> { - // Validate public key format (32-bytes hex) - if !pubkey.chars().all(|c| c.is_ascii_hexdigit()) || pubkey.len() != 64 { - return Err(MostroCantDo(CantDoReason::InvalidPubkey)); - } - - let mut conn = pool - .acquire() - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - let rows_affected = sqlx::query( - r#" - SELECT EXISTS (SELECT 1 FROM orders WHERE master_seller_pubkey = ?1 AND (status = 'waiting-payment' OR status = 'waiting-buyer-invoice')) - "#, - ) - .bind(pubkey) - .map(|row: SqliteRow| row.get(0)) - .fetch_one(&mut conn) - .await.map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - - Ok(rows_affected) + has_pending_order(pool, pubkey, "master_seller_pubkey").await } /// Check if the buyer has a pending order in the database with status waiting-payment or waiting-buyer-invoice pub async fn buyer_has_pending_order( pool: &SqlitePool, pubkey: String, ) -> anyhow::Result<bool, MostroError> { - // Validate public key format (32-bytes hex) - if !pubkey.chars().all(|c| c.is_ascii_hexdigit()) || pubkey.len() != 64 { - return Err(MostroCantDo(CantDoReason::InvalidPubkey)); - } - - let mut conn = pool - .acquire() - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - let rows_affected = sqlx::query( - r#" - SELECT EXISTS (SELECT 1 FROM orders WHERE master_buyer_pubkey = ?1 AND (status = 'waiting-payment' OR status = 'waiting-buyer-invoice')) - "#, - ) - .bind(pubkey) - .map(|row: SqliteRow| row.get(0)) - .fetch_one(&mut conn) - .await.map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - - Ok(rows_affected) + has_pending_order(pool, pubkey, "master_buyer_pubkey").await }
There was a problem hiding this comment.
Hi @arkanoider Good job. However, I found 2 cases where it can be improved because it shouldn't work like this.
- A creates an order
- B takes A's order but doesn't respond
- A tries to take C's order and Mostro doesn't let him (A doesn't know that his order was taken because the taker doesn't continue)
This could be used by a malicious user to block other users from taking orders
How it should work is that A could take an order even if it is in status waiting-buyer-invoice or waiting-payment if it is the taker's turn
- A takes an order from B and does his part
- B doesn't respond and leaves A waiting
- A tries to take an order from C and Mostro doesn't let him
Mostro should allow A to take another order if he has already done his part
|
@Catrya need some more refinement, you're right. Have to check how to manage better all the possible cases. |
So for case 1 @Catrya , just for my mental recap: -- A creates a sell order This is the sequence for case 1? |
Yes, but Mostro should let A take another order |
I tested quickly and with a clean db with two sell orders from A and two from B and B was able to take a new order correctly after A took one. But I could be totally wrong @Catrya please do a round on your side again when you can. |
You're right, I just checked again and it's possible. |
maybe some edge cases depending on db situation, do your test meanwhile i will fix event ratings issue. |
|
Will try to do another round of testing on this tonite or tomorrow morning @Catrya . Let me know if you found wrong behaviour of this PR. |
Hi @arkanoider this case have to be fixed:
I tested again and the first case that I mentioned worked fine as I told you later. |
confirm i did not retest second case, will be on it tonight or tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/db.rs (1)
456-508: Consider refactoring to reduce code duplication.The
seller_has_pending_orderandbuyer_has_pending_orderfunctions are nearly identical, differing only in the column name and documentation. Consider extracting the common logic into a shared function.Here's a suggested refactor:
async fn has_pending_order( pool: &SqlitePool, pubkey: String, role_column: &str, ) -> anyhow::Result<bool, MostroError> { // Validate public key format (32-bytes hex) if !pubkey.chars().all(|c| c.is_ascii_hexdigit()) || pubkey.len() != 64 { return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } let mut conn = pool .acquire() .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; let query = format!( r#" SELECT EXISTS ( SELECT 1 FROM orders WHERE {} = ?1 AND (status = 'waiting-payment' OR status = 'waiting-buyer-invoice') ) "#, role_column ); let rows_affected = sqlx::query(&query) .bind(pubkey) .map(|row: SqliteRow| row.get(0)) .fetch_one(&mut conn) .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; Ok(rows_affected) } pub async fn seller_has_pending_order( pool: &SqlitePool, pubkey: String, ) -> anyhow::Result<bool, MostroError> { has_pending_order(pool, pubkey, "master_seller_pubkey").await } pub async fn buyer_has_pending_order( pool: &SqlitePool, pubkey: String, ) -> anyhow::Result<bool, MostroError> { has_pending_order(pool, pubkey, "master_buyer_pubkey").await }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/db.rs(2 hunks)
🔇 Additional comments (2)
src/db.rs (2)
3-7: LGTM! Error handling imports are well-organized.The new imports from
mostro_core::errorare appropriately added to support the enhanced error handling in the new functions.
456-508:Details
❓ Verification inconclusive
Consider edge cases in order status checks.
Based on the PR discussion, the current implementation might incorrectly block users who have completed their part of the transaction. For example, if User A creates an order and User B takes it but doesn't respond, User A would be blocked from taking new orders.
Consider adding additional checks to allow users to take new orders if they've completed their part of the transaction (e.g., seller has provided invoice, buyer has made payment).
Let's verify the current order status flow:
🏁 Script executed:
#!/bin/bash # Description: Check the order status flow in the codebase # Look for order status transitions to understand the complete flow rg -A 5 "status = " --type rustLength of output: 7937
Attention: Revisit Pending Order Status Conditions
The current implementation in
src/db.rsrelies solely on checking for orders with status'waiting-payment'(for sellers) and'waiting-buyer-invoice'(for buyers). However, the order status flow throughout the codebase (e.g., insrc/flow.rsandsrc/scheduler.rs) suggests that there are additional transitions that mark an order as active or otherwise progressing toward completion. This raises the risk that a user might be incorrectly blocked—especially in edge cases where part of the transaction has already been completed (for example, when the seller has provided the invoice or the buyer has made the payment), but the order still briefly holds one of these statuses.Recommendations:
- Seller Check: Reassess whether the condition
status = 'waiting-payment'fully represents a pending seller order. Consider incorporating a check for orders that have moved past the waiting stage.- Buyer Check: Similarly, confirm that matching on
status = 'waiting-buyer-invoice'is sufficient. Evaluate if there are states (or additional fields) that indicate the buyer has already taken action.- Overall Status Flow: Verify that order status transitions (as seen in
src/flow.rsand/src/scheduler.rs) align with the intended logic. Adjust these functions to allow new orders when an order has reached a completed or non-blocking state.It would be beneficial to review the full order lifecycle to ensure this pending order logic correctly differentiates between truly active pending orders and those that can be considered completed or stale.
66f8e05 to
9aed9b7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Hi @arkanoider I still find this error in my side |
|
tACK @grunch @arkanoider |
|
Conflicts are due to latest mostro-core in toml. Let me know if I can resolve and merge @grunch ... |
please solve those conflicts, we can't merge like this |
Done |
|
tACK again @arkanoider @grunch |
Implementation of #427 :
User should just have one waiting-payment or waiting-buyer-invoice order pending when taking orders ( sell or buy ), in case of other pending orders they receive a CantDo message ( maybe we can add a more meaningful one )
Summary by CodeRabbit
mostro-coredependency to version 0.6.28, potentially enhancing library functionality.