refactor(simdv2): allow non-comet server components #22351
Conversation
This renames NewRootCmd to NewCometBFTRootCmd, to make it clear that it uses CometBFT for the consensus layer. Callers who want to use a different consensus-providing server component can instead call NewRootCmdWithConsensusComponent, passing in a callback to be evaluated with a client context in order to produce the component. This pattern is working for Gordian-Cosmos integration. I'm not especially tied to the new function names. I'm also open to more clear naming for "consensus component" which occurs a few times.
mark-rushakoff
requested review from
kocubinski,
julienrbrt and
a team
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve significant modifications to the command structure within 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 (5)
simapp/v2/simdv2/cmd/testnet_test.go (1)
Line range hint
19-24: Consider enhancing test coverage.The current test only verifies successful command execution. Consider adding test cases for:
- Error scenarios (e.g., invalid flags)
- Verification of created files and their contents
- Different configuration combinations
Would you like me to help generate additional test cases to improve coverage?
simapp/v2/simdv2/cmd/root_test.go (2)
Line range hint
1-43: Enhance test coverage for the new functionality.The current test file only covers the CometBFT-specific functionality. To ensure robust testing, consider adding:
- A new test function
TestNewRootCmdWithConsensusComponentthat verifies:
- Custom consensus component initialization
- Error cases for invalid component configurations
- Component callback execution
- Integration tests demonstrating interoperability with non-CometBFT components
Example test structure:
func TestNewRootCmdWithConsensusComponent(t *testing.T) { tests := []struct { name string component func(client.Context) (server.Component, error) wantErr bool }{ // Add test cases here } // ... test implementation }Would you like me to help create these additional test cases?
Based on the gathered information, I can now provide a final verification response.
The review comment's concern about additional test coverage is valid but needs refinement
The code shows that
NewCometBFTRootCmdis actually a wrapper aroundNewRootCmdWithConsensusComponent(as seen insimapp/v2/simdv2/cmd/root_di.go). While the review correctly identifies the need for more test coverage, the specific suggestions need adjustment because:
NewCometBFTRootCmdis already implemented to useNewRootCmdWithConsensusComponentinternally, showing proper architectural separation- The current test validates the basic initialization path, which is essential for both CometBFT and non-CometBFT scenarios
- The test file appropriately tests the command's core functionality (initialization and home flag registration)
Recommendations for additional test coverage:
- Test initialization with different consensus component configurations using the existing
NewRootCmdWithConsensusComponent- Add verification of the generated files and configurations after initialization
- Test error scenarios when invalid consensus configurations are provided
🔗 Analysis chain
Line range hint
19-26: Additional test coverage needed for non-CometBFT components.While this test covers the basic CometBFT initialization, we should add test cases for:
- Initialization with a custom consensus component using
NewRootCmdWithConsensusComponent- Verification of the initialization results beyond just checking for errors
Let me gather more context about the codebase structure and the new functionality to provide a more accurate verification.
Let me check the specific implementation details of the root command and consensus components to make a more informed verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other test files that might cover the new functionality fd -e go -x rg -l "NewRootCmdWithConsensusComponent.*_test"Length of output: 61
Script:
#!/bin/bash # Check for the existence and implementation of NewRootCmdWithConsensusComponent ast-grep --pattern 'func NewRootCmdWithConsensusComponent' # Look for any test files in the cmd directory fd -e go ".*_test.go" cmd/ # Check for any consensus-related interfaces or types rg -l "ConsensusComponent|consensus" --type goLength of output: 6871
Script:
#!/bin/bash # Check the implementation of root command in the same directory rg -A 10 "NewCometBFTRootCmd" simapp/v2/simdv2/cmd/ # Check for any consensus-related command implementations rg -A 10 "NewRootCmd.*Consensus" server/v2/ # Look for any test setup related to consensus components rg -A 5 "TestRoot" --type goLength of output: 6311
simapp/v2/simdv2/cmd/commands.go (1)
46-46: LGTM! Consider adding documentation.The new
consensusComponentparameter effectively supports the PR's goal of allowing non-CometBFT server components. Consider adding a godoc comment to document the purpose of this parameter and provide an example of how to use it with different consensus components.func initRootCmd[T transaction.Tx]( rootCmd *cobra.Command, moduleManager *runtimev2.MM[T], + // consensusComponent allows specifying a custom consensus implementation + // Example: cometbft.New() or custom.ConsensusComponent() consensusComponent serverv2.ServerComponent[T], ) {simapp/v2/simdv2/cmd/root_di.go (1)
33-35: Consider refining the function documentation commentsThe comments for
NewCometBFTRootCmdcould be more concise. As per the Uber Go Style Guide, avoid stating the obvious or duplicating information clear from the code. Consider combining or simplifying the comments to enhance readability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- simapp/v2/simdv2/cmd/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/root_di.go (3 hunks)
- simapp/v2/simdv2/cmd/root_test.go (2 hunks)
- simapp/v2/simdv2/cmd/testnet_test.go (1 hunks)
- simapp/v2/simdv2/main.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/root_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/root_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"simapp/v2/simdv2/cmd/testnet_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"simapp/v2/simdv2/main.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (8)
simapp/v2/simdv2/main.go (1)
15-15: LGTM! Note: Breaking change.The renaming from
NewRootCmdtoNewCometBFTRootCmdaccurately reflects the use of CometBFT as the consensus layer. However, this is a breaking change that will require updates from existing users.Let's verify if the documentation needs to be updated:
simapp/v2/simdv2/cmd/testnet_test.go (1)
19-19: LGTM: Command rename aligns with PR objectives.The change from
NewRootCmdtoNewCometBFTRootCmdcorrectly reflects the explicit use of CometBFT as the consensus layer.simapp/v2/simdv2/cmd/root_test.go (1)
Line range hint
32-43: LGTM! The home flag registration test is well-structured.The test properly verifies the flag registration functionality, which is independent of the consensus component changes.
simapp/v2/simdv2/cmd/commands.go (1)
Line range hint
72-76: Verify component initialization order.The component initialization looks correct, but let's verify that the order of components (consensus → gRPC → store → telemetry → REST) doesn't introduce any dependencies or startup timing issues.
Let's check for any ordering constraints in the codebase:
✅ Verification successful
Let me gather more specific information about component dependencies and initialization order.
This will help us verify if there are any specific ordering requirements during component initialization and startup.
Based on the code analysis, I can now provide the final response:
Component initialization order is correctly implemented
The verification shows that:
- Components are started concurrently using errgroup, meaning there's no strict startup order dependency
- Each component is initialized sequentially but independently through the
Initmethod- The server implementation doesn't enforce any specific ordering constraints between components
The current order (consensus → gRPC → store → telemetry → REST) is suitable as:
- Consensus component is primary and correctly placed first
- Supporting services (gRPC, store, telemetry) follow
- REST gateway is appropriately last as it may depend on other services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any ordering requirements or dependencies between components rg -A 5 "AddCommands|ServerComponent" --type goLength of output: 9023
Script:
#!/bin/bash # Check for component dependencies in Start and Init methods ast-grep --pattern 'func ($x *Server[$_]) Start(context.Context) error { $$$ }' # Check component initialization order requirements rg -B 3 -A 3 "Start|Init" server/v2/server.goLength of output: 2339
simapp/v2/simdv2/cmd/root_di.go (4)
16-17: Necessary imports added correctlyThe added imports
serverv2 "cosmossdk.io/server/v2"and"cosmossdk.io/server/v2/cometbft"are appropriately included to support the new consensus component functionality.
36-44: Function implementation is well-structuredThe
NewCometBFTRootCmdfunction correctly utilizes generics and delegates toNewRootCmdWithConsensusComponent, allowing for flexibility in specifying the consensus component. The implementation aligns with Go best practices.
46-51: Function definition and documentation are appropriateThe
NewRootCmdWithConsensusComponentfunction is properly defined with a clear purpose. The use of generics and the function signature facilitate the injection of different consensus components. The documentation provides adequate context for users.
104-105: Consensus component integration is correctly implementedThe instantiation of
consensusComponentusingmakeConsensusComponent(clientCtx)and its subsequent use ininitRootCmdeffectively integrates the new consensus component into the command initialization process. Variable naming is clear and consistent.
julienrbrt
added
the
backport/v0.52.x
|
😁 |
(cherry picked from commit 6e6255d)
…#22353) Co-authored-by: Mark Rushakoff <[email protected]>
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
This renames NewRootCmd to NewCometBFTRootCmd, to make it clear that it uses CometBFT for the consensus layer. Callers who want to use a different consensus-providing server component can instead call NewRootCmdWithConsensusComponent, passing in a callback to be evaluated with a client context in order to produce the component.
This pattern is working for Gordian-Cosmos integration.
I'm not especially tied to the new function names. I'm also open to more clear naming for "consensus component" which occurs a few times.
/cc @kocubinski as this may overlap or conflict with #22267.
Summary by CodeRabbit
New Features
NewCometBFTRootCmdfunction for improved consensus component integration.NewRootCmdWithConsensusComponentfunction for flexible command creation.Bug Fixes
Tests