Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: deterministically fetch perp info from state #2341

Merged
merged 4 commits into from
Sep 25, 2024
Merged

Fix: deterministically fetch perp info from state #2341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c44650
order the perp ids so the iteration is determistic
ttl33 Sep 25, 2024
5cac752
nit:update position size to match the name
ttl33 Sep 25, 2024
732d173
fix lint
ttl33 Sep 25, 2024
51b0cf5
rename
ttl33 Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions protocol/testutil/constants/positions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ var (
big.NewInt(0),
big.NewInt(0),
)
// SOL positions
PerpetualPosition_OneSolLong = *testutil.CreateSinglePerpetualPosition(
2,
big.NewInt(10_000_000_000), // 0.1 SOL
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Change to = 1 SOL to match name of the constant.

big.NewInt(0),
big.NewInt(0),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding more SOL position constants for comprehensive testing.

While the addition of PerpetualPosition_OneSolLong is valuable, consider expanding the SOL-related test constants to match the coverage provided for other cryptocurrencies like BTC and ETH. This could include:

  1. A short SOL position
  2. Different sizes of SOL positions (e.g., 1 SOL, 0.01 SOL)
  3. Positions with non-zero funding values

Here's an example of additional constants you might want to add:

PerpetualPosition_OneSolShort = *testutil.CreateSinglePerpetualPosition(
    2,
    big.NewInt(-10_000_000_000), // -0.1 SOL
    big.NewInt(0),
    big.NewInt(0),
)

PerpetualPosition_OneSolLong = *testutil.CreateSinglePerpetualPosition(
    2,
    big.NewInt(100_000_000_000), // 1 SOL
    big.NewInt(0),
    big.NewInt(0),
)

PerpetualPosition_OneSolLong_PositiveFunding = *testutil.CreateSinglePerpetualPosition(
    2,
    big.NewInt(10_000_000_000), // 0.1 SOL
    big.NewInt(1000000), // Some positive funding
    big.NewInt(0),
)

These additional constants would provide a more comprehensive set of test scenarios for SOL-related functionality.

// Long position for arbitrary isolated market
PerpetualPosition_OneISOLong = *testutil.CreateSinglePerpetualPosition(
3,
Expand Down
19 changes: 11 additions & 8 deletions protocol/x/subaccounts/keeper/subaccount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,30 +770,33 @@ func (k Keeper) GetAllRelevantPerpetuals(
perptypes.PerpInfos,
error,
) {
subaccountIds := make(map[types.SubaccountId]struct{})
perpIds := make(map[uint32]struct{})
subaccountIdsMap := make(map[types.SubaccountId]struct{})
perpIdsMap := make(map[uint32]struct{})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could be named as sets and not maps.


// Add all relevant perpetuals in every update.
for _, update := range updates {
// If this subaccount has not been processed already, get all of its existing perpetuals.
if _, exists := subaccountIds[update.SubaccountId]; !exists {
if _, exists := subaccountIdsMap[update.SubaccountId]; !exists {
sa := k.GetSubaccount(ctx, update.SubaccountId)
for _, postition := range sa.PerpetualPositions {
perpIds[postition.PerpetualId] = struct{}{}
perpIdsMap[postition.PerpetualId] = struct{}{}
}
subaccountIds[update.SubaccountId] = struct{}{}
subaccountIdsMap[update.SubaccountId] = struct{}{}
}

// Add all perpetuals in the update.
for _, perpUpdate := range update.PerpetualUpdates {
perpIds[perpUpdate.GetId()] = struct{}{}
perpIdsMap[perpUpdate.GetId()] = struct{}{}
}
}

// Important: Sort the perpIds to ensure determinism!
perpIdsOrdered := lib.GetSortedKeys[lib.Sortable[uint32]](perpIdsMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/non-blocking: sortedPerpIds to match convention elsewhere in code?


// Get all perpetual information from state.
ltCache := make(map[uint32]perptypes.LiquidityTier)
perpInfos := make(perptypes.PerpInfos, len(perpIds))
for perpId := range perpIds {
perpInfos := make(perptypes.PerpInfos, len(perpIdsOrdered))
for _, perpId := range perpIdsOrdered {
perpetual, price, err := k.perpetualsKeeper.GetPerpetualAndMarketPrice(ctx, perpId)
if err != nil {
return nil, err
Expand Down
111 changes: 111 additions & 0 deletions protocol/x/subaccounts/keeper/subaccount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/dydxprotocol/v4-chain/protocol/lib"

storetypes "cosmossdk.io/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/dydxprotocol/v4-chain/protocol/dtypes"
Expand Down Expand Up @@ -6019,3 +6020,113 @@ func TestGetNetCollateralAndMarginRequirements(t *testing.T) {
})
}
}

func TestGetAllRelevantPerpetuals_Deterministic(t *testing.T) {
tests := map[string]struct {
// state
perpetuals []perptypes.Perpetual

// subaccount state
assetPositions []*types.AssetPosition
perpetualPositions []*types.PerpetualPosition

// updates
assetUpdates []types.AssetUpdate
perpetualUpdates []types.PerpetualUpdate

// expectations
expectedNetCollateral *big.Int
expectedInitialMargin *big.Int
expectedMaintenanceMargin *big.Int
expectedErr error
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Not needed.

}{
"Gas used is deterministic when erroring on gas usage": {
assetPositions: testutil.CreateUsdcAssetPositions(big.NewInt(10_000_000_001)), // $10,000.000001
perpetuals: []perptypes.Perpetual{
constants.BtcUsd_NoMarginRequirement,
constants.EthUsd_NoMarginRequirement,
constants.SolUsd_20PercentInitial_10PercentMaintenance,
},
perpetualPositions: []*types.PerpetualPosition{
&constants.PerpetualPosition_OneBTCLong,
&constants.PerpetualPosition_OneTenthEthLong,
&constants.PerpetualPosition_OneSolLong,
},
assetUpdates: []types.AssetUpdate{
{
AssetId: constants.Usdc.Id,
BigQuantumsDelta: big.NewInt(1_000_000), // +1 USDC
},
},
perpetualUpdates: []types.PerpetualUpdate{
{
PerpetualId: uint32(0),
BigQuantumsDelta: big.NewInt(-200_000_000), // -2 BTC
},
{
PerpetualId: uint32(1),
BigQuantumsDelta: big.NewInt(250_000_000), // .25 ETH
},
{
PerpetualId: uint32(2),
BigQuantumsDelta: big.NewInt(500_000_000), // .005 SOL
},
},
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
// Setup.
ctx, keeper, pricesKeeper, perpetualsKeeper, _, _, assetsKeeper, _, _, _, _ := keepertest.SubaccountsKeepers(
t,
true,
)
keepertest.CreateTestMarkets(t, ctx, pricesKeeper)
keepertest.CreateTestLiquidityTiers(t, ctx, perpetualsKeeper)
keepertest.CreateTestPerpetuals(t, ctx, perpetualsKeeper)
for _, p := range tc.perpetuals {
perpetualsKeeper.SetPerpetualForTest(ctx, p)
}
require.NoError(t, keepertest.CreateUsdcAsset(ctx, assetsKeeper))

subaccount := createNSubaccount(keeper, ctx, 1, big.NewInt(1_000))[0]
subaccount.PerpetualPositions = tc.perpetualPositions
subaccount.AssetPositions = tc.assetPositions
keeper.SetSubaccount(ctx, subaccount)
subaccountId := *subaccount.Id

update := types.Update{
SubaccountId: subaccountId,
AssetUpdates: tc.assetUpdates,
PerpetualUpdates: tc.perpetualUpdates,
}

// Execute.
gasUsedBefore := ctx.GasMeter().GasConsumed()
_, err := keeper.GetAllRelevantPerpetuals(ctx, []types.Update{update})
require.NoError(t, err)
gasUsedAfter := ctx.GasMeter().GasConsumed()

gasUsed := uint64(0)
for range 100 { // run 100 times since it's highly unlikely gas usage is deterministic over 100 times if there's non-determinism.
// divide by 2 so that the state read fails at least second to last time.
ctxWithLimitedGas := ctx.WithGasMeter(storetypes.NewGasMeter((gasUsedAfter - gasUsedBefore) / 2))

require.PanicsWithValue(
t,
storetypes.ErrorOutOfGas{Descriptor: "ReadFlat"},
func() {
keeper.GetAllRelevantPerpetuals(ctxWithLimitedGas, []types.Update{update})
},
)

if gasUsed == 0 {
gasUsed = ctxWithLimitedGas.GasMeter().GasConsumed()
require.Greater(t, gasUsed, uint64(0))
} else {
require.Equal(t, gasUsed, ctxWithLimitedGas.GasMeter().GasConsumed(), "Gas usage when out of gas is not deterministic")
}
}
})
}
}
Loading