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

[TRA-70] Add state migrations for isolated markets #1155

Merged
merged 6 commits into from
Mar 8, 2024
Merged

[TRA-70] Add state migrations for isolated markets #1155

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a78c68b
wip upgrade handler
shrenujb Mar 6, 2024
7d63b0a
Merge branch 'main' into tra70
shrenujb Mar 7, 2024
142696a
fix lint and gen mocks
shrenujb Mar 7, 2024
321628d
address comments, add tests
shrenujb Mar 8, 2024
f7d05e3
add restriction to only set type if current is UNSPECIFIED
shrenujb Mar 8, 2024
19a71ad
parametrize all set perp market type tests
shrenujb Mar 8, 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
4 changes: 3 additions & 1 deletion protocol/app/upgrades.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ package app
import (
"fmt"

v5_0_0 "github.com/dydxprotocol/v4-chain/protocol/app/upgrades/v5.0.0"

upgradetypes "cosmossdk.io/x/upgrade/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/app/upgrades"
v5_0_0 "github.com/dydxprotocol/v4-chain/protocol/app/upgrades/v5.0.0"
)

var (
Expand All @@ -29,6 +30,7 @@ func (app *App) setupUpgradeHandlers() {
v5_0_0.CreateUpgradeHandler(
app.ModuleManager,
app.configurator,
app.PerpetualsKeeper,
),
)
}
Expand Down
25 changes: 25 additions & 0 deletions protocol/app/upgrades/v5.0.0/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,44 @@ import (
"context"
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"

perptypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types"

upgradetypes "cosmossdk.io/x/upgrade/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/dydxprotocol/v4-chain/protocol/lib"
)

// Set all existing perpetuals to cross market type
func perpetualsUpgrade(
ctx sdk.Context,
perpetualsKeeper perptypes.PerpetualsKeeper,
) {
// Set all perpetuals to cross market type
perpetuals := perpetualsKeeper.GetAllPerpetuals(ctx)
for _, p := range perpetuals {
_, err := perpetualsKeeper.SetPerpetualMarketType(
ctx, p.GetId(),
perptypes.PerpetualMarketType_PERPETUAL_MARKET_TYPE_CROSS)
if err != nil {
panic(fmt.Sprintf("failed to set perpetual market type for perpetual %d: %s", p.GetId(), err))
}
}
}

func CreateUpgradeHandler(
mm *module.Manager,
configurator module.Configurator,
perpetualsKeeper perptypes.PerpetualsKeeper,
) upgradetypes.UpgradeHandler {
return func(ctx context.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {
sdkCtx := lib.UnwrapSDKContext(ctx, "app/upgrades")
sdkCtx.Logger().Info(fmt.Sprintf("Running %s Upgrade...", UpgradeName))

// Set all perpetuals to cross market type
perpetualsUpgrade(sdkCtx, perpetualsKeeper)

Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

The call to perpetualsUpgrade within CreateUpgradeHandler does not handle potential errors. Consider capturing and handling any errors returned by perpetualsUpgrade to ensure that the upgrade process can respond appropriately to failures.

// TODO(TRA-93): Initialize `x/vault` module.

return mm.RunMigrations(ctx, configurator, vm)
Expand Down
40 changes: 40 additions & 0 deletions protocol/mocks/PerpetualsKeeper.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 38 additions & 4 deletions protocol/x/perpetuals/keeper/perpetual.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (k Keeper) CreatePerpetual(
}

// Store the new perpetual.
k.setPerpetual(ctx, perpetual)
k.SetPerpetual(ctx, perpetual)

k.SetEmptyPremiumSamples(ctx)
k.SetEmptyPremiumVotes(ctx)
Expand Down Expand Up @@ -165,7 +165,7 @@ func (k Keeper) ModifyPerpetual(
}

// Store the modified perpetual.
k.setPerpetual(ctx, perpetual)
k.SetPerpetual(ctx, perpetual)

// Emit indexer event.
k.GetIndexerEventManager().AddTxnEvent(
Expand All @@ -186,6 +186,40 @@ func (k Keeper) ModifyPerpetual(
return perpetual, nil
}

func (k Keeper) SetPerpetualMarketType(
shrenujb marked this conversation as resolved.
Show resolved Hide resolved
ctx sdk.Context,
perpetualId uint32,
marketType types.PerpetualMarketType,
) (types.Perpetual, error) {
if marketType == types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_UNSPECIFIED {
return types.Perpetual{}, errorsmod.Wrap(
types.ErrInvalidMarketType,
fmt.Sprintf("invalid market type %v for perpetual %d", marketType, perpetualId),
)
}

// Get perpetual.
perpetual, err := k.GetPerpetual(ctx, perpetualId)
if err != nil {
return perpetual, err
}

if perpetual.Params.MarketType != types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_UNSPECIFIED {
return types.Perpetual{}, errorsmod.Wrap(
types.ErrInvalidMarketType,
fmt.Sprintf("perpetual %d already has market type %v", perpetualId, perpetual.Params.MarketType),
)
}

// Modify perpetual.
perpetual.Params.MarketType = marketType

// Store the modified perpetual.
k.SetPerpetual(ctx, perpetual)

return perpetual, nil
}
Comment on lines +189 to +221
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of SetPerpetualMarketType function has several aspects to consider:

  • Correctness and Logic: The function correctly checks for an unspecified market type and returns an error if encountered. However, the logic to prevent changing an already specified market type (line 207-212) seems to contradict the PR objectives and previous discussions. The condition checks if the market type is not unspecified and returns an error, which means it's impossible to change the market type once set. This contradicts the intention to migrate perpetuals to a specific market type and the discussion about adding a restriction and then removing it in future releases.
  • Performance: The function performs a GetPerpetual call to fetch the current state of the perpetual before updating it. This is necessary and does not pose a performance issue given the context.
  • Security/PII Leakage: No PII or sensitive data handling is involved in this function.
  • Error Handling: Errors are correctly wrapped with context, providing clarity on the operation that failed.
  • Maintainability: The function is straightforward and maintains consistency with the existing codebase structure. However, the logic might need revisiting based on the intended functionality regarding market type updates.

Consider revisiting the implementation logic concerning the ability to update the market type of a perpetual, especially in light of the discussions and objectives outlined in the PR and previous comments.


// GetPerpetual returns a perpetual from its id.
func (k Keeper) GetPerpetual(
ctx sdk.Context,
Expand Down Expand Up @@ -1181,7 +1215,7 @@ func (k Keeper) ModifyFundingIndex(
bigFundingIndex.Add(bigFundingIndex, bigFundingIndexDelta)

perpetual.FundingIndex = dtypes.NewIntFromBigInt(bigFundingIndex)
k.setPerpetual(ctx, perpetual)
k.SetPerpetual(ctx, perpetual)
return nil
}

Expand All @@ -1207,7 +1241,7 @@ func (k Keeper) SetEmptyPremiumVotes(
)
}

func (k Keeper) setPerpetual(
func (k Keeper) SetPerpetual(
ctx sdk.Context,
perpetual types.Perpetual,
) {
Expand Down
63 changes: 63 additions & 0 deletions protocol/x/perpetuals/keeper/perpetual_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,69 @@ func TestModifyPerpetual_Failure(t *testing.T) {
}
}

func TestSetPerpetualMarketType_Success(t *testing.T) {
vincentwschau marked this conversation as resolved.
Show resolved Hide resolved
pc := keepertest.PerpetualsKeepers(t)
// Create liquidity tiers and perpetuals,
perp := keepertest.CreateLiquidityTiersAndNPerpetuals(t, pc.Ctx, pc.PerpetualsKeeper, pc.PricesKeeper, 1)[0]
perp.Params.MarketType = types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_UNSPECIFIED
pc.PerpetualsKeeper.SetPerpetual(pc.Ctx, perp)

_, err := pc.PerpetualsKeeper.SetPerpetualMarketType(
pc.Ctx,
perp.Params.Id,
types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_CROSS,
)
require.NoError(t, err)

rst, err := pc.PerpetualsKeeper.GetPerpetual(
pc.Ctx,
perp.Params.Id,
)
require.NoError(t, err)
require.Equal(
t,
types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_CROSS,
rst.Params.MarketType,
)
}

func TestSetPerpetualMarketType_Failure_SettingToUnspecified(t *testing.T) {
pc := keepertest.PerpetualsKeepers(t)
// Create liquidity tiers and perpetuals,
perp := keepertest.CreateLiquidityTiersAndNPerpetuals(t, pc.Ctx, pc.PerpetualsKeeper, pc.PricesKeeper, 1)[0]

_, err := pc.PerpetualsKeeper.SetPerpetualMarketType(
pc.Ctx,
perp.Params.Id,
types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_UNSPECIFIED,
)
expectedErr := errorsmod.Wrap(
types.ErrInvalidMarketType,
fmt.Sprintf(
"invalid market type %v for perpetual %d",
types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_UNSPECIFIED, perp.GetId(),
),
)
require.EqualError(t, err, expectedErr.Error())
}

func TestSetPerpetualMarketType_Failure_MarketTypeAlreadySet(t *testing.T) {
pc := keepertest.PerpetualsKeepers(t)
// Create liquidity tiers and perpetuals,
perp := keepertest.CreateLiquidityTiersAndNPerpetuals(t, pc.Ctx, pc.PerpetualsKeeper, pc.PricesKeeper, 1)[0]

_, err := pc.PerpetualsKeeper.SetPerpetualMarketType(
pc.Ctx,
perp.Params.Id,
types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_ISOLATED,
)
expectedErr := errorsmod.Wrap(
types.ErrInvalidMarketType,
fmt.Sprintf("perpetual %d already has market type %v", perp.GetId(), perp.Params.MarketType),
)
require.EqualError(t, err, expectedErr.Error())
}

func TestGetPerpetual_Success(t *testing.T) {
pc := keepertest.PerpetualsKeepers(t)
// Create liquidity tiers and perpetuals,
Expand Down
8 changes: 8 additions & 0 deletions protocol/x/perpetuals/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,12 @@ type PerpetualsKeeper interface {
ctx sdk.Context,
params Params,
) error
SetPerpetualMarketType(
ctx sdk.Context,
id uint32,
marketType PerpetualMarketType,
) (Perpetual, error)
GetAllPerpetuals(
ctx sdk.Context,
) []Perpetual
}
Loading