feat(sims): Integration with app v2 #23013
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant enhancements to the Cosmos SDK's simulation framework, focusing on the integration with application version 2. The changes span multiple packages, including Changes
Sequence DiagramsequenceDiagram
participant SimApp as Simulation App
participant AppManager as App Manager
participant STF as State Transition Function
participant SimMsgFactory as Simulation Message Factory
participant Validator as Validator Set
SimApp->>AppManager: DeliverSims(context, block)
AppManager->>SimMsgFactory: Generate Transactions
SimMsgFactory-->>AppManager: Return Transactions
AppManager->>STF: Process Transactions
STF->>Validator: Update Validator Set
STF-->>AppManager: Return Block Response
AppManager-->>SimApp: Simulation Complete
Possibly related PRs
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
|
github-actions
bot
added
C:x/staking
C:x/gov
C:Simulations
C:Store
C:x/auth
C:x/nft
C:server/v2
julienrbrt
added
the
backport/v0.52.x
alpe
force-pushed
the
alex/sims2_v2_dev
branch
2 times, most recently
from
2877f68 to
4b8d17b
Compare
alpe
changed the title
alpe
force-pushed
the
alex/sims2_v2_dev
branch
from
0615dd9 to
faae91e
Compare
| @@ -0,0 +1,469 @@ | |||
| package simapp | |||
There was a problem hiding this comment.
I am going to refactor and move this into the simsx package in a new PR
| simsCtx := context.WithValue(exCtx, key, hi.Time) //nolint: staticcheck // using string key to decouple | ||
| var results []server.TxResult | ||
| var i int32 | ||
| for tx := range simsBuilder(simsCtx) { |
There was a problem hiding this comment.
This calls the sims message factories that were setup in sims_runner.doMainLoop()
server/v2/stf/sims_entry.go
Outdated
| if err := isCtxCancelled(simsCtx); err != nil { | ||
| return nil, err | ||
| } | ||
| results = append(results, s.deliverTx(simsCtx, newState, tx, transaction.ExecModeFinalize, hi, i+1)) |
There was a problem hiding this comment.
and delivered like block TX
| hi header.Info, | ||
| ) ([]server.TxResult, error) | ||
|
|
||
| func (s STF[T]) deliverBlock( |
There was a problem hiding this comment.
common code used by DeliverBlock and DeliverSims
alpe
changed the title
alpe
requested review from
auricom,
julienrbrt,
tac0turtle,
a team,
facundomedica,
testinginprod,
JulianToledano,
ziscky,
lucaslopezf,
sontrinh16 and
aaronc
as code owners
julienrbrt
changed the title
There was a problem hiding this comment.
Feel free to clean all the previous ones, they were commented out waiting for this.
We should indeed append -v2 so when have both in the backport.
There was a problem hiding this comment.
We need as well to re-enable it on CI and call the right jobs for main and eventually both in the backport in 0.52 (.github/workflows/sims.yml)
There was a problem hiding this comment.
I will fix them when the CLI params are integrated (in a new PR)
There was a problem hiding this comment.
Can we create a tracking PR for it?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
simapp/v2/go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe Go version
1.23.4is invalid. The latest stable version of Go is 1.22.0. Please update to a valid Go version.-go 1.23.4 +go 1.22
🧹 Nitpick comments (4)
simsx/v2/txutils.go (3)
71-73: Use lowercase error messagesThe error message
"unexpected Tx type"should start with a lowercase letter according to Go conventions.Apply this diff:
- return out, errors.New("unexpected Tx type") + return out, errors.New("unexpected tx type")
175-177: Use lowercase error messagesThe error message
"unexpected Tx type"should start with a lowercase letter according to Go conventions.Apply this diff:
- return out, errors.New("unexpected Tx type") + return out, errors.New("unexpected tx type")
192-194: Use lowercase error messagesThe error message
"unexpected Tx type"should start with a lowercase letter according to Go conventions.Apply this diff:
- return out, errors.New("unexpected Tx type") + return out, errors.New("unexpected tx type")server/v2/appmanager/appmanager.go (1)
205-225: Refactor common logic betweenDeliverBlockandDeliverSimsmethodsThe
DeliverBlockandDeliverSimsmethods share similar logic for retrieving the latest state and validating block height. Consider refactoring the common code into a helper function to reduce code duplication and improve maintainability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/build/simulations.mk(1 hunks)server/v2/appmanager/appmanager.go(4 hunks)server/v2/stf/sims_entry.go(1 hunks)simapp/v2/go.mod(1 hunks)simapp/v2/sim_runner.go(1 hunks)simsx/v2/msg_factory.go(1 hunks)simsx/v2/txutils.go(1 hunks)simsx/v2/valset.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- simsx/v2/msg_factory.go
- scripts/build/simulations.mk
- server/v2/stf/sims_entry.go
- simsx/v2/valset.go
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/appmanager/appmanager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simsx/v2/txutils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/sim_runner.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 (2)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (3)
simapp/v2/sim_runner.go (1)
65-69: Verify the block time constants for correctnessThe constant
maxTimePerBlockis set to10_000 * time.Second, which equals approximately 2.78 hours. This seems unusually long for a block time. Please verify if this is intentional.simapp/v2/go.mod (2)
53-56: LGTM: Dependencies align with app v2 integrationThe new dependencies are appropriate for the PR's purpose and are properly versioned.
Line range hint
271-282: LGTM: Well-structured replace directivesThe replace directives are well-organized and properly handle:
- Security fixes
- Local development paths
- Server v2 integration requirements
|
Thanks for the feedback! I have applied the changes. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
simapp/v2/sim_runner.go (1)
145-147: 🛠️ Refactor suggestionConsider consolidating
AppManagerandModuleManageraccessSince
xapp.GetApp()provides access to theModuleManager, you might not need bothAppManagerandModuleManagerin theTestInstance. Consider using one of the following approaches to simplify:
- Access the
ModuleManagerthroughAppManagerwhen needed.- Expose the
AppManagerinterface directly if specific functionalities are required.This can help reduce redundancy and improve maintainability.
🧹 Nitpick comments (5)
server/v2/appmanager/appmanager.go (1)
204-225: Clean and consistent implementation.The DeliverSims implementation:
- Follows the same pattern as DeliverBlock for consistency
- Has comprehensive error handling
- Properly delegates to the state transition function
However, consider adding a comment explaining the relationship between this method and DeliverBlock, as they share similar patterns.
-// DeliverSims same as DeliverBlock for sims only. +// DeliverSims processes simulated transactions similar to DeliverBlock, but uses +// a transaction generator (simsBuilder) instead of a predefined transaction list.simapp/v2/sim_runner.go (4)
195-195: Assist with reading simulation parameters from diskThere's a TODO comment indicating that simulation parameters should be read from disk as before. I can help implement this functionality to restore the previous behavior.
Do you want me to generate the code to read the simulation parameters from disk or open a GitHub issue to track this task?
240-240: Help replace legacy test data functionsThere's a TODO comment suggesting the replacement of legacy test data functions. I can assist in updating this part of the code to use the new testing utilities.
Do you want me to generate the updated code or open a GitHub issue to track this task?
345-348: Offer to implement CLI flags for simulation parametersThe code contains a TODO comment indicating that
numBlocksandmaxTXPerBlockshould be read from the CLI. I can help implement this functionality to make the simulation parameters configurable via command-line flags.Do you want me to generate the code to add CLI flags for these parameters or open a GitHub issue to track this task?
461-461: Assist with registering legacy and v1 message proposalsThere's a TODO comment indicating the need to register legacy and v1 message proposals. I can help implement this functionality to ensure all proposal types are properly registered.
Do you want me to generate the code for registering these proposals or open a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/v2/appmanager/appmanager.go(4 hunks)simapp/v2/sim_runner.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/appmanager/appmanager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/sim_runner.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 (7)
- GitHub Check: tests (00)
- GitHub Check: test-store-v2
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (4)
server/v2/appmanager/appmanager.go (3)
9-9: LGTM! Import addition is appropriate.The "iter" package import is necessary for handling transaction sequences in the new simulation functionality.
20-20: Well-structured interface composition!The embedding of TransactionFuzzer interface aligns with the previous review discussion and follows the Interface Segregation Principle.
61-70: Clear and well-documented interface definition.The TransactionFuzzer interface is well-documented and follows Go conventions. The separation of simulation-specific functionality improves modularity.
simapp/v2/sim_runner.go (1)
1-492: Code conforms to Uber Go style and best practicesThe implementation in
sim_runner.gois well-structured, follows the Uber Golang style guide, and adheres to best practices. The code is clear, readable, and maintainable.
|
@alpe lgtm - just looks like a formatting lint error |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
44-44: Add blank lines around lists for better readabilityAccording to markdown best practices, lists should be surrounded by blank lines to improve readability and ensure proper rendering.
### Client Breaking Changes + * [#11797](https://github.com/cosmos/cosmos-sdk/pull/11797) Remove all RegisterRESTRoutes (previously deprecated) * [#11089](https://github.com/cosmos/cosmos-sdk/pull/11089) interacting with the node through `grpc.Dial` requires clients to pass a codec refer to [doc](https://docs.cosmos.network/main/user/run-node/interact-node). +🧰 Tools
🪛 Markdownlint (0.37.0)
44-44: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
📜 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"
🪛 Markdownlint (0.37.0)
CHANGELOG.md
44-44: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-x-staking
- GitHub Check: test-x-nft
- GitHub Check: test-x-gov
- GitHub Check: test-store-v2
- GitHub Check: test-store
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: runtime-v2
- GitHub Check: dependency-review
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint
1-2000: Well-structured and comprehensive changelogThe changelog follows good practices with:
- Clear version sections with dates
- Categorized changes (Features, Improvements, Bug Fixes, etc.)
- Linked PR references
- Breaking changes clearly marked
- Detailed descriptions of changes
This makes it easy for users to understand the evolution of the codebase.
🧰 Tools
🪛 Markdownlint (0.37.0)
43-43: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
44-44: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
Co-authored-by: Alex | Interchain Labs <[email protected]> (cherry picked from commit cf721a6) # Conflicts: # CHANGELOG.md # runtime/v2/app.go # scripts/build/simulations.mk # server/v2/appmanager/appmanager.go # server/v2/appmanager/stf.go # server/v2/stf/stf.go # server/v2/streaming/examples/file/file.go # simapp/v2/go.mod # store/v2/db/db.go
Co-authored-by: Alexander Peters <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #20469
Sims integration with App v2
Most run parameters are currently hard coded in the test runner.
TODOs that are done in separate PRs
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
Based on the comprehensive summary of changes, here are the release notes:
Release Notes
New Features
Improvements
Simulation Enhancements
Database Support
Deprecations
These release notes provide a high-level overview of the significant changes introduced in this update, focusing on the end-user and developer-facing improvements.