proto/cosmos/base/grpc/v2/service.proto (2)

8-12: Consider adding service and RPC method documentation

While the service design is solid, adding documentation comments would help developers understand:

• The purpose of the Service
• When to use Query vs ListQueryHandlers
• The expected types that can be passed in the Any message

Add proto comments like this:

 // Service provides a generic query interface for cosmos-sdk applications
 service Service {
   // Query handles dynamic querying of application state
   rpc Query(QueryRequest) returns (QueryResponse) {}

   // ListQueryHandlers returns all available query handlers supported by this service
   rpc ListQueryHandlers(ListQueryHandlersRequest) returns (ListQueryHandlersResponse) {}
 }

---

28-31: Enhance Handler message field naming and documentation

The Handler message fields could be more descriptive:

Consider this improvement:

 message Handler {
-  string request_name  = 1;
-  string response_name = 2;
   // fully_qualified_request_type is the protobuf message type URL of the request
   string fully_qualified_request_type  = 1;
   // fully_qualified_response_type is the protobuf message type URL of the response
   string fully_qualified_response_type = 2;
 }

server/v2/api/grpc/service_test.go (4)

27-29: Enhance ValidateBasic implementation.

The ValidateBasic method should validate the Data field to ensure it's not empty or malformed.

func (m *MockRequestMessage) ValidateBasic() error {
-	return nil
 	if m.Data == "" {
 		return fmt.Errorf("data field cannot be empty")
 	}
 	return nil
}

---

25-25: Improve String() implementation for better debugging.

The String() method returns an empty string, which isn't helpful for debugging. Consider implementing a meaningful string representation.

-func (m *MockRequestMessage) String() string { return "" }
 func (m *MockRequestMessage) String() string {
 	return fmt.Sprintf("MockRequestMessage{Data: %s}", m.Data)
}

Also applies to: 36-36

---

61-158: Enhance test coverage and readability.

Consider the following improvements:

1. Add test case for invalid request message (malformed protobuf)
2. Use a named constant for the height parameter instead of magic number 0
3. Make test data more descriptive (e.g., "request" -> "test_query_data")

Example additional test case:

{
    name: "invalid request message",
    setupMock: func(app *mockApp[transaction.Tx]) {
        app.On("QueryHandlers").Return(map[string]appmodulev2.Handler{})
    },
    request: &QueryRequest{
        Request: &gogoproto.Any{
            TypeUrl: "/MockRequestMessage",
            Value:   []byte("invalid protobuf"),
        },
    },
    expectError:   true,
    expectedError: "rpc error: code = InvalidArgument desc = failed to unmarshal request",
},

---

160-209: Add more test cases for ListQueryHandlers.

The current test suite only covers the success scenario. Consider adding test cases for:

1. Empty handlers map
2. Multiple handlers
3. Error scenarios (if applicable)

Example additional test case:

{
    name: "empty handlers",
    setupMock: func(app *mockApp[transaction.Tx]) {
        app.On("QueryHandlers").Return(map[string]appmodulev2.Handler{})
    },
},

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

76-85: Ensure deterministic ordering when listing query handlers

Iterating over maps in Go does not guarantee order, which may lead to non-deterministic responses. If the order of handlers in ListQueryHandlersResponse matters, consider sorting the handler names.

Apply this diff to sort the handler names:

 import (
     "sort"
 )

 var handlerDescriptors []*Handler
 handlerNames := make([]string, 0, len(handlersMap))
 for handlerName := range handlersMap {
     handlerNames = append(handlerNames, handlerName)
 }
 sort.Strings(handlerNames)

-for handlerName := range handlersMap {
 for _, handlerName := range handlerNames {
     msg := handlersMap[handlerName].MakeMsg()
     resp := handlersMap[handlerName].MakeMsgResp()

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

api/cosmos/base/grpc/v2/service.pulsar.go (2)

5-5: Unnecessary alias for standard library import

On line 5, the standard library package fmt is imported with an alias, which is unnecessary:

import (
	fmt "fmt"

Per the Uber Go Style Guide, standard library packages should be imported without aliases unless there's a naming conflict. Removing the alias improves code clarity.

Apply this diff to remove the unnecessary alias:

 import (
-	fmt "fmt"
 	"fmt"

---

11-13: Unnecessary aliases for standard library imports

Lines 11-13 import standard library packages with aliases, which is unnecessary:

	io "io"
	reflect "reflect"
	sync "sync"

According to the Uber Go Style Guide, standard library packages should be imported without aliases unless there's a naming conflict. Removing these aliases enhances readability.

Apply this diff to remove the unnecessary aliases:

 import (
 	// other imports
-	io "io"
-	reflect "reflect"
-	sync "sync"
 	"io"
 	"reflect"
 	"sync"
 )

Configuration used: .coderabbit.yml
Review profile: CHILL

proto/cosmos/base/grpc/v2/service.proto (2)

8-12: Add documentation comments for the Service and RPCs.

Please add documentation comments to describe:

• The purpose and functionality of the Service
• The expected behavior and use cases for the Query RPC
• The purpose and return format of the ListQueryHandlers RPC

Example:

 // Service provides a generic query interface for cosmos-sdk state.
 service Service {
   // Query handles arbitrary query requests from clients and returns the corresponding responses.
   rpc Query(QueryRequest) returns (QueryResponse) {}

   // ListQueryHandlers returns a list of registered query handlers and their associated request/response types.
   rpc ListQueryHandlers(ListQueryHandlersRequest) returns (ListQueryHandlersResponse) {}
 }

The service design looks good, providing a flexible query interface using protobuf Any type.

🧰 Tools

🪛 GitHub Check: lint

[failure] 8-8:
Service "Service" should have a non-empty comment for documentation.

---

[failure] 9-9:
RPC "Query" should have a non-empty comment for documentation.

---

[failure] 11-11:
RPC "ListQueryHandlers" should have a non-empty comment for documentation.

---

14-31: Add documentation comments for all message types.

Please add documentation comments to describe each message type and its fields. Example:

 // QueryRequest wraps an arbitrary protobuf message as a query request.
 message QueryRequest {
   // request is the actual query request message.
   google.protobuf.Any request = 1;
 }

 // QueryResponse wraps an arbitrary protobuf message as a query response.
 message QueryResponse {
   // response contains the query result message.
   google.protobuf.Any response = 1;
 }

 // ListQueryHandlersRequest is used to request a list of all registered query handlers.
 message ListQueryHandlersRequest {}

 // ListQueryHandlersResponse contains a list of all registered query handlers.
 message ListQueryHandlersResponse {
   // handlers is the list of registered query handlers.
   repeated Handler handlers = 1;
 }

 // Handler describes a registered query handler's request and response types.
 message Handler {
   // request_name is the fully-qualified protobuf message name of the handler's request type.
   string request_name  = 1;
   // response_name is the fully-qualified protobuf message name of the handler's response type.
   string response_name = 2;
 }

The message definitions are well-structured and follow protobuf best practices:

• Using Any type for generic request/response handling
• Empty request message for ListQueryHandlers is appropriate
• Handler message provides necessary type information for client code generation

🧰 Tools

🪛 GitHub Check: lint

[failure] 14-14:
Message "QueryRequest" should have a non-empty comment for documentation.

---

[failure] 18-18:
Message "QueryResponse" should have a non-empty comment for documentation.

---

[failure] 22-22:
Message "ListQueryHandlersRequest" should have a non-empty comment for documentation.

---

[failure] 24-24:
Message "ListQueryHandlersResponse" should have a non-empty comment for documentation.

---

[failure] 28-28:
Message "Handler" should have a non-empty comment for documentation.

Configuration used: .coderabbit.yml
Review profile: CHILL

proto/cosmos/base/grpc/v2/service.proto (2)

8-15: Enhance service documentation for better clarity

While the service structure is good, the RPC documentation could be more descriptive:

• For Query: Specify what types of requests are supported and any constraints
• For ListQueryHandlers: Explain the purpose of handlers and when to use this endpoint

Consider updating the comments like this:

-// Service defines the gRPC service for query server for v2
 // Service defines the gRPC query service that allows clients to send dynamic queries
 // to the Cosmos SDK v2 server.
 service Service {
-  // Query queries the server with a request, the request can be any sdk Msg.
   // Query executes a query against the server. The request must be a valid protobuf
   // message type registered with the server's query handlers.
   rpc Query(QueryRequest) returns (QueryResponse) {}

-  // ListQueryHandlers lists all the available query handlers.
   // ListQueryHandlers returns a list of all registered query handlers and their
   // corresponding request/response message types, helping clients discover available queries.
   rpc ListQueryHandlers(ListQueryHandlersRequest) returns (ListQueryHandlersResponse) {}
 }

---

17-39: Consider future scalability improvements

The message definitions are well-structured, but consider these future enhancements:

1. ListQueryHandlersRequest might need pagination support as the number of handlers grows
2. Handler message could benefit from additional metadata fields like:
   • Description of the handler's purpose
   • Deprecated status
   • Required permissions

Would you like me to propose a detailed design for these enhancements?

Configuration used: .coderabbit.yml
Review profile: CHILL