chore: cleanup core/app #21368
chore: cleanup core/app #21368
Conversation
WalkthroughWalkthroughThe recent changes involve a significant restructuring of the codebase, primarily transitioning from an Changes
Sequence Diagram(s)sequenceDiagram
participant A as App
participant S as Server
participant T as Tests
A->>S: Send BlockRequest
S->>S: Process Block
S-->>A: Return BlockResponse
A->>T: Validate response
T-->>A: Log results
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 as PR comments)
Additionally, you can add Documentation and Community
|
github-actions
bot
added
C:Cosmovisor
![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, codebase verification and nitpick comments (22)
server/v2/stf/go.mod (1)
Testify is still in use across test files. The removal of
github.com/stretchr/testifyfromgo.modwill affect numerous test files that currently depend on it for assertions and other functionalities. Please ensure these test files are updated to remove or replace thetestifydependency.
- Test files still importing
testify:
x/protocolpool/types/types_test.gox/upgrade/types/storeloader_test.gox/upgrade/types/plan_test.go- ... (and many more as listed in the output)
Analysis chain
Line range hint
5-5: Verify the impact of removingtestify.The removal of
github.com/stretchr/testifymay affect test assertions and mocking. Ensure that the tests have been updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test files for any usage of `testify`. # Test: Search for any remaining usage of `testify`. Expect: No occurrences. rg --type go 'testify' --glob '!go.mod'Length of output: 51043
server/v2/types.go (1)
7-7: Inconsistent Transition fromcoreapptoserverThe transition from
coreapp.InterfaceRegistrytoserver.InterfaceRegistryis incomplete. Whileserver.InterfaceRegistryis used in several files,coreapp.InterfaceRegistryis still present insimapp/v2/app_di.go. Please review and update the code to ensure a consistent transition across the codebase.
simapp/v2/app_di.go: Containscoreapp.InterfaceRegistryserver/v2/types.go,server/v2/server_test.go,server/v2/api/grpc/codec.go: Useserver.InterfaceRegistryAnalysis chain
Verify the transition from
coreapptoserver.The import of
InterfaceRegistryhas shifted fromcoreapptoserver. Ensure that this change is reflected throughout the codebase and does not introduce inconsistencies.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the transition from `coreapp` to `server`. # Test: Search for any remaining usage of `coreapp.InterfaceRegistry`. Expect: No occurrences. rg --type go 'coreapp.InterfaceRegistry'Length of output: 130
Script:
#!/bin/bash # Search for occurrences of `server.InterfaceRegistry` to verify if the transition is partially implemented. rg --type go 'server.InterfaceRegistry'Length of output: 376
server/v2/appmanager/types.go (1)
6-6: Incomplete Refactoring fromappmanagertoserver.The refactoring is incomplete as there are still references to
appmanager.BlockResponseandappmanager.TxResultin the codebase. Please review and update these references to ensure consistency with the newserverpackage.
server/v2/cometbft/utils.go: References toappmanager.BlockResponseandappmanager.TxResult.server/v2/cometbft/handlers/defaults.go: Reference toappmanager.TxResult.Analysis chain
Verify the refactoring from
appmanagertoserver.The types used in
StateTransitionFunctionmethods have been updated. Ensure that all related code reflects this change and functions correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the refactoring from `appmanager` to `server`. # Test: Search for any remaining usage of `appmanager.BlockRequest`, `appmanager.BlockResponse`, and `appmanager.TxResult`. Expect: No occurrences. rg --type go 'appmanager\.(BlockRequest|BlockResponse|TxResult)'Length of output: 503
server/v2/stf/stf_router_test.go (10)
23-24: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging. Consider including the test case name or scenario in the error message.
- t.Errorf("expected message: %v, got: %v", expectedMsg, gotMsg) + t.Errorf("TestRouter: expected message: %v, got: %v", expectedMsg, gotMsg)
32-33: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("unexpected error: %v", err) + t.Errorf("TestRouter: unexpected error: %v", err)
46-47: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Error("expected error, got nil") + t.Error("TestRouter: expected error, got nil")
53-54: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("unexpected error: %v", err) + t.Errorf("TestRouter: unexpected error: %v", err)
56-57: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("expected response: %v, got: %v", expectedResp, gotResp) + t.Errorf("TestRouter: expected response: %v, got: %v", expectedResp, gotResp)
64-65: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("unexpected error: %v", err) + t.Errorf("TestRouter: unexpected error: %v", err)
67-68: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("expected response: %v, got: %v", expectedResp, gotResp) + t.Errorf("TestRouter: expected response: %v, got: %v", expectedResp, gotResp)
116-117: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Error("expected error, got nil") + t.Error("TestMerge: expected error, got nil")
120-121: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("unexpected error: %v", err) + t.Errorf("TestMerge: unexpected error: %v", err)
123-124: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("expected merged message: %v, got: %v", tt.expected, tt.dst) + t.Errorf("TestMerge: expected merged message: %v, got: %v", tt.expected, tt.dst)server/v2/stf/branch/branch_test.go (9)
13-15: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Error setting value: %v", err) + t.Errorf("TestBranch: Error setting value: %v", err)
20-21: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Error getting value: %v", err) + t.Errorf("TestBranch: Error getting value: %v", err)
24-25: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Expected nil value, got: %v", value) + t.Errorf("TestBranch: Expected nil value, got: %v", value)
28-29: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Expected value: %s, got: %s", wantValue, value) + t.Errorf("TestBranch: Expected value: %s, got: %s", wantValue, value)
36-37: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Error deleting value: %v", err) + t.Errorf("TestBranch: Error deleting value: %v", err)
54-55: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Error creating iterator: %v", err) + t.Errorf("TestBranch: Error creating iterator: %v", err)
60-61: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Expected iterator to be valid") + t.Errorf("TestBranch: Expected iterator to be valid")
65-66: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Expected key: %s, got: %s", wantKey, gotKey) + t.Errorf("TestBranch: Expected key: %s, got: %s", wantKey, gotKey)
68-69: Use consistent error messages.Ensure that error messages are consistent and provide enough context for debugging.
- t.Errorf("Expected value: %s, got: %s", wantValue, gotValue) + t.Errorf("TestBranch: Expected value: %s, got: %s", wantValue, gotValue)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
server/v2/stf/go.sumis excluded by!**/*.sum
Files selected for processing (17)
- core/server/app.go (4 hunks)
- core/server/codec.go (1 hunks)
- core/server/doc.go (1 hunks)
- runtime/module.go (2 hunks)
- server/v2/api/grpc/codec.go (2 hunks)
- server/v2/appmanager/appmanager.go (4 hunks)
- server/v2/appmanager/types.go (2 hunks)
- server/v2/server_test.go (2 hunks)
- server/v2/stf/branch/branch_test.go (3 hunks)
- server/v2/stf/core_branch_service_test.go (4 hunks)
- server/v2/stf/go.mod (1 hunks)
- server/v2/stf/identity.go (1 hunks)
- server/v2/stf/stf.go (14 hunks)
- server/v2/stf/stf_router_test.go (3 hunks)
- server/v2/stf/stf_test.go (10 hunks)
- server/v2/types.go (2 hunks)
- tools/cosmovisor/cmd/cosmovisor/version_test.go (1 hunks)
Files skipped from review due to trivial changes (3)
- core/server/doc.go
- server/v2/stf/identity.go
- tools/cosmovisor/cmd/cosmovisor/version_test.go
Additional context used
Path-based instructions (13)
core/server/codec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/appmanager/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/server/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/api/grpc/codec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/core_branch_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/stf/stf_router_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/stf/branch/branch_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/appmanager/appmanager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/stf_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/stf/stf.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (35)
server/v2/stf/go.mod (1)
Line range hint
6-6: Ensure compatibility with the newgogoprotoversion.The replace directive for
github.com/cosmos/gogoprotohas been updated. Verify that this version is compatible with the rest of the codebase.core/server/codec.go (2)
14-18: Consider the implications of type aliasing forAnyResolver.The type aliasing of
AnyResolvermight simplify imports but could also obscure the origin of the interface. Ensure this does not lead to confusion in the codebase.Verification successful
Type aliasing for
AnyResolveris consistent and clear.The aliasing of
AnyResolveris used consistently across the codebase, primarily in conjunction withjsonpb. This does not obscure the origin of the interface, as its usage is clear and well-documented. The alias simplifies imports without causing confusion.
- Usage is consistent in files such as
server/v2/api/grpcgateway/server.goandcodec/unknownproto/unknown_fields.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `AnyResolver`. # Test: Search for usage of `AnyResolver`. Expect: Clear and consistent usage. rg --type go 'AnyResolver'Length of output: 3229
7-12: Ensure comprehensive implementation ofInterfaceRegistry.The
InterfaceRegistryinterface is defined with methods for listing implementations and interfaces. Ensure that implementations of this interface are thorough and consistent.Verification successful
Comprehensive Implementation Verified for
InterfaceRegistry.The
interfaceRegistrystruct implements bothListImplementationsandListAllInterfacesmethods, ensuring a thorough and consistent implementation of theInterfaceRegistryinterface.
ListImplementationsmethod is implemented atcodec/types/interface_registry.go:214.ListAllInterfacesmethod is implemented atcodec/types/interface_registry.go:205.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify implementations of `InterfaceRegistry`. # Test: Search for implementations of `InterfaceRegistry`. Expect: Consistent method implementations. ast-grep --lang go --pattern 'type $_ struct { $$$ }' --pattern 'func ($_ *$_) ListImplementations($_) []string { $$$ }'Length of output: 289
Script:
#!/bin/bash # Find structs that implement the InterfaceRegistry interface ast-grep --lang go --pattern 'type $_ struct { $$$ }' --pattern '$_ InterfaceRegistry' # Find ListImplementations method implementations ast-grep --lang go --pattern 'func ($_ *$_) ListImplementations($_) []string { $$$ }'Length of output: 1348
Script:
#!/bin/bash # Find ListAllInterfaces method implementations ast-grep --lang go --pattern 'func ($_ *$_) ListAllInterfaces() []string { $$$ }'Length of output: 624
server/v2/types.go (1)
17-17: Check for seamless integration ofserver.InterfaceRegistry.The return type of
InterfaceRegistryhas been updated. Verify that this change is compatible with existing implementations.Verification successful
Compatibility of
server.InterfaceRegistryChange VerifiedThe return type change for
InterfaceRegistryinserver.InterfaceRegistryappears to be compatible with existing implementations and usages in the codebase. No issues were found in the following locations:
- Mock implementation in
server/v2/server_test.go.- Usage in
server/v2/api/grpc/codec.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility with `server.InterfaceRegistry`. # Test: Search for implementations of `AppI` and check for compatibility. Expect: No issues. ast-grep --lang go --pattern 'type $_ struct { $$$ }' --pattern 'func ($_ *$_) InterfaceRegistry() server.InterfaceRegistry { $$$ }'Length of output: 301
Script:
#!/bin/bash # Search for implementations of the InterfaceRegistry method returning server.InterfaceRegistry ast-grep --lang go --pattern 'func ($_ *$_) InterfaceRegistry() server.InterfaceRegistry { $$$ }' # Search for all usages of server.InterfaceRegistry in the codebase rg 'server.InterfaceRegistry' -A 3Length of output: 1078
server/v2/appmanager/types.go (3)
16-18: Ensure consistency inDeliverBlockmethod changes.The
DeliverBlockmethod now usesserver.BlockRequestandserver.BlockResponse. Verify that these changes are consistent with the rest of the codebase.Verification successful
Consistency in
DeliverBlockMethod Changes VerifiedThe changes to the
DeliverBlockmethod, involvingserver.BlockRequestandserver.BlockResponse, are consistently integrated across the codebase. These types are used in both implementation and test files, confirming their proper adoption.
- Files with Consistent Usage:
server/v2/stf/stf.goserver/v2/stf/stf_test.goserver/v2/appmanager/appmanager.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency in `DeliverBlock` method changes. # Test: Search for implementations of `DeliverBlock` and check for consistency. Expect: Consistent usage of `server` types. ast-grep --lang go --pattern 'func ($_ *$_) DeliverBlock(ctx context.Context, block *server.BlockRequest[$_], state store.ReaderMap) (blockResult *server.BlockResponse, newState store.WriterMap, err error) { $$$ }'Length of output: 216
Script:
#!/bin/bash # Description: Search for all usages of `server.BlockRequest` and `server.BlockResponse` in the codebase. # Search for usage of `server.BlockRequest` rg 'server\.BlockRequest' -A 3 # Search for usage of `server.BlockResponse` rg 'server\.BlockResponse' -A 3Length of output: 4798
34-34: ConfirmSimulatemethod changes.The
Simulatemethod now returnsserver.TxResult. Verify that this change is consistent across all implementations.
26-26: ValidateValidateTxmethod changes.The
ValidateTxmethod now returnsserver.TxResult. Ensure that this change is reflected in all implementations.core/server/app.go (4)
Line range hint
12-23: Review ofBlockRequeststruct.The
BlockRequeststruct is well-defined and uses a generic typeTfor transactions, which enhances flexibility. Ensure that the use of generics is consistent with the rest of the codebase.
Line range hint
25-33: Review ofBlockResponsestruct.The
BlockResponsestruct is clear and concise, capturing essential elements of a block response. The use ofappmodulev2.ValidatorUpdateandevent.Eventis appropriate. Ensure these dependencies are correctly imported and utilized.
Line range hint
35-48: Review ofTxResultstruct.The
TxResultstruct provides a comprehensive representation of a transaction's execution result. The inclusion of fields likeEvents,Error, andGasUsedis beneficial for detailed transaction analysis.
1-1: Package name change toserver.The package name has been changed from
apptoserver. This reflects the restructuring of the codebase to align with the new architecture. Ensure that all references to this package are updated accordingly.Verification successful
Package name change verified successfully.
The package name change from
apptoserverhas been correctly applied throughout the codebase. No occurrences ofpackage appwere found, confirming that all references have been updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to the package name have been updated. # Test: Search for the old package name usage. Expect: No occurrences of the old package name. rg --type go -A 5 $'package app'Length of output: 11155
server/v2/server_test.go (2)
14-14: Import statement update tocoreserver.The import statement has been updated from
coreapptocoreserver. This aligns with the architectural changes. Ensure that all references toInterfaceRegistryare consistent with this change.
45-47: Method signature update inmockApp.The
InterfaceRegistrymethod now returnscoreserver.InterfaceRegistry, reflecting the updated architecture. This change is consistent with the import updates and maintains the intended functionality.server/v2/api/grpc/codec.go (3)
16-21: Introduction ofprotocdcinterface.The
protocdcinterface is well-defined, providing methods for marshaling, unmarshaling, and retrieving the codec name. This enhances flexibility and clarity in codec operations.
24-29: Update toprotoCodecstruct.The
protoCodecstruct now usesserver.InterfaceRegistry, aligning with the new architecture. This change supports better modularity and integration with server functionalities.
73-75: Update togrpcProtoCodecstruct.The
grpcProtoCodecstruct now uses theprotocdcinterface, standardizing the codec behavior across different message types. This change is consistent with the architectural updates.server/v2/stf/core_branch_service_test.go (3)
63-67: Improved error handling in test cases.The test now uses conditional statements for error checking instead of
requireassertions. This change improves clarity in error reporting and maintains the same logical checks.
78-83: Consistent error handling in test cases.The test case consistently uses conditional statements to check for expected errors and gas usage, enhancing readability and error reporting.
95-104: Precision check for gas limit.The test now includes a conditional check for gas limit precision, ensuring correctness in gas usage validation.
server/v2/appmanager/appmanager.go (4)
119-120: Verify consistency with newservertypes.Ensure that all usages of
BlockRequestandBlockResponsealign with their new definitions in theserverpackage.
150-153: Verify consistency with newservertypes.Ensure that all usages of
TxResultalign with their new definitions in theserverpackage.
141-144: Verify consistency with newservertypes.Ensure that all usages of
TxResultalign with their new definitions in theserverpackage.
42-45: Verify consistency with newservertypes.Ensure that all usages of
BlockRequestandBlockResponsealign with their new definitions in theserverpackage.Verification successful
Verification Successful: Consistent Usage of
serverTypesThe transition to using
server.BlockRequest[T]andserver.BlockResponseis consistent across the codebase. No inconsistencies were found in their usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `BlockRequest` and `BlockResponse` in the server package. # Test: Search for `BlockRequest` and `BlockResponse` usages. Expect: Consistent usage with the new definitions. rg --type go 'BlockRequest|BlockResponse'Length of output: 123379
runtime/module.go (1)
291-291: Verify consistency with newservertypes.Ensure that all usages of
VersionModifieralign with their new definitions in theserverpackage.server/v2/stf/stf_test.go (7)
100-108: Improved error handling in test case.The use of
t.Errorfwith detailed error messages enhances the clarity of test failures.
114-137: Enhanced transaction result validation.The test case now includes explicit checks for transaction results, improving test reliability.
153-176: Robust handling of out-of-gas scenario.The test case effectively checks for state integrity and expected errors when out of gas.
186-198: Accurate error message verification.The test case effectively verifies that the correct error messages are returned on transaction failure.
211-223: Effective post-transaction failure handling.The test case accurately detects and reports errors occurring during post-transaction execution.
237-249: Comprehensive failure scenario testing.The test case effectively checks for errors in both transaction and post-transaction phases.
261-273: Accurate validation failure detection.The test case effectively verifies that validation errors are correctly reported.
server/v2/stf/stf.go (4)
Line range hint
85-155: Aligned with new architecture.The
DeliverBlockmethod now usesservertypes, improving modularity and consistency.
Line range hint
172-203: Improved transaction processing.The
deliverTxmethod now returnsserver.TxResult, enhancing error handling and processing.
410-414: Enhanced simulation accuracy.The
Simulatemethod now returnsserver.TxResult, improving simulation consistency.
428-431: Improved validation accuracy.The
ValidateTxmethod now returnsserver.TxResult, enhancing validation consistency.
There was a problem hiding this comment.
LGTM! Nice improvement.
The only point I have is to better define the split between core/server/InterfaceRegistry and core/registry/InterfaceRegistrar could be explained. Especially because the actual implementation in the SDK called InterfaceRegistry is both (
Line 24 in e7ebe8e
|
The build is failing because there are still usages of |
There was a problem hiding this comment.
This is an improvement but it would be nice to have docs on fields too. For instance, TxResult has so many things in it and some see duplicated:
RespandDataLogandInfoError,CodeandCodespace
This obviously is directly pulled from ABCI. But do we really want to replicate all of this verbatim in core & server/v2? At least we should have docs for clarification
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- core/server/app.go (3 hunks)
- core/server/codec.go (1 hunks)
- server/v2/stf/stf.go (14 hunks)
Files skipped from review as they are similar to previous changes (2)
- core/server/codec.go
- server/v2/stf/stf.go
Additional context used
Path-based instructions (1)
core/server/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
core/server/app.go (5)
Line range hint
1-10: Package renaming and imports look good.The renaming from
apptoserveraligns with the PR objectives. The imports are appropriate and necessary for the defined structures.
Line range hint
12-23:BlockRequeststruct is well-defined.The use of generics enhances flexibility, and the comments clarify its purpose. The struct is appropriately structured for handling block-related requests.
25-31:BlockResponsestruct is comprehensive and clear.The struct effectively captures the necessary components for block responses, with clear comments explaining its role.
34-39:TxResultstruct and documentation are appropriate.The struct is well-documented, and its fields are suitable for capturing the results of transaction execution.
Line range hint
44-51:VersionModifierinterface is necessary and well-defined.The interface aligns with the architectural goals of managing versioning and consensus parameters.
core/server/app.go
Outdated
| Events []event.Event // Events produced by the transaction. | ||
| Resp []transaction.Msg // Response messages produced by the transaction. | ||
| Error error // Error produced by the transaction. | ||
| Code uint32 // Code produced by the transaction. |
There was a problem hiding this comment.
Can we have these before the field so that they're doc comments? Also this doesn't really explain what these mean. Copilot can mostly write docs for all fields and then we can expand where needed
There was a problem hiding this comment.
i went through the useage in comets spec and how we use it in baseapp and removed fields that we previously populated with error information. Ill double check with users if thats okay
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (11)
- core/server/app.go (3 hunks)
- core/server/doc.go (1 hunks)
- runtime/v2/module.go (2 hunks)
- server/v2/cometbft/abci.go (5 hunks)
- server/v2/cometbft/handlers/defaults.go (1 hunks)
- server/v2/cometbft/streaming.go (3 hunks)
- server/v2/cometbft/utils.go (4 hunks)
- simapp/v2/app_di.go (2 hunks)
- simapp/v2/app_test.go (3 hunks)
- x/upgrade/depinject.go (2 hunks)
- x/upgrade/keeper/keeper.go (3 hunks)
Files skipped from review due to trivial changes (1)
- core/server/doc.go
Additional context used
Path-based instructions (10)
core/server/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/streaming.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/upgrade/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/cometbft/handlers/defaults.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/upgrade/keeper/keeper.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.
Additional comments not posted (19)
core/server/app.go (3)
Line range hint
12-23: Verify the correctness ofAppHash.The
AppHashfield was questioned in previous reviews. Ensure its usage and definition are correct.
25-31: LGTM!The
BlockResponsestructure is well-defined and aligns with its intended purpose.
34-49: LGTM!The
TxResultstructure is well-documented and correctly implemented.server/v2/cometbft/streaming.go (1)
Line range hint
13-35: Verify the impact of changes on functionality.The switch to
server.TxResultand the simplification ofExecTxResultmay affect functionality. Ensure that these changes align with the intended behavior.x/upgrade/depinject.go (1)
43-43: Verify the correctness ofAppVersionModifiertype change.The change to
coreserver.VersionModifiershould be verified to ensure it aligns with the intended architectural decisions.simapp/v2/app_test.go (3)
16-16: Import statement update aligns with restructuring.The import statement for
serverreplacesapp2, aligning with the restructuring efforts. Ensure that all dependencies are correctly updated.
126-126: Ensure consistency inMoveNextBlock.The usage of
server.BlockRequestinMoveNextBlockis consistent with the changes inNewTestApp. Ensure that this change does not affect the logic of the test.
83-83: Correct usage ofserver.BlockRequest.The change from
app2.BlockRequesttoserver.BlockRequestis consistent with the import update. Verify that theserver.BlockRequestprovides the same functionality as before.Run the following script to verify the
BlockRequestimplementation:Verification successful
Verification successful for
server.BlockRequest.The
BlockRequesttype is defined in theserverpackage with fields that are consistent with its likely usage in blockchain-related operations. The change fromapp2.BlockRequesttoserver.BlockRequestis valid.
- Location:
core/server/app.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `BlockRequest` in the server package. ast-grep --lang go --pattern $'package server { $$$ type BlockRequest[$$$] struct { $$$ } $$$ }'Length of output: 608
Script:
#!/bin/bash # Description: Verify the implementation of `BlockRequest` in the server package. ast-grep --lang go --pattern $'type BlockRequest[$$$] struct { $$$ }'Length of output: 72
Script:
#!/bin/bash # Description: Search for the definition of `BlockRequest` across the entire codebase. rg 'type BlockRequest' -A 5Length of output: 271
server/v2/cometbft/handlers/defaults.go (2)
12-12: Import statement update aligns with restructuring.The import statement for
serverreplacesappmanager, aligning with the restructuring efforts. Ensure that all dependencies are correctly updated.
19-19: Updated return type inValidateTx.The return type of
ValidateTxhas been updated toserver.TxResult. Ensure that this change is compatible with the existing logic and does not introduce any errors.Run the following script to verify the
TxResultimplementation:Verification successful
Updated return type in
ValidateTxis compatible.The
TxResulttype is well-defined incore/server/app.gowith fields covering necessary transaction details. Ensure that the rest of the codebase handles these fields appropriately to maintain compatibility.
core/server/app.go: Lines 35-50Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `TxResult` in the server package. ast-grep --lang go --pattern $'package server { $$$ type TxResult struct { $$$ } $$$ }'Length of output: 590
Script:
#!/bin/bash # Description: Search for the `TxResult` type definition in the Go files. ast-grep --lang go --pattern $'type TxResult struct { $$$ }'Length of output: 1023
simapp/v2/app_di.go (2)
10-10: Import statement update aligns with restructuring.The import statement for
serverreplacescoreapp, aligning with the restructuring efforts. Ensure that all dependencies are correctly updated.
215-215: Updated return type inInterfaceRegistry.The return type of
InterfaceRegistryhas been updated toserver.InterfaceRegistry. Ensure that this change is compatible with the existing logic and does not introduce any errors.Run the following script to verify the
InterfaceRegistryimplementation:runtime/v2/module.go (1)
22-22: Verify the usage ofProvideAppVersionModifier.The function now returns a
server.VersionModifierinstead ofapp.VersionModifier. Ensure that all usages of this function are updated accordingly in the codebase.Run the following script to verify the function usage:
Also applies to: 245-245
Verification successful
Verification Successful: Function Usage Updated
The function
ProvideAppVersionModifierhas been updated to returnserver.VersionModifier, and all usages in the codebase are consistent with this change. There are no other occurrences that require updates.
runtime/module.goruntime/v2/module.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ProvideAppVersionModifier` match the new return type. # Test: Search for the function usage. Expect: Only occurrences of the new return type. rg --type go -A 5 $'ProvideAppVersionModifier'Length of output: 1256
Script:
#!/bin/bash # Search for all usages of ProvideAppVersionModifier across the codebase to ensure they match the new return type. rg --type go 'ProvideAppVersionModifier'Length of output: 515
server/v2/cometbft/utils.go (1)
23-23: Verify the impact of type changes in function signatures.The functions now use
server.BlockResponseandserver.TxResultinstead ofappmanagertypes. Ensure that all related code is updated to accommodate these changes.Run the following script to verify the impact of these changes:
Also applies to: 71-71, 102-102, 149-149
Verification successful
Type Changes Successfully Implemented
The transition to using
server.BlockResponseandserver.TxResulthas been completed without any remaining instances of the oldappmanagertypes. All related code appears to be updated accordingly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `finalizeBlockResponse`, `intoABCITxResults`, and `intoABCISimulationResponse` match the new parameter types. # Test: Search for the function usage. Expect: Only occurrences of the new parameter types. rg --type go -A 5 $'finalizeBlockResponse|intoABCITxResults|intoABCISimulationResponse'Length of output: 2584
Script:
#!/bin/bash # Search for any remaining instances of the old types `appmanager.BlockResponse` and `appmanager.TxResult` rg --type go 'appmanager.BlockResponse|appmanager.TxResult'Length of output: 61
x/upgrade/keeper/keeper.go (1)
17-17: Verify the usage ofversionModifierin theKeeperstruct.The
versionModifierfield now usesserver.VersionModifierinstead ofapp.VersionModifier. Ensure that all related code is updated to accommodate this change.Run the following script to verify the usage of
versionModifier:Also applies to: 37-37, 54-54
server/v2/cometbft/abci.go (4)
17-17: Import addition is appropriate.The addition of the
serverimport aligns with the changes in block request handling.
288-288: Logging adjustment is approved.The omission of the
Logfield from the warning message may indicate a refinement in the logging strategy.
440-440: Transition toserver.BlockRequest[T]is approved.The change reflects a shift in the source of the block request structure, likely for better modularity.
Ensure that all instances of
BlockRequest[T]are updated throughout the codebase.
267-267: Transition toserver.BlockRequest[T]is approved.The change reflects a shift in the source of the block request structure, likely for better modularity.
Ensure that all instances of
BlockRequest[T]are updated throughout the codebase.Run the following script to verify the usage of
BlockRequest[T]:Verification successful
Transition to
server.BlockRequest[T]is consistent across the codebase.All instances of
BlockRequest[T]have been updated to useserver.BlockRequest[T]. The change is uniformly applied in the following files:
server/v2/stf/stf.goserver/v2/cometbft/abci.goserver/v2/appmanager/appmanager.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instances of `BlockRequest[T]` are updated. # Test: Search for the usage of `BlockRequest[T]`. Expect: Only occurrences of `server.BlockRequest[T]`. rg --type go 'BlockRequest\[T\]'Length of output: 415
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
core/server/app.go (2)
Line range hint
12-23: Clarify the purpose of theAppHashfield.The comment for
AppHashcould be more descriptive to explain its role in the block request.Consider expanding the comment to clarify what
AppHashrepresents in the context of a block request.
34-49: Consider making theCodefield explanation more concise.The explanation for the
Codefield is informative but could be simplified for clarity.Consider rephrasing to make the explanation more concise while retaining the necessary details.
server/v2/cometbft/handlers/defaults.go (1)
19-19: Implementation ofValidateTxnot returningserver.TxResult.The
ValidateTxmethod inx/auth/ante/sigverify.gocurrently returns anerrorinstead ofserver.TxResult. This needs to be updated to align with theAppManagerinterface change.
- File:
x/auth/ante/sigverify.go- Method:
ValidateTxAnalysis chain
Interface change approved.
The
ValidateTxmethod now returningserver.TxResultaligns with the refactoring efforts.Ensure that all implementations of
AppManagerare updated to returnserver.TxResult.Run the following script to verify the implementation of
AppManager:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all implementations of `AppManager` return `server.TxResult`. # Test: Search for the implementation of `AppManager`. Expect: Only occurrences of `server.TxResult`. ast-grep --lang go --pattern $'type $_ struct { $$$ } func ($_ *$_) ValidateTx(ctx context.Context, tx $_) (server.TxResult, error) { $$$ }'Length of output: 680
Script:
#!/bin/bash # Description: Verify all implementations of `AppManager` return `server.TxResult`. # Find all implementations of the `ValidateTx` method and check for the return type. rg --multiline --multiline-dotall 'func \(\w+ \*?\w+\) ValidateTx\(.+\) \((.+), error\)' -A 5Length of output: 8471
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (11)
- core/server/app.go (3 hunks)
- core/server/doc.go (1 hunks)
- runtime/v2/module.go (2 hunks)
- server/v2/cometbft/abci.go (5 hunks)
- server/v2/cometbft/handlers/defaults.go (1 hunks)
- server/v2/cometbft/streaming.go (3 hunks)
- server/v2/cometbft/utils.go (4 hunks)
- simapp/v2/app_di.go (2 hunks)
- simapp/v2/app_test.go (3 hunks)
- x/upgrade/depinject.go (2 hunks)
- x/upgrade/keeper/keeper.go (3 hunks)
Files skipped from review due to trivial changes (1)
- core/server/doc.go
Additional context used
Path-based instructions (10)
core/server/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/streaming.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/upgrade/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/cometbft/handlers/defaults.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/upgrade/keeper/keeper.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.
Additional comments not posted (18)
core/server/app.go (2)
25-32: LGTM! TheBlockResponsestruct is well-defined.The struct aligns with the intended functionality and enhances clarity in block processing.
Line range hint
51-59: LGTM! TheVersionModifierinterface is clear and well-defined.The interface aligns with its intended purpose of managing app versioning.
server/v2/cometbft/streaming.go (1)
17-17: LGTM! ThestreamDeliverBlockChangesfunction changes are appropriate.The switch to
server.TxResultaligns with the refactor goals and maintains the function's logic.x/upgrade/depinject.go (1)
43-43: LGTM! TheModuleInputsstruct changes are appropriate.The shift to
coreserver.VersionModifieraligns with the architectural update and enhances dependency management.simapp/v2/app_test.go (3)
16-16: Import change approved.The addition of the
serverpackage aligns with the refactoring ofBlockRequestusage.
Line range hint
126-131: BlockRequest refactoring approved.The usage of
server.BlockRequestis consistent with the refactoring efforts.Ensure that all references to
BlockRequestare updated throughout the codebase.
Line range hint
83-89: BlockRequest refactoring approved.The usage of
server.BlockRequestis consistent with the refactoring efforts.Ensure that all references to
BlockRequestare updated throughout the codebase.Run the following script to verify the usage of
BlockRequest:Verification successful
BlockRequest references updated successfully.
The
BlockRequesttype has been updated toserver.BlockRequestwhere applicable. Other references in the codebase are unrelated to this change.
- The usage of
server.BlockRequestis consistent with the refactoring efforts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `BlockRequest` are updated. # Test: Search for the usage of `BlockRequest`. Expect: Only occurrences of `server.BlockRequest`. rg --type go -A 5 $'BlockRequest'Length of output: 216968
simapp/v2/app_di.go (1)
215-215: Method change approved.The
InterfaceRegistrymethod now returningserver.InterfaceRegistryaligns with the refactoring efforts.Ensure that all usages of
InterfaceRegistryare updated to handleserver.InterfaceRegistry.Run the following script to verify the usage of
InterfaceRegistry:runtime/v2/module.go (2)
22-22: Import change approved.The addition of the
serverpackage is consistent with the shift in functionality fromapptoserver.
245-246: Function signature change approved.The change in the return type of
ProvideAppVersionModifierfromapp.VersionModifiertoserver.VersionModifieraligns with the architectural shift.server/v2/cometbft/utils.go (3)
Line range hint
71-75: Parameter type change approved.The change in the parameter type of
finalizeBlockResponsefromappmanager.BlockResponsetoserver.BlockResponsealigns with the architectural shift.
102-103: Parameter type change approved.The change in the parameter type of
intoABCITxResultsfromappmanager.TxResulttoserver.TxResultis consistent with the architectural shift.
149-150: Parameter type change approved.The change in the parameter type of
intoABCISimulationResponsefromappmanager.TxResulttoserver.TxResultaligns with the architectural transition.x/upgrade/keeper/keeper.go (2)
37-37: Struct field type change approved.The change in the
versionModifierfield type fromapp.VersionModifiertoserver.VersionModifierreflects the architectural shift in version management.
54-55: Function parameter type change approved.The change in the parameter type for
vsinNewKeeperfromapp.VersionModifiertoserver.VersionModifieraligns with the architectural reorganization.server/v2/cometbft/abci.go (3)
17-17: Import change approved.The addition of the
serverpackage import aligns with the refactoring changes.
267-267: Refactor toserver.BlockRequestapproved.The transition to using
server.BlockRequestreflects the architectural restructuring and maintains the existing logic.
440-440: Refactor toserver.BlockRequestapproved.The transition to using
server.BlockRequestis consistent with the architectural restructuring and maintains the existing logic.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- server/v2/stf/stf.go (14 hunks)
- server/v2/stf/stf_router_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- server/v2/stf/stf.go
- server/v2/stf/stf_router_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- runtime/v2/module.go (2 hunks)
- server/v2/stf/stf_router_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- runtime/v2/module.go
- server/v2/stf/stf_router_test.go
|
Due to module changes here we need to partially backport this |
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
(cherry picked from commit 355f748) # Conflicts: # core/server/app.go # runtime/v2/module.go # server/v2/api/grpc/codec.go # server/v2/appmanager/appmanager.go # server/v2/appmanager/types.go # server/v2/server_test.go # server/v2/stf/branch/branch_test.go # server/v2/stf/core_branch_service_test.go # server/v2/stf/go.mod # server/v2/stf/go.sum # server/v2/stf/stf.go # server/v2/stf/stf_router_test.go # server/v2/stf/stf_test.go # server/v2/types.go
Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
ref #21176
remove query and initchain structs as they are not needed
move interfaces to where they are used
rewrite stf to not use testify
rename app to server
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
appmanagertypes toservertypes throughout the application, streamlining functionality.Chores