Reviewing files that changed from the base of the PR and between d5df4c1 and 5fb8de6.

• Cargo.lock is excluded by !**/*.lock

• Cargo.toml (3 hunks)
• migrations/20230928145530_disputes.sql (1 hunks)
• src/app.rs (5 hunks)
• src/app/add_invoice.rs (4 hunks)
• src/app/admin_add_solver.rs (1 hunks)
• src/app/admin_cancel.rs (4 hunks)
• src/app/admin_settle.rs (4 hunks)
• src/app/admin_take_dispute.rs (5 hunks)
• src/app/dispute.rs (6 hunks)
• src/app/fiat_sent.rs (2 hunks)
• src/app/order.rs (1 hunks)
• src/app/rate_user.rs (0 hunks)
• src/app/release.rs (3 hunks)
• src/app/take_buy.rs (0 hunks)
• src/app/take_sell.rs (0 hunks)
• src/bitcoin_price.rs (2 hunks)
• src/cli.rs (2 hunks)
• src/cli/settings.rs (8 hunks)
• src/db.rs (33 hunks)
• src/flow.rs (2 hunks)
• src/lightning/mod.rs (0 hunks)
• src/main.rs (2 hunks)
• src/messages.rs (1 hunks)
• src/nip33.rs (5 hunks)
• src/scheduler.rs (6 hunks)
• src/util.rs (12 hunks)

• src/lightning/mod.rs
• src/app/take_buy.rs
• src/app/take_sell.rs
• src/app/rate_user.rs

• GitHub Check: tests

Cargo.toml (3)

19-19: Dependency version update for nostr-sdk looks good.

The version upgrade from 0.38.0 to 0.40.0 appears appropriate as part of the broader refactoring to use custom error types instead of anyhow.

---

40-40: Verify compatibility with this significant mostro-core version upgrade.

The mostro-core version has been updated from 0.6.28 to 0.6.38, which is a substantial jump of 10 minor versions.

Please ensure all tests pass with this updated dependency version. This upgrade likely introduces new functionality and possibly API changes that should be verified.

---

51-51: LGTM - Dev dependency formatting change.

This appears to be a minor formatting change in the dev dependencies section.

src/app/fiat_sent.rs (2)

56-59: Improved explicitness in Peer initialization.

Direct struct initialization makes the code more explicit about which fields are being set, improving readability and maintainability compared to using the Peer::new() constructor.

---

72-75: Consistent Peer initialization approach.

This change follows the same pattern as the earlier peer initialization, maintaining consistency in the codebase.

src/main.rs (3)

17-17: Improved import structure with crate-relative path.

Using crate::db::find_held_invoices makes the import hierarchy clearer and aligns with Rust best practices.

---

19-19: Consistent import pattern.

Using crate::lightning::LndConnector maintains consistency with the import style in the rest of the file.

---

93-93: Simplified client subscription call.

Changed from passing vec![subscription] to directly passing subscription, which is a cleaner approach if the API now supports accepting a single filter directly.

src/messages.rs (2)

1-1: Appropriate error handling import.

Replacing the generic anyhow::Result with the domain-specific MostroError aligns with the broader refactoring effort to use more explicit error types throughout the codebase.

---

7-7: Improved error type specificity.

Changing the return type from Result<String> to Result<String, MostroError> makes the error handling more explicit, following Rust best practices of clearly communicating possible error conditions.

src/app/admin_add_solver.rs (1)

38-39: Security check update aligns with codebase standardization

The change from checking event.rumor.pubkey to event.sender for authentication is consistent with the broader refactoring across admin modules. This establishes event.sender as the authoritative source for public key verification in admin actions.

migrations/20230928145530_disputes.sql (1)

5-5:

Details

❓ Verification inconclusive

Migration adds required column but lacks default value for existing records

The addition of the non-nullable order_previous_status column will enhance dispute tracking by storing the previous order status. However, this migration might fail on databases with existing records since it doesn't specify a default value for existing rows.

Consider validating this migration against a database with existing records or modifying it to include a default value:

-  order_previous_status varchar(10) not null,
+  order_previous_status varchar(10) not null DEFAULT 'unknown',

---

🏁 Script executed:

#!/bin/bash
# Check if there are existing disputes records that would be affected
echo "Checking for existing disputes records"
sqlite3 mostro.db "SELECT COUNT(*) FROM disputes;"

Length of output: 237

---

Ensure non-nullable column won’t break on existing data

In migrations/20230928145530_disputes.sql at line 5, you’ve added:

order_previous_status varchar(10) NOT NULL,

If the disputes table already contains rows, this migration will fail because there’s no value for existing records. Please verify and update the migration by either:

• Adding a default value:

  - order_previous_status varchar(10) NOT NULL,
  + order_previous_status varchar(10) NOT NULL DEFAULT 'unknown',

• Or backfilling before enforcing NOT NULL, for example:

  ALTER TABLE disputes ADD COLUMN order_previous_status varchar(10);
  UPDATE disputes SET order_previous_status = 'unknown' WHERE order_previous_status IS NULL;
  ALTER TABLE disputes ALTER COLUMN order_previous_status SET NOT NULL;

Request you to confirm whether there are existing dispute records in your environments and adjust the migration accordingly.

src/flow.rs (2)

1-1: Import updated to support new reputation notification functionality

The import statement has been updated to include the new notify_taker_reputation function that will be used later in the file.

---

108-112: Enhanced user experience with reputation notifications

Adding reputation notifications for takers provides valuable transparency about counterparty reliability, which helps users make more informed decisions. This implementation correctly calls the new utility function after sending messages to both parties.

src/app/order.rs (1)

50-89: Excellent documentation improvement

The added documentation for order_action thoroughly explains the function's purpose, behavior, parameters, error conditions, and includes a helpful usage example. This significantly improves code maintainability and developer onboarding.

src/cli.rs (3)

5-5: Import change aligns with error handling refactoring

The import of MostroError from mostro_core::error replaces the previously used anyhow::Result, supporting the codebase-wide shift to custom error types.

---

32-32: Function return signature updated for consistent error handling

The return type has been changed from anyhow::Result<PathBuf> to Result<PathBuf, MostroError>, making the error type explicit and consistent with the rest of the codebase.

---

35-39: Updated error propagation with explicit Ok wrapping

The function now wraps the result of init_default_dir with Ok(...) and uses the ? operator to propagate errors properly, matching the new return type signature.

src/app/admin_settle.rs (4)

34-34: Fixed solver verification to use correct event sender field

Changed from using event.rumor.pubkey to event.sender for solver verification, ensuring consistent access to the sender's public key throughout the codebase.

---

49-61: Improved control flow for cooperative cancellation

The logic for handling cooperative cancellation has been restructured to enqueue a message and return early if the order status is CooperativelyCanceled. This improves readability and makes the control flow more explicit.

---

86-87: Updated tag creation method

Changed from Tags::new to Tags::from_list for creating Tags, consistent with tag construction changes elsewhere in the codebase.

---

107-107: Changed event passing to use reference

Now passing a reference to the event (&event) rather than moving it, which is more efficient and aligns with similar changes across other modules.

src/scheduler.rs (5)

12-13: Added custom error imports to replace anyhow

Imported MostroError and ServiceError from mostro_core::error to support the codebase-wide shift from general anyhow error handling to custom error types.

---

127-127: Changed event sending to use references

Now passing references to events (&event) in client.send_event calls rather than moving them, which is more efficient and consistent with the pattern used elsewhere in the codebase.

Also applies to: 157-157, 214-214

---

235-235: Updated function return type for consistent error handling

Changed the return type of job_cancel_orders from anyhow::Result<()> to Result<(), MostroError>, aligning with the broader project-wide removal of anyhow in favor of MostroError.

---

238-253: Improved error handling with specific error types

Updated error handling in the database connection and key retrieval code to use specific MostroError variants with contextual information, rather than generic anyhow errors.

---

402-406: Enhanced logging with tracing instead of println

Replaced println! with tracing::info! for logging expired orders, which is a better practice for structured logging in a production application.

src/app/release.rs (3)

5-5: Added imports for Nostr event handling

Imported new_event and order_to_tags from the nip33 module to support the refactored create_order_event function.

---

117-117: Changed event sending to use reference

Now passing a reference to the event (&event) in the client.send_event call rather than moving it, which aligns with similar changes across the codebase.

---

406-428: Enhanced order event creation with privacy awareness

Significantly improved the create_order_event function to respect privacy settings:

1. Now checks if the order is a full privacy order for both buyer and seller
2. For non-private orders, fetches user reputation data and passes it to tag generation
3. For private orders, omits reputation data in tags
4. Adds validation to ensure tags are generated before creating an event
5. Returns a specific error if tag creation fails

This approach properly respects user privacy settings while still providing reputation information when appropriate.

src/app/admin_cancel.rs (2)

48-60: Early-return branch never persists any state change

When the order is already CooperativelyCanceled you enqueue CooperativeCancelAccepted and immediately return, but no DB or event update is performed.
Is that intentional? If the goal is merely to acknowledge the request it’s fine, otherwise you may want to call update_order_event (or at least persist CooperativeCancelAccepted) before returning.

---

136-139: Inconsistent use of event.sender vs event.rumor.pubkey

Above, the solver (and DM recipients inside enqueue_order_msg) are identified with event.sender, yet for the admin DM you fall back to event.rumor.pubkey.

Please double-check that this is the intended recipient. If the new convention is “always trust sender”, the line below should be updated:

-send_dm(event.rumor.pubkey, sender_keys, message.clone(), None)
+send_dm(event.sender,        sender_keys, message.clone(), None)

Mixing the two may DM the wrong key in multi-proxy scenarios.

src/app/add_invoice.rs (1)

27-35: Potentially wrong notification recipient

enqueue_order_msg sends InvoiceUpdated to order.get_buyer_pubkey(), yet the buyer is the one who just updated the invoice. Shouldn’t the seller be informed instead?

Please verify the business rule; if notification should go to the counter-party, switch to order.get_seller_pubkey().

-        order.get_buyer_pubkey().map_err(MostroInternalErr)?,
+        order.get_seller_pubkey().map_err(MostroInternalErr)?,

src/app.rs (2)

234-236: Admin-take dispute call correctly forwards my_keys now – nice catch
Passing my_keys keeps the handler signature consistent with the recent refactor, preventing compile-time breakage.

---

298-306: Good move: deserialize directly into Message to avoid double parsing
The switch to (Message, Option<Signature>) removes the previous Value → Message detour and saves one allocation / traversal.

src/app/dispute.rs (1)

109-145: Queue operations ignore failures – bubble them up or log
enqueue_order_msg(...).await; returns () today, but if it ever evolves to Result, silent failures would break UX. Even now, the await may panic in case of channel closure. Consider:

if let Err(e) = enqueue_order_msg(...).await {
    tracing::error!("Unable to enqueue dispute notification: {e}");
}

or change the helper signature to propagate errors.

src/nip33.rs (1)

94-107: Boolean logic may skip events for valid states

create_status_tags sets create_event using is_sell_order().is_ok() / is_buy_order().is_ok().
Those calls return true even when the order is not of the expected kind but the check merely succeeded.
Use order.is_sell_order()? (or compare order.kind) to avoid false negatives.

Reviewing files that changed from the base of the PR and between 5fb8de6 and 8588983.

• src/app/add_invoice.rs (4 hunks)
• src/app/admin_take_dispute.rs (5 hunks)
• src/db.rs (33 hunks)
• src/nip33.rs (5 hunks)

• GitHub Check: tests

src/app/add_invoice.rs (6)

16-36: Function name change clarifies purpose

The function has been renamed from check_order_status to pay_new_invoice, which better reflects its actual purpose. The implementation also consistently uses the MostroError type for better error handling.

---

48-48: Improved control flow with separate status check

Extracting the order status into a variable before processing makes the code more readable and maintainable.

---

60-69: Better status handling with explicit match

The match statement provides clearer handling for different order statuses, making the control flow easier to understand.

---

71-73: Added reputation notification improves user experience

Notifying the taker about the counterparty's reputation is a good enhancement that helps users make informed decisions.

---

81-90: Improved error handling for order event updates

This change properly handles errors from update_order_event by using a match statement with explicit error handling. The early return on error prevents proceeding with inconsistent order data.

This implementation addresses a previous review comment about ensuring DB consistency when update_order_event fails - now the operation is properly aborted on error.

---

92-111: Use of updated order data in messaging

Using the updated order data from active_order for subsequent messaging ensures consistency throughout the operation.

src/app/admin_take_dispute.rs (7)

20-108: Good extraction of dispute info preparation logic

The new helper function prepare_solver_info_message encapsulates the complex logic of constructing a SolverDisputeInfo object. It properly handles privacy modes, retrieves master pubkeys, and conditionally fetches user presence data.

The conditional logic for determining which party is the initiator and counterpart is well-structured, and the proper error handling throughout the function ensures robustness.

---

149-169: Improved dispute and order retrieval with better error handling

Using the utility function get_dispute and retrieving the order directly by ID simplifies the code and provides consistent error handling.

---

173-174: Updated dispute solver attribution

Now using event.sender instead of rumor.pubkey for setting the solver's public key, which correctly attributes the dispute to the event sender.

---

185-186: Use of dedicated helper for solver info message

Using the extracted helper function prepare_solver_info_message makes the code more modular and readable.

---

206-215: Improved message structure with solver peer info

Using Payload::Peer with the solver's public key provides a cleaner interface for communicating solver information to users.

---

217-238: Use of passed mostro_keys parameter

Using the passed mostro_keys parameter instead of fetching keys internally reduces duplication and improves consistency.

---

277-278: Proper reference passing for event sending

Using &event instead of passing by value ensures efficient event passing to the send_event method.

src/nip33.rs (7)

35-35: Consistent tag list building

Using Tags::from_list instead of Tags::new maintains consistency in tag construction throughout the codebase.

---

50-68: Fixed rating tag creation with proper JSON formatting

The updated function now correctly handles missing reputation data by returning an empty JSON object "{}" instead of a sentinel string. It also properly handles the case where the reputation timestamp is zero.

This change addresses a previous review comment about returning valid JSON when reputation data is missing, which prevents downstream JSON parsing errors.

---

93-107: Well-structured status tag mapping function

The new helper function create_status_tags encapsulates the logic for mapping order statuses to NIP69-compatible statuses and determining whether to create event tags. This improves code organization and reusability.

---

115-118: Improved function signature with Result and Optional return

Changing the return type to Result<Option<Tags>, MostroError> properly handles both errors and the case when no tags should be created.

---

119-129: Proper handling of Lightning network data

Using LN_STATUS.get() to retrieve the current Lightning network status ensures accurate network information in the tags.

---

179-191: Conditional reputation tag insertion

Conditionally inserting the reputation tag at a specific index only when data is available improves the handling of optional data.

---

203-203: Consistent tag list construction

Using Tags::from_list for info tags maintains consistency with the tag creation pattern used elsewhere in the file.

src/db.rs (5)

23-37: Improved database file creation with OpenOptions

The code now uses OpenOptions with create_new(true) instead of std::fs::File::create_new, making it compatible with stable Rust while maintaining the same functionality.

This change addresses a previous review comment about using stable Rust APIs instead of nightly-only features.

---

55-57: Consistent error handling pattern

The consistent use of MostroInternalErr(ServiceError::DbAccessError(e.to_string())) for database errors improves error specificity and consistency.

---

135-146: Standardized error handling in database queries

All database query functions now consistently return Result<_, MostroError> with appropriate error mapping, which aligns with the broader codebase changes to remove anyhow in favor of custom error types.

---

570-572: Added cross-field consistency check for ratings

The added check ensures that min_rating <= last_rating <= max_rating, which preserves important data integrity constraints.

This change addresses a previous review comment about ensuring cross-field consistency in rating data, preventing contradictory data such as having min_rating > last_rating.

---

597-601: Improved logging for solver assignment

Using tracing::info! instead of debug prints provides better visibility into solver assignments for troubleshooting and monitoring.