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: wire new handlers to grpc #22333

Merged
merged 17 commits into from
Nov 5, 2024
2,585 changes: 2,585 additions & 0 deletions api/cosmos/base/grpc/v2/service.pulsar.go

Large diffs are not rendered by default.

159 changes: 159 additions & 0 deletions api/cosmos/base/grpc/v2/service_grpc.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions proto/cosmos/base/grpc/v2/service.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
syntax = "proto3";
package cosmos.base.grpc.v2;

import "google/protobuf/any.proto";

option go_package = "github.com/cosmos/cosmos-sdk/server/v2/api/grpc";
randygrok marked this conversation as resolved.
Show resolved Hide resolved

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

Package directory structure needs adjustment

The package "cosmos.base.grpc.v2" should be within a directory "cosmos/base/grpc/v2" relative to root, but it's currently in "proto/cosmos/base/grpc/v2". This mismatch could cause issues with buf and other Protocol Buffer tools.

Consider moving the file to the correct directory structure or adjusting the package name to match the current directory structure.

🧰 Tools
🪛 buf

2-2: Files with package "cosmos.base.grpc.v2" must be within a directory "cosmos/base/grpc/v2" relative to root but were in directory "proto/cosmos/base/grpc/v2".

(PACKAGE_DIRECTORY_MATCH)

service Service {
rpc Query(QueryRequest) returns (QueryResponse) {}

rpc ListQueryHandlers(ListQueryHandlersRequest) returns (ListQueryHandlersResponse) {}
}

message QueryRequest {
google.protobuf.Any request = 1;
}

message QueryResponse {
google.protobuf.Any response = 1;
}

message ListQueryHandlersRequest {}

message ListQueryHandlersResponse {
repeated Handler handlers = 1;
}

message Handler {
string request_name = 1;
string response_name = 2;
}
2 changes: 2 additions & 0 deletions server/v2/api/grpc/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ func (s *Server[T]) Init(appI serverv2.AppI[T], cfg map[string]any, logger log.L
// Reflection allows external clients to see what services and methods the gRPC server exposes.
gogoreflection.Register(grpcSrv, slices.Collect(maps.Keys(methodsMap)), logger.With("sub-module", "grpc-reflection"))

RegisterV2Service(grpcSrv, appI)

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 error handling and documentation for service registration.

The RegisterV2Service call should include error handling to ensure proper service registration. Additionally, consider adding a comment explaining the purpose of this registration.

Apply this diff:

+	// Register the v2 gRPC service for handling queries
+	if err := RegisterV2Service(grpcSrv, appI); err != nil {
+		return fmt.Errorf("failed to register v2 service: %w", err)
+	}
-	RegisterV2Service(grpcSrv, appI)
📝 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
RegisterV2Service(grpcSrv, appI)
// Register the v2 gRPC service for handling queries
if err := RegisterV2Service(grpcSrv, appI); err != nil {
return fmt.Errorf("failed to register v2 service: %w", err)
}

s.grpcSrv = grpcSrv
s.config = serverCfg
s.logger = logger.With(log.ModuleKey, s.Name())
Expand Down
88 changes: 88 additions & 0 deletions server/v2/api/grpc/service.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package grpc

import (
"context"

"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"cosmossdk.io/core/transaction"
serverv2 "cosmossdk.io/server/v2"
"github.com/cosmos/gogoproto/proto"
gogoproto "github.com/cosmos/gogoproto/types/any"
)

// RegisterV2Service registers the V2 gRPC service implementation with the given server.
// It takes a generic type T that implements the transaction.Tx interface.
func RegisterV2Service[T transaction.Tx](server *grpc.Server, app serverv2.AppI[T]) {
randygrok marked this conversation as resolved.
Show resolved Hide resolved
if server == nil || app == nil {
panic("server and app must not be nil")
}
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 panicking; return an error instead in RegisterV2Service

Panicking on invalid input can cause the application to crash unexpectedly. It's better to return an error and allow the caller to handle it appropriately.

Apply this diff to return an error:

-func RegisterV2Service[T transaction.Tx](server *grpc.Server, app serverv2.AppI[T]) {
+func RegisterV2Service[T transaction.Tx](server *grpc.Server, app serverv2.AppI[T]) error {
     if server == nil || app == nil {
-        panic("server and app must not be nil")
+        return errors.New("server and app must not be nil")
     }
     RegisterServiceServer(server, NewV2Service(app))
+    return nil
 }
📝 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
if server == nil || app == nil {
panic("server and app must not be nil")
}
if server == nil || app == nil {
return errors.New("server and app must not be nil")
}

RegisterServiceServer(server, NewV2Service(app))
}

// V2Service implements the gRPC service interface for handling queries and listing handlers.
type V2Service[T transaction.Tx] struct {
app serverv2.AppI[T]
}

// NewV2Service creates a new instance of V2Service with the provided app.
func NewV2Service[T transaction.Tx](app serverv2.AppI[T]) *V2Service[T] {
return &V2Service[T]{app: app}
}

// Query handles incoming query requests by unmarshaling the request, processing it,
// and returning the response in an Any protobuf message.
func (s V2Service[T]) Query(ctx context.Context, request *QueryRequest) (*QueryResponse, error) {
if request == nil || request.Request == nil {
return nil, status.Error(codes.InvalidArgument, "empty request")
}

msgName := request.Request.TypeUrl
handlers := s.app.QueryHandlers()

handler, exists := handlers[msgName]
if !exists {
return nil, status.Errorf(codes.NotFound, "handler not found for %s", msgName)
}
Comment on lines +34 to +35
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 exposing internal error details to clients

Returning detailed error messages can leak internal information and pose security risks. Consider returning generic error messages to the client and logging the detailed errors internally.

Apply this diff to adjust the error messages:

     if !exists {
-        return nil, status.Errorf(codes.NotFound, "handler not found for %s", msgName)
+        return nil, status.Error(codes.NotFound, "handler not found")
     }

     if err := proto.Unmarshal(request.Request.Value, protoMsg); err != nil {
-        return nil, status.Errorf(codes.InvalidArgument, "failed to unmarshal request: %v", err)
+        return nil, status.Error(codes.InvalidArgument, "invalid request format")
     }

     if err != nil {
-        return nil, status.Errorf(codes.Internal, "query failed: %v", err)
+        return nil, status.Error(codes.Internal, "internal server error")
     }

Also applies to: 52-53, 57-58


protoMsg := handler.MakeMsg()
if err := proto.Unmarshal(request.Request.Value, protoMsg); err != nil {
return nil, status.Errorf(codes.InvalidArgument, "failed to unmarshal request: %v", err)
}

queryResp, err := s.app.Query(ctx, 0, protoMsg)
Copy link
Member

Choose a reason for hiding this comment

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

this only works for latest height, we should add a way to query historical info as well

Copy link
Member

Choose a reason for hiding this comment

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

This comment should be done in a follow-up indeed. Let's track this.

if err != nil {
return nil, status.Errorf(codes.Internal, "query failed: %v", err)
}

respBytes, err := proto.Marshal(queryResp)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to marshal response: %v", err)
}

anyResp := &gogoproto.Any{
TypeUrl: "/" + proto.MessageName(queryResp),
Value: respBytes,
}

return &QueryResponse{Response: anyResp}, nil
}

func (s V2Service[T]) ListQueryHandlers(_ context.Context, _ *ListQueryHandlersRequest) (*ListQueryHandlersResponse, error) {
handlersMap := s.app.QueryHandlers()

var handlerDescriptors []*Handler
for handlerName := range handlersMap {
msg := handlersMap[handlerName].MakeMsg()
resp := handlersMap[handlerName].MakeMsgResp()

handlerDescriptors = append(handlerDescriptors, &Handler{
RequestName: proto.MessageName(msg),
ResponseName: proto.MessageName(resp),
})
}
Fixed Show fixed Hide fixed
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

Ensure deterministic iteration over map to avoid non-determinism

Iterating over a map in Go does not guarantee order, which may lead to non-deterministic results. If the order of handlerDescriptors is important, consider sorting the handler names before iterating over them.

Apply this diff to sort the handler names:

+import (
+    "sort"
+    // other imports
+)

 var handlerDescriptors []*Handler
+var handlerNames []string
 for handlerName := range handlersMap {
+    handlerNames = append(handlerNames, handlerName)
+}
+
+sort.Strings(handlerNames)
+
+for _, handlerName := range handlerNames {
     msg := handlersMap[handlerName].MakeMsg()
     resp := handlersMap[handlerName].MakeMsgResp()

     handlerDescriptors = append(handlerDescriptors, &Handler{
         RequestName:  proto.MessageName(msg),
         ResponseName: proto.MessageName(resp),
     })
 }

This ensures that the handlers are processed in a consistent order.

📝 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
for handlerName := range handlersMap {
msg := handlersMap[handlerName].MakeMsg()
resp := handlersMap[handlerName].MakeMsgResp()
handlerDescriptors = append(handlerDescriptors, &Handler{
RequestName: proto.MessageName(msg),
ResponseName: proto.MessageName(resp),
})
}
var handlerDescriptors []*Handler
var handlerNames []string
for handlerName := range handlersMap {
handlerNames = append(handlerNames, handlerName)
}
sort.Strings(handlerNames)
for _, handlerName := range handlerNames {
msg := handlersMap[handlerName].MakeMsg()
resp := handlersMap[handlerName].MakeMsgResp()
handlerDescriptors = append(handlerDescriptors, &Handler{
RequestName: proto.MessageName(msg),
ResponseName: proto.MessageName(resp),
})
}


return &ListQueryHandlersResponse{Handlers: handlerDescriptors}, nil
}
Loading
Loading