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.

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

[TRA-81] Check isolated market constraints in UpdateSubaccount. #1158

Merged
merged 14 commits into from
Mar 13, 2024
12 changes: 12 additions & 0 deletions protocol/testutil/constants/perpetuals.go
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,18 @@ var (
},
FundingIndex: dtypes.ZeroInt(),
}
Iso2Usd_IsolatedMarket = perptypes.Perpetual{
Params: perptypes.PerpetualParams{
Id: 4,
Ticker: "ISO2-USD",
MarketId: uint32(4),
AtomicResolution: int32(-7),
DefaultFundingPpm: int32(0),
LiquidityTier: uint32(3),
MarketType: perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED,
},
FundingIndex: dtypes.ZeroInt(),
}
)

var TestMarketPerpetuals = []perptypes.Perpetual{
Expand Down
1 change: 1 addition & 0 deletions protocol/testutil/constants/pricefeed.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ var (
MarketId1 = uint32(1)
MarketId2 = uint32(2)
MarketId3 = uint32(3)
MarketId4 = uint32(4)

MarketId7 = uint32(7)
MarketId8 = uint32(8)
Expand Down
26 changes: 26 additions & 0 deletions protocol/testutil/constants/prices.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const (
SolUsdPair = "SOL-USD"
LtcUsdPair = "LTC-USD"
IsoUsdPair = "ISO-USD"
Iso2UsdPair = "ISO2-USD"

BtcUsdExponent = -5
EthUsdExponent = -6
Expand All @@ -36,6 +37,7 @@ const (
SolUsdExponent = -8
LtcUsdExponent = -7
IsoUsdExponent = -8
Iso2UsdExponent = -7

CoinbaseExchangeName = "Coinbase"
BinanceExchangeName = "Binance"
Expand Down Expand Up @@ -212,6 +214,15 @@ var TestMarketExchangeConfigs = map[pricefeedclient.MarketId]string{
}
]
}`,
exchange_config.MARKET_ISO2_USD: `{
"exchanges": [
{
"exchangeName": "Binance",
"ticker": "ISO2USDT",
"adjustByMarket": "USDT-USD"
}
]
}`,
}

var TestMarketParams = []types.MarketParam{
Expand Down Expand Up @@ -247,6 +258,14 @@ var TestMarketParams = []types.MarketParam{
MinPriceChangePpm: 50,
ExchangeConfigJson: TestMarketExchangeConfigs[exchange_config.MARKET_ISO_USD],
},
{
Id: 4,
Pair: Iso2UsdPair,
Exponent: Iso2UsdExponent,
MinExchanges: 1,
MinPriceChangePpm: 50,
ExchangeConfigJson: TestMarketExchangeConfigs[exchange_config.MARKET_ISO2_USD],
},
}

var TestMarketPrices = []types.MarketPrice{
Expand All @@ -270,13 +289,19 @@ var TestMarketPrices = []types.MarketPrice{
Exponent: IsoUsdExponent,
Price: FiveBillion, // 50$ == 1 ISO
},
{
Id: 4,
Exponent: Iso2UsdExponent,
Price: ThreeBillion, // 300$ == 1 ISO
},
}

var TestMarketIdsToExponents = map[uint32]int32{
0: BtcUsdExponent,
1: EthUsdExponent,
2: SolUsdExponent,
3: IsoUsdExponent,
4: Iso2UsdExponent,
}

var TestPricesGenesisState = types.GenesisState{
Expand All @@ -290,6 +315,7 @@ var (
types.NewMarketPriceUpdate(MarketId1, Price6),
types.NewMarketPriceUpdate(MarketId2, Price7),
types.NewMarketPriceUpdate(MarketId3, Price4),
types.NewMarketPriceUpdate(MarketId4, Price3),
}

// `MsgUpdateMarketPrices`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,9 @@ const (
// MARKET_TEST_USD is the id used for the TEST-USD market pair.
MARKET_TEST_USD types.MarketId = 33

// Arbitrary isolated market
MARKET_ISO_USD types.MarketId = 999_999
// Arbitrary isolated markets
MARKET_ISO2_USD types.MarketId = 999_998
MARKET_ISO_USD types.MarketId = 999_999

// Non-trading markets.
// MARKET_USDT_USD is the id for the USDT-USD market pair.
Expand Down
144 changes: 144 additions & 0 deletions protocol/x/subaccounts/keeper/isolated_subaccount.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
package keeper

import (
errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/lib"
perptypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types"
"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
)

// checkIsolatedSubaccountConstaints will validate all `updates` to the relevant subaccounts against
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding, do we ever expect these constraints to fail during normal operations? If isolated subaccounts is abstracted away from the user, then upstream x/clob logic would be responsible for populating the updates correctly right? So this is more of an internal sanity check?

Copy link
Contributor Author

@vincentwschau vincentwschau Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream x/clob logic relies on UpdateSubaccounts to determine if the updates to a subaccount for a match are valid. This isn't an internal sanity check, this would be the function / logic that would ensure orders which result in a match that would lead to an invalid subaccount state (based on these new constraints from isolated markets) result in an error.
So during "normal" operations, it is possible for a user with an isolated subaccount to place an order for a different perpetual. Only when a match occurs, would the update fail and the order fail.
There is additional non-scoped work to add logic to x/clob to prevent users from even sending orders that would lead to matches that are invalid based on the isolated market constraints.

// isolated subaccount constraints.
// The `updates` have to contain `Subaccounts` with unique `SubaccountIds`.
// The input subaccounts must be settled.
//
// Returns a `success` value of `true` if all updates are valid.
// Returns a `successPerUpdates` value, which is a slice of `UpdateResult`.
// These map to the updates and are used to indicate which of the updates
// caused a failure, if any.
func (k Keeper) checkIsolatedSubaccountConstraints(
ctx sdk.Context,
settledUpdates []settledUpdate,
perpetuals []perptypes.Perpetual,
) (
success bool,
successPerUpdate []types.UpdateResult,
err error,
) {
success = true
successPerUpdate = make([]types.UpdateResult, len(settledUpdates))
var idOfSettledUpdates = make(map[types.SubaccountId]struct{})
var perpIdToMarketType = make(map[uint32]perptypes.PerpetualMarketType)

for _, perpetual := range perpetuals {
perpIdToMarketType[perpetual.GetId()] = perpetual.Params.MarketType
}

for i, u := range settledUpdates {
_, exists := idOfSettledUpdates[*u.SettledSubaccount.Id]

if exists {
return false, nil, types.ErrNonUniqueUpdatesSubaccount
}
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved

result, err := isValidIsolatedPerpetualUpdates(u, perpIdToMarketType)
if err != nil {
return false, nil, err
}
if result != types.Success {
success = false
}

successPerUpdate[i] = result
idOfSettledUpdates[*u.SettledSubaccount.Id] = struct{}{}
}

return success, successPerUpdate, nil
}

// Checks whether the perpetual updates to a settled subaccount violates constraints for isolated
// perpetuals. This function assumes the settled subaccount is valid and does not violate the
// the constraints.
// The constraint being checked is:
// - a subaccount with a position in an isolated perpetual cannot have updates other perpetuals
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved
// - a subaccount with a position in a non-isolated perpetual cannot have updates for isolated
// perpetuals
// - a subaccount with no positions cannot be updated to have positions in multiple isolated
// perpetuals or a combination of isolated and non-isolated perpetuals
func isValidIsolatedPerpetualUpdates(
settledUpdate settledUpdate,
perpIdToMarketType map[uint32]perptypes.PerpetualMarketType,
) (types.UpdateResult, error) {
// If there are no perpetual updates, then this update does not violate constraints for isolated
// markets.
if len(settledUpdate.PerpetualUpdates) == 0 {
return types.Success, nil
}

// Check if the updates contain an update to an isolated perpetual.
hasIsolatedUpdate := false
isolatedUpdatePerpetualId := uint32(0)
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved
for _, perpetualUpdate := range settledUpdate.PerpetualUpdates {
marketType, exists := perpIdToMarketType[perpetualUpdate.PerpetualId]
if !exists {
return types.UpdateCausedError, errorsmod.Wrap(
perptypes.ErrPerpetualDoesNotExist, lib.UintToString(perpetualUpdate.PerpetualId),
)
}

if marketType == perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED {
hasIsolatedUpdate = true
isolatedUpdatePerpetualId = perpetualUpdate.PerpetualId
}
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved
}
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved

// Check if the subaccount has a position in an isolated perpetual.
// Assumes the subaccount itself does not violate the isolated perpetual constraints.
isIsolatedSubaccount := false
isolatedPositionPerpetualId := uint32(0)
hasPerpetualPositions := len(settledUpdate.SettledSubaccount.PerpetualPositions) > 0
for _, perpetualPosition := range settledUpdate.SettledSubaccount.PerpetualPositions {
marketType, exists := perpIdToMarketType[perpetualPosition.PerpetualId]
if !exists {
return types.UpdateCausedError, errorsmod.Wrap(
perptypes.ErrPerpetualDoesNotExist, lib.UintToString(perpetualPosition.PerpetualId),
)
}

if marketType == perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED {
isIsolatedSubaccount = true
isolatedPositionPerpetualId = perpetualPosition.PerpetualId
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved
}
}

// A subaccount with a perpetual position in an isolated perpetual cannot have updates to other
// non-isolated perpetuals.
if isIsolatedSubaccount && !hasIsolatedUpdate {
return types.ViolatesIsolatedSubaccountConstraints, nil
}

// A subaccount with perpetual positions in non-isolated perpetuals cannot have an update
// to an isolated perpetual.
if !isIsolatedSubaccount && hasPerpetualPositions && hasIsolatedUpdate {
return types.ViolatesIsolatedSubaccountConstraints, nil
}

// There cannot be more than a single perpetual update if an update to an isolated perpetual
// exists in the slice of perpetual updates.
if hasIsolatedUpdate && len(settledUpdate.PerpetualUpdates) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible we have multiple perpetual update entries with the same isolated perpetual?

return types.ViolatesIsolatedSubaccountConstraints, nil
}

// Note we can assume that if `hasIsolatedUpdate` is true, there is only a single perpetual
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved
// update for the subaccount, given the above check.
// A subaccount with a perpetual position in an isolated perpetual cannot have an update to
// another isolated perpetual.
if isIsolatedSubaccount &&
hasIsolatedUpdate &&
isolatedPositionPerpetualId != isolatedUpdatePerpetualId {
return types.ViolatesIsolatedSubaccountConstraints, nil
}

return types.Success, nil
}
7 changes: 7 additions & 0 deletions protocol/x/subaccounts/keeper/subaccount.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,13 @@ func (k Keeper) UpdateSubaccounts(
perpIdToFundingIndex[perp.Params.Id] = perp.FundingIndex
}

// Check if the updates satisfy the isolated perpetual constraints.
success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints(
Copy link
Contributor

@teddyding teddyding Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put this in internalCanUpdateSubaccounts? IIRC, we try to maintain the constraint that CanUpdateSubaccount success guarantees UpdateSubaccounts success on the same input updates. This will no longer hold true if the input updates fails checkIsolatedSubaccountConstraints, which is only checked in UpdateSubaccounts

Either way, should probably put this check before the collateralization check logic in internalCanUpdateSubaccounts to short-circuit those expensive checks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CanUpdateSubaccounts has a different constraint from UpdateSubaccounts which is why I decided to pull this function into UpdateSubaccounts and not CanUpdateSubaccounts.
CanUpdateSubaccounts does not require unique subaccounts in updates, but UpdateSubaccounts does.
There may not be a use of this specific behavior of CanUpdateSubaccounts, I can check, however I preferred changing UpdateSubaccounts rather than CanUpdateSubaccounts after seeing this difference between the 2 functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CanUpdateSubaccounts is called in AddOrderToOrderbookCollatCheck
Would this mean that an order violating constraints can potentially be go on the orderbook?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's true that it can be added to the orderbook since AddOrderToOrderbookCollatCheck calls CanUpdateSubaccounts. However this is also a path where having a set of updates with non-unique subaccounts are run through CanUpdateSubaccounts but never through UpdateSubaccounts and just adding the above check to CanUpdateSubaccounts would break a working flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the unique subaccount check in checkIsolatedSubaccountConstraints and moved into internalCanUpdateSubaccounts. Specified in comments that the function does not consider constraints across multiple updates.

ctx, settledUpdates, allPerps)
if !success || err != nil {
return success, successPerUpdate, err
}

// Apply the updates to perpetual positions.
UpdatePerpetualPositions(
settledUpdates,
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading
Loading