From 36f47ace0025419a52e0dffaa3ac30290d877ac5 Mon Sep 17 00:00:00 2001
From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com>
Date: Tue, 19 Mar 2024 02:33:37 -0600
Subject: [PATCH 1/6] [TRA-139] Add logic to transfer collateral when
 open/close isolated perpetual position.

---
 .../subaccounts/keeper/isolated_subaccount.go | 193 ++++++++++++--
 protocol/x/subaccounts/keeper/subaccount.go   |  49 +++-
 .../x/subaccounts/keeper/subaccount_test.go   | 246 +++++++++++++++++-
 3 files changed, 451 insertions(+), 37 deletions(-)

diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go
index e5547d3986..8c7303949b 100644
--- a/protocol/x/subaccounts/keeper/isolated_subaccount.go
+++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go
@@ -2,16 +2,35 @@ package keeper
 
 import (
 	"math"
+	"math/big"
 
 	errorsmod "cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/dydxprotocol/v4-chain/protocol/lib"
+	assettypes "github.com/dydxprotocol/v4-chain/protocol/x/assets/types"
 	perptypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types"
 	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
 )
 
+type positionStateTransition uint
+
+const (
+	opened positionStateTransition = iota
+	closed
+)
+
+// Represents a state transition for an isolated perpetual.
+type isolatedPerpetualStateTranisition struct {
+	perpetualId uint32
+	// TODO(DEC-715): Support non-USDC assets.
+	// USDC position size of the subaccount that has a state change for an isolated perpetual.
+	usdcQuantumsBeforeUpdate *big.Int
+	transition               positionStateTransition
+}
+
 // checkIsolatedSubaccountConstaints will validate all `updates` to the relevant subaccounts against
-// isolated subaccount constraints.
+// isolated subaccount constraints and computes whether each update leads to a state change
+// (open / close) to an isolated perpetual occurred due to the updates if they are valid.
 // This function checks each update in isolation, so if multiple updates for the same subaccount id
 // are passed in, they are not evaluated separately.
 // The input subaccounts must be settled.
@@ -20,6 +39,10 @@ import (
 // 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.
+// Returns a `isolatedPerpetualStateTransitions` value, which is a slice of
+// `isolatedPerpetualStateTransition`.
+// These map to the updates and are used to indicate which of the updates opened or closed an
+// isolated perpetual position.
 func (k Keeper) checkIsolatedSubaccountConstraints(
 	ctx sdk.Context,
 	settledUpdates []settledUpdate,
@@ -27,10 +50,12 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 ) (
 	success bool,
 	successPerUpdate []types.UpdateResult,
+	isolatedPerpetualStateTranisitions []*isolatedPerpetualStateTranisition,
 	err error,
 ) {
 	success = true
 	successPerUpdate = make([]types.UpdateResult, len(settledUpdates))
+	isolatedPerpetualStateTranisitions = make([]*isolatedPerpetualStateTranisition, len(settledUpdates))
 	var perpIdToMarketType = make(map[uint32]perptypes.PerpetualMarketType)
 
 	for _, perpetual := range perpetuals {
@@ -38,23 +63,25 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 	}
 
 	for i, u := range settledUpdates {
-		result, err := isValidIsolatedPerpetualUpdates(u, perpIdToMarketType)
+		result, stateTransition, err := processIsolatedPerpetualUpdates(u, perpIdToMarketType)
 		if err != nil {
-			return false, nil, err
+			return false, nil, nil, err
 		}
 		if result != types.Success {
 			success = false
 		}
 
 		successPerUpdate[i] = result
+		isolatedPerpetualStateTranisitions[i] = stateTransition
 	}
 
-	return success, successPerUpdate, nil
+	return success, successPerUpdate, isolatedPerpetualStateTranisitions, 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.
+// processIsolatedPerpetualUpdates checks whether the perpetual updates to a settled subaccount
+// violates constraints for isolated perpetuals and computes whether the perpetual updates result in
+// a state change (open / close) for an isolated perpetual position if the updates are valid.
+// This function assumes the settled subaccount is valid and does not violate the constraints.
 // The constraint being checked is:
 //   - a subaccount with a position in an isolated perpetual cannot have updates for other
 //     perpetuals
@@ -62,23 +89,27 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 //     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(
+//
+// If there is a state change (open / close) from the perpetual updates, it is returned along with
+// the perpetual id of the isolated perpetual and the size of the USDC position in the subaccount.
+func processIsolatedPerpetualUpdates(
 	settledUpdate settledUpdate,
 	perpIdToMarketType map[uint32]perptypes.PerpetualMarketType,
-) (types.UpdateResult, error) {
+) (types.UpdateResult, *isolatedPerpetualStateTranisition, 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
+		return types.Success, nil, nil
 	}
 
 	// Check if the updates contain an update to an isolated perpetual.
 	hasIsolatedUpdate := false
 	isolatedUpdatePerpetualId := uint32(math.MaxUint32)
+	isolatedUpdate := (*types.PerpetualUpdate)(nil)
 	for _, perpetualUpdate := range settledUpdate.PerpetualUpdates {
 		marketType, exists := perpIdToMarketType[perpetualUpdate.PerpetualId]
 		if !exists {
-			return types.UpdateCausedError, errorsmod.Wrap(
+			return types.UpdateCausedError, nil, errorsmod.Wrap(
 				perptypes.ErrPerpetualDoesNotExist, lib.UintToString(perpetualUpdate.PerpetualId),
 			)
 		}
@@ -86,6 +117,10 @@ func isValidIsolatedPerpetualUpdates(
 		if marketType == perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED {
 			hasIsolatedUpdate = true
 			isolatedUpdatePerpetualId = perpetualUpdate.PerpetualId
+			isolatedUpdate = &types.PerpetualUpdate{
+				PerpetualId:      perpetualUpdate.PerpetualId,
+				BigQuantumsDelta: perpetualUpdate.GetBigQuantums(),
+			}
 			break
 		}
 	}
@@ -94,11 +129,12 @@ func isValidIsolatedPerpetualUpdates(
 	// Assumes the subaccount itself does not violate the isolated perpetual constraints.
 	isIsolatedSubaccount := false
 	isolatedPositionPerpetualId := uint32(math.MaxUint32)
+	isolatedPerpetualPosition := (*types.PerpetualPosition)(nil)
 	hasPerpetualPositions := len(settledUpdate.SettledSubaccount.PerpetualPositions) > 0
 	for _, perpetualPosition := range settledUpdate.SettledSubaccount.PerpetualPositions {
 		marketType, exists := perpIdToMarketType[perpetualPosition.PerpetualId]
 		if !exists {
-			return types.UpdateCausedError, errorsmod.Wrap(
+			return types.UpdateCausedError, nil, errorsmod.Wrap(
 				perptypes.ErrPerpetualDoesNotExist, lib.UintToString(perpetualPosition.PerpetualId),
 			)
 		}
@@ -106,6 +142,7 @@ func isValidIsolatedPerpetualUpdates(
 		if marketType == perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED {
 			isIsolatedSubaccount = true
 			isolatedPositionPerpetualId = perpetualPosition.PerpetualId
+			isolatedPerpetualPosition = perpetualPosition
 			break
 		}
 	}
@@ -113,19 +150,19 @@ func isValidIsolatedPerpetualUpdates(
 	// 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
+		return types.ViolatesIsolatedSubaccountConstraints, nil, 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
+		return types.ViolatesIsolatedSubaccountConstraints, nil, 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
+		return types.ViolatesIsolatedSubaccountConstraints, nil, nil
 	}
 
 	// Note we can assume that if `hasIsolatedUpdate` is true, there is only a single perpetual
@@ -135,8 +172,130 @@ func isValidIsolatedPerpetualUpdates(
 	if isIsolatedSubaccount &&
 		hasIsolatedUpdate &&
 		isolatedPositionPerpetualId != isolatedUpdatePerpetualId {
-		return types.ViolatesIsolatedSubaccountConstraints, nil
+		return types.ViolatesIsolatedSubaccountConstraints, nil, nil
+	}
+
+	return types.Success,
+		getIsolatedPerpetualStateTransition(
+			settledUpdate.SettledSubaccount,
+			isolatedPerpetualPosition,
+			isolatedUpdate,
+		),
+		nil
+}
+
+// getIsolatedPerpetualStateTransition computes whether an isolated perpetual position will be
+// opened or closed for a subaccount given an isolated perpetual update for the subaccount.
+// Input subaccount account must be settled.
+func getIsolatedPerpetualStateTransition(
+	settledSubaccount types.Subaccount,
+	isolatedPerpetualPosition *types.PerpetualPosition,
+	isolatedPerpetualUpdate *types.PerpetualUpdate,
+) *isolatedPerpetualStateTranisition {
+	// If there is no update to an isolated perpetual position, then no state transitions have
+	// happened for the isolated perpetual.
+	if isolatedPerpetualUpdate == nil {
+		return nil
+	}
+
+	perpetualId := isolatedPerpetualUpdate.PerpetualId
+	// TODO(DEC-715): Support non-USDC assets.
+	usdcQuantumsBeforeUpdate := new(big.Int).Set(settledSubaccount.GetUsdcPosition())
+
+	// If the subaccount has no isolated perpetual position, then this update is opening an isolated
+	// perpetual position.
+	if isolatedPerpetualPosition == nil {
+		return &isolatedPerpetualStateTranisition{
+			perpetualId:              perpetualId,
+			usdcQuantumsBeforeUpdate: usdcQuantumsBeforeUpdate,
+			transition:               opened,
+		}
+	}
+
+	// If the position size after the update is zero, then this update is closing an isolated
+	// perpetual position.
+	if finalPositionSize := new(big.Int).Add(
+		isolatedPerpetualPosition.GetBigQuantums(),
+		isolatedPerpetualUpdate.GetBigQuantums(),
+	); finalPositionSize.Cmp(lib.BigInt0()) == 0 {
+		return &isolatedPerpetualStateTranisition{
+			perpetualId:              perpetualId,
+			usdcQuantumsBeforeUpdate: usdcQuantumsBeforeUpdate,
+			transition:               closed,
+		}
+	}
+
+	// If a position was not opened or closed, no state transition happened from the perpetual
+	// update.
+	return nil
+}
+
+// transferCollateralForIsolatedPerpetual transfers collateral between an isolated collateral pool
+// and the cross-perpetual collateral pool based on whether an isolated perpetual position was
+// opened or closed in a subaccount.
+// Note: This uses the `x/bank` keeper and modifies `x/bank` state.
+func (k *Keeper) transferCollateralForIsolatedPerpetual(
+	ctx sdk.Context,
+	updatedSubaccount types.Subaccount,
+	stateTransition *isolatedPerpetualStateTranisition,
+) error {
+	// No collateral to transfer if no state transition.
+	if stateTransition == nil {
+		return nil
+	}
+
+	isolatedCollateralPoolAddr, err := k.GetCollateralPoolFromPerpetualId(ctx, stateTransition.perpetualId)
+	if err != nil {
+		return err
+	}
+
+	// If an isolated perpetual position was opened in the subaccount, then move collateral equivalent
+	// to the USDC asset position size of the subaccount before the update from the
+	// cross-perpetual collateral pool to the isolated perpetual collateral pool.
+	if stateTransition.transition == opened {
+		_, coinToTransfer, err := k.assetsKeeper.ConvertAssetToCoin(
+			ctx,
+			// TODO(DEC-715): Support non-USDC assets.
+			assettypes.AssetUsdc.Id,
+			stateTransition.usdcQuantumsBeforeUpdate,
+		)
+		if err != nil {
+			return err
+		}
+
+		if err := k.bankKeeper.SendCoins(
+			ctx,
+			types.ModuleAddress,
+			isolatedCollateralPoolAddr,
+			[]sdk.Coin{coinToTransfer},
+		); err != nil {
+			return err
+		}
+		return nil
+		// If the isolated perpetual position was closed, then move collateral equivalent to the USDC
+		// asset position size of the subaccount after the update from the isolated perpetual collateral
+		// pool to the cross-perpetual collateral pool.
+	} else if stateTransition.transition == closed {
+		_, coinToTransfer, err := k.assetsKeeper.ConvertAssetToCoin(
+			ctx,
+			// TODO(DEC-715): Support non-USDC assets.
+			assettypes.AssetUsdc.Id,
+			updatedSubaccount.GetUsdcPosition(),
+		)
+		if err != nil {
+			return err
+		}
+
+		if err := k.bankKeeper.SendCoins(
+			ctx,
+			isolatedCollateralPoolAddr,
+			types.ModuleAddress,
+			[]sdk.Coin{coinToTransfer},
+		); err != nil {
+			return err
+		}
+		return nil
 	}
 
-	return types.Success, nil
+	return nil
 }
diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go
index ca9e978e80..0788f736c2 100644
--- a/protocol/x/subaccounts/keeper/subaccount.go
+++ b/protocol/x/subaccounts/keeper/subaccount.go
@@ -255,6 +255,9 @@ func (k Keeper) getSettledUpdates(
 // Returns a boolean indicating whether the update was successfully applied or not. If `false`, then no
 // updates to any subaccount were made. A second return value returns an array of `UpdateResult` which map
 // to the `updates` to indicate which of the updates caused a failure, if any.
+// This function also transfers collateral between the cross-perpetual collateral pool and isolated
+// perpetual collateral pools if any of the updates led to an isolated perpetual posititon to be opened
+// or closed. This is done using the `x/bank` keeper and updates `x/bank` state.
 //
 // Each `SubaccountId` in the `updates` must be unique or an error is returned.
 func (k Keeper) UpdateSubaccounts(
@@ -281,12 +284,13 @@ func (k Keeper) UpdateSubaccounts(
 	}
 
 	allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx)
-	success, successPerUpdate, err = k.internalCanUpdateSubaccounts(
+	success, successPerUpdate, isolatedPerpetualStateTranisitions, err := k.internalCanUpdateSubaccounts(
 		ctx,
 		settledUpdates,
 		updateType,
 		allPerps,
 	)
+
 	if !success || err != nil {
 		return success, successPerUpdate, err
 	}
@@ -306,6 +310,18 @@ func (k Keeper) UpdateSubaccounts(
 	// Apply the updates to asset positions.
 	UpdateAssetPositions(settledUpdates)
 
+	// Transfer collateral between collateral pools for any isolated perpetual positions that changed
+	// state due to an update.
+	for i, stateTransition := range isolatedPerpetualStateTranisitions {
+		if err := k.transferCollateralForIsolatedPerpetual(
+			ctx,
+			settledUpdates[i].SettledSubaccount,
+			stateTransition,
+		); err != nil {
+			return false, nil, err
+		}
+	}
+
 	// Apply all updates, including a subaccount update event in the Indexer block message
 	// per update and emit a cometbft event for each settled funding payment.
 	for _, u := range settledUpdates {
@@ -384,7 +400,8 @@ func (k Keeper) CanUpdateSubaccounts(
 	}
 
 	allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx)
-	return k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, allPerps)
+	success, successPerUpdate, _, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, allPerps)
+	return success, successPerUpdate, err
 }
 
 // getSettledSubaccount returns 1. a new settled subaccount given an unsettled subaccount,
@@ -515,7 +532,8 @@ func checkPositionUpdatable(
 	return nil
 }
 
-// internalCanUpdateSubaccounts will validate all `updates` to the relevant subaccounts.
+// internalCanUpdateSubaccounts will validate all `updates` to the relevant subaccounts and compute
+// if any of the updates led to an isolated perpetual position being opened or closed.
 // 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
@@ -526,6 +544,10 @@ func checkPositionUpdatable(
 // 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.
+// Returns a `isolatedPerpetualStateTransitions` value, which is a slice of
+// `isolatedPerpetualStateTransition`.
+// Thess map to the updates and are used to indicate which of the updates led to an isolated
+// perpetual position being opened or closed.
 func (k Keeper) internalCanUpdateSubaccounts(
 	ctx sdk.Context,
 	settledUpdates []settledUpdate,
@@ -534,21 +556,22 @@ func (k Keeper) internalCanUpdateSubaccounts(
 ) (
 	success bool,
 	successPerUpdate []types.UpdateResult,
+	isolatedPerpetualStateTransitions []*isolatedPerpetualStateTranisition,
 	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(
+	success, successPerUpdate, isolatedPerpetualStateTranisitions, err := k.checkIsolatedSubaccountConstraints(
 		ctx,
 		settledUpdates,
 		perpetuals,
 	)
 	if err != nil {
-		return false, nil, err
+		return false, nil, nil, err
 	}
 	if !success {
-		return success, successPerUpdate, nil
+		return success, successPerUpdate, nil, nil
 	}
 
 	// Block all withdrawals and transfers if either of the following is true within the last
@@ -605,7 +628,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 				metrics.GetLabelForBoolValue(metrics.SubaccountsNegativeTncSubaccountSeen, negativeTncSubaccountSeen),
 				metrics.GetLabelForBoolValue(metrics.ChainOutageSeen, chainOutageSeen),
 			)
-			return success, successPerUpdate, nil
+			return success, successPerUpdate, nil, nil
 		}
 	}
 
@@ -619,7 +642,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		for _, perpUpdate := range u.PerpetualUpdates {
 			err := checkPositionUpdatable(ctx, k.perpetualsKeeper, perpUpdate)
 			if err != nil {
-				return false, nil, err
+				return false, nil, nil, err
 			}
 		}
 
@@ -627,7 +650,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		for _, assetUpdate := range u.AssetUpdates {
 			err := checkPositionUpdatable(ctx, k.assetsKeeper, assetUpdate)
 			if err != nil {
-				return false, nil, err
+				return false, nil, nil, err
 			}
 		}
 
@@ -637,7 +660,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 			bigNewMaintenanceMargin,
 			err := k.internalGetNetCollateralAndMarginRequirements(ctx, u)
 		if err != nil {
-			return false, nil, err
+			return false, nil, nil, err
 		}
 
 		var result = types.Success
@@ -652,7 +675,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 
 			bytes, err := proto.Marshal(u.SettledSubaccount.Id)
 			if err != nil {
-				return false, nil, err
+				return false, nil, nil, err
 			}
 			saKey := string(bytes)
 
@@ -666,7 +689,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 					emptyUpdate,
 				)
 				if err != nil {
-					return false, nil, err
+					return false, nil, nil, err
 				}
 			}
 
@@ -688,7 +711,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		successPerUpdate[i] = result
 	}
 
-	return success, successPerUpdate, nil
+	return success, successPerUpdate, isolatedPerpetualStateTranisitions, nil
 }
 
 // IsValidStateTransitionForUndercollateralizedSubaccount returns an `UpdateResult`
diff --git a/protocol/x/subaccounts/keeper/subaccount_test.go b/protocol/x/subaccounts/keeper/subaccount_test.go
index 49fcaab685..e21b711b91 100644
--- a/protocol/x/subaccounts/keeper/subaccount_test.go
+++ b/protocol/x/subaccounts/keeper/subaccount_test.go
@@ -6,12 +6,15 @@ import (
 	"strconv"
 	"testing"
 
+	sdkmath "cosmossdk.io/math"
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	"github.com/dydxprotocol/v4-chain/protocol/lib"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/dydxprotocol/v4-chain/protocol/dtypes"
 	indexerevents "github.com/dydxprotocol/v4-chain/protocol/indexer/events"
+	bank_testutil "github.com/dydxprotocol/v4-chain/protocol/testutil/bank"
 	big_testutil "github.com/dydxprotocol/v4-chain/protocol/testutil/big"
 	"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
 	testutil "github.com/dydxprotocol/v4-chain/protocol/testutil/keeper"
@@ -299,16 +302,20 @@ func TestUpdateSubaccounts(t *testing.T) {
 		perpetualPositions []*types.PerpetualPosition
 		assetPositions     []*types.AssetPosition
 
+		// collateral pool state
+		collateralPoolUsdcBalances map[string]int64
+
 		// updates
 		updates []types.Update
 
 		// expectations
-		expectedQuoteBalance       *big.Int
-		expectedPerpetualPositions []*types.PerpetualPosition
-		expectedAssetPositions     []*types.AssetPosition
-		expectedSuccess            bool
-		expectedSuccessPerUpdate   []types.UpdateResult
-		expectedErr                error
+		expectedCollateralPoolUsdcBalances map[string]int64
+		expectedQuoteBalance               *big.Int
+		expectedPerpetualPositions         []*types.PerpetualPosition
+		expectedAssetPositions             []*types.AssetPosition
+		expectedSuccess                    bool
+		expectedSuccessPerUpdate           []types.UpdateResult
+		expectedErr                        error
 
 		// Only contains the updated perpetual positions, to assert against the events included.
 		expectedUpdatedPerpetualPositions     map[types.SubaccountId][]*types.PerpetualPosition
@@ -2252,11 +2259,212 @@ func TestUpdateSubaccounts(t *testing.T) {
 			},
 			msgSenderEnabled: true,
 		},
+		`Isolated - subaccounts - empty subaccount has update to open position for isolated perpetual,
+		collateral is moved from cross-perpetual collateral pool to isolated perpetual collateral pool`: {
+			assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)),
+			collateralPoolUsdcBalances: map[string]int64{
+				types.ModuleAddress.String(): 1_500_000_000_000, // $1,500,000 USDC
+			},
+			expectedCollateralPoolUsdcBalances: map[string]int64{
+				types.ModuleAddress.String(): 500_000_000_000, // $500,000 USDC
+				authtypes.NewModuleAddress(
+					types.ModuleName + ":" + lib.UintToString(constants.PerpetualPosition_OneISOLong.PerpetualId),
+				).String(): 1_000_000_000_000, // $1,000,000 USDC
+			},
+			expectedSuccess:          true,
+			expectedSuccessPerUpdate: []types.UpdateResult{types.Success},
+			perpetuals: []perptypes.Perpetual{
+				constants.BtcUsd_NoMarginRequirement,
+				constants.IsoUsd_IsolatedMarket,
+			},
+			perpetualPositions: []*types.PerpetualPosition{},
+			expectedPerpetualPositions: []*types.PerpetualPosition{
+				{
+					PerpetualId:  uint32(3),
+					Quantums:     dtypes.NewInt(1_000_000_000), // 1 ISO
+					FundingIndex: dtypes.NewInt(0),
+				},
+			},
+			expectedUpdatedPerpetualPositions: map[types.SubaccountId][]*types.PerpetualPosition{
+				defaultSubaccountId: {
+					{
+						PerpetualId:  uint32(3),
+						Quantums:     dtypes.NewInt(1_000_000_000), // 1 ISO
+						FundingIndex: dtypes.NewInt(0),
+					},
+				},
+			},
+			expectedAssetPositions: []*types.AssetPosition{
+				{
+					AssetId:  uint32(0),
+					Quantums: dtypes.NewInt(999_900_000_000),
+				},
+			},
+			expectedUpdatedAssetPositions: map[types.SubaccountId][]*types.AssetPosition{
+				defaultSubaccountId: {
+					{
+						AssetId:  uint32(0),
+						Quantums: dtypes.NewInt(999_900_000_000),
+					},
+				},
+			},
+			updates: []types.Update{
+				{
+					AssetUpdates: testutil.CreateUsdcAssetUpdate(big.NewInt(-100_000_000)), // -$100
+					PerpetualUpdates: []types.PerpetualUpdate{
+						{
+							PerpetualId:      uint32(3),
+							BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO
+						},
+					},
+				},
+			},
+			msgSenderEnabled: true,
+		},
+		`Isolated - subaccounts - subaccount has update to close position for isolated perpetual,
+		collateral is moved from isolated perpetual collateral pool to cross perpetual collateral pool`: {
+			assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(999_900_000_000)),
+			collateralPoolUsdcBalances: map[string]int64{
+				types.ModuleAddress.String(): 2_000_000_000_000, // $500,000 USDC
+				authtypes.NewModuleAddress(
+					types.ModuleName + ":" + lib.UintToString(constants.PerpetualPosition_OneISOLong.PerpetualId),
+				).String(): 1_500_000_000_000, // $1,500,000 USDC
+			},
+			expectedCollateralPoolUsdcBalances: map[string]int64{
+				types.ModuleAddress.String(): 3_000_000_000_000, // $3,000,000 USDC
+				authtypes.NewModuleAddress(
+					types.ModuleName + ":" + lib.UintToString(constants.PerpetualPosition_OneISOLong.PerpetualId),
+				).String(): 500_000_000_000, // $500,000 USDC
+			},
+			expectedSuccess:          true,
+			expectedSuccessPerUpdate: []types.UpdateResult{types.Success},
+			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{},
+			expectedUpdatedPerpetualPositions: map[types.SubaccountId][]*types.PerpetualPosition{
+				defaultSubaccountId: {
+					{
+						PerpetualId:  uint32(3),
+						Quantums:     dtypes.NewInt(0),
+						FundingIndex: dtypes.NewInt(0),
+					},
+				},
+			},
+			expectedAssetPositions: []*types.AssetPosition{
+				{
+					AssetId:  uint32(0),
+					Quantums: dtypes.NewInt(1_000_000_000_000),
+				},
+			},
+			expectedUpdatedAssetPositions: map[types.SubaccountId][]*types.AssetPosition{
+				defaultSubaccountId: {
+					{
+						AssetId:  uint32(0),
+						Quantums: dtypes.NewInt(1_000_000_000_000),
+					},
+				},
+			},
+			updates: []types.Update{
+				{
+					AssetUpdates: testutil.CreateUsdcAssetUpdate(big.NewInt(100_000_000)), // $100
+					PerpetualUpdates: []types.PerpetualUpdate{
+						{
+							PerpetualId:      uint32(3),
+							BigQuantumsDelta: big.NewInt(-1_000_000_000), // -1 ISO
+						},
+					},
+				},
+			},
+			msgSenderEnabled: true,
+		},
+		`Isolated subaccounts - empty subaccount has update to open position for isolated perpetual, 
+		errors out when collateral pool for cross perpetuals has no funds`: {
+			assetPositions:           testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)),
+			expectedSuccess:          false,
+			expectedSuccessPerUpdate: []types.UpdateResult{},
+			perpetuals: []perptypes.Perpetual{
+				constants.BtcUsd_NoMarginRequirement,
+				constants.IsoUsd_IsolatedMarket,
+			},
+			perpetualPositions:         []*types.PerpetualPosition{},
+			expectedPerpetualPositions: []*types.PerpetualPosition{},
+			expectedAssetPositions: []*types.AssetPosition{
+				{
+					AssetId:  uint32(0),
+					Quantums: dtypes.NewInt(1_000_000_000_000),
+				},
+			},
+			updates: []types.Update{
+				{
+					AssetUpdates: testutil.CreateUsdcAssetUpdate(big.NewInt(-100_000_000)), // -$100
+					PerpetualUpdates: []types.PerpetualUpdate{
+						{
+							PerpetualId:      uint32(3),
+							BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO
+						},
+					},
+				},
+			},
+			expectedErr:      sdkerrors.ErrInsufficientFunds,
+			msgSenderEnabled: true,
+		},
+		`Isolated subaccounts - isolated subaccount has update to close position for isolated perpetual, 
+		errors out when collateral pool for isolated perpetual has no funds`: {
+			assetPositions:           testutil.CreateUsdcAssetPosition(big.NewInt(1_000_000_000_000)),
+			expectedSuccess:          false,
+			expectedSuccessPerUpdate: []types.UpdateResult{},
+			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), // 1 ISO
+					FundingIndex: dtypes.NewInt(0),
+				},
+			},
+			expectedAssetPositions: []*types.AssetPosition{
+				{
+					AssetId:  uint32(0),
+					Quantums: dtypes.NewInt(1_000_000_000_000),
+				},
+			},
+			updates: []types.Update{
+				{
+					AssetUpdates: testutil.CreateUsdcAssetUpdate(big.NewInt(100_000_000)), // $100
+					PerpetualUpdates: []types.PerpetualUpdate{
+						{
+							PerpetualId:      uint32(3),
+							BigQuantumsDelta: big.NewInt(-1_000_000_000), // -1 ISO
+						},
+					},
+				},
+			},
+			expectedErr:      sdkerrors.ErrInsufficientFunds,
+			msgSenderEnabled: true,
+		},
 	}
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			ctx, keeper, pricesKeeper, perpetualsKeeper, _, _, assetsKeeper, _, _ := testutil.SubaccountsKeepers(
+			ctx, keeper, pricesKeeper, perpetualsKeeper, _, bankKeeper, assetsKeeper, _, _ := testutil.SubaccountsKeepers(
 				t,
 				tc.msgSenderEnabled,
 			)
@@ -2313,6 +2521,18 @@ func TestUpdateSubaccounts(t *testing.T) {
 				}
 			}
 
+			for collateralPoolAddr, usdcBal := range tc.collateralPoolUsdcBalances {
+				err := bank_testutil.FundAccount(
+					ctx,
+					sdk.MustAccAddressFromBech32(collateralPoolAddr),
+					sdk.Coins{
+						sdk.NewCoin(asstypes.AssetUsdc.Denom, sdkmath.NewInt(usdcBal)),
+					},
+					*bankKeeper,
+				)
+				require.NoError(t, err)
+			}
+
 			subaccount := createNSubaccount(keeper, ctx, 1, big.NewInt(1_000))[0]
 			subaccount.PerpetualPositions = tc.perpetualPositions
 			subaccount.AssetPositions = tc.assetPositions
@@ -2365,6 +2585,18 @@ func TestUpdateSubaccounts(t *testing.T) {
 			for i, ep := range tc.expectedAssetPositions {
 				require.Equal(t, *ep, *newSubaccount.AssetPositions[i])
 			}
+
+			for collateralPoolAddr, expectedUsdcBal := range tc.expectedCollateralPoolUsdcBalances {
+				usdcBal := bankKeeper.GetBalance(
+					ctx,
+					sdk.MustAccAddressFromBech32(collateralPoolAddr),
+					asstypes.AssetUsdc.Denom,
+				)
+				require.Equal(t,
+					sdk.NewCoin(asstypes.AssetUsdc.Denom, sdkmath.NewInt(expectedUsdcBal)),
+					usdcBal,
+				)
+			}
 		})
 	}
 }

From 049b8b0fc7ede031bf8788feab2a5ccbe4cc9410 Mon Sep 17 00:00:00 2001
From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com>
Date: Tue, 19 Mar 2024 08:17:58 -0600
Subject: [PATCH 2/6] Fix typo.

---
 .../subaccounts/keeper/isolated_subaccount.go | 20 +++++++++----------
 protocol/x/subaccounts/keeper/subaccount.go   | 10 +++++-----
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go
index 8c7303949b..9c76923844 100644
--- a/protocol/x/subaccounts/keeper/isolated_subaccount.go
+++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go
@@ -20,7 +20,7 @@ const (
 )
 
 // Represents a state transition for an isolated perpetual.
-type isolatedPerpetualStateTranisition struct {
+type isolatedPerpetualStateTransition struct {
 	perpetualId uint32
 	// TODO(DEC-715): Support non-USDC assets.
 	// USDC position size of the subaccount that has a state change for an isolated perpetual.
@@ -50,12 +50,12 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 ) (
 	success bool,
 	successPerUpdate []types.UpdateResult,
-	isolatedPerpetualStateTranisitions []*isolatedPerpetualStateTranisition,
+	isolatedPerpetualStateTransitions []*isolatedPerpetualStateTransition,
 	err error,
 ) {
 	success = true
 	successPerUpdate = make([]types.UpdateResult, len(settledUpdates))
-	isolatedPerpetualStateTranisitions = make([]*isolatedPerpetualStateTranisition, len(settledUpdates))
+	isolatedPerpetualStateTransitions = make([]*isolatedPerpetualStateTransition, len(settledUpdates))
 	var perpIdToMarketType = make(map[uint32]perptypes.PerpetualMarketType)
 
 	for _, perpetual := range perpetuals {
@@ -72,10 +72,10 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 		}
 
 		successPerUpdate[i] = result
-		isolatedPerpetualStateTranisitions[i] = stateTransition
+		isolatedPerpetualStateTransitions[i] = stateTransition
 	}
 
-	return success, successPerUpdate, isolatedPerpetualStateTranisitions, nil
+	return success, successPerUpdate, isolatedPerpetualStateTransitions, nil
 }
 
 // processIsolatedPerpetualUpdates checks whether the perpetual updates to a settled subaccount
@@ -95,7 +95,7 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 func processIsolatedPerpetualUpdates(
 	settledUpdate settledUpdate,
 	perpIdToMarketType map[uint32]perptypes.PerpetualMarketType,
-) (types.UpdateResult, *isolatedPerpetualStateTranisition, error) {
+) (types.UpdateResult, *isolatedPerpetualStateTransition, error) {
 	// If there are no perpetual updates, then this update does not violate constraints for isolated
 	// markets.
 	if len(settledUpdate.PerpetualUpdates) == 0 {
@@ -191,7 +191,7 @@ func getIsolatedPerpetualStateTransition(
 	settledSubaccount types.Subaccount,
 	isolatedPerpetualPosition *types.PerpetualPosition,
 	isolatedPerpetualUpdate *types.PerpetualUpdate,
-) *isolatedPerpetualStateTranisition {
+) *isolatedPerpetualStateTransition {
 	// If there is no update to an isolated perpetual position, then no state transitions have
 	// happened for the isolated perpetual.
 	if isolatedPerpetualUpdate == nil {
@@ -205,7 +205,7 @@ func getIsolatedPerpetualStateTransition(
 	// If the subaccount has no isolated perpetual position, then this update is opening an isolated
 	// perpetual position.
 	if isolatedPerpetualPosition == nil {
-		return &isolatedPerpetualStateTranisition{
+		return &isolatedPerpetualStateTransition{
 			perpetualId:              perpetualId,
 			usdcQuantumsBeforeUpdate: usdcQuantumsBeforeUpdate,
 			transition:               opened,
@@ -218,7 +218,7 @@ func getIsolatedPerpetualStateTransition(
 		isolatedPerpetualPosition.GetBigQuantums(),
 		isolatedPerpetualUpdate.GetBigQuantums(),
 	); finalPositionSize.Cmp(lib.BigInt0()) == 0 {
-		return &isolatedPerpetualStateTranisition{
+		return &isolatedPerpetualStateTransition{
 			perpetualId:              perpetualId,
 			usdcQuantumsBeforeUpdate: usdcQuantumsBeforeUpdate,
 			transition:               closed,
@@ -237,7 +237,7 @@ func getIsolatedPerpetualStateTransition(
 func (k *Keeper) transferCollateralForIsolatedPerpetual(
 	ctx sdk.Context,
 	updatedSubaccount types.Subaccount,
-	stateTransition *isolatedPerpetualStateTranisition,
+	stateTransition *isolatedPerpetualStateTransition,
 ) error {
 	// No collateral to transfer if no state transition.
 	if stateTransition == nil {
diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go
index 0788f736c2..bf0d3ab518 100644
--- a/protocol/x/subaccounts/keeper/subaccount.go
+++ b/protocol/x/subaccounts/keeper/subaccount.go
@@ -284,7 +284,7 @@ func (k Keeper) UpdateSubaccounts(
 	}
 
 	allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx)
-	success, successPerUpdate, isolatedPerpetualStateTranisitions, err := k.internalCanUpdateSubaccounts(
+	success, successPerUpdate, isolatedPerpetualStateTransitions, err := k.internalCanUpdateSubaccounts(
 		ctx,
 		settledUpdates,
 		updateType,
@@ -312,7 +312,7 @@ func (k Keeper) UpdateSubaccounts(
 
 	// Transfer collateral between collateral pools for any isolated perpetual positions that changed
 	// state due to an update.
-	for i, stateTransition := range isolatedPerpetualStateTranisitions {
+	for i, stateTransition := range isolatedPerpetualStateTransitions {
 		if err := k.transferCollateralForIsolatedPerpetual(
 			ctx,
 			settledUpdates[i].SettledSubaccount,
@@ -556,13 +556,13 @@ func (k Keeper) internalCanUpdateSubaccounts(
 ) (
 	success bool,
 	successPerUpdate []types.UpdateResult,
-	isolatedPerpetualStateTransitions []*isolatedPerpetualStateTranisition,
+	isolatedPerpetualStateTransitions []*isolatedPerpetualStateTransition,
 	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, isolatedPerpetualStateTranisitions, err := k.checkIsolatedSubaccountConstraints(
+	success, successPerUpdate, isolatedPerpetualStateTransitions, err = k.checkIsolatedSubaccountConstraints(
 		ctx,
 		settledUpdates,
 		perpetuals,
@@ -711,7 +711,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		successPerUpdate[i] = result
 	}
 
-	return success, successPerUpdate, isolatedPerpetualStateTranisitions, nil
+	return success, successPerUpdate, isolatedPerpetualStateTransitions, nil
 }
 
 // IsValidStateTransitionForUndercollateralizedSubaccount returns an `UpdateResult`

From 582905b7d07b0cb993d2f67a339712c56ef07de6 Mon Sep 17 00:00:00 2001
From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com>
Date: Tue, 19 Mar 2024 08:54:54 -0600
Subject: [PATCH 3/6] Add additional error cases, improve code readability.

---
 .../subaccounts/keeper/isolated_subaccount.go | 102 ++++++++++++------
 .../x/subaccounts/keeper/subaccount_test.go   |   2 +-
 2 files changed, 68 insertions(+), 36 deletions(-)

diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go
index 9c76923844..638ae50abe 100644
--- a/protocol/x/subaccounts/keeper/isolated_subaccount.go
+++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go
@@ -19,6 +19,20 @@ const (
 	closed
 )
 
+var positionStateTransitionStringMap = map[positionStateTransition]string{
+	opened: "opened",
+	closed: "closed",
+}
+
+func (t positionStateTransition) String() string {
+	result, exists := positionStateTransitionStringMap[t]
+	if !exists {
+		return "UnexpectedStateTransitionError"
+	}
+
+	return result
+}
+
 // Represents a state transition for an isolated perpetual.
 type isolatedPerpetualStateTransition struct {
 	perpetualId uint32
@@ -248,54 +262,72 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 	if err != nil {
 		return err
 	}
+	var toModuleAddr sdk.AccAddress
+	var fromModuleAddr sdk.AccAddress
+	var usdcQuantums *big.Int
 
 	// If an isolated perpetual position was opened in the subaccount, then move collateral equivalent
 	// to the USDC asset position size of the subaccount before the update from the
 	// cross-perpetual collateral pool to the isolated perpetual collateral pool.
 	if stateTransition.transition == opened {
-		_, coinToTransfer, err := k.assetsKeeper.ConvertAssetToCoin(
-			ctx,
-			// TODO(DEC-715): Support non-USDC assets.
-			assettypes.AssetUsdc.Id,
-			stateTransition.usdcQuantumsBeforeUpdate,
-		)
-		if err != nil {
-			return err
-		}
-
-		if err := k.bankKeeper.SendCoins(
-			ctx,
-			types.ModuleAddress,
-			isolatedCollateralPoolAddr,
-			[]sdk.Coin{coinToTransfer},
-		); err != nil {
-			return err
-		}
-		return nil
+		toModuleAddr = isolatedCollateralPoolAddr
+		fromModuleAddr = types.ModuleAddress
+		usdcQuantums = stateTransition.usdcQuantumsBeforeUpdate
 		// If the isolated perpetual position was closed, then move collateral equivalent to the USDC
 		// asset position size of the subaccount after the update from the isolated perpetual collateral
 		// pool to the cross-perpetual collateral pool.
 	} else if stateTransition.transition == closed {
-		_, coinToTransfer, err := k.assetsKeeper.ConvertAssetToCoin(
-			ctx,
-			// TODO(DEC-715): Support non-USDC assets.
-			assettypes.AssetUsdc.Id,
-			updatedSubaccount.GetUsdcPosition(),
+		toModuleAddr = types.ModuleAddress
+		fromModuleAddr = isolatedCollateralPoolAddr
+		usdcQuantums = updatedSubaccount.GetUsdcPosition()
+	} else {
+		// Should never hit this.
+		return errorsmod.Wrapf(
+			types.ErrFailedToUpdateSubaccounts,
+			"Invalid state transition %v for isolated perpetual with id %d in subaccount with id %v",
+			stateTransition,
+			stateTransition.perpetualId,
+			updatedSubaccount.Id,
 		)
-		if err != nil {
-			return err
-		}
+	}
 
-		if err := k.bankKeeper.SendCoins(
-			ctx,
-			isolatedCollateralPoolAddr,
-			types.ModuleAddress,
-			[]sdk.Coin{coinToTransfer},
-		); err != nil {
-			return err
-		}
+	// If there are zero quantums to transfer, don't transfer collateral.
+	if usdcQuantums.Sign() == 0 {
 		return nil
 	}
 
+	// Invalid to transfer negative quantums. This should already be caught by collateralization
+	// checks as well.
+	if usdcQuantums.Sign() == -1 {
+		return errorsmod.Wrapf(
+			types.ErrFailedToUpdateSubaccounts,
+			"Subaccount with id %v %s perpteual position with perpetual id %d with negative collateral %s to transfer",
+			updatedSubaccount.Id,
+			stateTransition.transition.String(),
+			stateTransition.perpetualId,
+			usdcQuantums.String(),
+		)
+	}
+
+	// Transfer collateral between collateral pools.
+	_, coinToTransfer, err := k.assetsKeeper.ConvertAssetToCoin(
+		ctx,
+		// TODO(DEC-715): Support non-USDC assets.
+		assettypes.AssetUsdc.Id,
+		usdcQuantums,
+	)
+	if err != nil {
+		return err
+	}
+
+	if err = k.bankKeeper.SendCoins(
+		ctx,
+		fromModuleAddr,
+		toModuleAddr,
+		[]sdk.Coin{coinToTransfer},
+	); err != nil {
+		return err
+	}
+
 	return nil
 }
diff --git a/protocol/x/subaccounts/keeper/subaccount_test.go b/protocol/x/subaccounts/keeper/subaccount_test.go
index e21b711b91..cbea84e10a 100644
--- a/protocol/x/subaccounts/keeper/subaccount_test.go
+++ b/protocol/x/subaccounts/keeper/subaccount_test.go
@@ -2323,7 +2323,7 @@ func TestUpdateSubaccounts(t *testing.T) {
 		},
 		`Isolated - subaccounts - subaccount has update to close position for isolated perpetual,
 		collateral is moved from isolated perpetual collateral pool to cross perpetual collateral pool`: {
-			assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(999_900_000_000)),
+			assetPositions: testutil.CreateUsdcAssetPosition(big.NewInt(999_900_000_000)), // $999,900 USDC
 			collateralPoolUsdcBalances: map[string]int64{
 				types.ModuleAddress.String(): 2_000_000_000_000, // $500,000 USDC
 				authtypes.NewModuleAddress(

From fe4c5bc4ec88de8f0f47b03e092b43fcad4c983d Mon Sep 17 00:00:00 2001
From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com>
Date: Tue, 19 Mar 2024 22:57:01 -0600
Subject: [PATCH 4/6] Address comments.

---
 .../subaccounts/keeper/isolated_subaccount.go | 115 +++++++-----------
 .../keeper/isolated_subaccount_test.go        | 107 ++++++++++++++++
 protocol/x/subaccounts/keeper/subaccount.go   |  20 +--
 ...ted_perpetual_position_state_transition.go |  34 ++++++
 ...erpetual_position_state_transition_test.go |  36 ++++++
 5 files changed, 232 insertions(+), 80 deletions(-)
 create mode 100644 protocol/x/subaccounts/keeper/isolated_subaccount_test.go
 create mode 100644 protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go
 create mode 100644 protocol/x/subaccounts/types/isolated_perpetual_position_state_transition_test.go

diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go
index 638ae50abe..3cef6a5af9 100644
--- a/protocol/x/subaccounts/keeper/isolated_subaccount.go
+++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go
@@ -12,37 +12,7 @@ import (
 	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
 )
 
-type positionStateTransition uint
-
-const (
-	opened positionStateTransition = iota
-	closed
-)
-
-var positionStateTransitionStringMap = map[positionStateTransition]string{
-	opened: "opened",
-	closed: "closed",
-}
-
-func (t positionStateTransition) String() string {
-	result, exists := positionStateTransitionStringMap[t]
-	if !exists {
-		return "UnexpectedStateTransitionError"
-	}
-
-	return result
-}
-
-// Represents a state transition for an isolated perpetual.
-type isolatedPerpetualStateTransition struct {
-	perpetualId uint32
-	// TODO(DEC-715): Support non-USDC assets.
-	// USDC position size of the subaccount that has a state change for an isolated perpetual.
-	usdcQuantumsBeforeUpdate *big.Int
-	transition               positionStateTransition
-}
-
-// checkIsolatedSubaccountConstaints will validate all `updates` to the relevant subaccounts against
+// processIsolatedSubaccountUpdates will validate all `updates` to the relevant subaccounts against
 // isolated subaccount constraints and computes whether each update leads to a state change
 // (open / close) to an isolated perpetual occurred due to the updates if they are valid.
 // This function checks each update in isolation, so if multiple updates for the same subaccount id
@@ -54,22 +24,25 @@ type isolatedPerpetualStateTransition struct {
 // These map to the updates and are used to indicate which of the updates
 // caused a failure, if any.
 // Returns a `isolatedPerpetualStateTransitions` value, which is a slice of
-// `isolatedPerpetualStateTransition`.
+// `IsolatedPerpetualStateTransition`.
 // These map to the updates and are used to indicate which of the updates opened or closed an
 // isolated perpetual position.
-func (k Keeper) checkIsolatedSubaccountConstraints(
+func (k Keeper) processIsolatedSubaccountUpdates(
 	ctx sdk.Context,
 	settledUpdates []settledUpdate,
 	perpetuals []perptypes.Perpetual,
 ) (
 	success bool,
 	successPerUpdate []types.UpdateResult,
-	isolatedPerpetualStateTransitions []*isolatedPerpetualStateTransition,
+	isolatedPerpetualPositionStateTransitions []*types.IsolatedPerpetualPositionStateTransition,
 	err error,
 ) {
 	success = true
 	successPerUpdate = make([]types.UpdateResult, len(settledUpdates))
-	isolatedPerpetualStateTransitions = make([]*isolatedPerpetualStateTransition, len(settledUpdates))
+	isolatedPerpetualPositionStateTransitions = make(
+		[]*types.IsolatedPerpetualPositionStateTransition,
+		len(settledUpdates),
+	)
 	var perpIdToMarketType = make(map[uint32]perptypes.PerpetualMarketType)
 
 	for _, perpetual := range perpetuals {
@@ -77,7 +50,7 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 	}
 
 	for i, u := range settledUpdates {
-		result, stateTransition, err := processIsolatedPerpetualUpdates(u, perpIdToMarketType)
+		result, stateTransition, err := processSingleIsolatedSubaccountUpdate(u, perpIdToMarketType)
 		if err != nil {
 			return false, nil, nil, err
 		}
@@ -86,13 +59,13 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 		}
 
 		successPerUpdate[i] = result
-		isolatedPerpetualStateTransitions[i] = stateTransition
+		isolatedPerpetualPositionStateTransitions[i] = stateTransition
 	}
 
-	return success, successPerUpdate, isolatedPerpetualStateTransitions, nil
+	return success, successPerUpdate, isolatedPerpetualPositionStateTransitions, nil
 }
 
-// processIsolatedPerpetualUpdates checks whether the perpetual updates to a settled subaccount
+// processSingleIsolatedSubaccountUpdate checks whether the perpetual updates to a settled subaccount
 // violates constraints for isolated perpetuals and computes whether the perpetual updates result in
 // a state change (open / close) for an isolated perpetual position if the updates are valid.
 // This function assumes the settled subaccount is valid and does not violate the constraints.
@@ -106,10 +79,10 @@ func (k Keeper) checkIsolatedSubaccountConstraints(
 //
 // If there is a state change (open / close) from the perpetual updates, it is returned along with
 // the perpetual id of the isolated perpetual and the size of the USDC position in the subaccount.
-func processIsolatedPerpetualUpdates(
+func processSingleIsolatedSubaccountUpdate(
 	settledUpdate settledUpdate,
 	perpIdToMarketType map[uint32]perptypes.PerpetualMarketType,
-) (types.UpdateResult, *isolatedPerpetualStateTransition, error) {
+) (types.UpdateResult, *types.IsolatedPerpetualPositionStateTransition, error) {
 	// If there are no perpetual updates, then this update does not violate constraints for isolated
 	// markets.
 	if len(settledUpdate.PerpetualUpdates) == 0 {
@@ -190,22 +163,22 @@ func processIsolatedPerpetualUpdates(
 	}
 
 	return types.Success,
-		getIsolatedPerpetualStateTransition(
-			settledUpdate.SettledSubaccount,
+		GetIsolatedPerpetualStateTransition(
+			// TODO(DEC-715): Support non-USDC assets.
+			settledUpdate.SettledSubaccount.GetUsdcPosition(),
 			isolatedPerpetualPosition,
 			isolatedUpdate,
 		),
 		nil
 }
 
-// getIsolatedPerpetualStateTransition computes whether an isolated perpetual position will be
+// GetIsolatedPerpetualStateTransition computes whether an isolated perpetual position will be
 // opened or closed for a subaccount given an isolated perpetual update for the subaccount.
-// Input subaccount account must be settled.
-func getIsolatedPerpetualStateTransition(
-	settledSubaccount types.Subaccount,
+func GetIsolatedPerpetualStateTransition(
+	subaccountQuoteQuantums *big.Int,
 	isolatedPerpetualPosition *types.PerpetualPosition,
 	isolatedPerpetualUpdate *types.PerpetualUpdate,
-) *isolatedPerpetualStateTransition {
+) *types.IsolatedPerpetualPositionStateTransition {
 	// If there is no update to an isolated perpetual position, then no state transitions have
 	// happened for the isolated perpetual.
 	if isolatedPerpetualUpdate == nil {
@@ -213,16 +186,14 @@ func getIsolatedPerpetualStateTransition(
 	}
 
 	perpetualId := isolatedPerpetualUpdate.PerpetualId
-	// TODO(DEC-715): Support non-USDC assets.
-	usdcQuantumsBeforeUpdate := new(big.Int).Set(settledSubaccount.GetUsdcPosition())
 
 	// If the subaccount has no isolated perpetual position, then this update is opening an isolated
 	// perpetual position.
 	if isolatedPerpetualPosition == nil {
-		return &isolatedPerpetualStateTransition{
-			perpetualId:              perpetualId,
-			usdcQuantumsBeforeUpdate: usdcQuantumsBeforeUpdate,
-			transition:               opened,
+		return &types.IsolatedPerpetualPositionStateTransition{
+			PerpetualId:               perpetualId,
+			QuoteQuantumsBeforeUpdate: subaccountQuoteQuantums,
+			Transition:                types.Opened,
 		}
 	}
 
@@ -232,10 +203,10 @@ func getIsolatedPerpetualStateTransition(
 		isolatedPerpetualPosition.GetBigQuantums(),
 		isolatedPerpetualUpdate.GetBigQuantums(),
 	); finalPositionSize.Cmp(lib.BigInt0()) == 0 {
-		return &isolatedPerpetualStateTransition{
-			perpetualId:              perpetualId,
-			usdcQuantumsBeforeUpdate: usdcQuantumsBeforeUpdate,
-			transition:               closed,
+		return &types.IsolatedPerpetualPositionStateTransition{
+			PerpetualId:               perpetualId,
+			QuoteQuantumsBeforeUpdate: subaccountQuoteQuantums,
+			Transition:                types.Closed,
 		}
 	}
 
@@ -251,61 +222,61 @@ func getIsolatedPerpetualStateTransition(
 func (k *Keeper) transferCollateralForIsolatedPerpetual(
 	ctx sdk.Context,
 	updatedSubaccount types.Subaccount,
-	stateTransition *isolatedPerpetualStateTransition,
+	stateTransition *types.IsolatedPerpetualPositionStateTransition,
 ) error {
 	// No collateral to transfer if no state transition.
 	if stateTransition == nil {
 		return nil
 	}
 
-	isolatedCollateralPoolAddr, err := k.GetCollateralPoolFromPerpetualId(ctx, stateTransition.perpetualId)
+	isolatedCollateralPoolAddr, err := k.GetCollateralPoolFromPerpetualId(ctx, stateTransition.PerpetualId)
 	if err != nil {
 		return err
 	}
 	var toModuleAddr sdk.AccAddress
 	var fromModuleAddr sdk.AccAddress
-	var usdcQuantums *big.Int
+	var quoteQuantums *big.Int
 
 	// If an isolated perpetual position was opened in the subaccount, then move collateral equivalent
 	// to the USDC asset position size of the subaccount before the update from the
 	// cross-perpetual collateral pool to the isolated perpetual collateral pool.
-	if stateTransition.transition == opened {
+	if stateTransition.Transition == types.Opened {
 		toModuleAddr = isolatedCollateralPoolAddr
 		fromModuleAddr = types.ModuleAddress
-		usdcQuantums = stateTransition.usdcQuantumsBeforeUpdate
+		quoteQuantums = stateTransition.QuoteQuantumsBeforeUpdate
 		// If the isolated perpetual position was closed, then move collateral equivalent to the USDC
 		// asset position size of the subaccount after the update from the isolated perpetual collateral
 		// pool to the cross-perpetual collateral pool.
-	} else if stateTransition.transition == closed {
+	} else if stateTransition.Transition == types.Closed {
 		toModuleAddr = types.ModuleAddress
 		fromModuleAddr = isolatedCollateralPoolAddr
-		usdcQuantums = updatedSubaccount.GetUsdcPosition()
+		quoteQuantums = updatedSubaccount.GetUsdcPosition()
 	} else {
 		// Should never hit this.
 		return errorsmod.Wrapf(
 			types.ErrFailedToUpdateSubaccounts,
 			"Invalid state transition %v for isolated perpetual with id %d in subaccount with id %v",
 			stateTransition,
-			stateTransition.perpetualId,
+			stateTransition.PerpetualId,
 			updatedSubaccount.Id,
 		)
 	}
 
 	// If there are zero quantums to transfer, don't transfer collateral.
-	if usdcQuantums.Sign() == 0 {
+	if quoteQuantums.Sign() == 0 {
 		return nil
 	}
 
 	// Invalid to transfer negative quantums. This should already be caught by collateralization
 	// checks as well.
-	if usdcQuantums.Sign() == -1 {
+	if quoteQuantums.Sign() == -1 {
 		return errorsmod.Wrapf(
 			types.ErrFailedToUpdateSubaccounts,
 			"Subaccount with id %v %s perpteual position with perpetual id %d with negative collateral %s to transfer",
 			updatedSubaccount.Id,
-			stateTransition.transition.String(),
-			stateTransition.perpetualId,
-			usdcQuantums.String(),
+			stateTransition.Transition.String(),
+			stateTransition.PerpetualId,
+			quoteQuantums.String(),
 		)
 	}
 
@@ -314,7 +285,7 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 		ctx,
 		// TODO(DEC-715): Support non-USDC assets.
 		assettypes.AssetUsdc.Id,
-		usdcQuantums,
+		quoteQuantums,
 	)
 	if err != nil {
 		return err
diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount_test.go b/protocol/x/subaccounts/keeper/isolated_subaccount_test.go
new file mode 100644
index 0000000000..af0455f87f
--- /dev/null
+++ b/protocol/x/subaccounts/keeper/isolated_subaccount_test.go
@@ -0,0 +1,107 @@
+package keeper_test
+
+import (
+	"math/big"
+	"testing"
+
+	"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
+	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/keeper"
+	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGetIsolatedPerpetualStateTransition(t *testing.T) {
+	tests := map[string]struct {
+		// parameters
+		subaccountQuoteQuantums   *big.Int
+		isolatedPerpetualUpdate   *types.PerpetualUpdate
+		isolatedPerpetualPosition *types.PerpetualPosition
+
+		// expectation
+		expectedStateTransition *types.IsolatedPerpetualPositionStateTransition
+	}{
+		`If perpetual update is nil, nil state transition is returned`: {
+			subaccountQuoteQuantums:   big.NewInt(100_000_000), // $100
+			isolatedPerpetualUpdate:   nil,
+			isolatedPerpetualPosition: nil,
+			expectedStateTransition:   nil,
+		},
+		`If perpetual update is nil and isolated position exists, nil state transition is returned`: {
+			subaccountQuoteQuantums:   big.NewInt(100_000_000), // $100
+			isolatedPerpetualUpdate:   nil,
+			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
+			expectedStateTransition:   nil,
+		},
+		`If perpetual update exists and isolated position is nil, state transition representing an
+		isolated perpetual position being opened is returned`: {
+			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
+			isolatedPerpetualUpdate: &types.PerpetualUpdate{
+				PerpetualId:      uint32(3),
+				BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO
+			},
+			isolatedPerpetualPosition: nil,
+			expectedStateTransition: &types.IsolatedPerpetualPositionStateTransition{
+				PerpetualId:               uint32(3),
+				QuoteQuantumsBeforeUpdate: big.NewInt(100_000_000),
+				Transition:                types.Opened,
+			},
+		},
+		`If perpetual update exists and isolated position exists, and perpetual update would close
+		isolated position, state transition representing an isolated perpetual position being closed is returned`: {
+			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
+			isolatedPerpetualUpdate: &types.PerpetualUpdate{
+				PerpetualId:      uint32(3),
+				BigQuantumsDelta: new(big.Int).Neg(constants.PerpetualPosition_OneISOLong.GetBigQuantums()),
+			},
+			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
+			expectedStateTransition: &types.IsolatedPerpetualPositionStateTransition{
+				PerpetualId:               uint32(3),
+				QuoteQuantumsBeforeUpdate: big.NewInt(100_000_000),
+				Transition:                types.Closed,
+			},
+		},
+		`If perpetual update exists and isolated position exists, and perpetual update would increase
+		isolated position, nil state transition is returned`: {
+			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
+			isolatedPerpetualUpdate: &types.PerpetualUpdate{
+				PerpetualId:      uint32(3),
+				BigQuantumsDelta: big.NewInt(10_000_000),
+			},
+			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
+			expectedStateTransition:   nil,
+		},
+		`If perpetual update exists and isolated position exists, and perpetual update would decrease
+		isolated position, nil state transition is returned`: {
+			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
+			isolatedPerpetualUpdate: &types.PerpetualUpdate{
+				PerpetualId:      uint32(3),
+				BigQuantumsDelta: big.NewInt(-10_000_000),
+			},
+			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
+			expectedStateTransition:   nil,
+		},
+		`If perpetual update exists and isolated position exists, and perpetual update would flip
+		isolated position, nil state transition is returned`: {
+			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
+			isolatedPerpetualUpdate: &types.PerpetualUpdate{
+				PerpetualId:      uint32(3),
+				BigQuantumsDelta: big.NewInt(-10_000_000_000),
+			},
+			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
+			expectedStateTransition:   nil,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(
+			name, func(t *testing.T) {
+				stateTransition := keeper.GetIsolatedPerpetualStateTransition(
+					tc.subaccountQuoteQuantums,
+					tc.isolatedPerpetualPosition,
+					tc.isolatedPerpetualUpdate,
+				)
+				require.Equal(t, tc.expectedStateTransition, stateTransition)
+			},
+		)
+	}
+}
diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go
index bf0d3ab518..ac15715d7d 100644
--- a/protocol/x/subaccounts/keeper/subaccount.go
+++ b/protocol/x/subaccounts/keeper/subaccount.go
@@ -284,7 +284,7 @@ func (k Keeper) UpdateSubaccounts(
 	}
 
 	allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx)
-	success, successPerUpdate, isolatedPerpetualStateTransitions, err := k.internalCanUpdateSubaccounts(
+	success, successPerUpdate, isolatedPerpetualPositionStateTransitions, err := k.internalCanUpdateSubaccounts(
 		ctx,
 		settledUpdates,
 		updateType,
@@ -312,9 +312,13 @@ func (k Keeper) UpdateSubaccounts(
 
 	// Transfer collateral between collateral pools for any isolated perpetual positions that changed
 	// state due to an update.
-	for i, stateTransition := range isolatedPerpetualStateTransitions {
+	for i, stateTransition := range isolatedPerpetualPositionStateTransitions {
 		if err := k.transferCollateralForIsolatedPerpetual(
 			ctx,
+			// settledUpdateds[i].SettledSubaccount has had the asset / perpetual updates applied to it
+			// above, so it is now an updated subaccount that can be passed into the function to execute
+			// the collateral transfers for isolated perpetual positions being opened/closed due to the
+			// updates.
 			settledUpdates[i].SettledSubaccount,
 			stateTransition,
 		); err != nil {
@@ -544,9 +548,9 @@ func checkPositionUpdatable(
 // 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.
-// Returns a `isolatedPerpetualStateTransitions` value, which is a slice of
-// `isolatedPerpetualStateTransition`.
-// Thess map to the updates and are used to indicate which of the updates led to an isolated
+// Returns a `isolatedPerpetualPositionStateTransitions` value, which is a slice of
+// `isolatedPerpetualPositionStateTransition`.
+// These map to the updates and are used to indicate which of the updates led to an isolated
 // perpetual position being opened or closed.
 func (k Keeper) internalCanUpdateSubaccounts(
 	ctx sdk.Context,
@@ -556,13 +560,13 @@ func (k Keeper) internalCanUpdateSubaccounts(
 ) (
 	success bool,
 	successPerUpdate []types.UpdateResult,
-	isolatedPerpetualStateTransitions []*isolatedPerpetualStateTransition,
+	isolatedPerpetualPositionStateTransitions []*types.IsolatedPerpetualPositionStateTransition,
 	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, isolatedPerpetualStateTransitions, err = k.checkIsolatedSubaccountConstraints(
+	success, successPerUpdate, isolatedPerpetualPositionStateTransitions, err = k.processIsolatedSubaccountUpdates(
 		ctx,
 		settledUpdates,
 		perpetuals,
@@ -711,7 +715,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		successPerUpdate[i] = result
 	}
 
-	return success, successPerUpdate, isolatedPerpetualStateTransitions, nil
+	return success, successPerUpdate, isolatedPerpetualPositionStateTransitions, nil
 }
 
 // IsValidStateTransitionForUndercollateralizedSubaccount returns an `UpdateResult`
diff --git a/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go b/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go
new file mode 100644
index 0000000000..9ea1604b8c
--- /dev/null
+++ b/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go
@@ -0,0 +1,34 @@
+package types
+
+import "math/big"
+
+type PositionStateTransition uint
+
+const (
+	Opened PositionStateTransition = iota
+	Closed
+)
+
+var positionStateTransitionStringMap = map[PositionStateTransition]string{
+	Opened: "opened",
+	Closed: "closed",
+}
+
+func (t PositionStateTransition) String() string {
+	result, exists := positionStateTransitionStringMap[t]
+	if !exists {
+		return "UnexpectedStateTransitionError"
+	}
+
+	return result
+}
+
+// Represents a state transition for an isolated perpetual position.
+type IsolatedPerpetualPositionStateTransition struct {
+	PerpetualId uint32
+	// TODO(DEC-715): Support non-USDC assets.
+	// Quote quantums position size of the subaccount that has a state change for an isolated perpetual.
+	QuoteQuantumsBeforeUpdate *big.Int
+	// The state transition that occurred for the isolated perpetual positions.
+	Transition PositionStateTransition
+}
diff --git a/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition_test.go b/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition_test.go
new file mode 100644
index 0000000000..b58c616858
--- /dev/null
+++ b/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition_test.go
@@ -0,0 +1,36 @@
+package types_test
+
+import (
+	"testing"
+
+	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPositionStateTransitionString(t *testing.T) {
+	tests := map[string]struct {
+		value          types.PositionStateTransition
+		expectedResult string
+	}{
+		"Opened": {
+			value:          types.Opened,
+			expectedResult: "opened",
+		},
+		"Closed": {
+			value:          types.Closed,
+			expectedResult: "closed",
+		},
+		"UnexpectedError": {
+			value:          types.PositionStateTransition(2),
+			expectedResult: "UnexpectedStateTransitionError",
+		},
+	}
+
+	// Run tests.
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			result := tc.value.String()
+			require.Equal(t, result, tc.expectedResult)
+		})
+	}
+}

From 039290be6e1488996b2898a23679469419580b2b Mon Sep 17 00:00:00 2001
From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com>
Date: Thu, 21 Mar 2024 14:50:12 -0400
Subject: [PATCH 5/6] Address code organization comments, move state transition
 logic to after updates.

---
 protocol/testutil/constants/positions.go      |   2 +-
 .../subaccounts/keeper/isolated_subaccount.go | 226 ++++++-----
 .../keeper/isolated_subaccount_test.go        | 362 ++++++++++++++----
 protocol/x/subaccounts/keeper/subaccount.go   |  59 ++-
 .../x/subaccounts/keeper/subaccount_helper.go |   8 +-
 protocol/x/subaccounts/keeper/update.go       |   4 +-
 ...ted_perpetual_position_state_transition.go |   9 +-
 7 files changed, 462 insertions(+), 208 deletions(-)

diff --git a/protocol/testutil/constants/positions.go b/protocol/testutil/constants/positions.go
index 5035068aa7..0dcf8e48cc 100644
--- a/protocol/testutil/constants/positions.go
+++ b/protocol/testutil/constants/positions.go
@@ -76,7 +76,7 @@ var (
 	// Long position for arbitrary isolated market
 	PerpetualPosition_OneISOLong = satypes.PerpetualPosition{
 		PerpetualId:  3,
-		Quantums:     dtypes.NewInt(100_000_000),
+		Quantums:     dtypes.NewInt(1_000_000_000),
 		FundingIndex: dtypes.NewInt(0),
 	}
 	PerpetualPosition_OneISO2Long = satypes.PerpetualPosition{
diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go
index 3cef6a5af9..26899a1dc9 100644
--- a/protocol/x/subaccounts/keeper/isolated_subaccount.go
+++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go
@@ -12,9 +12,8 @@ import (
 	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
 )
 
-// processIsolatedSubaccountUpdates will validate all `updates` to the relevant subaccounts against
-// isolated subaccount constraints and computes whether each update leads to a state change
-// (open / close) to an isolated perpetual occurred due to the updates if they are valid.
+// 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 not evaluated separately.
 // The input subaccounts must be settled.
@@ -23,52 +22,37 @@ import (
 // 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.
-// Returns a `isolatedPerpetualStateTransitions` value, which is a slice of
-// `IsolatedPerpetualStateTransition`.
-// These map to the updates and are used to indicate which of the updates opened or closed an
-// isolated perpetual position.
-func (k Keeper) processIsolatedSubaccountUpdates(
+func (k Keeper) checkIsolatedSubaccountConstraints(
 	ctx sdk.Context,
-	settledUpdates []settledUpdate,
+	settledUpdates []SettledUpdate,
 	perpetuals []perptypes.Perpetual,
 ) (
 	success bool,
 	successPerUpdate []types.UpdateResult,
-	isolatedPerpetualPositionStateTransitions []*types.IsolatedPerpetualPositionStateTransition,
 	err error,
 ) {
 	success = true
 	successPerUpdate = make([]types.UpdateResult, len(settledUpdates))
-	isolatedPerpetualPositionStateTransitions = make(
-		[]*types.IsolatedPerpetualPositionStateTransition,
-		len(settledUpdates),
-	)
-	var perpIdToMarketType = make(map[uint32]perptypes.PerpetualMarketType)
-
-	for _, perpetual := range perpetuals {
-		perpIdToMarketType[perpetual.GetId()] = perpetual.Params.MarketType
-	}
+	perpIdToMarketType := getPerpIdToMarketTypeMap(perpetuals)
 
 	for i, u := range settledUpdates {
-		result, stateTransition, err := processSingleIsolatedSubaccountUpdate(u, perpIdToMarketType)
+		result, err := isValidIsolatedPerpetualUpdates(u, perpIdToMarketType)
 		if err != nil {
-			return false, nil, nil, err
+			return false, nil, err
 		}
 		if result != types.Success {
 			success = false
 		}
 
 		successPerUpdate[i] = result
-		isolatedPerpetualPositionStateTransitions[i] = stateTransition
 	}
 
-	return success, successPerUpdate, isolatedPerpetualPositionStateTransitions, nil
+	return success, successPerUpdate, nil
 }
 
-// processSingleIsolatedSubaccountUpdate checks whether the perpetual updates to a settled subaccount
-// violates constraints for isolated perpetuals and computes whether the perpetual updates result in
-// a state change (open / close) for an isolated perpetual position if the updates are valid.
-// This function assumes the settled subaccount is valid and does not violate the constraints.
+// 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 for other
 //     perpetuals
@@ -76,27 +60,23 @@ func (k Keeper) processIsolatedSubaccountUpdates(
 //     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
-//
-// If there is a state change (open / close) from the perpetual updates, it is returned along with
-// the perpetual id of the isolated perpetual and the size of the USDC position in the subaccount.
-func processSingleIsolatedSubaccountUpdate(
-	settledUpdate settledUpdate,
+func isValidIsolatedPerpetualUpdates(
+	settledUpdate SettledUpdate,
 	perpIdToMarketType map[uint32]perptypes.PerpetualMarketType,
-) (types.UpdateResult, *types.IsolatedPerpetualPositionStateTransition, error) {
+) (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, nil
+		return types.Success, nil
 	}
 
 	// Check if the updates contain an update to an isolated perpetual.
 	hasIsolatedUpdate := false
 	isolatedUpdatePerpetualId := uint32(math.MaxUint32)
-	isolatedUpdate := (*types.PerpetualUpdate)(nil)
 	for _, perpetualUpdate := range settledUpdate.PerpetualUpdates {
 		marketType, exists := perpIdToMarketType[perpetualUpdate.PerpetualId]
 		if !exists {
-			return types.UpdateCausedError, nil, errorsmod.Wrap(
+			return types.UpdateCausedError, errorsmod.Wrap(
 				perptypes.ErrPerpetualDoesNotExist, lib.UintToString(perpetualUpdate.PerpetualId),
 			)
 		}
@@ -104,10 +84,6 @@ func processSingleIsolatedSubaccountUpdate(
 		if marketType == perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED {
 			hasIsolatedUpdate = true
 			isolatedUpdatePerpetualId = perpetualUpdate.PerpetualId
-			isolatedUpdate = &types.PerpetualUpdate{
-				PerpetualId:      perpetualUpdate.PerpetualId,
-				BigQuantumsDelta: perpetualUpdate.GetBigQuantums(),
-			}
 			break
 		}
 	}
@@ -116,12 +92,11 @@ func processSingleIsolatedSubaccountUpdate(
 	// Assumes the subaccount itself does not violate the isolated perpetual constraints.
 	isIsolatedSubaccount := false
 	isolatedPositionPerpetualId := uint32(math.MaxUint32)
-	isolatedPerpetualPosition := (*types.PerpetualPosition)(nil)
 	hasPerpetualPositions := len(settledUpdate.SettledSubaccount.PerpetualPositions) > 0
 	for _, perpetualPosition := range settledUpdate.SettledSubaccount.PerpetualPositions {
 		marketType, exists := perpIdToMarketType[perpetualPosition.PerpetualId]
 		if !exists {
-			return types.UpdateCausedError, nil, errorsmod.Wrap(
+			return types.UpdateCausedError, errorsmod.Wrap(
 				perptypes.ErrPerpetualDoesNotExist, lib.UintToString(perpetualPosition.PerpetualId),
 			)
 		}
@@ -129,7 +104,6 @@ func processSingleIsolatedSubaccountUpdate(
 		if marketType == perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED {
 			isIsolatedSubaccount = true
 			isolatedPositionPerpetualId = perpetualPosition.PerpetualId
-			isolatedPerpetualPosition = perpetualPosition
 			break
 		}
 	}
@@ -137,19 +111,19 @@ func processSingleIsolatedSubaccountUpdate(
 	// 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, nil
+		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, nil
+		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, nil
+		return types.ViolatesIsolatedSubaccountConstraints, nil
 	}
 
 	// Note we can assume that if `hasIsolatedUpdate` is true, there is only a single perpetual
@@ -159,60 +133,106 @@ func processSingleIsolatedSubaccountUpdate(
 	if isIsolatedSubaccount &&
 		hasIsolatedUpdate &&
 		isolatedPositionPerpetualId != isolatedUpdatePerpetualId {
-		return types.ViolatesIsolatedSubaccountConstraints, nil, nil
+		return types.ViolatesIsolatedSubaccountConstraints, nil
 	}
 
-	return types.Success,
-		GetIsolatedPerpetualStateTransition(
-			// TODO(DEC-715): Support non-USDC assets.
-			settledUpdate.SettledSubaccount.GetUsdcPosition(),
-			isolatedPerpetualPosition,
-			isolatedUpdate,
-		),
-		nil
+	return types.Success, nil
 }
 
 // GetIsolatedPerpetualStateTransition computes whether an isolated perpetual position will be
-// opened or closed for a subaccount given an isolated perpetual update for the subaccount.
+// opened or closed for a subaccount.
+// This function assumes that the subaccount is valid under isolated perpetual constraints.
+// The input `settledUpdate` must have an updated subaccount (`settledUpdate.SettledSubaccount`),
+// so all the updates must have been applied already to the subaccount.
 func GetIsolatedPerpetualStateTransition(
-	subaccountQuoteQuantums *big.Int,
-	isolatedPerpetualPosition *types.PerpetualPosition,
-	isolatedPerpetualUpdate *types.PerpetualUpdate,
-) *types.IsolatedPerpetualPositionStateTransition {
-	// If there is no update to an isolated perpetual position, then no state transitions have
-	// happened for the isolated perpetual.
-	if isolatedPerpetualUpdate == nil {
-		return nil
+	settledUpdateWithUpdatedSubaccount SettledUpdate,
+	perpetuals []perptypes.Perpetual,
+) (*types.IsolatedPerpetualPositionStateTransition, error) {
+	perpIdToMarketType := getPerpIdToMarketTypeMap(perpetuals)
+	// This subaccount needs to have had the updates in the `settledUpdate` already applied to it.
+	updatedSubaccount := settledUpdateWithUpdatedSubaccount.SettledSubaccount
+	// If there are no perpetual updates, then no perpetual position could have been opened or closed
+	// on the subaccount.
+	if len(settledUpdateWithUpdatedSubaccount.PerpetualUpdates) == 0 {
+		return nil, nil
 	}
 
-	perpetualId := isolatedPerpetualUpdate.PerpetualId
+	// If there are more than 1 valid perpetual update, or more than 1 valid perpetual position on the
+	// subaccount, it is not isolated to an isolated perpetual, and so no isolated perpetual position
+	// could have been opened or closed.
+	if len(settledUpdateWithUpdatedSubaccount.PerpetualUpdates) > 1 ||
+		len(updatedSubaccount.PerpetualPositions) > 1 {
+		return nil, nil
+	}
 
-	// If the subaccount has no isolated perpetual position, then this update is opening an isolated
-	// perpetual position.
-	if isolatedPerpetualPosition == nil {
-		return &types.IsolatedPerpetualPositionStateTransition{
-			PerpetualId:               perpetualId,
-			QuoteQuantumsBeforeUpdate: subaccountQuoteQuantums,
-			Transition:                types.Opened,
-		}
+	// Now, from the above checks, we know there is only a single perpetual update and 0 or 1 perpetual
+	// positions.
+	perpetualUpdate := settledUpdateWithUpdatedSubaccount.PerpetualUpdates[0]
+	marketType, exists := perpIdToMarketType[perpetualUpdate.PerpetualId]
+	if !exists {
+		return nil, errorsmod.Wrap(
+			perptypes.ErrPerpetualDoesNotExist, lib.UintToString(perpetualUpdate.PerpetualId),
+		)
+	}
+	// If the perpetual update is not for an isolated perpetual, no isolated perpetual position is
+	// being opened or closed.
+	if marketType != perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED {
+		return nil, nil
 	}
 
-	// If the position size after the update is zero, then this update is closing an isolated
-	// perpetual position.
-	if finalPositionSize := new(big.Int).Add(
-		isolatedPerpetualPosition.GetBigQuantums(),
-		isolatedPerpetualUpdate.GetBigQuantums(),
-	); finalPositionSize.Cmp(lib.BigInt0()) == 0 {
+	// If the updated subaccount does not have any perpetual positions, then an isolated perpetual
+	// position must have been closed due to the perpetual update.
+	if len(updatedSubaccount.PerpetualPositions) == 0 {
 		return &types.IsolatedPerpetualPositionStateTransition{
-			PerpetualId:               perpetualId,
-			QuoteQuantumsBeforeUpdate: subaccountQuoteQuantums,
-			Transition:                types.Closed,
+			SubaccountId:  updatedSubaccount.Id,
+			PerpetualId:   perpetualUpdate.PerpetualId,
+			QuoteQuantums: updatedSubaccount.GetUsdcPosition(),
+			Transition:    types.Closed,
+		}, nil
+	}
+
+	// After the above checks, the subaccount must have only a single perpetual position, which is for
+	// the same isolated perpetual as the perpetual update.
+	perpetualPosition := updatedSubaccount.PerpetualPositions[0]
+	// If the size of the update and the position are the same, the perpetual update must have opened
+	// the position.
+	if perpetualUpdate.GetBigQuantums().Cmp(perpetualPosition.GetBigQuantums()) == 0 {
+		if len(settledUpdateWithUpdatedSubaccount.AssetUpdates) != 1 {
+			return nil, errorsmod.Wrapf(
+				types.ErrFailedToUpdateSubaccounts,
+				"Subaccount with id %v opened perpteual position with perpetual id %d with invalid number of"+
+					" changes to asset positions (%d), should only be 1 asset update",
+				updatedSubaccount.Id,
+				perpetualUpdate.PerpetualId,
+				len(settledUpdateWithUpdatedSubaccount.AssetUpdates),
+			)
+		}
+		if settledUpdateWithUpdatedSubaccount.AssetUpdates[0].AssetId != assettypes.AssetUsdc.Id {
+			return nil, errorsmod.Wrapf(
+				types.ErrFailedToUpdateSubaccounts,
+				"Subaccount with id %v opened perpteual position with perpetual id %d without a change to the"+
+					" quote currency's asset position.",
+				updatedSubaccount.Id,
+				perpetualUpdate.PerpetualId,
+			)
 		}
+		// Collateral equal to the quote currency asset position before the update was applied needs to be transferred.
+		// Subtract the delta from the updated subaccount's quote currency asset position size to get the size
+		// of the quote currency asset position.
+		quoteQuantumsBeforeUpdate := new(big.Int).Sub(
+			updatedSubaccount.GetUsdcPosition(),
+			settledUpdateWithUpdatedSubaccount.AssetUpdates[0].GetBigQuantums(),
+		)
+		return &types.IsolatedPerpetualPositionStateTransition{
+			SubaccountId:  updatedSubaccount.Id,
+			PerpetualId:   perpetualUpdate.PerpetualId,
+			QuoteQuantums: quoteQuantumsBeforeUpdate,
+			Transition:    types.Opened,
+		}, nil
 	}
 
-	// If a position was not opened or closed, no state transition happened from the perpetual
-	// update.
-	return nil
+	// The isolated perpetual position changed size but was not opened or closed.
+	return nil, nil
 }
 
 // transferCollateralForIsolatedPerpetual transfers collateral between an isolated collateral pool
@@ -221,7 +241,6 @@ func GetIsolatedPerpetualStateTransition(
 // Note: This uses the `x/bank` keeper and modifies `x/bank` state.
 func (k *Keeper) transferCollateralForIsolatedPerpetual(
 	ctx sdk.Context,
-	updatedSubaccount types.Subaccount,
 	stateTransition *types.IsolatedPerpetualPositionStateTransition,
 ) error {
 	// No collateral to transfer if no state transition.
@@ -235,22 +254,17 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 	}
 	var toModuleAddr sdk.AccAddress
 	var fromModuleAddr sdk.AccAddress
-	var quoteQuantums *big.Int
 
-	// If an isolated perpetual position was opened in the subaccount, then move collateral equivalent
-	// to the USDC asset position size of the subaccount before the update from the
+	// If an isolated perpetual position was opened in the subaccount, then move collateral from the
 	// cross-perpetual collateral pool to the isolated perpetual collateral pool.
 	if stateTransition.Transition == types.Opened {
 		toModuleAddr = isolatedCollateralPoolAddr
 		fromModuleAddr = types.ModuleAddress
-		quoteQuantums = stateTransition.QuoteQuantumsBeforeUpdate
-		// If the isolated perpetual position was closed, then move collateral equivalent to the USDC
-		// asset position size of the subaccount after the update from the isolated perpetual collateral
-		// pool to the cross-perpetual collateral pool.
+		// If the isolated perpetual position was closed, then move collateral from the isolated
+		// perpetual collateral pool to the cross-perpetual collateral pool.
 	} else if stateTransition.Transition == types.Closed {
 		toModuleAddr = types.ModuleAddress
 		fromModuleAddr = isolatedCollateralPoolAddr
-		quoteQuantums = updatedSubaccount.GetUsdcPosition()
 	} else {
 		// Should never hit this.
 		return errorsmod.Wrapf(
@@ -258,25 +272,25 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 			"Invalid state transition %v for isolated perpetual with id %d in subaccount with id %v",
 			stateTransition,
 			stateTransition.PerpetualId,
-			updatedSubaccount.Id,
+			stateTransition.SubaccountId,
 		)
 	}
 
 	// If there are zero quantums to transfer, don't transfer collateral.
-	if quoteQuantums.Sign() == 0 {
+	if stateTransition.QuoteQuantums.Sign() == 0 {
 		return nil
 	}
 
 	// Invalid to transfer negative quantums. This should already be caught by collateralization
 	// checks as well.
-	if quoteQuantums.Sign() == -1 {
+	if stateTransition.QuoteQuantums.Sign() == -1 {
 		return errorsmod.Wrapf(
 			types.ErrFailedToUpdateSubaccounts,
 			"Subaccount with id %v %s perpteual position with perpetual id %d with negative collateral %s to transfer",
-			updatedSubaccount.Id,
+			stateTransition.SubaccountId,
 			stateTransition.Transition.String(),
 			stateTransition.PerpetualId,
-			quoteQuantums.String(),
+			stateTransition.QuoteQuantums.String(),
 		)
 	}
 
@@ -285,7 +299,7 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 		ctx,
 		// TODO(DEC-715): Support non-USDC assets.
 		assettypes.AssetUsdc.Id,
-		quoteQuantums,
+		stateTransition.QuoteQuantums,
 	)
 	if err != nil {
 		return err
@@ -302,3 +316,15 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 
 	return nil
 }
+
+func getPerpIdToMarketTypeMap(
+	perpetuals []perptypes.Perpetual,
+) map[uint32]perptypes.PerpetualMarketType {
+	var perpIdToMarketType = make(map[uint32]perptypes.PerpetualMarketType)
+
+	for _, perpetual := range perpetuals {
+		perpIdToMarketType[perpetual.GetId()] = perpetual.Params.MarketType
+	}
+
+	return perpIdToMarketType
+}
diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount_test.go b/protocol/x/subaccounts/keeper/isolated_subaccount_test.go
index af0455f87f..25073a920e 100644
--- a/protocol/x/subaccounts/keeper/isolated_subaccount_test.go
+++ b/protocol/x/subaccounts/keeper/isolated_subaccount_test.go
@@ -4,7 +4,10 @@ import (
 	"math/big"
 	"testing"
 
+	"github.com/dydxprotocol/v4-chain/protocol/dtypes"
 	"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
+	assettypes "github.com/dydxprotocol/v4-chain/protocol/x/assets/types"
+	perptypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types"
 	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/keeper"
 	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
 	"github.com/stretchr/testify/require"
@@ -13,94 +16,319 @@ import (
 func TestGetIsolatedPerpetualStateTransition(t *testing.T) {
 	tests := map[string]struct {
 		// parameters
-		subaccountQuoteQuantums   *big.Int
-		isolatedPerpetualUpdate   *types.PerpetualUpdate
-		isolatedPerpetualPosition *types.PerpetualPosition
+		settledUpdateWithUpdatedSubaccount keeper.SettledUpdate
+		perpetuals                         []perptypes.Perpetual
 
 		// expectation
 		expectedStateTransition *types.IsolatedPerpetualPositionStateTransition
+		expectedErr             error
 	}{
-		`If perpetual update is nil, nil state transition is returned`: {
-			subaccountQuoteQuantums:   big.NewInt(100_000_000), // $100
-			isolatedPerpetualUpdate:   nil,
-			isolatedPerpetualPosition: nil,
-			expectedStateTransition:   nil,
+		`If no perpetual updates, nil state transition is returned`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id:                 &constants.Alice_Num0,
+					PerpetualPositions: nil,
+					AssetPositions:     nil,
+				},
+				PerpetualUpdates: nil,
+				AssetUpdates:     nil,
+			},
+			perpetuals:              nil,
+			expectedStateTransition: nil,
+		},
+		`If single non-isolated perpetual updates, nil state transition is returned`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id:                 &constants.Alice_Num0,
+					PerpetualPositions: nil,
+					AssetPositions:     nil,
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(0),
+						BigQuantumsDelta: big.NewInt(-100),
+					},
+				},
+				AssetUpdates: nil,
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.BtcUsd_100PercentMarginRequirement,
+			},
+			expectedStateTransition: nil,
 		},
-		`If perpetual update is nil and isolated position exists, nil state transition is returned`: {
-			subaccountQuoteQuantums:   big.NewInt(100_000_000), // $100
-			isolatedPerpetualUpdate:   nil,
-			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
-			expectedStateTransition:   nil,
+		`If multiple non-isolated perpetual updates, nil state transition is returned`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id:                 &constants.Alice_Num0,
+					PerpetualPositions: nil,
+					AssetPositions:     nil,
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(0),
+						BigQuantumsDelta: big.NewInt(-100),
+					},
+					{
+						PerpetualId:      uint32(1),
+						BigQuantumsDelta: big.NewInt(-200),
+					},
+				},
+				AssetUpdates: nil,
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.BtcUsd_100PercentMarginRequirement,
+				constants.EthUsd_NoMarginRequirement,
+			},
+			expectedStateTransition: nil,
 		},
-		`If perpetual update exists and isolated position is nil, state transition representing an
-		isolated perpetual position being opened is returned`: {
-			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
-			isolatedPerpetualUpdate: &types.PerpetualUpdate{
-				PerpetualId:      uint32(3),
-				BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO
-			},
-			isolatedPerpetualPosition: nil,
+		`If multiple non-isolated perpetual positions, nil state transition is returned`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id: &constants.Alice_Num0,
+					PerpetualPositions: []*types.PerpetualPosition{
+						&constants.PerpetualPosition_OneBTCLong,
+						&constants.PerpetualPosition_OneTenthEthLong,
+					},
+					AssetPositions: nil,
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(0),
+						BigQuantumsDelta: big.NewInt(-100),
+					},
+				},
+				AssetUpdates: nil,
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.BtcUsd_100PercentMarginRequirement,
+				constants.EthUsd_NoMarginRequirement,
+			},
+			expectedStateTransition: nil,
+		},
+		`If single isolated perpetual update, no perpetual position, state transition is returned for closed position`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id:                 &constants.Alice_Num0,
+					PerpetualPositions: nil,
+					AssetPositions: []*types.AssetPosition{
+						&constants.Usdc_Asset_10_000,
+					},
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(3),
+						BigQuantumsDelta: big.NewInt(-1_000_000_000),
+					},
+				},
+				AssetUpdates: []types.AssetUpdate{
+					{
+						AssetId:          assettypes.AssetUsdc.Id,
+						BigQuantumsDelta: big.NewInt(100_000_000),
+					},
+				},
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.IsoUsd_IsolatedMarket,
+			},
 			expectedStateTransition: &types.IsolatedPerpetualPositionStateTransition{
-				PerpetualId:               uint32(3),
-				QuoteQuantumsBeforeUpdate: big.NewInt(100_000_000),
-				Transition:                types.Opened,
+				SubaccountId:  &constants.Alice_Num0,
+				PerpetualId:   uint32(3),
+				QuoteQuantums: constants.Usdc_Asset_10_000.GetBigQuantums(),
+				Transition:    types.Closed,
 			},
 		},
-		`If perpetual update exists and isolated position exists, and perpetual update would close
-		isolated position, state transition representing an isolated perpetual position being closed is returned`: {
-			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
-			isolatedPerpetualUpdate: &types.PerpetualUpdate{
-				PerpetualId:      uint32(3),
-				BigQuantumsDelta: new(big.Int).Neg(constants.PerpetualPosition_OneISOLong.GetBigQuantums()),
-			},
-			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
+		`If single isolated perpetual update, existing perpetual position with same size, state transition is returned for
+		opened position`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id: &constants.Alice_Num0,
+					PerpetualPositions: []*types.PerpetualPosition{
+						&constants.PerpetualPosition_OneISOLong,
+					},
+					AssetPositions: []*types.AssetPosition{
+						{
+							AssetId:  assettypes.AssetUsdc.Id,
+							Quantums: dtypes.NewInt(-40_000_000), // -$40
+						},
+					},
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(3),
+						BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO
+					},
+				},
+				AssetUpdates: []types.AssetUpdate{
+					{
+						AssetId:          assettypes.AssetUsdc.Id,
+						BigQuantumsDelta: big.NewInt(-50_000_000), // -$50
+					},
+				},
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.IsoUsd_IsolatedMarket,
+			},
 			expectedStateTransition: &types.IsolatedPerpetualPositionStateTransition{
-				PerpetualId:               uint32(3),
-				QuoteQuantumsBeforeUpdate: big.NewInt(100_000_000),
-				Transition:                types.Closed,
+				SubaccountId:  &constants.Alice_Num0,
+				PerpetualId:   uint32(3),
+				QuoteQuantums: big.NewInt(10_000_000), // $-40 - (-$50)
+				Transition:    types.Opened,
 			},
 		},
-		`If perpetual update exists and isolated position exists, and perpetual update would increase
-		isolated position, nil state transition is returned`: {
-			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
-			isolatedPerpetualUpdate: &types.PerpetualUpdate{
-				PerpetualId:      uint32(3),
-				BigQuantumsDelta: big.NewInt(10_000_000),
-			},
-			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
-			expectedStateTransition:   nil,
+		`If single isolated perpetual update, existing perpetual position with different size, nil state transition
+		returned`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id: &constants.Alice_Num0,
+					PerpetualPositions: []*types.PerpetualPosition{
+						&constants.PerpetualPosition_OneISOLong,
+					},
+					AssetPositions: []*types.AssetPosition{
+						{
+							AssetId:  assettypes.AssetUsdc.Id,
+							Quantums: dtypes.NewInt(-40_000_000), // -$65
+						},
+					},
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(3),
+						BigQuantumsDelta: big.NewInt(500_000_000), // 0.5 ISO
+					},
+				},
+				AssetUpdates: []types.AssetUpdate{
+					{
+						AssetId:          assettypes.AssetUsdc.Id,
+						BigQuantumsDelta: big.NewInt(-25_000_000), // -$25
+					},
+				},
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.IsoUsd_IsolatedMarket,
+			},
+			expectedStateTransition: nil,
 		},
-		`If perpetual update exists and isolated position exists, and perpetual update would decrease
-		isolated position, nil state transition is returned`: {
-			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
-			isolatedPerpetualUpdate: &types.PerpetualUpdate{
-				PerpetualId:      uint32(3),
-				BigQuantumsDelta: big.NewInt(-10_000_000),
-			},
-			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
-			expectedStateTransition:   nil,
+		`Returns error if perpetual position was opened with no asset updates`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id: &constants.Alice_Num0,
+					PerpetualPositions: []*types.PerpetualPosition{
+						&constants.PerpetualPosition_OneISOLong,
+					},
+					AssetPositions: []*types.AssetPosition{
+						{
+							AssetId:  assettypes.AssetUsdc.Id,
+							Quantums: dtypes.NewInt(50_000_000), // $50
+						},
+					},
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(3),
+						BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO
+					},
+				},
+				AssetUpdates: nil,
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.IsoUsd_IsolatedMarket,
+			},
+			expectedStateTransition: nil,
+			expectedErr:             types.ErrFailedToUpdateSubaccounts,
 		},
-		`If perpetual update exists and isolated position exists, and perpetual update would flip
-		isolated position, nil state transition is returned`: {
-			subaccountQuoteQuantums: big.NewInt(100_000_000), // $100
-			isolatedPerpetualUpdate: &types.PerpetualUpdate{
-				PerpetualId:      uint32(3),
-				BigQuantumsDelta: big.NewInt(-10_000_000_000),
-			},
-			isolatedPerpetualPosition: &constants.PerpetualPosition_OneISOLong,
-			expectedStateTransition:   nil,
+		`Returns error if perpetual position was opened with multiple asset updates`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id: &constants.Alice_Num0,
+					PerpetualPositions: []*types.PerpetualPosition{
+						&constants.PerpetualPosition_OneISOLong,
+					},
+					AssetPositions: []*types.AssetPosition{
+						{
+							AssetId:  assettypes.AssetUsdc.Id,
+							Quantums: dtypes.NewInt(-40_000_000), // -$40
+						},
+						{
+							AssetId:  constants.BtcUsd.Id,
+							Quantums: dtypes.NewInt(100_000_000), // 1 BTC
+						},
+					},
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(3),
+						BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO
+					},
+				},
+				AssetUpdates: []types.AssetUpdate{
+					{
+						AssetId:          assettypes.AssetUsdc.Id,
+						BigQuantumsDelta: big.NewInt(-50_000_000), // -$50
+					},
+					{
+						AssetId:          constants.BtcUsd.Id,
+						BigQuantumsDelta: big.NewInt(100_000_000), // 1 BTC
+					},
+				},
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.IsoUsd_IsolatedMarket,
+			},
+			expectedStateTransition: nil,
+			expectedErr:             types.ErrFailedToUpdateSubaccounts,
+		},
+		`Returns error if perpetual position was opened with non-usdc asset update`: {
+			settledUpdateWithUpdatedSubaccount: keeper.SettledUpdate{
+				SettledSubaccount: types.Subaccount{
+					Id: &constants.Alice_Num0,
+					PerpetualPositions: []*types.PerpetualPosition{
+						&constants.PerpetualPosition_OneISOLong,
+					},
+					AssetPositions: []*types.AssetPosition{
+						{
+							AssetId:  assettypes.AssetUsdc.Id,
+							Quantums: dtypes.NewInt(50_000_000), // $50
+						},
+						{
+							AssetId:  constants.BtcUsd.Id,
+							Quantums: dtypes.NewInt(100_000_000), // 1 BTC
+						},
+					},
+				},
+				PerpetualUpdates: []types.PerpetualUpdate{
+					{
+						PerpetualId:      uint32(3),
+						BigQuantumsDelta: big.NewInt(1_000_000_000), // 1 ISO
+					},
+				},
+				AssetUpdates: []types.AssetUpdate{
+					{
+						AssetId:          constants.BtcUsd.Id,
+						BigQuantumsDelta: big.NewInt(100_000_000), // 1 BTC
+					},
+				},
+			},
+			perpetuals: []perptypes.Perpetual{
+				constants.IsoUsd_IsolatedMarket,
+			},
+			expectedStateTransition: nil,
+			expectedErr:             types.ErrFailedToUpdateSubaccounts,
 		},
 	}
 
 	for name, tc := range tests {
 		t.Run(
 			name, func(t *testing.T) {
-				stateTransition := keeper.GetIsolatedPerpetualStateTransition(
-					tc.subaccountQuoteQuantums,
-					tc.isolatedPerpetualPosition,
-					tc.isolatedPerpetualUpdate,
+				stateTransition, err := keeper.GetIsolatedPerpetualStateTransition(
+					tc.settledUpdateWithUpdatedSubaccount,
+					tc.perpetuals,
 				)
-				require.Equal(t, tc.expectedStateTransition, stateTransition)
+				if tc.expectedErr != nil {
+					require.Error(t, tc.expectedErr, err)
+				} else {
+					require.NoError(t, err)
+					require.Equal(t, tc.expectedStateTransition, stateTransition)
+				}
 			},
 		)
 	}
diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go
index ac15715d7d..4ff5439290 100644
--- a/protocol/x/subaccounts/keeper/subaccount.go
+++ b/protocol/x/subaccounts/keeper/subaccount.go
@@ -206,12 +206,12 @@ func (k Keeper) getSettledUpdates(
 	updates []types.Update,
 	requireUniqueSubaccount bool,
 ) (
-	settledUpdates []settledUpdate,
+	settledUpdates []SettledUpdate,
 	subaccountIdToFundingPayments map[types.SubaccountId]map[uint32]dtypes.SerializableInt,
 	err error,
 ) {
 	var idToSettledSubaccount = make(map[types.SubaccountId]types.Subaccount)
-	settledUpdates = make([]settledUpdate, len(updates))
+	settledUpdates = make([]SettledUpdate, len(updates))
 	subaccountIdToFundingPayments = make(map[types.SubaccountId]map[uint32]dtypes.SerializableInt)
 
 	// Iterate over all updates and query the relevant `Subaccounts`.
@@ -236,7 +236,7 @@ func (k Keeper) getSettledUpdates(
 			subaccountIdToFundingPayments[u.SubaccountId] = fundingPayments
 		}
 
-		settledUpdate := settledUpdate{
+		settledUpdate := SettledUpdate{
 			SettledSubaccount: settledSubaccount,
 			AssetUpdates:      u.AssetUpdates,
 			PerpetualUpdates:  u.PerpetualUpdates,
@@ -284,7 +284,7 @@ func (k Keeper) UpdateSubaccounts(
 	}
 
 	allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx)
-	success, successPerUpdate, isolatedPerpetualPositionStateTransitions, err := k.internalCanUpdateSubaccounts(
+	success, successPerUpdate, err = k.internalCanUpdateSubaccounts(
 		ctx,
 		settledUpdates,
 		updateType,
@@ -312,14 +312,18 @@ func (k Keeper) UpdateSubaccounts(
 
 	// Transfer collateral between collateral pools for any isolated perpetual positions that changed
 	// state due to an update.
-	for i, stateTransition := range isolatedPerpetualPositionStateTransitions {
+	for _, settledUpdateWithUpdatedSubaccount := range settledUpdates {
+		// The subaccount in `settledUpdateWithUpdatedSubaccount` already has the perpetual updates
+		// and asset updates applied to it.
+		stateTransition, err := GetIsolatedPerpetualStateTransition(
+			settledUpdateWithUpdatedSubaccount,
+			allPerps,
+		)
+		if err != nil {
+			return false, nil, err
+		}
 		if err := k.transferCollateralForIsolatedPerpetual(
 			ctx,
-			// settledUpdateds[i].SettledSubaccount has had the asset / perpetual updates applied to it
-			// above, so it is now an updated subaccount that can be passed into the function to execute
-			// the collateral transfers for isolated perpetual positions being opened/closed due to the
-			// updates.
-			settledUpdates[i].SettledSubaccount,
 			stateTransition,
 		); err != nil {
 			return false, nil, err
@@ -404,7 +408,7 @@ func (k Keeper) CanUpdateSubaccounts(
 	}
 
 	allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx)
-	success, successPerUpdate, _, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, allPerps)
+	success, successPerUpdate, err = k.internalCanUpdateSubaccounts(ctx, settledUpdates, updateType, allPerps)
 	return success, successPerUpdate, err
 }
 
@@ -548,34 +552,29 @@ func checkPositionUpdatable(
 // 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.
-// Returns a `isolatedPerpetualPositionStateTransitions` value, which is a slice of
-// `isolatedPerpetualPositionStateTransition`.
-// These map to the updates and are used to indicate which of the updates led to an isolated
-// perpetual position being opened or closed.
 func (k Keeper) internalCanUpdateSubaccounts(
 	ctx sdk.Context,
-	settledUpdates []settledUpdate,
+	settledUpdates []SettledUpdate,
 	updateType types.UpdateType,
 	perpetuals []perptypes.Perpetual,
 ) (
 	success bool,
 	successPerUpdate []types.UpdateResult,
-	isolatedPerpetualPositionStateTransitions []*types.IsolatedPerpetualPositionStateTransition,
 	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, isolatedPerpetualPositionStateTransitions, err = k.processIsolatedSubaccountUpdates(
+	success, successPerUpdate, err = k.checkIsolatedSubaccountConstraints(
 		ctx,
 		settledUpdates,
 		perpetuals,
 	)
 	if err != nil {
-		return false, nil, nil, err
+		return false, nil, err
 	}
 	if !success {
-		return success, successPerUpdate, nil, nil
+		return success, successPerUpdate, nil
 	}
 
 	// Block all withdrawals and transfers if either of the following is true within the last
@@ -632,7 +631,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 				metrics.GetLabelForBoolValue(metrics.SubaccountsNegativeTncSubaccountSeen, negativeTncSubaccountSeen),
 				metrics.GetLabelForBoolValue(metrics.ChainOutageSeen, chainOutageSeen),
 			)
-			return success, successPerUpdate, nil, nil
+			return success, successPerUpdate, nil
 		}
 	}
 
@@ -646,7 +645,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		for _, perpUpdate := range u.PerpetualUpdates {
 			err := checkPositionUpdatable(ctx, k.perpetualsKeeper, perpUpdate)
 			if err != nil {
-				return false, nil, nil, err
+				return false, nil, err
 			}
 		}
 
@@ -654,7 +653,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		for _, assetUpdate := range u.AssetUpdates {
 			err := checkPositionUpdatable(ctx, k.assetsKeeper, assetUpdate)
 			if err != nil {
-				return false, nil, nil, err
+				return false, nil, err
 			}
 		}
 
@@ -664,7 +663,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 			bigNewMaintenanceMargin,
 			err := k.internalGetNetCollateralAndMarginRequirements(ctx, u)
 		if err != nil {
-			return false, nil, nil, err
+			return false, nil, err
 		}
 
 		var result = types.Success
@@ -673,13 +672,13 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		// We must now check if the state transition is valid.
 		if bigNewInitialMargin.Cmp(bigNewNetCollateral) > 0 {
 			// Get the current collateralization and margin requirements without the update applied.
-			emptyUpdate := settledUpdate{
+			emptyUpdate := SettledUpdate{
 				SettledSubaccount: u.SettledSubaccount,
 			}
 
 			bytes, err := proto.Marshal(u.SettledSubaccount.Id)
 			if err != nil {
-				return false, nil, nil, err
+				return false, nil, err
 			}
 			saKey := string(bytes)
 
@@ -693,7 +692,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 					emptyUpdate,
 				)
 				if err != nil {
-					return false, nil, nil, err
+					return false, nil, err
 				}
 			}
 
@@ -715,7 +714,7 @@ func (k Keeper) internalCanUpdateSubaccounts(
 		successPerUpdate[i] = result
 	}
 
-	return success, successPerUpdate, isolatedPerpetualPositionStateTransitions, nil
+	return success, successPerUpdate, nil
 }
 
 // IsValidStateTransitionForUndercollateralizedSubaccount returns an `UpdateResult`
@@ -812,7 +811,7 @@ func (k Keeper) GetNetCollateralAndMarginRequirements(
 		return nil, nil, nil, err
 	}
 
-	settledUpdate := settledUpdate{
+	settledUpdate := SettledUpdate{
 		SettledSubaccount: settledSubaccount,
 		AssetUpdates:      update.AssetUpdates,
 		PerpetualUpdates:  update.PerpetualUpdates,
@@ -837,7 +836,7 @@ func (k Keeper) GetNetCollateralAndMarginRequirements(
 // If two position updates reference the same position, an error is returned.
 func (k Keeper) internalGetNetCollateralAndMarginRequirements(
 	ctx sdk.Context,
-	settledUpdate settledUpdate,
+	settledUpdate SettledUpdate,
 ) (
 	bigNetCollateral *big.Int,
 	bigInitialMargin *big.Int,
diff --git a/protocol/x/subaccounts/keeper/subaccount_helper.go b/protocol/x/subaccounts/keeper/subaccount_helper.go
index 71721cb9a6..79b14fbc2e 100644
--- a/protocol/x/subaccounts/keeper/subaccount_helper.go
+++ b/protocol/x/subaccounts/keeper/subaccount_helper.go
@@ -12,7 +12,7 @@ import (
 // been updated. This will include any asset postions that were closed due to an update.
 // TODO(DEC-1295): look into reducing code duplication here using Generics+Reflect.
 func getUpdatedAssetPositions(
-	update settledUpdate,
+	update SettledUpdate,
 ) []*types.AssetPosition {
 	assetIdToPositionMap := make(map[uint32]*types.AssetPosition)
 	for _, assetPosition := range update.SettledSubaccount.AssetPositions {
@@ -53,7 +53,7 @@ func getUpdatedAssetPositions(
 // been updated. This will include any perpetual postions that were closed due to an update or that
 // received / paid out funding payments..
 func getUpdatedPerpetualPositions(
-	update settledUpdate,
+	update SettledUpdate,
 	fundingPayments map[uint32]dtypes.SerializableInt,
 ) []*types.PerpetualPosition {
 	perpetualIdToPositionMap := make(map[uint32]*types.PerpetualPosition)
@@ -102,7 +102,7 @@ func getUpdatedPerpetualPositions(
 // to reflect settledUpdate.PerpetualUpdates.
 // For newly created positions, use `perpIdToFundingIndex` map to populate the `FundingIndex` field.
 func UpdatePerpetualPositions(
-	settledUpdates []settledUpdate,
+	settledUpdates []SettledUpdate,
 	perpIdToFundingIndex map[uint32]dtypes.SerializableInt,
 ) {
 	// Apply the updates.
@@ -166,7 +166,7 @@ func UpdatePerpetualPositions(
 // For each settledUpdate in settledUpdates, updates its SettledSubaccount.AssetPositions
 // to reflect settledUpdate.AssetUpdates.
 func UpdateAssetPositions(
-	settledUpdates []settledUpdate,
+	settledUpdates []SettledUpdate,
 ) {
 	// Apply the updates.
 	for i, u := range settledUpdates {
diff --git a/protocol/x/subaccounts/keeper/update.go b/protocol/x/subaccounts/keeper/update.go
index 179139452a..30e0ad58f7 100644
--- a/protocol/x/subaccounts/keeper/update.go
+++ b/protocol/x/subaccounts/keeper/update.go
@@ -4,11 +4,11 @@ import (
 	"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
 )
 
-// settledUpdate is used internally in the subaccounts keeper to
+// SettledUpdate is used internally in the subaccounts keeper to
 // to specify changes to one or more `Subaccounts` (for example the
 // result of a trade, transfer, etc).
 // The subaccount is always in its settled state.
-type settledUpdate struct {
+type SettledUpdate struct {
 	// The `Subaccount` for which this update applies to, in its settled form.
 	SettledSubaccount types.Subaccount
 	// A list of changes to make to any `AssetPositions` in the `Subaccount`.
diff --git a/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go b/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go
index 9ea1604b8c..ce57aef490 100644
--- a/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go
+++ b/protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go
@@ -23,12 +23,13 @@ func (t PositionStateTransition) String() string {
 	return result
 }
 
-// Represents a state transition for an isolated perpetual position.
+// Represents a state transition for an isolated perpetual position in a subaccount.
 type IsolatedPerpetualPositionStateTransition struct {
-	PerpetualId uint32
+	SubaccountId *SubaccountId
+	PerpetualId  uint32
 	// TODO(DEC-715): Support non-USDC assets.
-	// Quote quantums position size of the subaccount that has a state change for an isolated perpetual.
-	QuoteQuantumsBeforeUpdate *big.Int
+	// Quote quantums of collateral to transfer as a result of the state transition.
+	QuoteQuantums *big.Int
 	// The state transition that occurred for the isolated perpetual positions.
 	Transition PositionStateTransition
 }

From 72e208260285f2f78f60fc9c4ae8044c58beee20 Mon Sep 17 00:00:00 2001
From: Vincent Chau <99756290+vincentwschau@users.noreply.github.com>
Date: Thu, 21 Mar 2024 18:56:59 -0400
Subject: [PATCH 6/6] Address nits.

---
 .../subaccounts/keeper/isolated_subaccount.go | 39 ++++++++++++++++---
 protocol/x/subaccounts/keeper/subaccount.go   | 14 ++-----
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/protocol/x/subaccounts/keeper/isolated_subaccount.go b/protocol/x/subaccounts/keeper/isolated_subaccount.go
index 26899a1dc9..d302817788 100644
--- a/protocol/x/subaccounts/keeper/isolated_subaccount.go
+++ b/protocol/x/subaccounts/keeper/isolated_subaccount.go
@@ -248,6 +248,11 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 		return nil
 	}
 
+	// If there are zero quantums to transfer, don't transfer collateral.
+	if stateTransition.QuoteQuantums.Sign() == 0 {
+		return nil
+	}
+
 	isolatedCollateralPoolAddr, err := k.GetCollateralPoolFromPerpetualId(ctx, stateTransition.PerpetualId)
 	if err != nil {
 		return err
@@ -276,11 +281,6 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 		)
 	}
 
-	// If there are zero quantums to transfer, don't transfer collateral.
-	if stateTransition.QuoteQuantums.Sign() == 0 {
-		return nil
-	}
-
 	// Invalid to transfer negative quantums. This should already be caught by collateralization
 	// checks as well.
 	if stateTransition.QuoteQuantums.Sign() == -1 {
@@ -317,6 +317,35 @@ func (k *Keeper) transferCollateralForIsolatedPerpetual(
 	return nil
 }
 
+// computeAndExecuteCollateralTransfer computes collateral transfers resulting from updates to
+// a subaccount and executes the collateral transfer using `x/bank`.`
+// The input `settledUpdate` must have an updated subaccount (`settledUpdate.SettledSubaccount`),
+// so all the updates must have been applied already to the subaccount.
+// Note: This uses the `x/bank` keeper and modifies `x/bank` state.
+func (k *Keeper) computeAndExecuteCollateralTransfer(
+	ctx sdk.Context,
+	settledUpdateWithUpdatedSubaccount SettledUpdate,
+	perpetuals []perptypes.Perpetual,
+) error {
+	// The subaccount in `settledUpdateWithUpdatedSubaccount` already has the perpetual updates
+	// and asset updates applied to it.
+	stateTransition, err := GetIsolatedPerpetualStateTransition(
+		settledUpdateWithUpdatedSubaccount,
+		perpetuals,
+	)
+	if err != nil {
+		return err
+	}
+	if err := k.transferCollateralForIsolatedPerpetual(
+		ctx,
+		stateTransition,
+	); err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func getPerpIdToMarketTypeMap(
 	perpetuals []perptypes.Perpetual,
 ) map[uint32]perptypes.PerpetualMarketType {
diff --git a/protocol/x/subaccounts/keeper/subaccount.go b/protocol/x/subaccounts/keeper/subaccount.go
index 4ff5439290..55a1b5fb14 100644
--- a/protocol/x/subaccounts/keeper/subaccount.go
+++ b/protocol/x/subaccounts/keeper/subaccount.go
@@ -313,18 +313,12 @@ func (k Keeper) UpdateSubaccounts(
 	// Transfer collateral between collateral pools for any isolated perpetual positions that changed
 	// state due to an update.
 	for _, settledUpdateWithUpdatedSubaccount := range settledUpdates {
-		// The subaccount in `settledUpdateWithUpdatedSubaccount` already has the perpetual updates
-		// and asset updates applied to it.
-		stateTransition, err := GetIsolatedPerpetualStateTransition(
+		if err := k.computeAndExecuteCollateralTransfer(
+			ctx,
+			// The subaccount in `settledUpdateWithUpdatedSubaccount` already has the perpetual updates
+			// and asset updates applied to it.
 			settledUpdateWithUpdatedSubaccount,
 			allPerps,
-		)
-		if err != nil {
-			return false, nil, err
-		}
-		if err := k.transferCollateralForIsolatedPerpetual(
-			ctx,
-			stateTransition,
 		); err != nil {
 			return false, nil, err
 		}