chore: api cleanups #19541
Merged
chore: api cleanups #19541
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates aim to enhance the codebase by removing deprecated functions related to public key conversions, refining module handling using Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (5)
- crypto/codec/cmt.go (1 hunks)
- types/module/module.go (5 hunks)
- types/module/module_int_test.go (3 hunks)
- x/staking/module.go (1 hunks)
- x/staking/types/validator.go (1 hunks)
Additional comments: 7
types/module/module_int_test.go (1)
- 21-21: The update to use
appmodule.AppModuleinstead ofinterface{}for theModulesfield in theManagerstruct enhances type safety and clarity. This ensures that only modules implementing theAppModuleinterface can be added, aligning with the Cosmos SDK's module system design goals.Also applies to: 46-46
x/staking/module.go (1)
- 171-171: The update to the
EndBlockfunction, indicating it can return validator updates, is a logical and necessary change. This aligns with the Cosmos SDK's expectations for module behavior and enhances the staking module's functionality.x/staking/types/validator.go (1)
- 473-478: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The introduction of
CmtConsPublicKeyand the deprecation ofTmConsPublicKeyrepresent a positive shift towards standardization in handling consensus public keys within the Cosmos SDK. This change enhances clarity and interoperability between the Cosmos SDK and CometBFT ecosystems.types/module/module.go (4)
- 139-139: The update to the
Managerstruct to useappmodule.AppModuleinstead ofinterface{}for theModulesmap is a positive change. It enhances type safety and clarity in the module management system.- 152-152: The
NewManagerfunction correctly initializes aManagerwithappmodule.AppModuletypes, ensuring consistency with the updatedManagerstruct. This change improves type safety and module management.- 182-182: Similarly, the
NewManagerFromMapfunction's adaptation to useappmodule.AppModulefor its map parameter aligns with theManagerstruct's changes, promoting type safety and clarity in module management.- 766-766: The simplification in the
EndBlockmethod by directly appendingmoduleValUpdatestovalidatorUpdatesis a logical improvement. It streamlines the process of collecting validator updates from modules, making the code more readable and efficient.
tac0turtle
approved these changes
Feb 23, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- runtime/services/autocli.go (2 hunks)
- testutil/mock/types_module_module.go (1 hunks)
- x/staking/types/validator.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/staking/types/validator.go
Additional comments: 4
runtime/services/autocli.go (2)
- 28-28: The change in the
NewAutoCLIQueryServicefunction'sappModulesparameter type frommap[string]interface{}tomap[string]appmodule.AppModuleenhances type safety and clarity, aligning with the PR's objectives.- 39-39: The modification in the
ExtractAutoCLIOptionsfunction'sappModulesparameter type frommap[string]interface{}tomap[string]appmodule.AppModuleis consistent with the PR's goal of refining type usage for better clarity and type safety.testutil/mock/types_module_module.go (1)
- 22-93: The addition of the
MockAppModuleBasicstruct and its methods for mocking theAppModuleBasicinterface, includingName,RegisterGRPCGatewayRoutes,RegisterInterfaces, andRegisterLegacyAminoCodec, is a valuable enhancement for testing. It allows for more comprehensive and isolated testing of modules that depend onAppModuleBasicwithout needing to instantiate the actual implementations.CHANGELOG.md (1)
- 150-150: The change log entry is clear and accurately reflects the changes made in the
cryptomodule as part of PR chore: api cleanups #19541. It follows the established format and provides the necessary details for understanding the impact of the changes.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- types/module/simulation.go (2 hunks)
Additional comments: 2
types/module/simulation.go (2)
- 9-9: The addition of the
cosmossdk.io/core/appmoduleimport statement is necessary for the updated function signature inNewSimulationManagerFromAppModules. This change aligns with the PR's objective of refining type usage and simplifying logic.- 65-65: The function signature of
NewSimulationManagerFromAppModuleshas been updated to usemap[string]appmodule.AppModuleinstead ofmap[string]interface{}. This change enhances type safety and clarity, which is in line with the PR's objectives of enhancing type usage and simplifying logic. However, it's important to ensure that all calls to this function throughout the codebase have been updated to match the new signature to avoid runtime errors.
alexanderbez
approved these changes
Feb 25, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- types/module/module_int_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- types/module/module_int_test.go
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
CHANGELOG.mdfile to reflect the removal and replacement of deprecated functions in thecryptomodule.MockAppModuleBasicstruct with associated methods for mockingAppModuleBasicinterface.