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

refactor(modules): adopt appmodulev2.Hasgenesis #19627

Merged
merged 28 commits into from
Mar 11, 2024
Merged

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented Mar 3, 2024

Description

adopt has genesis from appmodulev2


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...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers 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...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features
    • Introduced a new MsgBurn message type in the Bank module for burning coins.
  • Bug Fixes
    • Enhanced error handling in various modules, including bank and authz, to improve reliability and control flow.
  • Documentation
    • Updated module manager documentation to reflect changes in function signatures and deprecated content.
  • Refactor
    • Simplified genesis state validation and initialization processes across multiple modules by removing unnecessary parameters.
    • Streamlined the handling of genesis data in the ORM model and various modules by adjusting method signatures and struct fields.
  • Chores
    • Updated dependencies, notably the github.com/cosmos/iavl module, to newer versions for improved stability and performance.

Copy link
Contributor

coderabbitai bot commented Mar 3, 2024

Walkthrough

Walkthrough

The changes primarily focus on enhancing modularity and reducing dependencies across various modules of the Cosmos SDK. This includes updating method signatures to utilize internal codecs, refining error handling, and adjusting genesis-related functionalities to streamline the initialization and export processes. Additionally, there's an emphasis on updating documentation and tests to align with these modifications, aiming for a more cohesive and flexible framework.

Changes

Files Change Summary
simapp/app.go, x/auth/module.go, x/authz/keeper/genesis_test.go, x/genutil/depinject.go, x/genutil/module.go, runtime/app.go, simapp/simd/cmd/testnet.go, simapp/export.go, x/bank/keeper/genesis.go, x/bank/keeper/genesis_test.go Updated method signatures, enhanced error handling, and removed unnecessary parameters to streamline module interactions.
docs/build/building-modules/01-module-manager.md, CHANGELOG.md Documentation updated to reflect changes in module manager and types module, indicating modifications in interfaces and function parameters.
go.mod, go.sum, store/go.sum Dependency updates including a version bump for github.com/cosmos/iavl and adjustments in go.sum files for consistency.
types/module/core_module.go, types/module/module.go, types/module/module_test.go, orm/model/ormdb/module.go, testutil/mock/types_mock_appmodule.go, testutil/mock/types_module_module.go Structural changes in modules and mocks to enhance modularity and reduce dependencies, including updates to genesis handling and interface adjustments.
x/genutil/client/cli/gentx.go, x/genutil/client/cli/init.go, x/genutil/client/cli/init_test.go, x/genutil/client/cli/validate_genesis.go Simplified command line interface operations by removing redundant parameters and leveraging internal structures for genesis validation and initialization.

Related issues

  • [Epic]: Server/v2 #18282: The emphasis on modularity and reducing dependencies, particularly in server components and application interfaces, aligns with the objective of making the Cosmos SDK server more modular. This PR's focus on streamlining interfaces and enhancing flexibility could contribute to the broader goal of allowing teams to compose servers with existing modules or create new ones without forking the SDK.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@tac0turtle tac0turtle marked this pull request as ready for review March 3, 2024 13:56
@tac0turtle tac0turtle requested a review from a team as a code owner March 3, 2024 13:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 7628592 and b197a54.
Files selected for processing (30)
  • core/appmodule/v2/genesis.go (1 hunks)
  • simapp/app.go (1 hunks)
  • tests/integration/evidence/keeper/infraction_test.go (1 hunks)
  • tests/integration/staking/keeper/vote_extensions_test.go (1 hunks)
  • types/module/module.go (2 hunks)
  • x/auth/module.go (4 hunks)
  • x/authz/keeper/genesis.go (3 hunks)
  • x/authz/module/module.go (3 hunks)
  • x/bank/keeper/genesis.go (3 hunks)
  • x/bank/keeper/keeper.go (1 hunks)
  • x/bank/module.go (4 hunks)
  • x/circuit/keeper/genesis.go (2 hunks)
  • x/circuit/keeper/genesis_test.go (1 hunks)
  • x/circuit/module.go (3 hunks)
  • x/distribution/keeper/genesis.go (11 hunks)
  • x/distribution/module.go (3 hunks)
  • x/evidence/depinject.go (1 hunks)
  • x/evidence/genesis.go (3 hunks)
  • x/evidence/module.go (4 hunks)
  • x/feegrant/module/module.go (3 hunks)
  • x/gov/genesis.go (4 hunks)
  • x/gov/module.go (3 hunks)
  • x/gov/testutil/expected_keepers_mocks.go (1 hunks)
  • x/mint/keeper/genesis.go (1 hunks)
  • x/mint/keeper/genesis_test.go (1 hunks)
  • x/mint/module.go (3 hunks)
  • x/protocolpool/module.go (3 hunks)
  • x/slashing/keeper/genesis.go (2 hunks)
  • x/slashing/module.go (3 hunks)
  • x/upgrade/module.go (5 hunks)
Additional comments: 103
core/appmodule/v2/genesis.go (1)
  • 13-13: The change in the return type of DefaultGenesis from Message to json.RawMessage is a positive adjustment, enhancing flexibility and efficiency in handling raw JSON genesis data.
x/mint/keeper/genesis.go (2)
  • 10-21: The update to return errors in InitGenesis enhances error handling and control flow, aligning with best practices in Go.
  • 25-36: The modifications to return errors in ExportGenesis improve error handling, following best practices in Go.
x/evidence/depinject.go (1)
  • 48-48: The modification in the order of arguments passed to NewAppModule in ProvideModule seems aligned with the intended changes. Please ensure all calls to NewAppModule across the codebase have been updated to reflect this new order.
Verification successful

The signature of NewAppModule in x/evidence/module.go as func NewAppModule(cdc codec.Codec, keeper keeper.Keeper, evidenceHandlers ...eviclient.EvidenceHandler) AppModule matches the usage in x/evidence/depinject.go with m := NewAppModule(in.Cdc, *k, in.EvidenceHandlers...). This confirms that the modification in the order of arguments passed to NewAppModule in ProvideModule is consistent with the expected signature of NewAppModule.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that all calls to NewAppModule have been updated.
rg "NewAppModule" --type go

Length of output: 11547



Script:

#!/bin/bash
# Extract the signature of NewAppModule from x/evidence/module.go
rg "func NewAppModule" x/evidence/module.go

Length of output: 161

x/evidence/genesis.go (2)
  • 17-35: The update to return errors in InitGenesis instead of using panic calls significantly improves error handling and control flow.
  • 47-53: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [39-52]

The modifications to return errors in ExportGenesis enhance error handling, aligning with the improvements made in InitGenesis.

x/authz/keeper/genesis.go (1)
  • 19-29: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [12-39]

The update to return errors in InitGenesis instead of using panic calls significantly improves error handling and control flow.

x/circuit/keeper/genesis.go (2)
  • 26-67: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [9-43]

The update to return errors in ExportGenesis enhances error handling and control flow, aligning with best practices in Go.

  • 47-66: The modifications to return errors in InitGenesis improve error handling, following best practices in Go.
x/bank/keeper/genesis.go (1)
  • 28-40: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [15-56]

The update to return errors in InitGenesis instead of using panic calls significantly improves error handling and control flow.

x/slashing/keeper/genesis.go (1)
  • 26-62: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [13-59]

The update to return errors in InitGenesis instead of using panic calls significantly improves error handling and control flow.

x/circuit/keeper/genesis_test.go (1)
  • 79-80: The update to handle errors returned by ExportGenesis in the test is necessary and aligns with the changes made to the function itself.
x/mint/keeper/genesis_test.go (1)
  • 89-90: The update to handle errors returned by ExportGenesis in the test is necessary and aligns with the changes made to the function itself.
x/circuit/module.go (6)
  • 13-13: The import of appmodulev2 is correctly added to support the new HasGenesis interface implementation. This change aligns with the PR's objective to adopt appmodulev2.HasGenesis across various modules.
  • 31-31: The implementation of the appmodulev2.HasGenesis interface is correctly declared for the AppModule struct. This is a necessary change to comply with the new interface requirements.
  • 81-82: The DefaultGenesis method signature has been correctly updated to return json.RawMessage instead of a specific message type. This change is consistent with the new HasGenesis interface requirements.
  • 86-88: The ValidateGenesis method signature has been correctly updated. The method now correctly takes a json.RawMessage as input, aligning with the updated interface requirements.
  • 96-104: The InitGenesis method signature and implementation have been correctly updated to align with the new interface requirements. The method now correctly takes context.Context and json.RawMessage as arguments. Error handling has been added, enhancing the robustness of the code.
  • 109-114: The ExportGenesis method signature and implementation have been correctly updated to return (json.RawMessage, error), aligning with the new interface requirements. Proper error handling is implemented, which is a good practice.
x/gov/genesis.go (8)
  • 16-19: The modification to return an error instead of panicking in the InitGenesis function when encountering issues with ProposalID.Set is a significant improvement in error handling. This change enhances the robustness and maintainability of the code.
  • 24-24: Returning an error for issues with Params.Set instead of panicking is a good practice. It allows for better error handling and control flow in the calling functions.
  • 29-29: The change to return an error for issues with Constitution.Set instead of panicking is consistent with the improved error handling strategy. This enhances the code's robustness.
  • 42-42: Returning an error when encountering issues with SetDeposit is a good practice. It aligns with the overall strategy of improving error handling in the module.
  • 50-54: The update to return an error for issues with address conversion and vote setting is a positive change. It ensures that errors are handled gracefully, improving the module's reliability.
  • 63-68: The changes to return errors for issues encountered while setting proposals in queues are commendable. This approach to error handling is consistent with modern Go practices and enhances the module's error handling capabilities.
  • 72-72: Returning an error for issues with Proposals.Set instead of panicking is a significant improvement. It aligns with the goal of enhancing error handling throughout the module.
  • 86-86: The overall change to return an error from the InitGenesis function, instead of panicking, is a substantial improvement in error handling. This approach is more robust and maintainable.
x/protocolpool/module.go (6)
  • 12-12: The import of appmodulev2 is correctly added to support the new HasGenesis interface implementation. This change aligns with the PR's objective to adopt appmodulev2.HasGenesis across various modules.
  • 32-32: The implementation of the appmodulev2.HasGenesis interface is correctly declared for the AppModule struct. This is a necessary change to comply with the new interface requirements.
  • 88-89: The DefaultGenesis method signature has been correctly updated to return json.RawMessage instead of a specific message type. This change is consistent with the new HasGenesis interface requirements.
  • 93-95: The ValidateGenesis method signature has been correctly updated. The method now correctly takes a json.RawMessage as input, aligning with the updated interface requirements.
  • 103-111: The InitGenesis method signature and implementation have been correctly updated to align with the new interface requirements. The method now correctly takes context.Context and json.RawMessage as arguments. Error handling has been added, enhancing the robustness of the code.
  • 115-120: The ExportGenesis method signature and implementation have been correctly updated to return (json.RawMessage, error), aligning with the new interface requirements. Proper error handling is implemented, which is a good practice.
x/feegrant/module/module.go (6)
  • 13-13: The import of appmodulev2 is correctly added to support the new HasGenesis interface implementation. This change aligns with the PR's objective to adopt appmodulev2.HasGenesis across various modules.
  • 31-31: The implementation of the appmodulev2.HasGenesis interface is correctly declared for the AppModule struct. This is a necessary change to comply with the new interface requirements.
  • 110-111: The DefaultGenesis method signature has been correctly updated to return json.RawMessage instead of a specific message type. This change is consistent with the new HasGenesis interface requirements.
  • 115-117: The ValidateGenesis method signature has been correctly updated. The method now correctly takes a json.RawMessage as input, aligning with the updated interface requirements.
  • 125-135: The InitGenesis method signature and implementation have been correctly updated to align with the new interface requirements. The method now correctly takes context.Context and json.RawMessage as arguments. Error handling has been added, enhancing the robustness of the code.
  • 139-145: The ExportGenesis method signature and implementation have been correctly updated to return (json.RawMessage, error), aligning with the new interface requirements. Proper error handling is implemented, which is a good practice.
x/upgrade/module.go (7)
  • 13-13: The import of appmodulev2 is correctly added to support the new HasGenesis interface implementation. This change aligns with the PR's objective to adopt appmodulev2.HasGenesis across various modules.
  • 36-36: The implementation of the appmodulev2.HasGenesis interface is correctly declared for the AppModule struct. This is a necessary change to comply with the new interface requirements.
  • 46-46: The addition of the cdc codec.Codec field to the AppModule struct is necessary for the serialization and deserialization of the genesis state, aligning with the updated method signatures that utilize cdc.
  • 110-110: The DefaultGenesis method signature has been correctly updated to return json.RawMessage instead of a specific message type. This change is consistent with the new HasGenesis interface requirements.
  • 115-115: The ValidateGenesis method signature has been correctly updated. The method now correctly takes a json.RawMessage as input, aligning with the updated interface requirements.
  • 107-130: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [120-141]

The InitGenesis method signature and implementation have been correctly updated to align with the new interface requirements. The method now correctly takes context.Context and json.RawMessage as arguments. Additionally, the logic for setting the version map automatically if available is a thoughtful addition, enhancing the module's initialization process.

  • 145-146: The ExportGenesis method signature and implementation have been correctly updated to return (json.RawMessage, error), aligning with the new interface requirements. The method correctly returns the default genesis state, which is consistent with the InitGenesis implementation.
x/evidence/module.go (7)
  • 13-13: The import of appmodulev2 is correctly added to support the new HasGenesis interface implementation. This change aligns with the PR's objective to adopt appmodulev2.HasGenesis across various modules.
  • 33-33: The implementation of the appmodulev2.HasGenesis interface is correctly declared for the AppModule struct. This is a necessary change to comply with the new interface requirements.
  • 43-53: The addition of the cdc codec.Codec field to the AppModule struct and its inclusion in the NewAppModule function signature are necessary changes for the serialization and deserialization of the genesis state. This aligns with the updated method signatures that utilize cdc.
  • 100-101: The DefaultGenesis method signature has been correctly updated to return json.RawMessage instead of a specific message type. This change is consistent with the new HasGenesis interface requirements.
  • 105-107: The ValidateGenesis method signature has been correctly updated. The method now correctly takes a json.RawMessage as input, aligning with the updated interface requirements.
  • 115-122: The InitGenesis method signature and implementation have been correctly updated to align with the new interface requirements. The method now correctly takes context.Context and json.RawMessage as arguments. Error handling has been added, enhancing the robustness of the code.
  • 126-131: The ExportGenesis method signature and implementation have been correctly updated to return (json.RawMessage, error), aligning with the new interface requirements. Proper error handling is implemented, which is a good practice.
x/authz/module/module.go (6)
  • 13-13: The import of appmodulev2 is correctly added to support the adoption of the appmodulev2.HasGenesis interface. This change aligns with the PR's objective to enhance module initialization and configuration.
  • 35-35: The implementation of the appmodulev2.HasGenesis interface by the AppModule struct is a key part of the refactor. This change is necessary for the adoption of the new interface and is correctly applied.
  • 112-113: The DefaultGenesis method's signature has been updated to remove the codec parameter, which is now accessed directly within the method. This simplification is part of the interface change and improves the method's usability.
  • 117-119: The ValidateGenesis method's signature has been updated to directly use the codec from the AppModule struct, removing the need for a codec parameter. This change is consistent with the interface update and enhances code readability.
  • 127-132: The InitGenesis method's signature has been updated to include error handling, aligning with the appmodulev2.HasGenesis interface requirements. This change improves error propagation and control flow within the module.
  • 136-138: The ExportGenesis method's signature has been updated to return an error in addition to the genesis state. This change is part of the interface adoption and enhances error handling during the export process.
x/auth/module.go (7)
  • 12-12: The import of appmodulev2 is correctly added to support the adoption of the appmodulev2.HasGenesis interface. This change aligns with the PR's objective to enhance module initialization and configuration.
  • 33-33: The implementation of the appmodulev2.HasGenesis interface by the AppModule struct is a key part of the refactor. This change is necessary for the adoption of the new interface and is correctly applied.
  • 44-44: The addition of the cdc codec.Codec field to the AppModule struct is crucial for the internal handling of codec operations, aligning with the changes in method signatures. This addition supports the removal of codec parameters from method signatures.
  • 109-110: The DefaultGenesis method's signature has been updated to remove the codec parameter, which is now accessed directly within the method. This simplification is part of the interface change and improves the method's usability.
  • 114-116: The ValidateGenesis method's signature has been updated to directly use the codec from the AppModule struct, removing the need for a codec parameter. This change is consistent with the interface update and enhances code readability.
  • 124-129: The InitGenesis method's signature has been updated to include error handling, aligning with the appmodulev2.HasGenesis interface requirements. This change improves error propagation and control flow within the module.
  • 134-139: The ExportGenesis method's signature has been updated to return an error in addition to the genesis state. This change is part of the interface adoption and enhances error handling during the export process.
x/mint/module.go (6)
  • 12-12: The import of appmodulev2 is correctly added to support the adoption of the appmodulev2.HasGenesis interface. This change aligns with the PR's objective to enhance module initialization and configuration.
  • 33-33: The implementation of the appmodulev2.HasGenesis interface by the AppModule struct is a key part of the refactor. This change is necessary for the adoption of the new interface and is correctly applied.
  • 117-118: The DefaultGenesis method's signature has been updated to remove the codec parameter, which is now accessed directly within the method. This simplification is part of the interface change and improves the method's usability.
  • 122-124: The ValidateGenesis method's signature has been updated to directly use the codec from the AppModule struct, removing the need for a codec parameter. This change is consistent with the interface update and enhances code readability.
  • 132-138: The InitGenesis method's signature has been updated to include error handling, aligning with the appmodulev2.HasGenesis interface requirements. This change improves error propagation and control flow within the module.
  • 143-148: The ExportGenesis method's signature has been updated to return an error in addition to the genesis state. This change is part of the interface adoption and enhances error handling during the export process.
x/bank/module.go (6)
  • 13-13: The import of appmodulev2 is correctly added to support the adoption of the appmodulev2.HasGenesis interface. This change aligns with the PR's objective to enhance module initialization and configuration.
  • 36-36: The implementation of the appmodulev2.HasGenesis interface by the AppModule struct is a key part of the refactor. This change is necessary for the adoption of the new interface and is correctly applied.
  • 121-122: The DefaultGenesis method's signature has been updated to remove the codec parameter, which is now accessed directly within the method. This simplification is part of the interface change and improves the method's usability.
  • 126-128: The ValidateGenesis method's signature has been updated to directly use the codec from the AppModule struct, removing the need for a codec parameter. This change is consistent with the interface update and enhances code readability.
  • 136-142: The InitGenesis method's signature has been updated to include error handling, aligning with the appmodulev2.HasGenesis interface requirements. This change improves error propagation and control flow within the module.
  • 147-149: The ExportGenesis method's signature has been updated to return an error in addition to the genesis state. This change is part of the interface adoption and enhances error handling during the export process.
x/slashing/module.go (6)
  • 12-12: The import of appmodulev2 is correctly added to support the adoption of the appmodulev2.HasGenesis interface. This change aligns with the PR's objective to enhance module initialization and configuration.
  • 33-33: The implementation of the appmodulev2.HasGenesis interface by the AppModule struct is a key part of the refactor. This change is necessary for the adoption of the new interface and is correctly applied.
  • 124-125: The DefaultGenesis method's signature has been updated to remove the codec parameter, which is now accessed directly within the method. This simplification is part of the interface change and improves the method's usability.
  • 129-131: The ValidateGenesis method's signature has been updated to directly use the codec from the AppModule struct, removing the need for a codec parameter. This change is consistent with the interface update and enhances code readability.
  • 139-144: The InitGenesis method's signature has been updated to include error handling, aligning with the appmodulev2.HasGenesis interface requirements. This change improves error propagation and control flow within the module.
  • 148-150: The ExportGenesis method's signature has been updated to return an error in addition to the genesis state. This change is part of the interface adoption and enhances error handling during the export process.
x/distribution/module.go (6)
  • 13-13: The import of appmodulev2 is correctly added to utilize the HasGenesis interface from appmodulev2. This aligns with the PR's objective to adopt the new interface across various modules.
  • 36-36: The implementation of the appmodulev2.HasGenesis interface is correctly declared for the AppModule struct. This ensures that the distribution module conforms to the new interface requirements.
  • 132-133: The DefaultGenesis method signature has been correctly updated to remove the cdc codec.JSONCodec parameter and to return json.RawMessage. This change is consistent with the new HasGenesis interface requirements and improves the method's modularity by using the am.cdc field internally for codec operations.
  • 137-139: The ValidateGenesis method signature has been correctly updated to align with the new HasGenesis interface requirements. The removal of the codec parameter and the use of am.cdc internally for unmarshalling the JSON data is a good practice for encapsulation.
  • 147-152: The InitGenesis method has been updated to include error handling, replacing previous panic scenarios with error returns. This enhances the robustness and error propagation within the module, allowing for better control flow and error management at higher levels of the application.
  • 157-162: The ExportGenesis method has been updated to return an error in addition to the json.RawMessage. This change improves error handling by allowing errors to be propagated up the call stack, rather than causing a panic. It aligns with best practices for error management in Go.
x/distribution/keeper/genesis.go (2)
  • 42-115: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [14-145]

The InitGenesis function has been significantly refactored to include comprehensive error handling for all operations that previously could panic. This is a crucial improvement, enhancing the robustness of the code by properly handling errors and allowing them to be managed at higher levels of the application. It's important to ensure that all callers of InitGenesis are updated to handle the potential errors returned by this function.

  • 142-160: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [149-263]

The ExportGenesis function has been updated to include error handling, similar to the changes in InitGenesis. This approach of returning errors instead of panicking is consistent with Go best practices and improves the maintainability and safety of the code. It's essential to verify that all callers of ExportGenesis properly handle the returned errors.

x/gov/module.go (6)
  • 13-13: The import of appmodulev2 is correctly added to utilize the HasGenesis interface from appmodulev2. This aligns with the PR's objective to adopt the new interface across various modules.
  • 38-38: The implementation of the appmodulev2.HasGenesis interface is correctly declared for the AppModule struct. This ensures that the governance module conforms to the new interface requirements.
  • 163-164: The DefaultGenesis method signature has been correctly updated to remove the codec parameter and to return json.RawMessage. This change is consistent with the new HasGenesis interface requirements and improves the method's modularity by using the am.cdc field internally for codec operations.
  • 168-170: The ValidateGenesis method signature has been correctly updated to align with the new HasGenesis interface requirements. The removal of the codec parameter and the use of am.cdc internally for unmarshalling the JSON data is a good practice for encapsulation.
  • 178-183: The InitGenesis method has been updated to include error handling, replacing previous panic scenarios with error returns. This enhances the robustness and error propagation within the module, allowing for better control flow and error management at higher levels of the application.
  • 187-192: The ExportGenesis method has been updated to return an error in addition to the json.RawMessage. This change improves error handling by allowing errors to be propagated up the call stack, rather than causing a panic. It aligns with best practices for error management in Go.
tests/integration/evidence/keeper/infraction_test.go (1)
  • 142-142: The change to pass cdc directly to evidence.NewAppModule aligns with the broader refactor to adopt appmodulev2.HasGenesis. This modification simplifies the initialization by utilizing the codec directly, which is a positive step towards enhancing modularity and maintainability. However, ensure that all instances where evidenceModule is initialized throughout the test suite and the main application are updated accordingly to prevent runtime errors.
x/bank/keeper/keeper.go (1)
  • 29-29: The modification to the InitGenesis function signature to return an error is a positive change, enhancing error handling and control flow within the module. Ensure that all invocations of InitGenesis across the codebase properly handle the returned error.
Verification successful

The provided context from the shell scripts output shows various instances of InitGenesis method calls across different modules within the Cosmos SDK. These instances demonstrate the implementation of error handling in the InitGenesis method across various modules such as x/upgrade, x/slashing, x/staking, x/protocolpool, and others. Each of these modules correctly implements the InitGenesis method with error handling, aligning with the change mentioned in the review comment.

From the output, it's evident that the InitGenesis method now returns an error in various modules, which is a significant change aimed at enhancing error handling and control flow within the Cosmos SDK modules. This change allows for better error propagation and handling, making the code more robust and maintainable.

Given the evidence from the shell scripts output, it can be concluded that the modification to the InitGenesis function signature to return an error is consistently implemented across the Cosmos SDK codebase. This change enhances error handling and control flow within the module, as initially assessed in the review comment.

Therefore, the review comment is verified as accurate based on the provided context and the output from the shell scripts.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for `InitGenesis` calls within the Cosmos SDK to ensure error handling is implemented.
rg --type go 'InitGenesis' --context 5

Length of output: 122684

types/module/module.go (1)
  • 35-35: The import of appmodulev2 is correctly added to support the new HasGenesis interface. This aligns with the PR's objectives of adopting the appmodulev2.HasGenesis interface across various modules.
simapp/app.go (1)
  • 423-423: The update to pass appCodec as a parameter to evidence.NewAppModule aligns with the PR's objective to enhance module initialization patterns. This ensures consistent codec usage across the application, improving modularity and maintainability. Ensure that corresponding changes are made in the evidence module to accept and utilize this parameter effectively.
x/gov/testutil/expected_keepers_mocks.go (1)
  • 577-581: The update to the InitGenesis method in the MockBankKeeper struct to return an error aligns with the PR's objectives of adopting the appmodulev2.HasGenesis interface and improving error handling. Ensure that all tests using this mock properly handle the returned error.

Comment on lines 221 to 224
if _, hasGenesis := module.(appmodulev2.HasGenesis); hasGenesis {
return !hasGenesis
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of the appmodulev2.HasGenesis check in the SetOrderInitGenesis method is correctly implemented and aligns with the PR's objectives. This ensures that modules implementing the new interface are correctly recognized during the initialization order setup.

One minor suggestion for improvement is to add a comment explaining the purpose of this new check, especially for developers who might be unfamiliar with the transition to appmodulev2. This can enhance code readability and maintainability.

+ // Check if the module implements the appmodulev2.HasGenesis interface
  if _, hasGenesis := module.(appmodulev2.HasGenesis); hasGenesis {
      return !hasGenesis
  }

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
if _, hasGenesis := module.(appmodulev2.HasGenesis); hasGenesis {
return !hasGenesis
}
// Check if the module implements the appmodulev2.HasGenesis interface
if _, hasGenesis := module.(appmodulev2.HasGenesis); hasGenesis {
return !hasGenesis
}

@@ -86,7 +86,7 @@ func TestValidateVoteExtensions(t *testing.T) {
votes = append(votes, ve)
}

err := baseapp.ValidateVoteExtensions(f.sdkCtx, f.stakingKeeper, f.sdkCtx.BlockHeight(), "chain-id-123", abci.ExtendedCommitInfo{Round: 0, Votes: votes})
err := baseapp.ValidateVoteExtensions(f.sdkCtx, f.stakingKeeper, abci.ExtendedCommitInfo{Round: 0, Votes: votes})
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The simplification in the call to baseapp.ValidateVoteExtensions observed in vote_extensions_test.go does not appear to be consistently applied across the codebase, as calls in other test files and the implementation show different parameters being used. It's recommended to ensure that the function's usage is consistent with its intended signature and application across all relevant parts of the codebase.

Analysis chain

The simplification in the call to baseapp.ValidateVoteExtensions in TestValidateVoteExtensions is a positive change. Please ensure this new signature is consistently used across the codebase.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that all calls to ValidateVoteExtensions have been updated.
rg "ValidateVoteExtensions" --type go

Length of output: 2834

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between b197a54 and 14240c1.
Files selected for processing (1)
  • x/evidence/CHANGELOG.md (1 hunks)

@@ -28,6 +28,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Api Breaking Changes

* [#19482](https://github.com/cosmos/cosmos-sdk/pull/19482) `appmodule.Environment` is passed to `NewKeeper` instead of individual services
* [#19627](https://github.com/cosmos/cosmos-sdk/pull/19627) `NewAppModule` now takes in a `codec.Codec` as its first arguement
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo in the word "arguement"; it should be "argument". Also, the article "a" should be replaced with "an" before a vowel sound. Additionally, there seem to be too many consecutive spaces before the word "now".

- * [#19627](https://github.com/cosmos/cosmos-sdk/pull/19627) `NewAppModule` now takes in a `codec.Codec` as its first arguement
+ * [#19627](https://github.com/cosmos/cosmos-sdk/pull/19627) `NewAppModule` now takes in a `codec.Codec` as its first argument

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
* [#19627](https://github.com/cosmos/cosmos-sdk/pull/19627) `NewAppModule` now takes in a `codec.Codec` as its first arguement
* [#19627](https://github.com/cosmos/cosmos-sdk/pull/19627) `NewAppModule` now takes in a `codec.Codec` as its first argument

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

I still don't like the fact that we end up with 4 genesis api.

Cannot we drop the one from types/modules are it is effectively the same as the new one?

We've already asked chain to modify things about genesis in the UPGRADING.md, so adding the removal of one arg is easy to do at the same time as they already need to touch those functions.

core/appmodule/v2/genesis.go Show resolved Hide resolved
types/module/module.go Outdated Show resolved Hide resolved
types/module/module.go Show resolved Hide resolved
@tac0turtle
Copy link
Member Author

note:

  • implement into validate and default genesis
  • break abciendblock to remove codec

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between d594876 and 4a364a3.
Files selected for processing (4)
  • types/module/module_test.go (7 hunks)
  • x/bank/CHANGELOG.md (2 hunks)
  • x/bank/keeper/genesis.go (4 hunks)
  • x/bank/keeper/genesis_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • types/module/module_test.go
  • x/bank/keeper/genesis.go
Additional comments: 6
x/bank/CHANGELOG.md (4)
  • 30-30: The entry for MsgBurn is clear and concise, accurately reflecting the introduction of a new message type. However, the static analysis tool flagged "MsgBurn" as a possible spelling mistake, which is a false positive in this context since it's a specific technical term.
  • 25-30: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [34-34]

The improvements section lists several methods that now return an error instead of panicking, which is a significant improvement in error handling. The static analysis tool flagged these method names as possible spelling mistakes, but these are specific technical terms and not errors.

  • 38-40: The API Breaking Changes section accurately reflects significant changes, such as BurnCoins taking an address instead of a module name, and the introduction of appmodule.Environment to the bank NewKeeper. The static analysis tool flagged "BurnCoins" as a possible spelling mistake, which is a false positive in this context.
  • 44-44: The Consensus Breaking Changes section mentions the removal of BaseAccount creation for non-existent accounts, which is a significant change. The static analysis tool flagged "BaseAccount" as a possible spelling mistake, but it's a specific technical term.
x/bank/keeper/genesis_test.go (2)
  • 70-70: The update to TestInitGenesis to include error handling for InitGenesis aligns with best practices for robust testing. This change ensures that the test will fail gracefully with an informative error message if InitGenesis encounters an issue.
  • 91-117: The modifications in the TestTotalSupply function, particularly the use of ErrorContains instead of PanicsWithError, represent an improvement in error handling and test clarity. This approach allows for more precise assertions about the errors returned by InitGenesis. However, ensure that all possible error scenarios are adequately covered by the test cases to maintain comprehensive test coverage.

Consider adding more test cases to cover additional error scenarios that might arise from InitGenesis to ensure comprehensive coverage.

Comment on lines 25 to 30

## [Unreleased]

### Consens Breaking Changes

* [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist

### Features

* [#17569](https://github.com/cosmos/cosmos-sdk/pull/17569) Introduce a new message type, `MsgBurn`, to burn coins.
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [34-34]

The phrase "if any module accounts does not exist or unauthorized" should be corrected for grammatical accuracy.

- if any module accounts does not exist or unauthorized.
+ if any module accounts do not exist or are unauthorized.

x/bank/CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 4a364a3 and d4d66db.
Files selected for processing (1)
  • x/bank/keeper/genesis_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/bank/keeper/genesis_test.go

@github-actions github-actions bot removed the C:Store label Mar 9, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between d4d66db and 7dba752.
Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • go.mod (1 hunks)
  • go.sum (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • CHANGELOG.md
  • go.mod
  • go.sum

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 7dba752 and 287cc1f.
Files selected for processing (11)
  • store/snapshots/helpers_test.go (1 hunks)
  • store/snapshots/store.go (2 hunks)
  • tests/integration/evidence/genesis_test.go (2 hunks)
  • tests/integration/gov/genesis_test.go (1 hunks)
  • x/circuit/keeper/genesis_test.go (1 hunks)
  • x/gov/genesis.go (6 hunks)
  • x/group/keeper/genesis_test.go (1 hunks)
  • x/mint/keeper/genesis_test.go (2 hunks)
  • x/nft/keeper/keeper_test.go (1 hunks)
  • x/slashing/keeper/genesis_test.go (1 hunks)
  • x/upgrade/module.go (4 hunks)
Files skipped from review as they are similar to previous changes (3)
  • x/circuit/keeper/genesis_test.go
  • x/gov/genesis.go
  • x/mint/keeper/genesis_test.go
Additional comments: 14
x/slashing/keeper/genesis_test.go (1)
  • 53-54: The addition of error handling for the InitGenesis function call is a good practice. It ensures that any issues during the initialization of the genesis state are caught and handled appropriately.
tests/integration/evidence/genesis_test.go (4)
  • 108-109: Adding error handling for the InitGenesis function call improves the robustness of the test by ensuring that any initialization errors are properly caught and reported.
  • 111-112: It's good to see error handling added for the InitGenesis function call in the failure case as well. This ensures that the test accurately reflects the expected behavior when initialization fails.
  • 154-155: Including error handling for the ExportGenesis function call in the success case is a best practice, ensuring that any issues during the export process are caught.
  • 158-159: Adding error handling for the ExportGenesis function call in the failure case is crucial for accurately testing the module's behavior under error conditions.
x/upgrade/module.go (6)
  • 13-13: Importing appmodulev2 to adopt the appmodulev2.HasGenesis interface aligns with the refactor goal of improving genesis handling in the upgrade module.
  • 36-36: Implementing the appmodulev2.HasGenesis interface is a key part of the refactor, ensuring that the upgrade module conforms to the new standards for genesis handling.
  • 109-109: The DefaultGenesis method now correctly returns an empty JSON object, aligning with the expected behavior for modules implementing the appmodulev2.HasGenesis interface.
  • 114-114: The ValidateGenesis method's implementation, which always succeeds, is appropriate given the context of the upgrade module's genesis data.
  • 106-129: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [119-140]

The InitGenesis method's implementation, particularly the handling of version maps, is a significant improvement for the upgrade module's genesis initialization process.

  • 144-145: The ExportGenesis method's implementation, returning the default genesis, is consistent with the module's approach to genesis data.
tests/integration/gov/genesis_test.go (1)
  • 207-224: Improving error handling in the InitGenesis function call within the governance module's integration tests is crucial for ensuring that genesis initialization errors are properly caught and handled.
x/group/keeper/genesis_test.go (1)
  • 148-149: Adding error handling for the InitGenesis function call in the group module's genesis test suite is essential for ensuring that any issues during genesis initialization are properly identified and handled.
x/nft/keeper/keeper_test.go (1)
  • 389-390: The addition of error handling for the InitGenesis function call is a good practice, ensuring that any errors during the initialization process are properly caught and handled. This change improves the robustness of the test suite by verifying that the InitGenesis function behaves as expected without silently failing.

Copy link
Member

@sontrinh16 sontrinh16 left a comment

Choose a reason for hiding this comment

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

hmmm some tests are still failling

@julienrbrt
Copy link
Member

hmmm some tests are still failling

Store is flaky and unrelated but agree that https://github.com/cosmos/cosmos-sdk/actions/runs/8213701247/job/22465391623#step:5:1 should be fixed.

Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

left one comment and gov integration test needs to be fixed.
Otherwise lgtm

tests/integration/gov/genesis_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 287cc1f and 7dd0c9c.
Files selected for processing (1)
  • tests/integration/gov/genesis_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • tests/integration/gov/genesis_test.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 7dd0c9c and 5427d0a.
Files selected for processing (1)
  • x/evidence/genesis.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/evidence/genesis.go

@julienrbrt julienrbrt added this pull request to the merge queue Mar 11, 2024
Merged via the queue into main with commit d2e4096 Mar 11, 2024
80 of 82 checks passed
@julienrbrt julienrbrt deleted the marko/genesis_cleanup branch March 11, 2024 10:26
@coderabbitai coderabbitai bot mentioned this pull request May 3, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants