Skip to content

Commit

Permalink
Remove collateralization-check for unmatched orders (#1587)
Browse files Browse the repository at this point in the history 
Co-authored-by: Roy Li <roy@dydx.exchange>
  • Loading branch information
@BrendanChou @roy-dydx
BrendanChou and roy-dydx authored May 30, 2024
1 parent 542a9d5 commit 4eb219b
Show file tree
Hide file tree
Showing 13 changed files with 23 additions and 807 deletions.
30 changes: 0 additions & 30 deletions protocol/mocks/MemClobKeeper.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 0 additions & 11 deletions protocol/testutil/memclob/keeper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,17 +284,6 @@ func (f *FakeMemClobKeeper) GetStatefulOrdersTimeSlice(
return orderIds
}

func (f *FakeMemClobKeeper) AddOrderToOrderbookSubaccountUpdatesCheck(
ctx sdk.Context,
clobPairId types.ClobPairId,
subaccountOpenOrders map[satypes.SubaccountId][]types.PendingOpenOrder,
) (
success bool,
successPerUpdate map[satypes.SubaccountId]satypes.UpdateResult,
) {
return f.collatCheckFn(subaccountOpenOrders)
}

func (f *FakeMemClobKeeper) addFakePositionSize(
ctx sdk.Context,
clobPairId types.ClobPairId,
Expand Down
50 changes: 0 additions & 50 deletions protocol/x/clob/e2e/order_removal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,35 +183,6 @@ func TestConditionalOrderRemoval(t *testing.T) {
// TODO(CORE-858): Re-enable determinism checks once non-determinism issue is found and resolved.
disableNonDeterminismChecks: true,
},
"under-collateralized conditional taker when adding to book is removed": {
subaccounts: []satypes.Subaccount{
constants.Carl_Num0_100000USD,
constants.Dave_Num0_10000USD,
},
orders: []clobtypes.Order{
constants.LongTermOrder_Carl_Num0_Id0_Clob0_Buy1BTC_Price49500_GTBT10,
// Does not cross with best bid.
constants.ConditionalOrder_Dave_Num0_Id0_Clob0_Sell1BTC_Price50000_GTBT10_SL_50003,
},
withdrawal: &sendingtypes.MsgWithdrawFromSubaccount{
Sender: constants.Dave_Num0,
Recipient: constants.DaveAccAddress.String(),
AssetId: constants.Usdc.Id,
Quantums: 10_000_000_000,
},
priceUpdate: &prices.MsgUpdateMarketPrices{
MarketPriceUpdates: []*prices.MsgUpdateMarketPrices_MarketPrice{
prices.NewMarketPriceUpdate(0, 5_000_250_000),
},
},

expectedOrderRemovals: []bool{
false,
true, // taker order fails add-to-orderbook collateralization check
},
// TODO(CORE-858): Re-enable determinism checks once non-determinism issue is found and resolved.
disableNonDeterminismChecks: true,
},
"under-collateralized conditional maker is removed": {
subaccounts: []satypes.Subaccount{
constants.Carl_Num0_10000USD,
Expand Down Expand Up @@ -808,27 +779,6 @@ func TestOrderRemoval(t *testing.T) {
// TODO(CORE-858): Re-enable determinism checks once non-determinism issue is found and resolved.
disableNonDeterminismChecks: true,
},
"under-collateralized taker when adding to book is removed": {
subaccounts: []satypes.Subaccount{
constants.Carl_Num0_10000USD,
constants.Dave_Num0_10000USD,
},
firstOrder: constants.LongTermOrder_Carl_Num0_Id0_Clob0_Buy1BTC_Price49500_GTBT10,
// Does not cross with best bid.
secondOrder: constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell1BTC_Price50000_GTBT10,

withdrawal: &sendingtypes.MsgWithdrawFromSubaccount{
Sender: constants.Dave_Num0,
Recipient: constants.DaveAccAddress.String(),
AssetId: constants.Usdc.Id,
Quantums: 10_000_000_000,
},

expectedFirstOrderRemoved: false,
expectedSecondOrderRemoved: true, // taker order fails add-to-orderbook collateralization check
// TODO(CORE-858): Re-enable determinism checks once non-determinism issue is found and resolved.
disableNonDeterminismChecks: true,
},
"under-collateralized maker is removed": {
subaccounts: []satypes.Subaccount{
constants.Carl_Num0_10000USD,
Expand Down
144 changes: 0 additions & 144 deletions protocol/x/clob/keeper/orders_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,29 +158,6 @@ func TestPlaceShortTermOrder(t *testing.T) {
constants.BtcUsd_SmallMarginRequirement.Params.Id: big.NewInt(0),
},
},
"Cannot place an order on the orderbook if the account would be undercollateralized": {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_SmallMarginRequirement,
constants.EthUsd_20PercentInitial_10PercentMaintenance,
},
subaccounts: []satypes.Subaccount{
constants.Carl_Num0_599USD,
},
clobs: []types.ClobPair{
constants.ClobPair_Btc,
constants.ClobPair_Eth,
},
feeParams: constants.PerpetualFeeParams,

order: constants.Order_Carl_Num0_Id3_Clob1_Buy1ETH_Price3000,

expectedOrderStatus: types.Undercollateralized,
expectedFilledSize: 0,
expectedOpenInterests: map[uint32]*big.Int{
// unchanged, no match happened
constants.BtcUsd_SmallMarginRequirement.Params.Id: big.NewInt(100_000_000),
},
},
"Can place an order on the orderbook if the subaccount is right at the initial margin ratio": {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_100PercentMarginRequirement,
Expand All @@ -202,28 +179,6 @@ func TestPlaceShortTermOrder(t *testing.T) {
constants.BtcUsd_SmallMarginRequirement.Params.Id: big.NewInt(100_000_000),
},
},
"Cannot place an order on the orderbook if the account would be undercollateralized due to fees paid": {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_100PercentMarginRequirement,
},
subaccounts: []satypes.Subaccount{
constants.Carl_Num0_1BTC_Short,
},
clobs: []types.ClobPair{
// Exact same set-up as the previous test, except the clob pair has fees.
constants.ClobPair_Btc,
},
feeParams: constants.PerpetualFeeParams,

order: constants.Order_Carl_Num0_Id0_Clob0_Buy10QtBTC_Price100000QuoteQt,

expectedOrderStatus: types.Undercollateralized,
expectedFilledSize: 0,
expectedOpenInterests: map[uint32]*big.Int{
// unchanged, no match happened
constants.BtcUsd_SmallMarginRequirement.Params.Id: big.NewInt(100_000_000),
},
},
"Can place an order on the orderbook if the account would be collateralized due to rebate": {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_100PercentMarginRequirement,
Expand Down Expand Up @@ -394,73 +349,6 @@ func TestPlaceShortTermOrder(t *testing.T) {
constants.BtcUsd_SmallMarginRequirement.Params.Id: big.NewInt(100_000_000),
},
},
// This is a regression test for an issue whereby orders that had been previously matched were being checked for
// collateralization as if the subticks of the order were `0`. This resulted in always using `0`
// `bigFillQuoteQuantums` for the order when performing collateralization checks during `PlaceOrder`.
// This meant that previous buy orders in the match queue could only ever increase collateralization
// of the subaccount.
// Context: https://dydx-team.slack.com/archives/C03SLFHC3L7/p1668105457456389
`Regression: New order should be undercollateralized when adding to the orderbook when previous fills make it
undercollateralized`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_100PercentMarginRequirement,
},
subaccounts: []satypes.Subaccount{
constants.Carl_Num1_500USD,
constants.Carl_Num0_10000USD,
},
clobs: []types.ClobPair{
constants.ClobPair_Btc,
},
existingOrders: []types.Order{
// The maker subaccount places an order which is a maker order to buy $500 worth of BTC.
// The subaccount has a balance of $500 worth of USDC, and the perpetual has a 100% margin requirement.
// This order does not match, and is placed on the book as a maker order.
constants.Order_Carl_Num1_Id0_Clob0_Buy1kQtBTC_Price50000,
// The taker subaccount places an order which fully fills the previous order.
constants.Order_Carl_Num0_Id0_Clob0_Sell1kQtBTC_Price50000,
},
feeParams: constants.PerpetualFeeParamsNoFee,
// The maker subaccount places a second order identical to the first.
// This should fail, because the maker subaccount currently has a balance of $0 USDC, and a perpetual of size
// 0.01 BTC ($500), and the perpetual has a 100% margin requirement.
order: constants.Order_Carl_Num1_Id1_Clob0_Buy1kQtBTC_Price50000,
expectedOrderStatus: types.Undercollateralized,
},
`Regression: New order should be undercollateralized when matching when previous fills make it
undercollateralized`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_100PercentMarginRequirement,
},
subaccounts: []satypes.Subaccount{
constants.Carl_Num1_500USD,
constants.Carl_Num0_10000USD,
},
clobs: []types.ClobPair{
constants.ClobPair_Btc,
},
existingOrders: []types.Order{
// The maker subaccount places an order which is a maker order to buy $500 worth of BTC.
// The subaccount has a balance of $500 worth of USDC, and the perpetual has a 100% margin requirement.
// This order does not match, and is placed on the book as a maker order.
constants.Order_Carl_Num1_Id0_Clob0_Buy1kQtBTC_Price50000,
// The taker subaccount places an order which fully fills the previous order.
constants.Order_Carl_Num0_Id0_Clob0_Sell1kQtBTC_Price50000,
// Match queue is now empty.
// The subaccount from the above order now places an order which is added to the book.
constants.Order_Carl_Num0_Id1_Clob0_Sell1kQtBTC_Price50000,
},
feeParams: constants.PerpetualFeeParamsNoFee,
// The maker subaccount places a second order identical to the first.
// This should fail, because the maker during matching, because subaccount currently has a balance of $0 USDC,
// and a perpetual of size 0.01 BTC ($500), and the perpetual has a 100% margin requirement.
order: constants.Order_Carl_Num1_Id1_Clob0_Buy1kQtBTC_Price50000,
expectedOrderStatus: types.Undercollateralized,
expectedOpenInterests: map[uint32]*big.Int{
// 1 BTC + 0.01 BTC filled
constants.BtcUsd_100PercentMarginRequirement.Params.Id: big.NewInt(101_000_000),
},
},
`New order should be undercollateralized when matching when previous fills make it undercollateralized when using
maker orders subticks, but would be collateralized if using taker order subticks`: {
perpetuals: []perptypes.Perpetual{
Expand Down Expand Up @@ -599,22 +487,6 @@ func TestPlaceShortTermOrder(t *testing.T) {
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
},
`Subaccount cannot place buy order due to a failed collateralization check with its maker price but would
pass if using the oracle price`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_50PercentInitial_40PercentMaintenance,
},
subaccounts: []satypes.Subaccount{constants.Carl_Num0_100000USD},
clobs: []types.ClobPair{
constants.ClobPair_Btc,
},
existingOrders: []types.Order{},
feeParams: constants.PerpetualFeeParamsNoFee,
order: constants.Order_Carl_Num0_Id0_Clob0_Buy1BTC_Price500000_GTB10,
expectedOrderStatus: types.Undercollateralized,
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
},
`Subaccount placed buy order passes collateralization check when using the maker price`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_50PercentInitial_40PercentMaintenance,
Expand Down Expand Up @@ -652,22 +524,6 @@ func TestPlaceShortTermOrder(t *testing.T) {
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
},
`Subaccount cannot place sell order due to a failed collateralization check with its maker price but would
pass if using the oracle price`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_50PercentInitial_40PercentMaintenance,
},
subaccounts: []satypes.Subaccount{constants.Carl_Num0_50000USD},
clobs: []types.ClobPair{
constants.ClobPair_Btc,
},
existingOrders: []types.Order{},
feeParams: constants.PerpetualFeeParamsNoFee,
order: constants.Order_Carl_Num0_Id0_Clob0_Sell1BTC_Price5000_GTB10,
expectedOrderStatus: types.Undercollateralized,
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
},
`Subaccount placed sell order passes collateralization check when using the maker price`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_50PercentInitial_40PercentMaintenance,
Expand Down
87 changes: 0 additions & 87 deletions protocol/x/clob/memclob/memclob.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -640,40 +640,6 @@ func (m *MemClobPriceTimePriority) PlaceOrder(
return orderSizeOptimisticallyFilledFromMatchingQuantums, orderStatus, offchainUpdates, nil
}

// The taker order has unfilled size which will be added to the orderbook as a maker order.
// Verify the maker order can be added to the orderbook by performing the add-to-orderbook
// subaccount updates check.
addOrderOrderStatus := m.addOrderToOrderbookSubaccountUpdatesCheck(
ctx,
order,
)

// If the add order to orderbook subaccount updates check failed, we cannot add the order to the orderbook.
if !addOrderOrderStatus.IsSuccess() {
if m.generateOffchainUpdates {
// Send an off-chain update message indicating the order should be removed from the orderbook
// on the Indexer.
if message, success := off_chain_updates.CreateOrderRemoveMessage(
ctx,
order.OrderId,
addOrderOrderStatus,
nil,
ocutypes.OrderRemoveV1_ORDER_REMOVAL_STATUS_BEST_EFFORT_CANCELED,
); success {
offchainUpdates.AddRemoveMessage(order.OrderId, message)
}
}

// remove stateful orders which fail collateralization check while being added to orderbook
if order.IsStatefulOrder() && !m.operationsToPropose.IsOrderRemovalInOperationsQueue(order.OrderId) {
m.operationsToPropose.MustAddOrderRemovalToOperationsQueue(
order.OrderId,
types.OrderRemoval_REMOVAL_REASON_UNDERCOLLATERALIZED,
)
}
return orderSizeOptimisticallyFilledFromMatchingQuantums, addOrderOrderStatus, offchainUpdates, nil
}

// If this is a Short-Term order and it's not in the operations queue, add the TX bytes to the
// operations to propose.
if order.IsShortTermOrder() &&
Expand Down Expand Up @@ -1481,59 +1447,6 @@ func (m *MemClobPriceTimePriority) validateNewOrder(
return nil
}

// addOrderToOrderbookSubaccountUpdatesCheck will perform a check to verify that the subaccount updates
// if the new maker order were to be fully filled are valid.
// It returns the result of this subaccount updates check. If the check returns an error, it will return
// the error so that it can be surfaced to the client.
//
// This function will assume that all prior order validation has passed, including the pre-requisite validation of
// `validateNewOrder` and the actual validation performed within `validateNewOrder`.
// Note that this is a loose check, mainly for the purposes of spam mitigation. We perform an additional
// check on the subaccount updates for orders when we attempt to match them.
func (m *MemClobPriceTimePriority) addOrderToOrderbookSubaccountUpdatesCheck(
ctx sdk.Context,
order types.Order,
) types.OrderStatus {
defer telemetry.ModuleMeasureSince(
types.ModuleName,
time.Now(),
metrics.PlaceOrder,
metrics.Memclob,
metrics.AddToOrderbookCollateralizationCheck,
metrics.Latency,
)

orderId := order.OrderId
subaccountId := orderId.SubaccountId

// For the collateralization check, use the remaining amount of the order that is resting on the book.
remainingAmount, hasRemainingAmount := m.GetOrderRemainingAmount(ctx, order)
if !hasRemainingAmount {
panic(fmt.Sprintf("addOrderToOrderbookSubaccountUpdatesCheck: order has no remaining amount %v", order))
}

pendingOpenOrder := types.PendingOpenOrder{
RemainingQuantums: remainingAmount,
IsBuy: order.IsBuy(),
Subticks: order.GetOrderSubticks(),
ClobPairId: order.GetClobPairId(),
}

// Temporarily construct the subaccountOpenOrders with a single PendingOpenOrder.
subaccountOpenOrders := make(map[satypes.SubaccountId][]types.PendingOpenOrder)
subaccountOpenOrders[subaccountId] = []types.PendingOpenOrder{pendingOpenOrder}

// TODO(DEC-1896): AddOrderToOrderbookSubaccountUpdatesCheck should accept a single PendingOpenOrder as a
// parameter rather than the subaccountOpenOrders map.
_, successPerSubaccountUpdate := m.clobKeeper.AddOrderToOrderbookSubaccountUpdatesCheck(
ctx,
order.GetClobPairId(),
subaccountOpenOrders,
)

return updateResultToOrderStatus(successPerSubaccountUpdate[subaccountId])
}

// mustAddOrderToOrderbook will add the order to the resting orderbook.
// This function will assume that all order validation has already been done.
// If `forceToFrontOfLevel` is true, places the order at the head of the level,
Expand Down
Loading

0 comments on commit 4eb219b

Please sign in to comment.