fix(gov): correct queryServer and autocli functionality #725
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the restructuring of module management and the governance query server. The Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
x/migrate/keeper/gov_test.go (1)
32-32: LGTM! Consider adding a comment for clarity.The simplification of
govImplinstantiation is a good change. It directly usesfxgovkeeper.NewMsgServerImpl, which aligns with the overall modifications mentioned in the PR summary.Consider adding a brief comment explaining the purpose of
govImplfor better code readability:+// Create a new governance message server implementation govImpl := fxgovkeeper.NewMsgServerImpl(suite.App.GovKeeper)x/gov/keeper/msg_server.go (1)
27-31: LGTM: Function signature simplified and implementation improved.The changes to
NewMsgServerImplare well-structured and align with the PR objectives. The simplified function signature and use ofgovkeeper.NewMsgServerImpl(k.Keeper)improve maintainability and consistency with the Cosmos SDK.Consider adding error handling:
func NewMsgServerImpl(k *Keeper) types.MsgServerPro { + if k == nil { + panic("Keeper cannot be nil") + } return &msgServer{ MsgServer: govkeeper.NewMsgServerImpl(k.Keeper), Keeper: k, } }This addition would prevent potential nil pointer dereferences if the function is called with a nil Keeper.
x/gov/keeper/keeper_test.go (1)
Line range hint
307-346: LGTM: Comprehensive test cases for CheckContractAddressIsDisabledThe new
TestCheckContractAddressIsDisabledfunction is a well-structured addition to the test suite. It uses table-driven tests to cover various scenarios, including empty disabled list, disabled address, disabled address and method, and non-disabled cases. This comprehensive approach ensures thorough testing of theCheckContractAddressIsDisabledfunction.Consider adding a test case for an invalid hex address or method ID to ensure the function handles malformed input correctly.
x/gov/keeper/grpc_query.go (2)
25-28: Consider renaming the fieldkfor clarityIn the
QueryServerstruct:type QueryServer struct { v1.QueryServer k *Keeper }The field
krefers to the keeper instance, but using a single-letter variable can reduce readability. Consider renamingktokeeperfor better clarity and consistency.
107-110: Rename fieldqsto avoid confusionIn the
legacyQueryServerstruct:type legacyQueryServer struct { v1beta1.QueryServer qs v1.QueryServer }The field
qsis similar to the receiverq, which may cause confusion. Consider renamingqstoqueryServerfor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- app/app.go (1 hunks)
- x/gov/keeper/grpc_query.go (5 hunks)
- x/gov/keeper/keeper_test.go (1 hunks)
- x/gov/keeper/msg_server.go (2 hunks)
- x/gov/module.go (2 hunks)
- x/migrate/keeper/gov_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (12)
x/migrate/keeper/gov_test.go (2)
77-77: LGTM! Consistent with previous change.The simplification of
govImplinstantiation is consistent with the change inTestMigrateGovInactive. This maintains uniformity across both test functions, which is a good practice.
Line range hint
1-124: Overall, the changes look good and align with the PR objectives.The modifications in this file simplify the instantiation of the governance message server (
govImpl) in both test functions. These changes:
- Improve code clarity by directly using
fxgovkeeper.NewMsgServerImpl.- Maintain consistency across both test functions.
- Do not alter the core functionality of the tests.
The changes align with the PR objectives of correcting queryServer and autocli functionality, specifically in the context of governance-related operations.
x/gov/keeper/msg_server.go (2)
13-13: LGTM: Import addition is appropriate.The addition of the
govkeeperimport is consistent with the changes made in theNewMsgServerImplfunction. This import allows the use ofgovkeeper.NewMsgServerImpl(), which is now being utilized in the updated implementation.
Line range hint
1-265: LGTM: Changes well-integrated with existing code.The modifications to
NewMsgServerImpland the addition of thegovkeeperimport are well-integrated with the rest of the file. The existing method implementations and themsgServerstruct remain unchanged and compatible with the new changes. This approach maintains the current functionality while improving the code structure.x/gov/keeper/keeper_test.go (1)
50-53: LGTM: Simplified initialization of msgServer and queryClientThe changes to the
SetupTestmethod streamline the initialization process by directly utilizing thekeeperimplementation from thefx-coremodule. This simplification removes the dependency on thegovkeeperfromcosmos-sdkand aligns with the overall refactoring of the governance module.app/app.go (1)
Line range hint
446-452: Approved: Enhanced module iteration in AutoCliOptsThis change improves the
AutoCliOptsmethod by iterating overapp.mm.Modulesinstead ofapp.ModuleBasics. This modification ensures that all registered modules, including any additional or custom modules, are properly included in the autocli options. This aligns well with the PR objective of correcting autocli functionality.The new implementation should provide a more comprehensive set of autocli options, potentially including modules that may not have been present in the basic module manager. This change enhances the flexibility and completeness of the autocli feature.
x/gov/keeper/grpc_query.go (2)
119-133: Ensure compatibility inTallyResultfor legacy queriesIn the
legacyQueryServer.TallyResultmethod, you're converting the response to a legacy format. Verify that all fields are correctly converted and that the legacy clients receive the expected data.
Line range hint
69-104: Ensure proper error handling inTallyResultIn the
TallyResultmethod, while handling different proposal statuses, ensure that all potential errors are properly checked and returned.Run the following script to check for unhandled errors:
This script searches for instances where an error is assigned but not immediately checked, which could lead to unhandled errors.
x/gov/module.go (4)
85-88: EmbeddingAppModuleBasicand renaming variables for clarityEmbedding
AppModuleBasicdirectly intoAppModuleenhances code modularity and leverages Go's embedding capabilities for cleaner interface implementation. RenamingakandbktoaccountKeeperandbankKeeperimproves readability by making the variable names more descriptive.
96-99: Ensure proper initialization ofAppModuleBasicIn the
NewAppModulefunction,AppModuleBasicis initialized with an empty struct. IfAppModuleBasicrequires specific initialization (e.g., proposal handlers), consider invokingNewAppModuleBasicto ensure all necessary fields are properly set.Verify if
AppModuleBasicneeds to be initialized with proposal handlers or other parameters. If so, update the initialization accordingly:func NewAppModule(cdc codec.Codec, keeper *keeper.Keeper, ak govtypes.AccountKeeper, bk govtypes.BankKeeper, ss govtypes.ParamSubspace) AppModule { return AppModule{ - AppModuleBasic: AppModuleBasic{}, + AppModuleBasic: NewAppModuleBasic(/* pass necessary parameters */), keeper: keeper, accountKeeper: ak, bankKeeper: bk, cdc: cdc, legacySubspace: ss, } }
105-110: Confirm empty method implementations for interface complianceThe methods
IsOnePerModuleTypeandIsAppModulehave empty bodies to satisfy thedepinject.OnePerModuleTypeandappmodule.AppModuleinterfaces. Ensure that these methods do not require any additional logic and that empty implementations are acceptable.Confirm that the interfaces are marker interfaces and that no further implementation is necessary.
162-162: Correct implementation ofConsensusVersionmethodThe
ConsensusVersionmethod correctly returns the consensus version from thegovmodule. This ensures that the module's consensus version is accurately communicated during upgrades.
Summary by CodeRabbit
New Features
QueryServertype for improved query handling.legacyQueryServertype.AppModulestructure with new methods for better organization.Bug Fixes
Chores
msgServerandgovImplin tests.