refactor(server/v2/cometbft): Handle non-module service queries #22803
Conversation
hieuvubk
requested review from
kocubinski,
julienrbrt,
a team,
facundomedica and
sontrinh16
as code owners
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily enhance the functionality of the Changes
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/abci.go (1)
38-40: Organize imports usinggcito conform to style guidelinesThe imports are not correctly organized according to the project's Golang style guidelines. Please run
gcito sort and group the imports properly.🧰 Tools
🪛 golangci-lint (1.62.2)
38-38: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
server/v2/cometbft/server.go (1)
44-44: Organize imports usinggcito conform to style guidelinesThe imports are not correctly organized according to the project's Golang style guidelines. Please run
gcito sort and group the imports properly.🧰 Tools
🪛 golangci-lint (1.62.2)
44-44: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/abci.go(5 hunks)server/v2/cometbft/server.go(2 hunks)simapp/v2/simdv2/cmd/commands.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/abci.go
38-38: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
server/v2/cometbft/server.go
44-44: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (4)
server/v2/cometbft/abci.go (2)
92-94: Addition of consensusAddressCodec field is appropriate
The new field consensusAddressCodec addresscodec.Codec added to the consensus struct is correctly implemented.
353-376: Generic handleCometService function is correctly implemented
The implementation of the generic handleCometService function appears correct and appropriately uses Go's type parameters.
simapp/v2/simdv2/cmd/commands.go (1)
114-114: Passing ConsensusAddressCodec is appropriate
The addition of deps.ClientContext.ConsensusAddressCodec in the cometbft.New call ensures the consensus server is initialized with the correct address codec.
server/v2/cometbft/server.go (1)
70-70: Addition of consensusAddressCodec parameter
The New function signature update to include consensusAddressCodec addresscodec.Codec is appropriate and ensures proper initialization of the consensus server.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
server/v2/cometbft/abci.go (2)
38-39: Format import statements according to Go conventionsThe import statements are not properly grouped and sorted. Please organize the imports into standard library packages, third-party packages, and internal packages, each separated by a blank line. This improves code readability and maintains consistency.
🧰 Tools
🪛 golangci-lint (1.62.2)
38-38: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
193-193: Fix typo in variable name 'lastestVersion'The variable
lastestVersionappears to be misspelled. It should belatestVersionfor clarity and correctness.Apply this diff to correct the typo:
- lastestVersion, err := c.store.GetLatestVersion() + latestVersion, err := c.store.GetLatestVersion()server/v2/cometbft/server.go (1)
44-44: Format import statements according to Go conventionsThe import statements are not properly grouped and sorted. Please organize the imports into standard library packages, third-party packages, and internal packages, each separated by a blank line. This enhances code readability and maintains consistency across the codebase.
🧰 Tools
🪛 golangci-lint (1.62.2)
44-44: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/abci.go(5 hunks)server/v2/cometbft/server.go(2 hunks)simapp/v2/simdv2/cmd/commands.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/server.go
44-44: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
server/v2/cometbft/abci.go
38-38: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (3)
server/v2/cometbft/abci.go (1)
254-285: 🛠️ Refactor suggestion
Generalize handling of CometBFT service methods
Currently, the code uses a switch statement with hardcoded method names to handle CometBFT service requests. To improve maintainability and scalability, consider dynamically routing requests to the appropriate handlers for all methods of the CometBFT service. This reduces code duplication and simplifies the addition of new methods in the future.
simapp/v2/simdv2/cmd/commands.go (1)
114-114: Reorder function arguments to group codec parameters together
Consider moving deps.ClientContext.ConsensusAddressCodec next to other codec arguments in the cometbft.New function call. Grouping codec parameters together enhances readability and aligns with consistent argument ordering.
Apply this diff to rearrange the arguments:
- deps.ClientContext.ConsensusAddressCodec,
logger,
simApp.Name(),
simApp.Store(),
simApp.App.AppManager,
simApp.AppCodec(),
&client.DefaultTxDecoder[T]{TxConfig: deps.TxConfig},
+ deps.ClientContext.ConsensusAddressCodec,
simApp.App.QueryHandlers(),
simApp.App.SchemaDecoderResolver(),server/v2/cometbft/server.go (1)
70-70: Reorder function parameters to group codec arguments together
As previously suggested, consider moving consensusAddressCodec next to other codec parameters in the New function signature. Grouping related parameters improves readability and aligns with common Go practices.
Apply this diff to rearrange the parameters:
func New[T transaction.Tx](
- consensusAddressCodec addresscodec.Codec,
logger log.Logger,
appName string,
store types.Store,
app appmanager.AppManager[T],
appCodec codec.Codec,
txCodec transaction.Codec[T],
+ consensusAddressCodec addresscodec.Codec,
queryHandlers map[string]appmodulev2.Handler,
decoderResolver decoding.DecoderResolver,
serverOptions ServerOptions[T],
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci.go(5 hunks)server/v2/cometbft/server.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/abci.go
37-37: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
38-38: ST1019(related information): other import of "cosmossdk.io/core/server"
(stylecheck)
server/v2/cometbft/server.go
41-41: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (7)
server/v2/cometbft/server.go (3)
41-41: LGTM!
The import is correctly placed and is used by the new consensusAddressCodec field.
🧰 Tools
🪛 golangci-lint (1.62.2)
41-41: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
70-70: Move the parameter next to other codec arguments.
For better code organization and readability, consider moving the consensusAddressCodec parameter next to the other codec-related parameters (appCodec, txCodec).
Line range hint 96-96: LGTM!
The field is correctly placed and is used in the node service implementation.
🧰 Tools
🪛 golangci-lint (1.62.2)
41-41: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
server/v2/cometbft/abci.go (4)
93-96: LGTM!
The new fields are correctly placed and are used in the Query method implementation.
193-201: Fix typo in variable name.
The variable name lastestVersion contains a typo; it should be latestVersion.
256-309: Add error handling and bounds checking.
- The error from
rpchttp.Newis ignored. Consider handling it to prevent potential runtime issues. - Add bounds check before accessing
paths[2]to prevent potential panic.
376-398: LGTM!
The function is well-structured with proper generics constraints and comprehensive error handling.
hieuvubk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
server/v2/cometbft/grpc.go (2)
20-28: Organize imports using 'gci' for consistencyThe imports in this file are not organized according to the Go import conventions. Consider running
gciwith the provided parameters to group and sort the imports for better readability and maintenance.Apply this diff to organize the imports:
import ( + "context" + "fmt" + "strings" abci "github.com/cometbft/cometbft/abci/types" abciproto "github.com/cometbft/cometbft/api/cometbft/abci/v1" "github.com/cosmos/gogoproto/proto" + "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" autocliv1 "cosmossdk.io/api/cosmos/autocli/v1" cmtv1beta1 "cosmossdk.io/api/cosmos/base/tendermint/v1beta1" "cosmossdk.io/core/server" corestore "cosmossdk.io/core/store" "cosmossdk.io/core/transaction" + "cosmossdk.io/log" + storeserver "cosmossdk.io/server/v2/store" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/client/grpc/cmtservice" nodeservice "github.com/cosmos/cosmos-sdk/client/grpc/node" codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/query" txtypes "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" )
117-117: Avoid usingfmt.Printlnin production codeUsing
fmt.Printlnfor logging is not recommended in production code as it writes directly to the standard output. Instead, use the provided logger to record debug information.Apply this diff to replace
fmt.Printlnwith proper logging:func (t txServer[T]) GetBlockWithTxs(ctx context.Context, req *txtypes.GetBlockWithTxsRequest) (*txtypes.GetBlockWithTxsResponse, error) { // ... txb, err := t.clientCtx.TxConfig.TxDecoder()(tx) - fmt.Println("TxDecoder", txb, err) + logger.Debug("Tx decoding result", "tx", txb, "error", err) if err != nil { return err } // ... }server/v2/cometbft/server.go (1)
41-41: Organize imports using 'gci' for consistencyThe imports in this file are not organized according to the Go import conventions. Please run
gciwith the provided parameters to properly group and sort the imports.🧰 Tools
🪛 golangci-lint (1.62.2)
41-41: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
server/v2/cometbft/abci.go (1)
37-41: Organize imports using 'gci' for consistencyThe imports are not organized according to the Go import conventions. Consider running
gciwith the provided parameters to group and sort the imports for better readability.🧰 Tools
🪛 golangci-lint (1.62.2)
37-37: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
38-38: ST1019(related information): other import of "cosmossdk.io/core/server"
(stylecheck)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
server/v2/cometbft/abci.go(5 hunks)server/v2/cometbft/grpc.go(7 hunks)server/v2/cometbft/server.go(3 hunks)simapp/v2/simdv2/cmd/commands.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simapp/v2/simdv2/cmd/commands.go
🧰 Additional context used
📓 Path-based instructions (3)
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/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/abci.go
37-37: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
38-38: ST1019(related information): other import of "cosmossdk.io/core/server"
(stylecheck)
server/v2/cometbft/grpc.go
77-77: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
server/v2/cometbft/server.go
41-41: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (5)
server/v2/cometbft/server.go (1)
76-76: Nitpick: Move consensusAddressCodec parameter next to other codec arguments
For better readability and consistency, consider moving the consensusAddressCodec parameter down next to the other codec arguments in the New function signature.
server/v2/cometbft/abci.go (4)
37-41: Remove duplicate import of cosmossdk.io/core/server
The package cosmossdk.io/core/server is imported twice—once without an alias and once as coreserver. This duplicate import can lead to confusion and should be consolidated.
Apply this diff to remove the duplicate import:
import (
// ...
"cosmossdk.io/core/server"
- coreserver "cosmossdk.io/core/server"
// ...
)🧰 Tools
🪛 golangci-lint (1.62.2)
37-37: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
38-38: ST1019(related information): other import of "cosmossdk.io/core/server"
(stylecheck)
195-199: Fix typo in variable name lastestVersion
There is a typo in the variable name lastestVersion; it should be latestVersion.
Apply this diff to correct the typo:
func (c *consensus[T]) Query(ctx context.Context, req *abciproto.QueryRequest) (resp *abciproto.QueryResponse, err error) {
// ...
// when a client did not provide a query height, manually inject the latest
if req.Height == 0 {
- lastestVersion, err := c.store.GetLatestVersion()
+ latestVersion, err := c.store.GetLatestVersion()
if err != nil {
return nil, err
}
- req.Height = int64(lastestVersion)
+ req.Height = int64(latestVersion)
}
// ...
}261-261:
Handle error returned by rpchttp.New
Currently, the error returned by rpchttp.New is ignored. To ensure robustness, the error should be checked and properly handled to prevent potential runtime issues.
Apply this diff to handle the error:
func (c *consensus[T]) maybeRunGRPCQuery(ctx context.Context, req *abci.QueryRequest) (resp *abciproto.QueryResponse, isGRPC bool, err error) {
// ...
// Handle CometBFT service
if strings.Contains(req.Path, "/cosmos.base.tendermint.v1beta1.Service") {
- rpcClient, _ := rpchttp.New(c.cfg.AppTomlConfig.Address)
+ rpcClient, err := rpchttp.New(c.cfg.AppTomlConfig.Address)
+ if err != nil {
+ return nil, true, err
+ }
cometQServer := cmtservice.NewQueryServer(rpcClient, c.Query, c.consensusAddressCodec)
// ...
}
// ...
}263-267:
Ensure paths slice has sufficient length before accessing paths[2]
Accessing paths[2] without verifying the length of the slice can lead to an index out-of-range panic if the slice has fewer than three elements. Please add a check to prevent potential runtime errors.
Apply this diff to add the length check:
paths := strings.Split(req.Path, "/")
+ if len(paths) <= 2 {
+ return nil, true, fmt.Errorf("invalid request path: %s", req.Path)
+ }
var resp transaction.Msg
var err error
switch paths[2] {| for i, count := offset, uint64(0); i > 0 && count != limit; i, count = i-1, count+1 { | ||
| if err = decodeTxAt(i); err != nil { | ||
| logger.Error("failed to decode tx", "error", err) | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Fix off-by-one error and potential underflow in reverse pagination loop
The reverse pagination loop may skip the first transaction (index 0) due to the condition i > 0 and can cause an underflow when i is 0 because i is of type uint64. This could lead to unexpected behavior or runtime errors.
Apply this diff to correct the loop:
if req.Pagination != nil && req.Pagination.Reverse {
- for i, count := offset, uint64(0); i > 0 && count != limit; i, count = i-1, count+1 {
+ for i, count := int64(offset), uint64(0); i >= 0 && count != limit; i, count = i-1, count+1 {
if err = decodeTxAt(uint64(i)); err != nil {
logger.Error("failed to decode tx", "error", err)
}
}
} else {
// ...
}Ensure that i is converted to int64 to handle the decrement properly and prevent underflow. Also, adjust the call to decodeTxAt to accept a uint64 parameter.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
server/v2/cometbft/grpc.go (4)
63-63: Add documentation for the consensus fieldConsider adding a comment explaining the purpose of the
consensusfield and its role in handling non-module service queries.
117-117: Remove debug print statementThe
fmt.Printlnstatement should be removed as it's likely a debugging artifact.- fmt.Println("TxDecoder", txb, err)
277-277: Address TODO comment regarding Tx interfaceThe TODO comment suggests a potential improvement to the Tx interface. Consider creating a follow-up task to address this.
Would you like me to create a GitHub issue to track this TODO item?
460-467: Add documentation for type constraintsConsider adding documentation explaining the purpose and requirements of the type constraints
PTandUT. This will help future maintainers understand the design decisions.server/v2/cometbft/abci.go (3)
92-95: Add documentation for new fieldsConsider adding documentation for:
queryHandlersMapgetProtoRegistryconsensusAddressCodeccfgMapThis will help explain their roles in query handling.
192-200: Add comment explaining height injection behaviorConsider adding a comment explaining why height injection is needed for non-module queries and why module queries are handled differently by AppManager.
264-266: Refactor duplicate path validationThe path length validation is duplicated across multiple service handlers. Consider extracting it into a helper function.
+func validatePath(path string) error { + paths := strings.Split(path, "/") + if len(paths) <= 2 { + return fmt.Errorf("invalid request path: %s", path) + } + return nil +}Also applies to: 299-301, 347-349
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/abci.go(5 hunks)server/v2/cometbft/grpc.go(7 hunks)server/v2/cometbft/server.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/server.go
🧰 Additional context used
📓 Path-based instructions (2)
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/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (6)
server/v2/cometbft/grpc.go (4)
20-20: LGTM: Import additions are appropriate
The new imports for logging and store functionality are well-organized and necessary for the enhanced query handling.
Also applies to: 28-29
183-209: LGTM: Well-structured implementation
The GetTxsEvent implementation has proper error handling and effectively uses the parseOrderBy helper function.
449-458: LGTM: Clean and complete implementation
The parseOrderBy function effectively handles all cases and provides a sensible default.
129-134:
Fix off-by-one error in reverse pagination
The reverse pagination loop has potential issues:
- It may skip the first transaction (index 0) due to the condition
i > 0 - Using
uint64forican cause underflow when decrementing
- for i, count := offset, uint64(0); i > 0 && count != limit; i, count = i-1, count+1 {
+ for i, count := int64(offset), uint64(0); i >= 0 && count != limit; i, count = i-1, count+1 {Likely invalid or redundant comment.
server/v2/cometbft/abci.go (2)
331-331:
Handle error from NewClientFromNode
The error from client.NewClientFromNode should not be ignored as it could lead to runtime issues.
- rpcClient, _ := client.NewClientFromNode(c.cfg.AppTomlConfig.Address)
+ rpcClient, err := client.NewClientFromNode(c.cfg.AppTomlConfig.Address)
+ if err != nil {
+ return nil, false, fmt.Errorf("failed to create client: %w", err)
+ }Likely invalid or redundant comment.
261-261:
Handle error from rpchttp.New
The error from rpchttp.New should not be ignored as it could lead to runtime issues.
- rpcClient, _ := rpchttp.New(c.cfg.AppTomlConfig.Address)
+ rpcClient, err := rpchttp.New(c.cfg.AppTomlConfig.Address)
+ if err != nil {
+ return nil, false, fmt.Errorf("failed to create RPC client: %w", err)
+ }Likely invalid or redundant comment.
| @@ -72,6 +73,7 @@ func New[T transaction.Tx]( | |||
| app appmanager.AppManager[T], | |||
| appCodec codec.Codec, | |||
| txCodec transaction.Codec[T], | |||
| consensusAddressCodec addresscodec.Codec, | |||
There was a problem hiding this comment.
It should be set in the consensus struct in this function.
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
server/v2/cometbft/grpc.go (1)
271-287: Consider addressing the TODO commentThe TODO comment suggests a potential improvement to the Tx interface. This should be tracked and addressed.
Would you like me to create a GitHub issue to track this improvement?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci.go(4 hunks)server/v2/cometbft/grpc.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/grpc.go
24-24: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (6)
server/v2/cometbft/grpc.go (4)
5-5: LGTM: Import and struct changes
The new imports and the addition of the consensus field to txServer struct are well-structured and necessary for handling external service queries.
Also applies to: 21-21, 67-67
187-213: LGTM: Well-structured GetTxsEvent implementation
The implementation includes proper error handling, type checking, and uses a helper function for order parsing.
576-598: LGTM: Well-implemented generic external service handler
The implementation provides good type safety and proper error handling.
76-153:
Fix off-by-one error and potential underflow in reverse pagination loop
The reverse pagination loop may skip the first transaction (index 0) due to the condition i > 0 and can cause an underflow when i is 0 because i is of type uint64.
Apply this diff to correct the loop:
if req.Pagination != nil && req.Pagination.Reverse {
- for i, count := offset, uint64(0); i > 0 && count != limit; i, count = i-1, count+1 {
+ for i, count := int64(offset), uint64(0); i >= 0 && count != limit; i, count = i-1, count+1 {
if err = decodeTxAt(uint64(i)); err != nil {
logger.Error("failed to decode tx", "error", err)
}
}server/v2/cometbft/abci.go (2)
18-18: LGTM: Consensus struct enhancements
The new fields and imports are well-structured and necessary for handling address encoding and configuration.
Also applies to: 87-90
187-195: LGTM: Automatic height injection for queries
The implementation automatically sets the query height to the latest version when not specified, improving the user experience.
server/v2/cometbft/grpc.go
Outdated
| var err error | ||
| switch paths[2] { | ||
| case "GetNodeInfo": | ||
| resp, err = handleExternalService(ctx, req, cometQServer.GetNodeInfo) | ||
| case "GetSyncing": | ||
| resp, err = handleExternalService(ctx, req, cometQServer.GetSyncing) | ||
| case "GetLatestBlock": | ||
| resp, err = handleExternalService(ctx, req, cometQServer.GetLatestBlock) | ||
| case "GetBlockByHeight": | ||
| resp, err = handleExternalService(ctx, req, cometQServer.GetBlockByHeight) | ||
| case "GetLatestValidatorSet": | ||
| resp, err = handleExternalService(ctx, req, cometQServer.GetLatestValidatorSet) | ||
| case "GetValidatorSetByHeight": | ||
| resp, err = handleExternalService(ctx, req, cometQServer.GetValidatorSetByHeight) | ||
| case "ABCIQuery": | ||
| resp, err = handleExternalService(ctx, req, cometQServer.ABCIQuery) | ||
| } | ||
|
|
||
| return resp, err | ||
| } | ||
|
|
||
| // Handle node service | ||
| if strings.Contains(req.Path, "/cosmos.base.node.v1beta1.Service") { | ||
| nodeQService := nodeServer[T]{c.cfgMap, c.cfg.AppTomlConfig, c} | ||
| paths := strings.Split(req.Path, "/") | ||
| if len(paths) <= 2 { | ||
| return nil, fmt.Errorf("invalid request path: %s", req.Path) | ||
| } | ||
|
|
||
| var resp transaction.Msg | ||
| var err error | ||
| switch paths[2] { | ||
| case "Config": | ||
| resp, err = handleExternalService(ctx, req, nodeQService.Config) | ||
| case "Status": | ||
| resp, err = handleExternalService(ctx, req, nodeQService.Status) | ||
| } | ||
|
|
||
| return resp, err | ||
| } | ||
|
|
||
| // Handle tx service | ||
| if strings.Contains(req.Path, "/cosmos.tx.v1beta1.Service") { | ||
| // init simple client context | ||
| amino := codec.NewLegacyAmino() | ||
| std.RegisterLegacyAminoCodec(amino) | ||
| txConfig := authtx.NewTxConfig( | ||
| c.appCodec, | ||
| c.appCodec.InterfaceRegistry().SigningContext().AddressCodec(), | ||
| c.appCodec.InterfaceRegistry().SigningContext().ValidatorAddressCodec(), | ||
| authtx.DefaultSignModes, | ||
| ) | ||
| rpcClient, _ := client.NewClientFromNode(c.cfg.AppTomlConfig.Address) | ||
|
|
||
| clientCtx := client.Context{}. | ||
| WithLegacyAmino(amino). | ||
| WithCodec(c.appCodec). | ||
| WithTxConfig(txConfig). | ||
| WithNodeURI(c.cfg.AppTomlConfig.Address). | ||
| WithClient(rpcClient) | ||
|
|
||
| txService := txServer[T]{ | ||
| clientCtx: clientCtx, | ||
| txCodec: c.txCodec, | ||
| app: c.app, | ||
| consensus: c, | ||
| } | ||
| paths := strings.Split(req.Path, "/") | ||
| if len(paths) <= 2 { | ||
| return nil, fmt.Errorf("invalid request path: %s", req.Path) | ||
| } | ||
|
|
||
| var resp transaction.Msg | ||
| var err error | ||
| switch paths[2] { | ||
| case "Simulate": | ||
| resp, err = handleExternalService(ctx, req, txService.Simulate) | ||
| case "GetTx": | ||
| resp, err = handleExternalService(ctx, req, txService.GetTx) | ||
| case "BroadcastTx": | ||
| return nil, errors.New("can't route a broadcast tx message") | ||
| case "GetTxsEvent": | ||
| resp, err = handleExternalService(ctx, req, txService.GetTxsEvent) | ||
| case "GetBlockWithTxs": | ||
| resp, err = handleExternalService(ctx, req, txService.GetBlockWithTxs) | ||
| case "TxDecode": | ||
| resp, err = handleExternalService(ctx, req, txService.TxDecode) | ||
| case "TxEncode": | ||
| resp, err = handleExternalService(ctx, req, txService.TxEncode) | ||
| case "TxEncodeAmino": | ||
| resp, err = handleExternalService(ctx, req, txService.TxEncodeAmino) | ||
| case "TxDecodeAmino": | ||
| resp, err = handleExternalService(ctx, req, txService.Simulate) | ||
| } | ||
|
|
||
| return resp, err | ||
| } | ||
|
|
||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Handle error from RPC client creation
The error from rpchttp.New is being ignored. This could lead to runtime issues if the client creation fails.
Apply this diff to handle the error:
- rpcClient, _ := rpchttp.New(c.cfg.AppTomlConfig.Address)
+ rpcClient, err := rpchttp.New(c.cfg.AppTomlConfig.Address)
+ if err != nil {
+ return nil, err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *consensus[T]) maybeHandleExternalServices(ctx context.Context, req *abci.QueryRequest) (transaction.Msg, error) { | |
| // Handle comet service | |
| if strings.Contains(req.Path, "/cosmos.base.tendermint.v1beta1.Service") { | |
| rpcClient, _ := rpchttp.New(c.cfg.AppTomlConfig.Address) | |
| cometQServer := cmtservice.NewQueryServer(rpcClient, c.Query, c.consensusAddressCodec) | |
| paths := strings.Split(req.Path, "/") | |
| if len(paths) <= 2 { | |
| return nil, fmt.Errorf("invalid request path: %s", req.Path) | |
| } | |
| var resp transaction.Msg | |
| var err error | |
| switch paths[2] { | |
| case "GetNodeInfo": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetNodeInfo) | |
| case "GetSyncing": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetSyncing) | |
| case "GetLatestBlock": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetLatestBlock) | |
| case "GetBlockByHeight": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetBlockByHeight) | |
| case "GetLatestValidatorSet": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetLatestValidatorSet) | |
| case "GetValidatorSetByHeight": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetValidatorSetByHeight) | |
| case "ABCIQuery": | |
| resp, err = handleExternalService(ctx, req, cometQServer.ABCIQuery) | |
| } | |
| return resp, err | |
| } | |
| // Handle node service | |
| if strings.Contains(req.Path, "/cosmos.base.node.v1beta1.Service") { | |
| nodeQService := nodeServer[T]{c.cfgMap, c.cfg.AppTomlConfig, c} | |
| paths := strings.Split(req.Path, "/") | |
| if len(paths) <= 2 { | |
| return nil, fmt.Errorf("invalid request path: %s", req.Path) | |
| } | |
| var resp transaction.Msg | |
| var err error | |
| switch paths[2] { | |
| case "Config": | |
| resp, err = handleExternalService(ctx, req, nodeQService.Config) | |
| case "Status": | |
| resp, err = handleExternalService(ctx, req, nodeQService.Status) | |
| } | |
| return resp, err | |
| } | |
| // Handle tx service | |
| if strings.Contains(req.Path, "/cosmos.tx.v1beta1.Service") { | |
| // init simple client context | |
| amino := codec.NewLegacyAmino() | |
| std.RegisterLegacyAminoCodec(amino) | |
| txConfig := authtx.NewTxConfig( | |
| c.appCodec, | |
| c.appCodec.InterfaceRegistry().SigningContext().AddressCodec(), | |
| c.appCodec.InterfaceRegistry().SigningContext().ValidatorAddressCodec(), | |
| authtx.DefaultSignModes, | |
| ) | |
| rpcClient, _ := client.NewClientFromNode(c.cfg.AppTomlConfig.Address) | |
| clientCtx := client.Context{}. | |
| WithLegacyAmino(amino). | |
| WithCodec(c.appCodec). | |
| WithTxConfig(txConfig). | |
| WithNodeURI(c.cfg.AppTomlConfig.Address). | |
| WithClient(rpcClient) | |
| txService := txServer[T]{ | |
| clientCtx: clientCtx, | |
| txCodec: c.txCodec, | |
| app: c.app, | |
| consensus: c, | |
| } | |
| paths := strings.Split(req.Path, "/") | |
| if len(paths) <= 2 { | |
| return nil, fmt.Errorf("invalid request path: %s", req.Path) | |
| } | |
| var resp transaction.Msg | |
| var err error | |
| switch paths[2] { | |
| case "Simulate": | |
| resp, err = handleExternalService(ctx, req, txService.Simulate) | |
| case "GetTx": | |
| resp, err = handleExternalService(ctx, req, txService.GetTx) | |
| case "BroadcastTx": | |
| return nil, errors.New("can't route a broadcast tx message") | |
| case "GetTxsEvent": | |
| resp, err = handleExternalService(ctx, req, txService.GetTxsEvent) | |
| case "GetBlockWithTxs": | |
| resp, err = handleExternalService(ctx, req, txService.GetBlockWithTxs) | |
| case "TxDecode": | |
| resp, err = handleExternalService(ctx, req, txService.TxDecode) | |
| case "TxEncode": | |
| resp, err = handleExternalService(ctx, req, txService.TxEncode) | |
| case "TxEncodeAmino": | |
| resp, err = handleExternalService(ctx, req, txService.TxEncodeAmino) | |
| case "TxDecodeAmino": | |
| resp, err = handleExternalService(ctx, req, txService.Simulate) | |
| } | |
| return resp, err | |
| } | |
| return nil, nil | |
| } | |
| func (c *consensus[T]) maybeHandleExternalServices(ctx context.Context, req *abci.QueryRequest) (transaction.Msg, error) { | |
| // Handle comet service | |
| if strings.Contains(req.Path, "/cosmos.base.tendermint.v1beta1.Service") { | |
| rpcClient, err := rpchttp.New(c.cfg.AppTomlConfig.Address) | |
| if err != nil { | |
| return nil, err | |
| } | |
| cometQServer := cmtservice.NewQueryServer(rpcClient, c.Query, c.consensusAddressCodec) | |
| paths := strings.Split(req.Path, "/") | |
| if len(paths) <= 2 { | |
| return nil, fmt.Errorf("invalid request path: %s", req.Path) | |
| } | |
| var resp transaction.Msg | |
| var err error | |
| switch paths[2] { | |
| case "GetNodeInfo": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetNodeInfo) | |
| case "GetSyncing": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetSyncing) | |
| case "GetLatestBlock": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetLatestBlock) | |
| case "GetBlockByHeight": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetBlockByHeight) | |
| case "GetLatestValidatorSet": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetLatestValidatorSet) | |
| case "GetValidatorSetByHeight": | |
| resp, err = handleExternalService(ctx, req, cometQServer.GetValidatorSetByHeight) | |
| case "ABCIQuery": | |
| resp, err = handleExternalService(ctx, req, cometQServer.ABCIQuery) | |
| } | |
| return resp, err | |
| } | |
| // Handle node service | |
| if strings.Contains(req.Path, "/cosmos.base.node.v1beta1.Service") { | |
| nodeQService := nodeServer[T]{c.cfgMap, c.cfg.AppTomlConfig, c} | |
| paths := strings.Split(req.Path, "/") | |
| if len(paths) <= 2 { | |
| return nil, fmt.Errorf("invalid request path: %s", req.Path) | |
| } | |
| var resp transaction.Msg | |
| var err error | |
| switch paths[2] { | |
| case "Config": | |
| resp, err = handleExternalService(ctx, req, nodeQService.Config) | |
| case "Status": | |
| resp, err = handleExternalService(ctx, req, nodeQService.Status) | |
| } | |
| return resp, err | |
| } | |
| // Handle tx service | |
| if strings.Contains(req.Path, "/cosmos.tx.v1beta1.Service") { | |
| // init simple client context | |
| amino := codec.NewLegacyAmino() | |
| std.RegisterLegacyAminoCodec(amino) | |
| txConfig := authtx.NewTxConfig( | |
| c.appCodec, | |
| c.appCodec.InterfaceRegistry().SigningContext().AddressCodec(), | |
| c.appCodec.InterfaceRegistry().SigningContext().ValidatorAddressCodec(), | |
| authtx.DefaultSignModes, | |
| ) | |
| rpcClient, _ := client.NewClientFromNode(c.cfg.AppTomlConfig.Address) | |
| clientCtx := client.Context{}. | |
| WithLegacyAmino(amino). | |
| WithCodec(c.appCodec). | |
| WithTxConfig(txConfig). | |
| WithNodeURI(c.cfg.AppTomlConfig.Address). | |
| WithClient(rpcClient) | |
| txService := txServer[T]{ | |
| clientCtx: clientCtx, | |
| txCodec: c.txCodec, | |
| app: c.app, | |
| consensus: c, | |
| } | |
| paths := strings.Split(req.Path, "/") | |
| if len(paths) <= 2 { | |
| return nil, fmt.Errorf("invalid request path: %s", req.Path) | |
| } | |
| var resp transaction.Msg | |
| var err error | |
| switch paths[2] { | |
| case "Simulate": | |
| resp, err = handleExternalService(ctx, req, txService.Simulate) | |
| case "GetTx": | |
| resp, err = handleExternalService(ctx, req, txService.GetTx) | |
| case "BroadcastTx": | |
| return nil, errors.New("can't route a broadcast tx message") | |
| case "GetTxsEvent": | |
| resp, err = handleExternalService(ctx, req, txService.GetTxsEvent) | |
| case "GetBlockWithTxs": | |
| resp, err = handleExternalService(ctx, req, txService.GetBlockWithTxs) | |
| case "TxDecode": | |
| resp, err = handleExternalService(ctx, req, txService.TxDecode) | |
| case "TxEncode": | |
| resp, err = handleExternalService(ctx, req, txService.TxEncode) | |
| case "TxEncodeAmino": | |
| resp, err = handleExternalService(ctx, req, txService.TxEncodeAmino) | |
| case "TxDecodeAmino": | |
| resp, err = handleExternalService(ctx, req, txService.Simulate) | |
| } | |
| return resp, err | |
| } | |
| return nil, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/v2/cometbft/grpc.go (1)
281-281: Address TODO comment regarding Tx interfaceThe TODO comment suggests breaking the Tx interface to add the AsTx method. This architectural decision needs to be addressed.
Would you like me to help create a GitHub issue to track this architectural improvement?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/abci.go(4 hunks)server/v2/cometbft/grpc.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/grpc.go
24-24: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (6)
server/v2/cometbft/grpc.go (4)
133-138: Fix off-by-one error and potential underflow in reverse pagination loop
The reverse pagination loop may skip the first transaction (index 0) due to the condition i > 0 and can cause an underflow when i is 0 because i is of type uint64.
187-213: LGTM! Comprehensive implementation with proper error handling
The GetTxsEvent implementation correctly handles request validation, error cases, and type assertions.
467-467: Handle error returned by rpchttp.New
The error returned by rpchttp.New is being ignored. This could lead to runtime issues if the client creation fails.
576-598: LGTM! Well-structured generic implementation
The handleExternalService function is well-implemented with proper type constraints and error handling.
server/v2/cometbft/abci.go (2)
87-90: LGTM! Well-structured additions to consensus type
The new fields enhance the consensus type's capabilities while maintaining good separation of concerns.
187-195: LGTM! Proper handling of query height
The implementation correctly sets the latest height for queries when not provided, with proper error handling.
|
Could you fix this: #22803 (comment) then it's good to merge |
julienrbrt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
server/v2/cometbft/grpc.go (1)
121-121: Remove debug print statementThe
fmt.Printlnstatement should be removed as it's likely a debug statement.Apply this diff:
-fmt.Println("TxDecoder", txb, err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/grpc.go(8 hunks)server/v2/cometbft/server.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/server.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/cometbft/grpc.go
24-24: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (4)
server/v2/cometbft/grpc.go (4)
577-599: LGTM: Well-designed generic helper function
The handleExternalService function is well-designed using Go generics to provide type-safe handling of external service requests.
133-138:
Fix off-by-one error in reverse pagination loop
The reverse pagination loop has an off-by-one error that may skip transactions and can cause an underflow when i is 0.
Apply this diff to fix the issue:
-for i, count := offset, uint64(0); i > 0 && count != limit; i, count = i-1, count+1 {
+for i, count := int64(offset), uint64(0); i >= 0 && count != limit; i, count = i-1, count+1 {528-529:
Handle error from client creation
Similar to the RPC client creation, the error from NewClientFromNode is being ignored.
Apply this diff to handle the error:
-rpcClient, _ := client.NewClientFromNode(c.cfg.AppTomlConfig.Address)
+rpcClient, err := client.NewClientFromNode(c.cfg.AppTomlConfig.Address)
+if err != nil {
+ return nil, fmt.Errorf("failed to create client: %w", err)
+}Likely invalid or redundant comment.
467-468:
Handle error from RPC client creation
The error from rpchttp.New is being ignored, which could lead to runtime issues if the client creation fails.
Apply this diff to handle the error:
-rpcClient, _ := rpchttp.New(c.cfg.AppTomlConfig.Address)
+rpcClient, err := rpchttp.New(c.cfg.AppTomlConfig.Address)
+if err != nil {
+ return nil, fmt.Errorf("failed to create RPC client: %w", err)
+}Likely invalid or redundant comment.
server/v2/cometbft/grpc.go
Outdated
| resp, err = handleExternalService(ctx, req, txService.Simulate) | ||
| } |
There was a problem hiding this comment.
Fix incorrect handler in TxDecodeAmino case
The TxDecodeAmino case is incorrectly using the Simulate handler instead of TxDecodeAmino.
Apply this diff to fix the handler:
-resp, err = handleExternalService(ctx, req, txService.Simulate)
+resp, err = handleExternalService(ctx, req, txService.TxDecodeAmino)📝 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.
| resp, err = handleExternalService(ctx, req, txService.Simulate) | |
| } | |
| resp, err = handleExternalService(ctx, req, txService.TxDecodeAmino) | |
| } |
Co-authored-by: Julien Robert <[email protected]> (cherry picked from commit a38a6a2)
…port #22803) (#22834) Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Follow up: #22753 #22785
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
Documentation