refactor(core): re-add handlers #21575
Conversation
WalkthroughWalkthroughThe changes introduce new helper functions for registering message handlers in the Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- core/appmodule/v2/handlers.go (2 hunks)
Additional context used
Path-based instructions (1)
core/appmodule/v2/handlers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
core/appmodule/v2/handlers.go (8)
24-24: Refactor: Simplified handler registration.The change to
RegisterPreHandlerby removing the message name simplifies the interface and reduces the dependency on specific message names at registration time. This aligns with the PR's goal to abstract away details like message names from the core components.
49-61: Enhancement: Type-safe registration for PreMsgHandlers.The introduction of
RegisterMsgPreHandlerusing generics enhances type safety by ensuring that the handler is associated with the correct message type at compile time. This prevents runtime type errors and improves the robustness of the message handling system.
65-65: Refactor: Streamlined MsgRouter interface.The
MsgRouterinterface has been simplified by changingRegistertoRegisterMsgHandler, which now directly accepts a handler without a message name. This change supports the PR's objective to internalize message name handling and reduce external dependencies.
87-99: Enhancement: Type-safe registration for MsgHandlers.
RegisterMsgHandlerfunction utilizes generics to ensure that handlers are correctly typed, enhancing the safety and maintainability of the code. This approach also simplifies the registration process by abstracting away the need to manually handle type assertions.
105-105: Refactor: Simplified PostMsgRouter interface.The modification to
RegisterPostMsgHandlerby removing the message name parameter simplifies the interface, making it consistent with other handler registration methods in this refactor.
129-145: Enhancement: Type-safe registration for PostMsgHandlers.The
RegisterPostMsgHandlerfunction introduces type safety for post-message handling, ensuring that both the request and response types are correctly handled. This method prevents type mismatches and enhances the reliability of the post-processing logic.
149-149: Refactor: Streamlined QueryRouter interface.The change to
RegisterQueryHandlerby removing the query name parameter aligns with the overall PR goal of simplifying and internalizing the handling of names within the system, thus reducing potential errors and dependencies.
171-183: Enhancement: Type-safe registration for QueryHandlers.The introduction of
RegisterQueryHandlerusing generics ensures that query handlers are type-safe, reducing the risk of runtime errors due to type mismatches. This change improves the maintainability and robustness of the query handling system.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
julienrbrt
requested review from
testinginprod,
kocubinski and
hieuvubk
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- core/appmodule/v2/handlers.go (2 hunks)
- runtime/v2/manager.go (3 hunks)
- server/v2/stf/stf_router.go (1 hunks)
Files skipped from review due to trivial changes (1)
- server/v2/stf/stf_router.go
Additional context used
Path-based instructions (2)
core/appmodule/v2/handlers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (9)
core/appmodule/v2/handlers.go (6)
20-27: Interface update enhances clarity.The renaming of
RegisterPreHandlertoRegisterPreMsgHandlerin thePreMsgRouterinterface improves clarity and consistency with other naming conventions in the system.
35-62: Excellent use of generics and documentation.The
RegisterMsgPreHandlerfunction effectively uses generics to ensure type safety, which is crucial for preventing runtime type errors. The inclusion of a detailed example in the comments is helpful for developers implementing this function.
65-71: Interface update enhances clarity.The renaming of
RegisterPostHandlertoRegisterPostMsgHandlerin thePostMsgRouterinterface improves clarity and consistency with other naming conventions in the system.
79-110: Excellent use of generics and documentation.The
RegisterPostMsgHandlerfunction effectively uses generics to ensure type safety, which is crucial for preventing runtime type errors. The inclusion of a detailed example in the comments is helpful for developers implementing this function.
113-116: Interface update enhances clarity.The renaming of
RegistertoRegisterHandlerin theMsgRouterinterface improves clarity and consistency with other naming conventions in the system.
131-167: Excellent use of generics and documentation.The
RegisterHandlerfunction effectively uses generics to ensure type safety, which is crucial for preventing runtime type errors. The inclusion of a detailed example in the comments is helpful for developers implementing this function.runtime/v2/manager.go (3)
607-614: Improved error handling and logging.The refactoring of error handling in the
registerServicesfunction enhances readability and provides more context in the error messages, which aids in debugging.
662-662: Enhanced error messaging for clarity.The inclusion of the service name and method name in the error messages within the
registerQueryHandlersfunction provides clearer context, which is beneficial for debugging.
683-683: Enhanced error messaging for clarity.The inclusion of the service name and method name in the error messages within the
registerMsgHandlersfunction provides clearer context, which is beneficial for debugging.
core/appmodule/v2/handlers.go
Outdated
| // PreMsgRouter. Example usage: | ||
| // ```go | ||
| // | ||
| // func (h Handlers) BeforeSend(ctx context.Context, req *types.MsgSend) (*types.QueryBalanceResponse, error) { |
There was a problem hiding this comment.
Response type here is invalid
| // } | ||
| // | ||
| // func (m Module) RegisterPreMsgHandlers(router appmodule.PreMsgRouter) { | ||
| // handlers := keeper.NewHandlers(m.keeper) |
There was a problem hiding this comment.
a way to use it. it's to simply define before send on the keeper and handle it via keeper, if we wanto to do it msg server style and have a wrapper like handlers then that is also fine.
There was a problem hiding this comment.
I like to think of the keeper as something only for state (basically, just holding public/private collections), and the business logic is somewhere else.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- core/appmodule/v2/handlers.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/appmodule/v2/handlers.go
Description
ref: #21176
I removed getting msg name directly from core.
I tried to use reflection to get it automatically, but it didn't work: ea34ede
So modules would just pass their message name alongside their implementation instead.
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation