refactor(server/v2): remove serverv2.AppI #22446
Conversation
julienrbrt
requested review from
kocubinski,
a team and
hieuvubk
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifying the handling 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
server/v2/api/grpc/service.go (1)
18-18: LGTM! Consider adding field documentation.The change from interface to function type is a good simplification that aligns with Go's preference for small, focused interfaces. Consider adding a doc comment to describe the queryable function's purpose and expected behavior.
+// queryable is a function that processes query messages and returns responses. +// It takes a context, version, and message, returning a response message or error. queryable func(ctx context.Context, version uint64, msg transaction.Msg) (transaction.Msg, error)server/v2/server_test.go (2)
57-57: Document the purpose of empty handler mapWhile the initialization looks good, consider adding a comment explaining why an empty handler map is sufficient for these tests. This helps future maintainers understand the test setup better.
+ // Empty handler map is used as we're only testing server initialization grpcServer, err := grpc.New[transaction.Tx](logger, &mockInterfaceRegistry{}, map[string]appmodulev2.Handler{}, nil, cfg)
60-60: Consider adding error scenarios to the testThe current test covers the happy path, but it would be valuable to add test cases for error scenarios, such as store initialization failures.
server/v2/go.mod (1)
Line range hint
3-3: Update Go version to a released version.The module currently specifies
go 1.23which hasn't been released yet. The latest stable version is Go 1.22.Apply this diff:
-go 1.23 +go 1.22server/v2/api/grpc/service_test.go (1)
Line range hint
40-54: Consider adding validation tests for mock messages.While the mock messages implement the required interfaces, consider adding tests for the
ValidateBasic()method to ensure proper validation handling.+func TestMockMessageValidation(t *testing.T) { + msg := &MockRequestMessage{Data: "test"} + assert.NoError(t, msg.ValidateBasic()) + + resp := &MockResponseMessage{Data: "test"} + assert.NoError(t, resp.ValidateBasic()) +}server/v2/api/grpc/server.go (1)
100-102: Consider improving error handling for queryable functionWhile the function type change is good, consider wrapping the error returned from
queryablebefore returning it to the client to prevent potential information leakage.Consider applying this change:
resp, err := queryable(ctx, height, req) if err != nil { - return err + return status.Error(codes.Internal, "failed to process query") }Also applies to: 150-153
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (9)
server/v2/api/grpc/server.go(4 hunks)server/v2/api/grpc/service.go(2 hunks)server/v2/api/grpc/service_test.go(2 hunks)server/v2/go.mod(1 hunks)server/v2/server_test.go(2 hunks)server/v2/store/server.go(2 hunks)server/v2/types.go(0 hunks)simapp/v2/app_di.go(0 hunks)simapp/v2/simdv2/cmd/commands.go(1 hunks)
💤 Files with no reviewable changes (2)
- server/v2/types.go
- simapp/v2/app_di.go
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/api/grpc/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpc/service.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpc/service_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
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"
server/v2/store/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.
🔇 Additional comments (12)
server/v2/api/grpc/service.go (2)
40-40: LGTM! Implementation correctly adapted to function type.
The change maintains proper error handling and type safety while simplifying the code by removing the interface indirection.
40-40: Verify the hardcoded version parameter.
The version parameter is hardcoded to 0 in the queryable function call. Please verify if this is intentional or if it should be derived from the request.
✅ Verification successful
Let me gather more context about the queryable function and its usage.
Let me check how this queryable function is being used in the service implementation and if there's any version information being passed from the request.
Let me check the full Query method implementation to understand how the request is handled.
Let me check the QueryRequest proto definition to see if it contains any version field.
Hardcoded version 0 is intentional and correct
The hardcoded version 0 in queryable function call is intentional. Looking at the appmanager implementation, version 0 is specifically used to query the latest available state, while non-zero versions are used for querying historical states at specific heights. Since the gRPC service's QueryRequest proto doesn't include a version field, defaulting to the latest state (version 0) is the expected behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version usage patterns in query handlers
rg -g '*.go' -B 3 -A 3 'func.*Query.*version.*uint64'
Length of output: 1630
Script:
#!/bin/bash
# Search for queryable function definition and its usages
ast-grep --pattern 'func $_ queryable($$$) ($$$)'
# Also search for any references to version in the service.go file
rg -g 'service.go' 'version'
Length of output: 422
Script:
#!/bin/bash
# Look for the service struct and method implementation
rg -g 'service.go' -B 5 -A 10 'type.*Service.*struct'
# Also check for any request handling related to version
ast-grep --pattern 'func ($_ *Service) $_($_ context.Context, $_ *$_) ($_, error) {
$$$
}'
Length of output: 1335
Script:
#!/bin/bash
# Get the full Query method implementation
rg -g 'service.go' -B 2 -A 20 'func.*Query.*QueryRequest.*QueryResponse'
# Also check the QueryRequest definition
rg -g '*.proto' 'message QueryRequest'
Length of output: 3337
Script:
#!/bin/bash
# Get the QueryRequest message definition
rg -g '*.proto' -A 5 'message QueryRequest'
# Also check the appmanager implementation since it handles version
rg -g 'appmanager.go' -A 10 'func.*Query.*version.*uint64'
Length of output: 1267
server/v2/store/server.go (3)
26-27: LGTM: Clean struct field organization
The field ordering follows Go conventions, and the use of storev2.RootStore provides a more specific and appropriate interface for the storage requirements.
50-50:
Breaking change: Stop method now performs cleanup
The Stop method now calls store.Close() instead of returning nil. This is a breaking change that could affect error handling in calling code.
30-38: Verify the impact of the RootStore interface change
The constructor changes look good, with proper error handling and clean initialization. However, we should verify all callers have been updated to provide a RootStore implementation.
✅ Verification successful
Let me verify the implementation of mockStore and simApp.Store() to ensure they implement the RootStore interface.
Let me check the SimApp implementation to ensure it properly provides the RootStore.
Let me check the store field in SimApp v2 to complete the verification.
Let me try a different approach to check the SimApp v2 structure.
RootStore interface change has been properly implemented
The verification confirms that all callers have been correctly updated:
mockStorein tests embedsstorev2.RootStoreSimApp[T]in v2 has astorefield of typestore.RootStoreand provides it through theStore()method- All callers (
server_test.goandcommands.go) use the correct implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all New function calls to ensure they're updated
rg -A 2 "store\.New\["
Length of output: 406
Script:
#!/bin/bash
# Check mockStore implementation
ast-grep --pattern 'type mockStore struct {
$$$
}'
# Check simApp.Store() implementation
ast-grep --pattern 'func ($_ *SimApp) Store() $_'
Length of output: 235
Script:
#!/bin/bash
# Check SimApp struct definition and its store field
ast-grep --pattern 'type SimApp struct {
$$$
}'
# Look for Store method implementation in simdv2
rg -A 3 'func.*Store\(\).*RootStore' simapp/v2/
Length of output: 3549
Script:
#!/bin/bash
# Check SimApp v2 struct definition
ast-grep --pattern 'type SimApp[T any] struct {
$$$
}'
Length of output: 58
Script:
#!/bin/bash
# Search for SimApp struct definition in v2 directory
rg -A 10 "type SimApp\[.*\] struct" simapp/v2/
# Also check the store field initialization
rg "store:" simapp/v2/
Length of output: 650
server/v2/server_test.go (1)
23-29: 🛠️ Refactor suggestion
Implement all RootStore interface methods in mockStore
The current mock implementation only implements the Close() method. To prevent potential runtime panics during testing, it's recommended to implement all methods from the storev2.RootStore interface, even if they're not used in current tests. This ensures the mock is complete and future-proof.
server/v2/go.mod (1)
45-45: LGTM! Moving schema to indirect dependencies.
Moving cosmossdk.io/schema to indirect dependencies aligns with the PR objective of removing serverv2.AppI. This change correctly reflects that the schema package is no longer directly imported but is still required by other dependencies.
Let's verify there are no direct imports of the schema package:
server/v2/api/grpc/service_test.go (2)
139-139: LGTM! Clean refactor of v2Service instantiation.
The changes correctly reflect the removal of serverv2.AppI interface, replacing it with direct function references. This simplifies the dependency structure while maintaining the same functionality.
Also applies to: 195-195
Line range hint 1-224: Verify test coverage for all error scenarios.
The test suite is well-structured with good coverage of success and error cases. However, let's verify that all error scenarios are covered after the interface removal.
server/v2/api/grpc/server.go (2)
51-51: LGTM: Good refactoring to function type
The change from interface type to function type for queryable parameter simplifies the API while maintaining the same functionality. This is a good example of following Go's principle of accepting interfaces and returning structs.
75-75: LGTM: Consistent service registration
The service registration is correctly updated to match the new function type parameter.
simapp/v2/simdv2/cmd/commands.go (1)
86-86: LGTM! Good interface segregation.
The change from passing the entire simApp to just simApp.Query is a good refactoring that follows the Interface Segregation Principle. It simplifies the API by providing only the required functionality (the query handler) instead of exposing the entire application interface.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
server/v2/store/server.go (1)
30-38: Consider adding store parameter validation.While the initialization looks good, we should validate that the store parameter is not nil to prevent potential panics during runtime operations.
func New[T transaction.Tx](store storev2.RootStore, cfg server.ConfigMap) (*Server[T], error) { + if store == nil { + return nil, fmt.Errorf("store cannot be nil") + } config, err := UnmarshalConfig(cfg) if err != nil { return nil, errserver/v2/server_test.go (2)
23-29: Consider implementing all RootStore interface methods.The current mockStore implementation only provides the Close() method. While this might be sufficient for the current test cases, it could lead to panics if other RootStore interface methods are called. Consider implementing all interface methods or using a mock generation tool like gomock for more robust testing.
Example implementation:
type mockStore struct { storev2.RootStore // Add fields to track method calls for verification closeWasCalled bool } func (m *mockStore) Close() error { m.closeWasCalled = true return nil } // Implement other RootStore methods with tracking...
Line range hint
89-97: Improve test reliability and cleanup.The current test implementation has two potential issues:
- Using a fixed 5-second delay for cancellation makes the test fragile
- Missing proper cleanup in defer statements
Consider this approach:
func TestServer(t *testing.T) { + cleanup := func() { + // Cleanup resources + os.RemoveAll(configPath) + } + defer cleanup() + // ... existing setup ... - go func() { - // wait 5sec and cancel context - <-time.After(5 * time.Second) + done := make(chan struct{}) + go func() { + // Signal when server is ready + done <- struct{}{} cancelFn() - - err = server.Stop(ctx) - require.NoError(t, err) }() + + select { + case <-done: + // Server started successfully + case <-time.After(10 * time.Second): + t.Fatal("server failed to start within timeout") + } + + err = server.Stop(ctx) + require.NoError(t, err)server/v2/go.mod (1)
Line range hint
3-3: Invalid Go version specified.The Go version
1.23specified in thego.modfile is not yet released. The latest stable version is Go 1.22.Apply this diff to update to the latest stable version:
-go 1.23 +go 1.22server/v2/api/grpc/server.go (2)
51-51: LGTM! Consider adding a doc comment.The change from interface to function type is a good simplification that aligns with Go's design principles. Consider adding a doc comment to describe the expected behavior of the queryable function.
Add this comment above the queryable parameter:
+// queryable is a function that handles queries at a specific version queryable func(ctx context.Context, version uint64, msg transaction.Msg) (transaction.Msg, error),Also applies to: 75-75
100-102: LGTM! Consider improving error handling.The implementation correctly uses the new function type and maintains proper gRPC streaming semantics. However, the error handling could be more specific.
Consider wrapping the error with gRPC status codes for better client-side handling:
resp, err := queryable(ctx, height, req) if err != nil { - return err + return status.Error(codes.Internal, fmt.Sprintf("failed to handle query: %v", err)) } err = stream.SendMsg(resp) if err != nil { - return err + return status.Error(codes.Internal, fmt.Sprintf("failed to send response: %v", err)) }Also applies to: 150-154
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (9)
server/v2/api/grpc/server.go(4 hunks)server/v2/api/grpc/service.go(2 hunks)server/v2/api/grpc/service_test.go(2 hunks)server/v2/go.mod(1 hunks)server/v2/server_test.go(2 hunks)server/v2/store/server.go(2 hunks)server/v2/types.go(0 hunks)simapp/v2/app_di.go(0 hunks)simapp/v2/simdv2/cmd/commands.go(1 hunks)
💤 Files with no reviewable changes (2)
- server/v2/types.go
- simapp/v2/app_di.go
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/api/grpc/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpc/service.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpc/service_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
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"
server/v2/store/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.
🔇 Additional comments (9)
server/v2/api/grpc/service.go (2)
18-18: LGTM: Good simplification of the interface to a function type
This change aligns well with Go's philosophy of small interfaces and the common pattern of using function types when an interface has only one method. It makes the code more straightforward and reduces indirection.
40-40: Consider making the version parameter configurable
The hardcoded version 0 in s.queryable(ctx, 0, protoMsg) might need to be made configurable or explained. If this is intentional, consider adding a comment explaining why it's always 0.
Let's check if this version parameter is used meaningfully elsewhere:
✅ Verification successful
Let me search for the specific usage of this version parameter in the queryable context:
Let me try a broader search to understand the queryable context:
Let me check how this version parameter is used in the Query implementation:
Let me check one more thing about the version parameter in the actual query implementation:
The hardcoded version 0 is intentional for current state queries
The version parameter in queryable() is used to specify which version of the state to query. A version of 0 indicates that the query should be executed against the current state, while non-zero values are used for historical state queries. This is evident from the codebase where:
- In
server/v2/appmanager/appmanager.go, version 0 is explicitly checked:
// if version is provided attempt to do a height query.
if version != 0 {
queryState, err := a.db.StateAt(version)- The gRPC service implementation in
server/v2/api/grpc/service.gointentionally uses 0 for regular queries against the current state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of the version parameter in query contexts
rg -A 3 "func.*\bversion\b.*uint64.*\)"
Length of output: 35787
Script:
#!/bin/bash
# Search for queryable function implementation and its version parameter usage
rg -A 5 "func.*queryable.*version.*uint64.*\)" server/
Length of output: 56
Script:
#!/bin/bash
# Search for the queryable function implementation
rg -A 10 "queryable.*context\.Context.*transaction\.Msg" server/
Length of output: 2426
Script:
#!/bin/bash
# Search for the Query implementation to see how version is used
rg -B 5 -A 15 "func.*Query.*context\.Context.*QueryRequest.*QueryResponse" server/
Length of output: 9907
Script:
#!/bin/bash
# Look for the actual implementation where version 0 is used
rg -B 10 -A 5 "queryable\(ctx, 0, protoMsg\)" server/
Length of output: 1062
server/v2/store/server.go (2)
26-27: LGTM! Field declarations follow style guidelines.
The struct fields are well-organized and properly documented.
50-50: Consider utilizing the context parameter for graceful shutdown.
The Stop method receives a context but doesn't use it. Consider passing the context to the store's Close method if it supports context-aware shutdown, which would allow for timeout and cancellation handling.
server/v2/go.mod (1)
45-45: LGTM! Moving schema to indirect dependencies.
The change to mark cosmossdk.io/schema as an indirect dependency aligns with the PR's objective of removing serverv2.AppI. This indicates that the schema package is now only required by other dependencies rather than being directly used.
server/v2/api/grpc/service_test.go (3)
139-139: LGTM: Clean transition to function-based approach
The initialization change maintains test coverage while aligning with the PR's objective of removing serverv2.AppI. The explicit use of mockApp.QueryHandlers() and mockApp.Query makes the dependencies clearer.
195-195: LGTM: Consistent implementation of function-based approach
The change maintains consistency with the TestQuery implementation and properly tests the ListQueryHandlers functionality.
Line range hint 139-195: Test coverage looks comprehensive
The test suite maintains excellent coverage with:
- Proper error case handling
- Comprehensive mock assertions
- Clear test structure
- Consistent implementation across test functions
The transition from interface-based to function-based approach is well tested.
simapp/v2/simdv2/cmd/commands.go (1)
86-86: LGTM! Good architectural improvement.
The change from passing simApp to simApp.Query is a positive architectural improvement that:
- Reduces coupling by passing only the required query functionality
- Makes the code more explicit about its dependencies
- Aligns with the interface segregation principle
Let's verify that all necessary query functionality remains accessible:
✅ Verification successful
Let me gather more specific information about the Query implementation and its usage in the new context.
Let me try one more search to understand the Query implementation and grpc server structure better.
Let me try one more search to understand the grpc server initialization and QueryHandlers usage.
Based on the search results, I can now provide a final response about the change to use simApp.Query instead of passing the entire simApp.
✅ The change is architecturally sound and properly implemented
The modification to pass simApp.Query instead of simApp to grpc.New[T] is correct because:
- The grpc server initialization explicitly requires the Query function along with QueryHandlers
- The change aligns with the principle of passing only what's needed rather than the entire app interface
- The QueryHandlers are still properly passed via
simApp.QueryHandlers() - The implementation maintains all necessary query functionality while reducing coupling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Query method provides all necessary functionality
# previously available through the app interface
# Search for query-related method calls on the app interface
rg -A 3 '\bQuery\b.*interface'
# Search for actual query handler implementations
ast-grep --pattern 'func ($app *$_) Query($_ $_) $_'
Length of output: 3240
Script:
#!/bin/bash
# Check the implementation and usage of Query in the specific app context
# Look for Query method implementation in the app
ast-grep --pattern 'func ($app *SimApp) Query($_ $_) $_'
# Look for references to Query in the grpc package
rg -A 3 'New\[.*\].*Query'
# Check the grpc server implementation
rg -A 5 'type Server\[T\]'
Length of output: 411
Script:
#!/bin/bash
# Look for the Query method implementation and grpc server structure
# Search for Query method in the app package
rg -A 5 'func.*Query.*context.Context'
# Check the grpc server implementation
rg -A 5 'func New\[T\]'
# Look for QueryHandlers implementation
ast-grep --pattern 'func ($app *$_) QueryHandlers() $_'
Length of output: 481215
Script:
#!/bin/bash
# Look for grpc server implementation and QueryHandlers usage
rg -A 5 'type Server\[T\]'
# Look for grpc.New implementation
rg -A 10 'func New\[T\].*grpc'
# Look for QueryHandlers usage
rg -A 3 'QueryHandlers\(\)'
Length of output: 2229
| grpcServer, err := grpc.New[transaction.Tx](logger, &mockInterfaceRegistry{}, map[string]appmodulev2.Handler{}, nil, cfg) | ||
| require.NoError(t, err) | ||
|
|
||
| storeServer, err := store.New[transaction.Tx](app.Store(), cfg) | ||
| storeServer, err := store.New[transaction.Tx](&mockStore{}, cfg) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
The current test implementation could be improved in several ways:
- Add negative test cases (e.g., invalid configurations)
- Verify server behavior and interactions
- Assert that mockStore is used correctly
Consider adding these test cases:
func TestServer_InvalidConfig(t *testing.T) {
// Test server initialization with invalid config
}
func TestServer_StoreInteractions(t *testing.T) {
// Test and verify store interactions
}
|
adding backport label for simapp changes |
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 3014713) # Conflicts: # server/v2/api/grpc/server.go # server/v2/api/grpc/service.go # server/v2/api/grpc/service_test.go # server/v2/go.mod # server/v2/server_test.go # server/v2/store/server.go
…22454) Co-authored-by: Julien Robert <[email protected]>
cosmos/cosmos-sdk#22446 introduced at least two bugs around closing database resources that have not yet been fixed as of the most recent commit to main; but the SDK team is aware of the issue.
Description
Realized we still haven't deleted serverv2.AppI (follow up of the Init removal)
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
Refactor
Chores