From 777bfff18e824dd0db05e1293a9db93e0f6394bd Mon Sep 17 00:00:00 2001 From: Mariusz Klochowicz Date: Mon, 1 May 2023 15:11:57 -0400 Subject: [PATCH 1/3] chore: Improve naming in orderbook API `taken()` gave the impression that you can only mark it as taken, but in fact one can unmark it as well. Whether "untaking" an order is a reasonable thing to do, I'm not so sure... --- coordinator/src/orderbook/db/orders.rs | 6 +++++- coordinator/src/orderbook/routes.rs | 4 ++-- coordinator/src/orderbook/tests/sample_test.rs | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/coordinator/src/orderbook/db/orders.rs b/coordinator/src/orderbook/db/orders.rs index 769e68b70..a3dd75209 100644 --- a/coordinator/src/orderbook/db/orders.rs +++ b/coordinator/src/orderbook/db/orders.rs @@ -164,7 +164,11 @@ pub fn insert(conn: &mut PgConnection, order: OrderbookNewOrder) -> QueryResult< } /// Returns the number of affected rows: 1. -pub fn taken(conn: &mut PgConnection, id: Uuid, is_taken: bool) -> QueryResult { +pub fn set_is_taken( + conn: &mut PgConnection, + id: Uuid, + is_taken: bool, +) -> QueryResult { let order: Order = diesel::update(orders::table) .filter(orders::trader_order_id.eq(id)) .set(orders::taken.eq(is_taken)) diff --git a/coordinator/src/orderbook/routes.rs b/coordinator/src/orderbook/routes.rs index 792ff9a84..a723b42f7 100644 --- a/coordinator/src/orderbook/routes.rs +++ b/coordinator/src/orderbook/routes.rs @@ -158,7 +158,7 @@ pub async fn post_order( notify_traders(matched_orders, authenticated_users.clone()).await; for order_id in orders_to_set_taken { - if let Err(err) = db::orders::taken(&mut conn, order_id, true) { + if let Err(err) = db::orders::set_is_taken(&mut conn, order_id, true) { let order_id = order_id.to_string(); tracing::error!(order_id, "Could not set order to taken {err:#}"); } @@ -192,7 +192,7 @@ pub async fn put_order( Json(updated_order): Json, ) -> Result, AppError> { let mut conn = get_db_connection(&state)?; - let order = orderbook::db::orders::taken(&mut conn, order_id, updated_order.taken) + let order = orderbook::db::orders::set_is_taken(&mut conn, order_id, updated_order.taken) .map_err(|e| AppError::InternalServerError(format!("Failed to update order: {e:#}")))?; let sender = state.tx_pricefeed.clone(); update_pricefeed(OrderbookMsg::Update(order.clone()), sender); diff --git a/coordinator/src/orderbook/tests/sample_test.rs b/coordinator/src/orderbook/tests/sample_test.rs index 5f79b8eda..c3893c511 100644 --- a/coordinator/src/orderbook/tests/sample_test.rs +++ b/coordinator/src/orderbook/tests/sample_test.rs @@ -31,7 +31,7 @@ async fn crud_test() { ) .unwrap(); - let order = orders::taken(&mut conn, order.id, true).unwrap(); + let order = orders::set_is_taken(&mut conn, order.id, true).unwrap(); assert!(order.taken); let deleted = orders::delete_with_id(&mut conn, order.id).unwrap(); From ecd1b5efd1d344ffbf645254722a9d4422e63d96 Mon Sep 17 00:00:00 2001 From: Mariusz Klochowicz Date: Mon, 1 May 2023 16:16:17 -0400 Subject: [PATCH 2/3] chore: Remove unneeded async/await We pretended that these calls were async, but they weren't. --- mobile/native/src/api.rs | 3 +-- mobile/native/src/trade/position/handler.rs | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/mobile/native/src/api.rs b/mobile/native/src/api.rs index 9d680566e..6b38f4a2c 100644 --- a/mobile/native/src/api.rs +++ b/mobile/native/src/api.rs @@ -157,8 +157,7 @@ pub async fn get_orders() -> Result> { #[tokio::main(flavor = "current_thread")] pub async fn get_positions() -> Result> { - let positions = position::handler::get_positions() - .await? + let positions = position::handler::get_positions()? .into_iter() .map(|order| order.into()) .collect::>(); diff --git a/mobile/native/src/trade/position/handler.rs b/mobile/native/src/trade/position/handler.rs index 6b4fac391..038dfb318 100644 --- a/mobile/native/src/trade/position/handler.rs +++ b/mobile/native/src/trade/position/handler.rs @@ -58,7 +58,7 @@ pub async fn trade(filled: FilledWith) -> Result<()> { } /// Fetch the positions from the database -pub async fn get_positions() -> Result> { +pub fn get_positions() -> Result> { db::get_positions() } @@ -169,8 +169,7 @@ pub fn price_update(prices: Prices) -> Result<()> { // a close position. In order to fixup the ui, we are // creating an order here and store it to the database. pub async fn close_position() -> Result<()> { - let positions = get_positions() - .await? + let positions = get_positions()? .into_iter() .filter(|p| p.position_state == PositionState::Open) .map(Position::from) From 01f393c2efebfbed45ff61f5d8850a21cb6484f5 Mon Sep 17 00:00:00 2001 From: Mariusz Klochowicz Date: Wed, 3 May 2023 14:48:44 -0400 Subject: [PATCH 3/3] fix(mobile): Only update position state after order submitted Extract the logic checking whether the order is matching the currently open position (we fail early if it's exact opposite direction and exact quantity). Position state is only updated if the order got submitted. --- mobile/native/src/trade/order/handler.rs | 4 ++- mobile/native/src/trade/position/handler.rs | 33 +++++++++++---------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/mobile/native/src/trade/order/handler.rs b/mobile/native/src/trade/order/handler.rs index ac1e15a0b..55ca019a9 100644 --- a/mobile/native/src/trade/order/handler.rs +++ b/mobile/native/src/trade/order/handler.rs @@ -7,6 +7,7 @@ use crate::trade::order::FailureReason; use crate::trade::order::Order; use crate::trade::order::OrderState; use crate::trade::position; +use crate::trade::position::handler::update_position_after_order_submitted; use anyhow::bail; use anyhow::Context; use anyhow::Result; @@ -17,7 +18,7 @@ pub async fn submit_order(order: Order) -> Result<()> { let url = format!("http://{}", config::get_http_endpoint()); let orderbook_client = OrderbookClient::new(Url::parse(&url)?); - if let Err(e) = position::handler::update_position_after_order_submitted(order) { + if let Err(e) = position::handler::get_position_matching_order(&order) { order_failed(Some(order.id), FailureReason::OrderNotAcceptable, e)?; bail!("Could not submit order because extending/reducing the position is not part of the MVP scope"); } @@ -35,6 +36,7 @@ pub async fn submit_order(order: Order) -> Result<()> { } update_order_state_in_db_and_ui(order.id, OrderState::Open)?; + update_position_after_order_submitted(&order)?; Ok(()) } diff --git a/mobile/native/src/trade/position/handler.rs b/mobile/native/src/trade/position/handler.rs index 038dfb318..1f0070051 100644 --- a/mobile/native/src/trade/position/handler.rs +++ b/mobile/native/src/trade/position/handler.rs @@ -9,7 +9,6 @@ use crate::trade::order::OrderState; use crate::trade::order::OrderType; use crate::trade::position::Position; use crate::trade::position::PositionState; -use anyhow::bail; use anyhow::ensure; use anyhow::Context; use anyhow::Result; @@ -66,24 +65,28 @@ pub fn get_positions() -> Result> { /// /// If the new order submitted is an order that closes the current position, then the position will /// be updated to `Closing` state. -pub fn update_position_after_order_submitted(submitted_order: Order) -> Result<()> { - if let Some(position) = db::get_positions()?.first() { - // closing the position - if position.direction == submitted_order.direction.opposite() - && position.quantity == submitted_order.quantity - { - db::update_position_state(position.contract_symbol, PositionState::Closing)?; - let mut position = position.clone(); - position.position_state = PositionState::Closing; - event::publish(&EventInternal::PositionUpdateNotification(position)); - } else { - bail!("Currently not possible to extend or reduce a position, you can only close the position with a counter-order"); - } +pub fn update_position_after_order_submitted(submitted_order: &Order) -> Result<()> { + if let Some(position) = get_position_matching_order(submitted_order)? { + db::update_position_state(position.contract_symbol, PositionState::Closing)?; + let mut position = position; + position.position_state = PositionState::Closing; + event::publish(&EventInternal::PositionUpdateNotification(position)); } - Ok(()) } +/// Returns the position that would be closed by submitted, if there is any +pub fn get_position_matching_order(order: &Order) -> Result> { + Ok(if let Some(position) = db::get_positions()?.first() { + // closing the position + ensure!(position.direction == order.direction.opposite() + && position.quantity == order.quantity, "Currently not possible to extend or reduce a position, you can only close the position with a counter-order"); + Some(position.clone()) + } else { + None + }) +} + /// Resets the position to open again /// /// This should be called if a went in a dirty state, e.g. the position is currently in