Skip to content

Commit

Permalink
[CT-619] deprecate pessimistic add-to-book collateralization check (#…
Browse files Browse the repository at this point in the history 
…1107) (#1377)

* [CT-619] deprecate pessimistic add-to-book collateralization check

* fix lint

* fix e2e test

* fix test

* Update protocol/x/clob/keeper/orders_test.go



* Update protocol/x/clob/keeper/orders_test.go



* remove comments

---------

Co-authored-by: Lukasz Cwik <126621805+lcwik@users.noreply.github.com>
  • Loading branch information
@jayy04 @lcwik
jayy04 and lcwik authored Apr 10, 2024
1 parent db2b93a commit 52397f9
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 101 deletions.
4 changes: 2 additions & 2 deletions protocol/testing/e2e/gov/add_new_market_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ const (
NumBlocksAfterTradingEnabled = 50
TestMarketId = 1001
// Expected response log when a order is submitted but oracle price is zero.
ExpectedPlaceOrderCheckTxResponseLog = "recovered: clob pair ID = (1001), perpetual ID = (1001), " +
"market ID = (1001): Oracle price must be > 0"
// https://github.com/dydxprotocol/v4-chain/blob/5ee11ed/protocol/x/perpetuals/keeper/perpetual.go#L1514-L1517
ExpectedPlaceOrderCheckTxResponseLog = "recovered: type: perpetual, id: 1001: product position is not updatable"
)

var (
Expand Down
29 changes: 28 additions & 1 deletion protocol/x/clob/keeper/msg_server_place_order_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"
"time"

"github.com/dydxprotocol/v4-chain/protocol/dtypes"
indexerevents "github.com/dydxprotocol/v4-chain/protocol/indexer/events"
"github.com/dydxprotocol/v4-chain/protocol/indexer/indexer_manager"
"github.com/dydxprotocol/v4-chain/protocol/lib"
Expand Down Expand Up @@ -48,9 +49,14 @@ func TestPlaceOrder_Error(t *testing.T) {
ExpectedError: types.ErrTimeExceedsGoodTilBlockTime,
},
"Returns an error when collateralization check fails": {
StatefulOrderPlacement: constants.LongTermOrder_Bob_Num0_Id2_Clob0_Buy15_Price5_GTBT10,
StatefulOrderPlacement: constants.LongTermOrder_Alice_Num0_Id1_Clob0_Buy1BTC_Price50000_GTBT15,
ExpectedError: types.ErrStatefulOrderCollateralizationCheckFailed,
},
"Returns an error when equity tier check fails": {
// Bob has TNC of $0.
StatefulOrderPlacement: constants.LongTermOrder_Bob_Num0_Id2_Clob0_Buy15_Price5_GTBT10,
ExpectedError: types.ErrOrderWouldExceedMaxOpenOrdersEquityTierLimit,
},
"Returns an error when order replacement is attempted": {
StatefulOrders: []types.Order{
constants.LongTermOrder_Alice_Num0_Id0_Clob0_Buy5_Price10_GTBT15,
Expand Down Expand Up @@ -129,6 +135,27 @@ func TestPlaceOrder_Error(t *testing.T) {
)
require.NoError(t, err)

ks.SubaccountsKeeper.SetSubaccount(ks.Ctx, constants.Alice_Num0_10_000USD)

err = ks.ClobKeeper.InitializeEquityTierLimit(
ks.Ctx,
types.EquityTierLimitConfiguration{
ShortTermOrderEquityTiers: []types.EquityTierLimit{
{
UsdTncRequired: dtypes.NewInt(20_000_000),
Limit: 5,
},
},
StatefulOrderEquityTiers: []types.EquityTierLimit{
{
UsdTncRequired: dtypes.NewInt(20_000_000),
Limit: 5,
},
},
},
)
require.NoError(t, err)

// Create ClobPair.
clobPair := constants.ClobPair_Btc
// PerpetualMarketCreateEvents are emitted when initializing the genesis state, so we need to mock
Expand Down
73 changes: 1 addition & 72 deletions protocol/x/clob/keeper/orders.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package keeper

import (
"fmt"
"math"
"math/big"
"time"

Expand Down Expand Up @@ -964,12 +963,8 @@ func (k Keeper) AddOrderToOrderbookCollatCheck(
pendingUpdates := types.NewPendingUpdates()

// Retrieve the associated `PerpetualId` for the `ClobPair`.
oraclePriceSubticksRat := k.GetOraclePriceSubticksRat(ctx, clobPair)
perpetualId := clobPair.MustGetPerpetualId()

// TODO(DEC-1713): Complete as many calculations from getPessimisticCollateralCheckPrice as possible here
// so we aren't recalculating the same thing within the loop.

iterateOverOpenOrdersStart := time.Now()
for subaccountId, openOrders := range subaccountOpenOrders {
telemetry.SetGauge(
Expand All @@ -986,37 +981,7 @@ func (k Keeper) AddOrderToOrderbookCollatCheck(
panic(fmt.Sprintf("Order `ClobPairId` must equal `clobPairId` for order %+v", openOrder))
}

// We don't want to allow users to place orders to improve their collateralization, so we choose between the
// order price (user-input) or the oracle price (sane default) and select the price that results in the
// most pessimistic collateral-check outcome.
collatCheckPriceSubticks, err := getPessimisticCollateralCheckPrice(
oraclePriceSubticksRat,
openOrder.Subticks,
openOrder.IsBuy,
)
if satypes.ErrIntegerOverflow.Is(err) {
// TODO(DEC-1701): Determine best action to take if the oracle price overflows max uint64
log.ErrorLog(
ctx,
fmt.Sprintf(
"Integer overflow: oracle price (subticks) exceeded uint64 max. "+
"perpetual ID = (%d), oracle price = (%+v), is buy = (%t)",
perpetualId,
oraclePriceSubticksRat,
openOrder.IsBuy,
),
)
} else if err != nil {
panic(
errorsmod.Wrapf(
err,
"perpetual id = (%d), oracle price = (%+v), is buy = (%t)",
perpetualId,
oraclePriceSubticksRat,
openOrder.IsBuy,
),
)
}
collatCheckPriceSubticks := openOrder.Subticks

bigFillQuoteQuantums, err := getFillQuoteQuantums(
clobPair,
Expand Down Expand Up @@ -1280,42 +1245,6 @@ func (k Keeper) SendOffchainMessages(
}
}

// getPessimisticCollateralCheckPrice returns the price in subticks we should use for collateralization checks.
// It pessimistically rounds oraclePriceSubticksRat (up for buys, down for sells) and then pessimistically
// chooses the subticks value to return: the highest for buys, the lowest for sells.
//
// Returns an error if:
// - the oraclePriceSubticksRat, after being rounded pessimistically, overflows a uint64
func getPessimisticCollateralCheckPrice(
oraclePriceSubticksRat *big.Rat,
makerOrderSubticks types.Subticks,
isBuy bool,
) (price types.Subticks, err error) {
// TODO(DEC-1713): Move this rounding to before the PendingOpenOrders loop.
// The oracle price is guaranteed to be >= 0. Since we are using this value for a collateralization check,
// we want to round pessimistically (up for buys, down for sells).
oraclePriceSubticksInt := lib.BigRatRound(oraclePriceSubticksRat, isBuy)

// TODO(DEC-1701): Determine best action to take if the oracle price overflows max uint64
var oraclePriceSubticks types.Subticks
if oraclePriceSubticksInt.IsUint64() {
oraclePriceSubticks = types.Subticks(lib.Max(1, oraclePriceSubticksInt.Uint64()))
} else {
// Clamping the oracle price here should be fine because there are 2 outcomes:
// 1. This is a sell order, in which case we choose the lowest value, so we choose the maker sell price which
// would be identical to the old logic.
// 2. This is a buy order, and we select uint64 max as the price, which will fail the collateral check in any
// real-world example.
oraclePriceSubticks = types.Subticks(math.MaxUint64)
err = satypes.ErrIntegerOverflow
}

if isBuy {
return lib.Max(makerOrderSubticks, oraclePriceSubticks), err
}
return lib.Min(makerOrderSubticks, oraclePriceSubticks), err
}

// getFillQuoteQuantums returns the total fillAmount price in quote quantums based on the maker subticks.
// This value is always positive.
//
Expand Down
68 changes: 42 additions & 26 deletions protocol/x/clob/keeper/orders_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"
"time"

"github.com/dydxprotocol/v4-chain/protocol/dtypes"
indexerevents "github.com/dydxprotocol/v4-chain/protocol/indexer/events"

cmt "github.com/cometbft/cometbft/types"
Expand Down Expand Up @@ -515,12 +516,10 @@ func TestPlaceShortTermOrder(t *testing.T) {
clobs: []types.ClobPair{
constants.ClobPair_Btc,
},
existingOrders: []types.Order{},
feeParams: constants.PerpetualFeeParamsNoFee,
order: constants.Order_Carl_Num0_Id0_Clob0_Buy1BTC_Price5subticks_GTB10,
expectedOrderStatus: types.Undercollateralized,
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
existingOrders: []types.Order{},
feeParams: constants.PerpetualFeeParamsNoFee,
order: constants.Order_Carl_Num0_Id0_Clob0_Buy1BTC_Price5subticks_GTB10,
expectedErr: types.ErrOrderWouldExceedMaxOpenOrdersEquityTierLimit,
},
`Subaccount cannot place maker sell order for 1 BTC at 500,000 with 0 collateral`: {
perpetuals: []perptypes.Perpetual{
Expand All @@ -530,20 +529,18 @@ func TestPlaceShortTermOrder(t *testing.T) {
clobs: []types.ClobPair{
constants.ClobPair_Btc,
},
existingOrders: []types.Order{},
feeParams: constants.PerpetualFeeParamsNoFee,
order: constants.Order_Carl_Num0_Id0_Clob0_Sell1BTC_Price500000_GTB10,
expectedOrderStatus: types.Undercollateralized,
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
existingOrders: []types.Order{},
feeParams: constants.PerpetualFeeParamsNoFee,
order: constants.Order_Carl_Num0_Id0_Clob0_Sell1BTC_Price500000_GTB10,
expectedErr: types.ErrOrderWouldExceedMaxOpenOrdersEquityTierLimit,
},
// <grouped tests: pessimistic value collateralization check -- BUY>
// The following 3 tests are a group to test the pessimistic value used for the collateralization check.
// 1. The first should have a lower asset value in its subaccount. (undercollateralized)
// <grouped tests: deprecating pessimistic value collateralization check -- BUY>
// The following 3 tests are a group to test the deprecation of pessimistic collateralization check.
// 1. The first should have a buy price well below the oracle price of 50,000. (success)
// 2. The second should have a buy price above the oracle price of 50,000. (undercollateralized)
// 3. The third should have the order in common with #1 and the subaccount in common with #2 and should succeed.
`Subaccount cannot place buy order due to a failed collateralization check with the oracle price but would
pass if using the maker price`: {
`Subaccount can now place buy order that would have failed the
deprecated pessimistic collateralization check with the oracle price`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_50PercentInitial_40PercentMaintenance,
},
Expand All @@ -554,7 +551,7 @@ func TestPlaceShortTermOrder(t *testing.T) {
existingOrders: []types.Order{},
feeParams: constants.PerpetualFeeParamsNoFee,
order: constants.Order_Carl_Num0_Id0_Clob0_Buy1BTC_Price5subticks_GTB10,
expectedOrderStatus: types.Undercollateralized,
expectedOrderStatus: types.Success,
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
},
Expand All @@ -574,7 +571,7 @@ func TestPlaceShortTermOrder(t *testing.T) {
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
},
`Subaccount placed buy order passes collateralization check when using the oracle price`: {
`Subaccount placed buy order passes collateralization check when using the maker price`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_50PercentInitial_40PercentMaintenance,
},
Expand All @@ -590,13 +587,13 @@ func TestPlaceShortTermOrder(t *testing.T) {
expectedMultiStoreWrites: []string{},
},
// <end of grouped tests: pessimistic value collateralization check -- BUY>
// <grouped tests: pessimistic value collateralization check -- SELL>
// The following 3 tests are a group to test the pessimistic value used for the collateralization check.
// 1. The first should have a lower asset value in its subaccount. (undercollateralized)
// <grouped tests: deprecating pessimistic value collateralization check -- SELL>
// The following 3 tests are a group to test the deprecation of pessimistic collateralization check.
// 1. The first should have a sell price well above the oracle price of 50,000. (success)
// 2. The second should have a sell price below the oracle price of 50,000 subticks. (undercollateralized)
// 3. The third should have the order in common with #1 and the subaccount in common with #2 and should succeed.
`Subaccount cannot place sell order due to a failed collateralization check with the oracle price but would
pass if using the maker price`: {
`Subaccount can now place sell order that would have failed the
deprecated pessimistic collateralization check with the oracle price`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_50PercentInitial_40PercentMaintenance,
},
Expand All @@ -607,7 +604,7 @@ func TestPlaceShortTermOrder(t *testing.T) {
existingOrders: []types.Order{},
feeParams: constants.PerpetualFeeParamsNoFee,
order: constants.Order_Carl_Num0_Id0_Clob0_Sell1BTC_Price500000_GTB10,
expectedOrderStatus: types.Undercollateralized,
expectedOrderStatus: types.Success,
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
},
Expand All @@ -627,7 +624,7 @@ func TestPlaceShortTermOrder(t *testing.T) {
expectedFilledSize: 0,
expectedMultiStoreWrites: []string{},
},
`Subaccount placed sell order passes collateralization check when using the oracle price`: {
`Subaccount placed sell order passes collateralization check when using the maker price`: {
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_50PercentInitial_40PercentMaintenance,
},
Expand Down Expand Up @@ -705,6 +702,25 @@ func TestPlaceShortTermOrder(t *testing.T) {
require.NoError(t, err)
}

err = ks.ClobKeeper.InitializeEquityTierLimit(
ctx,
types.EquityTierLimitConfiguration{
ShortTermOrderEquityTiers: []types.EquityTierLimit{
{
UsdTncRequired: dtypes.NewInt(20_000_000),
Limit: 5,
},
},
StatefulOrderEquityTiers: []types.EquityTierLimit{
{
UsdTncRequired: dtypes.NewInt(20_000_000),
Limit: 5,
},
},
},
)
require.NoError(t, err)

// Create all existing orders.
for _, order := range tc.existingOrders {
_, _, err := ks.ClobKeeper.PlaceShortTermOrder(ctx, &types.MsgPlaceOrder{Order: order})
Expand Down

0 comments on commit 52397f9

Please sign in to comment.