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

refactor(runtime/v2): untie runtimev2 from the legacy usage of gRPC #22043

Merged
merged 8 commits into from
Oct 3, 2024

Conversation

testinginprod
Copy link
Contributor

@testinginprod testinginprod commented Oct 2, 2024

Description

Closes: #XXXX


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

Release Notes

  • New Features

    • Introduced a modular approach for handling query operations with the addition of QueryHandlers.
    • Enhanced the initialization process for the CometBFTServer to utilize the new query handling mechanism.
  • Bug Fixes

    • Improved error handling during service registration.
  • Documentation

    • Updated import statements to reflect changes in dependencies.
  • Chores

    • Cleaned up redundant code and comments across various files.

These changes enhance the overall functionality and maintainability of the application, particularly in how it processes queries.

Copy link
Contributor

coderabbitai bot commented Oct 2, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The changes in this pull request involve significant modifications to the App struct and related components in the runtime/v2 directory, transitioning from a gRPC-centric approach to a modular handler system for managing queries. The updates include the removal of the GRPCMethodsToMessageMap and the introduction of a QueryHandlers map. Similar changes are reflected in the manager.go, module.go, and server.go files, enhancing the registration and handling of services and queries. The modifications also extend to the go.mod files, updating dependencies and ensuring consistency across the application.

Changes

File Change Summary
runtime/v2/app.go Removed GRPCMethodsToMessageMap, added QueryHandlers, removed GetGPRCMethodsToMessageMap, added GetQueryHandlers.
runtime/v2/manager.go Updated configurator to use queryHandlers, refined service registration logic, updated method signatures.
runtime/v2/module.go Added QueryHandlers to App, modified ProvideAppBuilder to initialize QueryHandlers.
server/v2/api/grpc/server.go Updated methodsMap to use query handlers, modified makeUnknownServiceHandler function signature.
server/v2/cometbft/abci.go Replaced grpcMethodsMap with queryHandlersMap, added getProtoRegistry, introduced maybeRunGRPCQuery.
server/v2/cometbft/go.mod Added requirement for google.golang.org/protobuf v1.34.2, updated indirect dependencies.
server/v2/cometbft/server.go Updated Init method to use GetQueryHandlers instead of GetGPRCMethodsToMessageMap.
server/v2/go.mod Updated cosmossdk.io/core version from v1.0.0-alpha.3 to v1.0.0-alpha.4.
server/v2/server_test.go Removed GetGPRCMethodsToMessageMap from mockApp, added GetQueryHandlers.
server/v2/types.go Updated AppI interface to replace GetGPRCMethodsToMessageMap with GetQueryHandlers.

Possibly related PRs

Suggested reviewers

  • julienrbrt
  • akhilkumarpilli
  • kocubinski
  • facundomedica
  • sontrinh16

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.

Comment on lines +636 to +638
for path, decoder := range c.queryHandlers {
app.QueryHandlers[path] = decoder
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
Comment on lines +655 to +657
for path, handler := range wrapper.handlers {
app.QueryHandlers[path] = handler
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
server/v2/cometbft/abci.go Fixed Show fixed Hide fixed
@testinginprod testinginprod marked this pull request as ready for review October 2, 2024 15:15
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: 9

🧹 Outside diff range and nitpick comments (6)
server/v2/types.go (1)

19-19: Interface change successfully decouples from gRPC

The replacement of GetGPRCMethodsToMessageMap with GetQueryHandlers effectively removes the direct dependency on gRPC-specific types. This change aligns well with the PR objective of decoupling runtimev2 from gRPC usage.

The new method signature using appmodulev2.Handler suggests a more flexible and modular approach to handling queries.

Consider adding a comment to explain the purpose of the GetQueryHandlers method and the structure of the returned map. This would improve code readability and maintainability.

server/v2/server_test.go (2)

39-41: LGTM: Refactoring aligns with PR objectives

The removal of GetGPRCMethodsToMessageMap and addition of GetQueryHandlers is in line with the PR's goal of decoupling from gRPC. The use of a map for handlers follows the Uber Go Style Guide recommendation.

However, the current implementation returns an empty map. Consider adding a TODO comment if there are plans to implement actual handlers in the future.

Consider adding a TODO comment if there are plans to implement actual handlers:

 func (*mockApp[T]) GetQueryHandlers() map[string]appmodulev2.Handler {
+	// TODO: Implement actual query handlers
 	return map[string]appmodulev2.Handler{}
 }

Test coverage for GetQueryHandlers is missing

The current tests define GetQueryHandlers in the mockApp but do not invoke or verify its functionality. To ensure comprehensive coverage, add test cases that call GetQueryHandlers and validate its behavior.

🔗 Analysis chain

Line range hint 52-106: Consider enhancing test coverage for new functionality

While the changes to mockApp align with the PR objectives, the TestServer function hasn't been updated to verify the new GetQueryHandlers method. To ensure comprehensive test coverage, consider adding a test case that specifically checks this new functionality.

To verify the current test coverage and identify areas for improvement, you can run the following command:

This will help identify if the GetQueryHandlers method is being adequately tested.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check test coverage for the server package

go test -coverprofile=coverage.out ./server/v2/...
go tool cover -func=coverage.out | grep -E "server.*\.go.*GetQueryHandlers"

Length of output: 270


Script:

#!/bin/bash
# Description: Search for usage of GetQueryHandlers in test files

rg "GetQueryHandlers" ./server/v2/*_test.go

Length of output: 205


Script:

#!/bin/bash
# Description: Search for invocations of GetQueryHandlers in test files

rg "GetQueryHandlers\(" ./server/v2/*_test.go

Length of output: 209

runtime/v2/module.go (1)

130-136: LGTM with a minor suggestion.

The addition of the QueryHandlers field to the App struct is consistent with the PR objective of refactoring runtimev2. The initialization is correct and follows the Uber Golang style guide.

Consider using make to initialize the QueryHandlers map with an initial capacity for better performance:

- QueryHandlers:      map[string]appmodulev2.Handler{},
+ QueryHandlers:      make(map[string]appmodulev2.Handler, 10), // Adjust capacity as needed

This can help reduce map resizing operations if you have an estimate of the number of handlers you'll be adding.

server/v2/cometbft/abci.go (1)

261-264: Address the TODO: Use codec instead of gogoproto.Unmarshal.

The comment indicates a pending switch to using a codec for unmarshaling the request data. Using the application's codec enhances consistency and maintainability.

Would you like assistance in implementing the codec here? I can help refactor the code to use the appropriate codec for unmarshaling.

runtime/v2/manager.go (1)

Line range hint 750-759: Ensure safe type assertion to prevent panics

The type assertion res.(transaction.Msg) could cause a panic if res does not implement transaction.Msg. Consider using a type assertion with a check to handle this safely.

Apply this diff to safely handle the type assertion:

 		res, err := md.Handler(ss, ctx, noopDecoder, messagePassingInterceptor(msg))
 		if err != nil {
 			return nil, err
 		}
-		return res.(transaction.Msg), nil
+		msgResp, ok := res.(transaction.Msg)
+		if !ok {
+			return nil, fmt.Errorf("expected transaction.Msg, got %T", res)
+		}
+		return msgResp, nil
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5c4f4ac and cc9aab4.

⛔ Files ignored due to path filters (1)
  • server/v2/go.sum is excluded by !**/*.sum
📒 Files selected for processing (10)
  • runtime/v2/app.go (3 hunks)
  • runtime/v2/manager.go (7 hunks)
  • runtime/v2/module.go (1 hunks)
  • server/v2/api/grpc/server.go (3 hunks)
  • server/v2/cometbft/abci.go (6 hunks)
  • server/v2/cometbft/go.mod (1 hunks)
  • server/v2/cometbft/server.go (1 hunks)
  • server/v2/go.mod (1 hunks)
  • server/v2/server_test.go (2 hunks)
  • server/v2/types.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
runtime/v2/app.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.

runtime/v2/module.go (1)

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

server/v2/api/grpc/server.go (1)

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

server/v2/cometbft/abci.go (1)

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

server/v2/cometbft/server.go (1)

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

server/v2/server_test.go (2)

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


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

server/v2/types.go (1)

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

🔇 Additional comments (10)
server/v2/types.go (1)

6-6: Import changes align with PR objectives

The removal of the gogoproto import and the addition of appmodulev2 are consistent with the goal of decoupling runtimev2 from gRPC usage. This change moves the codebase towards a more modular structure.

server/v2/server_test.go (1)

14-14: LGTM: Import change aligns with PR objectives

The addition of the appmodulev2 import is consistent with the PR's goal of decoupling from gRPC. This import supports the new GetQueryHandlers method, which is part of the refactoring process.

server/v2/go.mod (1)

16-16: Dependency version update looks good.

The update of cosmossdk.io/core from v1.0.0-alpha.3 to v1.0.0-alpha.4 is consistent with the PR objectives of refactoring and modernizing the architecture. This change appears to be intentional and aligns with the overall goal of the pull request.

To ensure this update doesn't introduce any breaking changes or compatibility issues, please run the following verification script:

✅ Verification successful

Dependency update verified successfully.

The update of cosmossdk.io/core to v1.0.0-alpha.4 does not introduce any breaking changes or compatibility issues based on the executed verification scripts.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any breaking changes or compatibility issues introduced by the core update.

# Test: Look for any error messages or warnings related to the core package in the build logs.
gh pr view 22043 --json comments | jq '.comments[].body' | grep -i "error.*cosmossdk.io/core" || echo "No core-related errors found in PR comments."

# Test: Check if there are any open issues related to this core version update.
gh issue list --repo cosmos/cosmos-sdk --search "in:title cosmossdk.io/core v1.0.0-alpha.4" --json number,title,state | jq '.[] | select(.state=="OPEN")'

Length of output: 350

server/v2/cometbft/server.go (3)

Line range hint 1-324: Summary: Changes align with PR objectives

The modifications in this file, primarily in the Init method of CometBFTServer, align well with the PR objective of decoupling runtimev2 from gRPC usage. The change from GetGPRCMethodsToMessageMap() to GetQueryHandlers() represents a shift towards a more generic query handling approach.

The localized nature of the change suggests a focused approach to the refactoring, which is commendable. However, it's crucial to ensure that this change is part of a broader, consistent refactoring effort across the codebase.

To further validate the changes:

  1. Confirm that similar updates have been made in related files where necessary.
  2. Verify that the new GetQueryHandlers() method is implemented consistently across all relevant interfaces and structs.
  3. Ensure that documentation is updated to reflect this architectural change.

To assist in this verification, you can run the following script:

#!/bin/bash
# Search for other files that might need similar updates
rg "GetGPRCMethodsToMessageMap" --type go --files-with-matches

# Check for the implementation of GetQueryHandlers
ast-grep --lang go --pattern 'func ($_) GetQueryHandlers() $_ {
  $$$
}'

# Look for any outdated documentation mentioning gRPC in the context of query handling
rg -i "grpc.*query" --type md

This script will help identify other files that might need similar updates, check for implementations of the new GetQueryHandlers() method, and look for any outdated documentation that might need to be updated.


108-108: Verify the impact of switching to GetQueryHandlers()

The change from appI.GetGPRCMethodsToMessageMap() to appI.GetQueryHandlers() aligns with the PR objective of decoupling runtimev2 from gRPC usage. This modification affects how query handlers are retrieved and utilized within the consensus initialization process.

Please ensure that:

  1. The GetQueryHandlers() method returns the expected data structure compatible with the rest of the initialization process.
  2. All other parts of the codebase that might have depended on the previous GetGPRCMethodsToMessageMap() method have been updated accordingly.
  3. The new implementation maintains or improves upon the functionality of the previous one.

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

#!/bin/bash
# Search for any remaining usage of GetGPRCMethodsToMessageMap
rg "GetGPRCMethodsToMessageMap" --type go

# Search for the new usage of GetQueryHandlers
rg "GetQueryHandlers" --type go -C 3

# Check for any potential interface changes
ast-grep --lang go --pattern 'type $_ interface {
  $$$
  GetQueryHandlers() $_
  $$$
}'

This script will help identify any remaining usage of the old method, confirm the new method's usage, and check for interface changes related to GetQueryHandlers().


Line range hint 1-324: Ensure comprehensive testing of CometBFTServer functionality

While the primary change in this file is localized to the Init method, it's crucial to verify that this modification doesn't have unintended consequences on the overall functionality of the CometBFTServer.

Please ensure that:

  1. All existing tests for the CometBFTServer pass with the new implementation.
  2. New tests are added to cover the changed behavior of query handling.
  3. Integration tests are run to confirm that the server still interacts correctly with other components of the system.

To assist in verifying the overall impact, you can run the following script:

This script will help identify relevant test files, check for any newly introduced TODOs or FIXMEs, and list all methods of CometBFTServer to ensure no unintended changes were made.

server/v2/cometbft/go.mod (1)

38-38: Approve the addition of google.golang.org/protobuf dependency

The addition of google.golang.org/protobuf v1.34.2 as a direct dependency aligns with the PR objective of refactoring runtimev2 and untying it from legacy gRPC usage. This change suggests a more direct use of protobuf in the module, which could lead to more efficient handling of protocol buffers.

To ensure this change is properly implemented across the codebase, please run the following script:

This will help confirm that the new dependency is being utilized as intended throughout the project.

runtime/v2/app.go (2)

9-9: Import statement for appmodulev2 is correctly added

The import of appmodulev2 is appropriate and correctly placed within the import block.


45-46: New field QueryHandlers is properly defined

The addition of the QueryHandlers field to the App struct is well-implemented. The field name is clear, and the accompanying comment follows Go documentation conventions.

server/v2/cometbft/abci.go (1)

201-203: Verify error handling when maybeRunGRPCQuery returns an error.

In the Query method, when maybeRunGRPCQuery returns an error with isGRPC set to true, the error might not be properly formatted for the ABCI response.

Consider adjusting the error handling to wrap the error in a QueryResult:

if isGRPC {
    if err != nil {
        return QueryResult(err, c.cfg.AppTomlConfig.Trace), nil
    }
    return resp, nil
}

This ensures that the error is correctly formatted and returned to the caller.

runtime/v2/app.go Show resolved Hide resolved
server/v2/api/grpc/server.go Outdated Show resolved Hide resolved
server/v2/api/grpc/server.go Show resolved Hide resolved
Comment on lines +69 to +70
queryHandlersMap map[string]appmodulev2.Handler
getProtoRegistry func() (*protoregistry.Files, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider Go version compatibility for sync.OnceValues.

The use of sync.OnceValues requires Go 1.21 or later. If the project supports earlier Go versions, this could lead to compatibility issues.

To maintain compatibility with older Go versions, consider using sync.Once with a separate variable:

var (
    protoRegistry     *protoregistry.Files
    protoRegistryOnce sync.Once
)

func (c *Consensus[T]) getProtoRegistry() (*protoregistry.Files, error) {
    var err error
    protoRegistryOnce.Do(func() {
        protoRegistry, err = gogoproto.MergedRegistry()
    })
    return protoRegistry, err
}

Update the Consensus struct accordingly:

- getProtoRegistry func() (*protoregistry.Files, error)
+ // Remove getProtoRegistry from the struct

Comment on lines +104 to +105
queryHandlersMap: queryHandlersMap,
getProtoRegistry: sync.OnceValues(func() (*protoregistry.Files, error) { return gogoproto.MergedRegistry() }),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update getProtoRegistry initialization for thread safety and compatibility.

Initializing getProtoRegistry with sync.OnceValues may not be suitable due to potential Go version incompatibilities. Adopting the standard sync.Once pattern ensures thread safety across supported Go versions.

Modify the initialization in the NewConsensus function:

- getProtoRegistry: sync.OnceValues(func() (*protoregistry.Files, error) { return gogoproto.MergedRegistry() }),
+ // Initialization handled in getProtoRegistry method using sync.Once

Ensure that the getProtoRegistry method is implemented as shown in the previous comment.

Committable suggestion was skipped due to low confidence.

Comment on lines +234 to +275
func (c *Consensus[T]) maybeRunGRPCQuery(ctx context.Context, req *abci.QueryRequest) (resp *abciproto.QueryResponse, isGRPC bool, err error) {
// if this fails then we cannot serve queries anymore
registry, err := c.getProtoRegistry()
if err != nil {
return nil, false, err
}

// in order to check if it's a gRPC query we ensure that there's a descriptor
// for the path, if such descriptor exists, and it is a method descriptor
// then we assume this is a gRPC query.
fullName := protoreflect.FullName(strings.ReplaceAll(req.Path, "/", "."))

desc, err := registry.FindDescriptorByName(fullName)
if err != nil {
return nil, false, err
}

md, isGRPC := desc.(protoreflect.MethodDescriptor)
if !isGRPC {
return nil, false, nil
}

handler, found := c.queryHandlersMap[string(md.Input().FullName())]
if !found {
return nil, true, fmt.Errorf("no query handler found for %s", fullName)
}
protoRequest := handler.MakeMsg()
err = gogoproto.Unmarshal(req.Data, protoRequest) // TODO: use codec
if err != nil {
return nil, true, fmt.Errorf("unable to decode gRPC request with path %s from ABCI.Query: %w", req.Path, err)
}
res, err := c.app.Query(ctx, uint64(req.Height), protoRequest)
if err != nil {
resp := QueryResult(err, c.cfg.AppTomlConfig.Trace)
resp.Height = req.Height
return resp, true, err

}

resp, err = queryResponse(res, req.Height)
return resp, isGRPC, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Wrap external errors to provide context in maybeRunGRPCQuery.

Errors returned from external packages like protoregistry should be wrapped to include additional context. This improves error traceability and debugging.

Apply the following changes to wrap errors:

- return nil, false, err
+ return nil, false, fmt.Errorf("failed to get protobuf registry: %w", err)
- return nil, false, err
+ return nil, false, fmt.Errorf("failed to find descriptor by name %s: %w", fullName, err)

This pattern should be applied to all external error returns within the function.

Committable suggestion was skipped due to low confidence.

Comment on lines +854 to +856
s.handlers = map[string]appmodulev2.Handler{}
}
s.decoders[requestName] = handler.MakeMsg
s.handlers[requestName] = handler
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid re-initializing the handlers map to prevent overwriting

The handlers map is being re-initialized inside the RegisterHandler method whenever s.error == nil, which can cause previously registered handlers to be lost. Initialize the handlers map only once, preferably when the stfRouterWrapper is created, or check if it is nil before initializing.

Apply this diff to fix the issue:

 if s.error == nil {
-	s.handlers = map[string]appmodulev2.Handler{}
+	if s.handlers == nil {
+		s.handlers = map[string]appmodulev2.Handler{}
+	}
 }
 s.handlers[requestName] = handler
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
s.handlers = map[string]appmodulev2.Handler{}
}
s.decoders[requestName] = handler.MakeMsg
s.handlers[requestName] = handler
if s.error == nil {
if s.handlers == nil {
s.handlers = map[string]appmodulev2.Handler{}
}
}
s.handlers[requestName] = handler

Comment on lines +620 to +626
c := &configurator{
queryHandlers: map[string]appmodulev2.Handler{},
stfQueryRouter: app.queryRouterBuilder,
stfMsgRouter: app.msgRouterBuilder,
registry: registry,
err: nil,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify struct initialization and avoid field alignment

Per the Uber Go style guide, aligning struct fields using extra spaces is discouraged. Additionally, explicitly initializing the err field to nil is unnecessary since the zero value of an error is nil.

Apply this diff to simplify the struct initialization:

 c := &configurator{
-    queryHandlers:  map[string]appmodulev2.Handler{},
-    stfQueryRouter: app.queryRouterBuilder,
-    stfMsgRouter:   app.msgRouterBuilder,
-    registry:       registry,
-    err:            nil,
+    queryHandlers: map[string]appmodulev2.Handler{},
+    stfQueryRouter: app.queryRouterBuilder,
+    stfMsgRouter: app.msgRouterBuilder,
+    registry: registry,
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
c := &configurator{
queryHandlers: map[string]appmodulev2.Handler{},
stfQueryRouter: app.queryRouterBuilder,
stfMsgRouter: app.msgRouterBuilder,
registry: registry,
err: nil,
}
c := &configurator{
queryHandlers: map[string]appmodulev2.Handler{},
stfQueryRouter: app.queryRouterBuilder,
stfMsgRouter: app.msgRouterBuilder,
registry: registry,
}

Comment on lines +763 to +768
MakeMsg: func() transaction.Msg {
return reflect.New(requestTyp.Elem()).Interface().(transaction.Msg)
},
MakeMsgResp: func() transaction.Msg {
return reflect.New(responseTyp.Elem()).Interface().(transaction.Msg)
},
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add checks for type assertions in MakeMsg and MakeMsgResp

The type assertions in MakeMsg and MakeMsgResp may cause panics if the created instances do not implement transaction.Msg. Consider adding checks to handle potential type assertion failures.

Apply this diff to safely handle the type assertions:

 MakeMsg: func() transaction.Msg {
-	return reflect.New(requestTyp.Elem()).Interface().(transaction.Msg)
+	msg, ok := reflect.New(requestTyp.Elem()).Interface().(transaction.Msg)
+	if !ok {
+		return nil
+	}
+	return msg
},
MakeMsgResp: func() transaction.Msg {
-	return reflect.New(responseTyp.Elem()).Interface().(transaction.Msg)
+	msgResp, ok := reflect.New(responseTyp.Elem()).Interface().(transaction.Msg)
+	if !ok {
+		return nil
+	}
+	return msgResp
},
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
MakeMsg: func() transaction.Msg {
return reflect.New(requestTyp.Elem()).Interface().(transaction.Msg)
},
MakeMsgResp: func() transaction.Msg {
return reflect.New(responseTyp.Elem()).Interface().(transaction.Msg)
},
MakeMsg: func() transaction.Msg {
msg, ok := reflect.New(requestTyp.Elem()).Interface().(transaction.Msg)
if !ok {
return nil
}
return msg
},
MakeMsgResp: func() transaction.Msg {
msgResp, ok := reflect.New(responseTyp.Elem()).Interface().(transaction.Msg)
if !ok {
return nil
}
return msgResp
},

@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Oct 2, 2024
@julienrbrt
Copy link
Member

backport label needed because of cometbft changes.

@julienrbrt julienrbrt changed the title refactor(runtimev2): untie runtimev2 from the legacy usage of gRPC refactor(runtime/v2): untie runtimev2 from the legacy usage of gRPC Oct 2, 2024
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

utACK

server/v2/api/grpc/server.go Outdated Show resolved Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 (5)
server/v2/api/grpc/server.go (5)

Line range hint 128-129: Improve error message for clarity

The error message "invalid get height from context: %v" is grammatically incorrect and may be unclear. Consider rephrasing it to enhance readability.

Apply this diff to adjust the error message:

-			return status.Errorf(codes.InvalidArgument, "invalid get height from context: %v", err)
+			return status.Errorf(codes.InvalidArgument, "failed to get height from context: %v", err)

Line range hint 131-133: Wrap errors with additional context

For better traceability, consider wrapping the error returned from querier.Query with additional context.

Apply this diff:

-			return err
+			return fmt.Errorf("querier.Query failed: %w", err)

Line range hint 135-137: Wrap stream send error with context

Similarly, wrapping the error from stream.SendMsg provides more insight into where the failure occurred.

Apply this diff:

-			if err != nil {
-				return err
-			}
+			if err != nil {
+				return fmt.Errorf("failed to send message over stream: %w", err)
+			}

Line range hint 207-209: Check for nil before stopping the gRPC server

Ensure that s.grpcSrv is not nil before calling GracefulStop() to prevent potential nil pointer dereference.

Apply this diff to add a nil check:

	func (s *Server[T]) Stop(ctx context.Context) error {
		if !s.config.Enable {
			return nil
		}

		s.logger.Info("stopping gRPC server...", "address", s.config.Address)
+		if s.grpcSrv != nil {
			s.grpcSrv.GracefulStop()
+		}
		return nil
	}

Line range hint 126-127: Handle client cancellation separately

When receiving messages from the stream, differentiate between client cancellation and other errors to provide accurate responses and maintain server robustness.

Consider modifying the error handling to check for context cancellation:

				if errors.Is(err, io.EOF) {
					return nil
				}
+				if errors.Is(err, context.Canceled) {
+					return status.Error(codes.Canceled, "client cancelled the request")
+				}
				return err
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between cc9aab4 and a79efec.

📒 Files selected for processing (1)
  • server/v2/api/grpc/server.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/grpc/server.go (1)

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

🔇 Additional comments (1)
server/v2/api/grpc/server.go (1)

91-91: ⚠️ Potential issue

Undefined function sync.OnceValues

The function sync.OnceValues used here is undefined in the Go sync package. Please ensure that OnceValues is correctly implemented or imported to avoid potential runtime errors.

Comment on lines +105 to +107
desc, err := registry.FindDescriptorByName(fullName)
if err != nil {
return fmt.Errorf("failed to find descriptor %s: %w", method, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Return appropriate gRPC error when descriptor is not found

Instead of returning a generic error, consider returning a gRPC status error to provide clearer feedback to the client when the descriptor is not found.

Apply this diff to modify the error handling:

			if err != nil {
-				return fmt.Errorf("failed to find descriptor %s: %w", method, err)
+				return status.Errorf(codes.Unimplemented, "method %s not found: %v", method, err)
			}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
desc, err := registry.FindDescriptorByName(fullName)
if err != nil {
return fmt.Errorf("failed to find descriptor %s: %w", method, err)
desc, err := registry.FindDescriptorByName(fullName)
if err != nil {
return status.Errorf(codes.Unimplemented, "method %s not found: %v", method, err)

Merged via the queue into main with commit 0f63ade Oct 3, 2024
74 of 75 checks passed
@testinginprod testinginprod deleted the tip/handlers_continuation branch October 3, 2024 10:16
mergify bot pushed a commit that referenced this pull request Oct 3, 2024
…22043)

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
(cherry picked from commit 0f63ade)

# Conflicts:
#	runtime/v2/app.go
#	runtime/v2/manager.go
#	runtime/v2/module.go
#	server/v2/api/grpc/server.go
#	server/v2/cometbft/go.mod
#	server/v2/go.mod
#	server/v2/go.sum
#	server/v2/server_test.go
#	server/v2/types.go
julienrbrt added a commit that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release C:server/v2 api C:server/v2 cometbft C:server/v2 Issues related to server/v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants