Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/only update position if order succeeded #585

Merged
merged 3 commits into from
May 3, 2023
Merged

Fix/only update position if order succeeded #585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
777bfff
chore: Improve naming in orderbook API
klochowicz May 1, 2023
ecd1b5e
chore: Remove unneeded async/await
klochowicz May 1, 2023
01f393c
fix(mobile): Only update position state after order submitted
klochowicz May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion coordinator/src/orderbook/db/orders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<OrderbookOrder> {
pub fn set_is_taken(
conn: &mut PgConnection,
id: Uuid,
is_taken: bool,
) -> QueryResult<OrderbookOrder> {
let order: Order = diesel::update(orders::table)
.filter(orders::trader_order_id.eq(id))
.set(orders::taken.eq(is_taken))
Expand Down
4 changes: 2 additions & 2 deletions coordinator/src/orderbook/routes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:#}");
}
Expand Down Expand Up @@ -192,7 +192,7 @@ pub async fn put_order(
Json(updated_order): Json<UpdateOrder>,
) -> Result<Json<Order>, 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);
Expand Down
2 changes: 1 addition & 1 deletion coordinator/src/orderbook/tests/sample_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
3 changes: 1 addition & 2 deletions mobile/native/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,7 @@ pub async fn get_orders() -> Result<Vec<Order>> {

#[tokio::main(flavor = "current_thread")]
pub async fn get_positions() -> Result<Vec<Position>> {
let positions = position::handler::get_positions()
.await?
let positions = position::handler::get_positions()?
.into_iter()
.map(|order| order.into())
.collect::<Vec<Position>>();
Expand Down
4 changes: 3 additions & 1 deletion mobile/native/src/trade/order/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
klochowicz marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered another thing 😅

It was on purpose that we updated the position immediately after submitting the order in case we have a closing scenarios - this update is basically what sets the button state to Closing in the UI so the user does not click it again.

I think the best way would be updating the position to Closing immediately in case we are in a closing scenarios, but resetting it to open in case we fail to submit the order. I think resetting the state to open was missing, not necessarily moving the update to a later point.

Sorry, I did not think about this detail yesterday.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best would be to change the state in the UI to set it to Closing upon clock until we get an update from the backend. But that might be a bit tricky as well :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I perceive the risk from this being small: the action is swiping not just clicking and we get the success dialog in between.

(e.g. one can't fat-finger twice accidentally the same way as with buttons).

This is a shortcoming though, and I'll work to solve it!

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");
}
Expand All @@ -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(())
}
Expand Down
38 changes: 20 additions & 18 deletions mobile/native/src/trade/position/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -58,32 +57,36 @@ pub async fn trade(filled: FilledWith) -> Result<()> {
}

/// Fetch the positions from the database
pub async fn get_positions() -> Result<Vec<Position>> {
pub fn get_positions() -> Result<Vec<Position>> {
db::get_positions()
}

/// Update the position once an order was submitted
///
/// 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<Option<Position>> {
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");
Comment on lines +82 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that we want to check this, but for me it's unexpected that the check is happening implicitly when calling get_position_matching_order.

I think it would be clearer if we extracted this into a separate function does_order_close_position and just call it if we do have an order in the database. It's two calls rather than one, but the current behaviour is surprising to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to work on that as a follow-up.

my goal was to change as little as possible, as the previous one accidentally changed the logic during the refactor.
(including keeping the flow of error handling etc.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I guess now that you extracted this function it just feels weird that this new function can error like this, whereas originally it didn't bother me. It's a minor detail, specially since as you say it was already effectively the same before your changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @luckysori that does_order_close_position is a better name than get_position_matching_order - the function was newly introduced. I think it'd be great to rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@da-kami : @luckysori didn't want to rename this, we wanted to extract one more function, probably with bool return. I opted to not do this for now, out of fear of last-minute in-adverted changes to the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix that up when we're removing PositionState from being stored in the db.

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
Expand Down Expand Up @@ -169,8 +172,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)
Expand Down