From 78eee2698a40f0f67b9f2f1ae8428167c410c436 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Fri, 8 Mar 2024 14:37:09 -0500 Subject: [PATCH 01/13] In progress changes. --- .../subaccounts/keeper/isolated_subaccount.go | 141 ++++++++++++++++++ protocol/x/subaccounts/keeper/subaccount.go | 7 + protocol/x/subaccounts/types/errors.go | 6 + protocol/x/subaccounts/types/update.go | 12 +- protocol/x/subaccounts/types/update_test.go | 6 +- 5 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 protocol/x/subaccounts/keeper/isolated_subaccount.go diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go new file mode 100644 index 0000000000..0bd57456d9 --- /dev/null +++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go @@ -0,0 +1,141 @@ +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 +// 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 + } + + 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: +// - the subaccount after the update cannot have a perpetual position in multiple perpetuals where +// one of the perpetuals is an isolated perpetual +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) + 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 + } + } + + // 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 + } + } + + // 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 { + return types.ViolatesIsolatedSubaccountConstraints, nil + } + + // Note we can assume that if `hasIsolatedUpdate` is true, there is only a single perpetual + // 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 +} diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go index c881a48064..f773d92f33 100644 --- a/protocol/x/subaccounts/keeper/subaccount.go +++ b/protocol/x/subaccounts/keeper/subaccount.go @@ -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( + ctx, settledUpdates, allPerps) + if !success || err != nil { + return success, successPerUpdate, err + } + // Apply the updates to perpetual positions. UpdatePerpetualPositions( settledUpdates, diff --git a/protocol/x/subaccounts/types/errors.go b/protocol/x/subaccounts/types/errors.go index 12af34a0a7..78da2e05a6 100644 --- a/protocol/x/subaccounts/types/errors.go +++ b/protocol/x/subaccounts/types/errors.go @@ -38,4 +38,10 @@ var ( ModuleName, 500, "asset transfer quantums is not positive") ErrAssetTransferThroughBankNotImplemented = errorsmod.Register( ModuleName, 501, "asset transfer (other than USDC) through the bank module is not implemented") + + // 600 - 699: isolated markets related. + ErrFailedToUpdateIsolatedSubaccount = errorsmod.Register( + ModuleName, 600, "failed to update subaccount with perpetual position in isolated market") + ErrMixedIsolatedCrossUpdatesNotSupported = errorsmod.Register( + ModuleName, 601, "updating isolated and cross perpetual positions is not supported") ) diff --git a/protocol/x/subaccounts/types/update.go b/protocol/x/subaccounts/types/update.go index d700bb1541..684b618b28 100644 --- a/protocol/x/subaccounts/types/update.go +++ b/protocol/x/subaccounts/types/update.go @@ -52,11 +52,12 @@ func GetErrorFromUpdateResults( } var updateResultStringMap = map[UpdateResult]string{ - Success: "Success", - NewlyUndercollateralized: "NewlyUndercollateralized", - StillUndercollateralized: "StillUndercollateralized", - WithdrawalsAndTransfersBlocked: "WithdrawalsAndTransfersBlocked", - UpdateCausedError: "UpdateCausedError", + Success: "Success", + NewlyUndercollateralized: "NewlyUndercollateralized", + StillUndercollateralized: "StillUndercollateralized", + WithdrawalsAndTransfersBlocked: "WithdrawalsAndTransfersBlocked", + UpdateCausedError: "UpdateCausedError", + ViolatesIsolatedSubaccountConstraints: "ViolatesIsolatedSubaccountConstraints", } const ( @@ -65,6 +66,7 @@ const ( StillUndercollateralized WithdrawalsAndTransfersBlocked UpdateCausedError + ViolatesIsolatedSubaccountConstraints ) // Update is used by the subaccounts keeper to allow other modules diff --git a/protocol/x/subaccounts/types/update_test.go b/protocol/x/subaccounts/types/update_test.go index 153241de08..c4cd9c5e74 100644 --- a/protocol/x/subaccounts/types/update_test.go +++ b/protocol/x/subaccounts/types/update_test.go @@ -88,8 +88,12 @@ func TestUpdateResultString(t *testing.T) { value: types.UpdateCausedError, expectedResult: "UpdateCausedError", }, + "ViolatesIsolatedSubaccountConstraints": { + value: types.ViolatesIsolatedSubaccountConstraints, + expectedResult: "ViolatesIsolatedSubaccountConstraints", + }, "UnexpectedError": { - value: types.UpdateResult(5), + value: types.UpdateResult(6), expectedResult: "UnexpectedError", }, } From 151a5d78e29042ab0cb7e83c2ff48801882ef811 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Fri, 8 Mar 2024 15:37:15 -0500 Subject: [PATCH 02/13] [TRA-68] Check isolated market constraints in UpdateSubaccount. --- protocol/testutil/constants/perpetuals.go | 12 ++ protocol/testutil/constants/pricefeed.go | 1 + protocol/testutil/constants/prices.go | 26 +++ .../pricefeed/exchange_config/market_id.go | 5 +- .../subaccounts/keeper/isolated_subaccount.go | 7 +- .../x/subaccounts/keeper/subaccount_test.go | 180 ++++++++++++++++++ 6 files changed, 227 insertions(+), 4 deletions(-) diff --git a/protocol/testutil/constants/perpetuals.go b/protocol/testutil/constants/perpetuals.go index 2069fe0e65..89f31f62f1 100644 --- a/protocol/testutil/constants/perpetuals.go +++ b/protocol/testutil/constants/perpetuals.go @@ -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{ diff --git a/protocol/testutil/constants/pricefeed.go b/protocol/testutil/constants/pricefeed.go index 47df2ae63a..e8ae1399c0 100644 --- a/protocol/testutil/constants/pricefeed.go +++ b/protocol/testutil/constants/pricefeed.go @@ -14,6 +14,7 @@ var ( MarketId1 = uint32(1) MarketId2 = uint32(2) MarketId3 = uint32(3) + MarketId4 = uint32(4) MarketId7 = uint32(7) MarketId8 = uint32(8) diff --git a/protocol/testutil/constants/prices.go b/protocol/testutil/constants/prices.go index 022d1793c5..abd04a8fbc 100644 --- a/protocol/testutil/constants/prices.go +++ b/protocol/testutil/constants/prices.go @@ -27,6 +27,7 @@ const ( SolUsdPair = "SOL-USD" LtcUsdPair = "LTC-USD" IsoUsdPair = "ISO-USD" + Iso2UsdPair = "ISO2-USD" BtcUsdExponent = -5 EthUsdExponent = -6 @@ -36,6 +37,7 @@ const ( SolUsdExponent = -8 LtcUsdExponent = -7 IsoUsdExponent = -8 + Iso2UsdExponent = -7 CoinbaseExchangeName = "Coinbase" BinanceExchangeName = "Binance" @@ -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{ @@ -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{ @@ -270,6 +289,11 @@ 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{ @@ -277,6 +301,7 @@ var TestMarketIdsToExponents = map[uint32]int32{ 1: EthUsdExponent, 2: SolUsdExponent, 3: IsoUsdExponent, + 4: Iso2UsdExponent, } var TestPricesGenesisState = types.GenesisState{ @@ -290,6 +315,7 @@ var ( types.NewMarketPriceUpdate(MarketId1, Price6), types.NewMarketPriceUpdate(MarketId2, Price7), types.NewMarketPriceUpdate(MarketId3, Price4), + types.NewMarketPriceUpdate(MarketId4, Price3), } // `MsgUpdateMarketPrices`. diff --git a/protocol/testutil/daemons/pricefeed/exchange_config/market_id.go b/protocol/testutil/daemons/pricefeed/exchange_config/market_id.go index 2123f76b1c..cb6fe6c4d6 100644 --- a/protocol/testutil/daemons/pricefeed/exchange_config/market_id.go +++ b/protocol/testutil/daemons/pricefeed/exchange_config/market_id.go @@ -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. diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go index 0bd57456d9..3f8000abff 100644 --- a/protocol/x/subaccounts/keeper/isolated_subaccount.go +++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go @@ -61,8 +61,11 @@ func (k Keeper) checkIsolatedSubaccountConstraints( // perpetuals. This function assumes the settled subaccount is valid and does not violate the // the constraints. // The constraint being checked is: -// - the subaccount after the update cannot have a perpetual position in multiple perpetuals where -// one of the perpetuals is an isolated perpetual +// - a subaccount with a position in an isolated perpetual cannot have updates other perpetuals +// - 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, diff --git a/protocol/x/subaccounts/keeper/subaccount_test.go b/protocol/x/subaccounts/keeper/subaccount_test.go index cd9e3d32e9..a963c77aff 100644 --- a/protocol/x/subaccounts/keeper/subaccount_test.go +++ b/protocol/x/subaccounts/keeper/subaccount_test.go @@ -2072,6 +2072,186 @@ func TestUpdateSubaccounts(t *testing.T) { }, msgSenderEnabled: true, }, + "Isolated subaccounts - has update for both an isolated perpetual and non-isolated perpetual": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.BtcUsd_NoMarginRequirement, + constants.IsoUsd_IsolatedMarket, + }, + expectedAssetPositions: []*types.AssetPosition{ + { + AssetId: uint32(0), + Quantums: dtypes.NewInt(1_000_000_000_000), + }, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(0), + BigQuantumsDelta: big.NewInt(-100_000_000), // -1 BTC + }, + { + PerpetualId: uint32(3), + BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO + }, + }, + }, + }, + msgSenderEnabled: true, + }, + "Isolated subaccounts - has update for both 2 isolated perpetuals": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.IsoUsd_IsolatedMarket, + constants.Iso2Usd_IsolatedMarket, + }, + expectedAssetPositions: []*types.AssetPosition{ + { + AssetId: uint32(0), + Quantums: dtypes.NewInt(1_000_000_000_000), + }, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(3), + BigQuantumsDelta: big.NewInt(-1_000_000_000), // 1 ISO + }, + { + PerpetualId: uint32(4), + BigQuantumsDelta: big.NewInt(10_000_000), // 1 ISO2 + }, + }, + }, + }, + msgSenderEnabled: true, + }, + "Isolated subaccounts - subaccount with isolated perpetual position has update for non-isolated perpetual": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.BtcUsd_NoMarginRequirement, + constants.IsoUsd_IsolatedMarket, + }, + perpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(3), + Quantums: dtypes.NewInt(1_000_000_000), // 1 ISO + FundingIndex: dtypes.NewInt(0), + }, + }, + expectedPerpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(3), + Quantums: dtypes.NewInt(1_000_000_000), + FundingIndex: dtypes.NewInt(0), + }, + }, + expectedAssetPositions: []*types.AssetPosition{ + { + AssetId: uint32(0), + Quantums: dtypes.NewInt(1_000_000_000_000), + }, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(0), + BigQuantumsDelta: big.NewInt(-100_000_000), // -1 BTC + }, + }, + }, + }, + msgSenderEnabled: true, + }, + "Isolated subaccounts - subaccount with isolated perpetual position has update for another isolated perpetual": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.IsoUsd_IsolatedMarket, + constants.Iso2Usd_IsolatedMarket, + }, + perpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(3), + Quantums: dtypes.NewInt(1_000_000_000), // 1 ISO + FundingIndex: dtypes.NewInt(0), + }, + }, + expectedPerpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(3), + Quantums: dtypes.NewInt(1_000_000_000), + FundingIndex: dtypes.NewInt(0), + }, + }, + expectedAssetPositions: []*types.AssetPosition{ + { + AssetId: uint32(0), + Quantums: dtypes.NewInt(1_000_000_000_000), + }, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(4), + BigQuantumsDelta: big.NewInt(-10_000_000), // -1 ISO2 + }, + }, + }, + }, + msgSenderEnabled: true, + }, + "Isolated subaccounts - subaccount with non-isolated perpetual position has update for isolated perpetual": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.BtcUsd_NoMarginRequirement, + constants.IsoUsd_IsolatedMarket, + }, + perpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(0), + Quantums: dtypes.NewInt(100_000_000), // 1 BTC + FundingIndex: dtypes.NewInt(0), + }, + }, + expectedPerpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(0), + Quantums: dtypes.NewInt(100_000_000), + FundingIndex: dtypes.NewInt(0), + }, + }, + expectedAssetPositions: []*types.AssetPosition{ + { + AssetId: uint32(0), + Quantums: dtypes.NewInt(1_000_000_000_000), + }, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(3), + BigQuantumsDelta: big.NewInt(-1_000_000_000), // -1 ISO + }, + }, + }, + }, + msgSenderEnabled: true, + }, } for name, tc := range tests { From 9d996f90b6fe17f6943ead117e954c8bfb4abf71 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Fri, 8 Mar 2024 15:57:23 -0500 Subject: [PATCH 03/13] Fix price feed. --- protocol/testutil/constants/pricefeed.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/protocol/testutil/constants/pricefeed.go b/protocol/testutil/constants/pricefeed.go index e8ae1399c0..f7bdf0e2c2 100644 --- a/protocol/testutil/constants/pricefeed.go +++ b/protocol/testutil/constants/pricefeed.go @@ -277,12 +277,19 @@ var ( Exchange3_Price3_TimeT, }, }, + { + MarketId: MarketId4, + ExchangePrices: []*api.ExchangePrice{ + Exchange3_Price3_TimeT, + }, + }, } AtTimeTSingleExchangeSmoothedPrices = map[uint32]uint64{ MarketId0: Exchange0_Price4_TimeT.Price, MarketId1: Exchange1_Price1_TimeT.Price, MarketId2: Exchange2_Price2_TimeT.Price, MarketId3: Exchange3_Price3_TimeT.Price, + MarketId4: Exchange3_Price3_TimeT.Price, } AtTimeTSingleExchangeSmoothedPricesPlus10 = map[uint32]uint64{ @@ -290,6 +297,7 @@ var ( MarketId1: Exchange1_Price1_TimeT.Price + 10, MarketId2: Exchange2_Price2_TimeT.Price + 10, MarketId3: Exchange3_Price3_TimeT.Price + 10, + MarketId4: Exchange3_Price3_TimeT.Price + 10, } AtTimeTSingleExchangeSmoothedPricesPlus7 = map[uint32]uint64{ @@ -297,6 +305,7 @@ var ( MarketId1: Exchange1_Price1_TimeT.Price + 7, MarketId2: Exchange2_Price2_TimeT.Price + 7, MarketId3: Exchange3_Price3_TimeT.Price + 7, + MarketId4: Exchange3_Price3_TimeT.Price + 7, } MixedTimePriceUpdate = []*api.MarketPriceUpdate{ From 104e3b3b6a9b5be198aa002d9cc4ddfa4aeeacf9 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Fri, 8 Mar 2024 15:59:14 -0500 Subject: [PATCH 04/13] Remove uneeded errors. --- protocol/x/subaccounts/types/errors.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/protocol/x/subaccounts/types/errors.go b/protocol/x/subaccounts/types/errors.go index 78da2e05a6..12af34a0a7 100644 --- a/protocol/x/subaccounts/types/errors.go +++ b/protocol/x/subaccounts/types/errors.go @@ -38,10 +38,4 @@ var ( ModuleName, 500, "asset transfer quantums is not positive") ErrAssetTransferThroughBankNotImplemented = errorsmod.Register( ModuleName, 501, "asset transfer (other than USDC) through the bank module is not implemented") - - // 600 - 699: isolated markets related. - ErrFailedToUpdateIsolatedSubaccount = errorsmod.Register( - ModuleName, 600, "failed to update subaccount with perpetual position in isolated market") - ErrMixedIsolatedCrossUpdatesNotSupported = errorsmod.Register( - ModuleName, 601, "updating isolated and cross perpetual positions is not supported") ) From 81234152d70c1edaea0396983aec8a0354c50631 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Fri, 8 Mar 2024 17:07:57 -0500 Subject: [PATCH 05/13] Fix price feed tests. --- protocol/testutil/constants/prices.go | 2 +- .../keeper/msg_server_update_market_prices_test.go | 11 +++++++++++ protocol/x/prices/keeper/smoothed_price_test.go | 5 ++++- protocol/x/prices/keeper/update_price_test.go | 3 +++ .../keeper/validate_market_price_updates_test.go | 5 +++-- 5 files changed, 22 insertions(+), 4 deletions(-) diff --git a/protocol/testutil/constants/prices.go b/protocol/testutil/constants/prices.go index abd04a8fbc..89c7ee360f 100644 --- a/protocol/testutil/constants/prices.go +++ b/protocol/testutil/constants/prices.go @@ -292,7 +292,7 @@ var TestMarketPrices = []types.MarketPrice{ { Id: 4, Exponent: Iso2UsdExponent, - Price: ThreeBillion, // 300$ == 1 ISO + Price: ThreeBillion, // 300$ == 1 ISO2 }, } diff --git a/protocol/x/prices/keeper/msg_server_update_market_prices_test.go b/protocol/x/prices/keeper/msg_server_update_market_prices_test.go index 145d363cbe..ebbf453d33 100644 --- a/protocol/x/prices/keeper/msg_server_update_market_prices_test.go +++ b/protocol/x/prices/keeper/msg_server_update_market_prices_test.go @@ -48,6 +48,7 @@ func TestUpdateMarketPrices_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, "Multiple updates": { @@ -58,6 +59,7 @@ func TestUpdateMarketPrices_Valid(t *testing.T) { constants.MarketId1: constants.Price6, constants.MarketId2: constants.Price7, constants.MarketId3: constants.Price4, + constants.MarketId4: constants.Price3, }, }, "Towards index price = true (current < update < index price)": { @@ -81,6 +83,7 @@ func TestUpdateMarketPrices_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, "Index price crossing = true (price increase), old_ticks > 1, new_ticks <= sqrt(old_ticks) = true": { @@ -104,6 +107,7 @@ func TestUpdateMarketPrices_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, "Index price crossing = true (price decrease), old_ticks > 1, new_ticks <= sqrt(old_ticks) = true": { @@ -127,6 +131,7 @@ func TestUpdateMarketPrices_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, "Index price crossing = true (price increase), old_ticks <= 1, new_ticks <= old_ticks = true": { @@ -150,6 +155,7 @@ func TestUpdateMarketPrices_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, "Index price crossing = true (price decrease), old_ticks <= 1, new_ticks <= old_ticks = true": { @@ -173,6 +179,7 @@ func TestUpdateMarketPrices_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, } @@ -226,6 +233,7 @@ func TestUpdateMarketPrices_SkipNonDeterministicCheck_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, "Index price trends in the opposite direction of update price from current price, but still updates state": { @@ -249,6 +257,7 @@ func TestUpdateMarketPrices_SkipNonDeterministicCheck_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, "Index price crossing = true, old_ticks > 1, new_ticks <= sqrt(old_ticks) = false": { @@ -272,6 +281,7 @@ func TestUpdateMarketPrices_SkipNonDeterministicCheck_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, "Index price crossing = true, old_ticks <= 1, new_ticks <= old_ticks = false": { @@ -295,6 +305,7 @@ func TestUpdateMarketPrices_SkipNonDeterministicCheck_Valid(t *testing.T) { constants.MarketId1: constants.ThreeBillion, // no change constants.MarketId2: constants.FiveBillion, // no change constants.MarketId3: constants.FiveBillion, // no change + constants.MarketId4: constants.ThreeBillion, // no change }, }, } diff --git a/protocol/x/prices/keeper/smoothed_price_test.go b/protocol/x/prices/keeper/smoothed_price_test.go index a8ce09400e..93237a2c13 100644 --- a/protocol/x/prices/keeper/smoothed_price_test.go +++ b/protocol/x/prices/keeper/smoothed_price_test.go @@ -59,6 +59,7 @@ func TestUpdateSmoothedPrices(t *testing.T) { constants.MarketId1: constants.Exchange1_Price1_TimeT.Price + 7, constants.MarketId2: constants.Exchange2_Price2_TimeT.Price + 35, constants.MarketId3: constants.Exchange3_Price3_TimeT.Price, + constants.MarketId4: constants.Exchange3_Price3_TimeT.Price, constants.MarketId7: constants.Price1, }, linearInterpolateFunc: lib.Uint64LinearInterpolate, @@ -81,7 +82,8 @@ func TestUpdateSmoothedPrices(t *testing.T) { expectedErr: "Error updating smoothed price for market 0: error while interpolating\n" + "Error updating smoothed price for market 1: error while interpolating\n" + "Error updating smoothed price for market 2: error while interpolating\n" + - "Error updating smoothed price for market 3: error while interpolating", + "Error updating smoothed price for market 3: error while interpolating\n" + + "Error updating smoothed price for market 4: error while interpolating", expectedResult: constants.AtTimeTSingleExchangeSmoothedPricesPlus10, // no change }, "Single interpolation error - returns error, continues updating other markets": { @@ -95,6 +97,7 @@ func TestUpdateSmoothedPrices(t *testing.T) { constants.MarketId1: constants.AtTimeTSingleExchangeSmoothedPricesPlus10[constants.MarketId1], // no change constants.MarketId2: constants.AtTimeTSingleExchangeSmoothedPricesPlus7[constants.MarketId2], // update constants.MarketId3: constants.AtTimeTSingleExchangeSmoothedPricesPlus10[constants.MarketId3], // update + constants.MarketId4: constants.AtTimeTSingleExchangeSmoothedPricesPlus7[constants.MarketId4], // update }, // no change }, } diff --git a/protocol/x/prices/keeper/update_price_test.go b/protocol/x/prices/keeper/update_price_test.go index c339289509..6c61b89ec9 100644 --- a/protocol/x/prices/keeper/update_price_test.go +++ b/protocol/x/prices/keeper/update_price_test.go @@ -201,6 +201,7 @@ func TestGetValidMarketPriceUpdates(t *testing.T) { types.NewMarketPriceUpdate(constants.MarketId1, constants.Price1+1), types.NewMarketPriceUpdate(constants.MarketId2, constants.Price2), types.NewMarketPriceUpdate(constants.MarketId3, constants.Price3), + types.NewMarketPriceUpdate(constants.MarketId4, constants.Price3), }, }, }, @@ -232,6 +233,7 @@ func TestGetValidMarketPriceUpdates(t *testing.T) { types.NewMarketPriceUpdate(constants.MarketId1, constants.Price1), types.NewMarketPriceUpdate(constants.MarketId2, constants.Price2), types.NewMarketPriceUpdate(constants.MarketId3, constants.Price3), + types.NewMarketPriceUpdate(constants.MarketId4, constants.Price3), }, }, }, @@ -250,6 +252,7 @@ func TestGetValidMarketPriceUpdates(t *testing.T) { types.NewMarketPriceUpdate(constants.MarketId1, constants.Price1), types.NewMarketPriceUpdate(constants.MarketId2, constants.Price2), types.NewMarketPriceUpdate(constants.MarketId3, constants.Price3), + types.NewMarketPriceUpdate(constants.MarketId4, constants.Price3), }, }, }, diff --git a/protocol/x/prices/keeper/validate_market_price_updates_test.go b/protocol/x/prices/keeper/validate_market_price_updates_test.go index 58e44ef5ff..10b6b92aca 100644 --- a/protocol/x/prices/keeper/validate_market_price_updates_test.go +++ b/protocol/x/prices/keeper/validate_market_price_updates_test.go @@ -374,7 +374,7 @@ func TestGetMarketsMissingFromPriceUpdates(t *testing.T) { smoothedIndexPrices: constants.AtTimeTSingleExchangeSmoothedPrices, // The returned market ids must be sorted. expectedMarketIds: []uint32{ - constants.MarketId0, constants.MarketId1, constants.MarketId2, constants.MarketId3, + constants.MarketId0, constants.MarketId1, constants.MarketId2, constants.MarketId3, constants.MarketId4, }, }, "Non-empty proposed updates, Empty local updates": { @@ -392,6 +392,7 @@ func TestGetMarketsMissingFromPriceUpdates(t *testing.T) { types.NewMarketPriceUpdate(constants.MarketId0, constants.Price5), types.NewMarketPriceUpdate(constants.MarketId1, constants.Price6), types.NewMarketPriceUpdate(constants.MarketId3, constants.Price7), + types.NewMarketPriceUpdate(constants.MarketId4, constants.Price4), }, indexPrices: constants.AtTimeTSingleExchangePriceUpdate, smoothedIndexPrices: constants.AtTimeTSingleExchangeSmoothedPrices, @@ -404,7 +405,7 @@ func TestGetMarketsMissingFromPriceUpdates(t *testing.T) { indexPrices: constants.AtTimeTSingleExchangePriceUpdate, smoothedIndexPrices: constants.AtTimeTSingleExchangeSmoothedPrices, // The returned market ids must be sorted. - expectedMarketIds: []uint32{constants.MarketId0, constants.MarketId2, constants.MarketId3}, + expectedMarketIds: []uint32{constants.MarketId0, constants.MarketId2, constants.MarketId3, constants.MarketId4}, }, } for name, tc := range tests { From 603f4708b2b3606d411c9c293a717a7a7375176f Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Fri, 8 Mar 2024 17:26:01 -0500 Subject: [PATCH 06/13] Fix daemon tests. --- protocol/daemons/liquidation/client/grpc_helper_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/protocol/daemons/liquidation/client/grpc_helper_test.go b/protocol/daemons/liquidation/client/grpc_helper_test.go index 2d164072ad..1b25ff48b8 100644 --- a/protocol/daemons/liquidation/client/grpc_helper_test.go +++ b/protocol/daemons/liquidation/client/grpc_helper_test.go @@ -411,6 +411,7 @@ func TestGetAllMarketPrices(t *testing.T) { MarketPrices: []pricestypes.MarketPrice{ constants.TestMarketPrices[2], constants.TestMarketPrices[3], + constants.TestMarketPrices[4], }, } mck.On("AllMarketPrices", mock.Anything, req2).Return(response2, nil) From d51033fe799e5ea0de3e46ae92e5211bacaa9ca4 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Mon, 11 Mar 2024 12:08:19 -0400 Subject: [PATCH 07/13] Fix nits. --- protocol/x/subaccounts/keeper/isolated_subaccount.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go index 3f8000abff..0424934aea 100644 --- a/protocol/x/subaccounts/keeper/isolated_subaccount.go +++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go @@ -36,9 +36,7 @@ func (k Keeper) checkIsolatedSubaccountConstraints( } for i, u := range settledUpdates { - _, exists := idOfSettledUpdates[*u.SettledSubaccount.Id] - - if exists { + if _, exists := idOfSettledUpdates[*u.SettledSubaccount.Id]; exists { return false, nil, types.ErrNonUniqueUpdatesSubaccount } @@ -61,7 +59,8 @@ func (k Keeper) checkIsolatedSubaccountConstraints( // 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 +// - a subaccount with a position in an isolated perpetual cannot have updates for other +// perpetuals // - 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 @@ -90,6 +89,7 @@ func isValidIsolatedPerpetualUpdates( if marketType == perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED { hasIsolatedUpdate = true isolatedUpdatePerpetualId = perpetualUpdate.PerpetualId + break } } @@ -109,6 +109,7 @@ func isValidIsolatedPerpetualUpdates( if marketType == perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED { isIsolatedSubaccount = true isolatedPositionPerpetualId = perpetualPosition.PerpetualId + break } } @@ -130,7 +131,7 @@ func isValidIsolatedPerpetualUpdates( return types.ViolatesIsolatedSubaccountConstraints, nil } - // Note we can assume that if `hasIsolatedUpdate` is true, there is only a single perpetual + // Note we can assume that if `hasIsolatedUpdate` is true, there is only a single perpetual // 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. From f4a7413c8f731c0b774d6933892d9e4c8babdfec Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Tue, 12 Mar 2024 16:24:21 -0400 Subject: [PATCH 08/13] Update CanUpdateSubaccounts as well. --- .../subaccounts/keeper/isolated_subaccount.go | 14 +- protocol/x/subaccounts/keeper/subaccount.go | 32 +++-- .../x/subaccounts/keeper/subaccount_test.go | 124 ++++++++++++++++++ 3 files changed, 157 insertions(+), 13 deletions(-) diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go index 0424934aea..50798a3805 100644 --- a/protocol/x/subaccounts/keeper/isolated_subaccount.go +++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go @@ -1,6 +1,8 @@ package keeper import ( + "math" + errorsmod "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/dydxprotocol/v4-chain/protocol/lib" @@ -10,7 +12,6 @@ import ( // checkIsolatedSubaccountConstaints will validate all `updates` to the relevant subaccounts against // 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. @@ -21,6 +22,7 @@ func (k Keeper) checkIsolatedSubaccountConstraints( ctx sdk.Context, settledUpdates []settledUpdate, perpetuals []perptypes.Perpetual, + uniqueSubaccounts bool, ) ( success bool, successPerUpdate []types.UpdateResult, @@ -36,8 +38,10 @@ func (k Keeper) checkIsolatedSubaccountConstraints( } for i, u := range settledUpdates { - if _, exists := idOfSettledUpdates[*u.SettledSubaccount.Id]; exists { - return false, nil, types.ErrNonUniqueUpdatesSubaccount + if uniqueSubaccounts { + if _, exists := idOfSettledUpdates[*u.SettledSubaccount.Id]; exists { + return false, nil, types.ErrNonUniqueUpdatesSubaccount + } } result, err := isValidIsolatedPerpetualUpdates(u, perpIdToMarketType) @@ -77,7 +81,7 @@ func isValidIsolatedPerpetualUpdates( // Check if the updates contain an update to an isolated perpetual. hasIsolatedUpdate := false - isolatedUpdatePerpetualId := uint32(0) + isolatedUpdatePerpetualId := uint32(math.MaxUint32) for _, perpetualUpdate := range settledUpdate.PerpetualUpdates { marketType, exists := perpIdToMarketType[perpetualUpdate.PerpetualId] if !exists { @@ -96,7 +100,7 @@ func isValidIsolatedPerpetualUpdates( // 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) + isolatedPositionPerpetualId := uint32(math.MaxUint32) hasPerpetualPositions := len(settledUpdate.SettledSubaccount.PerpetualPositions) > 0 for _, perpetualPosition := range settledUpdate.SettledSubaccount.PerpetualPositions { marketType, exists := perpIdToMarketType[perpetualPosition.PerpetualId] diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go index f773d92f33..8df5cb0388 100644 --- a/protocol/x/subaccounts/keeper/subaccount.go +++ b/protocol/x/subaccounts/keeper/subaccount.go @@ -280,25 +280,29 @@ func (k Keeper) UpdateSubaccounts( return false, nil, err } + // Check if the updates satisfy the isolated perpetual constraints. + allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) + success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( + ctx, + settledUpdates, + allPerps, + true, // uniqueSubaccounts + ) + if !success || err != nil { + return success, successPerUpdate, err + } + success, successPerUpdate, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType) if !success || err != nil { return success, successPerUpdate, err } // Get a mapping from perpetual Id to current perpetual funding index. - allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) perpIdToFundingIndex := make(map[uint32]dtypes.SerializableInt) for _, perp := range allPerps { perpIdToFundingIndex[perp.Params.Id] = perp.FundingIndex } - // Check if the updates satisfy the isolated perpetual constraints. - success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( - ctx, settledUpdates, allPerps) - if !success || err != nil { - return success, successPerUpdate, err - } - // Apply the updates to perpetual positions. UpdatePerpetualPositions( settledUpdates, @@ -385,6 +389,18 @@ func (k Keeper) CanUpdateSubaccounts( return false, nil, err } + // Check if the updates satisfy the isolated perpetual constraints. + allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) + success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( + ctx, + settledUpdates, + allPerps, + false, // uniqueSubaccounts + ) + if !success || err != nil { + return success, successPerUpdate, err + } + return k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType) } diff --git a/protocol/x/subaccounts/keeper/subaccount_test.go b/protocol/x/subaccounts/keeper/subaccount_test.go index a963c77aff..49fcaab685 100644 --- a/protocol/x/subaccounts/keeper/subaccount_test.go +++ b/protocol/x/subaccounts/keeper/subaccount_test.go @@ -3638,6 +3638,130 @@ func TestCanUpdateSubaccounts(t *testing.T) { }, }, }, + "Isolated subaccounts - has update for both an isolated perpetual and non-isolated perpetual": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.BtcUsd_NoMarginRequirement, + constants.IsoUsd_IsolatedMarket, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(0), + BigQuantumsDelta: big.NewInt(-100_000_000), // -1 BTC + }, + { + PerpetualId: uint32(3), + BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO + }, + }, + }, + }, + }, + "Isolated subaccounts - has update for both 2 isolated perpetuals": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.IsoUsd_IsolatedMarket, + constants.Iso2Usd_IsolatedMarket, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(3), + BigQuantumsDelta: big.NewInt(-1_000_000_000), // 1 ISO + }, + { + PerpetualId: uint32(4), + BigQuantumsDelta: big.NewInt(10_000_000), // 1 ISO2 + }, + }, + }, + }, + }, + "Isolated subaccounts - subaccount with isolated perpetual position has update for non-isolated perpetual": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.BtcUsd_NoMarginRequirement, + constants.IsoUsd_IsolatedMarket, + }, + perpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(3), + Quantums: dtypes.NewInt(1_000_000_000), // 1 ISO + FundingIndex: dtypes.NewInt(0), + }, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(0), + BigQuantumsDelta: big.NewInt(-100_000_000), // -1 BTC + }, + }, + }, + }, + }, + "Isolated subaccounts - subaccount with isolated perpetual position has update for another isolated perpetual": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.IsoUsd_IsolatedMarket, + constants.Iso2Usd_IsolatedMarket, + }, + perpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(3), + Quantums: dtypes.NewInt(1_000_000_000), // 1 ISO + FundingIndex: dtypes.NewInt(0), + }, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(4), + BigQuantumsDelta: big.NewInt(-10_000_000), // -1 ISO2 + }, + }, + }, + }, + }, + "Isolated subaccounts - subaccount with non-isolated perpetual position has update for isolated perpetual": { + assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)), + expectedSuccess: false, + expectedSuccessPerUpdate: []types.UpdateResult{types.ViolatesIsolatedSubaccountConstraints}, + perpetuals: []perptypes.Perpetual{ + constants.BtcUsd_NoMarginRequirement, + constants.IsoUsd_IsolatedMarket, + }, + perpetualPositions: []*types.PerpetualPosition{ + { + PerpetualId: uint32(0), + Quantums: dtypes.NewInt(100_000_000), // 1 BTC + FundingIndex: dtypes.NewInt(0), + }, + }, + updates: []types.Update{ + { + PerpetualUpdates: []types.PerpetualUpdate{ + { + PerpetualId: uint32(3), + BigQuantumsDelta: big.NewInt(-1_000_000_000), // -1 ISO + }, + }, + }, + }, + }, } for name, tc := range tests { From 733c4c3024bdb721e71bab5549ffbc7cca714d1b Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Tue, 12 Mar 2024 16:43:46 -0400 Subject: [PATCH 09/13] Fix comment. --- .../subaccounts/keeper/isolated_subaccount.go | 11 +----- protocol/x/subaccounts/keeper/subaccount.go | 38 +++++++++---------- 2 files changed, 20 insertions(+), 29 deletions(-) diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go index 50798a3805..9dd3006a1c 100644 --- a/protocol/x/subaccounts/keeper/isolated_subaccount.go +++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go @@ -12,6 +12,8 @@ import ( // checkIsolatedSubaccountConstaints will validate all `updates` to the relevant subaccounts against // isolated subaccount constraints. +// This function checks each update in isolation, so if multiple updates for the same subaccount id +// are passed in, they are evaluated together // The input subaccounts must be settled. // // Returns a `success` value of `true` if all updates are valid. @@ -22,7 +24,6 @@ func (k Keeper) checkIsolatedSubaccountConstraints( ctx sdk.Context, settledUpdates []settledUpdate, perpetuals []perptypes.Perpetual, - uniqueSubaccounts bool, ) ( success bool, successPerUpdate []types.UpdateResult, @@ -30,7 +31,6 @@ func (k Keeper) checkIsolatedSubaccountConstraints( ) { 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 { @@ -38,12 +38,6 @@ func (k Keeper) checkIsolatedSubaccountConstraints( } for i, u := range settledUpdates { - if uniqueSubaccounts { - if _, exists := idOfSettledUpdates[*u.SettledSubaccount.Id]; exists { - return false, nil, types.ErrNonUniqueUpdatesSubaccount - } - } - result, err := isValidIsolatedPerpetualUpdates(u, perpIdToMarketType) if err != nil { return false, nil, err @@ -53,7 +47,6 @@ func (k Keeper) checkIsolatedSubaccountConstraints( } successPerUpdate[i] = result - idOfSettledUpdates[*u.SettledSubaccount.Id] = struct{}{} } return success, successPerUpdate, nil diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go index 8df5cb0388..6a2e56b2c6 100644 --- a/protocol/x/subaccounts/keeper/subaccount.go +++ b/protocol/x/subaccounts/keeper/subaccount.go @@ -280,23 +280,17 @@ func (k Keeper) UpdateSubaccounts( return false, nil, err } - // Check if the updates satisfy the isolated perpetual constraints. allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) - success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( + success, successPerUpdate, err = k.internalCanUpdateSubaccounts( ctx, settledUpdates, + updateType, allPerps, - true, // uniqueSubaccounts ) if !success || err != nil { return success, successPerUpdate, err } - success, successPerUpdate, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType) - if !success || err != nil { - return success, successPerUpdate, err - } - // Get a mapping from perpetual Id to current perpetual funding index. perpIdToFundingIndex := make(map[uint32]dtypes.SerializableInt) for _, perp := range allPerps { @@ -389,19 +383,8 @@ func (k Keeper) CanUpdateSubaccounts( return false, nil, err } - // Check if the updates satisfy the isolated perpetual constraints. allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) - success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( - ctx, - settledUpdates, - allPerps, - false, // uniqueSubaccounts - ) - if !success || err != nil { - return success, successPerUpdate, err - } - - return k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType) + return k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, allPerps) } // getSettledSubaccount returns 1. a new settled subaccount given an unsettled subaccount, @@ -534,6 +517,7 @@ func checkPositionUpdatable( // internalCanUpdateSubaccounts will validate all `updates` to the relevant subaccounts. // The `updates` do not have to contain `Subaccounts` with unique `SubaccountIds`. +// The `updates` do not have to contain `Subaccounts` with unique `SubaccountIds`. // Each update is considered in isolation. Thus if two updates are provided // with the same `Subaccount`, they are validated without respect to each // other. @@ -547,6 +531,7 @@ func (k Keeper) internalCanUpdateSubaccounts( ctx sdk.Context, settledUpdates []settledUpdate, updateType types.UpdateType, + perpetuals []perptypes.Perpetual, ) ( success bool, successPerUpdate []types.UpdateResult, @@ -555,6 +540,19 @@ func (k Keeper) internalCanUpdateSubaccounts( success = true successPerUpdate = make([]types.UpdateResult, len(settledUpdates)) + // Check if the updates satisfy the isolated perpetual constraints. + success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( + ctx, + settledUpdates, + perpetuals, + ) + if err != nil { + return false, nil, err + } + if !success { + return success, successPerUpdate, nil + } + // Block all withdrawals and transfers if either of the following is true within the last // `WITHDRAWAL_AND_TRANSFERS_BLOCKED_AFTER_NEGATIVE_TNC_SUBACCOUNT_SEEN_BLOCKS`: // - There was a negative TNC subaccount seen. From 3b5287350140dd8370324e6f0a96a71a46027e16 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Tue, 12 Mar 2024 16:50:40 -0400 Subject: [PATCH 10/13] Fix lint. --- protocol/x/subaccounts/keeper/subaccount.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go index 6a2e56b2c6..73a90d577a 100644 --- a/protocol/x/subaccounts/keeper/subaccount.go +++ b/protocol/x/subaccounts/keeper/subaccount.go @@ -537,9 +537,6 @@ func (k Keeper) internalCanUpdateSubaccounts( successPerUpdate []types.UpdateResult, err error, ) { - success = true - successPerUpdate = make([]types.UpdateResult, len(settledUpdates)) - // Check if the updates satisfy the isolated perpetual constraints. success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( ctx, From ea0985bd6bb5dab712afbb7b79abeb0f4cb97aa7 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Tue, 12 Mar 2024 16:54:47 -0400 Subject: [PATCH 11/13] Add TODO. --- protocol/x/subaccounts/keeper/subaccount.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go index 73a90d577a..a819333cea 100644 --- a/protocol/x/subaccounts/keeper/subaccount.go +++ b/protocol/x/subaccounts/keeper/subaccount.go @@ -537,6 +537,8 @@ func (k Keeper) internalCanUpdateSubaccounts( successPerUpdate []types.UpdateResult, err error, ) { + // TODO(TRA-99): Add integration / E2E tests on order placement / matching with this new + // constraint. // Check if the updates satisfy the isolated perpetual constraints. success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints( ctx, From 124bc3f9b7b33b664c6aadb02b28f7bef1f786a2 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Wed, 13 Mar 2024 11:28:54 -0400 Subject: [PATCH 12/13] Increase gas limit in E2E test for transfers. --- protocol/app/e2e/ante_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocol/app/e2e/ante_test.go b/protocol/app/e2e/ante_test.go index 3cb7ec9fa7..815952064d 100644 --- a/protocol/app/e2e/ante_test.go +++ b/protocol/app/e2e/ante_test.go @@ -137,7 +137,7 @@ func TestParallelAnteHandler_ClobAndOther(t *testing.T) { }, }, constants.TestFeeCoins_5Cents, - 100_000, + 110_000, ctx.ChainID(), []uint64{account.GetAccountNumber()}, []uint64{sequenceNumber}, From a0ed3da12196b2ecb42c94f76c136b5f5f707783 Mon Sep 17 00:00:00 2001 From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com> Date: Wed, 13 Mar 2024 12:28:08 -0400 Subject: [PATCH 13/13] Fix comments. --- protocol/x/subaccounts/keeper/isolated_subaccount.go | 2 +- protocol/x/subaccounts/keeper/subaccount.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go index 9dd3006a1c..e5547d3986 100644 --- a/protocol/x/subaccounts/keeper/isolated_subaccount.go +++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go @@ -13,7 +13,7 @@ import ( // checkIsolatedSubaccountConstaints will validate all `updates` to the relevant subaccounts against // isolated subaccount constraints. // This function checks each update in isolation, so if multiple updates for the same subaccount id -// are passed in, they are evaluated together +// are passed in, they are not evaluated separately. // The input subaccounts must be settled. // // Returns a `success` value of `true` if all updates are valid. diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go index a819333cea..ca9e978e80 100644 --- a/protocol/x/subaccounts/keeper/subaccount.go +++ b/protocol/x/subaccounts/keeper/subaccount.go @@ -517,7 +517,6 @@ func checkPositionUpdatable( // internalCanUpdateSubaccounts will validate all `updates` to the relevant subaccounts. // The `updates` do not have to contain `Subaccounts` with unique `SubaccountIds`. -// The `updates` do not have to contain `Subaccounts` with unique `SubaccountIds`. // Each update is considered in isolation. Thus if two updates are provided // with the same `Subaccount`, they are validated without respect to each // other.