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(x/**)!: genesis return errors #19740

Merged
merged 8 commits into from
Mar 13, 2024
Merged

Conversation

chixiaowen
Copy link
Contributor

@chixiaowen chixiaowen commented Mar 13, 2024

Description

Closes: #19713


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

  • Bug Fixes

    • Enhanced error handling in InitGenesis and ExportGenesis functions for improved stability in various modules.
    • Improved test reliability with explicit error checks in critical functions.
  • Refactor

    • Replaced panic statements with error returns in key initialization and export functions to enhance stability and error reporting.
  • Documentation

    • Updated CHANGELOG entries to reflect error handling enhancements and panic prevention measures in multiple modules.

@chixiaowen chixiaowen requested a review from a team as a code owner March 13, 2024 04:29
Copy link
Contributor

coderabbitai bot commented Mar 13, 2024

Walkthrough

The changes revolve around enhancing error handling across various modules within the Cosmos SDK, specifically focusing on InitGenesis and ExportGenesis functions. Instead of panicking upon encountering errors, these functions now return errors, aligning with best practices in error handling. This shift towards returning errors instead of causing panics improves the robustness and reliability of the codebase, ensuring that modules react gracefully to unexpected states or inputs during the genesis block initialization and export phases.

Changes

File Pattern Change Summary
tests/integration/.../genesis_test.go Enhanced error handling in InitGenesis and ExportGenesis tests; use of assert.NilError and require.NotNil.
x/*/CHANGELOG.md Documentation of changes to prevent panics in InitGenesis and ExportGenesis.
x/*/keeper/genesis.go Replaced panic statements with error returns in InitGenesis; ExportGenesis functions now return errors.
x/*/module.go, types/module/... Updated function signatures to return errors in InitGenesis and ExportGenesis, enhancing error handling.
testutil/mock/... Updated mock functions to return errors, aligning with changes in module interfaces.

Assessment against linked issues

Objective Addressed Explanation
Verify all genesis return errors (#19713) The changes across various modules and tests ensure that InitGenesis and ExportGenesis do not panic but return errors, aligning with the objectives stated in the issue.

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.

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: 3

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between a569ba6 and 0e3db68.
Files selected for processing (31)
  • tests/integration/gov/genesis_test.go (1 hunks)
  • tests/integration/staking/keeper/genesis_test.go (3 hunks)
  • x/authz/CHANGELOG.md (1 hunks)
  • x/authz/keeper/genesis.go (2 hunks)
  • x/bank/CHANGELOG.md (1 hunks)
  • x/bank/keeper/genesis.go (2 hunks)
  • x/bank/keeper/genesis_test.go (1 hunks)
  • x/bank/keeper/keeper.go (1 hunks)
  • x/bank/module.go (1 hunks)
  • x/crisis/keeper/genesis.go (1 hunks)
  • x/crisis/keeper/genesis_test.go (1 hunks)
  • x/crisis/module.go (1 hunks)
  • x/distribution/CHANGELOG.md (1 hunks)
  • x/distribution/keeper/genesis.go (1 hunks)
  • x/gov/CHANGELOG.md (1 hunks)
  • x/gov/genesis.go (2 hunks)
  • x/group/CHANGELOG.md (1 hunks)
  • x/group/keeper/genesis.go (2 hunks)
  • x/group/keeper/genesis_test.go (1 hunks)
  • x/group/module/module.go (1 hunks)
  • x/nft/CHANGELOG.md (1 hunks)
  • x/nft/keeper/genesis.go (3 hunks)
  • x/nft/keeper/keeper_test.go (1 hunks)
  • x/nft/keeper/msg_server_test.go (1 hunks)
  • x/nft/module/module.go (1 hunks)
  • x/slashing/CHANGELOG.md (1 hunks)
  • x/slashing/keeper/genesis.go (4 hunks)
  • x/slashing/module.go (1 hunks)
  • x/staking/CHANGELOG.md (1 hunks)
  • x/staking/keeper/genesis.go (9 hunks)
  • x/staking/module.go (1 hunks)
Check Runs (26)
main completed (2)
tests (03) completed (1)
tests (02) completed (1)
tests (01) completed (1)
tests (00) completed (2)
test-x-upgrade completed (1)
test-x-staking completed (1)
test-x-slashing completed (3)
test-x-protocolpool completed (1)
test-x-params completed (1)
test-x-mint completed (1)
test-x-group completed (1)
test-x-gov completed (1)
test-x-feegrant completed (1)
test-x-evidence completed (1)
test-x-distribution completed (1)
test-x-bank completed (1)
test-x-authz completed (1)
test-x-auth completed (1)
test-x-tx completed (1)
test-x-nft completed (1)
test-x-circuit completed (1)
test-x-accounts completed (1)
test-store completed (1)
test-simapp completed (1)
test-orm completed (1)
Additional comments: 32
x/crisis/keeper/genesis.go (1)
  • 17-22: The modifications to the ExportGenesis function, which now returns an error in addition to the genesis state, are correctly implemented. This change enhances error handling by avoiding panic and returning errors instead, aligning with best practices in Go.
x/authz/keeper/genesis.go (1)
  • 32-32: The update to return an error in the InitGenesis function when an expected authorization is not found, instead of panicking, is a significant improvement. This aligns with best practices for error handling in Go.
x/nft/CHANGELOG.md (2)
  • 37-46: > 📝 NOTE

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

Consider revising "queries that receives request via query string" to "queries that receive requests via query string" for grammatical accuracy.

  • 42-42: The changelog entry correctly documents the API breaking change related to enhancing error handling in InitGenesis and ExportGenesis functions. This is an important update for users of the SDK.
x/nft/keeper/genesis.go (1)
  • 40-46: > 📝 NOTE

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

The modifications to the ExportGenesis function, which now returns an error in addition to the genesis state, are correctly implemented. This change enhances error handling by avoiding panic and returning errors instead, aligning with best practices in Go.

x/group/CHANGELOG.md (2)
  • 35-35: Consider revising "router service so no baseapp.MessageRouter is required is NewKeeper anymore" for clarity. Perhaps, "router service, so no baseapp.MessageRouter is required in NewKeeper anymore" would be clearer.
  • 38-38: The changelog entry correctly documents the API breaking change related to enhancing error handling in InitGenesis and ExportGenesis functions. This is an important update for users of the SDK.
x/bank/keeper/genesis.go (1)
  • 57-66: > 📝 NOTE

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

The modifications to the ExportGenesis function, which now returns an error in addition to the genesis state, are correctly implemented. This change enhances error handling by avoiding panic and returning errors instead, aligning with best practices in Go.

x/bank/CHANGELOG.md (2)
  • 39-39: Consider adding a space after the period for better readability in the guiding principles section.
  • 41-41: The changelog entry correctly documents the API breaking change related to enhancing error handling in InitGenesis and ExportGenesis functions. This is an important update for users of the SDK.
x/authz/CHANGELOG.md (2)
  • 35-41: > 📝 NOTE

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

Consider revising "doesn't take a message router anymore" to "no longer requires a message router" for clarity and conciseness.

  • 38-38: The changelog entry correctly documents the API breaking change related to enhancing error handling in InitGenesis and ExportGenesis functions. This is an important update for users of the SDK.
x/crisis/keeper/genesis_test.go (2)
  • 58-59: The modifications to handle the additional error return value from ExportGenesis are correctly implemented. Using s.Require().NoError(err) for error assertion is appropriate and aligns with best practices for Go testing.
  • 67-68: The handling of the additional error return value from ExportGenesis in this segment is also correctly implemented. Consistent use of s.Require().NoError(err) for error assertion aligns with Go testing best practices.
x/group/keeper/genesis.go (1)
  • 54-93: > 📝 NOTE

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

The modifications to the ExportGenesis function, including the addition of error handling and changing the return type to include an error, significantly improve the robustness of the function. Replacing panic instances with error returns and using errors.Wrap for adding context to errors align with Go best practices.

x/slashing/keeper/genesis.go (2)
  • 14-36: The introduction of fnErr to capture errors within the loop in InitGenesis is a significant improvement, enhancing error handling and control flow. This approach allows for a cleaner and more robust error handling mechanism.
  • 68-84: > 📝 NOTE

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

The modifications to ExportGenesis, including the enhanced error handling by returning errors instead of panicking, align with Go best practices and significantly improve the function's reliability and robustness.

x/nft/keeper/msg_server_test.go (1)
  • 44-45: The modifications to handle the additional error return value from ExportGenesis are correctly implemented. Using s.Require().NoError(err) for error assertion is appropriate and aligns with best practices for Go testing.
x/gov/genesis.go (2)
  • 35-35: The modification to return an error if the governance module account has not been set is a significant improvement in error handling within the InitGenesis function. This change enhances the robustness and clarity of the module's initialization process.
  • 82-84: The addition of a check to ensure that the total deposits equal the module account's balance, and returning an error if they do not match, significantly improves the error handling and data integrity checks within the InitGenesis function.
x/nft/module/module.go (1)
  • 112-115: The modifications to the ExportGenesis function, including the addition of error handling and changing the return type to include an error, significantly improve the robustness of the function. Returning an error if one occurs during the export process aligns with Go best practices.
x/crisis/module.go (1)
  • 138-141: The modifications to the ExportGenesis function, including the addition of error handling and changing the return type to include an error, significantly improve the robustness of the function. Returning an error if one occurs during the export process aligns with Go best practices.
x/gov/CHANGELOG.md (1)
  • 70-70: The changelog entry for PR Verify all genesis return errors #19713 is clear and effectively communicates the changes made to the InitGenesis and ExportGenesis functions. However, there are several typographical and grammatical issues throughout the document that need attention. For instance:
  • Incorrect verb forms, such as "include" instead of "includes" and "replicates" instead of "replicate".
  • Missing spaces between sentences.
  • Possible spelling mistakes, like "expedited" and "params".

It's important to ensure that the changelog is as clear and error-free as possible, as it serves as a key document for users and developers to understand the changes in each version.

x/distribution/CHANGELOG.md (1)
  • 52-52: The changelog entry for PR Verify all genesis return errors #19713 effectively communicates the changes made to the InitGenesis and ExportGenesis functions in the distribution module. However, similar to the previous file, there are several typographical and grammatical issues throughout the document that need attention. For instance:
  • Missing spaces between sentences.
  • Possible spelling mistakes, like "AddresState" and "VoteInfos".

Ensuring the clarity and correctness of the changelog is crucial for users and developers to understand the changes in each version.

x/bank/keeper/genesis_test.go (1)
  • 41-42: The changes made to the TestExportGenesis function to capture and check the error returned by ExportGenesis are well-implemented and align with best practices for error handling in tests. This ensures that the test accurately reflects the behavior of the ExportGenesis function and fails appropriately if an error occurs.
x/group/module/module.go (1)
  • 148-151: The changes made to the ExportGenesis function to return an error along with the exported genesis state significantly enhance error handling. This ensures that any issues during the export process are properly communicated to the caller, aligning with Go's best practices for error handling.
x/bank/module.go (1)
  • 148-151: The modification to the ExportGenesis function to include error handling is a good practice. It aligns with Go's idiomatic way of handling errors by returning them to the caller. This change enhances the robustness of the function by allowing the caller to handle errors appropriately.
x/slashing/module.go (1)
  • 150-153: The update to the ExportGenesis function to return an error is a positive change, ensuring that error handling is consistent with Go's best practices. This approach allows for better error management and debugging.
x/staking/CHANGELOG.md (1)
  • 97-97: The changelog entry correctly documents the enhancement made to the InitGenesis and ExportGenesis functions in the staking module. This entry provides clear information on the improvements made to error handling, aligning with the changes observed in the code.
x/group/keeper/genesis_test.go (1)
  • 189-190: The change to handle errors returned by ExportGenesis method with s.Require().NoError(err) is a good practice. It enhances error handling and aligns with the PR's objective to improve the robustness of the code by avoiding panic instances.
x/nft/keeper/keeper_test.go (1)
  • 364-365: The addition of error handling with s.Require().NoError(err) for the ExportGenesis method in the TestExportGenesis function is commendable. It ensures that errors are properly checked and handled, enhancing the robustness of the test suite.
x/bank/keeper/keeper.go (1)
  • 30-30: Updating the ExportGenesis function signature to include an error return is a significant improvement. It aligns with the best practices for error handling in Go, enhancing the SDK's stability by ensuring that errors are properly returned and handled.

Comment on lines 153 to 156
res, err := am.keeper.InitGenesis(ctx, &genesisState)
if err != nil {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the InitGenesis function, when an error occurs, it returns nil instead of an error. This approach might not be ideal as it suppresses the error, potentially leading to harder-to-debug issues downstream. Consider returning the error to the caller to handle it appropriately.

- if err != nil {
-   return nil
- }
+ return nil, err

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
res, err := am.keeper.InitGenesis(ctx, &genesisState)
if err != nil {
return nil
}
res, err := am.keeper.InitGenesis(ctx, &genesisState)
return nil, err

Comment on lines 163 to 166
genesis, err := am.keeper.ExportGenesis(ctx)
if err != nil {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the InitGenesis function, the ExportGenesis function also returns nil when an error occurs. It's generally better to return the error to allow the caller to decide how to handle it. This change ensures that errors are not silently ignored.

- if err != nil {
-   return nil
- }
+ return nil, err

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
genesis, err := am.keeper.ExportGenesis(ctx)
if err != nil {
return nil
}
genesis, err := am.keeper.ExportGenesis(ctx)
return nil, err

@@ -43,5 +43,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
* remove from `Keeper`: `AddPubkey`
* [#19440](https://github.com/cosmos/cosmos-sdk/pull/19440) Slashing Module creation takes `appmodule.Environment` instead of individual services
* [#19458](https://github.com/cosmos/cosmos-sdk/pull/19458) ValidatorSigningInfo.IndexOffset is deprecated, and no longer used. The index is now derived using just the StartHeight.
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19713) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry clearly documents the improvements made to error handling in the InitGenesis and ExportGenesis functions, as well as the deprecation of ValidatorSigningInfo.IndexOffset. However, there are some minor grammatical issues to address for clarity:

  • "SignInfo's" should be "SignInfos" to avoid incorrect possessive form.
  • "validator's who did not miss a block" should be "validators who did not miss a block" to correct the possessive form and match the plural context.
- Avoid writing SignInfo's for validator's who did not miss a block. (Every BeginBlock)
+ Avoid writing SignInfos for validators who did not miss a block. (Every BeginBlock)

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
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19713) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19713) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic

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

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 0e3db68 and 7519ce8.
Files selected for processing (8)
  • x/authz/CHANGELOG.md (1 hunks)
  • x/bank/CHANGELOG.md (1 hunks)
  • x/distribution/CHANGELOG.md (1 hunks)
  • x/gov/CHANGELOG.md (1 hunks)
  • x/group/CHANGELOG.md (1 hunks)
  • x/nft/CHANGELOG.md (1 hunks)
  • x/slashing/CHANGELOG.md (1 hunks)
  • x/staking/CHANGELOG.md (1 hunks)
Check Runs (27)
repo-analysis completed (1)
main completed (2)
tests (03) completed (1)
tests (02) completed (1)
tests (01) completed (1)
tests (00) completed (1)
test-x-upgrade completed (1)
test-x-staking completed (1)
test-x-slashing completed (3)
test-x-protocolpool completed (1)
test-x-params completed (1)
test-x-mint completed (1)
test-x-group completed (1)
test-x-gov completed (1)
test-x-feegrant completed (1)
test-x-evidence completed (1)
test-x-bank completed (1)
test-x-authz completed (1)
test-x-auth completed (1)
test-x-tx completed (1)
test-x-nft completed (1)
test-x-distribution completed (1)
test-x-circuit completed (1)
test-x-accounts completed (1)
test-store completed (1)
test-simapp completed (1)
test-orm completed (1)
Additional comments: 2
x/distribution/CHANGELOG.md (1)
x/staking/CHANGELOG.md (1)

@@ -37,6 +37,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

* [#19367](https://github.com/cosmos/cosmos-sdk/pull/19367) `appmodule.Environment` is received on the Keeper to get access to different application services

### API Breaking Changes

* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
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 minor grammatical issue in the changelog entry. The verb "receives" should agree with its subject "queries," which is plural. Consider revising the sentence for grammatical accuracy.

- Added new versions for `Balance`, `Owner`, `Supply`, `NFT`, `Class` queries that receives request via query string.
+ Added new versions for `Balance`, `Owner`, `Supply`, `NFT`, `Class` queries that receive requests via query string.

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
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic

The changelog entry for the API breaking changes section is well-formed but contains a minor typographical error. The word "do" in "do not panic" is unnecessarily repeated.

- Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
+ Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

Note: Ensure to add a period at the end of the sentence for consistency with other entries.


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
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -35,3 +35,4 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#19638](https://github.com/cosmos/cosmos-sdk/pull/19638) Migrate module to use `appmodule.Environment` router service so no `baseapp.MessageRouter` is required is `NewKeeper` anymore.
* [#19489](https://github.com/cosmos/cosmos-sdk/pull/19489) `appmodule.Environment` is received on the Keeper to get access to different application services.
* [#19410](https://github.com/cosmos/cosmos-sdk/pull/19410) Migrate to Store Service.
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry for the API breaking changes section is well-formed but contains a minor typographical error. The word "do" in "do not panic" is unnecessarily repeated.

- Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
+ Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

Note: Ensure to add a period at the end of the sentence for consistency with other entries.


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
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -38,6 +38,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#17569](https://github.com/cosmos/cosmos-sdk/pull/17569) `BurnCoins` takes an address instead of a module name
* [#19477](https://github.com/cosmos/cosmos-sdk/pull/19477) `appmodule.Environment` is passed to bank `NewKeeper`
* [#19627](https://github.com/cosmos/cosmos-sdk/pull/19627) The genesis api has been updated to match `appmodule.HasGenesis`.
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry for the API breaking changes section is well-formed but contains a minor typographical error. The word "do" in "do not panic" is unnecessarily repeated.

- Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
+ Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

Note: Ensure to add a period at the end of the sentence for consistency with other entries.


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
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -35,7 +35,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#19490](https://github.com/cosmos/cosmos-sdk/pull/19490) `appmodule.Environment` is received on the Keeper to get access to different application services.
* [#18737](https://github.com/cosmos/cosmos-sdk/pull/18737) Update the keeper method `DequeueAndDeleteExpiredGrants` to take a limit argument for the number of grants to prune.
* [#16509](https://github.com/cosmos/cosmos-sdk/pull/16509) `AcceptResponse` has been moved to sdk/types/authz and the `Updated` field is now of the type `sdk.Msg` instead of `authz.Authorization`.

* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry for the API breaking changes section is well-formed but contains a minor typographical error. The word "do" in "do not panic" is unnecessarily repeated.

- Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
+ Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

Note: Ensure to add a period at the end of the sentence for consistency with other entries.


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
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -43,5 +43,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
* remove from `Keeper`: `AddPubkey`
* [#19440](https://github.com/cosmos/cosmos-sdk/pull/19440) Slashing Module creation takes `appmodule.Environment` instead of individual services
* [#19458](https://github.com/cosmos/cosmos-sdk/pull/19458) ValidatorSigningInfo.IndexOffset is deprecated, and no longer used. The index is now derived using just the StartHeight.
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry for the API breaking changes section is well-formed but contains a minor typographical error. The word "do" in "do not panic" is unnecessarily repeated.

- Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
+ Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

Note: Ensure to add a period at the end of the sentence for consistency with other entries.


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
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -67,6 +67,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#18532](https://github.com/cosmos/cosmos-sdk/pull/18532) All functions that were taking an expedited bool parameter now take a `ProposalType` parameter instead.
* [#17496](https://github.com/cosmos/cosmos-sdk/pull/17496) in `x/gov/types/v1beta1/vote.go` `NewVote` was removed, constructing the struct is required for this type.
* [#19101](https://github.com/cosmos/cosmos-sdk/pull/19101) Move `QueryProposalVotesParams` and `QueryVoteParams` from the `types/v1` package to `utils` and remove unused `querier.go` file.
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry for the API breaking changes section is well-formed but contains a minor typographical error. The word "do" in "do not panic" is unnecessarily repeated.

- Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
+ Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

Note: Ensure to add a period at the end of the sentence for consistency with other entries.


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
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic
* [#19713](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@chixiaowen chixiaowen changed the title !fix: genesis return errors fix: genesis return errors Mar 13, 2024
@github-actions github-actions bot added the C:x/genutil genutil module issues label Mar 13, 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: 7

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 7519ce8 and aba3118.
Files selected for processing (15)
  • testutil/mock/types_mock_appmodule.go (2 hunks)
  • testutil/mock/types_module_module.go (2 hunks)
  • types/module/core_module.go (3 hunks)
  • types/module/module.go (4 hunks)
  • x/authz/CHANGELOG.md (1 hunks)
  • x/bank/CHANGELOG.md (1 hunks)
  • x/distribution/CHANGELOG.md (1 hunks)
  • x/genutil/module.go (1 hunks)
  • x/gov/CHANGELOG.md (1 hunks)
  • x/group/CHANGELOG.md (1 hunks)
  • x/nft/CHANGELOG.md (1 hunks)
  • x/slashing/CHANGELOG.md (1 hunks)
  • x/slashing/keeper/genesis_test.go (1 hunks)
  • x/staking/CHANGELOG.md (1 hunks)
  • x/staking/module.go (1 hunks)
Check Runs (1)
labeler completed (1)
Files skipped from review as they are similar to previous changes (1)
  • x/staking/module.go
Additional comments: 15
x/slashing/keeper/genesis_test.go (1)
  • 33-34: The modification to include error handling in TestExportAndInitGenesis is correctly implemented and follows best practices for error handling in tests.

This ensures that the test fails gracefully and provides meaningful error messages when keeper.ExportGenesis(ctx) encounters an issue.

x/genutil/module.go (2)
  • 80-87: The modifications to the InitGenesis function to include error handling are correctly implemented and follow Go best practices.

This change enhances the robustness of the module by ensuring that errors are properly returned and can be handled by the caller.

  • 91-92: The ExportGenesis function correctly returns the default genesis state and nil for the error, adhering to the updated function signature.

This is a straightforward implementation that aligns with the expectations for an ExportGenesis function without error conditions.

types/module/core_module.go (2)
  • 106-148: > 📝 NOTE

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

The update to ExportGenesis to return errors instead of panicking is a significant improvement in error handling. However, ensure that all callers of ExportGenesis are updated to handle the returned error appropriately.

  • 106-148: > 📝 NOTE

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

Similarly, the changes to InitGenesis to return errors enhance the robustness of error handling. As with ExportGenesis, verify that all invocations of InitGenesis properly handle the new error return value.

x/staking/CHANGELOG.md (2)
  • 97-97: The changelog entry for PR refactor(x/**)!: genesis return errors #19740 is clear and accurately reflects the changes made to improve error handling in the InitGenesis and ExportGenesis functions by ensuring they do not panic. This is a crucial update for enhancing the robustness of the staking module.
  • 94-97: > 📝 NOTE

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

While reviewing the changelog, several static analysis hints suggested possible spelling mistakes and grammatical issues. Most of these are false positives due to the technical nature of the content and the use of code references. For future documentation, consider enclosing technical terms and code references in code blocks to improve readability and clarity. This approach can also help in reducing false positives from static analysis tools.

testutil/mock/types_module_module.go (2)
  • 378-383: The update to the ExportGenesis method to include an error in its return signature is a positive change, aligning with Go's best practices for error handling. This adjustment allows for more robust testing scenarios where error conditions can be simulated and handled appropriately.
  • 393-398: Similarly, the modification to the InitGenesis method to return an error enhances the testing capabilities by allowing error scenarios to be tested. It's crucial to ensure that all tests using this mock are updated to handle the new error return value, ensuring that error handling is properly tested.
testutil/mock/types_mock_appmodule.go (2)
  • 270-275: The modification to the ExportGenesis method to return both json.RawMessage and error aligns with the PR objectives of enhancing error handling. This change ensures that potential errors during the export of genesis state can be handled gracefully rather than causing a panic. It's a good practice to return errors in such critical operations, allowing the calling function to decide on the error handling strategy.
  • 285-290: The update to the InitGenesis method to return both []types.ValidatorUpdate and error is consistent with the PR's goal of improving error handling across the Cosmos SDK. This change allows for better control over the initialization process of the genesis state, particularly in handling scenarios where the initialization might fail due to invalid data or other issues. Returning an error provides a clear indication of failure, which is crucial for robust and reliable application behavior.
types/module/module.go (4)
  • 101-102: The HasABCIGenesis interface now correctly includes error returns in both InitGenesis and ExportGenesis methods. This change aligns with Go's best practices for error handling and improves the robustness of the SDK by avoiding panics.
  • 457-460: In the InitGenesis method of the Manager struct, error handling for the InitGenesis call on modules implementing HasABCIGenesis is correctly implemented. This ensures that any errors encountered during the initialization of genesis state are properly returned and can be handled by the caller, enhancing the reliability of the genesis process.
  • 539-546: The ExportGenesisForModules method in the Manager struct correctly handles errors returned by the ExportGenesis method of modules implementing HasABCIGenesis. This change ensures that errors during the export of genesis state are properly propagated, allowing for better error handling and debugging.
  • 699-702: In the RunMigrations method of the Manager struct, the handling of errors returned by the InitGenesis method for new modules is correctly implemented. This ensures that any errors encountered during the initialization of new modules as part of the migration process are properly handled, maintaining the integrity of the migration process.

@@ -49,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* remove `Keeper`: `IterateValidatorCurrentRewards`, `GetValidatorCurrentRewards`, `SetValidatorCurrentRewards`, `DeleteValidatorCurrentRewards`
* [#17657](https://github.com/cosmos/cosmos-sdk/pull/17657) The distribution module keeper now takes a new argument `PoolKeeper` in addition.
* [#17670](https://github.com/cosmos/cosmos-sdk/pull/17670) `AllocateTokens` takes `comet.VoteInfos` instead of `[]abci.VoteInfo`
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The entry for PR #19740 could be enhanced for clarity. Consider specifying the modules affected by the changes to InitGenesis and ExportGenesis to provide more context to the reader.

- * [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
+ * [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Updated `InitGenesis` and `ExportGenesis` in various modules (e.g., authz, bank, crisis) to return errors instead of panicking, enhancing error handling.

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
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Updated `InitGenesis` and `ExportGenesis` in various modules (e.g., authz, bank, crisis) to return errors instead of panicking, enhancing error handling.

@@ -37,6 +37,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

* [#19367](https://github.com/cosmos/cosmos-sdk/pull/19367) `appmodule.Environment` is received on the Keeper to get access to different application services

### API Breaking Changes

* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry is clear and adheres to the changelog standards. However, consider revising the verb form for better grammatical agreement in the summary of other entries.

For example, in the entry for [#19367], the phrase "queries that receives request" could be improved for grammatical accuracy.

- queries that receives request
+ queries that receive requests

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
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -35,3 +35,4 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#19638](https://github.com/cosmos/cosmos-sdk/pull/19638) Migrate module to use `appmodule.Environment` router service so no `baseapp.MessageRouter` is required is `NewKeeper` anymore.
* [#19489](https://github.com/cosmos/cosmos-sdk/pull/19489) `appmodule.Environment` is received on the Keeper to get access to different application services.
* [#19410](https://github.com/cosmos/cosmos-sdk/pull/19410) Migrate to Store Service.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry is clear and adheres to the changelog standards. However, consider revising the verb form for better grammatical agreement in the summary of other entries.

For example, in the entry for [#19638], the phrase "so no baseapp.MessageRouter is required is NewKeeper anymore" could be improved for clarity and grammatical accuracy.

- so no `baseapp.MessageRouter` is required is `NewKeeper` anymore
+ so that `NewKeeper` no longer requires a `baseapp.MessageRouter`.

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
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -38,6 +38,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#17569](https://github.com/cosmos/cosmos-sdk/pull/17569) `BurnCoins` takes an address instead of a module name
* [#19477](https://github.com/cosmos/cosmos-sdk/pull/19477) `appmodule.Environment` is passed to bank `NewKeeper`
* [#19627](https://github.com/cosmos/cosmos-sdk/pull/19627) The genesis api has been updated to match `appmodule.HasGenesis`.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry is clear and adheres to the changelog standards. However, consider revising the verb form for better grammatical agreement in the summary of other entries.

For example, in the entry for [#18636], the phrase "methods now returns an error" could be improved for grammatical accuracy.

- methods now returns an error
+ methods now return an error

Committable suggestion

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

Suggested change
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -35,7 +35,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#19490](https://github.com/cosmos/cosmos-sdk/pull/19490) `appmodule.Environment` is received on the Keeper to get access to different application services.
* [#18737](https://github.com/cosmos/cosmos-sdk/pull/18737) Update the keeper method `DequeueAndDeleteExpiredGrants` to take a limit argument for the number of grants to prune.
* [#16509](https://github.com/cosmos/cosmos-sdk/pull/16509) `AcceptResponse` has been moved to sdk/types/authz and the `Updated` field is now of the type `sdk.Msg` instead of `authz.Authorization`.

* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry is clear and adheres to the changelog standards. However, consider revising the verb form for better grammatical agreement in the summary of other entries.

For example, in the entry for [#19637], the phrase "doesn't take a message router anymore" could be improved for clarity.

- doesn't take a message router anymore
+ no longer requires a message router

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
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -43,5 +43,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
* remove from `Keeper`: `AddPubkey`
* [#19440](https://github.com/cosmos/cosmos-sdk/pull/19440) Slashing Module creation takes `appmodule.Environment` instead of individual services
* [#19458](https://github.com/cosmos/cosmos-sdk/pull/19458) ValidatorSigningInfo.IndexOffset is deprecated, and no longer used. The index is now derived using just the StartHeight.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry is clear and adheres to the changelog standards. However, consider revising the verb form for better grammatical agreement in the summary of other entries.

For example, in the entry for [#19458], the phrase "Avoid writing SignInfo's for validator's who did not miss a block" could be improved for grammatical accuracy and clarity.

- Avoid writing SignInfo's for validator's who did not miss a block
+ Avoid writing SignInfos for validators who did not miss a block

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
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@@ -67,6 +67,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#18532](https://github.com/cosmos/cosmos-sdk/pull/18532) All functions that were taking an expedited bool parameter now take a `ProposalType` parameter instead.
* [#17496](https://github.com/cosmos/cosmos-sdk/pull/17496) in `x/gov/types/v1beta1/vote.go` `NewVote` was removed, constructing the struct is required for this type.
* [#19101](https://github.com/cosmos/cosmos-sdk/pull/19101) Move `QueryProposalVotesParams` and `QueryVoteParams` from the `types/v1` package to `utils` and remove unused `querier.go` file.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog entry is clear and adheres to the changelog standards. However, consider revising the verb form for better grammatical agreement in the summary of other entries.

For example, in the entry for [#19101], the phrase "Add message based params configuration" could be improved for clarity.

- Add message based params configuration
+ Added message-based params configuration

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
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.
* [#19740](https://github.com/cosmos/cosmos-sdk/pull/19740) Verify `InitGenesis` and `ExportGenesis` module code and keeper code do not panic.

@julienrbrt julienrbrt changed the title fix: genesis return errors refactor!: genesis return errors Mar 13, 2024
@julienrbrt
Copy link
Member

Awesome! could you fix the conflicts please?

@github-actions github-actions bot removed the C:x/genutil genutil module issues label Mar 13, 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 aba3118 and ebc26d7.
Files selected for processing (9)
  • types/module/core_module.go (2 hunks)
  • types/module/module.go (1 hunks)
  • x/bank/module.go (1 hunks)
  • x/crisis/module.go (1 hunks)
  • x/group/module/module.go (1 hunks)
  • x/nft/module/module.go (1 hunks)
  • x/slashing/module.go (1 hunks)
  • x/staking/CHANGELOG.md (1 hunks)
  • x/staking/module.go (2 hunks)
Check Runs (27)
tests (03) completed (1)
tests (02) completed (1)
tests (01) completed (1)
test-x-bank completed (1)
test-x-upgrade completed (1)
test-x-staking completed (1)
test-x-slashing completed (1)
test-x-mint completed (1)
test-x-group completed (1)
test-x-gov completed (1)
test-x-authz completed (1)
test-x-auth completed (1)
test-x-tx completed (1)
test-x-protocolpool completed (1)
test-x-nft completed (1)
test-x-feegrant completed (1)
test-x-params completed (1)
test-x-circuit completed (1)
test-x-evidence completed (1)
test-x-distribution completed (1)
test-x-accounts completed (1)
test-store completed (1)
test-log completed (1)
test-hubl completed (1)
test-sim-nondeterminism completed (1)
test-simapp completed (1)
test-orm completed (1)
Files skipped from review as they are similar to previous changes (8)
  • types/module/core_module.go
  • types/module/module.go
  • x/bank/module.go
  • x/crisis/module.go
  • x/group/module/module.go
  • x/nft/module/module.go
  • x/slashing/module.go
  • x/staking/module.go
Additional comments: 2
x/staking/CHANGELOG.md (2)
  • 98-98: The changelog entry for PR refactor(x/**)!: genesis return errors #19740 is clear and accurately reflects the changes made to the InitGenesis and ExportGenesis functions. It adheres to the changelog guidelines and effectively communicates the prevention of panics, enhancing the robustness of the module.
  • 95-98: > 📝 NOTE

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

The static analysis has flagged several potential spelling mistakes throughout the changelog. However, upon review, these are predominantly false positives related to technical terms, code references, and context-specific language that are correctly used in this document. It's important to ensure technical content, especially code references and module names, are enclosed within code blocks to maintain clarity and prevent such flags in automated checks.

@chixiaowen
Copy link
Contributor Author

Awesome! could you fix the conflicts please?

done! @julienrbrt

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.

Thank you!
Two nits.

@@ -112,15 +112,18 @@ func (c coreAppModuleAdaptor) ExportGenesis(ctx context.Context) (json.RawMessag
}

if mod, ok := c.module.(HasABCIGenesis); ok {
return mod.ExportGenesis(ctx)
eg, err := mod.ExportGenesis(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

not needed, we can just return as it was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is needed , because staking module need.

Copy link
Member

Choose a reason for hiding this comment

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

your snippet does the same thing as previously but is just more verbose, this is why it can be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

o, i see, i will do it

types/module/core_module.go Outdated Show resolved Hide resolved
types/module/module.go Outdated Show resolved Hide resolved
types/module/core_module.go Outdated Show resolved Hide resolved
x/authz/keeper/genesis.go Outdated Show resolved Hide resolved
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.

same comments from julien, otherwise lgtm

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 ebc26d7 and bf63bac.
Files selected for processing (1)
  • x/authz/keeper/genesis.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/authz/keeper/genesis.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 bf63bac and 2720016.
Files selected for processing (1)
  • x/authz/keeper/genesis.go (2 hunks)
Check Runs (25)
tests (03) completed (1)
tests (02) completed (1)
tests (01) completed (1)
test-x-upgrade completed (1)
test-x-staking completed (1)
test-x-slashing completed (1)
test-x-params completed (1)
test-x-mint completed (1)
test-x-group completed (1)
test-x-gov completed (1)
test-x-evidence completed (1)
test-x-distribution completed (1)
test-x-bank completed (1)
test-x-authz completed (1)
test-x-auth completed (1)
test-x-tx completed (1)
test-x-protocolpool completed (1)
test-x-nft completed (1)
test-x-feegrant completed (1)
test-x-circuit completed (1)
test-x-accounts completed (1)
test-store completed (1)
test-orm completed (1)
test-log completed (1)
test-hubl completed (1)
Files skipped from review as they are similar to previous changes (1)
  • x/authz/keeper/genesis.go

types/module/module.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 2720016 and bcf4a04.
Files selected for processing (1)
  • x/authz/keeper/genesis.go (2 hunks)
Check Runs (25)
tests (03) completed (1)
tests (02) completed (1)
tests (01) completed (1)
test-x-staking completed (1)
test-x-slashing completed (1)
test-x-mint completed (1)
test-x-bank completed (1)
test-x-authz completed (1)
test-x-auth completed (1)
test-x-upgrade completed (1)
test-x-tx completed (1)
test-x-protocolpool completed (1)
test-x-params completed (1)
test-x-nft completed (1)
test-x-group completed (1)
test-x-gov completed (1)
test-x-feegrant completed (1)
test-x-evidence completed (1)
test-x-distribution completed (1)
test-x-circuit completed (1)
test-x-accounts completed (1)
test-log completed (1)
test-store completed (1)
test-hubl completed (1)
test-cosmovisor completed (1)
Files skipped from review as they are similar to previous changes (1)
  • x/authz/keeper/genesis.go

@julienrbrt julienrbrt added this pull request to the merge queue Mar 13, 2024
@julienrbrt julienrbrt removed this pull request from the merge queue due to a manual request Mar 13, 2024
@julienrbrt julienrbrt changed the title refactor!: genesis return errors refactor(x/**)!: genesis return errors Mar 13, 2024
@julienrbrt julienrbrt added this pull request to the merge queue Mar 13, 2024
Merged via the queue into cosmos:main with commit db82004 Mar 13, 2024
57 of 59 checks passed
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.

Verify all genesis return errors
4 participants