-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
[CT-619] deprecate pessimistic add-to-book collateralization check #1107
Changes from 4 commits
844eea0
69c9879
28be1b0
7cb7ad3
6ac1ae7
b733c1a
1473914
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,6 @@ package keeper | |
|
||
import ( | ||
"fmt" | ||
"math" | ||
"math/big" | ||
"time" | ||
|
||
|
@@ -961,7 +960,6 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this ticket isn't relevant anymore right? since we don't use pessimistic oracle price There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. - [ 🤌 |
||
|
@@ -983,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, | ||
|
@@ -1277,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. | ||
// | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this and the test below now fails because of equity tier limit and not collat check |
||
}, | ||
`Subaccount cannot place maker sell order for 1 BTC at 500,000 with 0 collateral`: { | ||
perpetuals: []perptypes.Perpetual{ | ||
|
@@ -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 failed the | ||
jayy04 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
deprecated pessimistic collateralization check with the oracle price`: { | ||
perpetuals: []perptypes.Perpetual{ | ||
constants.BtcUsd_50PercentInitial_40PercentMaintenance, | ||
}, | ||
|
@@ -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{}, | ||
}, | ||
|
@@ -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, | ||
}, | ||
|
@@ -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 failed the | ||
jayy04 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
deprecated pessimistic collateralization check with the oracle price`: { | ||
perpetuals: []perptypes.Perpetual{ | ||
constants.BtcUsd_50PercentInitial_40PercentMaintenance, | ||
}, | ||
|
@@ -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{}, | ||
}, | ||
|
@@ -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, | ||
}, | ||
|
@@ -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}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
give alice some money so that she fails b/c collat check and not equity tier