Fix/only update position if order succeeded #585
Conversation
`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...
We pretended that these calls were async, but they weren't.
| 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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Let's fix that up when we're removing PositionState from being stored in the db.
klochowicz
force-pushed
the
fix/only-update-position-if-order-succeeded
branch
from
abac1ea
to
9529912
Compare
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.
klochowicz
force-pushed
the
fix/only-update-position-if-order-succeeded
branch
from
9529912
to
01f393c
Compare
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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!
| 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"); |
There was a problem hiding this comment.
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.
|
bors r+ |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Improved version of #567 . Made sure I did the least amount of changes needed, and addressed issues raised by @da-kami and @holzeis in previous review.