fix(x/simulation): finalize block after operations #23382
Conversation
📝 WalkthroughWalkthroughThis pull request introduces changes to the Cosmos SDK's simulation module, specifically ensuring that block finalization occurs after all queued operations are processed. The Changes
Assessment against linked issues
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
🧹 Nitpick comments (6)
x/simulation/simulate.go (1)
258-262: LGTM! Block finalization timing fixed.The block finalization is now correctly placed after operations are executed, which aligns with the PR objectives.
Consider enhancing the error message to include the block hash for better debugging:
- return params, accs, fmt.Errorf("block finalization failed at height %d: %w", blockHeight, err) + return params, accs, fmt.Errorf("block finalization failed at height %d (hash: %X): %w", blockHeight, finalizeBlockReq.Hash, err)🧰 Tools
🪛 GitHub Actions: Tests / Code Coverage
[error] Unable to download artifact '5c63edab8d7b956371a5e3703fffd1380797f992-e2e-coverage'. Artifact not found.
CHANGELOG.md (5)
Line range hint
1-1: Add missing title headerThe changelog should start with a # title header for better documentation structure.
+ # Changelog
Line range hint
14-25: Improve changelog guiding principles section formattingThe guiding principles section should be formatted as a proper markdown list for better readability.
- Changelogs are for humans, not machines. - There should be an entry for every single version. - The same types of changes should be grouped. - Versions and sections should be linkable. - The latest version comes first. - The release date of each version is displayed. - Mention whether you follow Semantic Versioning. + * Changelogs are for humans, not machines. + * There should be an entry for every single version. + * The same types of changes should be grouped. + * Versions and sections should be linkable. + * The latest version comes first. + * The release date of each version is displayed. + * Mention whether you follow Semantic Versioning.
Line range hint
27-37: Improve usage section formattingThe usage section should be formatted as a proper markdown list for better readability.
- Change log entries are to be added to the Unreleased section under the - appropriate stanza (see below). Each entry is required to include a tag and - the Github issue reference in the following format: - - * (<tag>) \#<issue-number> message + # Usage + + * Change log entries should be added to the Unreleased section under the + appropriate stanza (see below) + * Each entry must include a tag and GitHub issue reference in the format: + * (<tag>) \#<issue-number> message
Line range hint
39-47: Improve types of changes section formattingThe types of changes section should be formatted consistently with proper markdown headers.
- Types of changes (Stanzas): - - "Features" for new features. - "Improvements" for changes in existing functionality. - "Deprecated" for soon-to-be removed features. - "Bug Fixes" for any bug fixes. - "Client Breaking" for breaking Protobuf, gRPC and REST routes used by end-users. - "CLI Breaking" for breaking CLI commands. - "API Breaking" for breaking exported APIs used by developers building on SDK. + ## Types of Changes + + * **Features**: New features + * **Improvements**: Changes in existing functionality + * **Deprecated**: Soon-to-be removed features + * **Bug Fixes**: Bug fixes + * **Client Breaking**: Breaking Protobuf, gRPC and REST routes used by end-users + * **CLI Breaking**: Breaking CLI commands + * **API Breaking**: Breaking exported APIs used by developers
Line range hint
48-49: Remove redundant reference linkThe reference to keepachangelog.com is redundant since the guiding principles are already listed above.
- "State Machine Breaking" for any changes that result in a different AppState given same genesisState and txList. - Ref: https://keepachangelog.com/en/1.0.0/ + * **State Machine Breaking**: Changes that result in a different AppState given same genesisState and txList
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)x/simulation/simulate.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/simulation/simulate.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 GitHub Actions: Tests / Code Coverage
x/simulation/simulate.go
[error] Unable to download artifact '5c63edab8d7b956371a5e3703fffd1380797f992-e2e-coverage'. Artifact not found.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (2)
x/simulation/simulate.go (2)
281-281: LGTM! Comprehensive simulation completion logging.The log message provides valuable metrics including height, block-time, operation count, run-time, and app-hash, which will help in monitoring and debugging simulations.
🧰 Tools
🪛 GitHub Actions: Tests / Code Coverage
[error] Unable to download artifact '5c63edab8d7b956371a5e3703fffd1380797f992-e2e-coverage'. Artifact not found.
258-262: Verify test coverage for block finalization changes.The pipeline indicates code coverage issues. Please ensure adequate test coverage for the modified block finalization logic.
Run this script to check test coverage for the modified code:
✅ Verification successful
Test coverage for block finalization is adequate
The codebase has comprehensive test coverage for block finalization, including various scenarios like invalid height, transaction handling, gas limits, and error cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for block finalization logic # Find all test files that might test block finalization echo "Searching for relevant test files..." rg -l "FinalizeBlock|SimulateFromSeedX" --type go --glob "*_test.go" # Check for specific test coverage of the block finalization logic echo "Checking test coverage of block finalization..." rg -A 5 "func.*Test.*FinalizeBlock|func.*Test.*SimulateFromSeedX" --type goLength of output: 4169
🧰 Tools
🪛 GitHub Actions: Tests / Code Coverage
[error] Unable to download artifact '5c63edab8d7b956371a5e3703fffd1380797f992-e2e-coverage'. Artifact not found.
|
Is there a way we can validate this behavior with a test? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
57-58: Inconsistent indentation in changelog entryThe bullet point for simulation module fix is not properly indented compared to other entries in the Bug Fixes section.
-* (x/simulation) [#23382](https://github.com/cosmos/cosmos-sdk/pull/23382) Finalize block after queued operations. + * (x/simulation) [#23382](https://github.com/cosmos/cosmos-sdk/pull/23382) Finalize block after queued operations.
Line range hint
785-786: Fix broken link in changelogThe link to previous versions points to a specific commit hash which may become invalid. Consider using a tag or release instead.
-[CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md#v0391---2020-08-11) (pre Stargate). +[CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/v0.39.1/CHANGELOG.md#v0391---2020-08-11) (pre Stargate).
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-core
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: dependency-review
- GitHub Check: Summary
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint
1-15: LGTM! Well-structured changelog headerThe header section follows the Keep a Changelog format and provides clear guiding principles for contributors.
Description
Closes: #22393
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
Bug Fixes
API Breaking Changes
x/paramsmodule.testutil/networkpackage; users should utilize the integration framework or system tests framework instead.GetBlockByHeightResponseto return an empty comet block.These changes enhance the Cosmos SDK's functionality and provide clearer migration paths for developers.