Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sims): Add sims2 framework and factory methods #21613

Merged
merged 19 commits into from
Sep 16, 2024
Merged

feat(sims): Add sims2 framework and factory methods #21613

merged 19 commits into from
Sep 16, 2024

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Sep 9, 2024

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.

Copy link
Contributor

coderabbitai bot commented Sep 9, 2024

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

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
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.

@alpe alpe changed the title Feat: Add sims2 framework and factory methods feat(sims): Add sims2 framework and factory methods Sep 9, 2024
@github-actions github-actions bot removed the C:server/v2 Issues related to server/v2 label Sep 10, 2024
@alpe alpe force-pushed the alex/sims2_main branch 3 times, most recently from 64821b2 to c220ca1 Compare September 11, 2024 14:34
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (2)
simsx/README.md (2)

30-39: Add blank lines around the code example and the heading.

The reporter section provides a clear explanation of the concept and a good code example. However, to improve readability, please consider the following:

  1. Add blank lines before and after the code example.
  2. Add blank lines before and after the heading.

Apply this diff to add blank lines around the code example and the heading:

It is also used to interact with the test data environment to not have errors checked all the time.

+
#### * [Reporter](https://github.com/cosmos/cosmos-sdk/blob/main/simsx/reporter.go)
+
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

+
```go
if reporter.IsSkipped() {
    return nil, nil
}

<details>
<summary>Tools</summary>

<details>
<summary>Markdownlint</summary><blockquote>

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

(MD022, blanks-around-headings)

---

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

</blockquote></details>

</details>

---

`41-42`: **Add blank lines around the heading.**

The test data environment section provides a clear explanation of the concept. However, to improve readability, please consider adding blank lines before and after the heading.

Apply this diff to add blank lines around the heading:

```diff

* 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.


<details>
<summary>Tools</summary>

<details>
<summary>Markdownlint</summary><blockquote>

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

(MD022, blanks-around-headings)

</blockquote></details>

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>Review details</summary>

**Configuration used: .coderabbit.yml**
**Review profile: CHILL**

<details>
<summary>Commits</summary>

Files that changed from the base of the PR and between 972661e9eb3f71934b942f2f3b995b254f6c3bed and 39549c9b5242162aa9cebfa27912c17a299e28fc.

</details>


<details>
<summary>Files selected for processing (6)</summary>

* simapp/sim_test.go (10 hunks)
* simsx/README.md (1 hunks)
* simsx/common_test.go (1 hunks)
* simsx/msg_factory_test.go (1 hunks)
* simsx/runner.go (1 hunks)
* x/gov/module.go (2 hunks)

</details>










<details>
<summary>Files skipped from review as they are similar to previous changes (2)</summary>

* simapp/sim_test.go
* simsx/msg_factory_test.go

</details>

<details>
<summary>Additional context used</summary>

<details>
<summary>Path-based instructions (4)</summary><blockquote>

<details>
<summary>simsx/README.md (1)</summary>

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


</details>
<details>
<summary>simsx/common_test.go (2)</summary>

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"


</details>
<details>
<summary>x/gov/module.go (1)</summary>

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

</details>
<details>
<summary>simsx/runner.go (1)</summary>

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

</details>

</blockquote></details>

<details>
<summary>Markdownlint</summary><blockquote>

<details>
<summary>simsx/README.md</summary><blockquote>

6-6: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time

(MD001, heading-increment)

---

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

(MD022, blanks-around-headings)

---

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

(MD022, blanks-around-headings)

---

9-9: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

23-23: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

35-35: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

</blockquote></details>

</blockquote></details>

</details>
<details>
<summary>Additional comments not posted (19)</summary><blockquote>

<details>
<summary>simsx/README.md (3)</summary><blockquote>

`1-5`: **LGTM!**

The introduction section provides a clear and concise overview of the simsx package. It explains the purpose and focus of the package well.

---

`6-18`: **Add blank lines around the code example.**

The message factory section provides a clear explanation of the concept and a good code example. However, to improve readability, please consider adding blank lines before and after the code example.

Apply this diff to add blank lines around the code example:

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

+
```go
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)
    }
}

Based on the learning from the past review comment, skipping the suggestion to fix the heading level as it is considered a personal preference by the maintainer.

<details>
<summary>Tools</summary>

<details>
<summary>Markdownlint</summary><blockquote>

6-6: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time

(MD001, heading-increment)

---

9-9: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

</blockquote></details>

</details>

---

`20-28`: **Add blank lines around the code example.**

The sims registry section provides a clear explanation of the concept and a good code example. However, to improve readability, please consider adding blank lines before and after the code example.

Apply this diff to add blank lines around the code example:

```diff
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:

+
```go
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())
}

Addressing the past review comment by adding a blank line after the code example.

<details>
<summary>Tools</summary>

<details>
<summary>Markdownlint</summary><blockquote>

23-23: null
Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

</blockquote></details>

</details>

</blockquote></details>
<details>
<summary>simsx/common_test.go (8)</summary><blockquote>

`25-35`: **LGTM!**

The `SimAccountFixture` function is a useful testing utility that creates a `SimAccount` with a deterministic random account and a default liquid balance. The ability to customize the account properties using mutator functions enhances its flexibility for various testing scenarios.

---

`38-46`: **LGTM!**

The `MemoryAccountSource` function is a handy testing utility that creates an in-memory account source from a list of `SimAccount`s. The use of a map with the account address string as the key enables efficient lookups when the `AccountSourceFn` is invoked.

---

`49-62`: **LGTM!**

The `txConfig` function sets up a proper `client.TxConfig` for testing by registering the required interfaces and implementations. The use of the `must` helper function ensures that any error during the setup will panic and fail the test immediately, providing clear feedback.

---

`71-73`: **LGTM!**

The `SimDeliver` method is a straightforward adapter that allows the `SimDeliverFn` function type to implement the `AppEntrypoint` interface. It simply invokes the underlying function with the provided arguments.

---

`79-81`: **LGTM!**

The `GetAccount` method enables the `AccountSourceFn` function type to implement the `AccountSource` interface. It simply invokes the underlying function with the provided context and account address, adhering to the interface contract.

---

`85-132`: **LGTM!**

The `FakeAccountI` type is a suitable test implementation of the `sdk.AccountI` interface. It embeds the `SimAccount` type to provide the necessary account properties and adds `id` and `seq` fields for customization. The getter methods are implemented correctly, and panicking in the setter methods is acceptable for testing purposes, as they are not expected to be called in the test scenarios.

---

`137-154`: **LGTM!**

The `MockAccountSourceX` type is a versatile mock implementation of the `AccountSourceX` interface. It defines function fields for `GetAccount` and `GetModuleAddress` methods, allowing customization of the mock's behavior for different test scenarios. Panicking when the function fields are not set ensures that the mock is used correctly and helps identify unexpected calls during testing.

---

`156-161`: **LGTM!**

The `must` function is a handy helper for handling errors in test setup code. It panics if the provided error is not nil, ensuring that the test fails immediately with clear feedback. The use of generics makes the function flexible and reusable for different return types.

</blockquote></details>
<details>
<summary>x/gov/module.go (2)</summary><blockquote>

`218-221`: **LGTM!**

The `ProposalMsgsX` function correctly registers the `TextProposalFactory` with a weight of 5 for the `submit_text_proposal` message type, utilizing the new `simsx` package for enhanced governance simulations.

---

`223-240`: **Looks good!**

The `WeightedOperationsX` function correctly registers various message factories for governance operations, including proposal submission, deposits, votes, and cancellations. It utilizes the new `simsx` package and the `WeightedProposalMsgIter` for a more structured approach to governance simulations. The inclusion of legacy proposal types ensures backward compatibility.

</blockquote></details>
<details>
<summary>simsx/runner.go (6)</summary><blockquote>

`63-82`: **LGTM!**

The `Run` function provides a clean and concise entry point to run simulation tests with default seeds and parameters. It delegates the actual execution to the `RunWithSeeds` function, maintaining a clear separation of concerns.

---

`84-118`: **Excellent implementation!**

The `RunWithSeeds` function is well-designed and efficiently runs simulation tests with the provided parameters. Key highlights include:

- Iterating over the seeds and running the simulation test for each seed in parallel, which can greatly improve execution time.
- Setting up the environment, creating an instance of the simulation app, and performing post-run actions for each seed, ensuring proper isolation and consistency.
- Utilizing the `testing.T` instance and `t.Parallel()` to ensure accurate reporting of test results and parallel execution.

Overall, this function provides a robust and efficient framework for running simulation tests.

---

`120-157`: **Well-structured and comprehensive!**

The `RunWithSeed` function provides a complete and well-organized implementation for running a simulation test with a specific seed. It encapsulates the entire flow of setting up the environment, creating the simulation app instance, running the simulation, checking the exported simulation, printing stats, performing post-run actions, and closing the app.

The function is structured in a logical and readable manner, making it easy to understand and maintain. It leverages the `SimulateFromSeedX` function to ensure the simulation is run correctly and consistently.

Additionally, the function includes valuable features such as checking the exported simulation and printing stats, which aid in debugging and analysis. It also ensures proper cleanup and resource management by performing post-run actions and closing the app.

Overall, this function is a solid and reliable component of the simulation testing framework.

---

`210-279`: **Comprehensive and flexible!**

The `prepareWeightedOps` function plays a crucial role in preparing the weighted operations for the simulation. It demonstrates a comprehensive and flexible approach to collecting proposal messages, contents, and weighted operations from the simulation modules.

Key aspects of the function include:

- Proper initialization of the simulation state and handling of parameter reading from a file, ensuring a consistent starting point for the simulation.
- Support for both the new `HasProposalMsgsX` interface and the legacy `HasLegacyProposalMsgs` and `HasLegacyProposalContents` interfaces when collecting proposal messages and contents. This allows for a smooth transition and backward compatibility with existing simulation modules.
- Collection of weighted operations from the simulation modules, supporting both the new `HasWeightedOperationsX` and `HasWeightedOperationsXWithProposals` interfaces and the legacy `HasLegacyWeightedOperations` interface. This flexibility ensures compatibility with various module implementations.
- Returning the weighted operations and a simulation reporter, providing the necessary data for running the simulation and capturing relevant information.

The function is well-structured and handles the preparation of weighted operations effectively, considering different interfaces and legacy support.

---

`281-326`: **Thorough and configurable!**

The `NewSimulationAppInstance` function provides a thorough and configurable approach to initializing and setting up a TestInstance of a SimulationApp. It encapsulates the necessary steps to create a fully functional simulation environment.

Noteworthy aspects of the function include:

- Creation of a temporary working directory and a LevelDB database for the simulation app, ensuring data isolation and persistence during the simulation.
- Initialization of a logger based on the verbosity flag and setting the logger's seed to the test configuration's seed, promoting consistent logging across test runs.
- Utilization of the provided app factory function to create an instance of the simulation app, allowing flexibility in creating different types of simulation apps.
- Configuration of app options and base app options based on the test configuration, enabling customization of the simulation app's behavior.
- Returning a comprehensive TestInstance that contains all the essential components for running the simulation, including the simulation app, database, working directory, configuration, logger, and execution log writer.

The function is well-structured and provides a solid foundation for setting up and running simulation tests. It offers flexibility and configurability while ensuring the necessary components are properly initialized.

---

`336-342`: **Clever and useful!**

The `WriteToDebugLog` function is a clever and useful adapter that allows writing data to the logger at the debug level using the `io.Writer` interface. It provides a convenient way to integrate logging with other parts of the codebase that expect an `io.Writer`.

By taking a logger as input and returning a `writerFn` that captures the logger and writes the provided bytes to it, the function effectively adapts the logger to the `io.Writer` interface. This enables seamless integration and allows for consistent logging across different components.

The function is concise, well-named, and serves a specific purpose effectively. It promotes code reuse and maintainability by leveraging the `io.Writer` interface.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job, love we got this cleanup

simsx/delivery.go Outdated Show resolved Hide resolved
simsx/runner.go Show resolved Hide resolved
simsx/runner.go Show resolved Hide resolved
(cherry picked from commit b75bab14bb9bf529529d49bd78622e0e046171e5)
* main:
  test: fix sims (#21735)
  build: bump proto-builder (#21730)
  refactor(schema)!: rename IntegerStringKind and DecimalStringKind (#21694)
  feat(types/collections): add `LegacyDec` collection value (#21693)
  refactor(server): alias AppOptions to coreserver.DynamicConfig (#21711)
  refactor(simapp): simplify simapp di (#21718)
  feat: replace the cosmos-db usecases in the tests with `core/testing` (#21525)
  feat(runtime/v2): store loader on simappv2 (#21704)
  docs(x/auth): vesting (#21715)
  build(deps): Bump google.golang.org/grpc from 1.66.1 to 1.66.2 (#21670)
  refactor(systemtest): Add cli.RunAndWait for common operations (#21689)
  fix(runtime/v2): provide default factory options if unset in app builder (#21690)
  chore: remove duplicate proto files for the same proto file (#21648)
  feat(x/genutil): add better error messages for genesis validation (#21701)
  build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.1 to 1.0.0-alpha.2 (#21698)
  test: migrate e2e/bank to system tests (#21607)
  chore: fix the gci lint issue in testutil (#21695)
  docs(x/authz): update grant docs (#21677)
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

simsx/msg_factory.go Dismissed Show dismissed Hide dismissed
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@github-actions github-actions bot added the C:x/genutil genutil module issues label Sep 16, 2024
@alpe alpe added this pull request to the merge queue Sep 16, 2024
Merged via the queue into main with commit bf77680 Sep 16, 2024
86 of 87 checks passed
@alpe alpe deleted the alex/sims2_main branch September 16, 2024 13:48
mergify bot pushed a commit that referenced this pull request Sep 16, 2024
(cherry picked from commit bf77680)

# Conflicts:
#	CHANGELOG.md
#	go.mod
#	simapp/sim_bench_test.go
#	testutil/sims/state_helpers.go
#	x/params/go.mod
#	x/staking/go.mod
@tac0turtle tac0turtle mentioned this pull request Sep 16, 2024
@coderabbitai coderabbitai bot mentioned this pull request Sep 16, 2024
12 tasks
@julienrbrt julienrbrt mentioned this pull request Sep 16, 2024
12 tasks
julienrbrt added a commit that referenced this pull request Sep 17, 2024
…#21752)

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
@coderabbitai coderabbitai bot mentioned this pull request Sep 27, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants