feat(simsx): add back AppStateFnWithExtendedCbs #23236
Conversation
with moduleStateCb to access moduleState and rawStateCb to extend rawState
📝 WalkthroughWalkthroughThe pull request introduces a new function, 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
🧹 Nitpick comments (1)
testutil/sims/state_helpers.go (1)
49-61: Enhance documentation with usage examplesWhile the documentation is clear, consider adding:
- Example usage of the callbacks
- Clarification of when exactly the panic occurs (currently only mentioned in docs)
Example documentation addition:
// Example: // // AppStateFnWithExtendedCbs( // cdc, // addressCodec, // validatorCodec, // modules, // genesisState, // func(name string, state interface{}) { // if name == "bank" { // bankState := state.(*banktypes.GenesisState) // // Modify bank state // } // }, // func(rawState map[string]json.RawMessage) { // // Add custom module state // rawState["custom"] = customState // }, // )
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testutil/sims/state_helpers.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
testutil/sims/state_helpers.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 (5)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (3)
testutil/sims/state_helpers.go (3)
45-47: Clean implementation of backward compatibilityThe modification elegantly maintains backward compatibility by delegating to the new extended function with nil callbacks.
163-165: Consider adding error handling for moduleStateCbThe callback could panic if the provided state doesn't match the expected type. Consider wrapping the callback in a recover block or documenting that users should handle type assertions safely.
169-173: Verify thread safety of rawStateCbThe rawState map is being modified after all module states are processed. Ensure that:
- The callback doesn't override existing module states
- Any concurrent access to rawState is properly synchronized
Run this script to check for potential concurrent modifications:
✅ Verification successful
Thread safety verification completed - No issues found
The code is thread-safe as the callback is executed synchronously within a single thread context, with no concurrent access to the rawState map. The callback's position after module state processing is a valid pattern for state customization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any concurrent usage of rawState map rg -A 5 "go\s+func.*rawState.*map\[string\]json\.RawMessage" # Search for any goroutine spawning in simulation code rg "go\s+func" --type goLength of output: 4795
Script:
#!/bin/bash # Check the simulation_helpers.go file content cat testutil/sims/simulation_helpers.goLength of output: 4968
| addressCodec, validatorCodec address.Codec, | ||
| modules []module.AppModuleSimulation, | ||
| genesisState map[string]json.RawMessage, | ||
| moduleStateCb func(moduleName string, genesisState interface{}), |
There was a problem hiding this comment.
Don't both moduleStateCb and rawStateCb all for the same functionality? What is the purpose of including both of them?
The semantics of this function become confused because it is unclear to callers that first the module state callback is executed and then second the raw state callback is executed, meaning things could be overwritten
There was a problem hiding this comment.
As there is use case of accessing specific module genesisState first, then mutate the raw state accordingly. May be we can do a better naming of the post callback to prevent confusion.
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
testutil/sims/state_helpers.go (1)
49-61: Improve documentation clarity regarding callback execution order.The documentation should explicitly state the execution order of callbacks to prevent confusion:
moduleStateCbis called first for each module statepostRawStateCbis called after all module states are processedAlso, consider renaming
postRawStateCbtofinalizeRawStateCbto better reflect its purpose as the final callback in the sequence.// AppStateFnWithExtendedCbs returns the initial application state using a genesis or the simulation parameters. // It panics if the user provides files for both of them. // If a file is not given for the genesis or the sim params, it creates a randomized one. // genesisState is the default genesis state of the whole app. -// moduleStateCb is the callback function to access moduleState. -// postRawStateCb is the callback function to extend rawState. +// moduleStateCb is called first for each module's state during processing. +// postRawStateCb is called after all module states are processed, allowing final modifications to the raw state.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testutil/sims/state_helpers.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
testutil/sims/state_helpers.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-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-integration
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (2)
testutil/sims/state_helpers.go (2)
45-47: LGTM! Clean delegation to the extended function.The modification maintains backward compatibility by delegating to
AppStateFnWithExtendedCbswith nil callbacks.
163-165: LGTM! Verify callback usage patterns.The callback implementation is correct with proper nil checks and logical execution order. However, it would be beneficial to verify how these callbacks are used across the codebase to ensure they're used as intended.
Let's check the callback usage patterns:
Also applies to: 169-172
✅ Verification successful
Callback implementation verified as correct
The callbacks are properly implemented as optional extension points. Current codebase shows no active usage of these callbacks, which is fine as they are designed for future extensibility. The nil checks are necessary and correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usage patterns of AppStateFnWithExtendedCbs to understand how callbacks are utilized # Search for direct function calls echo "Direct function calls:" rg -A 5 "AppStateFnWithExtendedCbs\(" --type go # Search for potential callback implementations echo -e "\nPotential callback implementations:" rg -A 5 "func.*\(.*moduleName string,.*genesisState interface{}\)" --type go rg -A 5 "func.*\(.*rawState map\[string\]json\.RawMessage\)" --type goLength of output: 1874
Script:
#!/bin/bash # Search for similar callback patterns and indirect usage echo "Similar callback patterns:" rg -B 2 -A 2 "func\(.*string.*interface{}\)" --type go echo -e "\nPotential variable assignments:" rg "var\s+\w+\s+func\(.*string.*interface{}\)" --type go rg ":\s*=\s*func\(.*string.*interface{}\)" --type goLength of output: 766
Script:
#!/bin/bash # Search with properly escaped patterns echo "Searching for callback patterns in test files:" rg -B 2 -A 2 "func.*moduleName.*interface" --type go echo -e "\nSearching test files for callback usage:" rg -B 2 -A 2 "moduleStateCb|postRawStateCb" --type go test echo -e "\nSearching for variable assignments:" rg "var\s+\w+\s*=\s*func.*moduleName" --type go rg "\w+\s*:=\s*func.*moduleName" --type goLength of output: 7454
with moduleStateCb to access moduleState and rawStateCb to extend rawState
Description
Closes: #XXXX
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
New Features
Documentation
The changes provide more flexible state initialization for testing and simulation scenarios, allowing developers to customize state generation with additional callback functions.