Merge pull request #1775 from get10101/fix/two-open-orders

Move past multiple orders in `Filling` state

mobile/lib/features/trade/async_order_change_notifier.dart

@@ -42,8 +42,10 @@ class AsyncOrderChangeNotifier extends ChangeNotifier implements Subscriber {
           TaskStatus status = TaskStatus.pending;
           switch (asyncOrder?.state) {
             case OrderState.open:
+            case OrderState.filling:
               status = TaskStatus.pending;
             case OrderState.failed:
+            case OrderState.rejected:
               status = TaskStatus.failed;
             case OrderState.filled:
               status = TaskStatus.success;

mobile/lib/features/trade/domain/order.dart

@@ -24,17 +24,23 @@ enum OrderReason {
 
 enum OrderState {
   open,
+  filling,
   filled,
-  failed;
+  failed,
+  rejected;
 
   static OrderState fromApi(bridge.OrderState orderState) {
     switch (orderState) {
       case bridge.OrderState.Open:
         return OrderState.open;
+      case bridge.OrderState.Filling:
+        return OrderState.filling;
       case bridge.OrderState.Filled:
         return OrderState.filled;
       case bridge.OrderState.Failed:
         return OrderState.failed;
+      case bridge.OrderState.Rejected:
+        return OrderState.rejected;
     }
   }
 }

mobile/lib/features/trade/order_list_item.dart

@@ -24,8 +24,13 @@ class OrderListItem extends StatelessWidget {
           Icons.pending,
           size: iconSize,
         ),
+      OrderState.filling => const Icon(
+          Icons.pending,
+          size: iconSize,
+        ),
       OrderState.filled => const Icon(Icons.check_circle, color: Colors.green, size: iconSize),
       OrderState.failed => const Icon(Icons.error, color: Colors.red, size: iconSize),
+      OrderState.rejected => const Icon(Icons.error, color: Colors.red, size: iconSize),
     };
 
     return Column(

mobile/lib/features/trade/submit_order_change_notifier.dart

@@ -71,11 +71,13 @@ class SubmitOrderChangeNotifier extends ChangeNotifier implements Subscriber {
       if (_pendingOrder?.id == order.id) {
         switch (order.state) {
           case OrderState.open:
+          case OrderState.filling:
             return;
           case OrderState.filled:
             _pendingOrder!.state = PendingOrderState.orderFilled;
             break;
           case OrderState.failed:
+          case OrderState.rejected:
             _pendingOrder!.state = PendingOrderState.orderFailed;
             break;
         }

mobile/native/src/db/mod.rs

@@ -1,6 +1,7 @@
 use crate::config;
 use crate::db::models::base64_engine;
 use crate::db::models::Channel;
+use crate::db::models::FailureReason;
 use crate::db::models::NewTrade;
 use crate::db::models::Order;
 use crate::db::models::OrderState;
@@ -13,7 +14,6 @@ use crate::db::models::Trade;
 use crate::db::models::Transaction;
 use crate::trade;
 use anyhow::anyhow;
-use anyhow::bail;
 use anyhow::Context;
 use anyhow::Result;
 use base64::Engine;
@@ -228,24 +228,50 @@ pub fn maybe_get_open_orders() -> Result<Vec<trade::order::Order>> {
     Ok(orders)
 }
 
-/// Returns an order if there is currently an order that is being filled
-pub fn maybe_get_order_in_filling() -> Result<Option<trade::order::Order>> {
-    let mut db = connection()?;
-    let orders = Order::get_by_state(OrderState::Filling, &mut db)?;
-
-    if orders.is_empty() {
-        return Ok(None);
-    }
-
-    if orders.len() > 1 {
-        bail!("More than one order is being filled at the same time, this should not happen. {orders:?}")
-    }
+/// Return an [`Order`] that is currently in [`OrderState::Filling`].
+pub fn get_order_in_filling() -> Result<Option<trade::order::Order>> {
+    let mut db = connection()?;
+
+    let mut orders = Order::get_by_state(OrderState::Filling, &mut db)?;
+
+    orders.sort_by(|a, b| b.creation_timestamp.cmp(&a.creation_timestamp));
+
+    let order = match orders.as_slice() {
+        [] => return Ok(None),
+        [order] => order,
+        // We strive to only have one order at a time in `OrderState::Filling`. But, if we do not
+        // manage, we take the most oldest one.
+        [oldest_order, rest @ ..] => {
+            tracing::warn!(
+                id = %oldest_order.id,
+                "Found more than one order in filling. Using oldest one",
+            );
+
+            // Clean up other orders in `OrderState::Filling`.
+            for order in rest {
+                tracing::debug!(
+                    id = %order.id,
+                    "Setting unexpected Filling order to Failed"
+                );
+
+                if let Err(e) = Order::update_state(
+                    order.id.clone(),
+                    (
+                        OrderState::Failed,
+                        Some(order.execution_price.expect("in Filling state")),
+                        Some(FailureReason::TimedOut),
+                    ),
+                    &mut db,
+                ) {
+                    tracing::error!("Failed to set old Filling order to Failed: {e:#}");
+                };
+            }
 
-    let first = orders
-        .get(0)
-        .expect("at this point we know there is exactly one order");
+            oldest_order
+        }
+    };
 
-    Ok(Some(first.clone().try_into()?))
+    Ok(Some(order.clone().try_into()?))
 }
 
 pub fn delete_order(order_id: Uuid) -> Result<()> {

mobile/native/src/ln_dlc/mod.rs

@@ -820,7 +820,7 @@ fn update_state_after_collab_revert(
         }
         None => {
             tracing::info!("Channel is reverted before the position got opened successfully.");
-            if let Some(order) = db::maybe_get_order_in_filling()? {
+            if let Some(order) = db::get_order_in_filling()? {
                 order::handler::order_failed(
                     Some(order.id),
                     FailureReason::ProposeDlcChannel,

mobile/native/src/orderbook.rs

@@ -162,7 +162,7 @@ pub fn subscribe(
                         };
 
                         if let Err(e) =
-                            handle_orderbook_mesage(orders.clone(), &mut cached_best_price, msg)
+                            handle_orderbook_message(orders.clone(), &mut cached_best_price, msg)
                                 .await
                         {
                             tracing::error!("Failed to handle event: {e:#}");
@@ -196,7 +196,7 @@ pub fn subscribe(
     Ok(())
 }
 
-async fn handle_orderbook_mesage(
+async fn handle_orderbook_message(
     orders: Arc<Mutex<Vec<Order>>>,
     cached_best_price: &mut Prices,
     msg: String,

mobile/native/src/trade/order/api.rs

@@ -14,13 +14,15 @@ pub enum OrderType {
 
 /// State of an order
 ///
-/// Please refer to [`crate::trade::order::OrderStateTrade`]
+/// Please refer to [`crate::trade::order::OrderState`]
 #[frb]
 #[derive(Debug, Clone, Copy)]
 pub enum OrderState {
     Open,
-    Failed,
+    Filling,
     Filled,
+    Failed,
+    Rejected,
 }
 
 #[frb]
@@ -130,14 +132,11 @@ impl From<order::OrderState> for OrderState {
             order::OrderState::Open => OrderState::Open,
             order::OrderState::Filled { .. } => OrderState::Filled,
             order::OrderState::Failed { .. } => OrderState::Failed,
+            order::OrderState::Rejected => OrderState::Rejected,
+            order::OrderState::Filling { .. } => OrderState::Filling,
             order::OrderState::Initial => unimplemented!(
                 "don't expose orders that were not submitted into the orderbook to the frontend!"
             ),
-            // TODO: At the moment the UI does not depict Rejected, we map it to Failed; for better
-            // feedback we should change that eventually
-            order::OrderState::Rejected => OrderState::Failed,
-            // We don't expose this state, but treat it as Open in the UI
-            order::OrderState::Filling { .. } => OrderState::Open,
         }
     }
 }

mobile/native/src/trade/order/handler.rs

@@ -1,5 +1,6 @@
 use crate::config;
 use crate::db;
+use crate::db::get_order_in_filling;
 use crate::db::maybe_get_open_orders;
 use crate::event;
 use crate::event::EventInternal;
@@ -22,6 +23,17 @@ use uuid::Uuid;
 const ORDER_OUTDATED_AFTER: Duration = Duration::minutes(5);
 
 pub async fn submit_order(order: Order) -> Result<Uuid> {
+    // Having an order in `Filling` should mean that the subchannel is in the midst of an update.
+    // Since we currently only support one subchannel per app, it does not make sense to start
+    // another update (by submitting a new order to the orderbook) until the current one is
+    // finished.
+    if let Some(filling_order) = get_order_in_filling()? {
+        bail!(
+            "Cannot submit new order when another one is in filling: {}",
+            filling_order.id
+        );
+    }
+
     let url = format!("http://{}", config::get_http_endpoint());
     let orderbook_client = OrderbookClient::new(Url::parse(&url)?);
 
@@ -51,10 +63,16 @@ pub(crate) fn order_filling(order_id: Uuid, execution_price: f32) -> Result<()>
         let e_string = format!("{e:#}");
         match order_failed(Some(order_id), FailureReason::FailedToSetToFilling, e) {
             Ok(()) => {
-                tracing::debug!(%order_id, "Set order to failed, after failing to set it to filling");
+                tracing::debug!(
+                    %order_id,
+                    "Set order to failed, after failing to set it to filling"
+                );
             }
             Err(e) => {
-                tracing::error!(%order_id, "Failed to set order to failed, after failing to set it to filling: {e:#}");
+                tracing::error!(
+                    %order_id,
+                    "Failed to set order to failed, after failing to set it to filling: {e:#}"
+                );
             }
         };
 
@@ -65,12 +83,15 @@ pub(crate) fn order_filling(order_id: Uuid, execution_price: f32) -> Result<()>
 }
 
 pub(crate) fn order_filled() -> Result<Order> {
-    let (order_being_filled, execution_price) = match get_order_being_filled()? {
-        order @ Order {
-            state: OrderState::Filling { execution_price },
-            ..
-        } => (order, execution_price),
-        order => bail!("Unexpected state: {:?}", order.state),
+    let (order_being_filled, execution_price) = match get_order_in_filling()? {
+        Some(
+            order @ Order {
+                state: OrderState::Filling { execution_price },
+                ..
+            },
+        ) => (order, execution_price),
+        Some(order) => bail!("Unexpected state: {:?}", order.state),
+        None => bail!("No order to mark as Filled"),
     };
 
     let filled_order = update_order_state_in_db_and_ui(
@@ -83,24 +104,22 @@ pub(crate) fn order_filled() -> Result<Order> {
     Ok(filled_order)
 }
 
-/// Update order state to failed
-///
-/// If the order_id is know we load the order by id and set it to failed.
-/// If the order_id is not known we load the order that is currently in `Filling` state and set it
-/// to failed.
+/// Update the [`Order`]'s state to [`OrderState::Failed`].
 pub(crate) fn order_failed(
     order_id: Option<Uuid>,
     reason: FailureReason,
     error: anyhow::Error,
 ) -> Result<()> {
-    tracing::error!("Failed to execute trade for order {order_id:?}: {reason:?}: {error:#}");
+    tracing::error!(?order_id, ?reason, "Failed to execute trade: {error:#}");
 
     let order_id = match order_id {
-        None => get_order_being_filled()?.id,
-        Some(order_id) => order_id,
+        None => get_order_in_filling()?.map(|order| order.id),
+        Some(order_id) => Some(order_id),
     };
 
-    update_order_state_in_db_and_ui(order_id, OrderState::Failed { reason })?;
+    if let Some(order_id) = order_id {
+        update_order_state_in_db_and_ui(order_id, OrderState::Failed { reason })?;
+    }
 
     // TODO: fixme. this so ugly, even a Sphynx cat is beautiful against this.
     // In this function we set the order to failed but here we try to set the position to open.
@@ -124,15 +143,6 @@ pub fn get_async_order() -> Result<Option<Order>> {
     db::get_async_order()
 }
 
-fn get_order_being_filled() -> Result<Order> {
-    let order_being_filled = db::maybe_get_order_in_filling()
-        .context("Failed to load order being filled")?
-        .context("No known orders being filled")?;
-
-    Ok(order_being_filled)
-}
-
-/// Checks open orders and sets them as failed in case they timed out.
 pub fn check_open_orders() -> Result<()> {
     let open_orders = match maybe_get_open_orders() {
         Ok(orders_being_filled) => orders_being_filled,