src/app/order.rs (2)

248-249: Avoid vacuous test: assert the structural condition

let _ = result.is_ok() || result.is_err(); does nothing. Keep structural intent but assert it to prevent silent passes.

Apply:

-        // Structural check: ensure call does not panic
-        let _ = result.is_ok() || result.is_err();
+        // Structural check: ensure call returns a Result without panicking
+        assert!(result.is_ok() || result.is_err());

---

271-272: Make structural test explicit

Prefer asserting the condition to avoid a no-op.

-        // Structural check: ensure call returns a Result without panicking
-        let _ = result2.is_ok() || result2.is_err();
+        // Structural check: ensure call returns a Result without panicking
+        assert!(result2.is_ok() || result2.is_err());

src/app.rs (4)

491-505: No-op tests: prefer minimal assertions

These are pure no-ops. To keep structural scope but avoid vacuous passes, assert a benign invariant (e.g., that the function returns or branch executes).

• Replace comment-only “No-op” lines with an assert!(true) or a simple property check tied to the code path.

---

599-605: Structural match: assert handled arms to avoid empty branches

Empty branches are fine for structure, but add a trivial assertion to avoid completely vacuous test code.

• Inside handled arms, add assert!(true); or let _: () = (); to indicate intentional no-op.

---

671-674: Prefer an assertion in the expected branch

Replace the “No-op” with assert!(true); so the branch is exercised explicitly; keep unreachable!() for the else as-is.

---

691-696: Use matches! for structural checks

Instead of an empty if let, assert the variant to keep the structural test meaningful.

-                    // Test that we got a message of some kind
-                    if let Message::Order(_) = message {}
+                    // Test that we got an Order message
+                    assert!(matches!(message, Message::Order(_)));

src/scheduler.rs (3)

241-308: Optional: include trade_index in notifications (if available)

Both maker/taker notifications pass trade_index: None. If you can derive the maker’s/taker’s current trade index from updated_order, consider setting it to keep sequencing consistent.

• Verify whether downstream validation uses trade_index on cancel/republish notifications. If so, populate it from updated_order.trade_index_{buyer|seller} matching the recipient role.

---

371-392: Don’t ignore DB reset result before republish; handle Ok(false)/Err

Ignoring update_order_to_initial_state can leave inconsistent state. Handle failure and short-circuit.

-                                    let _ = update_order_to_initial_state(
-                                        &pool,
-                                        order.id,
-                                        order.amount,
-                                        order.fee,
-                                    )
-                                    .await;
+                                    match update_order_to_initial_state(
+                                        &pool,
+                                        order.id,
+                                        order.amount,
+                                        order.fee,
+                                    )
+                                    .await {
+                                        Ok(true) => {}
+                                        Ok(false) => {
+                                            tracing::warn!(
+                                                "No rows updated resetting order {} to initial state",
+                                                order.id
+                                            );
+                                            continue;
+                                        }
+                                        Err(e) => {
+                                            tracing::warn!(
+                                                "Failed to reset order {} to initial state: {}",
+                                                order.id, e
+                                            );
+                                            continue;
+                                        }
+                                    }

---

415-423: Replace println! with tracing

println! in async jobs bypasses your logging config.

-                            println!("Edited order: {:?}", edited_order);
+                            tracing::debug!("Edited order: {:?}", edited_order);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.

src/app/cancel.rs (1)

178-182: Use the returned order to ensure state consistency.

The edit_pubkeys_order function returns a refreshed Order object after updating the database, but this result is discarded. While the subsequent operations (update_order_to_initial_state, update_order_event) may work with the stale order reference in this specific flow, it's a safer practice to use the canonical post-update state.

According to the enriched summary, the scheduler now relies on the edited order object for its cancellation flow. Adopting the same pattern here ensures consistency across cancel paths and avoids potential issues if future code expects the order pubkey fields to reflect the nulled state.

Apply this diff:

     // clean pubkeys from db
-    let _ = edit_pubkeys_order(pool, order)
+    *order = edit_pubkeys_order(pool, order)
         .await
         .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;

This ensures that the in-memory order reflects the database state after pubkey nullification, maintaining consistency with the scheduler's approach described in the PR context.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/app/order.rs (1)

219-231: Consider aligning test names with actual scope.

Tests like test_order_action_with_valid_order, test_order_action_range_order_validation, and test_order_action_zero_amount_with_premium have names suggesting they validate specific business logic, but they don't include order payloads (line 176), so they only test the early-return path and message construction without panics. While this aligns with the structural testing approach, the names might mislead future maintainers.

Consider either:

1. Renaming to reflect structural scope (e.g., test_order_action_message_construction_no_panic)
2. Adding comments explaining the limited scope
3. Leaving as-is if the naming convention is understood across the project

Based on learnings.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.026Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.