feat(server/v2/grpcgateway): register grpcgateway server and module endpoints #22701
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks promising! Thanks for tackling this.
Just note that if we merge this (as this will be done before I'm done lol), this will be a temporary solution.
Our goal is to kill that explicit gRPC gateway registration (RegisterGRPCGatewayRoutes and generated code), and do some proto options parsing directly in the grpcgateway server. This will allow us to let user have modules even smaller and no SDK dependency and gRPC dependencies at all (roughly this: #20798 (comment))
julienrbrt
changed the title
since contains prefix: error in json rpc client, with http response metadata: (Status: 200 OK, Protocol HTTP/1.1).
![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
🧹 Outside diff range and nitpick comments (6)
simapp/v2/simdv2/cmd/commands.go (1)
36-36: Organize imports as pergcitool recommendationsThe imports are not properly grouped and sorted according to the
gcitool, which helps maintain consistency and readability.Apply this diff to organize the imports:
+ "github.com/grpc-ecosystem/grpc-gateway/runtime" + "github.com/spf13/cobra" "cosmossdk.io/client/v2/offchain" + "cosmossdk.io/core/server" + "cosmossdk.io/log" + "cosmossdk.io/runtime/v2" + "cosmossdk.io/server/v2" + grpcserver "cosmossdk.io/server/v2/api/grpc" + "cosmossdk.io/server/v2/api/grpcgateway" + "cosmossdk.io/server/v2/api/rest" + "cosmossdk.io/server/v2/api/telemetry" + "cosmossdk.io/server/v2/cometbft" + serverstore "cosmossdk.io/server/v2/store" + "cosmossdk.io/simapp/v2" + confixcmd "cosmossdk.io/tools/confix/cmd" - "cosmossdk.io/client/v2/offchain" - coreserver "cosmossdk.io/core/server" - "cosmossdk.io/log" - runtimev2 "cosmossdk.io/runtime/v2" - serverv2 "cosmossdk.io/server/v2" - grpcserver "cosmossdk.io/server/v2/api/grpc" - "cosmossdk.io/server/v2/api/grpcgateway" - "cosmossdk.io/server/v2/api/rest" - "cosmossdk.io/server/v2/api/telemetry" - "cosmossdk.io/server/v2/cometbft" - serverstore "cosmossdk.io/server/v2/store" - "cosmossdk.io/simapp/v2" - confixcmd "cosmossdk.io/tools/confix/cmd" "github.com/cosmos/cosmos-sdk/client" - sdkclient "github.com/cosmos/cosmos-sdk/client" + sdkclient "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/client/config" "github.com/cosmos/cosmos-sdk/client/debug" "github.com/cosmos/cosmos-sdk/client/keys"🧰 Tools
🪛 golangci-lint (1.62.2)
36-36: 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/api/grpcgateway/server.go (1)
34-34: EncapsulateGRPCGatewayRouterwith an accessor methodExporting the
GRPCGatewayRouterfield exposes internal implementation details. Consider providing a getter method to maintain encapsulation and allow for future internal changes without affecting external code.Implement an accessor method like:
func (s *Server[T]) GetGRPCGatewayRouter() *runtime.ServeMux { return s.GRPCGatewayRouter }CHANGELOG.md (4)
Line range hint
1-1: Add version table at the top of changelogConsider adding a version table at the top of the changelog that lists all versions with their release dates and links. This helps users quickly navigate to specific versions.
Line range hint
13-15: Improve PR objectives section formattingThe PR objectives section contains raw GitHub markdown which makes it harder to read. Consider reformatting this section to match the rest of the changelog style.
Line range hint
1039-1042: Fix inconsistent heading styleThe heading "Previous Releases" uses a different style (H2 with markdown link) compared to other sections. Consider using the same heading style as other sections for consistency.
Line range hint
1044-1044: Remove stray code blockThere is a stray empty code block at the end of the file that should be removed.
- ```
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
tests/systemtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)server/v2/api/grpc/server.go(1 hunks)server/v2/api/grpcgateway/server.go(4 hunks)simapp/v2/simdv2/cmd/commands.go(6 hunks)simapp/v2/simdv2/cmd/testnet.go(4 hunks)tests/systemtests/go.mod(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
tests/systemtests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
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.
server/v2/api/grpcgateway/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/testnet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 golangci-lint (1.62.2)
simapp/v2/simdv2/cmd/commands.go
24-24: ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/client"
(stylecheck)
36-36: 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)
simapp/v2/simdv2/cmd/commands.go (1)
163-167:
Iteration over map may lead to non-deterministic behavior
Iterating over a map (deps.ModuleManager.Modules()) can result in non-deterministic execution order, which may cause unexpected behavior. This is important in scenarios where the order of registration affects the system.
Consider converting the map to a slice or ensuring the iteration order is deterministic to maintain consistent behavior.
simapp/v2/simdv2/cmd/testnet.go (3)
23-23: LGTM!
The import statement correctly includes the grpcgateway package, enabling gRPC Gateway functionality.
Line range hint 196-214: Verify port configurations to avoid conflicts
When running multiple validators on a single machine, ensure that the grpcgatewayConfig addresses are correctly configured to prevent port conflicts.
Please confirm that apiPort and grpcPort offsets are correctly applied.
348-350: LGTM!
The grpcgatewayServer is appropriately instantiated and included in the server components, enhancing the API capabilities.
server/v2/api/grpcgateway/server.go (1)
86-92: LGTM!
The NewWithConfigOptions method enhances flexibility by allowing configuration options during server initialization.
server/v2/api/grpc/server.go (1)
131-134: LGTM!
The GrpcServer() method provides necessary access to the internal gRPC server instance, facilitating integration with other components.
simapp/v2/simdv2/cmd/commands.go
Outdated
| @@ -20,6 +21,7 @@ import ( | |||
| confixcmd "cosmossdk.io/tools/confix/cmd" | |||
|
|
|||
| "github.com/cosmos/cosmos-sdk/client" | |||
| sdkclient "github.com/cosmos/cosmos-sdk/client" | |||
There was a problem hiding this comment.
Avoid duplicate imports of the same package
The package "github.com/cosmos/cosmos-sdk/client" is imported twice: once without an alias and once with the alias sdkclient. This can cause confusion and potential conflicts.
Apply the following diff to remove the duplicate import:
- "github.com/cosmos/cosmos-sdk/client"Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
24-24: ST1019(related information): other import of "github.com/cosmos/cosmos-sdk/client"
(stylecheck)
tests/systemtests/go.mod
Outdated
| @@ -167,3 +167,5 @@ require ( | |||
| pgregory.net/rapid v1.1.0 // indirect | |||
| sigs.k8s.io/yaml v1.4.0 // indirect | |||
| ) | |||
|
|
|||
| replace cosmossdk.io/systemtests => ../../systemtests | |||
There was a problem hiding this comment.
Remove local replace directive before merging
The replace cosmossdk.io/systemtests => ../../systemtests directive is intended for local development and testing. Committing it can cause build issues for others who do not have the same directory structure.
Please remove the replace directive before merging to the main branch.
tests/systemtests/go.mod
Outdated
| @@ -167,3 +167,5 @@ require ( | |||
| pgregory.net/rapid v1.1.0 // indirect | |||
| sigs.k8s.io/yaml v1.4.0 // indirect | |||
| ) | |||
|
|
|||
| replace cosmossdk.io/systemtests => ../../systemtests | |||
There was a problem hiding this comment.
Could you remove that replace? We have rc.2 tagged
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
simapp/v2/simdv2/cmd/commands.go (1)
38-40: Add godoc comments for the interfaceThe interface is well-designed, but would benefit from godoc comments explaining its purpose and usage.
Add documentation like this:
+// ModuleWithGRPCGateway defines the interface that should be implemented by +// modules that want to register their gRPC Gateway routes. type ModuleWithGRPCGateway interface { + // RegisterGRPCGatewayRoutes registers the gRPC Gateway routes for the module. RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *gwruntime.ServeMux) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
simapp/v2/simdv2/cmd/commands.go(5 hunks)simapp/v2/simdv2/cmd/testnet.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simapp/v2/simdv2/cmd/testnet.go
🧰 Additional context used
📓 Path-based instructions (1)
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
simapp/v2/simdv2/cmd/commands.go
35-35: 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)
simapp/v2/simdv2/cmd/commands.go (4)
15-15: LGTM: Import statements are correctly organized
The new imports for gRPC gateway functionality are properly placed and necessary for the implementation.
Also applies to: 35-35
152-160: LGTM: Proper server initialization with error handling
The gRPC gateway server initialization is well-structured with appropriate error handling and dependency injection.
180-180: LGTM: Server command properly registered
The gRPC gateway server is correctly added to the server commands list.
162-166: Verify module registration order consistency
The implementation is correct, but let's verify that the module registration order is deterministic across different runs.
✅ Verification successful
Module registration order is deterministic and well-implemented
The implementation ensures consistent module ordering through several mechanisms:
- Module names are explicitly sorted using
slices.Sorted(maps.Keys(modules))in theNewModuleManagerconstructor - The order is preserved in pre-blockers, begin-blockers, and end-blockers
- Migration order follows a deterministic pattern with
defaultMigrationsOrderthat sorts modules alphabetically - Module registration prevents duplicates and maintains consistency through name validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ModuleManager.Modules() returns modules in a consistent order
# Search for Modules() method implementation
ast-grep --pattern 'func ($mm *MM) Modules() []$_ {
$$$
}'
# Search for any tests verifying module order
rg -l "TestModule.*Order" --type go
Length of output: 100
Script:
#!/bin/bash
# Let's try different patterns to find the Modules() implementation and module ordering tests
# Search for Modules() method with more flexible pattern
ast-grep --pattern 'func $_($_ *ModuleManager) Modules() []$_ {'
# Also try searching in the specific package
rg -A 10 "func.*Modules\(\)" simapp/v2/simdv2/
# Look for module registration related tests with broader patterns
rg -l "Test.*Module.*Registration" --type go
# Check for module manager initialization
rg -A 5 "NewModuleManager|InitModuleManager" --type go
Length of output: 982
Script:
#!/bin/bash
# Let's examine the module manager implementation more closely
# Check the module manager constructor and its ordering logic
rg -A 15 "func NewModuleManager" runtime/v2/manager.go
# Look for any module ordering related constants or variables
rg "ModuleOrder|OrderedModules|moduleOrder" runtime/v2/
# Check if there's any explicit sorting or ordering logic
rg -A 5 "sort\.|append\(|Order" runtime/v2/manager.go
# Look for module registration in tests
rg -A 10 "RegisterModule|AddModule" runtime/v2/
Length of output: 3848
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
simapp/v2/simdv2/cmd/commands.go (1)
38-40: Add godoc comments for the interfaceThe interface is well-defined, but would benefit from documentation explaining its purpose and usage.
Add godoc comments:
+// ModuleWithGRPCGateway defines the interface that modules must implement to register +// their gRPC Gateway routes. type ModuleWithGRPCGateway interface { + // RegisterGRPCGatewayRoutes registers the gRPC Gateway routes for the module. RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *gwruntime.ServeMux) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
simapp/v2/simdv2/cmd/commands.go(6 hunks)simapp/v2/simdv2/cmd/testnet.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simapp/v2/simdv2/cmd/testnet.go
🧰 Additional context used
📓 Path-based instructions (1)
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
simapp/v2/simdv2/cmd/commands.go (4)
6-6: LGTM: Required imports added correctly
The new imports for gRPC gateway runtime and server components are properly organized and necessary for the new functionality.
Also applies to: 16-16
150-160: LGTM: Proper server initialization with error handling
The gRPC gateway server initialization is well-structured with appropriate error handling and dependency injection.
180-180: LGTM: Server properly added to command list
The gRPC gateway server is correctly added to the server commands list.
162-166:
Ensure deterministic module registration order
The current implementation iterates over modules using a map, which could lead to non-deterministic registration order.
Consider using a deterministic approach:
-for _, mod := range deps.ModuleManager.Modules() {
+// Get modules in a deterministic order
+modules := deps.ModuleManager.OrderedModules()
+for _, mod := range modules {
if gmod, ok := mod.(ModuleWithGRPCGateway); ok {
gmod.RegisterGRPCGatewayRoutes(deps.ClientContext, grpcgatewayServer.GRPCGatewayRouter)
}
}
CHANGELOG.md
Outdated
| @@ -51,6 +51,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| * RocksDB libraries have been upgraded to support RockDB v9 instead of v8. | |||
| * (testutil/integration) [#22616](https://github.com/cosmos/cosmos-sdk/pull/22616) Remove double context in integration tests v1. | |||
| * Use integrationApp.Context() instead of creating a context prior. | |||
| * (server) [#22701](https://github.com/cosmos/cosmos-sdk/pull/22701) Register grpcgateway server and module endpoints for server/v2. | |||
server/v2/api/grpcgateway/server.go
Outdated
| @@ -31,7 +31,7 @@ type Server[T transaction.Tx] struct { | |||
|
|
|||
| server *http.Server | |||
| gRPCSrv *grpc.Server | |||
There was a problem hiding this comment.
Looks like grpc.Server isn't used here. Let's remove it from here
server/v2/api/grpc/server.go
Outdated
| @@ -128,6 +128,10 @@ func (s *Server[T]) StartCmdFlags() *pflag.FlagSet { | |||
| return flags | |||
| } | |||
|
|
|||
| func (s *Server[T]) GrpcServer() *grpc.Server { | |||
There was a problem hiding this comment.
See other comment, if we don't need the grpc server, let's not expose this
simapp/v2/simdv2/cmd/commands.go
Outdated
| @@ -33,6 +35,10 @@ import ( | |||
| v2 "github.com/cosmos/cosmos-sdk/x/genutil/v2/cli" | |||
| ) | |||
|
|
|||
| type ModuleWithGRPCGateway interface { | |||
simapp/v2/simdv2/cmd/commands.go
Outdated
| } | ||
|
|
||
| for _, mod := range deps.ModuleManager.Modules() { | ||
| if gmod, ok := mod.(ModuleWithGRPCGateway); ok { |
There was a problem hiding this comment.
let's use module.HasGRPCGateway (from types/modules), and can you add a /TODO(@julienrbrt) and link #22701 (review)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
simapp/v2/simdv2/cmd/commands.go (1)
148-156: Consider documenting the nil configuration parameterThe server initialization looks good, but the
nilvalue passed as the third parameter could benefit from a comment explaining why no configuration options are needed here.grpcgatewayServer, err := grpcgateway.New[T]( logger, deps.GlobalConfig, - nil, + nil, // No additional configuration options needed for basic setup simApp.InterfaceRegistry(), )
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
simapp/v2/simdv2/cmd/commands.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (4)
simapp/v2/simdv2/cmd/commands.go (4)
15-15: LGTM: Import addition is well-placed
The grpcgateway import is correctly placed with other server-related imports.
90-90: LGTM: CLI skeleton properly includes gRPC gateway server
The gRPC gateway server is correctly added to the server components list.
158-163: Use module.HasGRPCGateway for type checking
As mentioned in the previous review, we should use module.HasGRPCGateway for type checking.
for _, mod := range deps.ModuleManager.Modules() {
- if gmod, ok := mod.(module.HasGRPCGateway); ok {
+ if module.HasGRPCGateway(mod) {
// TODO(@julienrbrt) https://github.com/cosmos/cosmos-sdk/pull/22701#pullrequestreview-2470651390
- gmod.RegisterGRPCGatewayRoutes(deps.ClientContext, grpcgatewayServer.GRPCGatewayRouter)
+ mod.RegisterGRPCGatewayRoutes(deps.ClientContext, grpcgatewayServer.GRPCGatewayRouter)
}
}177-177: LGTM: Server properly added to command list
The gRPC gateway server is correctly added to the AddCommands call.
| for _, mod := range deps.ModuleManager.Modules() { | ||
| if gmod, ok := mod.(module.HasGRPCGateway); ok { | ||
| // TODO(@julienrbrt) https://github.com/cosmos/cosmos-sdk/pull/22701#pullrequestreview-2470651390 | ||
| gmod.RegisterGRPCGatewayRoutes(deps.ClientContext, grpcgatewayServer.GRPCGatewayRouter) | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
julienrbrt
added
the
backport/v0.52.x
|
Will be partially backported (simapp changes) |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
Description
Closes: #XXXX
Ref:
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