test: add simulation tests #2947
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the application structure, focusing on modularity and simulation capabilities. Key modifications include the addition of a Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2947 +/- ##
===========================================
- Coverage 64.47% 64.06% -0.41%
===========================================
Files 409 412 +3
Lines 28641 28960 +319
===========================================
+ Hits 18465 18554 +89
- Misses 9381 9611 +230
Partials 795 795
|
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (12)
e2e/runner/admin_evm.go (2)
15-15: Approve logging statement change with minor suggestion.The modification to remove
fmt.Sprintfis correct and improves performance. However, for consistency with Go'slogpackage and to potentially leverage structured logging in the future, consider using key-value pairs for logging.Consider refactoring the logging statement as follows:
r.Logger.Info("TSS Address Update", "tx", tx.Hash().String())This approach separates the message from the dynamic value, which can be beneficial for log parsing and consistency across the codebase.
29-29: Approve logging statement change with suggestions for improvement.The modification to remove
fmt.Sprintfis correct and improves performance. However, there are opportunities for further enhancement:
For consistency with Go's
logpackage and to potentially leverage structured logging in the future, consider using key-value pairs for logging.The
UpdateTssAddressForConnectorandUpdateTssAddressForErc20custodyfunctions are very similar, suggesting an opportunity for refactoring to reduce code duplication.Consider refactoring the logging statement as follows:
r.Logger.Info("TSS ERC20 Address Update", "tx", tx.Hash().String())
- To reduce code duplication, consider creating a generic function for updating TSS addresses:
func (r *E2ERunner) updateTssAddress(updater func(*bind.TransactOpts, common.Address) (*types.Transaction, error), getter func(*bind.CallOpts) (common.Address, error), logMessage string) error { if err := r.SetTSSAddresses(); err != nil { return err } tx, err := updater(r.EVMAuth, r.TSSAddress) if err != nil { return err } r.Logger.Info(logMessage, "tx", tx.Hash().String()) receipt := utils.MustWaitForTxReceipt(r.Ctx, r.EVMClient, tx, r.Logger, r.ReceiptTimeout) utils.RequireTxSuccessful(r, receipt) tssAddress, err := getter(&bind.CallOpts{Context: r.Ctx}) if err != nil { return err } if tssAddress != r.TSSAddress { return fmt.Errorf("TSS address mismatch: expected %s, got %s", r.TSSAddress, tssAddress) } return nil }Then, you can simplify both
UpdateTssAddressForConnectorandUpdateTssAddressForErc20custody:func (r *E2ERunner) UpdateTssAddressForConnector() { err := r.updateTssAddress( r.ConnectorEth.UpdateTssAddress, r.ConnectorEth.TssAddress, "TSS Address Update", ) require.NoError(r, err) } func (r *E2ERunner) UpdateTssAddressForErc20custody() { err := r.updateTssAddress( r.ERC20Custody.UpdateTSSAddress, r.ERC20Custody.TssAddress, "TSS ERC20 Address Update", ) require.NoError(r, err) }This refactoring reduces code duplication and improves maintainability.
e2e/runner/report.go (1)
62-62: Approve change with minor suggestion for robustnessThe modification to use a format specifier for the
tablestring enhances clarity and consistency in logging. This change is appropriate and aligns with best practices for string formatting in Go.To further improve robustness, consider using the
%vformat specifier instead of%s. This would allow for graceful handling of potential non-string types without causing a panic.Consider applying the following change:
-r.Logger.PrintNoPrefix("%s", table) +r.Logger.PrintNoPrefix("%v", table)This minor adjustment ensures that the logging remains functional even if
tableis unexpectedly not a string type.tests/simulation/sim/sim_config.go (1)
9-30: Consider addressing the TODO comment and grouping related flags.The global variables for simulation flags are well-defined. However, there are two points to consider:
There's a TODO comment on line 23 suggesting the removal of
FlagOnOperationValue. Consider creating an issue to track this task for future refactoring.To improve readability, consider grouping related flags together. For example, you could group all path-related flags, all boolean flags, etc.
Example of grouping:
var ( // File path flags FlagGenesisFileValue string FlagParamsFileValue string FlagExportParamsPathValue string FlagExportStatePathValue string FlagExportStatsPathValue string // Numeric configuration flags FlagExportParamsHeightValue int FlagSeedValue int64 FlagInitialBlockHeightValue int FlagNumBlocksValue int FlagBlockSizeValue int FlagGenesisTimeValue int64 // Boolean flags FlagLeanValue bool FlagCommitValue bool FlagOnOperationValue bool // TODO: Remove in favor of binary search for invariant violation FlagAllInvariantsValue bool FlagEnabledValue bool FlagVerboseValue bool // Other flags FlagPeriodValue uint )This grouping enhances code readability and maintainability.
changelog.md (3)
Line range hint
1-39: Enhance changelog structure and consistency.The "Unreleased" section provides a comprehensive overview of recent changes. However, consider the following improvements:
- Add dates to each entry to provide better context for when changes were made.
- Ensure consistent formatting across all entries (e.g., some entries start with asterisks, others with pull request numbers).
- Consider grouping similar changes together (e.g., all simulation test-related changes).
To improve consistency, consider applying the following format to all entries:
### Category (e.g., Features, Fixes, etc.) - [#PR_NUMBER](PR_URL) - Brief description of the change (YYYY-MM-DD)
Line range hint
41-53: Enhance v12.2.4 section with additional context.The v12.2.4 section provides a clear list of fixes. To improve it further:
- Add a brief summary of the version's main focus or theme.
- Consider adding dates to each fix for better tracking.
- Provide more context for each fix, explaining the impact of the change.
Consider adding a summary at the beginning of the section:
## v12.2.4 (YYYY-MM-DD) This release focuses on improving stability and addressing critical issues in gas price calculation, transaction processing, and chain-specific optimizations. ### Fixes - [#1638](ISSUE_URL) - Ensure external chain height always increases (YYYY-MM-DD) - Impact: Prevents potential synchronization issues and improves chain reliability. ...
Line range hint
1-1589: Improve overall changelog structure and content.The changelog provides a comprehensive history of changes across multiple versions. To enhance its usefulness and readability:
- Add a table of contents at the beginning of the file, linking to each version.
- Ensure consistent formatting across all versions and categories.
- Consider adding a "Breaking Changes" section at the top of each version, if applicable.
- Provide more context for significant changes, explaining the rationale and impact.
- Use consistent date formatting throughout the changelog.
Consider restructuring the changelog as follows:
# Changelog ## Table of Contents - [Unreleased](#unreleased) - [v12.2.4](#v1224) - [v12.1.0](#v1210) ... ## Unreleased ### Breaking Changes ... ### Features ... ### Fixes ... ## v12.2.4 (YYYY-MM-DD) ### Breaking Changes ... ### Fixes ...This structure will improve navigation and readability, making it easier for users and developers to track changes across versions.
tests/simulation/sim_test.go (2)
75-76: Uset.Logandt.Logfinstead offmt.Printlnandfmt.Printfin testsUsing
t.Logandt.Logfensures that log messages are properly associated with the test's output and are only displayed when the test fails or when verbose output is requested. This enhances the readability of test results and integrates better with Go's testing framework.Apply this diff to replace
fmt.Printlnandfmt.Printf:- fmt.Println("Running tests for numSeeds: ", numSeeds, " numTimesToRunPerSeed: ", numTimesToRunPerSeed) + t.Log("Running tests for numSeeds: ", numSeeds, " numTimesToRunPerSeed: ", numTimesToRunPerSeed) - fmt.Printf( + t.Logf( "running non-determinism simulation; seed %d: %d/%d, attempt: %d/%d\n", config.Seed, i+1, numSeeds, j+1, numTimesToRunPerSeed, )Also applies to: 101-104
132-142: Compare byte slices directly without converting to stringsWhen comparing byte slices, it's more idiomatic to compare them directly rather than converting them to strings. Converting byte slices to strings can cause unexpected results if the byte slices contain invalid UTF-8 sequences. Using
require.Equalwith byte slices will correctly handle the comparison.Apply this diff to compare byte slices directly:
require.Equal( t, - string(appHashList[0]), - string(appHashList[j]), + appHashList[0], + appHashList[j], "non-determinism in seed %d: %d/%d, attempt: %d/%d\n", config.Seed, i+1,tests/simulation/sim/sim_state.go (2)
214-222: Replacefmt.Printfwith structured loggingUsing
fmt.Printffor logging is not recommended in production code. Consider using a structured logging library likelogorzapto provide better log management and configurability.Refactor the code:
- fmt.Printf( + log.Infof( `Selected randomly generated parameters for simulated genesis: { stake_per_account: "%d", initially_bonded_validators: "%d" } `, initialStake, numInitiallyBonded, )Ensure you import the appropriate logging package.
207-209: Log adjustments whennumInitiallyBondedexceedsnumAccsWhen adjusting
numInitiallyBondedto not exceednumAccs, it's helpful to log this adjustment for transparency.Add a log statement:
if numInitiallyBonded > numAccs { numInitiallyBonded = numAccs + log.Warnf("Adjusted numInitiallyBonded to %d as it exceeded the number of accounts", numInitiallyBonded) }app/modules.go (1)
59-108: Consider modularizing the simulation modules setup for better maintainability.The
simulationModulesfunction initializes a list ofAppModuleSimulationinstances in a monolithic manner. As the application grows and more modules are added, this approach can become unwieldy. Consider breaking down the initialization into smaller, dedicated functions or employing a registration pattern to enhance scalability and readability.For example, you might refactor the code like this:
func simulationModules( app *App, appCodec codec.Codec, _ bool, ) []module.AppModuleSimulation { modules := []module.AppModuleSimulation{ auth.NewAppModule( appCodec, app.AccountKeeper, authsims.RandomGenesisAccounts, app.GetSubspace(authtypes.ModuleName), ), // ... other modules ... } return modules }Then, for each module, create an initialization function or use a loop if applicable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- app/app.go (4 hunks)
- app/modules.go (1 hunks)
- changelog.md (1 hunks)
- e2e/runner/admin_evm.go (2 hunks)
- e2e/runner/report.go (1 hunks)
- sims.mk (1 hunks)
- tests/simulation/sim/sim_config.go (1 hunks)
- tests/simulation/sim/sim_state.go (1 hunks)
- tests/simulation/sim/sim_utils.go (1 hunks)
- tests/simulation/sim_test.go (1 hunks)
- testutil/simapp/default_constants.go (0 hunks)
- testutil/simapp/simapp.go (0 hunks)
💤 Files with no reviewable changes (2)
- testutil/simapp/default_constants.go
- testutil/simapp/simapp.go
🧰 Additional context used
📓 Path-based instructions (8)
app/app.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.app/modules.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/admin_evm.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/report.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim/sim_config.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim/sim_state.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim/sim_utils.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.tests/simulation/sim_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (6)
sims.mk (1)
38-42: Correct usage of .PHONY declaration.The .PHONY declaration is correctly implemented, including all the defined targets. This follows Makefile best practices and ensures that these targets are always executed, regardless of whether a file with the same name exists.
tests/simulation/sim/sim_config.go (1)
1-7: Package declaration and imports are appropriate.The package name
simis concise and fitting for a simulation configuration package. The import statement correctly references the Cosmos SDK simulation package.tests/simulation/sim/sim_utils.go (1)
47-47: Review the setting ofMaxTxGasWantedin ante handler optionsAt line 47,
MaxTxGasWantedis set to0. This configuration may impact the gas consumption behavior during transaction processing.Ensure that setting
MaxTxGasWantedto0aligns with the simulation requirements. If unlimited gas is intended, this setting is acceptable. Otherwise, consider specifying a reasonable default value to prevent potential issues with gas calculations.tests/simulation/sim/sim_state.go (1)
240-240:⚠️ Potential issueUnchecked error after JSON marshaling
After marshaling
genesisState, it's good practice to check for errors before proceeding.Ensure that the error is handled:
appState, err := json.Marshal(genesisState) if err != nil { + return nil, nil, fmt.Errorf("failed to marshal genesis state: %w", err) }Likely invalid or redundant comment.
app/app.go (1)
820-821: Proper registration of store decodersThe call to
app.sm.RegisterStoreDecoders()ensures that the simulation manager has the necessary store decoders registered. Verify that all relevant modules have their store decoders implemented and registered correctly.app/modules.go (1)
54-54: Ensure all interfaces are properly registered to prevent runtime errors.While
basicManager.RegisterInterfaces(app.interfaceRegistry)registers interfaces for the modules, it's important to verify that all necessary interfaces are included, especially after adding new modules or making changes. Missing interface registrations can lead to issues with serialization and module interactions.Run the following script to review registered interfaces:
✅ Verification successful
All interfaces are properly registered.
The review of the
RegisterInterfacescalls across the codebase confirms thatbasicManager.RegisterInterfaces(app.interfaceRegistry)effectively aggregates all necessary interface registrations. No omissions were detected, ensuring comprehensive interface registration and safeguarding against potential runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List all registered interfaces to verify completeness. # Expected Result: All module interfaces should be registered without omissions. rg 'RegisterInterfaces' -t go -A 3Length of output: 10035
c1db4cf |
Tried it out , I wasn't able to produce the error . Which might be the reason that it is not hitting the edge case which caused the panic in our tests |
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
Description
Add simulation tests to the node repository
Use the following commands to run
Closes : #2960
The simulation tests would be added into the the CI as part of this issue : #2960
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
Tests