refactor(x/genutil,server): add export functions to x/gentutil #18303
Conversation
WalkthroughThis update introduces significant changes to the CLI commands and the migration process in the Cosmos SDK. The changes include reorganization of CLI commands, introduction of in-place store migrations, and modifications to function calls and test functions. The update also includes changes to the way the database is opened and the addition of new functionalities to the Changes
Poem
TipsChat with CodeRabbit Bot (
|
github-actions
bot
added
C:CLI
C:x/genutil
julienrbrt
requested review from
facundomedica and
tac0turtle
and removed request for
alexanderbez and
atheeshp
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (14)
- CHANGELOG.md (1 hunks)
- docs/architecture/adr-041-in-place-store-migrations.md (2 hunks)
- server/cmt_cmds.go (1 hunks)
- server/constructors_test.go (1 hunks)
- server/rollback.go (1 hunks)
- server/start.go (6 hunks)
- server/util.go (3 hunks)
- simapp/simd/cmd/commands.go (2 hunks)
- tools/cosmovisor/process.go (1 hunks)
- x/genutil/README.md (2 hunks)
- x/genutil/client/cli/commands.go (2 hunks)
- x/genutil/client/cli/export.go (5 hunks)
- x/genutil/client/cli/export_test.go (3 hunks)
- x/genutil/client/cli/init_test.go (2 hunks)
Files skipped from review due to trivial changes (5)
- server/constructors_test.go
- tools/cosmovisor/process.go
- x/genutil/client/cli/export.go
- x/genutil/client/cli/export_test.go
- x/genutil/client/cli/init_test.go
Additional comments: 22
CHANGELOG.md (1)
- 187-193: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [190-194]
The changes in the CLI commands are well documented. Ensure that all users are informed about these breaking changes to avoid confusion and potential issues. Also, consider providing examples of the new command usage for better clarity.
server/rollback.go (1)
- 27-33: The function
OpenDBis used here instead ofopenDB. Ensure that theOpenDBfunction is correctly defined and exported, and that it has the same functionality as the previousopenDBfunction. Also, verify that the change in function signature does not affect the rest of the codebase.server/util.go (2)
- 344-349: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [323-349]
The
ExportCmdcommand is removed from the list of commands added torootCmd. Ensure that this command is added elsewhere in the codebase to maintain functionality.
- 454-458: The
openDBfunction has been renamed toOpenDB. Ensure that all calls to this function throughout the codebase have been updated to match the new name.docs/architecture/adr-041-in-place-store-migrations.md (2)
14-20: The context provided here is clear and gives a good understanding of the current state of the system and the problems it presents.
147-161: The consequences of the proposed changes are well outlined, including potential issues that could arise if module developers do not correctly track consensus-breaking changes. It's good to see that the Cosmos SDK will continue to support JSON migrations, providing flexibility for developers.
server/cmt_cmds.go (1)
- 372-378: The
openDBfunction has been replaced withOpenDB. Ensure that theOpenDBfunction has the same functionality as theopenDBfunction or that any differences are accounted for in the code. Also, ensure that theOpenDBfunction is properly handling any errors that may occur when opening the database.simapp/simd/cmd/commands.go (3)
54-54: Ensure that all calls to
server.AddCommandsthroughout the codebase have been updated to match the new signature.59-59: Ensure that all calls to
genesisCommandthroughout the codebase have been updated to match the new signature.71-71: Ensure that all calls to
genesisCommandthroughout the codebase have been updated to match the new signature.x/genutil/README.md (3)
12-15: The addition of the new functionality "Application state export into a genesis file" is well documented. Ensure that the implementation aligns with the documentation.
88-90: This warning is important and should be clearly visible to the users. It helps to prevent potential issues when validating a genesis from a previous version of the application.
92-105: The documentation for the new
exportcommand is clear and concise. It explains the purpose of the command and how to use it, including the available flags. Ensure that the actual implementation of the command aligns with this documentation.x/genutil/client/cli/commands.go (3)
16-18: The function signature of
Commandshas been updated to include a new parameterappExportof typeservertypes.AppExporter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.22-24: The function signature of
CommandsWithCustomMigrationMaphas been updated to include a new parameterappExportof typeservertypes.AppExporter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.38-38: The
ExportCmdfunction is now being called with theappExportparameter. Ensure that theExportCmdfunction has been updated to handle this new parameter correctly.server/start.go (6)
30-32: The new import
"cosmossdk.io/log"has been added. Ensure that this package is available and correctly installed in your environment.117-119: The function
opts.DBOpenerhas been changed fromopenDBtoOpenDB. Make sure that theOpenDBfunction is defined and accessible in the current scope.438-444: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [441-446]
The comment added to the
getGenDocProviderfunction provides a clear explanation of its purpose. This is a good practice for code readability and maintainability.
453-457: The function
setupTraceWriterhas been renamed toSetupTraceWriterand now takes alogger log.Loggerparameter in addition to thesvrCtxandtraceWriterFileparameters. The function also returns a cleanup function. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.467-469: The error handling here is appropriate. If the trace writer fails to close, an error message is logged. This is a good practice for error handling.
627-633: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [630-634]
The function
startAppnow callsSetupTraceWriterwith thesvrCtx.LoggerandsvrCtx.Viper.GetString(flagTraceStore)parameters. Ensure that theSetupTraceWriterfunction is defined and accessible in the current scope.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (2 hunks)
Additional comments: 2
CHANGELOG.md (2)
- 79-85: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [82-86]
The changes in the API are well documented. However, it would be helpful to provide more context or examples for the changes, especially for those who are not familiar with the codebase. For instance, what does it mean that "Gov Hooks now returns error and are 'blocking' if they fail"? What are the implications of this change? Also, what is the impact of removing
MsgDeposit,MsgRedelegateExec, andMsgUnbondExecdue to AutoCLI migration?
- 191-194: The CLI breaking changes are also well documented. However, similar to the API changes, it would be beneficial to provide more context or examples for these changes. For instance, what is the impact of moving
appd exportwith other genesis commands? How does the change in the order of arguments forappd tx vesting create-vesting-accountaffect its usage? What are the implications of the change in behavior forappd tx distribution withdraw-rewards?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (2 hunks)
- tests/e2e/genutil/export_test.go (4 hunks)
Additional comments: 6
CHANGELOG.md (2)
- 79-85: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [82-86]
The changes in the API are significant and could potentially break existing integrations. Ensure that all dependencies and integrations are updated to reflect these changes. Also, the change in the behavior of Gov Hooks could affect the application flow, so it's important to verify that the application behaves as expected after these changes.
- 188-194: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [191-195]
The changes in the CLI commands could potentially break existing scripts or automated tasks that use these commands. Make sure to update all scripts and tasks to use the new command syntax. Also, the change in the behavior of the
appd tx distribution withdraw-rewardscommand could affect the expected results, so it's important to verify that the command behaves as expected after these changes.tests/e2e/genutil/export_test.go (4)
1-7: The package name has been changed from
server_testtogenutil_test. This change is fine as long as it doesn't conflict with any other packages in the project.27-32: The import statement
genutilclihas been added. This is fine as long as the package is used in the code.77-90: The test cases have been updated to use the new flag names
--heightand--for-zero-heightinstead ofserver.FlagHeightandserver.FlagForZeroHeightrespectively. This is a good change as it makes the test cases more readable and less dependent on the server package.200-203: The function
ExportCmdhas been modified to usegenutilcli.ExportCmdinstead ofserver.ExportCmd. This is a good change as it consolidates the export functionality into thegenutilclipackage.
Description
Add exports functions x/genutil. Genutil manages the genesis and exports to a genesis, so it consolidates x/genutil functionalities and simplifies server.
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Bug Fixes:
Chores:
Please note that the provided summary of changes does not include any specific bug fixes or chores. If there are any bug fixes or chores that were not mentioned in the summary, please provide the details so that they can be included in the release notes.