Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core/handlers): improve handlers registration DevX #22007

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

testinginprod
Copy link
Contributor

@testinginprod testinginprod commented Oct 1, 2024

Description

This PR removes the need to provide the message name when registering a handler.
It allows the runtime implementer to define its custom logic to extract messages names from the router.

It will also streamline gRPC handling and Rest handling by not requiring us to use reflection.


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, you can find examples of the prefixes below:
  • 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.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

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
    • Introduced a new generic interface, GenericMsg, for enhanced flexibility in message handling.
  • Improvements
    • Renamed and restructured handler-related types and functions for clarity and consistency in the handler registration process.
  • Dependency Updates
    • Updated various module dependencies in the go.mod file for improved functionality and performance.

@testinginprod testinginprod requested a review from a team as a code owner October 1, 2024 17:25
Copy link
Contributor

coderabbitai bot commented Oct 1, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces significant changes to the core/appmodule/v2/handlers.go and core/transaction/transaction.go files. It renames and restructures handler-related types and functions to improve clarity and consistency. Additionally, a new generic interface, GenericMsg, is added to enhance message handling flexibility within the transaction system.

Changes

File Path Change Summary
core/appmodule/v2/handlers.go Renamed Handler to HandlerFunc, updated registration function names for pre and post-message handlers, modified RegisterHandler to accept a Handler struct, and updated type parameters for RegisterMsgHandler.
core/transaction/transaction.go Introduced a new generic interface GenericMsg that extends the existing Msg interface, allowing for non-pointer versions of messages in generic contexts.
simapp/v2/go.mod Updated dependencies and replace directives, including the removal of cosmossdk.io/core and adjustments for local paths for several modules.

Possibly related PRs

Suggested labels

C:x/accounts, C:x/protocolpool, C:x/params, C:x/epochs, C:x/distribution, C:x/mint, C:x/slashing, C:x/staking, C:x/gov, C:x/authz, C:x/nft, C:x/group, C:x/consensus, C:core/testing, C:server/v2, C:Store, C:x/circuit, C:x/tx, C:server/v2 stf, C:server/v2 cometbft, C:server/v2 appmanager, C:CLI, C:schema

Suggested reviewers

  • kocubinski
  • facundomedica
  • sontrinh16
  • julienrbrt

📜 Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 15b896f and 192fbad.

⛔ Files ignored due to path filters (1)
  • simapp/v2/go.sum is excluded by !**/*.sum
📒 Files selected for processing (1)
  • simapp/v2/go.mod (1 hunks)
🔇 Additional comments (1)
simapp/v2/go.mod (1)

8-8: Verify compatibility with updated cosmossdk.io/core dependency

The cosmossdk.io/core dependency has been updated to a new pre-release version (v1.0.0-alpha.3.0.20241001182821-3f9c9a087760). While this update likely includes new features or bug fixes, it's important to ensure it doesn't introduce any instability.

  1. Please verify that this specific version is compatible with the rest of the project.
  2. Consider documenting the reason for choosing this particular version, especially since it appears to be a development snapshot.
  3. Ensure that all tests pass with this new version and that there are no breaking changes in the API.

To help verify the impact of this change, you can run the following script:

This script will help identify any potential breaking changes or deprecations that might affect your project.


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
🪧 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>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • 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 testing code 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

github-actions bot commented Oct 1, 2024

@testinginprod your pull request is missing a changelog!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
core/appmodule/v2/handlers.go (1)

165-185: Approve updates to RegisterMsgHandler function

The changes to the RegisterMsgHandler function significantly improve its flexibility and type safety:

  1. The function signature now uses more generic type parameters, allowing for greater flexibility in message handling.
  2. The implementation leverages the new Handler struct, which aligns well with the earlier changes in the file.
  3. The addition of MakeMsg and MakeMsgResp functions enables reflection-free instantiation of request and response types.

These improvements align perfectly with the PR objective of enhancing the handlers registration developer experience and removing the need for reflection in gRPC and REST handling.

Consider adding a brief comment explaining the purpose of the PReq and PResp type parameters for improved code readability. For example:

// PReq and PResp are protobuf-generated types that implement the GenericMsg interface
func RegisterMsgHandler[Req, Resp any, PReq transaction.GenericMsg[Req], PResp transaction.GenericMsg[Resp]](
    // ... rest of the function
)
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 52d8b2e and 983fb0a.

📒 Files selected for processing (2)
  • core/appmodule/v2/handlers.go (5 hunks)
  • core/transaction/transaction.go (1 hunks)
🧰 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.

core/transaction/transaction.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

🔇 Additional comments (9)
core/transaction/transaction.go (3)

13-19: Excellent addition of the GenericMsg[T any] interface

The new GenericMsg[T any] interface is a well-designed and valuable addition to the package. It effectively leverages Go's generics to provide flexibility in handling both pointer and non-pointer versions of messages that implement the Msg interface. This enhancement aligns well with modern Go practices and improves type safety.

The clear and concise documentation explains the purpose and usage of the interface, which is crucial for maintainability and ease of use for other developers.


13-19: Seamless integration with existing code

The introduction of the GenericMsg[T any] interface is a non-breaking change that enhances the package's capabilities without affecting existing code. It complements the Msg interface nicely, providing additional flexibility for users who need to work with non-pointer versions of messages in generic contexts.

This addition demonstrates thoughtful design that anticipates future needs while maintaining backward compatibility.


Line range hint 1-19: Overall excellent code quality and structure

The file core/transaction/transaction.go maintains high code quality and clear structure. The new GenericMsg[T any] interface is well-integrated with the existing code, enhancing the package's functionality without disrupting its organization.

The code consistently adheres to the Uber Go Style Guide, with clear naming conventions, appropriate documentation, and logical grouping of related elements. This approach ensures that the file remains readable and maintainable as it evolves.

core/appmodule/v2/handlers.go (6)

113-122: Approve addition of Handler struct

The new Handler struct is a well-designed addition that encapsulates all necessary information for message handling. It includes:

  1. Func: The actual handler function.
  2. MakeMsg: A function to instantiate the request message type.
  3. MakeMsgResp: A function to instantiate the response message type.

This structure allows for more flexible and type-safe message handling, enabling runtime type instantiation which can be particularly useful for reflection-free operations. This change aligns well with the PR objective of improving the handlers registration developer experience.


Line range hint 1-185: Summary of changes in core/appmodule/v2/handlers.go

The modifications in this file significantly enhance the message handling system:

  1. Improved naming conventions for better clarity (e.g., HandlerFunc, RegisterPreMsgHandler).
  2. Introduction of a new Handler struct that encapsulates all necessary information for message handling.
  3. Updated RegisterMsgHandler function with more generic type parameters and reflection-free message instantiation.

These changes align perfectly with the PR objectives of improving the handlers registration developer experience and removing the need for reflection in gRPC and REST handling. The code is now more consistent, flexible, and type-safe.


22-24: Approve renaming of RegisterPreHandler to RegisterPreMsgHandler

The renaming of RegisterPreHandler to RegisterPreMsgHandler improves clarity by explicitly indicating that this function registers a pre-message handler. This change enhances code consistency and readability.

To ensure this change doesn't introduce any issues, please run the following command to check for any remaining references to the old RegisterPreHandler function:

#!/bin/bash
# Search for any remaining references to the old RegisterPreHandler function
rg --type go 'RegisterPreHandler\('

25-27: Approve renaming of RegisterGlobalPreHandler to RegisterGlobalPreMsgHandler

The renaming of RegisterGlobalPreHandler to RegisterGlobalPreMsgHandler improves clarity by explicitly indicating that this function registers a global pre-message handler. This change enhances code consistency and readability.

To ensure this change doesn't introduce any issues, please run the following command to check for any remaining references to the old RegisterGlobalPreHandler function:

#!/bin/bash
# Search for any remaining references to the old RegisterGlobalPreHandler function
rg --type go 'RegisterGlobalPreHandler\('

67-69: Approve renaming of RegisterPostHandler to RegisterPostMsgHandler

The renaming of RegisterPostHandler to RegisterPostMsgHandler improves clarity by explicitly indicating that this function registers a post-message handler. This change enhances code consistency and readability.

To ensure this change doesn't introduce any issues, please run the following command to check for any remaining references to the old RegisterPostHandler function:

#!/bin/bash
# Search for any remaining references to the old RegisterPostHandler function
rg --type go 'RegisterPostHandler\('

70-71: Approve renaming of RegisterGlobalPostHandler to RegisterGlobalPostMsgHandler

The renaming of RegisterGlobalPostHandler to RegisterGlobalPostMsgHandler improves clarity by explicitly indicating that this function registers a global post-message handler. This change enhances code consistency and readability.

To ensure this change doesn't introduce any issues, please run the following command to check for any remaining references to the old RegisterGlobalPostHandler function:

#!/bin/bash
# Search for any remaining references to the old RegisterGlobalPostHandler function
rg --type go 'RegisterGlobalPostHandler\('

Comment on lines +13 to +14
// HandlerFunc handles the state transition of the provided message.
HandlerFunc = func(ctx context.Context, msg transaction.Msg) (msgResp transaction.Msg, err error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Remaining references to Handler found

The renaming of Handler to HandlerFunc in core/appmodule/v2/handlers.go has introduced multiple remaining references to the old Handler type across the codebase. Please ensure that all instances are reviewed and updated accordingly to maintain consistency and prevent potential issues.

🔗 Analysis chain

Approve renaming of Handler to HandlerFunc

The renaming of Handler to HandlerFunc improves clarity and aligns with Go naming conventions for function types. This change enhances code readability.

To ensure this change doesn't introduce any issues, please run the following command to check for any remaining references to the old Handler type:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to the old Handler type
rg --type go 'type.*Handler.*=.*func\(ctx context\.Context, msg transaction\.Msg\).*\(transaction\.Msg, error\)'

Length of output: 138


Script:

#!/bin/bash
# Corrected script to search for any remaining references to the old Handler type
rg --type go "type\s+Handler\s*=\s*func\(ctx\s+context\.Context,\s+msg\s+transaction\.Msg\)\s+\(transaction\.Msg,\s+error\)"

Length of output: 126


Script:

#!/bin/bash
# Search for any remaining references to the old Handler type across the codebase
rg --type go '\bHandler\b'

Length of output: 56529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants