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

Jump to bottom

[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 3 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
1 change: 1 addition & 0 deletions protocol/app/upgrades/v5.0.0/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const (
var Upgrade = upgrades.Upgrade{
UpgradeName: UpgradeName,
StoreUpgrades: store.StoreUpgrades{

shrenujb marked this conversation as resolved.
Show resolved Hide resolved
Added: []string{
vaulttypes.StoreKey,
},
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.

27 changes: 27 additions & 0 deletions protocol/x/perpetuals/keeper/perpetual.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,33 @@ 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("market type %v", marketType),
)
}

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

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

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

return perpetual, nil
}
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The SetPerpetualMarketType function correctly implements the logic to update the market type of a perpetual. However, there are a few areas that could be improved for better maintainability and error handling:

  • Error Handling: When the market type is PERPETUAL_MARKET_TYPE_UNSPECIFIED, the function returns a wrapped error indicating an invalid market type. It's good practice to include the perpetual ID in the error message for easier debugging.
  • Logging: Consider adding logging for critical operations, such as updating the market type of a perpetual. This can be helpful for auditing and troubleshooting.
  • Validation: The function assumes that the provided marketType is either valid or unspecified. If there are more market types added in the future, this function might silently fail or behave unexpectedly. It might be beneficial to validate against all known market types explicitly.
func (k Keeper) SetPerpetualMarketType(
	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("market type %v", marketType),
+			fmt.Sprintf("invalid market type %v for perpetual %d", marketType, perpetualId),
		)
	}
	...
}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func (k Keeper) SetPerpetualMarketType(
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("market type %v", marketType),
)
}
// Get perpetual.
perpetual, err := k.GetPerpetual(ctx, perpetualId)
if err != nil {
return perpetual, err
}
// Modify perpetual.
perpetual.Params.MarketType = marketType
// Store the modified perpetual.
k.setPerpetual(ctx, perpetual)
return perpetual, nil
}
func (k Keeper) SetPerpetualMarketType(
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
}
// Modify perpetual.
perpetual.Params.MarketType = marketType
// Store the modified perpetual.
k.setPerpetual(ctx, perpetual)
return perpetual, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

@shrenujb: I'm glad you found the suggestion helpful! If you have any more questions or need further assistance, feel free to ask. 😊

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


// GetPerpetual returns a perpetual from its id.
func (k Keeper) GetPerpetual(
ctx sdk.Context,
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