feat: wire new handlers to grpc #22333
Conversation
github-actions
bot
added
C:server/v2
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| protoimpl "google.golang.org/protobuf/runtime/protoimpl" | ||
| anypb "google.golang.org/protobuf/types/known/anypb" | ||
| io "io" | ||
| reflect "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
| x.Request = value.Message().Interface().(*anypb.Any) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.QueryRequest")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.QueryRequest")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.grpc.v2.QueryRequest does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.Response = value.Message().Interface().(*anypb.Any) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.QueryResponse")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.QueryResponse")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.grpc.v2.QueryResponse does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.ListQueryHandlersRequest")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.grpc.v2.ListQueryHandlersRequest does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.Handlers = *clv.list | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.ListQueryHandlersResponse")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.ListQueryHandlersResponse")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.grpc.v2.ListQueryHandlersResponse does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.ResponseName = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.Handler")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.Handler")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.grpc.v2.Handler does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new gRPC service for the Cosmos ecosystem, defining several message types and service methods for handling queries. Key additions include Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)server/v2/api/grpc/service.go (1)Pattern 🔇 Additional comments (1)server/v2/api/grpc/service.go (1)
The code effectively defines the gRPC service with appropriate methods and error handling. The implementation is clean and follows the Uber Golang style guide. 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
randygrok
requested review from
a team,
julienrbrt and
hieuvubk
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (9)
proto/cosmos/base/grpc/v2/service.proto (2)
8-12: Consider adding service and RPC method documentationWhile 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 documentationThe 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
ValidateBasicmethod should validate theDatafield 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:
- Add test case for invalid request message (malformed protobuf)
- Use a named constant for the height parameter instead of magic number 0
- 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:
- Empty handlers map
- Multiple handlers
- 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 handlersIterating over maps in Go does not guarantee order, which may lead to non-deterministic responses. If the order of handlers in
ListQueryHandlersResponsematters, 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 importOn line 5, the standard library package
fmtis 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 importsLines 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" )
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
api/cosmos/base/grpc/v2/service_grpc.pb.gois excluded by!**/*.pb.goserver/v2/api/grpc/service.pb.gois excluded by!**/*.pb.goserver/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
- api/cosmos/base/grpc/v2/service.pulsar.go (1 hunks)
- proto/cosmos/base/grpc/v2/service.proto (1 hunks)
- server/v2/api/grpc/server.go (1 hunks)
- server/v2/api/grpc/service.go (1 hunks)
- server/v2/api/grpc/service_test.go (1 hunks)
- server/v2/go.mod (5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
api/cosmos/base/grpc/v2/service.pulsar.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/api/grpc/service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpc/service_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"
🪛 buf
proto/cosmos/base/grpc/v2/service.proto
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)
🔇 Additional comments (6)
proto/cosmos/base/grpc/v2/service.proto (1)
14-20: Consider adding validation for Any messagesThe QueryRequest and QueryResponse messages use Any type without apparent validation. Consider:
- Adding comments describing valid message types
- Implementing runtime validation in the service implementation
Let's check if there are any validation mechanisms in place:
server/v2/api/grpc/server.go (1)
74-75: Verify service implementation and integration.Let's verify that the service implementation exists and is properly integrated with the query handling system.
✅ Verification successful
Service implementation is properly integrated and functional
The verification shows that:
- The V2Service implementation exists in
server/v2/api/grpc/service.gowith proper query handling- The service registration function is correctly implemented with type safety checks
- The service provides core functionality for:
- Query handling with proper error management
- Query handler listing capability
- Integration with the app interface through
serverv2.AppI[T]🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the V2Service implementation and its integration # Test 1: Check if the service implementation exists echo "Checking for V2Service implementation..." rg -l "type V2Service" . # Test 2: Check the service registration function echo "Checking service registration implementation..." rg -l "func RegisterV2Service" # Test 3: Check for protobuf service definition echo "Checking for protobuf service definition..." fd -e proto | xargs rg "service.*V2"Length of output: 453
Script:
#!/bin/bash # Let's examine the service implementation and registration in detail # Check the content of the service implementation file echo "=== Service Implementation ===" cat ./server/v2/api/grpc/service.go # Look for any V2-related interfaces or types in the codebase echo -e "\n=== V2-related Types and Interfaces ===" rg "type.*V2" -A 3 # Check for any proto files that might contain the service definition echo -e "\n=== Proto Files in the Repository ===" fd -e protoLength of output: 23175
server/v2/api/grpc/service_test.go (1)
42-59: LGTM! Well-structured mock implementation.The mock app implementation is well-structured with proper error handling and type assertions.
server/v2/go.mod (3)
3-5: Verify Go toolchain compatibility.The toolchain version (1.23.2) is ahead of the specified Go version (1.23.1). While this usually works, it's recommended to keep them in sync to avoid potential compatibility issues.
37-37: Review dependency version updates.The updates to key dependencies appear to be minor version bumps:
- go-toml/v2: v2.2.2 -> v2.2.3
- cockroachdb/errors: v1.11.1 -> v1.11.3
- cockroachdb/pebble: v1.1.2
These are patch version updates and should not introduce breaking changes. However, please ensure that the changes in these dependencies, particularly in cockroachdb packages, don't affect the performance or behavior of your database operations.
Also applies to: 67-70
27-27: Verify dependency compatibility with new gRPC handlers.The addition of bank module and numerous indirect dependencies aligns with the PR's objective of wiring new gRPC handlers. However, let's verify the compatibility of these dependencies.
Also applies to: 51-157
✅ Verification successful
Based on the search results, I can see that the bank module is extensively integrated into the codebase with proper gRPC handlers and dependencies. The key findings show:
- The bank module has proper gRPC service definitions and handlers in both v1beta1 and v2 versions
- The module is properly integrated with protobuf/gRPC infrastructure
- The handlers are properly wired up in the keeper layer
- The module is used extensively across tests and other modules
- The dependency is used consistently across the codebase
Let me verify one more aspect about the handlers implementation:
Let me check the actual handler implementations:
Let me check the full handler implementations to verify all gRPC handlers are properly implemented:
Bank module's gRPC handlers are properly implemented and compatible
The verification confirms that the bank module's gRPC handlers are properly implemented with all necessary message handlers and query handlers:
Message handlers:
MsgUpdateParams- For parameter updatesMsgSend- For coin transfersMsgMint- For minting new coinsQuery handlers:
QueryParams- For querying module parametersQueryBalance- For querying account balancesThe handlers include proper input validation, address encoding/decoding, and error handling. The dependency is well integrated and compatible with the gRPC infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential dependency conflicts or issues # Test 1: Check for any dependency conflicts go mod verify # Test 2: Check if there are any unnecessary dependencies go mod tidy -v # Test 3: Check if all dependencies are being used go mod why -m cosmossdk.io/x/bankLength of output: 279
Script:
#!/bin/bash # Let's analyze the Go imports and dependencies using grep and ast-grep # since Go toolchain is not available in the sandbox # Check for bank module imports and usage rg -l "cosmossdk.io/x/bank" --type go # Check for gRPC related imports and handlers rg "google.golang.org/grpc" --type go -A 2 # Look for bank module usage in handlers ast-grep --pattern 'func $_($_ *server.Context) $_Handler { $$$ }' # Check for any bank module type usage rg "bank\." --type go -A 2Length of output: 436716
Script:
#!/bin/bash # Check bank module handler implementations rg -l "func.*Handler" x/bank/v2/keeper/Length of output: 70
Script:
#!/bin/bash # Check the bank module handler implementations rg "func.*Handler" x/bank/v2/keeper/handlers.go -A 5Length of output: 303
Script:
#!/bin/bash # Check all handler implementations in the bank module cat x/bank/v2/keeper/handlers.goLength of output: 4109
| syntax = "proto3"; | ||
| package cosmos.base.grpc.v2; | ||
|
|
||
| import "google/protobuf/any.proto"; | ||
|
|
||
| option go_package = "github.com/cosmos/cosmos-sdk/server/v2/api/grpc"; | ||
|
|
There was a problem hiding this comment.
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)
server/v2/api/grpc/server.go
Outdated
| RegisterV2Service(grpcSrv, appI) | ||
|
|
There was a problem hiding this comment.
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.
| 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) | |
| } |
| type MockResponseMessage struct { | ||
| Data string `protobuf:"bytes,1,opt,name=data,proto3" json:"data,omitempty"` | ||
| } | ||
|
|
||
| func (m *MockResponseMessage) Reset() {} | ||
| func (m *MockResponseMessage) String() string { return "" } | ||
| func (m *MockResponseMessage) ProtoMessage() {} | ||
| func (m *MockResponseMessage) ValidateBasic() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add XXX_MessageName method to MockResponseMessage.
For consistency with MockRequestMessage, implement the XXX_MessageName method for MockResponseMessage.
+func (m *MockResponseMessage) XXX_MessageName() string {
+ return "MockResponseMessage"
+}📝 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.
| type MockResponseMessage struct { | |
| Data string `protobuf:"bytes,1,opt,name=data,proto3" json:"data,omitempty"` | |
| } | |
| func (m *MockResponseMessage) Reset() {} | |
| func (m *MockResponseMessage) String() string { return "" } | |
| func (m *MockResponseMessage) ProtoMessage() {} | |
| func (m *MockResponseMessage) ValidateBasic() error { | |
| return nil | |
| } | |
| type MockResponseMessage struct { | |
| Data string `protobuf:"bytes,1,opt,name=data,proto3" json:"data,omitempty"` | |
| } | |
| func (m *MockResponseMessage) Reset() {} | |
| func (m *MockResponseMessage) String() string { return "" } | |
| func (m *MockResponseMessage) ProtoMessage() {} | |
| func (m *MockResponseMessage) ValidateBasic() error { | |
| return nil | |
| } | |
| func (m *MockResponseMessage) XXX_MessageName() string { | |
| return "MockResponseMessage" | |
| } |
server/v2/api/grpc/service_test.go
Outdated
| func createTestRequest(t *testing.T) *QueryRequest { | ||
| reqMsg := &MockRequestMessage{Data: "request"} | ||
| reqBytes, err := proto.Marshal(reqMsg) | ||
| if err != nil { | ||
| t.Fatalf("failed to marshal request: %v", err) | ||
| } | ||
|
|
||
| return &QueryRequest{ | ||
| Request: &gogoproto.Any{ | ||
| TypeUrl: "/" + proto.MessageName(reqMsg), | ||
| Value: reqBytes, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make createTestRequest more flexible.
Consider parameterizing the helper function to make it more reusable for different test scenarios.
-func createTestRequest(t *testing.T) *QueryRequest {
+func createTestRequest(t *testing.T, data string) *QueryRequest {
- reqMsg := &MockRequestMessage{Data: "request"}
+ reqMsg := &MockRequestMessage{Data: data}
reqBytes, err := proto.Marshal(reqMsg)
if err != nil {
t.Fatalf("failed to marshal request: %v", err)
}
return &QueryRequest{
Request: &gogoproto.Any{
TypeUrl: "/" + proto.MessageName(reqMsg),
Value: reqBytes,
},
}
}📝 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.
| func createTestRequest(t *testing.T) *QueryRequest { | |
| reqMsg := &MockRequestMessage{Data: "request"} | |
| reqBytes, err := proto.Marshal(reqMsg) | |
| if err != nil { | |
| t.Fatalf("failed to marshal request: %v", err) | |
| } | |
| return &QueryRequest{ | |
| Request: &gogoproto.Any{ | |
| TypeUrl: "/" + proto.MessageName(reqMsg), | |
| Value: reqBytes, | |
| }, | |
| } | |
| } | |
| func createTestRequest(t *testing.T, data string) *QueryRequest { | |
| reqMsg := &MockRequestMessage{Data: data} | |
| reqBytes, err := proto.Marshal(reqMsg) | |
| if err != nil { | |
| t.Fatalf("failed to marshal request: %v", err) | |
| } | |
| return &QueryRequest{ | |
| Request: &gogoproto.Any{ | |
| TypeUrl: "/" + proto.MessageName(reqMsg), | |
| Value: reqBytes, | |
| }, | |
| } | |
| } |
| return nil, status.Errorf(codes.NotFound, "handler not found for %s", msgName) | ||
| } |
There was a problem hiding this comment.
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
server/v2/api/grpc/service.go
Outdated
| if server == nil || app == nil { | ||
| panic("server and app must not be nil") | ||
| } |
There was a problem hiding this comment.
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.
| 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") | |
| } |
| // Code generated by protoc-gen-go-pulsar. DO NOT EDIT. | ||
| package grpcv2 |
There was a problem hiding this comment.
Inconsistent code generation tools used
The file starts with a code generation notice from protoc-gen-go-pulsar on lines 1-2:
// Code generated by protoc-gen-go-pulsar. DO NOT EDIT.However, later in the file at lines 2220-2221, there is another code generation notice indicating the code was generated by protoc-gen-go:
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:Mixing generated code from different tools in the same file may lead to conflicts, unexpected behavior, and maintenance challenges. Consider separating the generated code into different files or ensuring that only a single code generator is used for this file to maintain consistency.
| unmarshal := func(input protoiface.UnmarshalInput) (protoiface.UnmarshalOutput, error) { | ||
| x := input.Message.Interface().(*QueryRequest) | ||
| if x == nil { | ||
| return protoiface.UnmarshalOutput{ | ||
| NoUnkeyedLiterals: input.NoUnkeyedLiterals, | ||
| Flags: input.Flags, | ||
| }, nil | ||
| } | ||
| options := runtime.UnmarshalInputToOptions(input) | ||
| _ = options | ||
| dAtA := input.Buf | ||
| l := len(dAtA) | ||
| iNdEx := 0 | ||
| for iNdEx < l { | ||
| preIndex := iNdEx | ||
| var wire uint64 | ||
| for shift := uint(0); ; shift += 7 { | ||
| if shift >= 64 { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, runtime.ErrIntOverflow | ||
| } | ||
| if iNdEx >= l { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, io.ErrUnexpectedEOF | ||
| } | ||
| b := dAtA[iNdEx] | ||
| iNdEx++ | ||
| wire |= uint64(b&0x7F) << shift | ||
| if b < 0x80 { | ||
| break | ||
| } | ||
| } | ||
| fieldNum := int32(wire >> 3) | ||
| wireType := int(wire & 0x7) | ||
| if wireType == 4 { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, fmt.Errorf("proto: QueryRequest: wiretype end group for non-group") | ||
| } | ||
| if fieldNum <= 0 { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, fmt.Errorf("proto: QueryRequest: illegal tag %d (wire type %d)", fieldNum, wire) | ||
| } | ||
| switch fieldNum { | ||
| case 1: | ||
| if wireType != 2 { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, fmt.Errorf("proto: wrong wireType = %d for field Request", wireType) | ||
| } | ||
| var msglen int | ||
| for shift := uint(0); ; shift += 7 { | ||
| if shift >= 64 { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, runtime.ErrIntOverflow | ||
| } | ||
| if iNdEx >= l { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, io.ErrUnexpectedEOF | ||
| } | ||
| b := dAtA[iNdEx] | ||
| iNdEx++ | ||
| msglen |= int(b&0x7F) << shift | ||
| if b < 0x80 { | ||
| break | ||
| } | ||
| } | ||
| if msglen < 0 { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, runtime.ErrInvalidLength | ||
| } | ||
| postIndex := iNdEx + msglen | ||
| if postIndex < 0 { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, runtime.ErrInvalidLength | ||
| } | ||
| if postIndex > l { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, io.ErrUnexpectedEOF | ||
| } | ||
| if x.Request == nil { | ||
| x.Request = &anypb.Any{} | ||
| } | ||
| if err := options.Unmarshal(dAtA[iNdEx:postIndex], x.Request); err != nil { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, err | ||
| } | ||
| iNdEx = postIndex | ||
| default: | ||
| iNdEx = preIndex | ||
| skippy, err := runtime.Skip(dAtA[iNdEx:]) | ||
| if err != nil { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, err | ||
| } | ||
| if (skippy < 0) || (iNdEx+skippy) < 0 { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, runtime.ErrInvalidLength | ||
| } | ||
| if (iNdEx + skippy) > l { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, io.ErrUnexpectedEOF | ||
| } | ||
| if !options.DiscardUnknown { | ||
| x.unknownFields = append(x.unknownFields, dAtA[iNdEx:iNdEx+skippy]...) | ||
| } | ||
| iNdEx += skippy | ||
| } | ||
| } | ||
|
|
||
| if iNdEx > l { | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, io.ErrUnexpectedEOF | ||
| } | ||
| return protoiface.UnmarshalOutput{NoUnkeyedLiterals: input.NoUnkeyedLiterals, Flags: input.Flags}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor long function for better maintainability
The unmarshal function in ProtoMethods spans over 98 lines (lines 341-439). Long functions can be difficult to read, test, and maintain. Per the Uber Go Style Guide, functions should be kept short and well-focused, ideally not exceeding 40 lines.
Consider refactoring the unmarshal function by breaking it into smaller, reusable helper functions that handle specific tasks such as:
- Parsing the wire format
- Processing each field based on
fieldNum - Error handling and validation
This will improve readability and maintainability of the code.
| // Code generated by protoc-gen-go. DO NOT EDIT. | ||
| // versions: |
There was a problem hiding this comment.
Duplicate code generation notice
At lines 2220-2221, there's a second code generation notice:
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:This suggests that the file includes code generated by protoc-gen-go in addition to the earlier code generated by protoc-gen-go-pulsar. Combining code from different generators in a single file may lead to confusion and potential conflicts. Ensure that the code is generated consistently using a single tool, or separate the generated code into distinct files.
|
@randygrok your pull request is missing a changelog! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
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
QueryRPC- The purpose and return format of the
ListQueryHandlersRPCExample:
+// 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.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
server/v2/api/grpc/service.pb.gois excluded by!**/*.pb.goserver/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
- proto/cosmos/base/grpc/v2/service.proto (1 hunks)
- server/v2/api/grpc/server.go (1 hunks)
- server/v2/api/grpc/service.go (1 hunks)
- server/v2/go.mod (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/v2/api/grpc/server.go
- server/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/grpc/service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf
proto/cosmos/base/grpc/v2/service.proto
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)
🪛 GitHub Check: lint
proto/cosmos/base/grpc/v2/service.proto
[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.
[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.
🔇 Additional comments (2)
server/v2/api/grpc/service.go (2)
20-26: Good job replacing panic with error return inRegisterV2ServiceThe function now returns an error instead of panicking when
serverorappis nil, which enhances code robustness and aligns with best practices.
50-51: Previous review comment is still valid regarding error messagesThe earlier feedback about avoiding exposure of internal error details to clients remains applicable. Please consider adjusting the error messages to be more generic.
Also applies to: 55-56, 59-60
server/v2/go.mod
Outdated
| cosmossdk.io/server/v2/appmanager => ./appmanager | ||
| cosmossdk.io/server/v2/stf => ./stf | ||
| cosmossdk.io/store/v2 => ../../store/v2 | ||
| cosmossdk.io/store/v2/db => ../../store/v2/db | ||
| cosmossdk.io/x/bank => ../../x/bank | ||
| cosmossdk.io/x/staking => ../../x/staking |
server/v2/go.mod
Outdated
| cosmossdk.io/server/v2/appmanager => ./appmanager | ||
| cosmossdk.io/server/v2/stf => ./stf | ||
| cosmossdk.io/store/v2 => ../../store/v2 | ||
| cosmossdk.io/store/v2/db => ../../store/v2/db | ||
| cosmossdk.io/x/bank => ../../x/bank |
server/v2/go.mod
Outdated
|
|
||
| replace ( | ||
| cosmossdk.io/api => ../../api | ||
| cosmossdk.io/core => ../../core |
There was a problem hiding this comment.
Why? What are the core changes?
There was a problem hiding this comment.
if I dont include core it does not build
There was a problem hiding this comment.
wait, maybe now works
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
proto/cosmos/base/grpc/v2/service.proto (2)
8-15: Enhance service documentation for better clarityWhile 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 improvementsThe message definitions are well-structured, but consider these future enhancements:
ListQueryHandlersRequestmight need pagination support as the number of handlers growsHandlermessage 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?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
api/cosmos/base/grpc/v2/service_grpc.pb.gois excluded by!**/*.pb.goserver/v2/api/grpc/service.pb.gois excluded by!**/*.pb.goserver/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
- api/cosmos/base/grpc/v2/service.pulsar.go (1 hunks)
- proto/cosmos/base/grpc/v2/service.proto (1 hunks)
- server/v2/api/grpc/service.go (1 hunks)
- server/v2/api/grpc/service_test.go (1 hunks)
- server/v2/go.mod (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/v2/api/grpc/service_test.go
- server/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
api/cosmos/base/grpc/v2/service.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpc/service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf
proto/cosmos/base/grpc/v2/service.proto
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)
🔇 Additional comments (2)
proto/cosmos/base/grpc/v2/service.proto (1)
1-7: LGTM! Package setup follows best practicesThe package configuration and imports are well-structured. The use of
google.protobuf.Anyis appropriate for handling dynamic message types, and the go_package option is correctly set.🧰 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)
server/v2/api/grpc/service.go (1)
50-51: Avoid exposing internal error details to clientsReturning 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. This issue was previously raised and is still applicable.
Apply this diff to adjust the error messages:
- return nil, status.Errorf(codes.NotFound, "handler not found for %s", msgName) + return nil, status.Error(codes.NotFound, "handler not found") - return nil, status.Errorf(codes.InvalidArgument, "failed to unmarshal request: %v", err) + return nil, status.Error(codes.InvalidArgument, "invalid request format") - return nil, status.Errorf(codes.Internal, "query failed: %v", err) + return nil, status.Error(codes.Internal, "internal server error") - return nil, status.Errorf(codes.Internal, "failed to marshal response: %v", err) + return nil, status.Error(codes.Internal, "internal server error")Also applies to: 55-56, 60-61, 65-66
server/v2/api/grpc/service.go
Outdated
| for handlerName := range handlersMap { | ||
| msg := handlersMap[handlerName].MakeMsg() | ||
| resp := handlersMap[handlerName].MakeMsgResp() | ||
|
|
||
| handlerDescriptors = append(handlerDescriptors, &Handler{ | ||
| RequestName: proto.MessageName(msg), | ||
| ResponseName: proto.MessageName(resp), | ||
| }) | ||
| } |
There was a problem hiding this comment.
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.
| 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), | |
| }) | |
| } |
| // QueryRequest is the request for the Query method | ||
| type QueryRequest struct { | ||
| state protoimpl.MessageState | ||
| sizeCache protoimpl.SizeCache | ||
| unknownFields protoimpl.UnknownFields | ||
|
|
||
| Request *anypb.Any `protobuf:"bytes,1,opt,name=request,proto3" json:"request,omitempty"` | ||
| } | ||
|
|
||
| func (x *QueryRequest) Reset() { | ||
| *x = QueryRequest{} | ||
| if protoimpl.UnsafeEnabled { | ||
| mi := &file_cosmos_base_grpc_v2_service_proto_msgTypes[0] | ||
| ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) | ||
| ms.StoreMessageInfo(mi) | ||
| } | ||
| } | ||
|
|
||
| func (x *QueryRequest) String() string { | ||
| return protoimpl.X.MessageStringOf(x) | ||
| } | ||
|
|
||
| func (*QueryRequest) ProtoMessage() {} | ||
|
|
||
| // Deprecated: Use QueryRequest.ProtoReflect.Descriptor instead. | ||
| func (*QueryRequest) Descriptor() ([]byte, []int) { | ||
| return file_cosmos_base_grpc_v2_service_proto_rawDescGZIP(), []int{0} | ||
| } | ||
|
|
||
| func (x *QueryRequest) GetRequest() *anypb.Any { | ||
| if x != nil { | ||
| return x.Request | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming fields to avoid stuttering
In the QueryRequest struct (lines 2233-2267), the field Request results in expressions like QueryRequest.Request, which can be repetitive. According to the Uber Go Style Guide, it's recommended to avoid stuttering in names. Consider renaming the field to something more descriptive to enhance readability.
| type QueryResponse struct { | ||
| state protoimpl.MessageState | ||
| sizeCache protoimpl.SizeCache | ||
| unknownFields protoimpl.UnknownFields | ||
|
|
||
| Response *anypb.Any `protobuf:"bytes,1,opt,name=response,proto3" json:"response,omitempty"` | ||
| } | ||
|
|
||
| func (x *QueryResponse) Reset() { | ||
| *x = QueryResponse{} | ||
| if protoimpl.UnsafeEnabled { | ||
| mi := &file_cosmos_base_grpc_v2_service_proto_msgTypes[1] | ||
| ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) | ||
| ms.StoreMessageInfo(mi) | ||
| } | ||
| } | ||
|
|
||
| func (x *QueryResponse) String() string { | ||
| return protoimpl.X.MessageStringOf(x) | ||
| } | ||
|
|
||
| func (*QueryResponse) ProtoMessage() {} | ||
|
|
||
| // Deprecated: Use QueryResponse.ProtoReflect.Descriptor instead. | ||
| func (*QueryResponse) Descriptor() ([]byte, []int) { | ||
| return file_cosmos_base_grpc_v2_service_proto_rawDescGZIP(), []int{1} | ||
| } | ||
|
|
||
| func (x *QueryResponse) GetResponse() *anypb.Any { | ||
| if x != nil { | ||
| return x.Response | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid repetitive naming in QueryResponse struct
Similarly, the QueryResponse struct (lines 2270-2303) has a field named Response, leading to QueryResponse.Response. To improve code clarity and adhere to the Uber Go Style Guide, consider renaming this field to prevent stuttering.
| RequestName string `protobuf:"bytes,1,opt,name=request_name,json=requestName,proto3" json:"request_name,omitempty"` | ||
| ResponseName string `protobuf:"bytes,2,opt,name=response_name,json=responseName,proto3" json:"response_name,omitempty"` | ||
| } | ||
|
|
||
| func (x *Handler) Reset() { | ||
| *x = Handler{} | ||
| if protoimpl.UnsafeEnabled { | ||
| mi := &file_cosmos_base_grpc_v2_service_proto_msgTypes[4] | ||
| ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) | ||
| ms.StoreMessageInfo(mi) | ||
| } | ||
| } | ||
|
|
||
| func (x *Handler) String() string { | ||
| return protoimpl.X.MessageStringOf(x) | ||
| } | ||
|
|
||
| func (*Handler) ProtoMessage() {} | ||
|
|
||
| // Deprecated: Use Handler.ProtoReflect.Descriptor instead. | ||
| func (*Handler) Descriptor() ([]byte, []int) { | ||
| return file_cosmos_base_grpc_v2_service_proto_rawDescGZIP(), []int{4} | ||
| } | ||
|
|
||
| func (x *Handler) GetRequestName() string { | ||
| if x != nil { | ||
| return x.RequestName | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| func (x *Handler) GetResponseName() string { | ||
| if x != nil { | ||
| return x.ResponseName | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify field names in the Handler struct
The Handler struct (lines 2374-2410) contains fields RequestName and ResponseName. To align with Go naming conventions and improve readability, consider renaming these fields to Request and Response. This reduces redundancy and makes the code cleaner.
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.grpc.v2.QueryRequest")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.grpc.v2.QueryRequest does not contain field %s", fd.FullName())) | ||
| } |
There was a problem hiding this comment.
Avoid using panic in library code
The use of panic in methods like Has, Set, and Mutable across multiple structs can lead to unexpected crashes. According to the Uber Go Style Guide, it's better to return errors instead of panicking, allowing callers to handle exceptions gracefully.
Also applies to: 175-179, 201-205, 565-569, 611-615, 635-639, 964-980, 992-996, 1059-1065, 1425-1438, 1451-1461, 1509-1515, 1880-1893, 1906-1917, 1930-1940
server/v2/api/grpc/service.go
Outdated
| return nil, status.Errorf(codes.InvalidArgument, "failed to unmarshal request: %v", err) | ||
| } | ||
|
|
||
| queryResp, err := s.app.Query(ctx, 0, protoMsg) |
There was a problem hiding this comment.
this only works for latest height, we should add a way to query historical info as well
There was a problem hiding this comment.
This comment should be done in a follow-up indeed. Let's track this.
| for handlerName := range s.queryHandlers { | ||
| msg := s.queryHandlers[handlerName].MakeMsg() | ||
| resp := s.queryHandlers[handlerName].MakeMsgResp() | ||
|
|
||
| handlerDescriptors = append(handlerDescriptors, &Handler{ | ||
| RequestName: proto.MessageName(msg), | ||
| ResponseName: proto.MessageName(resp), | ||
| }) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* main: (24 commits) build(deps): upgrade to [email protected] (#22436) docs: Update tendermint validators query pagination documentation (#22412) refactor(client/v2): offchain uses client/v2/factory (#22344) feat: wire new handlers to grpc (#22333) fix(x/group): proper address rendering in error (#22425) refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403) docs: update ADR 59 (#22423) build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407) docs: Module account address documentation (#22289) feat(baseapp): add per message telemetry (#22175) docs: Update Twitter Links to X in Documentation (#22408) docs: redirect the remote generation page (#22404) refactor(serverv2): remove unused interface methods, honuor context (#22394) fix(server/v2): return ErrHelp (#22399) feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349) refactor(math): refactor ApproxRoot for readality (#22263) docs: fix KWallet Handbook (#22395) feat(client/v2): broadcast logic (#22282) refactor(server/v2): eager config loading (#22267) test(system): check feePayers signature (#22389) ...
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...
!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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores