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

Remove collateralization-check for unmatched orders #1587

Merged
merged 2 commits into from
May 30, 2024
Merged

Remove collateralization-check for unmatched orders #1587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd125ee
remove collateralization-check for unmatched orders
BrendanChou May 26, 2024
e5c6b8e
fix tests
roy-dydx May 29, 2024
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
30 changes: 0 additions & 30 deletions protocol/mocks/MemClobKeeper.go
View file
Open in desktop

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,40 +637,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 @@ -1467,59 +1433,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
Loading