feat(runtime): message router service#19571
Merged
Merged
Conversation
Contributor
WalkthroughThe recent updates introduce significant enhancements across the board, focusing on routing services, codec implementations, and command-line interface improvements. A notable addition is the 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 (
|
julienrbrt
commented
Feb 26, 2024
|
|
||
| require ( | ||
| cosmossdk.io/log v1.3.1 | ||
| github.com/cosmos/gogoproto v1.4.11 |
Contributor
Author
There was a problem hiding this comment.
this dep will be gone in server_modular as I can make use of this: https://github.com/cosmos/cosmos-sdk/pull/19499/files#diff-c1fc6f9f872ad64b8a1e6dee2e5e8bb2d675b920a403e621b8cf1f4ac5d6938cR12-R21
julienrbrt
changed the title
12 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (11)
- CHANGELOG.md (1 hunks)
- baseapp/msg_service_router.go (1 hunks)
- core/CHANGELOG.md (1 hunks)
- core/appmodule/environment.go (1 hunks)
- core/go.mod (2 hunks)
- core/go.sum (3 hunks)
- core/router/service.go (1 hunks)
- runtime/environment.go (1 hunks)
- runtime/module.go (2 hunks)
- runtime/router.go (1 hunks)
- testutil/integration/router.go (1 hunks)
Additional comments: 14
core/router/service.go (1)
- 9-13: The introduction of the
Serviceinterface withInvokeTypedandInvokeUntypedmethods is well-designed, adhering to Go's naming conventions and best practices for handling typed and untyped requests. The use ofcontext.Contextandprotoiface.MessageV1is appropriate for the operations being performed.core/appmodule/environment.go (1)
- 15-22: The addition of the
MessageRouterServicefield to theEnvironmentstruct is well-integrated and follows Go's naming conventions and struct organization best practices. This change effectively incorporates the new message router service into the application's environment for broader access across modules.core/go.mod (2)
- 7-7: The addition of
github.com/cosmos/gogoproto v1.4.11is relevant for protobuf handling within the project. Ensure this aligns with the project's long-term dependencies strategy, especially considering the note about future removal in server modular updates.- 16-16: The addition of
github.com/google/go-cmp v0.6.0is suitable for implementing comparison functionality in tests or other parts of the project. This dependency should be used judiciously to avoid unnecessary complexity in comparisons.runtime/environment.go (1)
- 12-33: The introduction of
EnvOptionfunctions for configuring theEnvironmentstruct with optional services like the message router and memory store is a well-implemented use of the functional options pattern. This approach enhances flexibility and scalability for theEnvironmentconfiguration.runtime/router.go (1)
- 19-77: The implementation of the
msgRouterServicestruct and its methodsInvokeTypedandInvokeUntypedis well-designed, adhering to best practices for error handling, context usage, and dynamic protobuf message handling. The use ofprotoregistry.GlobalTypesfor message type resolution is appropriate and demonstrates a good understanding of protobuf's capabilities.core/go.sum (1)
- 12-18: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [4-43]
The updates to the
go.sumfile correctly reflect the addition of new dependencies and updates to existing ones as outlined in thego.modfile. These changes are essential for ensuring the integrity and reproducibility of builds.testutil/integration/router.go (1)
- 84-84: The integration of the message router service into the test application environment through the
runtime.EnvWithMessageRouterService(router)option in theruntime.NewEnvironmentfunction call is correctly implemented. This ensures that the message router service is available and properly configured during integration tests.core/CHANGELOG.md (1)
- 45-45: The changelog entry for PR #19571 is correctly formatted and accurately describes the addition of
router.Serviceand its integration intoappmodule.Environment. Good adherence to the changelog's guiding principles.runtime/module.go (1)
- 214-226: The changes made to
ProvideEnvironmentand the refactoring ofProvideMemoryStoreServiceimprove the modularity and clarity of service provision within the module. These changes appear to be logically sound and correctly implemented.Please verify the impact of these changes on modules that previously depended on
ProvideMemoryStoreServiceto ensure they are still functioning as expected.baseapp/msg_service_router.go (1)
- 28-29: The addition of
ResponseNameByRequestNameandHybridHandlerByMsgNameto theMessageRouterinterface enhances the message router's functionality by allowing for more flexible and efficient message handling. These changes are correctly implemented and follow best practices.Please verify the integration and testing of these new methods to ensure they work as expected in the broader system.
CHANGELOG.md (3)
- 45-45: The entry for the implementation of
core/router.Serviceis clear and correctly links to the PR. It succinctly describes the feature's impact on the runtime and its presence in all modules using dependency injection.- 45-45: The description of the new method
IsGTfortypes.Coinis clear and straightforward. It's good that it specifies the method's functionality, making it easy for readers to understand its purpose.- 45-45: The addition of the
--qrcodeflag to thekeys showcommand is a user-friendly feature that enhances the command's usability. The description is clear and directly states the new capability. However, ensuring that the documentation elsewhere (e.g., command help text or user guides) is updated to reflect this new flag would be important for user awareness.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import ( | ||
| "context" | ||
| "fmt" | ||
| "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- baseapp/msg_service_router.go (2 hunks)
- runtime/router.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- baseapp/msg_service_router.go
- runtime/router.go
tac0turtle
approved these changes
Feb 27, 2024
tac0turtle
reviewed
Feb 29, 2024
tac0turtle
reviewed
Feb 29, 2024
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- core/router/service.go (1 hunks)
- runtime/router.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- core/router/service.go
- runtime/router.go
tac0turtle
reviewed
Feb 29, 2024
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (15)
- baseapp/baseapp.go (1 hunks)
- baseapp/grpcrouter.go (3 hunks)
- baseapp/grpcserver.go (1 hunks)
- baseapp/msg_service_router.go (4 hunks)
- baseapp/options.go (1 hunks)
- core/appmodule/environment.go (1 hunks)
- core/router/service.go (1 hunks)
- runtime/app.go (1 hunks)
- runtime/builder.go (1 hunks)
- runtime/environment.go (1 hunks)
- runtime/module.go (4 hunks)
- runtime/router.go (1 hunks)
- runtime/router_test.go (1 hunks)
- testutil/integration/router.go (1 hunks)
- x/accounts/keeper.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- baseapp/msg_service_router.go
- core/appmodule/environment.go
- core/router/service.go
- runtime/environment.go
- runtime/router.go
- runtime/router_test.go
- testutil/integration/router.go
Additional comments: 11
runtime/builder.go (1)
- 34-34: The addition of
SetGRPCQueryRoutercorrectly integrates the newgrpcQueryRouterinto the application's base setup, aligning with the PR's objectives to enhance message routing capabilities within the Cosmos SDK.baseapp/grpcrouter.go (3)
- 27-28: The addition of the
responseByRequestNamemap in theGRPCQueryRouterstruct is a valuable enhancement, enabling the mapping of request names to response names and supporting the PR's goal of improving message routing capabilities.- 139-141: The introduction of the
ResponseNameByRequestNamemethod is a logical extension of theresponseByRequestNamemap, facilitating easy retrieval of response names by request names, which aligns with the PR's objectives.- 149-158: Updating the
registerHybridHandlermethod to map input names to output names supports the enhanced message routing mechanism by ensuring the correct response type is associated with each request.runtime/app.go (1)
- 52-52: The addition of the
grpcQueryRouterfield to theAppstruct correctly integrates the newGRPCQueryRouterinto the application, supporting the PR's goal of enhancing message routing capabilities.runtime/module.go (1)
- 217-236: The updates to the
ProvideEnvironmentfunction, including the addition ofmsgServiceRouterandqueryServiceRouterparameters, correctly support the enhanced message routing mechanism by ensuring the necessary routers are available within the application's environment.baseapp/options.go (2)
- 390-392: The method
SetMsgServiceRoutercorrectly assigns the providedMsgServiceRouterto theBaseAppinstance. This change enhances the configurability of message routing within the SDK. The method is straightforward and adheres to Go best practices.- 395-397: The method
SetGRPCQueryRoutercorrectly assigns the providedGRPCQueryRouterto theBaseAppinstance. This addition allows for more flexible configuration of gRPC query routing, aligning with the SDK's goals of enhancing routing mechanisms. The implementation is concise and follows Go best practices.x/accounts/keeper.go (2)
- 107-107: The comment added to the
msgRouterfield indicates a future intention to utilize the environment for message routing. This comment is clear and provides valuable insight into planned architectural enhancements. It's important for future development to ensure that the transition to using the environment is seamless and well-documented.- 109-109: Similarly, the comment added to the
queryRouterfield suggests an intention to leverage the environment for query routing. This comment is concise and informative, highlighting a strategic direction for enhancing routing capabilities. Future implementations should carefully consider the impact on existing functionality and ensure backward compatibility.baseapp/baseapp.go (1)
- 284-285: The introduction of
GRPCQueryRouterin place ofMsgServiceRouterrepresents a significant change in how queries are routed within the application. This shift towards gRPC for query handling could enhance the application's scalability and performance by leveraging gRPC's efficient communication protocols. However, it's crucial to ensure that all existing functionalities that relied onMsgServiceRouterare adequately adapted to useGRPCQueryRouter. Additionally, thorough testing is necessary to verify that the new routing mechanism works as expected across different scenarios and does not introduce any regressions.
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- core/CHANGELOG.md (1 hunks)
- core/appmodule/v2/environment.go (1 hunks)
- runtime/app.go (1 hunks)
- x/accounts/keeper.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- core/CHANGELOG.md
- runtime/app.go
- x/accounts/keeper.go
Additional comments: 3
core/appmodule/v2/environment.go (3)
- 8-8: The addition of the
routerpackage in the import statements is necessary for the newRouterServicefield. This change is correctly implemented.- 15-15: The addition of the
Loggerfield to theEnvironmentstruct enhances logging capabilities within the SDK's runtime environment. This is a positive change for debugging and monitoring purposes.- 21-21: Introducing the
RouterServicefield of typerouter.Routeris a key part of this PR's objective to enhance message routing within the SDK. This change aligns with the PR's goals and is correctly implemented.
tac0turtle
reviewed
Mar 3, 2024
tac0turtle
approved these changes
Mar 3, 2024
Contributor
|
lint failure is unrelated |
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (5)
- core/appmodule/v2/environment.go (1 hunks)
- core/router/service.go (1 hunks)
- runtime/router.go (1 hunks)
- x/accounts/keeper.go (3 hunks)
- x/accounts/utils_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- core/appmodule/v2/environment.go
- core/router/service.go
- runtime/router.go
- x/accounts/keeper.go
Additional comments: 1
x/accounts/utils_test.go (1)
- 92-92: The method
ResponseNameByMsgNamecorrectly reflects its purpose of determining the response name based on the message name. This change aligns with the broader PR objectives of enhancing message routing within the Cosmos SDK. The implementation appears straightforward and adheres to Go best practices. However, ensure that all references to the old method nameResponseNameByRequestNameacross the codebase have been updated toResponseNameByMsgNameto maintain consistency and avoid potential runtime errors.
testinginprod
approved these changes
Mar 4, 2024
testinginprod
left a comment
testinginprod
left a comment
Contributor
There was a problem hiding this comment.
lgtm, but i think we'll need to investigate if the InvokeUntyped API is good enough and safe
Contributor
Author
I have added tests, and migrating gov to use it here: #19481. |
12 tasks
12 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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
ref: #19542
Implement msg router service on main.
Once this is merged, I'll be adding the same in runtime/v2
Additionally, I'll update main to use the service in gov, group and authz
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
keys showcommand.Keeperstruct in various modules to correctly utilize the environment for event service access, addressing issues with message handling and routing.