chore(simapp/v2): consensus server isn't command dep #23480
Conversation
📝 WalkthroughWalkthroughThe pull request modifies 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)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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
🧹 Nitpick comments (1)
simapp/v2/simdv2/cmd/commands.go (1)
105-122: LGTM! Clean consensus server initialization.The direct initialization of the consensus server with
cometbft.Newis more robust than the previous approach. All necessary parameters are properly provided.Consider wrapping the error with additional context:
- return nil, err + return nil, fmt.Errorf("failed to create consensus server: %w", err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 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.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: tests (00)
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-simapp-v2
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (2)
simapp/v2/simdv2/cmd/commands.go (2)
49-49: LGTM! Good cleanup of dependencies.The removal of the
ConsensusServerfield fromCommandDependenciessimplifies the dependency management by eliminating optional initialization. This change aligns well with the PR objective.
Line range hint
82-88: LGTM! Consistent integration across different paths.The changes maintain proper functionality for both client-only and full app paths while ensuring consistent integration with GRPC services and server commands.
Also applies to: 137-140, 165-165
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
simapp/v2/simdv2/cmd/commands.go(7 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.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (2)
simapp/v2/simdv2/cmd/commands.go (2)
49-49: LGTM: Good refactoring of dependencies!The addition of
ClientContextand removal ofConsensusServerfield improves the dependency structure by:
- Providing necessary context for consensus server initialization
- Removing redundant server instance storage
105-120: LGTM: Well-structured consensus server initialization!The consensus server initialization is properly implemented with:
- Comprehensive error handling
- Complete configuration passing
- Clear dependency management
github-actions
bot
added
C:server/v2
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
runtime/v2/builder.go(1 hunks)server/v2/appmanager/config.go(1 hunks)simapp/v2/app_config.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/v2/appmanager/config.go
🧰 Additional context used
📓 Path-based instructions (2)
simapp/v2/app_config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/v2/builder.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/app_config.go
201-201: overriden is a misspelling of overridden
(misspell)
🪛 GitHub Actions: Lint
simapp/v2/app_config.go
[error] 201-201: Misspelling: 'overriden' should be 'overridden'
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test-log
- GitHub Check: test-store
- GitHub Check: test-orm
- GitHub Check: test-hubl
- GitHub Check: test-simapp-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (1)
runtime/v2/builder.go (1)
Line range hint
127-134: LGTM: Simplified AppManager instantiation.The removal of the generic type parameter streamlines the code while maintaining the same functionality.
| @@ -2,7 +2,7 @@ package appmanager | |||
|
|
|||
| // Config represents the configuration options for the app manager. | |||
| type Config struct { | |||
| ValidateTxGasLimit uint64 `mapstructure:"validate-tx-gas-limit"` // TODO: check how this works on app mempool | |||
| ValidateTxGasLimit uint64 `mapstructure:"validate-tx-gas-limit"` | |||
There was a problem hiding this comment.
i believe this can be removed, we should open an issue on it to track it.
There was a problem hiding this comment.
It's being used here actually:
cosmos-sdk/server/v2/appmanager/appmanager.go
Line 235 in 3860b2b
It is part of the stf interface:
cosmos-sdk/server/v2/appmanager/appmanager.go
Line 235 in 3860b2b
Description
Small nit I found upgrading
chain-minimalto v2Author 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
ConsensusServerfield from command dependencies.AppManagerinstantiation to remove generic type parameter.GasConfigfor clarity.