fix(server/v2/comebft): wire missing services + fix simulation #21964
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files, primarily focusing on the implementation of gRPC services and protocol buffer message types for querying configuration and status in a Cosmos-based application. New message types Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
github-actions
bot
added
C:server/v2
julienrbrt
added
the
backport/v0.52.x
julienrbrt
changed the title
|
i'll only try to finish this, this week. for all other matters (except reviews) i'm afk. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| protoimpl "google.golang.org/protobuf/runtime/protoimpl" | ||
| timestamppb "google.golang.org/protobuf/types/known/timestamppb" | ||
| io "io" | ||
| reflect "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
| switch fd.FullName() { | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.node.v2.ConfigRequest")) |
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.node.v2.ConfigRequest")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.node.v2.ConfigRequest does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.MinimumGasPrice = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.node.v2.ConfigResponse")) |
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.node.v2.ConfigResponse")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.node.v2.ConfigResponse does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
julienrbrt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (21)
server/v2/api/grpc/nodeservice/service.go (2)
12-18: Add input validation in the constructorConsider validating the
cfgparameter to ensure it contains the required "server" configuration.func NewQueryServer(cfg server.ConfigMap) ServiceServer { + if cfg == nil { + panic("config map cannot be nil") + } + if _, ok := cfg["server"]; !ok { + panic("server configuration is required") + } return queryServer{cfg: cfg} }
32-43: Track the Status implementation as a separate taskThe Status method is currently a stub with commented-out implementation. The comment indicates that environment and execution context are missing, which needs to be addressed.
Would you like me to create a GitHub issue to track the implementation of the Status method? The issue would include:
- Requirements for environment and execution context
- Implementation details from the commented code
- Testing requirements
server/v2/cometbft/client/rpc/utils.go (1)
Line range hint
31-45: Consider optimizing the serialization steps.The current implementation performs multiple serialization steps:
- Block → Proto
- Proto → Bytes
- Bytes → cmttypes.Block
Consider investigating if it's possible to convert directly from the initial proto to
cmttypes.Blockto reduce overhead.proto/cosmos/base/node/v2/query.proto (3)
1-9: LGTM! Consider adding file-level documentation.The file setup follows Cosmos SDK conventions. Consider adding a file-level comment describing the purpose and scope of these node-related queries.
syntax = "proto3"; package cosmos.base.node.v2; +// query.proto defines the gRPC querier service for node-related information. + import "google/api/annotations.proto";🧰 Tools
🪛 buf
4-4: import "google/api/annotations.proto": file does not exist
(COMPILE)
10-20: Consider using a more specific service name.The generic name
Servicecould potentially cause naming conflicts. Consider renaming toNodeServiceorNodeQueryServiceto better reflect its purpose.-service Service { +service NodeService {
22-28: Add field documentation and consider additional configuration fields.The
minimum_gas_pricefield lacks documentation. Also, consider if other configuration fields might be useful for operators.message ConfigResponse { - string minimum_gas_price = 1; + // minimum_gas_price defines the minimum gas price value accepted by this node + string minimum_gas_price = 1; + // pruning_keep_recent defines the number of recent heights to keep on disk (optional) + uint64 pruning_keep_recent = 2; + // pruning_interval defines the height interval after which pruning will run (optional) + uint64 pruning_interval = 3; }server/v2/server_test.go (1)
Line range hint
29-39: Improve test reliability and mock implementationsSeveral improvements could enhance the test's reliability and maintainability:
- The mock implementations currently panic. Consider implementing test-friendly versions:
func (*mockInterfaceRegistry) Resolve(typeUrl string) (gogoproto.Message, error) { return nil, errors.New("not implemented in test") }
- Replace the fixed 5-second timeout with a more reliable approach:
- <-time.After(5 * time.Second) + done := make(chan struct{}) + go func() { + // Wait for server to be ready + done <- struct{}{} + }() + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("server startup timeout") + }
- Improve error handling in the goroutine:
go func() { + defer cancelFn() // wait 5sec and cancel context <-time.After(5 * time.Second) - cancelFn() err = server.Stop(ctx) - require.NoError(t, err) + if err != nil { + t.Errorf("failed to stop server: %v", err) + } }()Also applies to: 82-91
simapp/v2/simdv2/cmd/root_di.go (1)
Line range hint
89-98: Consider enhancing error handling for dependency injectionWhile the current error handling is functional, consider wrapping the errors with more context to aid in debugging. This would help identify whether issues arise from app construction or client construction paths.
Example enhancement:
commandDeps := CommandDependencies[T]{ GlobalConfig: configMap, TxConfig: clientCtx.TxConfig, ModuleManager: moduleManager, SimApp: simApp, ClientContext: clientCtx, + // Consider validating required dependencies here + if moduleManager == nil { + return fmt.Errorf("failed to initialize command dependencies: module manager is nil") + } }server/v2/cometbft/client/rpc/block.go (2)
Line range hint
74-80: Enhance error message clarityWhile the error handling is good, the error message could be more specific about what failed during the conversion.
Consider this improvement:
-return nil, fmt.Errorf("unable to create response block from comet result block: %v", resBlock) +return nil, fmt.Errorf("failed to convert comet result block to response block: block height %d", resBlock.Block.Header.Height)
Line range hint
99-105: Maintain consistent error handling styleGood consistency with GetBlockByHeight. Consider applying the same error message improvement here.
Consider this improvement:
-return nil, fmt.Errorf("unable to create response block from comet result block: %v", resBlock) +return nil, fmt.Errorf("failed to convert comet result block to response block: block hash %X", resBlock.Block.Hash())server/v2/cometbft/query.go (2)
Line range hint
88-92: Enhance error message for invalid heightThe error message could be more specific about the valid height requirements.
Consider updating the error message:
- "cannot query with proof when height <= 1; please provide a valid height", + "proof queries require height > 1",
Line range hint
112-127: Consider batching proof operations marshalingThe current implementation marshals and appends proof operations one at a time. Consider batching the operations to reduce allocations.
Here's a suggested optimization:
if req.Prove { + ops := make([]crypto.ProofOp, len(qRes.ProofOps)) + for i, proof := range qRes.ProofOps { bz, err := proof.Proof.Marshal() if err != nil { return nil, errorsmod.Wrap(err, "failed to marshal proof") } - res.ProofOps = &crypto.ProofOps{ - Ops: []crypto.ProofOp{ - { - Type: proof.Type, - Key: proof.Key, - Data: bz, - }, - }, - } + ops[i] = crypto.ProofOp{ + Type: proof.Type, + Key: proof.Key, + Data: bz, + } } + res.ProofOps = &crypto.ProofOps{Ops: ops} }simapp/v2/simdv2/cmd/commands.go (1)
126-142: Enhance error handling in gRPC server initializationWhile the initialization is well-structured, consider wrapping the error with additional context to aid in debugging.
grpcServer, err := grpcserver.New[T]( logger, simApp.InterfaceRegistry(), simApp.QueryHandlers(), simApp.Query, []func(*grpc.Server) error{ deps.ConsensusServer.Consensus.GRPCServiceRegistrar( deps.ClientContext, deps.GlobalConfig, ), }, deps.GlobalConfig, ) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to initialize gRPC server: %w", err) }simapp/v2/go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe Go version
1.23.1is not valid. The latest stable version of Go is 1.22.1.Apply this diff to fix the Go version:
-go 1.23.1 +go 1.22x/staking/keeper/grpc_query.go (1)
Line range hint
558-571: Consider adding error documentationThe implementation is clean and follows best practices. However, consider documenting the possible error cases in the function comment to help API consumers better handle errors.
+// queryAllRedelegations returns all redelegations for a delegator. +// Returns collections.ErrNotFound if no redelegations exist for the delegator. +// Returns error if the delegator address is invalid or if pagination fails. func queryAllRedelegations(ctx context.Context, k Querier, req *types.QueryRedelegationsRequest) (redels types.Redelegations, res *query.PageResponse, err error) {server/v2/api/grpc/server.go (3)
74-75: Handle potential errors during service registrationEven if
RegisterServiceServerdoes not currently return an error, future implementations or changes might introduce error handling. To make the code more robust, consider checking and handling any potential errors when registering the service.
77-77: Ensure error handling for node service registrationSimilarly, consider capturing and handling any errors that might occur during
nodeservice.RegisterServiceServer. This will enhance the reliability of the service initialization process.
79-79: Capitalize the first word in commentsAccording to the Uber Go Style Guide, comments should begin with a capital letter.
Apply this diff:
- // reflection allows external clients to see what services and methods the gRPC server exposes. + // Reflection allows external clients to see what services and methods the gRPC server exposes.server/v2/cometbft/grpc.go (2)
98-110: Add explanations for unimplemented methodsThe methods
GetBlockWithTxs,GetTx, andGetTxsEventare unimplemented and currently returncodes.Unimplementederrors. Consider adding comments to explain why these methods are unimplemented and specify any plans for future implementation.
137-142: Standardize error handling in theSimulatemethodThe
Simulatemethod uses botherrorsmod.Wrapandstatus.Errorffor error handling. To maintain consistency and clarity, consider standardizing the error handling approach throughout the method.server/v2/cometbft/abci.go (1)
277-277: Extract hardcoded simulation path to a constantTo enhance maintainability and reduce the risk of typos, consider defining the simulation path
"/cosmos.tx.v1beta1.Service/Simulate"as a constant.Apply this diff to extract the simulation path:
+const SimulatePath = "/cosmos.tx.v1beta1.Service/Simulate" ... - if req.Path == "/cosmos.tx.v1beta1.Service/Simulate" { + if req.Path == SimulatePath {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
api/cosmos/base/node/v2/query_grpc.pb.gois excluded by!**/*.pb.goserver/v2/api/grpc/nodeservice/query.pb.gois excluded by!**/*.pb.goserver/v2/api/grpc/nodeservice/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (24)
api/cosmos/base/node/v2/query.pulsar.go(1 hunks)client/grpc/cmtservice/service.go(2 hunks)proto/cosmos/base/node/v2/query.proto(1 hunks)server/v2/api/grpc/nodeservice/service.go(1 hunks)server/v2/api/grpc/server.go(3 hunks)server/v2/cometbft/abci.go(4 hunks)server/v2/cometbft/abci_test.go(1 hunks)server/v2/cometbft/client/grpc/cmtservice/autocli.go(0 hunks)server/v2/cometbft/client/grpc/cmtservice/service.go(0 hunks)server/v2/cometbft/client/rpc/block.go(3 hunks)server/v2/cometbft/client/rpc/utils.go(2 hunks)server/v2/cometbft/go.mod(2 hunks)server/v2/cometbft/grpc.go(1 hunks)server/v2/cometbft/query.go(1 hunks)server/v2/cometbft/server.go(0 hunks)server/v2/go.mod(1 hunks)server/v2/server_test.go(1 hunks)server/v2/stf/branch/mergeiter.go(1 hunks)simapp/v2/go.mod(1 hunks)simapp/v2/simdv2/cmd/commands.go(6 hunks)simapp/v2/simdv2/cmd/root_di.go(1 hunks)tests/integration/accounts/base_account_test.go(1 hunks)tests/integration/accounts/wiring_test.go(1 hunks)x/staking/keeper/grpc_query.go(2 hunks)
💤 Files with no reviewable changes (3)
- server/v2/cometbft/client/grpc/cmtservice/autocli.go
- server/v2/cometbft/client/grpc/cmtservice/service.go
- server/v2/cometbft/server.go
✅ Files skipped from review due to trivial changes (4)
- api/cosmos/base/node/v2/query.pulsar.go
- server/v2/stf/branch/mergeiter.go
- tests/integration/accounts/base_account_test.go
- tests/integration/accounts/wiring_test.go
🧰 Additional context used
📓 Path-based instructions (13)
client/grpc/cmtservice/service.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpc/nodeservice/service.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/abci_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/cometbft/client/rpc/block.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/client/rpc/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/query.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"
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/root_di.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/staking/keeper/grpc_query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
simapp/v2/simdv2/cmd/root_di.go (1)
Learnt from: kocubinski
PR: cosmos/cosmos-sdk#22267
File: simapp/v2/simdv2/cmd/root_di.go:26-26
Timestamp: 2024-11-10T03:53:32.474Z
Learning: In `simapp/v2/simdv2/cmd/root_di.go`, the initial call to `InitRootCmd` with empty `CommandDependencies` is required to initialize configuration.
🪛 buf
proto/cosmos/base/node/v2/query.proto
4-4: import "google/api/annotations.proto": file does not exist
(COMPILE)
🔇 Additional comments (22)
server/v2/api/grpc/nodeservice/service.go (3)
1-8: LGTM: Clean package structure and imports
The package name and imports follow Go conventions and are well-organized.
10-10: LGTM: Proper interface implementation check
Good practice using compile-time interface compliance check.
20-30: Previous review comment about safe type assertions is still applicable
server/v2/cometbft/client/rpc/utils.go (2)
Line range hint 18-24: LGTM! Error handling is thorough and follows best practices.
The error messages are descriptive and include the necessary context for debugging.
30-31: LGTM! Function name follows Go naming conventions.
The renaming from NewResponseResultBlock to responseResultBlock correctly reflects that this is an unexported utility function.
proto/cosmos/base/node/v2/query.proto (1)
30-39: LGTM! Well-documented status fields.
The status response fields are well-chosen and properly documented. The use of gogoproto for timestamp handling is appropriate.
server/v2/server_test.go (1)
56-56: Verify the purpose of the new nil parameter
The test should include cases that exercise the new parameter (extraOptions) with non-nil values to ensure proper functionality.
Consider adding test cases that:
- Pass valid options to verify proper initialization
- Pass invalid options to verify error handling
- Document the purpose of this parameter in the test
// Example test case structure
testCases := []struct {
name string
extraOptions interface{}
wantErr bool
}{
{"nil options", nil, false},
{"valid options", validOptions, false},
{"invalid options", invalidOptions, true},
}✅ Verification successful
The nil parameter is for extraGRPCHandlers which allows registering additional handlers
The test case passes nil for extraGRPCHandlers parameter which is valid since it's an optional slice of handler registration functions. The nil value simply means no additional gRPC handlers will be registered beyond the default ones.
While adding test cases for non-nil values would improve coverage, passing nil is a valid use case that should be tested as it represents the common scenario where no extra handlers are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition and usage of grpc.New to understand the new parameter
ast-grep --pattern 'func New[transaction.Tx]($$$) ($$$)'
Length of output: 58
Script:
#!/bin/bash
# Let's try a broader search for the grpc.New function definition
ast-grep --pattern 'func New[$_]($$$) ($$$)'
# Also search for any usage of grpc.New
rg "grpc\.New" -A 3
Length of output: 75750
Script:
#!/bin/bash
# Let's look for the grpc.New function definition in the API/GRPC package specifically
rg -A 10 "func New\[T transaction.Tx\]" server/v2/api/grpc/
# Also search for any changes related to extraGRPCHandlers
rg "extraGRPCHandlers" -A 3
Length of output: 1267
simapp/v2/simdv2/cmd/root_di.go (1)
97-97: LGTM: ClientContext addition is properly placed
The addition of ClientContext to CommandDependencies is well-positioned after proper initialization and follows the dependency injection pattern consistently.
server/v2/cometbft/client/rpc/block.go (1)
53-61: LGTM: Well-structured response handling
The direct initialization of SearchBlocksResult with pagination details is clear and maintainable. The totalPages calculation ensures accurate pagination information.
server/v2/cometbft/query.go (2)
86-86: LGTM! Function signature simplified
The removal of the types.Store parameter simplifies the interface while maintaining functionality through the consensus instance's store field.
Line range hint 94-98: Investigate store name encoding optimization
The TODO comment suggests a potential performance optimization for store name encoding. This should be investigated to determine if there's a meaningful performance impact.
Let's check for similar patterns in the codebase:
server/v2/go.mod (1)
38-38: LGTM! Verify dependency compatibility.
The addition of google.golang.org/genproto/googleapis/api is properly structured and aligns with the PR's objective of wiring missing services.
Let's verify the dependency compatibility:
✅ Verification successful
Dependency version is well-aligned across the ecosystem
The verification shows that:
- The specific version
v0.0.0-20240903143218-8af14fe29dc1is consistently used across multiple Cosmos SDK modules and tools - No open bug issues are found for this version in the upstream repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential compatibility issues with the new dependency
# Check if this version is used by other cosmos-sdk modules
rg -l "google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1"
# Check if there are any known issues with this version
gh api graphql -f query='
{
repository(owner: "googleapis", name: "go-genproto") {
issues(first: 5, states: OPEN, labels: ["Type: Bug"]) {
nodes {
title
url
createdAt
}
}
}
}'
Length of output: 1668
simapp/v2/simdv2/cmd/commands.go (3)
8-8: LGTM: Import changes are appropriate
The new imports support the enhanced gRPC functionality, and the alias helps avoid naming conflicts.
Also applies to: 16-16
44-47: LGTM: Well-documented structural changes
The new fields enhance the structure's capabilities while maintaining type safety. The comments effectively explain the design decision for using *cometbft.CometBFTServer[T] over a more generic type.
Line range hint 150-154: LGTM: Server command wiring is consistent
The updated AddCommands call correctly integrates the new ConsensusServer while maintaining the established parameter pattern.
client/grpc/cmtservice/service.go (2)
41-41: LGTM! Parameter name change improves clarity.
The rename from clientCtx to cometRPC better reflects the parameter's purpose and maintains consistency with the struct field name rpc.
Also applies to: 46-46
287-287: Verify the removal of interface registry usage.
The interface registry parameter has been marked as unused. While this follows Go conventions, please verify that this doesn't impact any potential type registration or interface unpacking needs, especially considering the presence of UnpackInterfaces method in this file.
✅ Verification successful
Let me generate additional verification scripts to check for UnpackInterfaces usage and interface registry dependencies.
Interface registry parameter can be safely removed
The interface registry parameter is only used as a dependency injection marker and is not actively used in the service implementation. The UnpackInterfaces functionality is self-contained within the GetLatestValidatorSetResponse type and uses the provided AnyUnpacker interface directly, without depending on the service's interface registry parameter. The only other usage is for creating Any types with codectypes.NewAnyWithValue() which also doesn't require the registry parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for interface registry usage patterns in the codebase
# to ensure we're not breaking any existing functionality.
# Search for interface registry usage patterns
echo "Searching for interface registry usage patterns..."
rg -A 3 "RegisterInterface|RegisterImplementations"
# Search for similar service registrations
echo "Searching for similar service registration patterns..."
ast-grep --pattern 'func Register$_Service($_, $_, $registry codectypes.InterfaceRegistry, $_)'
Length of output: 80539
Script:
#!/bin/bash
# Check for UnpackInterfaces usage and any interface registry dependencies in the service file
# Search for UnpackInterfaces method and its implementation
echo "Searching for UnpackInterfaces usage..."
rg -A 5 "UnpackInterfaces" client/grpc/cmtservice/service.go
# Search for any interface registry usage in the service file
echo "Searching for interface registry usage..."
rg "InterfaceRegistry|codectypes" client/grpc/cmtservice/service.go
Length of output: 871
server/v2/cometbft/abci_test.go (1)
702-702:
The past review comment about error handling is still valid.
The NewConsensus initialization error is still not being handled, which could mask setup issues in tests.
As mentioned in the previous review, apply this diff to handle potential errors:
- return NewConsensus[mock.Tx](log.NewNopLogger(), "testing-app", am,
+ consensus, err := NewConsensus[mock.Tx](log.NewNopLogger(), "testing-app", am,
mempool, map[string]struct{}{}, nil, mockStore,
Config{AppTomlConfig: DefaultAppTomlConfig()}, mock.TxCodec{}, "test")
+ require.NoError(t, err)
+ return consensusx/staking/keeper/grpc_query.go (1)
437-437: LGTM! Clean refactor of queryAllRedelegations call
The simplified function call aligns with the removal of direct store access in favor of using the collections API.
server/v2/cometbft/grpc.go (3)
141-142: Duplicate Comment: Avoid potential nil pointer dereference when returning error
The past review comment regarding a potential nil pointer dereference at lines 141-142 remains valid and applicable to the current code segment.
195-198: Duplicate Comment: Prevent runtime panic by safely handling type assertions
The previous concern about unsafe type assertions at lines 195-198 is still valid and should be addressed to prevent potential runtime panics.
214-218: Duplicate Comment: Populate 'Timestamp' and 'AppHash' in 'StatusResponse'
The past suggestion to populate the Timestamp and AppHash fields in StatusResponse at lines 214-218 remains applicable. Providing these values would enhance the completeness of the status information.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/abci.go (2)
275-317: Consider refactoring simulation handling for better maintainabilityThe simulation handling logic is complex and could benefit from being broken down into smaller, more focused functions.
Consider extracting these functions:
handleSimulateRequest- For decoding and validating the requestprocessSimulation- For running the simulationpackMessageResponses- For handling message responsesExample refactor:
func (c *Consensus[T]) handleSimulateRequest(data []byte) (*txtypes.SimulateRequest, error) { simulateRequest := &txtypes.SimulateRequest{} if err := gogoproto.Unmarshal(data, simulateRequest); err != nil { return nil, fmt.Errorf("unable to decode simulate request: %w", err) } return simulateRequest, nil } func (c *Consensus[T]) processSimulation(ctx context.Context, txBytes []byte) (*server.TxResult, error) { tx, err := c.txCodec.Decode(txBytes) if err != nil { return nil, fmt.Errorf("failed to decode tx: %w", err) } txResult, _, err := c.app.Simulate(ctx, tx) if err != nil { if txResult != nil { return nil, fmt.Errorf("%v with gas used: '%d'", err, txResult.GasUsed) } return nil, fmt.Errorf("simulation failed: %v", err) } return txResult, nil } func (c *Consensus[T]) packMessageResponses(messages []gogoproto.Message) ([]*codectypes.Any, error) { msgResponses := make([]*codectypes.Any, 0, len(messages)) for _, msg := range messages { anyMsg, err := codectypes.NewAnyWithValue(msg) if err != nil { return nil, fmt.Errorf("failed to pack message response: %w", err) } msgResponses = append(msgResponses, anyMsg) } return msgResponses, nil }
294-303: Optimize message response handlingThe message response handling could be optimized by pre-allocating the slice and using a single error check.
Apply this diff to optimize the code:
-msgResponses := make([]*codectypes.Any, 0, len(txResult.Resp)) -// pack the messages into Any -for _, msg := range txResult.Resp { - anyMsg, err := codectypes.NewAnyWithValue(msg) - if err != nil { - return nil, true, fmt.Errorf("failed to pack message response: %w", err) - } - - msgResponses = append(msgResponses, anyMsg) -} +msgResponses := make([]*codectypes.Any, len(txResult.Resp)) +var packErr error +for i, msg := range txResult.Resp { + if msgResponses[i], packErr = codectypes.NewAnyWithValue(msg); packErr != nil { + return nil, true, fmt.Errorf("failed to pack message response at index %d: %w", i, packErr) + } +}
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
server/v2/cometbft/abci.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
server/v2/cometbft/abci.go (2)
42-46: LGTM: Well-defined query path constants
The constants are well-named and improve code maintainability by centralizing query path definitions.
229-239: LGTM: Clear and informative query path handling
The switch statement is well-structured and the error message now includes the specific query path, improving debuggability.
| return queryServer{cfg: cfg} | ||
| } | ||
|
|
||
| func (s queryServer) Config(ctx context.Context, _ *ConfigRequest) (*ConfigResponse, error) { |
There was a problem hiding this comment.
1 Q why we need v2 service since v1 was registed by comet. Like what is different?
There was a problem hiding this comment.
The comet one is here for backward compatibility. But we cannot get all data there.
There was a problem hiding this comment.
The comet one is here for backward compatibility. But we cannot get all data there.
This service has less fields as the other aren't relevant as they are store server or consensus server dependent.
| // note, environment nor execution context isn't available in the context | ||
|
|
||
| // return &StatusResponse{ | ||
| // Height: uint64(headerInfo.Height), |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
proto/cosmos/base/node/v2/query.proto (2)
8-14: Consider a more specific service nameWhile the implementation is correct, the service name 'Service' is quite generic. Consider renaming it to something more specific like 'NodeService' or 'NodeQueryService' to better reflect its purpose.
Additionally, the RPC method documentation could be enhanced to describe the returned configuration details.
-// Service defines the gRPC querier service for node related queries. -service Service { +// NodeService defines the gRPC querier service for node related queries. +service NodeService { // Config queries for the operator configuration. + // Returns the node's configuration including minimum gas price settings. rpc Config(ConfigRequest) returns (ConfigResponse) {
16-22: Document minimum_gas_price field formatPlease enhance the documentation to specify:
- The expected format of minimum_gas_price (e.g., "0.1uatom")
- Any validation rules or constraints
- Whether it supports multiple denominations
// ConfigResponse defines the response structure for the Config gRPC query. message ConfigResponse { + // minimum_gas_price specifies the minimum gas price value for transactions + // Format: {amount}{denomination} (e.g., "0.1uatom") string minimum_gas_price = 1; }simapp/v2/simdv2/cmd/commands.go (2)
43-46: Document the purpose of ClientContext fieldWhile the ConsensusServer field is well documented, please add a comment explaining why ClientContext was added and its role in the dependency structure.
125-141: Consider more specific error handlingWhile the error handling is present, consider wrapping the error with more context about which part of the gRPC server creation failed (e.g., base initialization vs extra handlers registration).
grpcServer, err := grpcserver.New[T]( logger, simApp.InterfaceRegistry(), simApp.QueryHandlers(), simApp.Query, deps.GlobalConfig, grpcserver.WithExtraGRPCHandlers[T]( deps.ConsensusServer.Consensus.GRPCServiceRegistrar( deps.ClientContext, deps.GlobalConfig, ), ), ) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create gRPC server: %w", err) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
api/cosmos/base/node/v2/query_grpc.pb.gois excluded by!**/*.pb.goserver/v2/api/grpc/nodeservice/query.pb.gois excluded by!**/*.pb.goserver/v2/api/grpc/nodeservice/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (5)
api/cosmos/base/node/v2/query.pulsar.go(1 hunks)proto/cosmos/base/node/v2/query.proto(1 hunks)server/v2/api/grpc/nodeservice/service.go(1 hunks)server/v2/api/grpc/server.go(5 hunks)simapp/v2/simdv2/cmd/commands.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/cosmos/base/node/v2/query.pulsar.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/api/grpc/nodeservice/service.go
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/api/grpc/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf
proto/cosmos/base/node/v2/query.proto
4-4: import "google/api/annotations.proto": file does not exist
(COMPILE)
🔇 Additional comments (9)
proto/cosmos/base/node/v2/query.proto (1)
1-7: LGTM: File structure follows best practices
The file structure follows protobuf best practices with proper syntax declaration, package naming convention, and go package specification.
🧰 Tools
🪛 buf
4-4: import "google/api/annotations.proto": file does not exist
(COMPILE)
simapp/v2/simdv2/cmd/commands.go (4)
15-15: LGTM: Clear import alias
The import alias grpcserver clearly distinguishes the gRPC server package and avoids potential naming conflicts.
81-85: LGTM: Clear server component initialization
The initialization of server components using struct literals improves code clarity and maintainability.
Line range hint 103-119: LGTM: Proper error handling and dependency injection
The ConsensusServer creation properly handles errors and follows good dependency injection patterns.
149-149: LGTM: Consistent server component usage
The server commands wiring consistently uses the new ConsensusServer component.
server/v2/api/grpc/server.go (4)
29-29: LGTM: Import added for nodeservice integration
The import is correctly added and follows Go import conventions.
43-44: LGTM: Server struct fields properly defined
The new fields grpcSrv and extraGRPCHandlers are well-typed and appropriately encapsulate the server's extended functionality.
78-79: LGTM: Node service properly registered
The node service is correctly registered with the gRPC server using the configuration from the server context.
100-114: LGTM: Well-implemented functional options pattern
The implementation follows Go best practices:
- Proper use of generics with transaction.Tx constraint
- Clear documentation for each option function
- Clean implementation of the functional options pattern
(cherry picked from commit d697a3d) # Conflicts: # server/v2/api/grpc/server.go # server/v2/cometbft/go.mod # server/v2/go.mod # server/v2/stf/branch/mergeiter.go
…ort #21964) (#22555) Co-authored-by: Julien Robert <[email protected]>
* main: build(deps): Bump cosmossdk.io/math from 1.3.0 to 1.4.0 (#22580) fix(server/v2/api/telemetry): enable global metrics (#22571) refactor(server/v2/cometbft): add `codec.Codec` and clean-up APIs (#22566) feat(core/coretesting): make memDB satisfy db.Db interface (#22570) Merge commit from fork fix(server(/v2)): fix fallback genesis path (#22564) fix: only overwrite context chainID when necessary (#22568) docs(client): Update setFeeGranter and setFeePayer comments (#22526) fix(baseapp): populate header info in `NewUncachedContext` (#22557) build(deps): Bump buf.build/gen/go/cometbft/cometbft/protocolbuffers/go from 1.35.1-20240701160653-fedbb9acfd2f.1 to 1.35.2-20240701160653-fedbb9acfd2f.1 in /api (#22551) build(deps): Bump github.com/creachadair/atomicfile from 0.3.5 to 0.3.6 in /tools/confix (#22552) docs: Update reference of Approximation (#22550) fix(server/v2/comebft): wire missing services + fix simulation (#21964) ci: fix permissions for GITHUB_TOKEN on dependabot workflows (#22547) ci: fix permissions for GITHUB_TOKEN in spell check workflow (#22545) build(deps): Bump google.golang.org/protobuf from 1.35.1 to 1.35.2 (#22537) fix(cosmovisor): premature upgrade on restart (#22528) fix(store/v2/pebble): handle version 0 in keys (#22524) refactor(server/v2/telemetry): swap redirects (#22520) docs: Update content in CODE_OF_CONDUCT.md (#22518)
Description
Closes: #21958
IMPORTANT and discovered while working on this PR:
In v1, external grpc services were available to modules (comet services, node service, tx service)
In v2 they aren't, which is why we have a special handling for the tx service for simulation.
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
Configmethod to retrieve minimum gas prices.Improvements
Bug Fixes
Chores
go.modfiles to reflect new paths and versions.