fix(simapp/v2): register extra gRPC gateway routes #22786
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications primarily to 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
julienrbrt
changed the title
julienrbrt
added
the
backport/v0.52.x
julienrbrt
changed the title
julienrbrt
requested review from
kocubinski,
a team,
facundomedica and
sontrinh16
as code owners
|
@julienrbrt your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/cometbft/grpc.go(5 hunks)simapp/v2/simdv2/cmd/commands.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
simapp/v2/simdv2/cmd/commands.go (1)
267-268: Consider the implications of the TODO comment.
The comment indicates that this function is temporary and will be removed in favor of direct registration within grpcgateway.Server. This architectural change should be tracked to ensure a smooth transition.
Run the following script to verify the referenced PR discussion:
✅ Verification successful
Let me gather more information about the grpcgateway server implementation to better understand the architectural changes.
Let me check the implementation of the registerGRPCGatewayRoutes function to understand what functionality will be moved.
TODO comment accurately reflects planned architectural change
The TODO comment refers to moving the route registration logic from the registerGRPCGatewayRoutes function directly into the grpcgateway.Server implementation. This is a valid architectural improvement as it would:
- Centralize the gateway route registration logic
- Remove the need for external registration functions
- Make the routing setup more cohesive within the server implementation
The change is already being tracked in PR #22701 and has been discussed in the review.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the referenced PR discussion about architectural changes
gh pr view 22701 --json comments | jq '.comments[] | select(.body | contains("grpcgateway.Server"))'
Length of output: 14035
Script:
#!/bin/bash
# Search for grpcgateway server implementation and related code
ast-grep --pattern 'type Server[T any] struct {
$$$
}'
# Search for related TODO comments
rg -n "TODO.*grpcgateway.Server" -A 2
Length of output: 446
Script:
#!/bin/bash
# Get the implementation of registerGRPCGatewayRoutes
ast-grep --pattern 'func registerGRPCGatewayRoutes[T transaction.Tx]($$$) {
$$$
}'
# Search for grpcgateway.Server usage
rg -n "grpcgateway.Server" -A 2
Length of output: 2187
server/v2/cometbft/grpc.go (2)
73-99: LGTM! Improved error handling in GetTx method.
The changes add proper input validation and structured error responses, making the API more robust and user-friendly.
284-327: LGTM! Well-structured CLI descriptor.
The CLI descriptor provides clear and comprehensive command options with appropriate descriptions and aliases.
| func (t txServer[T]) TxDecodeAmino(_ context.Context, req *txtypes.TxDecodeAminoRequest) (*txtypes.TxDecodeAminoResponse, error) { | ||
| if req.AminoBinary == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "invalid empty tx bytes") | ||
| } | ||
|
|
||
| var stdTx legacytx.StdTx | ||
| err := t.clientCtx.LegacyAmino.Unmarshal(req.AminoBinary, &stdTx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| res, err := t.clientCtx.LegacyAmino.MarshalJSON(stdTx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &txtypes.TxDecodeAminoResponse{ | ||
| AminoJson: string(res), | ||
| }, nil |
There was a problem hiding this comment.
💡 Codebase verification
Add size limits to protect against memory exhaustion attacks
The gRPC service has default message size limits (10MB for receive and MaxInt32 for send), but the Amino codec operations lack specific size limits. To prevent potential memory exhaustion attacks:
- Add size validation before unmarshaling in
TxDecodeAmino(e.g., check iflen(req.AminoBinary)exceeds a reasonable limit) - Add size validation for the JSON string in
TxEncodeAmino(e.g., check iflen(req.AminoJson)exceeds a reasonable limit) - Consider using the gRPC service's
DefaultGRPCMaxRecvMsgSize(10MB) as a reference point for the size limits
🔗 Analysis chain
Consider adding size limits for Amino encoding/decoding.
The methods handle input validation but lack size limits for the binary/JSON data. This could potentially lead to memory exhaustion attacks.
Run the following script to check if size limits are implemented elsewhere:
Also applies to: 221-239
Let me gather more information about size limits in the codebase.
Let me check for any size validation in the Amino codec implementation.
Let me check if there are any size limits defined in the configuration or constants.
Let me check for any size limits in the gRPC configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for size limit implementations in Amino codec usage
rg -A 5 "LegacyAmino.*Marshal"
Length of output: 9168
Script:
#!/bin/bash
# Check for any size limits or validation in the Amino codec implementation
rg -A 5 "func.*Unmarshal.*Amino"
# Check for any configuration or constants related to size limits
rg -i "maxsize|max_size|size.*limit"
Length of output: 95385
Script:
#!/bin/bash
# Check the Amino codec implementation for size validation
rg -A 5 "func.*Unmarshal.*" codec/amino.go
# Check for any size validation in the tx service
rg -A 5 "func.*TxDecode" server/v2/cometbft/grpc.go
Length of output: 1849
Script:
#!/bin/bash
# Check for any size limits in configuration files
rg -i "max.*size.*=|size.*limit.*="
# Check for size-related constants
rg "const.*size|const.*limit"
Length of output: 117538
Script:
#!/bin/bash
# Check for gRPC size limits in configuration
rg -A 5 "DefaultGRPCMaxRecvMsgSize|DefaultGRPCMaxSendMsgSize" server/config/
# Check for any size validation in the tx service implementation
rg -A 5 "func.*Tx.*Amino" server/v2/cometbft/
Length of output: 2089
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
(cherry picked from commit 9525d91)
…22798) Co-authored-by: Julien Robert <[email protected]>
Description
The routes won't be working just yet, and that should be fixed during #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