Line range hint 14-108: Consider potential race conditions in order processing.

The time window between status validation and order updates could allow concurrent order takes. Consider implementing optimistic locking or a transaction to ensure atomicity.

Line range hint 26-133: Consider refactoring for improved maintainability.

The admin_settle_action function handles multiple responsibilities:

1. Dispute validation
2. Order status management
3. Invoice settlement
4. Event notifications
5. Payment processing

Consider breaking this down into smaller, focused functions to improve maintainability and testability.

Consider extracting these responsibilities into separate functions:

async fn validate_dispute_solver(pool: &Pool<Sqlite>, sender: &PublicKey, order_id: u32) -> Result<()>
async fn handle_dispute_settlement(dispute: &mut Dispute, my_keys: &Keys) -> Result<Event>
async fn notify_participants(order: &Order, message: &str, sender: &PublicKey) -> Result<()>

This would make the main function more readable and easier to maintain:

pub async fn admin_settle_action(...) -> Result<()> {
    let order_id = msg.get_inner_message_kind().id.ok_or_else(|| Error::msg("No order id"))?;
    validate_dispute_solver(pool, &event.sender, order_id).await?;
    // ... rest of the logic
}

Line range hint 99-111: Enhance error handling for fiat amount validation.

The current implementation has several areas for improvement:

1. The error message doesn't provide context about valid ranges, which could confuse users.
2. The order state is mutated before all validations are complete.

Consider this approach:

    // Get amount request if user requested one for range order - fiat amount will be used below
    if let Some(am) = get_fiat_amount_requested(&order, &msg) {
+       // Validate the amount is within acceptable bounds before mutating order
+       if !is_valid_fiat_range(&order, am) {
+           send_new_order_msg(
+               Some(order.id),
+               Action::OutOfRangeFiatAmount,
+               Some(format!("Amount must be between {} and {}", order.min_amount, order.max_amount)),
+               &event.sender,
+           ).await;
+           return Ok(());
+       }
        order.fiat_amount = am;
    } else {
        send_new_order_msg(
            Some(order.id),
            Action::OutOfRangeFiatAmount,
            None,
            &event.sender,
        )
        .await;
        return Ok(());
    }

Line range hint 52-69: Enhance invoice validation error handling.

The current invoice validation could be more robust and informative:

1. Error messages don't specify which validation failed (amount, fee, or format)
2. No protection against potential integer overflow in amount validation

Consider this improvement:

    if let Some(payment_request) = msg.get_inner_message_kind().get_payment_request() {
        pr = {
+           // Validate basic invoice format first
+           if let Err(e) = validate_invoice_format(&payment_request) {
+               send_cant_do_msg(
+                   Some(order.id),
+                   Some("Invalid invoice format".to_string()),
+                   &event.sender,
+               ).await;
+               return Ok(());
+           }
+           // Then validate amounts
+           let total_expected = order.amount.checked_add(order.fee)
+               .ok_or_else(|| Error::msg("Amount overflow"))?;
            match is_valid_invoice(
                payment_request.clone(),
-               Some(order.amount as u64),
-               Some(order.fee as u64),
+               Some(total_expected as u64),
+               None,
            ).await {
                Ok(_) => Some(payment_request),
                Err(e) => {
                    send_cant_do_msg(
                        Some(order.id),
-                       Some(e.to_string()),
+                       Some(format!("Invoice amount mismatch: expected {} sats", total_expected)),
                        &event.sender,
                    ).await;
                    error!("{e}");
                    return Ok(());
                }
            }
        };
    }

Line range hint 71-98: Improve order status transition handling.

While the status validation works, it could be more informative and traceable:

1. Error messages don't specify the current status vs expected status
2. Status transitions aren't logged for audit purposes

Consider this enhancement:

    let order_status = match Status::from_str(&order.status) {
        Ok(s) => s,
        Err(e) => {
-           error!("Order Id {order_id} wrong status: {e:?}");
+           error!("Order Id {order_id} invalid status format '{order.status}': {e:?}");
            return Ok(());
        }
    };

    // Buyer can take Pending orders only
    match order_status {
-       Status::Pending => {}
+       Status::Pending => {
+           tracing::info!(
+               "Order {} status transition: {} -> WaitingBuyerInvoice",
+               order.id,
+               order_status
+           );
+       }
        _ => {
            send_new_order_msg(
                Some(order.id),
                Action::NotAllowedByStatus,
-               None,
+               Some(format!(
+                   "Order must be in Pending status (current: {})",
+                   order_status
+               )),
                &buyer_pubkey,
            )
            .await;
            return Ok(());
        }
    }

Line range hint 17-142: Consider breaking down the admin_cancel_action function.

While the function is well-structured with proper error handling, it's quite long and handles multiple responsibilities (order validation, dispute handling, Lightning operations, notifications). Consider breaking it down into smaller, focused functions for better maintainability.

Consider extracting these logical blocks into separate functions:

1. Order validation and status checks
2. Lightning fund return handling
3. Dispute status update
4. Notification dispatch

Example structure:

async fn validate_order_status(order: &Order, event: &UnwrappedGift) -> Result<()> {
    // Status validation logic
}

async fn handle_lightning_refund(ln_client: &mut LndConnector, order: &Order) -> Result<()> {
    // Lightning operations
}

async fn update_dispute_status(
    pool: &Pool<Sqlite>,
    my_keys: &Keys,
    order_id: i64,
) -> Result<()> {
    // Dispute handling
}

async fn send_notifications(
    order: &Order,
    event: &UnwrappedGift,
    message: String,
) -> Result<()> {
    // Notification logic
}

Line range hint 34-35: Consider further reducing global state

While the current changes improve error handling, consider taking this further by removing the static NOSTR_CLIENT entirely. Instead, you could:

1. Initialize the client in main
2. Pass it as a parameter to components that need it
3. Use dependency injection patterns

This would make the code more testable and reduce global state dependencies.

Line range hint 1-182: Consider refactoring for improved maintainability.

While the current changes are good, here are some suggestions to improve the overall code structure:

1. Consider extracting validation logic into separate functions:

fn validate_order_status(order: &Order) -> Result<Status> {
    Status::from_str(&order.status).map_err(|e| {
        error!("Order Id {} wrong status: {e:?}", order.id);
        Error::msg("Invalid order status")
    })
}

2. Consider splitting the function into smaller, focused functions:

• validate_order_and_permissions
• process_invoice
• handle_preimage_case

3. Make error handling more consistent by logging all error cases before returning.

Would you like me to provide a complete refactoring proposal?

44-59: Consider logging errors when Nostr client is unavailable.

While the error handling has improved by using get_nostr_client(), errors are currently silently ignored. Consider logging these errors to help with debugging and monitoring.

-    if let Ok(client) = get_nostr_client() {
-        let _ = client.send_event(relay_ev).await;
+    match get_nostr_client() {
+        Ok(client) => {
+            if let Err(e) = client.send_event(relay_ev).await {
+                error!("Failed to send relay event: {}", e);
+            }
+        }
+        Err(e) => error!("Failed to get Nostr client: {}", e),
+    }

84-87: Apply consistent error handling for Nostr client operations.

Similar to the previous function, consider improving error logging here as well.

-    if let Ok(client) = get_nostr_client() {
-        let _ = client.send_event(info_ev).await;
+    match get_nostr_client() {
+        Ok(client) => {
+            if let Err(e) = client.send_event(info_ev).await {
+                error!("Failed to send info event: {}", e);
+            }
+        }
+        Err(e) => error!("Failed to get Nostr client: {}", e),
+    }

Line range hint 7-141: Consider standardizing Nostr client error handling.

While the changes successfully remove direct NOSTR_CLIENT usage, there's an opportunity to standardize error handling across all Nostr client operations. Consider creating a helper function that encapsulates the common pattern of getting the client and sending events with proper error handling.

Example:

async fn send_nostr_event(event: Event) {
    match get_nostr_client() {
        Ok(client) => {
            if let Err(e) = client.send_event(event).await {
                error!("Failed to send Nostr event: {}", e);
            }
        }
        Err(e) => error!("Failed to get Nostr client: {}", e),
    }
}

This would promote consistency and reduce code duplication across the codebase.

249-252: Improve error handling in send_dm function

While the change to use get_nostr_client() is good, we're silently ignoring both client initialization and event sending errors. Consider logging these errors for better observability.

 if let Ok(client) = get_nostr_client() {
-    let _ = client.send_event(event).await;
+    if let Err(e) = client.send_event(event).await {
+        tracing::warn!("Failed to send DM: {}", e);
+    }
+} else {
+    tracing::warn!("Failed to get NOSTR client for sending DM");
 }

610-617: Add documentation for the Result type and improve error message

Consider adding documentation for the return type and providing a more descriptive error message.

 /// Getter function with error management for nostr Client
+/// 
+/// # Returns
+/// 
+/// Returns a Result containing:
+/// - Ok(&'static Client): A reference to the initialized NOSTR client
+/// - Err: When the client hasn't been initialized
 pub fn get_nostr_client() -> Result<&'static Client> {
     if let Some(client) = NOSTR_CLIENT.get() {
         Ok(client)
     } else {
-        Err(Error::msg("Client not initialized!"))
+        Err(Error::msg("NOSTR client has not been initialized. Ensure connect_nostr() is called first."))
     }
 }

619-626: Add documentation for the Option type

Consider adding documentation for the return type to improve API clarity.

 /// Getter function with error management for nostr relays
+/// 
+/// # Returns
+/// 
+/// Returns an Option containing:
+/// - Some(HashMap<Url, Relay>): The map of active NOSTR relays when client is initialized
+/// - None: When the client hasn't been initialized
 pub async fn get_nostr_relays() -> Option<HashMap<Url, Relay>> {
     if let Some(client) = NOSTR_CLIENT.get() {
         Some(client.relays().await)
     } else {
         None
     }
 }

Files that changed from the base of the PR and between 6fdcf74 and da3beec.

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

• src/app/add_invoice.rs (1 hunks)
• src/app/admin_cancel.rs (2 hunks)
• src/app/admin_settle.rs (2 hunks)
• src/app/admin_take_dispute.rs (2 hunks)
• src/app/dispute.rs (2 hunks)
• src/app/take_buy.rs (2 hunks)
• src/app/take_sell.rs (1 hunks)
• src/main.rs (3 hunks)
• src/scheduler.rs (5 hunks)
• src/util.rs (4 hunks)

Reviewing files that changed from the base of the PR and between da3beec and e33ee8e.

• src/app/admin_cancel.rs (2 hunks)
• src/app/admin_settle.rs (2 hunks)
• src/app/admin_take_dispute.rs (2 hunks)
• src/main.rs (3 hunks)

• src/app/admin_settle.rs
• src/app/admin_take_dispute.rs

Reviewing files that changed from the base of the PR and between e33ee8e and 92a371f.

• src/util.rs (5 hunks)

Reviewing files that changed from the base of the PR and between 92a371f and 29f6449.

• src/app/admin_add_solver.rs (1 hunks)
• src/app/admin_cancel.rs (5 hunks)
• src/app/admin_settle.rs (3 hunks)
• src/app/admin_take_dispute.rs (4 hunks)
• src/util.rs (7 hunks)

• src/app/admin_cancel.rs
• src/app/admin_settle.rs
• src/app/admin_take_dispute.rs