feat(core): add migrations registering#19370
Conversation
WalkthroughWalkthroughThe recent updates across various components introduce a consistent shift towards utilizing Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
Bot
added
C:x/gov
C:x/distribution
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 42
Configuration used: .coderabbit.yml
Files selected for processing (41)
- core/appmodule/migrations.go (1 hunks)
- core/appmodule/module.go (2 hunks)
- docs/build/building-modules/13-upgrade.md (1 hunks)
- runtime/services/autocli.go (1 hunks)
- types/module/configurator.go (4 hunks)
- types/module/core_module.go (1 hunks)
- types/module/module.go (1 hunks)
- x/auth/keeper/migrations.go (2 hunks)
- x/auth/module.go (3 hunks)
- x/authz/keeper/migrations.go (2 hunks)
- x/authz/module/module.go (3 hunks)
- x/bank/keeper/migrations.go (2 hunks)
- x/bank/module.go (3 hunks)
- x/circuit/module.go (3 hunks)
- x/crisis/keeper/migrator.go (2 hunks)
- x/crisis/module.go (3 hunks)
- x/distribution/keeper/migrations.go (2 hunks)
- x/distribution/migrations/v4/migrate.go (1 hunks)
- x/distribution/migrations/v4/migrate_funds.go (2 hunks)
- x/distribution/module.go (3 hunks)
- x/feegrant/keeper/migrations.go (2 hunks)
- x/feegrant/module/module.go (3 hunks)
- x/gov/keeper/migrations.go (2 hunks)
- x/gov/migrations/v5/store.go (2 hunks)
- x/gov/migrations/v6/store.go (1 hunks)
- x/gov/module.go (3 hunks)
- x/group/keeper/migrations.go (1 hunks)
- x/group/module/module.go (4 hunks)
- x/mint/keeper/migrator.go (2 hunks)
- x/mint/module.go (3 hunks)
- x/params/module.go (3 hunks)
- x/protocolpool/module.go (3 hunks)
- x/slashing/keeper/migrations.go (2 hunks)
- x/slashing/migrations/v4/migrate.go (7 hunks)
- x/slashing/module.go (3 hunks)
- x/staking/keeper/migrations.go (2 hunks)
- x/staking/migrations/v5/migrations_test.go (5 hunks)
- x/staking/migrations/v5/store.go (3 hunks)
- x/staking/module.go (3 hunks)
- x/upgrade/keeper/migrations.go (4 hunks)
- x/upgrade/module.go (3 hunks)
Files skipped from review due to trivial changes (1)
- x/group/keeper/migrations.go
Additional comments: 38
runtime/services/autocli.go (1)
- 116-116: Looks good as a placeholder. Ensure future implementations are added as needed.
x/staking/migrations/v5/migrations_test.go (5)
- 13-13: Adding logging to tests is a good practice for improved diagnostics.
- 32-32: Proper use of
log.NewTestLogger(t)for enhanced test output.- 68-68: Correctly passing
loggertoMigrateStorefor logging within migration tests.- 88-88: Appropriate addition of
loggerfor logging in migration tests.- 104-104: Properly using
loggerinMigrateStorecall within test.x/slashing/migrations/v4/migrate.go (7)
- 4-4: Correctly updating imports to include
context.- 20-20: Properly updating function signature to use
context.Context.- 62-62: Appropriate update to function signature for
context.Context.- 82-82: Correctly updating function signature to use
context.Context.- 104-104: Properly updating function signature to use
context.Context.- 119-119: Appropriate update to function signature for
context.Context.- 128-128: Correctly updating function signature to use
context.Context.x/upgrade/module.go (4)
- 10-10: Correctly adding
google.golang.org/grpcimport for GRPC service registration.- 37-38: Properly implementing
appmodule.HasServicesandappmodule.HasMigrationsinterfaces.- 89-94: Correctly refactoring
RegisterServicesto usegrpc.ServiceRegistrarand adding error handling.- 96-106: Appropriately adding
RegisterMigrationsmethod for handling module migrations.x/crisis/module.go (4)
- 11-11: Correctly adding
google.golang.org/grpcimport for GRPC service registration.- 33-34: Properly implementing
appmodule.HasServicesandappmodule.HasMigrationsinterfaces.- 114-118: Correctly refactoring
RegisterServicesto usegrpc.ServiceRegistrarand adding error handling.- 120-127: Appropriately adding
RegisterMigrationsmethod for handling module migrations.x/feegrant/module/module.go (4)
- 10-10: Correctly adding
google.golang.org/grpcimport for GRPC service registration.- 31-32: Properly implementing
appmodule.HasServicesandappmodule.HasMigrationsinterfaces.- 49-55: Correctly refactoring
RegisterServicesto usegrpc.ServiceRegistrarand adding error handling.- 57-64: Appropriately adding
RegisterMigrationsmethod for handling module migrations.types/module/core_module.go (1)
- 204-209: Correctly adding conditional migration registration for modules implementing
HasMigrations.x/authz/module/module.go (4)
- 10-10: Correctly adding
google.golang.org/grpcimport for GRPC service registration.- 33-34: Properly implementing
appmodule.HasServicesandappmodule.HasMigrationsinterfaces.- 47-53: Correctly refactoring
RegisterServicesto usegrpc.ServiceRegistrarand adding error handling.- 55-61: Appropriately adding
RegisterMigrationsmethod for handling module migrations.x/group/module/module.go (4)
- 10-10: Correctly adding
google.golang.org/grpcimport for GRPC service registration.- 38-39: Properly implementing
appmodule.HasServicesandappmodule.HasMigrationsinterfaces.- 129-135: Correctly refactoring
RegisterServicesto usegrpc.ServiceRegistrarand adding error handling.- 137-143: Commented out
RegisterMigrationsmethod indicates a placeholder for future migration logic. Ensure implementation is added as needed.x/staking/module.go (4)
- 11-11: Correctly adding
google.golang.org/grpcimport for GRPC service registration.- 40-41: Properly implementing
appmodule.HasServicesandappmodule.HasMigrationsinterfaces.- 125-130: Correctly refactoring
RegisterServicesto usegrpc.ServiceRegistrarand adding error handling.- 132-147: Appropriately adding
RegisterMigrationsmethod for handling module migrations.
|
|
||
| import ( | ||
| v2 "cosmossdk.io/x/authz/migrations/v2" | ||
| "context" |
There was a problem hiding this comment.
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.
| "context" | |
| // Package keeper provides functionality to manage and store authz grants. | |
| "context" |
|
|
||
| // Migrate1to2 migrates from version 1 to 2. | ||
| func (m Migrator) Migrate1to2(ctx sdk.Context) error { | ||
| func (m Migrator) Migrate1to2(ctx context.Context) error { |
There was a problem hiding this comment.
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.
| func (m Migrator) Migrate1to2(ctx context.Context) error { | |
| func (m Migrator) Migrate1to2(ctx sdk.Context) error { |
|
|
||
| import ( | ||
| v2 "cosmossdk.io/x/feegrant/migrations/v2" | ||
| "context" |
There was a problem hiding this comment.
Missing import documentation.
+ // Package keeper provides functionality to manage and store feegrant 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.
| "context" | |
| // Package keeper provides functionality to manage and store feegrant grants. | |
| "context" |
|
|
||
| // Migrate1to2 migrates from version 1 to 2. | ||
| func (m Migrator) Migrate1to2(ctx sdk.Context) error { | ||
| func (m Migrator) Migrate1to2(ctx context.Context) error { |
There was a problem hiding this comment.
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.
| func (m Migrator) Migrate1to2(ctx context.Context) error { | |
| func (m Migrator) Migrate1to2(ctx sdk.Context) error { |
|
|
||
| // Migrate1to2 migrates from version 1 to 2. | ||
| func (m Migrator) Migrate1to2(ctx sdk.Context) error { | ||
| func (m Migrator) Migrate1to2(ctx context.Context) error { |
There was a problem hiding this comment.
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.
| func (m Migrator) Migrate1to2(ctx context.Context) error { | |
| func (m Migrator) Migrate1to2(ctx sdk.Context) error { |
|
|
||
| // 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 { |
There was a problem hiding this comment.
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.
| func (m Migrator) Migrate2to3(ctx context.Context) error { | |
| func (m Migrator) Migrate2to3(ctx sdk.Context) error { |
|
|
||
| // 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 { |
There was a problem hiding this comment.
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.
| func (m Migrator) Migrate3to4(ctx context.Context) error { | |
| func (m Migrator) Migrate3to4(ctx sdk.Context) error { |
| @@ -0,0 +1,16 @@ | |||
| package appmodule | |||
There was a problem hiding this comment.
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.
| package appmodule | |
| // Package appmodule provides interfaces and structures for managing application modules. | |
| package appmodule |
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: .coderabbit.yml
Files selected for processing (4)
- CHANGELOG.md (2 hunks)
- UPGRADING.md (2 hunks)
- core/CHANGELOG.md (1 hunks)
- x/group/module/module.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/group/module/module.go
Additional comments: 8
core/CHANGELOG.md (1)
- 41-49: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Ensure the "Unreleased" section follows the guiding principles mentioned at the beginning of the file, especially regarding the grouping of the same types of changes and displaying the release date once the version is released.
UPGRADING.md (7)
- 15-16: The merging of
GasConsumptionDecoratorandIncreaseSequenceDecoratorintoSigVerificationDecoratorsimplifies the AnteHandler setup. Ensure that the merged functionality is thoroughly tested to prevent any unintended side effects on transaction processing.- 20-23: The refactoring of the
clientpackage to use address codecs is a significant change. Verify that all instances where address codecs are required have been updated to include them in theclient.Context. This is crucial for maintaining compatibility with the new address handling approach.- 38-38: The introduction of the
appmodule.Environmentinterface is a notable enhancement for fetching services. Ensure that all modules that require access to services are updated to use this interface, and verify that the transition fromsdk.UnwrapContext(ctx)is seamless and well-documented.- 46-46: Support for unordered transactions introduces a significant change in transaction processing. It's important to ensure that the implementation is compatible with existing transaction handling mechanisms and that the potential for transactions to be executed in any order is clearly communicated to developers and users.
- 127-127: The removal of old module migrations and the requirement to migrate to v0.50 before upgrading to v0.51 could potentially impact developers who have not completed the necessary migrations. It's crucial to provide clear guidance and support for this migration process to ensure a smooth transition.
- 131-131: The introduction of the Core API in v0.47 is a significant enhancement for module development. Ensure that the documentation and examples provided are sufficient for developers to effectively utilize the new interfaces and that the transition from
sdk.Contextis well-supported.- 133-133: The migration of dependency injection functions to
cosmossdk.io/depinject/appconfigrequires attention to ensure that all modules using dependency injection are updated accordingly. Verify that the migration is straightforward and well-documented to minimize disruption for developers.
| ### Deprecated | ||
|
|
||
| * (simapp) [#19146](https://github.com/cosmos/cosmos-sdk/pull/19146) Replace `--v` CLI option with `--validator-count`/`-n`. | ||
| * (module) [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Depcreate ` module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServicecs` instead from Core API. | ||
|
|
There was a problem hiding this comment.
Typo in "Depcreate". It should be "Deprecate".
- Depcreate ` module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServicecs` instead from Core API.
+ Deprecate `module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServices` instead from Core API.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.
| ### Deprecated | |
| * (simapp) [#19146](https://github.com/cosmos/cosmos-sdk/pull/19146) Replace `--v` CLI option with `--validator-count`/`-n`. | |
| * (module) [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Depcreate ` module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServicecs` instead from Core API. | |
| ### Deprecated | |
| * (simapp) [#19146](https://github.com/cosmos/cosmos-sdk/pull/19146) Replace `--v` CLI option with `--validator-count`/`-n`. | |
| * (module) [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Deprecate `module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServices` instead from Core API. | |
| * [#18379](https://github.com/cosmos/cosmos-sdk/pull/18379) Add branch service. | ||
| * [#18457](https://github.com/cosmos/cosmos-sdk/pull/18457) Add branch.ExecuteWithGasLimit. | ||
| * [#19041](https://github.com/cosmos/cosmos-sdk/pull/19041) Add `appmodule.Environment` interface to fetch different services | ||
| * [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Add `appmodule.Migrations` interface to handle migrations |
There was a problem hiding this comment.
The entry for PR #19370 lacks a brief description of what the appmodule.Migrations interface does or its impact. Consider adding a concise explanation to provide more context to the reader.
| * [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Add `appmodule.Migrations` interface to handle migrations | ||
|
|
||
| ### API Breaking | ||
| ### API Breaking Changes |
There was a problem hiding this comment.
The URL in the entry for PR #18861 has a typo: httpes should be https.
- * [#18861](httpes://github.com/cosmos/cosmos-sdk/pull/18861) Moved `coin.ParseCoin` to `client/v2/internal`.
+ * [#18861](https://github.com/cosmos/cosmos-sdk/pull/18861) Moved `coin.ParseCoin` to `client/v2/internal`.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.
| ### API Breaking Changes | |
| ### API Breaking Changes | |
| * [#18861](https://github.com/cosmos/cosmos-sdk/pull/18861) Moved `coin.ParseCoin` to `client/v2/internal`. |
The entry for PR #19041 under "API Breaking Changes" is duplicated. It's already mentioned under "Features". Consider removing the duplicate or clarifying if there are distinct aspects of the PR that belong to both categories.
| ### Deprecated | ||
|
|
||
| * (simapp) [#19146](https://github.com/cosmos/cosmos-sdk/pull/19146) Replace `--v` CLI option with `--validator-count`/`-n`. | ||
| * (module) [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Depcreate ` module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServicecs` instead from Core API. |
There was a problem hiding this comment.
Typo in "HasServicecs". It should be "HasServices".
- use `appmodule.HasMigrations` and `appmodule.HasServicecs` instead from Core API.
+ use `appmodule.HasMigrations` and `appmodule.HasServices` instead from Core API.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.
| * (module) [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Depcreate ` module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServicecs` instead from Core API. | |
| * (module) [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Depcreate ` module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServices` instead from Core API. |
| ```diff | ||
| clientCtx = clientCtx. | ||
| + WithAddressCodec(addressCodec). | ||
| + WithValidatorAddressCodec(validatorAddressCodec). | ||
| + WithConsensusAddressCodec(consensusAddressCodec) | ||
| ``` |
There was a problem hiding this comment.
The code snippet provided for updating the client.Context with address codecs is helpful. However, ensure that the snippet is placed in the correct context within the documentation and that it's clear to developers where and how to implement these changes in their applications.
+ Ensure this snippet is accompanied by a clear explanation of where to implement these changes in the application code.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.
| ```diff | |
| clientCtx = clientCtx. | |
| + WithAddressCodec(addressCodec). | |
| + WithValidatorAddressCodec(validatorAddressCodec). | |
| + WithConsensusAddressCodec(consensusAddressCodec) | |
| ``` | |
| clientCtx = clientCtx. | |
| WithAddressCodec(addressCodec). | |
| WithValidatorAddressCodec(validatorAddressCodec). | |
| WithConsensusAddressCodec(consensusAddressCodec) |
| ```go | ||
| app.CircuitKeeper = circuitkeeper.NewKeeper(runtime.NewEnvironment((keys[circuittypes.StoreKey]), nil), appCodec, authtypes.NewModuleAddress(govtypes.ModuleName).String(), app.AuthKeeper.AddressCodec()) | ||
| ``` |
There was a problem hiding this comment.
The example provided for using the appmodule.Environment interface with the CircuitKeeper is valuable. Recommend adding more examples or a detailed explanation to help developers understand how to implement this interface across different modules and services.
+ Consider adding additional examples or a more detailed guide on implementing the `appmodule.Environment` interface across various 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.
| ```go | |
| app.CircuitKeeper = circuitkeeper.NewKeeper(runtime.NewEnvironment((keys[circuittypes.StoreKey]), nil), appCodec, authtypes.NewModuleAddress(govtypes.ModuleName).String(), app.AuthKeeper.AddressCodec()) | |
| ``` | |
| ```go | |
| app.CircuitKeeper = circuitkeeper.NewKeeper(runtime.NewEnvironment((keys[circuittypes.StoreKey]), nil), appCodec, authtypes.NewModuleAddress(govtypes.ModuleName).String(), app.AuthKeeper.AddressCodec()) |
</details>
<!-- suggestion_end -->
<!-- This is an auto-generated comment by CodeRabbit -->
| Old module migrations have been removed. It is required to migrate to v0.50 prior to upgrading to v0.51 for not missing any module migrations. | ||
|
|
||
| ### Modules | ||
| ##### Core API |
There was a problem hiding this comment.
Clarify the requirement for migrating to v0.50 before upgrading to v0.51 to ensure developers understand the importance of completing module migrations in the correct order. Providing additional resources or guidance on this process could be beneficial.
+ Add more detailed instructions or resources to assist developers with the migration process from v0.50 to v0.51.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.
| ##### Core API | |
| ##### Core API | |
| Add more detailed instructions or resources to assist developers with the migration process from v0.50 to v0.51. |
| Core API has been introduces for modules in v0.47. With the deprecation of `sdk.Context`, we strongly recommend to use the `cosmossdk.io/core/appmodule` interfaces for the modules. This will allow the modules to work out of the box with server/v2 and baseapp, as well as limit their dependencies on the SDK. | ||
|
|
||
| ##### Dependency Injection | ||
| ##### Dependency Injection |
There was a problem hiding this comment.
Highlight the significance of updating imports for dependency injection functions and ensure that developers are aware of the changes to cosmossdk.io/depinject/appconfig. Providing examples of updated imports could help clarify the required changes.
+ Include examples of updated imports for dependency injection functions to guide developers through the migration process.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.
| ##### Dependency Injection | |
| ##### Dependency Injection | |
| Include examples of updated imports for dependency injection functions to guide developers through the migration process. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- CHANGELOG.md (2 hunks)
- types/module/configurator.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- types/module/configurator.go
| 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 { |
There was a problem hiding this comment.
Interesting we could drop grpc here with an interface. Out of scope for this pr
5 participants
Description
Ref: #19330
On main we just need to create an interface and migrate the modules migrations functions to use context directly and environement.
On the server_modular branch, once synced with main, we should implement the
MigrationRegistrarin runtime/v2 own module manager, which should takes this feedback into account: #15120Group cannot be migrated just yet: #19382
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Refactor
sdk.Contextto standard Go'scontext.Context, standardizing the way context is managed across the system.Tests