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.

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

feat(core): add migrations registering #19370

Merged
merged 14 commits into from
Feb 9, 2024
16 changes: 16 additions & 0 deletions core/appmodule/migrations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package appmodule
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing package documentation.

+ // Package appmodule provides interfaces and structures for managing application modules.

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
package appmodule
// Package appmodule provides interfaces and structures for managing application modules.
package appmodule


import "context"

type MigrationRegistrar interface {
// Register registers an in-place store migration for a module. The
// handler is a migration script to perform in-place migrations from version
// `fromVersion` to version `fromVersion+1`.
//
// EACH TIME a module's ConsensusVersion increments, a new migration MUST
// be registered using this function. If a migration handler is missing for
// a particular function, the upgrade logic (see RunMigrations function)
// will panic. If the ConsensusVersion bump does not introduce any store
// changes, then a no-op function must be registered here.
Register(moduleName string, fromVersion uint64, handler func(context.Context) error) error
Comment on lines +5 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

The MigrationRegistrar interface documentation should clarify the expected behavior when fromVersion is not sequential.

+ // It is the caller's responsibility to ensure migrations are registered in a sequential manner without gaps.

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
type MigrationRegistrar interface {
// Register registers an in-place store migration for a module. The
// handler is a migration script to perform in-place migrations from version
// `fromVersion` to version `fromVersion+1`.
//
// EACH TIME a module's ConsensusVersion increments, a new migration MUST
// be registered using this function. If a migration handler is missing for
// a particular function, the upgrade logic (see RunMigrations function)
// will panic. If the ConsensusVersion bump does not introduce any store
// changes, then a no-op function must be registered here.
Register(moduleName string, fromVersion uint64, handler func(context.Context) error) error
type MigrationRegistrar interface {
// Register registers an in-place store migration for a module. The
// handler is a migration script to perform in-place migrations from version
// `fromVersion` to version `fromVersion+1`.
//
// EACH TIME a module's ConsensusVersion increments, a new migration MUST
// be registered using this function. If a migration handler is missing for
// a particular function, the upgrade logic (see RunMigrations function)
// will panic. If the ConsensusVersion bump does not introduce any store
// changes, then a no-op function must be registered here.
// It is the caller's responsibility to ensure migrations are registered in a sequential manner without gaps.
Register(moduleName string, fromVersion uint64, handler func(context.Context) error) error

}
26 changes: 17 additions & 9 deletions core/appmodule/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,12 @@ type HasServices interface {
RegisterServices(grpc.ServiceRegistrar) error
}

// HasPrepareCheckState is an extension interface that contains information about the AppModule
// and PrepareCheckState.
type HasPrepareCheckState interface {
// HasMigrations is the extension interface that modules should implement to register migrations.
type HasMigrations interface {
AppModule
PrepareCheckState(context.Context) error
}

// HasPrecommit is an extension interface that contains information about the AppModule and Precommit.
type HasPrecommit interface {
AppModule
Precommit(context.Context) error
// RegisterMigrations registers the module's migrations with the app's migrator.
RegisterMigrations(MigrationRegistrar) error
}

// ResponsePreBlock represents the response from the PreBlock method.
Expand Down Expand Up @@ -86,3 +81,16 @@ type HasEndBlocker interface {
// a block.
EndBlock(context.Context) error
}

// HasPrepareCheckState is an extension interface that contains information about the AppModule
// and PrepareCheckState.
type HasPrepareCheckState interface {
AppModule
PrepareCheckState(context.Context) error
}

// HasPrecommit is an extension interface that contains information about the AppModule and Precommit.
type HasPrecommit interface {
AppModule
Precommit(context.Context) error
}
2 changes: 2 additions & 0 deletions docs/build/building-modules/13-upgrade.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ Since these migrations are functions that need access to a Keeper's store, use a
https://github.com/cosmos/cosmos-sdk/blob/v0.50.0-alpha.0/x/bank/keeper/migrations.go
```

<!-- TODO v0.51: explain the new way with core -->

## Writing Migration Scripts

To define the functionality that takes place during an upgrade, write a migration script and place the functions in a `migrations/` directory. For example, to write migration scripts for the bank module, place the functions in `x/bank/migrations/`. Use the recommended naming convention for these functions. For example, `v2bank` is the script that migrates the package `x/bank/migrations/v2`:
Expand Down
2 changes: 2 additions & 0 deletions runtime/services/autocli.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ func (a *autocliConfigurator) RegisterMigration(string, uint64, module.Migration
return nil
}

func (a *autocliConfigurator) Register(string, uint64, func(context.Context) error) error { return nil }

func (a *autocliConfigurator) RegisterService(sd *grpc.ServiceDesc, ss interface{}) {
if a.registryCache == nil {
a.registryCache, a.err = proto.MergedRegistry()
Expand Down
15 changes: 15 additions & 0 deletions types/module/configurator.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package module

import (
"context"
"fmt"

"github.com/cosmos/gogoproto/grpc"
Expand All @@ -20,6 +21,8 @@ import (
// their services in the RegisterServices method. It is designed to eventually
// support module object capabilities isolation as described in
// https://github.com/cosmos/cosmos-sdk/issues/7093
// Deprecated: The Configurator is deprecated.
// Preferably use core services for registering msg/query server and migrations.
Comment on lines +24 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecation of Configurator interface is not accompanied by an alternative implementation or migration path for existing users.

type Configurator interface {
grpc.Server

Expand All @@ -45,6 +48,10 @@ type Configurator interface {
// will panic. If the ConsensusVersion bump does not introduce any store
// changes, then a no-op function must be registered here.
RegisterMigration(moduleName string, fromVersion uint64, handler MigrationHandler) error

// Register registers an in-place store migration for a module.
// It permits to register modules migrations that have migrated to serverv2 but still be compatible with baseapp.
Register(moduleName string, fromVersion uint64, handler func(context.Context) error) error
Comment on lines +52 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

The Register method lacks documentation on how it differs from RegisterMigration and under what circumstances it should be used.

}

type configurator struct {
Expand Down Expand Up @@ -119,6 +126,14 @@ func (c *configurator) RegisterMigration(moduleName string, fromVersion uint64,
return nil
}

// Register implements the Configurator.Register method
// It permis to register modules migrations that have migrated to serverv2 but still be compatible with baseapp.
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
func (c *configurator) Register(moduleName string, fromVersion uint64, handler func(context.Context) error) error {
return c.RegisterMigration(moduleName, fromVersion, func(sdkCtx sdk.Context) error {
return handler(sdkCtx)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The Register method's implementation directly calls RegisterMigration without adapting the context from context.Context to sdk.Context, potentially leading to confusion or misuse.


// runModuleMigrations runs all in-place store migrations for one given module from a
// version to another version.
func (c *configurator) runModuleMigrations(ctx sdk.Context, moduleName string, fromVersion, toVersion uint64) error {
Expand Down
7 changes: 7 additions & 0 deletions types/module/core_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ func (c coreAppModuleBasicAdaptor) RegisterServices(cfg Configurator) {
panic(err)
}
}

if module, ok := c.module.(appmodule.HasMigrations); ok {
err := module.RegisterMigrations(cfg)
if err != nil {
panic(err)
}
}
}

func (c coreAppModuleBasicAdaptor) IsOnePerModuleType() {}
Expand Down
7 changes: 7 additions & 0 deletions types/module/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,13 @@ func (m *Manager) RegisterServices(cfg Configurator) error {
}
}

if module, ok := module.(appmodule.HasMigrations); ok {
err := module.RegisterMigrations(cfg)
if err != nil {
return err
}
}
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

if cfg.Error() != nil {
return cfg.Error()
}
Expand Down
19 changes: 9 additions & 10 deletions x/auth/keeper/migrations.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package keeper

import (
"github.com/cosmos/gogoproto/grpc"
"context"

v5 "cosmossdk.io/x/auth/migrations/v5"
"cosmossdk.io/x/auth/types"
Expand All @@ -11,46 +11,45 @@ import (

// Migrator is a struct for handling in-place store migrations.
type Migrator struct {
keeper AccountKeeper
queryServer grpc.Server
keeper AccountKeeper
}

// NewMigrator returns a new Migrator.
func NewMigrator(keeper AccountKeeper, queryServer grpc.Server) Migrator {
return Migrator{keeper: keeper, queryServer: queryServer}
func NewMigrator(keeper AccountKeeper) Migrator {
return Migrator{keeper: keeper}
}

// Migrate1to2 migrates from version 1 to 2.
func (m Migrator) Migrate1to2(ctx sdk.Context) error {
func (m Migrator) Migrate1to2(ctx context.Context) error {
return nil
}

// Migrate2to3 migrates from consensus version 2 to version 3. Specifically, for each account
// we index the account's ID to their address.
func (m Migrator) Migrate2to3(ctx sdk.Context) error {
func (m Migrator) Migrate2to3(ctx context.Context) error {
return nil
}

// Migrate3to4 migrates the x/auth module state from the consensus version 3 to
// version 4. Specifically, it takes the parameters that are currently stored
// and managed by the x/params modules and stores them directly into the x/auth
// module state.
func (m Migrator) Migrate3to4(ctx sdk.Context) error {
func (m Migrator) Migrate3to4(ctx context.Context) error {
return nil
}

// Migrate4To5 migrates the x/auth module state from the consensus version 4 to 5.
// It migrates the GlobalAccountNumber from being a protobuf defined value to a
// big-endian encoded uint64, it also migrates it to use a more canonical prefix.
func (m Migrator) Migrate4To5(ctx sdk.Context) error {
func (m Migrator) Migrate4To5(ctx context.Context) error {
return v5.Migrate(ctx, m.keeper.storeService, m.keeper.AccountNumber)
}

// V45_SetAccount implements V45_SetAccount
// set the account without map to accAddr to accNumber.
//
// NOTE: This is used for testing purposes only.
func (m Migrator) V45SetAccount(ctx sdk.Context, acc sdk.AccountI) error {
func (m Migrator) V45SetAccount(ctx context.Context, acc sdk.AccountI) error {
addr := acc.GetAddress()
store := m.keeper.storeService.OpenKVStore(ctx)

Expand Down
40 changes: 23 additions & 17 deletions x/auth/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"

gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime"
"google.golang.org/grpc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing import documentation for "google.golang.org/grpc".


"cosmossdk.io/core/address"
"cosmossdk.io/core/appmodule"
Expand All @@ -30,9 +31,10 @@ var (
_ module.AppModuleBasic = AppModule{}
_ module.AppModuleSimulation = AppModule{}
_ module.HasGenesis = AppModule{}
_ module.HasServices = AppModule{}

_ appmodule.AppModule = AppModule{}
_ appmodule.AppModule = AppModule{}
_ appmodule.HasServices = AppModule{}
_ appmodule.HasMigrations = AppModule{}
Comment on lines +35 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Interface assertions for appmodule.AppModule, appmodule.HasServices, and appmodule.HasMigrations are not accompanied by actual implementation of these interfaces.

)

// AppModuleBasic defines the basic application module used by the auth module.
Expand Down Expand Up @@ -98,27 +100,31 @@ func NewAppModule(cdc codec.Codec, accountKeeper keeper.AccountKeeper, randGenAc
}
}

// RegisterServices registers a GRPC query service to respond to the
// module-specific GRPC queries.
func (am AppModule) RegisterServices(cfg module.Configurator) {
types.RegisterMsgServer(cfg.MsgServer(), keeper.NewMsgServerImpl(am.accountKeeper))
types.RegisterQueryServer(cfg.QueryServer(), keeper.NewQueryServer(am.accountKeeper))
// RegisterServices registers module services.
func (am AppModule) RegisterServices(registrar grpc.ServiceRegistrar) error {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting we could drop grpc here with an interface. Out of scope for this pr

types.RegisterMsgServer(registrar, keeper.NewMsgServerImpl(am.accountKeeper))
types.RegisterQueryServer(registrar, keeper.NewQueryServer(am.accountKeeper))

m := keeper.NewMigrator(am.accountKeeper, cfg.QueryServer())
if err := cfg.RegisterMigration(types.ModuleName, 1, m.Migrate1to2); err != nil {
panic(fmt.Sprintf("failed to migrate x/%s from version 1 to 2: %v", types.ModuleName, err))
}
return nil
}
Comment on lines +103 to +109
Copy link
Contributor

Choose a reason for hiding this comment

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

RegisterServices method lacks error handling for service registration failures.


if err := cfg.RegisterMigration(types.ModuleName, 2, m.Migrate2to3); err != nil {
panic(fmt.Sprintf("failed to migrate x/%s from version 2 to 3: %v", types.ModuleName, err))
func (am AppModule) RegisterMigrations(mr appmodule.MigrationRegistrar) error {
m := keeper.NewMigrator(am.accountKeeper)
if err := mr.Register(types.ModuleName, 1, m.Migrate1to2); err != nil {
return fmt.Errorf("failed to migrate x/%s from version 1 to 2: %w", types.ModuleName, err)
}

if err := cfg.RegisterMigration(types.ModuleName, 3, m.Migrate3to4); err != nil {
panic(fmt.Sprintf("failed to migrate x/%s from version 3 to 4: %v", types.ModuleName, err))
if err := mr.Register(types.ModuleName, 2, m.Migrate2to3); err != nil {
return fmt.Errorf("failed to migrate x/%s from version 2 to 3: %w", types.ModuleName, err)
}
if err := cfg.RegisterMigration(types.ModuleName, 4, m.Migrate4To5); err != nil {
panic(fmt.Sprintf("failed to migrate x/%s from version 4 to 5", types.ModuleName))
if err := mr.Register(types.ModuleName, 3, m.Migrate3to4); err != nil {
return fmt.Errorf("failed to migrate x/%s from version 3 to 4: %w", types.ModuleName, err)
}
if err := mr.Register(types.ModuleName, 4, m.Migrate4To5); err != nil {
return fmt.Errorf("failed to migrate x/%s from version 4 to 5: %w", types.ModuleName, err)
}

return nil
Comment on lines +111 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

RegisterMigrations method does not check for potential errors from NewMigrator or the validity of the migration handler before registration.

}

// InitGenesis performs genesis initialization for the auth module. It returns
Expand Down
6 changes: 3 additions & 3 deletions x/authz/keeper/migrations.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package keeper

import (
v2 "cosmossdk.io/x/authz/migrations/v2"
"context"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing import documentation.

+ // Package keeper provides functionality to manage and store authz grants.

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
"context"
// Package keeper provides functionality to manage and store authz grants.
"context"


sdk "github.com/cosmos/cosmos-sdk/types"
v2 "cosmossdk.io/x/authz/migrations/v2"
)

// Migrator is a struct for handling in-place store migrations.
Expand All @@ -17,6 +17,6 @@ func NewMigrator(keeper Keeper) Migrator {
}

// Migrate1to2 migrates from version 1 to 2.
func (m Migrator) Migrate1to2(ctx sdk.Context) error {
func (m Migrator) Migrate1to2(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect context usage for Migrate1to2. Ensure sdk.Context is used for compatibility with the SDK's context management.

- func (m Migrator) Migrate1to2(ctx context.Context) error {
+ func (m Migrator) Migrate1to2(ctx sdk.Context) error {

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 (m Migrator) Migrate1to2(ctx context.Context) error {
func (m Migrator) Migrate1to2(ctx sdk.Context) error {

return v2.MigrateStore(ctx, m.keeper.storeService, m.keeper.cdc)
}
25 changes: 16 additions & 9 deletions x/authz/module/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime"
"github.com/spf13/cobra"
"google.golang.org/grpc"

"cosmossdk.io/core/appmodule"
"cosmossdk.io/errors"
Expand All @@ -26,10 +27,11 @@ var (
_ module.AppModuleBasic = AppModule{}
_ module.AppModuleSimulation = AppModule{}
_ module.HasGenesis = AppModule{}
_ module.HasServices = AppModule{}

_ appmodule.AppModule = AppModule{}
_ appmodule.HasBeginBlocker = AppModule{}
_ appmodule.HasServices = AppModule{}
_ appmodule.HasMigrations = AppModule{}
)

// AppModuleBasic defines the basic application module used by the authz module.
Expand All @@ -42,16 +44,21 @@ func (AppModuleBasic) Name() string {
return authz.ModuleName
}

// RegisterServices registers a gRPC query service to respond to the
// module-specific gRPC queries.
func (am AppModule) RegisterServices(cfg module.Configurator) {
authz.RegisterQueryServer(cfg.QueryServer(), am.keeper)
authz.RegisterMsgServer(cfg.MsgServer(), am.keeper)
// RegisterServices registers module services.
func (am AppModule) RegisterServices(registrar grpc.ServiceRegistrar) error {
authz.RegisterQueryServer(registrar, am.keeper)
authz.RegisterMsgServer(registrar, am.keeper)

return nil
}

func (am AppModule) RegisterMigrations(mr appmodule.MigrationRegistrar) error {
m := keeper.NewMigrator(am.keeper)
err := cfg.RegisterMigration(authz.ModuleName, 1, m.Migrate1to2)
if err != nil {
panic(fmt.Sprintf("failed to migrate x/%s from version 1 to 2: %v", authz.ModuleName, err))
if err := mr.Register(authz.ModuleName, 1, m.Migrate1to2); err != nil {
return fmt.Errorf("failed to migrate x/%s from version 1 to 2: %w", authz.ModuleName, err)
}

return nil
}

// RegisterLegacyAminoCodec registers the authz module's types for the given codec.
Expand Down
10 changes: 4 additions & 6 deletions x/bank/keeper/migrations.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package keeper

import (
sdk "github.com/cosmos/cosmos-sdk/types"
)
import "context"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing import documentation.

+ // Package keeper provides functionality to manage and store bank transactions and balances.

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
import "context"
// Package keeper provides functionality to manage and store bank transactions and balances.
import "context"


// Migrator is a struct for handling in-place store migrations.
type Migrator struct {
Expand All @@ -15,16 +13,16 @@ func NewMigrator(keeper BaseKeeper) Migrator {
}

// Migrate1to2 migrates from version 1 to 2.
func (m Migrator) Migrate1to2(ctx sdk.Context) error {
func (m Migrator) Migrate1to2(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect context usage for Migrate1to2. Use sdk.Context for SDK compatibility.

- func (m Migrator) Migrate1to2(ctx context.Context) error {
+ func (m Migrator) Migrate1to2(ctx sdk.Context) error {

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 (m Migrator) Migrate1to2(ctx context.Context) error {
func (m Migrator) Migrate1to2(ctx sdk.Context) error {

return nil
}

// Migrate2to3 migrates x/bank storage from version 2 to 3.
func (m Migrator) Migrate2to3(ctx sdk.Context) error {
func (m Migrator) Migrate2to3(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect context usage for Migrate2to3. Use sdk.Context for SDK compatibility.

- func (m Migrator) Migrate2to3(ctx context.Context) error {
+ func (m Migrator) Migrate2to3(ctx sdk.Context) error {

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 (m Migrator) Migrate2to3(ctx context.Context) error {
func (m Migrator) Migrate2to3(ctx sdk.Context) error {

return nil
}

// Migrate3to4 migrates x/bank storage from version 3 to 4.
func (m Migrator) Migrate3to4(ctx sdk.Context) error {
func (m Migrator) Migrate3to4(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect context usage for Migrate3to4. Use sdk.Context for SDK compatibility.

- func (m Migrator) Migrate3to4(ctx context.Context) error {
+ func (m Migrator) Migrate3to4(ctx sdk.Context) error {

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 (m Migrator) Migrate3to4(ctx context.Context) error {
func (m Migrator) Migrate3to4(ctx sdk.Context) error {

return nil
}
Loading
Loading