feat(sims): Add sims2 framework and factory methods

[alpe]

Description

This PR introduces some new helper types to simplify message construction for simulations (sims). The focus is on better dev UX for new message factories.
Technically, they are adapters that build upon the existing sims framework.
This PR also included the refactoring for all x</module>/simulation/operations into the new factory structures.

(Reviewer note at the end)

* Message factory

Simple functions as factories for dedicated sdk.Msgs. They have access to the context, reporter and test data environment. For example:

func MsgSendFactory() simsx.SimMsgFactoryFn[*types.MsgSend] {
	return func(ctx context.Context, testData *simsx.ChainDataSource, reporter simsx.SimulationReporter) ([]simsx.SimAccount, *types.MsgSend) {
		from := testData.AnyAccount(reporter, simsx.WithSpendableBalance())
		to := testData.AnyAccount(reporter, simsx.ExcludeAccounts(from))
		coins := from.LiquidBalance().RandSubsetCoins(reporter, simsx.WithSendEnabledCoins())
		return []simsx.SimAccount{from}, types.NewMsgSend(from.AddressBech32, to.AddressBech32, coins)
	}
}

* Sims registry

A new helper to register message factories with a default weight value. They can be overwritten by a parameters file as before. The registry is passed to the AppModule type. For example:

func (am AppModule) WeightedOperationsX(weights simsx.WeightSource, reg simsx.Registry) {
	reg.Add(weights.Get("msg_send", 100), simulation.MsgSendFactory())
	reg.Add(weights.Get("msg_multisend", 10), simulation.MsgMultiSendFactory())
}

* Reporter

The reporter is a flow control structure that can be used in message factories to skip execution at any point. The idea is similar to the testing.T Skip in Go stdlib. Internally, it converts skip, success and failure events to legacy sim messages.
The reporter also provides some capability to print an execution summary.
It is also used to interact with the test data environment to not have errors checked all the time.
Message factories may want to abort early via

if reporter.IsSkipped() {
	return nil, nil
}

* Test data environment

The test data environment provides simple access to accounts and other test data used in most message factories. It also encapsulates some app internals like bank keeper or address codec.

Note to the reviewers

Apologies for this big PR. 🙈 It created too much extra work to keep the smaller PRs updated with changes in main as they pile up.

• A good start for the review are the message factories in the modules. They can be compared against the old operations to confirm the behaviour is still the same (more or less).
  Please note. the gov module is the most complex one compared to the others due to the proposal and legacy support.
• The package name simsx is not necessarily the best name. If somebody would come up with a better name, I would refactor this in a new PR. Naming is hard

Summary by CodeRabbit

• New Features

  • Introduced a new SelectBy method in the API for enhanced thread-safe operations.
  • Added the sims2 framework with factory methods for simplified message creation.
  • Enhanced simulation capabilities with a new delivery mechanism for blockchain transactions.

• Bug Fixes

  • Improved functionality by ensuring blocks are committed only up to the specified halt height.

• Refactor

  • Streamlined module initialization by reducing dependencies for several modules.
  • Refactored simulation-related functionalities for better management.

• Documentation

  • Updated CHANGELOG.md to reflect significant updates and enhancements in the API.

• Tests

  • Adjusted test configurations to improve context management and streamline testing processes.

[coderabbitai]

Walkthrough

Walkthrough

This pull request introduces significant updates to the API, including the addition of a new method SelectBy to the Mempool interface and the introduction of the sims2 framework with factory methods for message creation. It modifies several application modules to streamline their initialization parameters, enhances testing capabilities with new test targets and flags, and refines dependency management in the go.mod file. Furthermore, it implements a simulation reporting system and various utility functions for slice manipulation, improving the overall functionality and maintainability of the codebase.

Changes

Files	Change Summary
CHANGELOG.md	Added entries for new SelectBy method in Mempool and sims2 framework with factory methods.
go.mod	Added and removed indirect dependencies for golang.org/x/exp.
tests/go.mod	Updated dependency for cosmossdk.io/x/nft to indirect.
x/params/go.mod	Removed multiple indirect dependencies to streamline the project.
x/protocolpool/go.mod	Updated gotest.tools/v3 to be an indirect dependency.
x/staking/go.mod	Updated gotest.tools/v3 to be an indirect dependency.
x/mint/go.mod	Updated gotest.tools/v3 to be an indirect dependency.
x/gov/go.mod	Updated gotest.tools/v3 to be an indirect dependency.
x/distribution/go.mod	Updated gotest.tools/v3 to be an indirect dependency.
tests/integration/genutil/init_test.go	Modified parameters in staking.NewAppModule for simplification.
simapp/sim_test.go	Transitioned from sims to simsx package, updated simulation framework and test functions.
simsx/README.md	Introduced simsx package with message factories and simulation utilities.
simsx/common_test.go	Added testing utilities for simulating account behavior and transaction logic.
simsx/msg_factory_test.go	Created unit tests for message factories to validate functionality.
simsx/runner.go	Enhanced simulation framework with new interfaces and methods for running simulations.
x/gov/module.go	Removed old governance methods and added new ones for enhanced proposal simulations.

Possibly related issues

• remove cosmossdk.io/x/* dependence in the root go module #18290: This PR addresses the need to remove cosmossdk.io/x/* dependencies in the root Go module, which is relevant as it reflects ongoing efforts to streamline module dependencies.

Possibly related PRs

• refactor: decouple comet from modules #21382: The changes in the main PR include the addition of the SelectBy method to the Mempool interface, which is directly related to the modifications made in the x/gov/module.go file where the governance proposal simulations were enhanced, indicating a connection in the API updates.
• fix(unorderedtx): issues reported in audit #21467: The changes in the main PR regarding the API updates and the introduction of new methods in the Mempool interface may connect with the modifications in the SimApp class in this retrieved PR, which enhances transaction management capabilities.
• build(deps): bump to core v1.0.0-alpha.1 and remove replaces #21603: The updates in the main PR regarding dependency management and the introduction of new features may relate to the changes in this retrieved PR that updates the core dependency version and removes replaces, indicating a shift in how dependencies are managed.

Suggested labels

C:x/genutil, C:server/v2, C:x/upgrade, C:x/accounts, C:core/testing

---

Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5af301c and 143dd59.

Files selected for processing (2)

• docs/build/building-modules/14-simulator.md (1 hunks)
• x/genutil/client/cli/validate_genesis_test.go (1 hunks)

Additional context used

Path-based instructions (2)

x/genutil/client/cli/validate_genesis_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"

docs/build/building-modules/14-simulator.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

Additional comments not posted (2)

x/genutil/client/cli/validate_genesis_test.go (1)

65-65: Verify the module's initialization with the reduced number of parameters.

The change in the instantiation of staking.NewAppModule reduces the number of parameters passed from four to three. Ensure that the module's initialization is still correct with the omitted nil argument and that it's no longer necessary for the test's context.

To verify that the change is correct, search for the staking.NewAppModule function declaration and check if its signature has been updated to accept fewer parameters. Run the following command:

Verification successful

---

Change in staking.NewAppModule instantiation is correct

The verification process confirms that the staking.NewAppModule function in x/staking/module.go indeed takes two parameters: cdc codec.Codec and keeper *keeper.Keeper. This aligns with the updated instantiation in the test file, where the nil argument was removed. The change is consistent with the actual function signature and does not introduce any issues.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the signature of the `staking.NewAppModule` function.

# Test: Search for the function declaration. Expect: Function signature with 3 parameters.
rg --type go -A 5 $'func NewAppModule\('

Length of output: 7563

docs/build/building-modules/14-simulator.md (1)

120-120: Verify the impact of the staking.NewAppModule signature change.

The staking.NewAppModule function signature has been modified:

• The cdc parameter has been added as the first argument.
• The app.accountKeeper and app.supplyKeeper arguments have been removed.

Please ensure that these changes do not adversely affect the initialization and functionality of the staking module. Verify that the removal of the accountKeeper and supplyKeeper dependencies does not break any necessary interactions or introduce bugs.

Run the following script to analyze the usage of staking.NewAppModule and its interactions with other modules:

Verification successful

To further verify the changes and their impact, let's examine the Keeper struct in the staking module and its initialization. This will help us understand how accountKeeper is now made available and if there are any replacements for the supplyKeeper functionality.

---

Verification of staking.NewAppModule signature change: No issues found

The changes to the staking.NewAppModule function signature have been implemented consistently across the codebase. The removal of accountKeeper and supplyKeeper from the constructor does not indicate that these dependencies are no longer used in the module. Instead, the module's architecture has been refactored:

1. The accountKeeper and bankKeeper are still part of the staking Keeper struct and are initialized in the NewKeeper function.
2. These dependencies are now injected into the Keeper struct during initialization, rather than being passed directly to the NewAppModule function.
3. The supplyKeeper has been completely removed from the staking module, with its functionality likely absorbed by the bankKeeper.

These changes appear to be part of a broader refactoring effort to improve the module's architecture and dependency management. The staking module's core functionality should remain intact, with the bankKeeper now handling operations previously managed by the supplyKeeper.

Developers should ensure that all necessary operations previously performed by the supplyKeeper are now correctly implemented using the bankKeeper.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the `staking.NewAppModule` signature change.

# Test 1: Search for `staking.NewAppModule` function calls. 
# Expect: Only occurrences with the new signature.
rg --type go $'staking\.NewAppModule'

# Test 2: Search for usages of `accountKeeper` and `supplyKeeper` within the staking module.
# Expect: No direct usages, as they have been removed from the constructor.
rg --type go --glob 'x/staking/**' -e $'accountKeeper|supplyKeeper'

Length of output: 3765

---

Script:

#!/bin/bash
# Description: Examine the Keeper struct and its initialization in the staking module

# Test 1: Check the Keeper struct definition
echo "Keeper struct definition:"
rg --type go --glob 'x/staking/**' -e 'type Keeper struct' -A 10

# Test 2: Check Keeper initialization
echo "\nKeeper initialization:"
rg --type go --glob 'x/staking/**' -e 'func NewKeeper\(' -A 15

# Test 3: Check for any new dependencies that might replace supplyKeeper
echo "\nPossible supplyKeeper replacements:"
rg --type go --glob 'x/staking/**' -e 'bankKeeper|BankKeeper|SupplyKeeper|supplyKeeper'

Length of output: 21815

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  -- I pushed a fix in commit <commit_id>, please review it.
  -- Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  -- @coderabbitai generate unit testing code for this file.
  -- @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  -- @coderabbitai generate interesting stats about this repository and render them as a table.
  -- @coderabbitai read src/utils.ts and generate unit testing code.
  -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  -- @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[tac0turtle]

nice job, love we got this cleanup

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (1)

simsx/delivery.go (1)

28-98: Excellent implementation of the simulation delivery mechanism!

The DeliverSimsMsg function is well-structured and documented, making it easy to understand its purpose and behavior. The error handling and reporting flow is robust, ensuring that failures are logged with relevant details.

A few suggestions for improvement:

1. Consider adding a comment explaining the purpose of the Collect function used on line 81, as it may not be immediately clear to readers.
2. The error variable on line 88 is named err2, which could be more descriptive. Consider renaming it to something like handlerErr or resultHandlerErr.
3. The comment generation on lines 89-92 could be extracted into a separate function for better readability and reusability.

Overall, the function is well-implemented and follows good practices for error handling and reporting.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 64ae68a and c31ed3b.

Files ignored due to path filters (1)

• x/params/go.sum is excluded by !**/*.sum

Files selected for processing (10)

• CHANGELOG.md (1 hunks)
• go.mod (1 hunks)
• simsx/delivery.go (1 hunks)
• simsx/runner.go (1 hunks)
• tests/go.mod (1 hunks)
• testutil/sims/simulation_helpers.go (1 hunks)
• x/gov/go.mod (1 hunks)
• x/params/go.mod (0 hunks)
• x/protocolpool/go.mod (1 hunks)
• x/staking/go.mod (1 hunks)

Files not reviewed due to no reviewable changes (1)

• x/params/go.mod

Files skipped from review due to trivial changes (2)

• tests/go.mod
• x/staking/go.mod

Files skipped from review as they are similar to previous changes (3)

• go.mod
• x/gov/go.mod
• x/protocolpool/go.mod

Additional context used

Path-based instructions (4)

simsx/delivery.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

testutil/sims/simulation_helpers.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

simsx/runner.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"

Markdownlint

CHANGELOG.md

57-57: null
Lists should be surrounded by blank lines

(MD032, blanks-around-lists)

Additional comments not posted (13)

simsx/delivery.go (3)

1-15: LGTM!

The package declaration and imports are correctly organized and formatted. The package name simsx is acceptable for now, but open for renaming suggestions as per the PR objectives.

---

16-26: LGTM!

The type definitions are clear and serve their intended purposes:

• AppEntrypoint alias provides a convenient way to refer to the simulation entry point interface.
• AccountSource interface encapsulates the retrieval of account information, which is necessary for generating mock transactions.
• SimDeliveryResultHandler function type allows for flexible handling of delivery errors, as some simulations may expect certain errors.

---

45-45: Past review comment addressed.

The DeliverSimsMsg function now takes a context.Context parameter, resolving the suggestion from the previous review.

testutil/sims/simulation_helpers.go (1)

53-56: LGTM!

The changes to the PrintStats function look good. The new logLine parameter allows for a more flexible logging approach, which is a nice improvement.

Just make sure to update the callers of this function to provide a logLine function.

simsx/runner.go (7)

53-59: LGTM!

The SimStateFactory struct is well-defined with clear field names and types. The addition of AccountSource and BalanceSource fields enhances the functionality by providing sources for accounts and balances. The fields have appropriate documentation comments explaining their purpose.

---

70-89: LGTM!

The Run function provides a convenient way to run simulation tests with default settings. It leverages the RunWithSeeds function to perform the actual simulation. The function signature and documentation comments are clear and informative.

---

91-125: LGTM!

The RunWithSeeds function provides a flexible way to run simulation tests with custom seeds and parameters. It sets up the environment and creates an instance of the simulation app for each seed. The parallel execution of tests for each seed improves performance. The function signature and documentation comments are clear and informative.

---

127-170: LGTM!

The RunWithSeed function provides a detailed implementation of running a simulation test for a single seed. It sets up the environment, creates an instance of the simulation app, and performs the simulation using the SimulateFromSeedX function. The function checks the simulation results, prints stats, and performs post-run actions. The function signature and documentation comments are clear and informative.

---

172-203: LGTM!

The new interfaces (HasWeightedOperationsX, HasWeightedOperationsXWithProposals, HasProposalMsgsX) provide a more structured and extensible way to define weighted operations and proposal messages for simulations. The legacy interfaces (HasLegacyWeightedOperations, HasLegacyProposalMsgs, HasLegacyProposalContents) are deprecated, indicating a transition to the new interface structure. The interface definitions are clear and well-documented.

---

205-220: LGTM!

The TestInstance struct provides a convenient way to encapsulate the necessary components for testing simulations. It includes fields for the app instance, database, working directory, configuration, logger, and execution log writer. The struct is well-documented, making it easy to understand its purpose and usage.

---

294-339: LGTM!

The NewSimulationAppInstance function provides a convenient way to initialize a TestInstance for a SimulationApp. It sets up the necessary components, such as the working directory, database, logger, and app options. The function handles the cleanup of the database on test completion. The function is well-documented, making it easy to understand its purpose and usage.

CHANGELOG.md (2)

57-57: The v0.50.x changes section looks good!

The content in this section follows the Keep A Changelog format consistently, with clear subsections for each category of change.

The markdownlint warnings about blank lines around lists can be safely ignored, as they are just minor formatting nits that don't impact the meaning or readability.

Nice work documenting the changes thoroughly and consistently!

Tools

Markdownlint

57-57: null
Lists should be surrounded by blank lines

(MD032, blanks-around-lists)

---

Line range hint 1000-1002: Good call linking to the previous changelog.

For a project with a long history of releases, it makes sense to link to the previous changelog rather than copying all that content into this file. It keeps this changelog focused and readable.

No changes needed here!

Tools

Markdownlint

56-56: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)

---

57-57: null
Lists should be surrounded by blank lines

(MD032, blanks-around-lists)

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (2)

CHANGELOG.md (2)

59-59: Consider adding a blank line before and after the list to improve readability.

According to Markdown best practices, it's recommended to surround lists with blank lines to visually separate them from the surrounding text. This helps improve the readability of the document.

Tools

Markdownlint

59-59: null
Lists should be surrounded by blank lines

(MD032, blanks-around-lists)

---

Line range hint 1-1: The CHANGELOG looks comprehensive and well-structured overall. Good job!

The categorization of changes into clear sections, references to relevant issues/PRs, and level of detail provided for each entry makes this an excellent changelog. It gives a good high-level overview of the major changes between SDK versions.

Just one minor nitpick around list formatting, otherwise looks great! Let me know if you would like me to submit a PR to fix the list formatting.

Tools

Markdownlint

58-58: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines

(MD022, blanks-around-headings)

---

59-59: null
Lists should be surrounded by blank lines

(MD032, blanks-around-lists)

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 618a49f and 67e7372.

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

CHANGELOG.md

59-59: null
Lists should be surrounded by blank lines

(MD032, blanks-around-lists)