feat: simapp/v2 #20412
feat: simapp/v2 #20412
Conversation
WalkthroughWalkthroughThe recent updates encompass various aspects of the project, particularly focusing on enhancing the Go debugging configurations, refining dependency injection, and improving the initialization and configuration of the simulation application (SimApp) v2. Key changes include restructuring interface registries, augmenting Dockerfile commands, refining context handling, enhancing module initialization, and expanding the command-line interface for SimApp v2. Additionally, new functionalities for exporting application states and managing genesis accounts have been introduced. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant VSCode
participant GoDebugger
participant SimApp
participant Docker
participant CosmosSDK
User->>VSCode: Launch debugging configuration
VSCode->>GoDebugger: Initialize with "simapp v2" settings
GoDebugger->>SimApp: Start debugging session
SimApp->>Docker: Copy necessary modules
Docker->>SimApp: Build and run environment
SimApp->>CosmosSDK: Initialize modules and configurations
CosmosSDK->>SimApp: Return initialized state
SimApp->>GoDebugger: Provide debugging information
GoDebugger->>VSCode: Display debugging information
VSCode->>User: Show debugging session
Note over User,VSCode: User interacts with debugging session
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 CodeRabbit Configration File (
|
github-actions
bot
added
C:CLI
C:x/auth
C:x/upgrade
C:x/genutil
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.consensus.v1.CometInfo")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.consensus.v1.CometInfo does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -33,6 +33,10 @@ func (i *Info) Bytes() ([]byte, error) { | |||
| binary.LittleEndian.PutUint64(heightBytes, uint64(i.Height)) | |||
| buf = append(buf, heightBytes...) | |||
|
|
|||
| // TODO; permit empty hash OK for genesis block? | |||
| if len(i.Hash) == 0 { | |||
There was a problem hiding this comment.
in baseapp empty hash is populated with hash("") @facundomedica would need to confirm this. If so then id propose we do the same
There was a problem hiding this comment.
I wasn't able to find where this happens, I'll add an issue to investigate it.
| @@ -44,6 +48,9 @@ func (i *Info) Bytes() ([]byte, error) { | |||
| binary.LittleEndian.PutUint64(timeBytes, uint64(i.Time.Unix())) | |||
| buf = append(buf, timeBytes...) | |||
|
|
|||
| if len(i.AppHash) == 0 { | |||
The current code in #20412, when attempting to run simd, was failing with the error: panic: proto: not found" Wrapping the error returned by FindDescriptorByName gave a more informative error: panic: failed to find descriptor for "cosmos.accounts.v1.Msg": proto: not found However, there is an earlier root cause. The a.err field was set due to a failure in proto.MergedRegistry but later overridden. Fixing that early return now shows the root cause: panic: failed to build registry cache: proto: could not resolve import "tendermint/abci/types.proto": not found Helps #20492.
|
lets rebase then get this merged |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Outside diff range and nitpick comments (7)
scripts/simapp-v2-init.sh (1)
16-16: Consider a cross-platform approach for thesedin-place edit.On Linux, the
-ioption does not require an extension. You might want to handle this conditionally based on the operating system.contrib/images/simd-env/Dockerfile (1)
Line range hint
3-3: Optimize Dockerfile by pinning package versions and using--no-cache.- RUN apk add build-base git linux-headers + RUN apk add build-base=1.0.0 git=2.24.3 linux-headers=4.19 --no-cache - RUN apk add bash curl jq + RUN apk add bash=5.0 curl=7.67.0 jq=1.6 --no-cacheAlso applies to: 38-39
tests/integration/tx/context_test.go (1)
61-75: Consider using a constant for the error message to improve maintainability.Using a constant for the error message string would make the tests less brittle to changes in the error messaging in the source code.
x/consensus/proto/cosmos/consensus/v1/tx.proto (1)
54-58: Explain the use ofgogoproto.nullable = falsein a comment.For better code clarity and maintainability, consider adding a comment explaining why
gogoproto.nullable = falseis used for theevidenceandlast_commitfields in theCometInfomessage.core/header/service.go (1)
Line range hint
36-53: Clarify the handling of empty hash values in comments.The logic for handling empty
HashandAppHashvalues in theBytesmethod is crucial for correct behavior, especially for genesis blocks. Consider adding comments to explain why these checks are necessary and how they contribute to the integrity of the block encoding.types/tx_msg.go (1)
55-55: Clarify the intended use ofGetReflectMessagesto prevent misuse in critical processing paths.Consider adding documentation to explain that this function is intended for dynamic API usage and should not be used for actual message processing.
api/cosmos/consensus/v1/tx.pulsar.go (1)
2113-2171: Ensure that theCometInfostruct fields are properly documented to enhance code readability and maintainability.Consider adding comments to each field in the
CometInfostruct to explain their purpose and usage within the system.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (5)
api/cosmos/streaming/v1/grpc_grpc.pb.gois excluded by!**/*.pb.gosimapp/v2/go.modis excluded by!**/*.modsimapp/v2/go.sumis excluded by!**/*.sumx/consensus/types/consensus.pb.gois excluded by!**/*.pb.gox/consensus/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (57)
- .vscode/launch.json (1 hunks)
- api/cosmos/consensus/v1/tx.pulsar.go (5 hunks)
- codec/depinject.go (3 hunks)
- contrib/images/simd-env/Dockerfile (1 hunks)
- core/context/context.go (1 hunks)
- core/header/service.go (2 hunks)
- core/store/store.go (2 hunks)
- go.work.example (3 hunks)
- runtime/v2/app.go (1 hunks)
- runtime/v2/manager.go (2 hunks)
- runtime/v2/module.go (4 hunks)
- scripts/simapp-v2-init.sh (1 hunks)
- server/mock/tx.go (2 hunks)
- server/v2/cometbft/abci_test.go (1 hunks)
- server/v2/cometbft/mock_store.go (1 hunks)
- server/v2/stf/branch/writer_map.go (1 hunks)
- server/v2/stf/core_router_service.go (3 hunks)
- server/v2/stf/mock/tx.go (1 hunks)
- server/v2/stf/stf.go (3 hunks)
- simapp/app.go (1 hunks)
- simapp/v2/README.md (1 hunks)
- simapp/v2/app_config.go (1 hunks)
- simapp/v2/app_di.go (1 hunks)
- simapp/v2/default.nix (1 hunks)
- simapp/v2/export.go (1 hunks)
- simapp/v2/genesis_account.go (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (1 hunks)
- simapp/v2/simdv2/cmd/config.go (1 hunks)
- simapp/v2/simdv2/cmd/root_di.go (1 hunks)
- simapp/v2/simdv2/cmd/root_test.go (1 hunks)
- simapp/v2/simdv2/main.go (1 hunks)
- simapp/v2/sonar-project.properties (1 hunks)
- simapp/v2/upgrades.go (1 hunks)
- store/v2/commitment/store.go (4 hunks)
- store/v2/database.go (1 hunks)
- store/v2/root/factory.go (1 hunks)
- store/v2/root/store.go (1 hunks)
- tests/integration/distribution/keeper/msg_server_test.go (2 hunks)
- tests/integration/tx/context_test.go (1 hunks)
- types/mempool/mempool_test.go (3 hunks)
- types/mempool/signer_extraction_adapater_test.go (2 hunks)
- types/tx_msg.go (1 hunks)
- x/auth/tx/gogotx.go (5 hunks)
- x/consensus/depinject.go (2 hunks)
- x/consensus/keeper/keeper.go (4 hunks)
- x/consensus/keeper/keeper_test.go (13 hunks)
- x/consensus/proto/cosmos/consensus/v1/tx.proto (2 hunks)
- x/consensus/types/codec.go (1 hunks)
- x/consensus/types/consensus.go (1 hunks)
- x/consensus/types/msgs.go (2 hunks)
- x/distribution/CHANGELOG.md (1 hunks)
- x/distribution/depinject.go (3 hunks)
- x/distribution/keeper/abci.go (2 hunks)
- x/distribution/keeper/allocation_test.go (5 hunks)
- x/distribution/keeper/delegation_test.go (9 hunks)
- x/distribution/keeper/keeper.go (4 hunks)
- x/distribution/keeper/keeper_test.go (1 hunks)
Files not processed due to max files limit (5)
- x/distribution/migrations/v4/migrate_funds_test.go
- x/distribution/module.go
- x/genutil/client/cli/init.go
- x/staking/keeper/genesis.go
- x/upgrade/keeper/abci.go
Files not summarized due to errors (1)
- api/cosmos/consensus/v1/tx.pulsar.go: Error: Message exceeds token limit
Files not reviewed due to errors (8)
- x/consensus/types/consensus.go (no review received)
- x/distribution/keeper/abci.go (no review received)
- x/consensus/types/msgs.go (no review received)
- simapp/v2/default.nix (no review received)
- simapp/v2/upgrades.go (no review received)
- x/distribution/CHANGELOG.md (no review received)
- simapp/v2/simdv2/cmd/root_di.go (no review received)
- x/distribution/keeper/keeper_test.go (no review received)
Files skipped from review due to trivial changes (2)
- simapp/v2/README.md
- simapp/v2/sonar-project.properties
Additional context used
Path-based instructions (49)
core/context/context.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/main.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/types/codec.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"x/consensus/types/consensus.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/keeper/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/types/msgs.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/upgrades.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/genesis_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/mock/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/signer_extraction_adapater_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"tests/integration/tx/context_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"store/v2/database.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/mock_store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.core/header/service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/branch/writer_map.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/factory.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.codec/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/stf/core_router_service.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci_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"runtime/v2/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/tx_msg.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/mock/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"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.x/distribution/keeper/keeper_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"core/store/store.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.simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/mempool_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/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/export.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/tx/gogotx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/consensus/keeper/keeper_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"store/v2/commitment/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/keeper/allocation_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.runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/distribution/keeper/msg_server_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/distribution/keeper/delegation_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"api/cosmos/consensus/v1/tx.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Biome
.vscode/launch.json
[error] 5-5: Expected a property but instead found '// Run E2E tests from the current dir in VSCode'.
[error] 7-7: End of file expected
[error] 7-7: End of file expected
[error] 7-7: End of file expected
[error] 7-7: End of file expected
[error] 8-8: End of file expected
[error] 8-8: End of file expected
[error] 8-8: End of file expected
[error] 8-8: End of file expected
[error] 9-9: End of file expected
[error] 9-9: End of file expected
[error] 9-9: End of file expected
[error] 9-9: End of file expected
[error] 10-10: End of file expected
[error] 10-10: End of file expected
[error] 10-10: End of file expected
[error] 10-10: End of file expected
[error] 11-11: End of file expected
[error] 11-11: End of file expected
[error] 11-11: End of file expected
Shellcheck
scripts/simapp-v2-init.sh
[info] 13-13: Double quote to prevent globbing and word splitting.
Hadolint
contrib/images/simd-env/Dockerfile
[warning] 3-3: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
[info] 3-3: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages
[warning] 38-38: Always tag the version of an image explicitly
[warning] 39-39: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>
[info] 39-39: Use the
--no-cacheswitch to avoid the need to use--updateand remove/var/cache/apk/*when done installing packages
Markdownlint
x/distribution/CHANGELOG.md
34-34: Expected: 2; Actual: 4
Unordered list indentation
35-35: Expected: 2; Actual: 4
Unordered list indentation
38-38: Expected: 2; Actual: 4
Unordered list indentation
40-40: Expected: 2; Actual: 4
Unordered list indentation
42-42: Expected: 2; Actual: 4
Unordered list indentation
44-44: Expected: 2; Actual: 4
Unordered list indentation
46-46: Expected: 2; Actual: 4
Unordered list indentation
49-49: Expected: 2; Actual: 4
Unordered list indentation
51-51: Expected: 2; Actual: 4
Unordered list indentation
76-76: null
Files should end with a single newline character
Additional comments not posted (57)
core/context/context.go (1)
3-5: The changes toExecModeKeyandCometInfoKeyenhance flexibility and reduce the risk of key collision by using anonymous struct types.simapp/v2/simdv2/main.go (1)
13-19: The main function is well-implemented with proper initialization and error handling for the simapp v2 application.go.work.example (1)
15-15: The addition of./simapp/v2to the workspace configuration is necessary and correct for integrating the new simapp v2 module.x/consensus/types/codec.go (1)
8-14: The registration ofMsgUpdateParamsunder thesdk.Msginterface is correctly implemented, ensuring proper message handling within the consensus module..vscode/launch.json (1)
16-24: The new debug configuration for "simapp v2" is set up correctly, facilitating development and debugging of the application.simapp/v2/simdv2/cmd/root_test.go (1)
17-43: The unit tests for the root command are well-implemented, providing sufficient coverage for the initialization and home flag registration functionalities.scripts/simapp-v2-init.sh (1)
28-33: Ensure the use of--keyring-backend testis well-documented for clarity.This is appropriate for testing environments but should be clearly documented to avoid confusion in production settings.
x/consensus/depinject.go (2)
41-41: Consider adding documentation for theAuthorityfield to clarify its role and usage within the module.
70-70: LGTM! The initialization of theAuthorityfield with a default value is a good practice in configuration management.simapp/v2/genesis_account.go (1)
11-48: The implementation ofSimGenesisAccountand its validation logic are well-constructed and adhere to best practices for data integrity and error handling.server/v2/stf/mock/tx.go (1)
26-28: Adding a nil check in theGetMessagesmethod enhances error handling and prevents potential runtime errors.x/distribution/depinject.go (1)
Line range hint
31-74: The integration ofCometServicein the distribution module's dependency injection setup is well-implemented, enhancing the module's capabilities and adhering to good practices in modular design.types/mempool/signer_extraction_adapater_test.go (1)
Line range hint
11-48: The unit tests forSignerExtractionAdapterare comprehensive, covering various transaction scenarios effectively and ensuring robustness in signer extraction logic.server/mock/tx.go (4)
99-101: TheHashmethod returns a zero-initialized byte array, which is appropriate for a mock implementation.
103-105: TheGetGasLimitmethod correctly returns a zero gas limit, suitable for a mock implementation.
107-109: TheGetMessagesmethod appropriately returns nil for both the messages and the error, which is expected in a mock implementation.
111-113: TheGetSendersmethod correctly returns nil for both the sender addresses and the error, suitable for a mock implementation.x/consensus/keeper/keeper.go (3)
77-93: TheUpdateParamsmethod correctly validates the consensus parameters and handles errors appropriately. It also emits an event after updating the parameters, which is a good practice for traceability.
95-111: TheSetParamsmethod correctly sets the initial consensus parameters and handles errors appropriately. This method is crucial for initializing consensus parameters when a chain starts.
113-125: TheGetCometInfomethod correctly retrieves comet information from the store and handles errors appropriately. The conversion of comet info to the core structure is well-implemented.simapp/v2/simdv2/cmd/config.go (3)
13-26: TheinitCometBFTConfigfunction correctly sets custom log levels for the CometBFT configuration. This customization is essential for controlling log verbosity based on the application's needs.
28-69: TheinitClientConfigfunction correctly sets up a custom client configuration, including gas adjustment settings. This customization allows for better control over transaction fees and is crucial for user experience.
71-126: TheinitAppConfigfunction correctly sets up a custom application configuration, including a custom field in the app.toml. This customization is essential for extending the application's configuration beyond the SDK's defaults.core/store/store.go (2)
136-136: Clarification in the comment forGetReaderimproves understanding of its functionality.
145-145: Clarification in the comment forGetWriterimproves understanding of its functionality.runtime/v2/module.go (1)
248-250: Addition ofProvideAppVersionModifiermethod aligns with the system's design and provides necessary functionality.simapp/v2/simdv2/cmd/commands.go (4)
90-130: ThestartCommandfunction correctly implements server initialization and graceful shutdown handling.
132-145: ThegenesisCommandfunction is well-implemented, enhancing functionality and flexibility by accepting additional commands as parameters.
147-167: ThequeryCommandfunction is correctly implemented, setting up querying subcommands effectively.
169-191: ThetxCommandfunction is correctly implemented, setting up transaction-related subcommands effectively.types/mempool/mempool_test.go (2)
79-97: Addition of methodsBytes,Hash,GetGasLimit,GetMessages, andGetSendersto thetestTxstruct enhances testability of the mempool functionality.
113-131: Addition of methodsBytes,Hash,GetGasLimit,GetMessages, andGetSendersto thesigErrTxstruct enhances testability of the mempool functionality.simapp/v2/app_di.go (1)
44-251: Addition of various keepers and settings in theSimAppstruct correctly implements the necessary functionality for the application, following best practices for dependency injection and configuration management.x/distribution/keeper/keeper.go (2)
31-32: The addition ofcometServiceto theKeeperstruct aligns with the PR's objective to integrate with theserver/v2infrastructure. Ensure that all methods usingcometServicehandle potential errors appropriately.
Line range hint
65-81: TheNewKeeperconstructor now initializescometService. This is a critical integration point for the new service. Ensure thatcometServiceis properly tested to handle edge cases and potential failures.Verification successful
The search results indicate that
cometServiceis referenced in the test filetests/integration/distribution/keeper/msg_server_test.go. This suggests that there are indeed tests that involvecometService, specifically in an integration test context.
- File:
tests/integration/distribution/keeper/msg_server_test.go
- References:
cometService := runtime.NewContextAwareCometInfoService()stakingKeeper := stakingkeeper.NewKeeper(..., cometService)distrKeeper := distrkeeper.NewKeeper(..., cometService, ...)These references show that
cometServiceis being instantiated and passed to various keepers, indicating that it is part of the integration tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `cometService` is properly tested. # Test: Search for tests related to `cometService`. Expect: Tests that cover various scenarios. rg --type go $'func Test.*cometService'Length of output: 40
Script:
#!/bin/bash # Description: Verify that `cometService` is referenced in test files. # Search for references to `cometService` in Go test files. rg --type go 'cometService' --glob '*_test.go' # Search for any test functions that might indirectly test `cometService`. rg --type go 'cometService' --glob '*_test.go' --context 10Length of output: 4365
x/auth/tx/gogotx.go (2)
31-33: ThenewWrapperFromDecodedTxfunction has been modified to handle transaction decoding and setup. Ensure that error messages are clear and provide enough context for debugging.
318-342: The caching mechanism ingogoTxWrapperuses SHA-256 for hashing and caches the transaction bytes. Ensure that the hash computation is only triggered when necessary to optimize performance.simapp/v2/app_config.go (1)
100-271: The configuration inappConfigis comprehensive and integrates multiple modules. Ensure that each module configuration is validated against the expected types and constraints to prevent runtime issues.x/consensus/keeper/keeper_test.go (3)
33-36: Setup logic appears correct and well-structured.
Line range hint
59-92: Test logic for GRPC queries is clear and effectively uses table-driven tests.
Line range hint
160-257: Test logic for updating parameters is comprehensive and correctly handles edge cases.store/v2/commitment/store.go (1)
115-125: MethodIsEmptycorrectly checks for the presence of the latest version key in the database.store/v2/root/store.go (1)
390-396: MethodwriteSCeffectively handles writing changesets to the state commitment tree, including migration logic.x/distribution/keeper/allocation_test.go (4)
32-39: LGTM! TheemptyCometServicestruct and its methodCometInfoare correctly implemented as a minimal stub forcomet.Service.
Line range hint
41-73: Ensure the new dependencytestCometServiceis correctly integrated and utilized within the test setup.Verification successful
The
testCometServiceis correctly integrated and utilized within theTestAllocateTokensToValidatorWithCommissionfunction. It is properly initialized and passed as a dependency to thekeeper.NewKeeperfunction, ensuring that the test setup is complete and accurate.
testCometServiceis defined as an instance ofemptyCometService.- It is used as a parameter in the
keeper.NewKeeperfunction call within the test function.This confirms that the new dependency
testCometServiceis correctly integrated into the test setup.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `testCometService` in the test setup. # Test: Search for the usage of `testCometService` in the test setup. Expect: Proper initialization and usage. rg --type go $'testCometService'Length of output: 905
Script:
#!/bin/bash # Description: Extract lines of code where `testCometService` is used within the `TestAllocateTokensToValidatorWithCommission` function. # Test: Ensure proper initialization and usage of `testCometService` in the test function. rg --type go -A 10 -B 10 'testCometService' x/distribution/keeper/allocation_test.goLength of output: 2692
281-281: Ensure the new dependencytestCometServiceis correctly integrated and utilized within the test setup.
140-140: Ensure the new dependencytestCometServiceis correctly integrated and utilized within the test setup.Verification successful
The
testCometServiceis correctly integrated and utilized within the test setup. It is initialized as an instance ofemptyCometService, which is defined as an empty struct. ThetestCometServiceis consistently used in the test functions acrossallocation_test.go,delegation_test.go, andkeeper_test.go.
allocation_test.go:testCometServiceis used in the setup of multiple test functions.delegation_test.go:testCometServiceis used in the setup of multiple test functions.keeper_test.go:testCometServiceis used in the setup of test functions.The integration appears to be correct and consistent across the test files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `testCometService` in the test setup. # Test: Search for the usage of `testCometService` in the test setup. Expect: Proper initialization and usage. rg --type go $'testCometService'Length of output: 905
Script:
#!/bin/bash # Description: Verify the definition and usage of `emptyCometService` and `testCometService`. # 1. Search for the definition of `emptyCometService`. rg --type go 'type emptyCometService' # 2. Examine the usage of `testCometService` in the relevant test files. rg --type go 'testCometService' x/distribution/keeper/allocation_test.go x/distribution/keeper/delegation_test.go x/distribution/keeper/keeper_test.goLength of output: 1220
Script:
#!/bin/bash # Description: Examine the context around the usage of `testCometService` in the test files. # 1. Display lines around the usage of `testCometService` in `allocation_test.go`. rg --type go -C 5 'testCometService' x/distribution/keeper/allocation_test.go # 2. Display lines around the usage of `testCometService` in `delegation_test.go`. rg --type go -C 5 'testCometService' x/distribution/keeper/delegation_test.go # 3. Display lines around the usage of `testCometService` in `keeper_test.go`. rg --type go -C 5 'testCometService' x/distribution/keeper/keeper_test.goLength of output: 2663
server/v2/stf/stf.go (2)
Line range hint
17-17: Review the initialization of theSTFstruct in theNewSTFfunction to ensure all fields are correctly set up.Also applies to: 25-25
Verification successful
The
NewSTFfunction is used in three places, and the extracted snippets provide sufficient context to verify the initialization of theSTFstruct. Here are the observations:
server/v2/stf/stf.go:
// NewSTF returns a new STF instance. func NewSTF[T transaction.Tx]( handleMsg func(ctx context.Context, msg transaction.Msg) (transaction.Msg, error), handleQuery func(ctx context.Context, req transaction.Msg) (transaction.Msg, error), doPreBlock func(ctx context.Context, txs []T) error, doBeginBlock func(ctx context.Context) error, doEndBlock func(ctx context.Context) error,server/v2/cometbft/abci_test.go:
s := stf.NewSTF( func(ctx context.Context, msg transaction.Msg) (msgResp transaction.Msg, err error) { err = kvSet(t, ctx, "exec") return nil, err }, func(ctx context.Context, msg transaction.Msg) (msgResp transaction.Msg, err error) {runtime/v2/builder.go:
a.app.stf = stf.NewSTF[transaction.Tx]( stfMsgHandler, stfQueryHandler, a.app.moduleManager.PreBlocker(), a.app.moduleManager.BeginBlock(), endBlocker,In all instances, the
NewSTFfunction is called with the appropriate parameters, ensuring that theSTFstruct is correctly initialized.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of the `STF` struct. # Test: Search for the usage of `NewSTF` in the codebase. Expect: Proper initialization of all fields. rg --type go $'NewSTF'Length of output: 245
Script:
#!/bin/bash # Extract the relevant code snippets where `NewSTF` is used to verify the initialization of the `STF` struct. # Extract usage in server/v2/stf/stf.go rg --type go --context 5 'NewSTF' server/v2/stf/stf.go # Extract usage in server/v2/cometbft/abci_test.go rg --type go --context 5 'NewSTF' server/v2/cometbft/abci_test.go # Extract usage in runtime/v2/builder.go rg --type go --context 5 'NewSTF' runtime/v2/builder.goLength of output: 1459
111-111: Ensure that theDeliverBlockmethod correctly handles the block delivery logic, especially the handling of genesis blocks and transaction execution.runtime/v2/manager.go (2)
281-284: The changes to theEndBlockfunction are well-integrated and follow best practices for blockchain module management.
281-284: Review the logic for handling multiple validator updates.Verification successful
The search results indicate that the
EndBlockfunction is used across various modules, and there are specific checks in place to ensure that no two modules provide validator updates simultaneously. The relevant code inruntime/v2/manager.goincludes logic to handle this scenario.
runtime/v2/manager.go:281-284: TheEndBlockfunction is defined here.runtime/v2/manager.go:300-305: The function iterates over modules and checks if they implementHasEndBlockerorhasABCIEndBlock.runtime/v2/manager.go:307-309: If a module provides validator updates, it ensures no other module has already done so.The code appears to handle the concern correctly by ensuring that only one module can provide validator updates. Therefore, the review comment is verified.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no two modules provide validator updates simultaneously. # Test: Search for the usage of `EndBlock` across modules. Expect: Proper handling of validator updates without conflicts. rg --type go $'EndBlock'Length of output: 31039
simapp/app.go (1)
360-360: Ensure thecometServiceis properly initialized before use.Verification successful
The
cometServiceis properly initialized before its use in thedistrkeeper.NewKeepercall in thesimapp/app.gofile.
cometServiceis initialized at line 360 insimapp/app.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `cometService` is initialized before its use in `distrkeeper.NewKeeper`. # Test: Search for the initialization of `cometService`. Expect: Initialization before its use. rg --type go --context 5 'cometService := runtime.NewContextAwareCometInfoService()'Length of output: 2338
tests/integration/distribution/keeper/msg_server_test.go (2)
133-133: Ensure proper initialization ofdistrKeeperwith all required dependencies.
159-159: Properly setting the proposer address in the context is crucial for simulating accurate block proposals.x/distribution/keeper/delegation_test.go (3)
537-537: Ensure thattestCometServiceis properly mocked or initialized before use.Verification successful
The initialization of
testCometServiceis confirmed inx/distribution/keeper/allocation_test.goasvar testCometService = &emptyCometService{}. This initialization ensures thattestCometServiceis properly mocked before its usage in the tests.
Initialization found in
x/distribution/keeper/allocation_test.go:var testCometService = &emptyCometService{}Usage in
x/distribution/keeper/delegation_test.gois consistent with the initialization.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of `testCometService` in the test setup. # Test: Search for the initialization of `testCometService`. Expect: Initialization before use. rg --type go $'testCometService'Length of output: 905
Script:
#!/bin/bash # Description: Verify the context of `testCometService` initialization and its usage in the relevant test files. # Check the initialization of `testCometService` in `allocation_test.go` rg --type go --context 5 'var testCometService = &emptyCometService{}' x/distribution/keeper/allocation_test.go # Check the usage of `testCometService` in `delegation_test.go` rg --type go --context 5 'testCometService' x/distribution/keeper/delegation_test.goLength of output: 1722
Script:
#!/bin/bash # Description: Verify the context of `testCometService` initialization and its usage in the relevant test files. # Correct the regex pattern and check the initialization of `testCometService` in `allocation_test.go` rg --type go --context 5 'var testCometService = \&emptyCometService\{\}' x/distribution/keeper/allocation_test.goLength of output: 544
910-910: Ensure thattestCometServiceis properly mocked or initialized before use.
756-756: Ensure thattestCometServiceis properly mocked or initialized before use.Verification successful
The
testCometServiceis initialized inallocation_test.goasvar testCometService = &emptyCometService{}. This initialization ensures thattestCometServiceis properly mocked before use.
allocation_test.go:var testCometService = &emptyCometService{}Since
testCometServiceis properly initialized inallocation_test.go, it is safe to assume that it is correctly used indelegation_test.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of `testCometService` in the test setup. # Test: Search for the initialization of `testCometService`. Expect: Initialization before use. rg --type go $'testCometService'Length of output: 905
Script:
#!/bin/bash # Description: Verify the definition and initialization of `emptyCometService` used for `testCometService`. # Test: Search for the definition of `emptyCometService`. Expect: Proper mock or initialization. rg --type go $'emptyCometService'Length of output: 665
Script:
#!/bin/bash # Description: Verify the initialization of `testCometService` in `delegation_test.go`. # Test: Search for the initialization of `testCometService` in `delegation_test.go`. Expect: Initialization using `emptyCometService`. rg --type go --context 5 'testCometService' x/distribution/keeper/delegation_test.goLength of output: 1440
Script:
#!/bin/bash # Description: Verify the initialization or import of `testCometService` in `delegation_test.go`. # Test: Search for the initialization or import of `testCometService` in `delegation_test.go`. Expect: Initialization or import statement. rg --type go --context 5 'testCometService' x/distribution/keeper/delegation_test.go | grep -E 'var|import'Length of output: 107
Script:
#!/bin/bash # Description: Verify the initialization or import of `testCometService` in related files. # Test: Search for the initialization or import of `testCometService` in related files. Expect: Initialization or import statement. rg --type go --context 5 'testCometService' x/distribution/keeper/allocation_test.go x/distribution/keeper/keeper_test.goLength of output: 3413
| jq '.app_state.gov.voting_params.voting_period = "600s"' genesis.json > temp.json && mv temp.json genesis.json | ||
|
|
||
| # to change the inflation | ||
| jq '.app_state.mint.minter.inflation = "0.300000000000000000"' genesis.json > temp.json && mv temp.json genesis.json | ||
|
|
||
| # change the initial height to 2 to work around store/v2 and iavl limitations with a genesis block | ||
| jq '.initial_height = 2' genesis.json > temp.json && mv temp.json genesis.json |
There was a problem hiding this comment.
Optimize the jq commands by combining them into a single invocation.
- jq '.app_state.gov.voting_params.voting_period = "600s"' genesis.json > temp.json && mv temp.json genesis.json
- jq '.app_state.mint.minter.inflation = "0.300000000000000000"' genesis.json > temp.json && mv temp.json genesis.json
- jq '.initial_height = 2' genesis.json > temp.json && mv temp.json genesis.json
+ jq '.app_state.gov.voting_params.voting_period = "600s" | .app_state.mint.minter.inflation = "0.300000000000000000" | .initial_height = 2' genesis.json > temp.json && mv temp.json genesis.jsonCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| jq '.app_state.gov.voting_params.voting_period = "600s"' genesis.json > temp.json && mv temp.json genesis.json | |
| # to change the inflation | |
| jq '.app_state.mint.minter.inflation = "0.300000000000000000"' genesis.json > temp.json && mv temp.json genesis.json | |
| # change the initial height to 2 to work around store/v2 and iavl limitations with a genesis block | |
| jq '.initial_height = 2' genesis.json > temp.json && mv temp.json genesis.json | |
| jq '.app_state.gov.voting_params.voting_period = "600s" | .app_state.mint.minter.inflation = "0.300000000000000000" | .initial_height = 2' genesis.json > temp.json && mv temp.json genesis.json |
| @@ -0,0 +1,113 @@ | |||
| package cometbft | |||
|
|
|||
| /* TODO this has been failing for months, it needs to be re-written | |||
There was a problem hiding this comment.
Address the failing test as indicated by the TODO comment.
Would you like assistance in rewriting this test to ensure reliability?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- simapp/v2/simdv2/cmd/commands.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- simapp/v2/simdv2/cmd/commands.go
| @@ -126,3 +126,11 @@ func (a *App) GetLogger() log.Logger { | |||
| func (a *App) ExecuteGenesisTx(_ []byte) error { | |||
| panic("not implemented") | |||
| } | |||
|
|
|||
| func (a *App) SetAppVersion(context.Context, uint64) error { | |||
There was a problem hiding this comment.
Can't we leave those unimplemented on the app?
The version modifier can be nil, so the logic will be skipped anyway.
| @@ -246,3 +244,7 @@ func ProvideGenesisTxHandler(appBuilder *AppBuilder) genesis.TxHandler { | |||
| func ProvideCometService() comet.Service { | |||
| return &services.ContextAwareCometInfoService{} | |||
| } | |||
|
|
|||
| func ProvideAppVersionModifier(app *AppBuilder) app.VersionModifier { | |||
| return app.app | |||
There was a problem hiding this comment.
I would return nil here (and add a comment saying only baseapp implements a version modifier --
cosmos-sdk/x/upgrade/keeper/keeper.go
Line 438 in 0a6ee40
simapp/v2/app_config.go
Outdated
There was a problem hiding this comment.
can we add the SkipStoreKeys like in app_config for simapp v1?
| // and appends "valoper" and "valcons" for validator and consensus addresses respectively. | ||
| // When providing a custom address codec in auth, custom address codecs must be provided here as well. | ||
| // | ||
| // func() runtime.ValidatorAddressCodec { return <- custom validator address codec type -> } |
There was a problem hiding this comment.
those aren't in runtime anymore, but in core I believe
| // ExportAppStateAndValidators exports the state of the application for a genesis file. | ||
| func (app *SimApp) ExportAppStateAndValidators(forZeroHeight bool, jailAllowedAddrs, modulesToExport []string) (servertypes.ExportedApp, error) { | ||
| panic("not implemented") | ||
| // server/v1 implementation is below: |
There was a problem hiding this comment.
can we delete all? we check easily find the server/v1 implementation on gh
| @@ -38,11 +38,14 @@ type ModuleInputs struct { | |||
| type ModuleOutputs struct { | |||
| depinject.Out | |||
|
|
|||
| Authority Authority | |||
There was a problem hiding this comment.
shouldn't we expose this from the consensus keeper instead? this feels a bit weird like this imho.
| @@ -35,6 +39,7 @@ func NewKeeper(cdc codec.BinaryCodec, env appmodule.Environment, authority strin | |||
| Environment: env, | |||
| authority: authority, | |||
| ParamsStore: collections.NewItem(sb, collections.NewPrefix("Consensus"), "params", codec.CollValue[cmtproto.ConsensusParams](cdc)), | |||
| cometInfo: collections.NewItem(sb, collections.NewPrefix("CometInfo"), "comet_info", codec.CollValue[types.CometInfo](cdc)), | |||
There was a problem hiding this comment.
nit, store prefix in consensus/types/keys.go (same for the other above)
|
|
||
| nextParams := params.Update(&consensusParams) |
There was a problem hiding this comment.
this should be maintained, this was a bug fix: #20347
There was a problem hiding this comment.
I actually think most of that PR has been lost during merge conflict resolution here
| ) | ||
|
|
||
| func (msg MsgUpdateParams) ToProtoConsensusParams() (cmtproto.ConsensusParams, error) { | ||
| if msg.Evidence == nil || msg.Block == nil || msg.Validator == nil { | ||
| return cmtproto.ConsensusParams{}, errors.New("all parameters must be present") | ||
| } | ||
|
|
||
| if msg.Abci != nil && msg.Feature != nil && msg.Feature.VoteExtensionsEnableHeight != nil { |
| sdkCtx = sdkCtx.WithBlockHeight(1 - sdk.ValidatorUpdateDelay) // TODO: remove this need for WithBlockHeight | ||
| ctx = sdkCtx | ||
|
|
||
| // sdkCtx := sdk.UnwrapSDKContext(ctx) |
There was a problem hiding this comment.
how can this get updated? I don't think we have a service for this.
cc @tac0turtle
At least I don't think we should comment it, we may miss it for baseapp.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
simapp/v2/go.modis excluded by!**/*.modsimapp/v2/go.sumis excluded by!**/*.sum
Files selected for processing (6)
- .vscode/launch.json (1 hunks)
- runtime/v2/manager.go (3 hunks)
- server/v2/stf/branch/writer_map.go (1 hunks)
- server/v2/stf/core_router_service.go (3 hunks)
- simapp/v2/export.go (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- runtime/v2/manager.go
- server/v2/stf/branch/writer_map.go
- server/v2/stf/core_router_service.go
- simapp/v2/export.go
- simapp/v2/simdv2/cmd/commands.go
Additional context used
Biome
.vscode/launch.json
[error] 5-5: Expected a property but instead found '// Run E2E tests from the current dir in VSCode'.
[error] 7-7: End of file expected
[error] 7-7: End of file expected
[error] 7-7: End of file expected
[error] 7-7: End of file expected
[error] 8-8: End of file expected
[error] 8-8: End of file expected
[error] 8-8: End of file expected
[error] 8-8: End of file expected
[error] 9-9: End of file expected
[error] 9-9: End of file expected
[error] 9-9: End of file expected
[error] 9-9: End of file expected
[error] 10-10: End of file expected
[error] 10-10: End of file expected
[error] 10-10: End of file expected
[error] 10-10: End of file expected
[error] 11-11: End of file expected
[error] 11-11: End of file expected
[error] 11-11: End of file expected
Additional comments not posted (1)
.vscode/launch.json (1)
16-22: The new VSCode configuration for "simapp/v2" looks correctly set up and aligns with the PR's objectives to support the new version.
Description
Introduce simapp/v2 which runs on server/v2 infrastructure.
Depends on:
#20387Includes: #20483, #20485
Closes: #20492
Testing
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.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
HashandAppHashfields in theInfostruct to ensure proper encoding.Refactor
ReaderMapandWriterMapinterfaces for consistency.GetStateChangesmethod to handle state changes recursively.Chores
simapp v2.