Fixing USER_SUSPEND method #102

KabDeveloper · 2021-06-09T00:12:42Z

Not yet finished

WORK IN PROGRESS

I have to figure out the way accounts balance are updated when an order is cancelled.

mzheravin · 2021-06-09T19:09:46Z

src/main/java/exchange/core2/core/common/cmd/CommandResultCode.java

@@ -61,6 +61,9 @@
 USER_MGMT_USER_NOT_SUSPENDABLE_NON_EMPTY_ACCOUNTS(-4131),
 USER_MGMT_USER_NOT_SUSPENDED(-4132),
 USER_MGMT_USER_ALREADY_SUSPENDED(-4133),
+ USER_MGMT_USER_NOT_REMOVABLE_HAS_POSITIONS(-4134),
+ USER_MGMT_USER_NOT_REMOVABLE_NON_EMPTY_ACCOUNTS(-4135),
+ USER_SUSPENDED(-4136),


You can rename to RISK_USER_SUSPENDED(-2005), because this is directly related to the Risk Control section.
This is was a bug indeed, suspended users can not place any new order. Thanks for fixing it.

Done. You are most welcome, thanks to you for sharing this awesome work !

mzheravin · 2021-06-09T19:21:40Z

src/main/java/exchange/core2/core/orderbook/IOrderBook.java

@@ -203,6 +211,10 @@ static CommandResultCode processCommand(final IOrderBook orderBook, final OrderC
 cmd.marketData = orderBook.getL2MarketDataSnapshot(size >= 0 ? size : Integer.MAX_VALUE);
 return CommandResultCode.SUCCESS;

+ } else if (commandType == OrderCommandType.SUSPEND_USER) {
+
+ return orderBook.cancelOrdersByUid(cmd);


Suspending users should not cancel existing orders as this is expensive (not low-latency) operation. Approach is different. I will put a description into the PR.

Check this idea please:
#102 (comment)

mzheravin · 2021-06-09T19:56:34Z

Initial idea of users removal was:

External system (gateway) lock trading for specific client, stop accepting new orders.
We assume client's orders and positions (if margin trading is enabled) - are known externally (should be tracked by events or journal). That is needed for steps 2 and 3.
Cancel requests sent for each of them, one by one. Some of them may not succeed because orders can get executed before cancel command arrive, but this is tracked by the external anyway.
If margin trading is enabled: After orders cancelled (received all corresponding events) - close positions by issuing corresponding closing orders, one by one. Because all limit orders were previously removed (and confirmed), resulting positions guaranteed to be closed completely.
Balance adjustment operations (withdrowals) issued to zero all balances.
Suspend command is issued to the core. User record completely removed from the risk control (if possible) - so it not affecting peformance in any way. However for finantial accounting cosistency, if all steps above were not executed correctly - exchange core can re-create user account in SUSPENDED state. For such account external system can try remove it again - by applying same steps. Fixing positions (if margin trading is enabled) would require RESUME user first (if we apply fix from this PR).

Straight forward removal with just a single command of caurse looks much easier for beginners. It has potential latency penalty. But i think we can add it if can proce consistency gurantees for such commands.

src/main/java/exchange/core2/core/orderbook/OrderBookNaiveImpl.java

mzheravin · 2021-06-09T20:07:35Z

src/main/java/exchange/core2/core/common/MatcherEventType.java

@@ -30,6 +30,9 @@
 // After cancel/reduce order - risk engine should unlock deposit accordingly
 REDUCE,

+ // cancel orders of suspended users
+ SUSPEND,


I think you shoud issue N * REDUCE events (for each cancelled order).
Because introducing new event type can make events handling much more difficult.

KabDeveloper · 2021-06-10T23:19:46Z

Initial idea of users removal was:

1. External system (gateway) lock trading for specific client, stop accepting new orders.
   We assume client's orders and positions (if margin trading is enabled) - are known externally (should be tracked by events or journal). That is needed for steps 2 and 3.

2. Cancel requests sent for each of them, one by one. Some of them may not succeed because orders can get executed before cancel command arrive, but this is tracked by the external anyway.

3. If margin trading is enabled: After orders cancelled (received all corresponding events) - close positions by issuing corresponding closing orders, one by one. Because all limit orders were previously removed (and confirmed), resulting positions guaranteed to be closed completely.

4. Balance adjustment operations (withdrowals) issued to zero all balances.

5. Suspend command is issued to the core.  User record completely removed from the risk control (if possible) - so it not affecting peformance in any way. However for finantial accounting cosistency, if all steps above were not executed correctly - exchange core can re-create user account in SUSPENDED state. For such account external system can try remove it again - by applying same steps. Fixing positions (if margin trading is enabled) would require RESUME user first (if we apply fix from this PR).

Straight forward removal with just a single command of caurse looks much easier for beginners. It has potential latency penalty. But i think we can add it if can proce consistency gurantees for such commands.

Thank you for your review of the code and all infos you've shared and the way it can be improved.

What if we can allow the admin to choose between issuing a SUSPEND user request to:
1- Only suspend user without cancelling all orders
2- Suspend both: user and related (spot) orders (CURRENCY_EXCHANGE_PAIR)

The command will be sent as orderId (0 or 1), 1 to allow removing orders.

Fixing USER_SUSPEND method

0c07b49

mzheravin reviewed Jun 9, 2021

View reviewed changes

src/main/java/exchange/core2/core/orderbook/OrderBookNaiveImpl.java Outdated Show resolved Hide resolved

mzheravin reviewed Jun 9, 2021

View reviewed changes

Fixing USER_SUSPEND method

ddade8c

KabDeveloper added 2 commits June 11, 2021 00:28

Fixing USER_SUSPEND method exchange-core#3

bcf6fad

Fixing USER_SUSPEND method exchange-core#4

77e1277

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing USER_SUSPEND method #102

Fixing USER_SUSPEND method #102

KabDeveloper commented Jun 9, 2021 •

edited

mzheravin Jun 9, 2021

KabDeveloper Jun 10, 2021

mzheravin Jun 9, 2021

KabDeveloper Jun 10, 2021

mzheravin commented Jun 9, 2021

mzheravin Jun 9, 2021 •

edited

KabDeveloper Jun 10, 2021

KabDeveloper commented Jun 10, 2021

Fixing USER_SUSPEND method #102

Are you sure you want to change the base?

Fixing USER_SUSPEND method #102

Conversation

KabDeveloper commented Jun 9, 2021 • edited

mzheravin Jun 9, 2021

Choose a reason for hiding this comment

KabDeveloper Jun 10, 2021

Choose a reason for hiding this comment

mzheravin Jun 9, 2021

Choose a reason for hiding this comment

KabDeveloper Jun 10, 2021

Choose a reason for hiding this comment

mzheravin commented Jun 9, 2021

mzheravin Jun 9, 2021 • edited

Choose a reason for hiding this comment

KabDeveloper Jun 10, 2021

Choose a reason for hiding this comment

KabDeveloper commented Jun 10, 2021

KabDeveloper commented Jun 9, 2021 •

edited

mzheravin Jun 9, 2021 •

edited