refactor(x/group)!: use router service

[julienrbrt]

Description

ref: #19542

---

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
  • Enhanced router service integration within the group module, improving overall routing efficiency.
• Refactor
  • Simplified the group keeper's initialization by removing the need for a message router parameter.
  • Updated proposal execution logic to be more efficient and error-resistant.
• Chores
  • Updated documentation to reflect migration to appmodule.Environment router service.
• Tests
  • Adjusted test setups to align with the new initialization parameters and removed obsolete code.

[coderabbitai]

Walkthrough

Walkthrough

The updates primarily focus on integrating a new router service within the module, eliminating the dependency on baseapp.MessageRouter. This involves modifications across various files, including adjusting the initialization of GroupKeeper and refactoring message execution logic to utilize the new router service. Additionally, changes streamline the code by removing unnecessary parameters and simplifying the setup for tests.

Changes

File(s)	Change Summary
runtime/router.go	Added strings package; Trimmed typeURL variable of leading slashes in CanInvoke functions.
simapp/app.go, x/group/keeper/genesis_test.go, x/group/keeper/grpc_query_test.go, x/group/keeper/keeper_test.go	Adjusted GroupKeeper and test setups to utilize the new router service; Removed or updated parameters accordingly.
x/group/CHANGELOG.md	Noted migration to use appmodule.Environment router service.
x/group/keeper/keeper.go, x/group/keeper/msg_server.go, x/group/keeper/proposal_executor.go	Removed baseapp.MessageRouter dependency; Refactored message execution to use the new router service.
x/group/module/depinject.go	Removed baseapp import and MsgServiceRouter references; Updated ProvideModule function.

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

• X
• Mastodon
• Reddit
• LinkedIn

---

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

[likhita-809]

lgtm

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between 3cbdf5a and f053333.

Files selected for processing (10)

• runtime/router.go (3 hunks)
• simapp/app.go (1 hunks)
• x/group/CHANGELOG.md (1 hunks)
• x/group/keeper/genesis_test.go (2 hunks)
• x/group/keeper/grpc_query_test.go (2 hunks)
• x/group/keeper/keeper.go (2 hunks)
• x/group/keeper/keeper_test.go (2 hunks)
• x/group/keeper/msg_server.go (1 hunks)
• x/group/keeper/proposal_executor.go (1 hunks)
• x/group/module/depinject.go (3 hunks)

Additional comments: 16

x/group/module/depinject.go (2)

• 30-35: The removal of MsgServiceRouter from the GroupInputs struct aligns with the refactor's goal to utilize appmodule.Environment router service, simplifying dependencies.
• 8-13: > 📝 NOTE

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

The update to the ProvideModule function, removing the MsgServiceRouter parameter, is consistent with the refactor towards using appmodule.Environment, streamlining the module's initialization process.

x/group/CHANGELOG.md (1)

• 35-35: The changelog entry accurately describes the migration of the module to use appmodule.Environment router service, categorizing it appropriately under "API Breaking Changes".

x/group/keeper/proposal_executor.go (1)

• 18-51: The updates to the doExecuteMsgs function, including the use of context.Context, updated time references, and refactored message handling logic, align with the refactor's goals towards a more modular and service-oriented approach.

runtime/router.go (2)

• 63-64: The update to trim leading slashes from typeURL in the CanInvoke function of msgRouterService standardizes type URL handling, enhancing the robustness of message routing.
• 120-121: The update to trim leading slashes from typeURL in the CanInvoke function of queryRouterService ensures consistent type URL handling across both message and query routing, enhancing service reliability.

x/group/keeper/genesis_test.go (1)

• 76-77: The updates to the GenesisTestSuite setup, including the integration of the new router service, align with the refactor's objectives and enhance the test environment to reflect the module's updated architecture.

x/group/keeper/grpc_query_test.go (2)

• 71-71: The removal of storeService initialization and its usage simplifies the setup for these tests. This change aligns with the refactor towards using a router service, as mentioned in the PR objectives. However, ensure that the removal of storeService does not impact any test cases that might have relied on its specific behavior.
• 73-73: The modification in groupKeeper initialization to exclude the bApp.GRPCQueryRouter() parameter is consistent with the architectural changes towards using a router service. This change should streamline the keeper's initialization process in tests. It's important to verify that all tests still pass and that the removal of this parameter does not inadvertently affect any functionality tested.

x/group/keeper/keeper.go (1)

• 83-83: The removal of the router variable from the Keeper struct and its constructor function NewKeeper is a significant change that aligns with the refactor towards using a router service. This change simplifies the Keeper struct by removing an unused dependency, which can enhance maintainability and clarity. Ensure that all references to the router in the module have been updated or removed accordingly, and that this change does not introduce any unintended side effects.

x/group/keeper/keeper_test.go (1)

• 79-81: The changes to the SetupTest function reflect the transition to using a router service for the x/group module. This is a significant architectural change that aligns with the PR objectives. However, it's crucial to ensure that the new env and s.groupKeeper initialization correctly integrates with the router service and does not introduce any unintended side effects.

• Correctness: The initialization of env with runtime.NewEnvironment and the subsequent creation of s.groupKeeper with keeper.NewKeeper appear to be correctly implemented, considering the new router service integration. The use of runtime.EnvWithRouterService to provide the router service to the environment is appropriate.
• Maintainability: This change simplifies the initialization by removing the direct dependency on the router variable and parameter from the Keeper struct and its constructor, respectively. It enhances the maintainability of the code by relying on the appmodule.Environment router service.
• Performance: There are no immediate performance concerns with this change. However, it's essential to monitor the impact of the new router service on the overall performance of the x/group module, especially in terms of message routing and handling.
• Best Practices: The transition to using a service-oriented architecture for routing within the x/group module follows the best practices of modularity and separation of concerns. It allows for more flexible and scalable module development.

Overall, the changes to the SetupTest function are well-implemented and align with the objectives of transitioning the x/group module to utilize a router service. It's recommended to continue monitoring the integration of the router service to ensure it meets the module's requirements without introducing regressions.

simapp/app.go (1)

• 362-362: The initialization of GroupKeeper has been modified to include additional parameters related to the router service. This change is crucial for integrating the new router service into the x/group module. Ensure that the GroupKeeper's constructor in x/group/keeper/keeper.go is updated accordingly to accept these new parameters. Additionally, verify that all instances where GroupKeeper is initialized throughout the codebase have been updated to pass the correct arguments.

Verification successful

---

The constructor of GroupKeeper in x/group/keeper/keeper.go has been confirmed to accept an appmodule.Environment parameter, which likely encapsulates the router service integration. This design choice addresses the concern of updating the GroupKeeper to include new parameters related to the router service, as mentioned in the review comment. Therefore, the initialization of GroupKeeper in simapp/app.go is consistent with the expected changes, and the instance in x/group/keeper/grpc_query_test.go not including the new parameters directly is not an issue due to its test environment context. The review comment is verified as the necessary updates appear to have been made correctly.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Search for GroupKeeper initialization in other parts of the codebase to ensure they are updated.
rg "NewKeeper" --type go | grep "groupkeeper"

Length of output: 423

---

---

Script:

#!/bin/bash
# Check the constructor signature of GroupKeeper in x/group/keeper/keeper.go to confirm it accepts the new parameters.
rg "func NewKeeper" x/group/keeper/keeper.go

Length of output: 166

x/group/keeper/msg_server.go (4)

• 859-862: The introduction of BranchService.Execute in the Exec method for executing proposal messages is a significant change. Ensure that the BranchService is properly initialized and available in the keeper's environment. This change aligns with the refactor towards using a router service, enhancing modularity and potentially improving error handling during message execution. However, it's crucial to verify that the BranchService correctly handles transaction boundaries and rollback scenarios in case of execution failures.
• 859-862: The error handling within the Exec method appears robust, with clear logging and status updates based on the execution outcome. It's important to ensure that the logs provide enough context for debugging and that the proposal status is accurately updated to reflect execution results. This approach helps maintain the integrity of the proposal's lifecycle and provides transparency on execution outcomes.
• 859-862: While the summary mentions the removal of caching and event handling code, this review focuses on the added or modified code. It's crucial to ensure that the removal of such code does not negatively impact the module's performance or functionality. If caching was previously used to optimize performance, verify that the new architecture maintains or improves upon these performance metrics. Similarly, ensure that all necessary events are still emitted correctly to maintain compatibility with external systems or clients relying on these events.
• 859-862: The refactor to utilize a router service, as demonstrated by the changes in the Exec method, marks a pivotal shift towards a more modular and service-oriented architecture. This approach is likely to enhance the maintainability and scalability of the x/group module. It's important to ensure that this architectural shift is consistently applied across the module to fully realize its benefits. Additionally, consider the impact on existing clients and downstream systems, ensuring that they can adapt to or leverage the new architecture without significant disruption.