refactor(sims): Introduce new sims method factory and helper structures #21037
Conversation
WalkthroughWalkthroughThe recent changes significantly enhance the simulation framework within the Cosmos SDK by optimizing testing capabilities, account management, and message delivery processes. Key improvements include the addition of new interfaces and methods, refined error handling, and better logging and reporting mechanisms. These updates result in a more modular, maintainable, and flexible testing environment that allows for improved management of simulation behaviors and outcomes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SimulationManager
participant AccountSource
participant BalanceSource
participant Logger
User->>+SimulationManager: Initiate simulation
SimulationManager->>+AccountSource: Fetch account info
AccountSource-->>-SimulationManager: Return account info
SimulationManager->>+BalanceSource: Fetch balance info
BalanceSource-->>-SimulationManager: Return balance info
SimulationManager->>Logger: Log simulation stats
Logger-->>-SimulationManager: Confirmation
SimulationManager-->>-User: Simulation results
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
bot
removed
C:x/staking
C:x/gov
C:x/slashing
C:x/distribution
alpe
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (5)
x/simulation/simulate_test.go (1)
15-57: Add a comment explaining the purpose ofrunQueuedTimeOperations.The test function is well-structured and covers multiple scenarios. However, adding a brief comment explaining the purpose of
runQueuedTimeOperationswould enhance readability.+ // runQueuedTimeOperations processes queued operations and returns the number of executed operations and future operations.simsx/environment_test.go (2)
13-50: Add a comment explaining the purpose ofAnyAccount.The test function is well-structured and covers multiple scenarios. However, adding a brief comment explaining the purpose of
AnyAccountwould enhance readability.+ // AnyAccount returns a random account from the chain data source, optionally filtered by the provided filters.
52-67: Add a comment explaining the purpose ofGetAccountandHasAccount.The test function is well-structured and covers both existing and non-existing accounts. However, adding a brief comment explaining the purpose of
GetAccountandHasAccountwould enhance readability.+ // GetAccount returns the account with the given address from the chain data source. + // HasAccount checks if the chain data source contains an account with the given address.x/simulation/operation.go (1)
Action Required: Incorrect function call to
queueOperationsThe function
queueOperationsis called with an incorrect argument type at the following location:
x/simulation/simulate.go: queueOperations(operationQueue, timeOperationQueue, futureOps)Please update this call to pass a pointer to a slice of
simulation.FutureOperation:queueOperations(operationQueue, &timeOperationQueue, futureOps)
Analysis chain
Line range hint
79-107:
LGTM! But verify the function usage in the codebase.The change to use a pointer to a slice improves efficiency by avoiding unnecessary copying. Ensure that all function calls to
queueOperationspass a pointer to a slice ofsimulation.FutureOperation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `queueOperations` pass a pointer to a slice of `simulation.FutureOperation`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'queueOperations'Length of output: 1277
types/module/simulation.go (1)
30-33: Issues Found Due to Method RemovalThe
WeightedOperationsmethod is still referenced in various parts of the codebase. Removing this method from theHasLegacyWeightedOperationsinterface intypes/module/simulation.gowill cause issues in the following files:
x/authz/simulation/operations.gox/authz/module/module.gox/slashing/module.gox/slashing/simulation/operations.gox/staking/simulation/operations.gox/protocolpool/depinject.gox/staking/depinject.gox/params/module.gox/nft/module/module.gox/group/simulation/operations.gox/group/simulation/operations_test.gox/gov/module.gox/group/module/module.gox/gov/simulation/operations.gox/nft/simulation/operations.gox/mint/module.gox/feegrant/simulation/operations.gox/distribution/module.gox/distribution/simulation/operations.gox/epochs/module.gox/evidence/module.gox/feegrant/module/depinject.gox/auth/module.gox/bank/simulation/operations.gox/bank/module.gotestutils/sims/runner.gotests/integration/staking/simulation/operations_test.goPlease ensure these references are updated or removed accordingly.
Analysis chain
Verify Impact of Method Removal
The
WeightedOperationsmethod has been removed from theSimulationManager. Ensure that this removal does not impact other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `WeightedOperations` method. # Test: Search for the method usage. Expect: No occurrences of the removed method. rg --type go -A 5 $'WeightedOperations'Length of output: 29775
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (31)
- scripts/build/simulations.mk (3 hunks)
- simapp/app.go (1 hunks)
- simapp/app_di.go (1 hunks)
- simapp/sim_bench_test.go (2 hunks)
- simapp/sim_test.go (5 hunks)
- simsx/common_test.go (1 hunks)
- simsx/delivery.go (1 hunks)
- simsx/delivery_test.go (1 hunks)
- simsx/environment.go (1 hunks)
- simsx/environment_test.go (1 hunks)
- simsx/msg_factory.go (1 hunks)
- simsx/msg_factory_test.go (1 hunks)
- simsx/params.go (1 hunks)
- simsx/registry.go (1 hunks)
- simsx/registry_test.go (1 hunks)
- simsx/reporter.go (1 hunks)
- simsx/reporter_test.go (1 hunks)
- simsx/slices.go (1 hunks)
- simsx/slices_test.go (1 hunks)
- tests/go.mod (1 hunks)
- testutil/sims/simulation_helpers.go (2 hunks)
- testutil/sims/state_helpers.go (2 hunks)
- testutils/sims/runner.go (8 hunks)
- types/module/simulation.go (6 hunks)
- types/simulation/account.go (2 hunks)
- types/simulation/config.go (1 hunks)
- x/simulation/client/cli/flags.go (3 hunks)
- x/simulation/log.go (1 hunks)
- x/simulation/operation.go (2 hunks)
- x/simulation/simulate.go (14 hunks)
- x/simulation/simulate_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- tests/go.mod
- x/simulation/log.go
Additional context used
Path-based instructions (28)
simapp/sim_bench_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"simsx/slices_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"simsx/slices.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/simulation/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simsx/params.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simsx/msg_factory_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"x/simulation/simulate_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"simsx/environment_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"types/simulation/account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simsx/delivery_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"x/simulation/client/cli/flags.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simsx/delivery.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/simulation/operation.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simsx/msg_factory.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/common_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"simsx/reporter_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"types/module/simulation.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simsx/registry.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simsx/registry_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"simsx/reporter.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/sim_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"testutil/sims/state_helpers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.testutils/sims/runner.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/simulation/simulate.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simsx/environment.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (110)
simapp/sim_bench_test.go (1)
21-21: LGTM! Verify the correctness ofsims.RunWithSeedusage.The refactoring simplifies the benchmark setup and improves readability. Ensure that
sims.RunWithSeedcorrectly encapsulates the necessary setup logic.Verification successful
Verified the correctness of
sims.RunWithSeedusage.The
RunWithSeedfunction correctly encapsulates the necessary setup logic, including environment setup, simulation execution, and post-run actions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `sims.RunWithSeed` usage. # Test: Search for the `RunWithSeed` function implementation. Expect: Correct encapsulation of setup logic. rg --type go -A 10 'func RunWithSeed' | grep -A 10 'RunWithSeed'Length of output: 1309
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the `RunWithSeed` function. # Test: Extract the full implementation of the `RunWithSeed` function. rg --type go -A 50 'func RunWithSeed' testutils/sims/runner.goLength of output: 2745
simsx/slices_test.go (1)
1-40: Good test coverage!The unit tests cover the
Collect,First, andOneOffunctions effectively. Ensure that edge cases are also considered in future tests.simsx/slices.go (1)
1-38: Well-documented and clean code!The helper functions for slice operations are well-documented and follow the Uber Golang style guide. Ensure to handle edge cases and panics as mentioned in the comments.
types/simulation/config.go (4)
28-29: LGTM! RenamingTtoTBimproves clarity.The renaming enhances the clarity regarding the type of this field.
29-29: LGTM! AddingFauxMerkleenhances configuration capabilities.The addition of this field expands the configuration options available.
37-40: LGTM! Changing the parameter totesting.TBincreases flexibility.This change allows the method to accept both
*testing.Tand*testing.B, enhancing its versatility.
40-40: Verify the usage of theWithmethod in the codebase.Ensure all calls to the
Withmethod pass the correct type of parameter.Verification successful
The
Withmethod is used correctly throughout the codebase. The method is designed to accept variadic parameters of typeinterface{}, allowing for flexibility in the types of parameters passed.
- The method definitions confirm that it can accept any type of parameter.
- The instances found in the codebase primarily pass strings and key-value pairs, which are appropriate for logging contexts.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to the `With` method pass the correct type of parameter. # Test: Search for the method usage. Expect: Only occurances of the new signature. rg --type go -A 5 $'.With('Length of output: 91
Script:
#!/bin/bash # Description: Verify all calls to the `With` method pass the correct type of parameter. # Test: Search for the method usage. Expect: Only occurrences of the new signature. rg --type go -A 5 '\.With\('Length of output: 25234
Script:
#!/bin/bash # Description: Find the definition of the `With` method to verify its parameter types. # Search for the method definition in Go files. rg --type go 'func (.*) With\(' -A 5Length of output: 2274
simsx/params.go (3)
10-12: LGTM! TheWeightSourceinterface is simple and clear.The interface defines a single method for retrieving weights, adhering to the Uber Golang style guide.
20-24: LGTM! TheWeightSourceFntype and its methodGetare correctly implemented.The function adapter correctly implements the
WeightSourceinterface.
44-51: LGTM! TheParamWeightSourcefunction is well-documented and correctly implemented.The function provides a clear and correct implementation for retrieving weights.
simsx/msg_factory_test.go (1)
14-52: LGTM! TheTestMsgFactoriesfunction covers various scenarios and uses assertions correctly.The test cases are well-structured and verify the expected outcomes.
Verify sufficient code coverage.
Ensure that the tests provide sufficient coverage for the changes in the pull request.
x/simulation/simulate_test.go (1)
3-13: Imports look good.The imports are appropriate for the test file.
simsx/environment_test.go (1)
3-11: Imports look good.The imports are appropriate for the test file.
types/simulation/account.go (2)
17-21: Enhancement toAccountstruct looks good.The addition of the
AddressBech32field enhances the functionality of theAccountstruct.
54-58: Modifications toRandomAccountsfunction look good.The modifications ensure that the
AddressBech32field is populated correctly.simsx/delivery_test.go (4)
15-42: Good coverage for skipped reporter scenario.The test case for "error when reporter skipped" is well-written and covers the scenario effectively. Ensure that the
SimDeliverFnfunction handles the skip correctly.
43-50: Good coverage for successful delivery scenario.The test case for "successful delivery" is well-written and covers the scenario effectively. Ensure that the
SimDeliverFnfunction handles the success correctly.
51-58: Good coverage for error delivery scenario.The test case for "error delivery" is well-written and covers the scenario effectively. Ensure that the
SimDeliverFnfunction handles the error correctly.
59-66: Good coverage for error delivery handled scenario.The test case for "error delivery handled" is well-written and covers the scenario effectively. Ensure that the
SimDeliverFnfunction handles the error correctly and that thedeliveryResultHandlerprocesses the error as expected.x/simulation/client/cli/flags.go (3)
33-33: Declare the new flagFlagFauxMerkle.The new flag
FlagFauxMerkleis declared correctly.
58-58: Add the new flag toGetSimulatorFlags.The new flag
FlagFauxMerkleis added to theGetSimulatorFlagsfunction correctly.
78-78: Include the new flag inNewConfigFromFlags.The new flag
FlagFauxMerkleis included in theNewConfigFromFlagsfunction correctly.simsx/delivery.go (4)
1-1: Package declaration.The package declaration is correct.
3-14: Imports.The imports are necessary and correctly included.
16-26: Type declarations.The type declarations are correct and necessary for the function implementation.
28-98: FunctionDeliverSimsMsgimplementation.The function
DeliverSimsMsgis well-implemented and covers the necessary logic for delivering simulation messages. The error handling and reporting are correctly implemented.simsx/msg_factory.go (4)
9-13: InterfaceSimMsgFactoryXlooks good.The interface methods are well-defined and align with the purpose of message factories.
28-56: TypeResultHandlingSimMsgFactorylooks good.The struct and its methods are well-implemented, providing functionality for handling delivery results.
63-81: TypeLazyStateSimMsgFactorylooks good.The struct and its methods are well-implemented, providing functionality for handling future operations.
90-112: TypeSimMsgFactoryFnlooks good.The type alias and its methods are well-defined, ensuring successful message delivery.
testutil/sims/simulation_helpers.go (1)
49-52: Refactor ofPrintStatsfunction looks good.The change enhances the function's usability by allowing it to integrate with various logging mechanisms.
scripts/build/simulations.mk (12)
5-5: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
19-19: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
25-25: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
30-30: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
35-35: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
42-42: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
47-47: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
52-53: LGTM! The addition of-failfastand-FauxMerkle=trueflags are correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure, and the-FauxMerkle=trueflag will influence the test's behavior regarding Merkle tree verification.
57-57: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
80-80: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
85-85: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.
91-91: LGTM! The addition of-failfastflag is correct.The
-failfastflag will help in getting quicker feedback by stopping the tests on the first failure.simsx/common_test.go (5)
24-35: LGTM! The functionSimAccountFixtureis correct.The function creates a
SimAccountwith optional mutators and follows best practices.
38-46: LGTM! The functionMemoryAccountSourceis correct.The function creates an
AccountSourceFnfrom a list ofSimAccountand follows best practices.
49-62: LGTM! The functiontxConfigis correct.The function creates a
client.TxConfigwith custom options and follows best practices.
136-154: LGTM! The typeMockAccountSourceXis correct.The type implements
AccountSourceXwith mock methods for testing and follows best practices.
156-161: LGTM! The functionmustis correct.The function panics if an error is encountered and follows best practices.
simsx/reporter_test.go (4)
14-70: LGTM! The test functionTestSimulationReporterToLegacyis correct.The test function covers various scenarios for converting
SimulationReporterto legacy operation messages and follows best practices.
73-134: LGTM! The test functionTestSimulationReporterTransitionsis correct.The test function covers various state transitions of
SimulationReporterand follows best practices.
136-156: LGTM! The test functionTestSkipHookis correct.The test function covers the skip hook functionality of
SimulationReporterand follows best practices.
158-217: LGTM! The test functionTestReporterSummaryis correct.The test function covers the summary generation of
SimulationReporterand follows best practices.types/module/simulation.go (5)
50-57: New Interfaces ApprovedThe
AccountSourceandBalanceSourceinterfaces enhance modularity and flexibility by providing methods to retrieve account information and spendable coins.
34-46: New Interfaces ApprovedThe
HasLegacyProposalMsgsandHasLegacyProposalContentsinterfaces standardize the simulation of governance proposals, replacing deprecated functionality.
34-56: New Types ApprovedThe
regCommonandAbstractRegistrytypes enhance code reuse and modularity by providing common functionality for registries.
72-77: Function Signature UpdatedThe
NewSimulationManagerfunction now acceptsAccountSourceandBalanceSourceparameters, enhancing the setup of the simulation manager.Ensure that all calls to
NewSimulationManagerare updated to match the new signature.Verification successful
Function Signature Updated
The
NewSimulationManagerfunction now acceptsAccountSourceandBalanceSourceparameters, enhancing the setup of the simulation manager.
- All calls to
NewSimulationManagerhave been updated to match the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewSimulationManager` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewSimulationManager'Length of output: 3417
Line range hint
87-109:
Function Signature UpdatedThe
NewSimulationManagerFromAppModulesfunction now acceptsAccountSourceandBalanceSourceparameters, enhancing the setup of the simulation manager.Ensure that all calls to
NewSimulationManagerFromAppModulesare updated to match the new signature.Verification successful
Function Signature Updated
The
NewSimulationManagerFromAppModulesfunction now acceptsAccountSourceandBalanceSourceparameters, enhancing the setup of the simulation manager.All calls to
NewSimulationManagerFromAppModuleshave been updated to match the new signature:
simapp/app.gosimapp/app_di.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewSimulationManagerFromAppModules` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewSimulationManagerFromAppModules'Length of output: 1943
simsx/registry.go (6)
17-27: New Interfaces ApprovedThe
Registry,FutureOpsRegistry, andAccountSourceXinterfaces enhance modularity and flexibility by providing methods to register message factories and manage accounts.
34-56: New Types ApprovedThe
regCommonandAbstractRegistrytypes enhance code reuse and modularity by providing common functionality for registries.
58-96: New Type and Functions ApprovedThe
WeightedOperationRegistryAdaptertype and its functions enhance the integration of new message factories with the legacy simulation system.
102-121: New Function ApprovedThe
legacyOperationAdapterfunction ensures compatibility between new message factories and the legacy simulation system.
123-141: New Type and Functions ApprovedThe
FutureOperationRegistryAdaptertype and its functions enhance the management of future operations in the simulation framework.
143-188: New Type and Functions ApprovedThe
SimsProposalRegistryAdaptertype and its functions enhance the integration of new proposal message factories with the legacy simulation system.simsx/registry_test.go (3)
118-172: Test Function ApprovedThe
TestSimsProposalRegistryAdapterfunction covers various scenarios, including successful execution, skipping execution, and error handling.Ensure that the test coverage is sufficient for the new functionality.
174-215: Test Function ApprovedThe
TestRunWithFailFastfunction covers various scenarios, including successful execution, skipping execution, and handling panics.Ensure that the test coverage is sufficient for the new functionality.
Verification successful
Test Function Approved
The
TestRunWithFailFastfunction is adequately covered by the tests in thesimsx/registry_test.gofile.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage for the new functionality. # Test: Run the tests and generate a coverage report. Expect: High coverage for the new functionality. go test ./simsx -coverprofile=coverage.out go tool cover -html=coverage.out -o coverage.htmlLength of output: 236
Script:
#!/bin/bash # Search for test functions in the Go codebase ast-grep --lang go --pattern 'func Test$_($_ *testing.T) { $$$ }' # Check for the usage of the `runWithFailFast` function in the test files rg 'runWithFailFast'Length of output: 545
20-115: Test Function ApprovedThe
TestSimsMsgRegistryAdapterfunction covers various scenarios, including successful execution, skipping execution, future ops registration, and error handling.Ensure that the test coverage is sufficient for the new functionality.
simsx/reporter.go (16)
19-34: Interface definition looks good.The
SimulationReporterinterface is well-defined and covers essential methods for reporting simulation results.
72-86: Struct definition looks good.The
BasicSimulationReporterstructure is well-defined and includes necessary fields for simulation reporting.
88-99: Constructor looks good.The
NewBasicSimulationReportermethod correctly initializes aBasicSimulationReporterinstance with optional skip hooks.
101-124: Method definition looks good.The
WithScopemethod correctly creates a newSimulationReporterinstance with additional scope.
126-128: Method definition looks good.The
Skipmethod correctly sets the status to skipped with a comment.
130-132: Method definition looks good.The
Skipfmethod correctly sets the status to skipped with a formatted comment.
134-136: Method definition looks good.The
IsSkippedmethod correctly checks if the status is skipped or completed.
138-154: Method definition looks good.The
ToLegacyOperationMsgmethod correctly converts the current status to a legacy operation message. Ensure proper error handling in case of operation abortion.
157-164: Method definition looks good.The
Failmethod correctly sets the status to completed with an error.
166-180: Method definition looks good.The
Successmethod correctly sets the status to completed with a success message. Ensure proper handling of proto bytes.
182-187: Method definition looks good.The
Closemethod correctly returns the error captured on fail.
189-208: Method definition looks good.The
toStatusmethod correctly transitions the status with comments and triggers skip callbacks if needed. Ensure the use ofpanicis justified.
221-225: Struct definition looks good.The
ExecutionSummarystructure is well-defined and includes necessary fields for managing execution summaries.
231-245: Method definition looks good.The
Addmethod correctly adds a new entry to the execution summary.
247-260: Method definition looks good.The
Stringmethod correctly returns a string representation of the execution summary.
262-268: Method definition looks good.The
summethod correctly returns the sum of integer values in a slice.simapp/sim_test.go (6)
71-71: Verify the use oftesting.TB.Ensure that the use of
testing.TBinstead of*testing.Tis justified and does not introduce any issues.
113-113: Verify the use oftesting.TB.Ensure that the use of
testing.TBinstead of*testing.Tis justified and does not introduce any issues.
186-186: Verify the use oftesting.TB.Ensure that the use of
testing.TBinstead of*testing.Tis justified and does not introduce any issues.
222-222: Verify the use oftesting.TB.Ensure that the use of
testing.TBinstead of*testing.Tis justified and does not introduce any issues.
187-187: Verify the use oftesting.TB.Ensure that the use of
testing.TBinstead of*testing.Tis justified and does not introduce any issues.
Line range hint
272-278: Function definition looks good.The
FuzzFullAppSimulationfunction correctly fuzz tests the full app simulation.testutil/sims/state_helpers.go (2)
254-273: Verify the use of_for the unused parameter.Ensure that the use of
_for the unusedio.Readerparameter is justified and does not introduce any issues.
275-307: Function definition looks good.The
AccountsFromAppStatefunction correctly extracts accounts from the app state.testutils/sims/runner.go (8)
75-75: LGTM! The parameter type change forpostRunActionsbroadens the scope of the testing interface.This change allows the use of any testing type that implements the
testing.TBinterface, increasing versatility.
103-103: LGTM! The parameter type change forpostRunActionsbroadens the scope of the testing interface.This change allows the use of any testing type that implements the
testing.TBinterface, increasing versatility.
117-153: LGTM! The new functionRunWithSeedimproves readability and maintainability.This function encapsulates the logic for running a single simulation run, separating concerns and enhancing clarity.
156-158: LGTM! The new interfaceHasWeightedOperationsXfacilitates the integration of weighted operations.This interface defines a contract for modules participating in the simulation, enhancing the capabilities for simulating complex behaviors.
159-162: LGTM! The new interfaceHasWeightedOperationsXWithProposalsfacilitates the integration of weighted operations and proposals.This interface defines a contract for modules participating in the simulation with proposals, enhancing the capabilities for simulating complex behaviors.
163-165: LGTM! The new interfaceHasProposalMsgsXfacilitates the integration of proposal messages.This interface defines a contract for modules participating in the simulation with proposal messages, enhancing the capabilities for simulating complex behaviors.
169-172: LGTM! The new interfaceHasLegacyWeightedOperationsfacilitates the integration of legacy weighted operations.This interface defines a contract for modules participating in the simulation with legacy weighted operations, enhancing the capabilities for simulating complex behaviors.
175-178: LGTM! The new interfaceHasLegacyProposalMsgsfacilitates the integration of legacy proposal messages.This interface defines a contract for modules participating in the simulation with legacy proposal messages, enhancing the capabilities for simulating complex behaviors.
x/simulation/simulate.go (5)
34-39: LGTM! The parameter type changes improve clarity.Using
simtypesinstead ofsimulationexplicitly indicates that these types are related to simulation functionality.
68-72: LGTM! The parameter type changes improve clarity.Using
simtypesinstead ofsimulationexplicitly indicates that these types are related to simulation functionality.
88-92: LGTM! The parameter type changes improve clarity.Using
simtypesinstead ofsimulationexplicitly indicates that these types are related to simulation functionality.
362-366: LGTM! The parameter type changes improve clarity and the error handling improvements enhance robustness.Using
simtypesinstead ofsimulationexplicitly indicates that these types are related to simulation functionality. The error handling improvements ensure that logs are printed upon encountering errors.
399-404: LGTM! The parameter type changes improve clarity and the error handling improvements enhance robustness.Using
simtypesinstead ofsimulationexplicitly indicates that these types are related to simulation functionality. The error handling improvements ensure that logs are printed upon encountering errors.simapp/app_di.go (1)
280-280: LGTM! The parameter list modification enhances the capability of the simulation manager.Including
app.AuthKeeperandapp.BankKeeperenhances the simulation manager's capability to manage simulations involving authentication and banking functionalities.simsx/environment.go (5)
18-29: LGTM!The
contextAwareBalanceSourcestruct and its methods are well-implemented.
32-45: LGTM! But verify the randomness quality.The
SimAccountstruct and its methods are well-implemented. Ensure that therandpackage provides sufficient randomness for your use case.
59-66: LGTM!The
CoinsFilterinterface andCoinsFilterFnfunction type are well-implemented.
80-99: LGTM!The
statefulCoinsFilterstruct and its methods are well-implemented.
386-455: LGTM!The
XRandstruct and its methods are well-implemented.simapp/app.go (1)
539-539: LGTM!The changes to the
NewSimAppfunction enhance the simulation manager's capabilities by includingAuthKeeperandBankKeeper. The overall control flow remains intact.
| type ChainDataSource struct { | ||
| r *rand.Rand | ||
| addressToAccountsPosIndex map[string]int | ||
| accounts []SimAccount | ||
| accountSource ModuleAccountSource | ||
| addressCodec address.Codec | ||
| bank contextAwareBalanceSource | ||
| } | ||
|
|
||
| // NewChainDataSource constructor | ||
| func NewChainDataSource( | ||
| ctx context.Context, | ||
| r *rand.Rand, | ||
| ak ModuleAccountSource, | ||
| bk BalanceSource, | ||
| codec address.Codec, | ||
| oldSimAcc ...simtypes.Account, | ||
| ) *ChainDataSource { | ||
| acc := make([]SimAccount, len(oldSimAcc)) | ||
| index := make(map[string]int, len(oldSimAcc)) | ||
| bank := contextAwareBalanceSource{ctx: ctx, bank: bk} | ||
| for i, a := range oldSimAcc { | ||
| acc[i] = SimAccount{Account: a, r: r, bank: bank} | ||
| index[a.AddressBech32] = i | ||
| } | ||
| return &ChainDataSource{ | ||
| r: r, | ||
| accountSource: ak, | ||
| addressCodec: codec, | ||
| accounts: acc, | ||
| bank: bank, | ||
| addressToAccountsPosIndex: index, | ||
| } |
There was a problem hiding this comment.
Revisit the use of panic in ChainDataSource.
The ChainDataSource struct and its methods are well-implemented. However, the use of panic in some methods should be revisited to ensure proper error handling.
- panic(err)
+ reporter.Skipf("failed to initialize chain data source: %s", err)Also applies to: 280-384
simsx/environment.go
Outdated
| return statefulCoinsFilterFn(func(s *SimAccount, coins sdk.Coins) bool { | ||
| for _, coin := range coins { | ||
| if !s.bank.IsSendEnabledDenom(coin.Denom) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| }) | ||
| } | ||
|
|
||
| // filter with context of SimAccount | ||
| type statefulCoinsFilter struct { | ||
| s *SimAccount | ||
| do func(s *SimAccount, c sdk.Coins) bool | ||
| } | ||
|
|
||
| // constructor | ||
| func statefulCoinsFilterFn(f func(s *SimAccount, c sdk.Coins) bool) CoinsFilter { | ||
| return &statefulCoinsFilter{do: f} | ||
| } | ||
|
|
||
| func (f statefulCoinsFilter) Accept(c sdk.Coins) bool { | ||
| if f.s == nil { | ||
| panic("account not set") | ||
| } | ||
| return f.do(f.s, c) | ||
| } | ||
|
|
||
| func (f *statefulCoinsFilter) visit(s *SimAccount) { | ||
| f.s = s | ||
| } | ||
|
|
||
| var _ visitable = &statefulCoinsFilter{} | ||
|
|
||
| type visitable interface { | ||
| visit(s *SimAccount) | ||
| } | ||
|
|
||
| // RandSubsetCoins return random amounts from the current balance. When the coins are empty, skip is called on the reporter. | ||
| // The amounts are removed from the liquid balance. | ||
| func (b *SimsAccountBalance) RandSubsetCoins(reporter SimulationReporter, filters ...CoinsFilter) sdk.Coins { | ||
| amount := b.randomAmount(5, reporter, b.Coins, filters...) | ||
| b.Coins = b.Coins.Sub(amount...) | ||
| if amount.Empty() { | ||
| reporter.Skip("got empty amounts") | ||
| } | ||
| return amount | ||
| } | ||
|
|
||
| // RandSubsetCoin return random amount from the current balance. When the coins are empty, skip is called on the reporter. | ||
| // The amount is removed from the liquid balance. | ||
| func (b *SimsAccountBalance) RandSubsetCoin(reporter SimulationReporter, denom string, filters ...CoinsFilter) sdk.Coin { | ||
| ok, coin := b.Find(denom) | ||
| if !ok { | ||
| reporter.Skipf("no such coin: %s", denom) | ||
| return sdk.NewCoin(denom, math.ZeroInt()) | ||
| } | ||
| amounts := b.randomAmount(5, reporter, sdk.Coins{coin}, filters...) | ||
| if amounts.Empty() { | ||
| reporter.Skip("empty coin") | ||
| return sdk.NewCoin(denom, math.ZeroInt()) | ||
| } | ||
| b.BlockAmount(amounts[0]) | ||
| return amounts[0] | ||
| } | ||
|
|
||
| // BlockAmount returns true when balance is > requested amount and subtracts the amount from the liquid balance | ||
| func (b *SimsAccountBalance) BlockAmount(amount sdk.Coin) bool { | ||
| ok, coin := b.Coins.Find(amount.Denom) | ||
| if !ok || !coin.IsPositive() || !coin.IsGTE(amount) { | ||
| return false | ||
| } | ||
| b.Coins = b.Coins.Sub(amount) | ||
| return true | ||
| } | ||
|
|
||
| func (b *SimsAccountBalance) randomAmount(retryCount int, reporter SimulationReporter, coins sdk.Coins, filters ...CoinsFilter) sdk.Coins { | ||
| if retryCount < 0 || b.Coins.Empty() { | ||
| reporter.Skip("failed to find matching amount") | ||
| return sdk.Coins{} | ||
| } | ||
| amount := simtypes.RandSubsetCoins(b.r, coins) | ||
| for _, filter := range filters { | ||
| if f, ok := filter.(visitable); ok { | ||
| f.visit(b.owner) | ||
| } | ||
| if !filter.Accept(amount) { | ||
| return b.randomAmount(retryCount-1, reporter, coins, filters...) | ||
| } | ||
| } | ||
| return amount | ||
| } | ||
|
|
||
| func (b *SimsAccountBalance) RandFees() sdk.Coins { | ||
| amount, err := simtypes.RandomFees(b.r, b.Coins) | ||
| if err != nil { | ||
| panic(err.Error()) // todo: setup a better way to abort execution | ||
| } // todo: revisit the panic | ||
| return amount | ||
| } |
There was a problem hiding this comment.
Revisit the use of panic in RandFees.
The SimsAccountBalance struct and its methods are well-implemented. However, the use of panic in RandFees should be revisited to ensure proper error handling.
- panic(err.Error()) // todo: setup a better way to abort execution
+ reporter.Skipf("failed to generate random fees: %s", err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type SimsAccountBalance struct { | |
| sdk.Coins | |
| owner *SimAccount | |
| r *rand.Rand | |
| } | |
| // NewSimsAccountBalance constructor | |
| func NewSimsAccountBalance(o *SimAccount, r *rand.Rand, coins sdk.Coins) *SimsAccountBalance { | |
| return &SimsAccountBalance{Coins: coins, r: r, owner: o} | |
| } | |
| type CoinsFilter interface { | |
| Accept(c sdk.Coins) bool // returns false to reject | |
| } | |
| type CoinsFilterFn func(c sdk.Coins) bool | |
| func (f CoinsFilterFn) Accept(c sdk.Coins) bool { | |
| return f(c) | |
| } | |
| func WithSendEnabledCoins() CoinsFilter { | |
| return statefulCoinsFilterFn(func(s *SimAccount, coins sdk.Coins) bool { | |
| for _, coin := range coins { | |
| if !s.bank.IsSendEnabledDenom(coin.Denom) { | |
| return false | |
| } | |
| } | |
| return true | |
| }) | |
| } | |
| // filter with context of SimAccount | |
| type statefulCoinsFilter struct { | |
| s *SimAccount | |
| do func(s *SimAccount, c sdk.Coins) bool | |
| } | |
| // constructor | |
| func statefulCoinsFilterFn(f func(s *SimAccount, c sdk.Coins) bool) CoinsFilter { | |
| return &statefulCoinsFilter{do: f} | |
| } | |
| func (f statefulCoinsFilter) Accept(c sdk.Coins) bool { | |
| if f.s == nil { | |
| panic("account not set") | |
| } | |
| return f.do(f.s, c) | |
| } | |
| func (f *statefulCoinsFilter) visit(s *SimAccount) { | |
| f.s = s | |
| } | |
| var _ visitable = &statefulCoinsFilter{} | |
| type visitable interface { | |
| visit(s *SimAccount) | |
| } | |
| // RandSubsetCoins return random amounts from the current balance. When the coins are empty, skip is called on the reporter. | |
| // The amounts are removed from the liquid balance. | |
| func (b *SimsAccountBalance) RandSubsetCoins(reporter SimulationReporter, filters ...CoinsFilter) sdk.Coins { | |
| amount := b.randomAmount(5, reporter, b.Coins, filters...) | |
| b.Coins = b.Coins.Sub(amount...) | |
| if amount.Empty() { | |
| reporter.Skip("got empty amounts") | |
| } | |
| return amount | |
| } | |
| // RandSubsetCoin return random amount from the current balance. When the coins are empty, skip is called on the reporter. | |
| // The amount is removed from the liquid balance. | |
| func (b *SimsAccountBalance) RandSubsetCoin(reporter SimulationReporter, denom string, filters ...CoinsFilter) sdk.Coin { | |
| ok, coin := b.Find(denom) | |
| if !ok { | |
| reporter.Skipf("no such coin: %s", denom) | |
| return sdk.NewCoin(denom, math.ZeroInt()) | |
| } | |
| amounts := b.randomAmount(5, reporter, sdk.Coins{coin}, filters...) | |
| if amounts.Empty() { | |
| reporter.Skip("empty coin") | |
| return sdk.NewCoin(denom, math.ZeroInt()) | |
| } | |
| b.BlockAmount(amounts[0]) | |
| return amounts[0] | |
| } | |
| // BlockAmount returns true when balance is > requested amount and subtracts the amount from the liquid balance | |
| func (b *SimsAccountBalance) BlockAmount(amount sdk.Coin) bool { | |
| ok, coin := b.Coins.Find(amount.Denom) | |
| if !ok || !coin.IsPositive() || !coin.IsGTE(amount) { | |
| return false | |
| } | |
| b.Coins = b.Coins.Sub(amount) | |
| return true | |
| } | |
| func (b *SimsAccountBalance) randomAmount(retryCount int, reporter SimulationReporter, coins sdk.Coins, filters ...CoinsFilter) sdk.Coins { | |
| if retryCount < 0 || b.Coins.Empty() { | |
| reporter.Skip("failed to find matching amount") | |
| return sdk.Coins{} | |
| } | |
| amount := simtypes.RandSubsetCoins(b.r, coins) | |
| for _, filter := range filters { | |
| if f, ok := filter.(visitable); ok { | |
| f.visit(b.owner) | |
| } | |
| if !filter.Accept(amount) { | |
| return b.randomAmount(retryCount-1, reporter, coins, filters...) | |
| } | |
| } | |
| return amount | |
| } | |
| func (b *SimsAccountBalance) RandFees() sdk.Coins { | |
| amount, err := simtypes.RandomFees(b.r, b.Coins) | |
| if err != nil { | |
| panic(err.Error()) // todo: setup a better way to abort execution | |
| } // todo: revisit the panic | |
| return amount | |
| } | |
| func (b *SimsAccountBalance) RandFees(reporter SimulationReporter) sdk.Coins { | |
| amount, err := simtypes.RandomFees(b.r, b.Coins) | |
| if err != nil { | |
| reporter.Skipf("failed to generate random fees: %s", err) | |
| return sdk.Coins{} // return an empty Coins object to handle the error case gracefully | |
| } // todo: revisit the panic | |
| return amount | |
| } |
| type FakeAccountI struct { | ||
| SimAccount | ||
| id, seq uint64 | ||
| } | ||
|
|
||
| func (m FakeAccountI) GetAddress() sdk.AccAddress { | ||
| return m.Address | ||
| } | ||
|
|
||
| func (m FakeAccountI) GetPubKey() cryptotypes.PubKey { | ||
| return m.PubKey | ||
| } | ||
|
|
||
| func (m FakeAccountI) GetAccountNumber() uint64 { | ||
| return m.id | ||
| } | ||
|
|
||
| func (m FakeAccountI) GetSequence() uint64 { | ||
| return m.seq | ||
| } | ||
|
|
||
| func (m FakeAccountI) Reset() { | ||
| panic("implement me") | ||
| } | ||
|
|
||
| func (m FakeAccountI) String() string { | ||
| panic("implement me") | ||
| } | ||
|
|
||
| func (m FakeAccountI) ProtoMessage() { | ||
| panic("implement me") | ||
| } | ||
|
|
||
| func (m FakeAccountI) SetAddress(address sdk.AccAddress) error { | ||
| panic("implement me") | ||
| } | ||
|
|
||
| func (m FakeAccountI) SetPubKey(key cryptotypes.PubKey) error { | ||
| panic("implement me") | ||
| } | ||
|
|
||
| func (m FakeAccountI) SetAccountNumber(u uint64) error { | ||
| panic("implement me") | ||
| } | ||
|
|
||
| func (m FakeAccountI) SetSequence(u uint64) error { | ||
| panic("implement me") | ||
| } |
There was a problem hiding this comment.
Avoid methods that panic in FakeAccountI.
While the type FakeAccountI is correct, methods like Reset, String, ProtoMessage, SetAddress, SetPubKey, SetAccountNumber, and SetSequence should ideally not panic. Consider providing meaningful implementations or documenting why they panic.
tac0turtle
assigned JulianToledano, facundomedica, tac0turtle, julienrbrt, samricotta and sontrinh16
| @@ -0,0 +1,98 @@ | |||
| package simsx | |||
There was a problem hiding this comment.
i dont follow the x suffix, it feels odd
* main: feat(log): remove core dependency and update core interface to be dependency free (#21045) chore: fix some comments (#21085) feat: simulate nested messages (#20291) chore(network): remove `DefaultConfigWithAppConfigWithQueryGasLimit` (#21055) fix(runtime): remove `appv1alpha1.Config` from runtime (#21042) feat(simapp/v2): Add store server to testnet init cmd (#21076) chore(indexer/postgres): update to changes on main (#21077) feat(schema/appdata): async listener mux'ing (#20879) ci: Use large box for 052 branch sims on CI (#21067) chore(all): replace all `fmt.Errorf` without paramters with `errors.New` (#21068)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- simapp/app.go (1 hunks)
- simapp/app_di.go (1 hunks)
- tests/go.mod (1 hunks)
- testutils/sims/runner.go (8 hunks)
- types/simulation/account.go (2 hunks)
- x/simulation/simulate.go (14 hunks)
Files skipped from review as they are similar to previous changes (4)
- simapp/app.go
- tests/go.mod
- types/simulation/account.go
- x/simulation/simulate.go
Additional context used
Path-based instructions (2)
testutils/sims/runner.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app_di.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (9)
testutils/sims/runner.go (8)
117-153: New function added:RunWithSeed.The addition of this function improves readability and maintainability by encapsulating the logic for a single simulation run.
156-158: New interface added:HasWeightedOperationsX.This interface defines a contract for modules to provide weighted operations for simulations.
159-162: New interface added:HasWeightedOperationsXWithProposals.This interface defines a contract for modules to provide weighted operations and proposals for simulations.
163-165: New interface added:HasProposalMsgsX.This interface defines a contract for modules to provide proposal messages for simulations.
169-171: New interface added:HasLegacyWeightedOperations.This interface defines a contract for modules to provide legacy weighted operations for simulations.
175-177: New interface added:HasLegacyProposalMsgs.This interface defines a contract for modules to provide legacy proposal messages for simulations.
75-75: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
Runmatch the new signature.
103-103: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
RunWithSeedsmatch the new signature.simapp/app_di.go (1)
279-279: Enhanced SimulationManager instantiation.The addition of
app.AuthKeeperandapp.BankKeeperas parameters toNewSimulationManagerFromAppModuleslikely enhances the simulation manager's capability to manage simulations involving authentication and banking functionalities.
discussed offline the deprecation/cleanup will come later, this was done to split prs |
There was a problem hiding this comment.
Great work! Looks good to me.
It would be super useful if you copy/paste your PR description in a README.md in simsx. I agree that the name is weird, but not having it under x/ makes more sense.
Do we backport this to v0.52? And delete x/simulation in the next version?
Also, as said in a comment above, I think the new sims API should be in core/appmodule/v2/sims.go (aliased to v1 too).
| @@ -276,7 +276,7 @@ func NewSimApp( | |||
| overrideModules := map[string]module.AppModuleSimulation{ | |||
| authtypes.ModuleName: auth.NewAppModule(app.appCodec, app.AuthKeeper, &app.AccountsKeeper, authsims.RandomGenesisAccounts), | |||
| } | |||
| app.sm = module.NewSimulationManagerFromAppModules(app.ModuleManager.Modules, overrideModules) | |||
| app.sm = module.NewSimulationManagerFromAppModules(app.AuthKeeper, app.BankKeeper, app.ModuleManager.Modules, overrideModules) | |||
There was a problem hiding this comment.
API breaking changelog entry in main changelog, and wiring change in simapp/CHANGELOG.md
| // SetupSimulation creates the config, db (levelDB), temporary directory and logger for the simulation tests. | ||
| // If `skip` is false it skips the current test. `skip` should be set using the `FlagEnabledValue` flag. | ||
| // Returns error on an invalid db instantiation or temp dir creation. | ||
| func SetupSimulation(config simtypes.Config, dirPrefix, dbName string, verbose, skip bool) (dbm.DB, string, log.Logger, bool, error) { |
| @@ -250,52 +251,58 @@ func AppStateRandomizedFn( | |||
|
|
|||
| // AppStateFromGenesisFileFn util function to generate the genesis AppState | |||
| // from a genesis.json file. | |||
| func AppStateFromGenesisFileFn(r io.Reader, cdc codec.JSONCodec, genesisFile string) (genutiltypes.AppGenesis, []simtypes.Account, error) { | |||
| func AppStateFromGenesisFileFn(_ io.Reader, cdc codec.JSONCodec, genesisFile string) (genutiltypes.AppGenesis, []simtypes.Account, error) { | |||
There was a problem hiding this comment.
Given we have other breaking changes? Why not breaking it here too?
| } | ||
|
|
||
| type ( | ||
| HasWeightedOperationsX interface { |
| } | ||
| } | ||
|
|
||
| type ( |
There was a problem hiding this comment.
I think simulations interfaces should maybe get a place in core/appmodule(v2).
type/module is actually for legacy stuff, so the legacy interfaces can stay there but it would be cool if the new one could be in core, as it is the central place of module APIs.
| PubKey cryptotypes.PubKey | ||
| Address sdk.AccAddress | ||
| ConsKey cryptotypes.PrivKey | ||
| AddressBech32 string |
There was a problem hiding this comment.
We should store the bytes and use the address codec wherever we need this.
There was a problem hiding this comment.
The encoding had some significant performance impact. The sim accounts are not modified during the whole sims run for a seed. Therefore it made sense for me to do the encoding once in the beginning and then use this field instead of re-encoding. This field is only used by sims code and not in the app or modules. WDYT?
|
|
||
| accs = tmpAccs | ||
| accs = slices.DeleteFunc(accs, func(acc simtypes.Account) bool { | ||
| return blockedAddrs[acc.AddressBech32] |
There was a problem hiding this comment.
Here we had the address codec available, why removing it?
| @@ -191,6 +186,10 @@ func SimulateFromSeedX( | |||
| exportedParams = params | |||
| } | |||
|
|
|||
| if _, err := app.FinalizeBlock(finalizeBlockReq); err != nil { | |||
|
Good feedback! Very much appreciated |
|
Closing in favour of #21613 |
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.
* Message factory
Simple functions as factories for dedicated sdk.Msgs. They have access to the context, reporter and test data environment. For example:
* 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:
* 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
* 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.
To discuss
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
Improvements
Test Enhancements
Bug Fixes