update simulation test #2983
update simulation test #2983
Conversation
WalkthroughThe pull request introduces a simulation test feature across multiple files in the IrisHub application. The changes primarily focus on adding a Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
go.mod (1)
281-285: Consider removing commented-out module replacements.The commented-out
replacedirectives may no longer be needed. Removing unused code improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
app/ante/decorators.go(1 hunks)app/export.go(5 hunks)app/sim_test.go(2 hunks)go.mod(2 hunks)sims.mk(0 hunks)
💤 Files with no reviewable changes (1)
- sims.mk
🧰 Additional context used
🪛 golangci-lint (1.62.2)
app/export.go
179-179: Error return value of app.StakingKeeper.IterateRedelegations is not checked
(errcheck)
191-191: Error return value of app.StakingKeeper.IterateUnbondingDelegations is not checked
(errcheck)
220-220: Error return value of app.StakingKeeper.SetValidator is not checked
(errcheck)
🔇 Additional comments (4)
app/ante/decorators.go (1)
79-79:
Verify removal of validation logic in ValidateServiceDecorator.
Removing the validation logic in ValidateServiceDecorator.AnteHandle means MsgCallService messages are no longer validated. This could introduce security risks if repeatable service invocations are not intended.
Run the following script to assess the impact of this change:
go.mod (1)
14-23: Verify compatibility and security of updated modules.
Ensure that the updated module versions are compatible with the codebase and do not introduce new vulnerabilities.
Run the following script to check for known vulnerabilities in the updated modules:
app/sim_test.go (2)
199-199: Ensure newApp uses newDB for accurate testing.
Using newDB instead of db when creating newApp ensures that tests are performed on a fresh database, preventing state leakage between tests.
372-377: Add error handling and specify ChainId in InitChain.
Including ChainId and checking the error returned by newApp.InitChain ensures proper initialization and catches potential issues early.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/export.go(5 hunks)go.mod(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
app/export.go
197-197: Error return value of app.StakingKeeper.IterateUnbondingDelegations is not checked
(errcheck)
🔇 Additional comments (5)
app/export.go (4)
86-96: LGTM: Improved address handling and error management
The changes properly implement the new address codec pattern and add comprehensive error handling for both address conversion and iteration operations.
126-155: LGTM: Well-structured error handling
The changes implement proper error handling for all operations and consistently use the new address codec pattern.
182-194: LGTM: Proper error handling added
Error handling has been appropriately added for both redelegation operations and iteration.
197-206:
Add missing error handling for IterateUnbondingDelegations
While error handling was added for SetUnbondingDelegation, the error return value from IterateUnbondingDelegations is not being checked.
Apply this diff to handle the error:
-app.StakingKeeper.IterateUnbondingDelegations(ctx, func(_ int64, ubd stakingtypes.UnbondingDelegation) (stop bool) {
+err = app.StakingKeeper.IterateUnbondingDelegations(ctx, func(_ int64, ubd stakingtypes.UnbondingDelegation) (stop bool) {
for i := range ubd.Entries {
ubd.Entries[i].CreationHeight = 0
}
err = app.StakingKeeper.SetUnbondingDelegation(ctx, ubd)
if err != nil {
panic(err)
}
return false
})
+if err != nil {
+ panic(err)
+}Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
197-197: Error return value of app.StakingKeeper.IterateUnbondingDelegations is not checked
(errcheck)
go.mod (1)
14-23: LGTM: Consistent version updates
All irisnet modules have been updated to the same version (20241217080151-0ad41be03ac6), maintaining consistency across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
testutil/app.go (1)
45-45: Document and parameterize simulation flag in test setupThe simulation flag is hardcoded to
true, which:
- Forces all tests to run in simulation mode
- Prevents testing non-simulation paths
- Lacks documentation explaining the implications
Consider making simulation configurable:
-true, +simulate bool,And update the function signature:
-func setup(appOpts servertypes.AppOptions, baseAppOptions ...func(*baseapp.BaseApp)) *AppWrapper { +func setup(appOpts servertypes.AppOptions, simulate bool, baseAppOptions ...func(*baseapp.BaseApp)) *AppWrapper {This allows tests to control simulation mode as needed.
app/sim_bench_test.go (1)
55-55: Extract duplicated simulation flag to a constantThe simulation flag is duplicated across both benchmark functions. To maintain consistency and make future updates easier, consider extracting it to a constant.
Add a constant at the package level:
+const benchmarkSimulateFlag = trueThen update both NewIrisApp calls:
-true, +benchmarkSimulateFlag,This makes it clear that both benchmarks intentionally use the same simulation configuration.
Also applies to: 123-123
cmd/iris/cmd/root.go (1)
266-267: Consider extracting the simulation flag to a constant.The
falsevalue is repeated across multiple locations. Consider defining a constant for better maintainability.+const defaultSimulateMode = false - false, + defaultSimulateMode,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/ante/ante.go(1 hunks)app/ante/decorators.go(2 hunks)app/ante/handler_options.go(1 hunks)app/app.go(2 hunks)app/sim_bench_test.go(2 hunks)app/sim_test.go(3 hunks)cmd/iris/cmd/root.go(3 hunks)testutil/app.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/ante/decorators.go
- app/sim_test.go
🔇 Additional comments (5)
app/ante/handler_options.go (1)
34-34: LGTM! Verify integration with simulation tests.
The addition of the Simulate field to HandlerOptions is clean and follows the existing pattern.
cmd/iris/cmd/root.go (2)
46-46: LGTM! Default simulation mode is correctly disabled.
Setting simulate to false for the temporary application is appropriate as it's used for initialization and not for actual simulation.
299-299: Verify simulation mode in export scenario.
Setting simulate to false during app export is logical, but we should verify this doesn't affect any existing simulation tests.
✅ Verification successful
Setting simulate to false during export is safe and won't affect simulation tests
After analyzing the codebase, I can confirm that setting simulate to false during app export is safe and won't affect any simulation tests. The simulation tests (TestFullAppSimulation, TestAppSimulationAfterImport, and TestAppStateDeterminism) use their own simulation configuration and don't rely on the export's simulate parameter. When they need to export state during testing, they explicitly control the simulation parameters through the test configuration.
The export functionality in TestAppImportExport also explicitly sets its own simulate parameter during the test, independent of the CLI command's default value:
exported, err := app.ExportAppStateAndValidators(false, []string{}, []string{})🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any simulation tests that might be affected by the export functionality
rg -A 5 "ExportAppStateAndValidators.*simulation"
Length of output: 51
Script:
#!/bin/bash
# Let's search for simulation-related tests and code more broadly
rg -i "simulation.*export" -A 5
# Also search for any simulation test files
fd "simulation.*test"
# Let's also check the context around where simulate is being set to false
rg -B 5 -A 5 "simulate.*false" cmd/iris/cmd/root.go
Length of output: 111
Script:
#!/bin/bash
# Let's look at the broader context of the export command implementation
rg -B 10 -A 10 "ExportAppStateAndValidators" cmd/iris/cmd/root.go
# Also check for any simulation-related code in the app
rg -l "simulation"
# Look for any test files that might be related to export functionality
fd "export.*test"
Length of output: 768
Script:
#!/bin/bash
# Let's check the simulation test files to see if they interact with export functionality
cat app/sim_test.go app/sim_bench_test.go
# Also look at the simulation genesis and decoder implementations
cat modules/mint/simulation/genesis.go modules/mint/simulation/decoder.go modules/mint/simulation/decoder_test.go
Length of output: 22403
app/app.go (2)
81-81: LGTM! Function signature change is clean.
The addition of the simulate parameter to NewIrisApp is well-placed and maintains backward compatibility.
199-199: Verify simulation behavior in ante handler.
The simulate parameter is correctly passed to the ante handler options. However, we should verify the behavior in simulation scenarios.
✅ Verification successful
Let me gather more information about the simulation behavior by checking for additional simulation-related code.
Let me check the actual implementation details in these files to understand the simulation behavior better.
Simulation behavior in ante handlers is properly implemented
The codebase shows clear and correct handling of the simulate parameter in ante handlers:
RejectMessagesDecorator: Consistently applies message rejection rules regardless of simulation mode, which is correct for validating transaction structureValidateServiceDecorator: Explicitly skips validation during simulation (if simulate { return next(ctx, tx, simulate) }), allowing simulation to proceedValidateTokenDecorator: Properly passes the simulation flag through to next handlers while maintaining token validation rules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the simulate parameter affects ante handler behavior
# Test: Look for simulation-specific handling in ante handlers
rg -A 5 "AnteHandle.*simulate"
Length of output: 474
Script:
#!/bin/bash
# Check for more simulation-related code in ante handlers
rg -l "simulate" app/ante/
# Look for the implementation details of RejectMessagesDecorator
ast-grep --pattern 'type RejectMessagesDecorator struct {
$$$
}'
# Check for any simulation-specific tests
rg -l "simulation" app/ante/
Length of output: 236
Script:
#!/bin/bash
# Check the content of decorators.go for simulation handling
cat app/ante/decorators.go
# Check the complete implementation of reject_msgs.go
cat app/ante/reject_msgs.go
Length of output: 4709
app/ante/handler_options.go
Outdated
| @@ -31,6 +31,7 @@ type HandlerOptions struct { | |||
| FeeMarketKeeper ethante.FeeMarketKeeper | |||
| BypassMinFeeMsgTypes []string | |||
| MaxTxGasWanted uint64 | |||
| Simulate bool | |||
wangjiulian
force-pushed
the
feature/up_simulate
branch
from
0410b37 to
8ebfe3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/ante/ante.go(1 hunks)app/ante/decorators.go(2 hunks)app/ante/handler_options.go(1 hunks)app/app.go(2 hunks)app/params/params.go(1 hunks)app/sim_bench_test.go(3 hunks)app/sim_test.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/params/params.go
🚧 Files skipped from review as they are similar to previous changes (4)
- app/ante/ante.go
- app/ante/handler_options.go
- app/app.go
- app/ante/decorators.go
🔇 Additional comments (3)
app/sim_bench_test.go (1)
56-58: LGTM: Simulation flag correctly implemented in benchmark tests
The changes consistently implement the simulation flag across both benchmark functions using SimTestAppOptions, which aligns with the simulation enhancement objectives.
Also applies to: 125-127
app/sim_test.go (2)
481-490: LGTM: Well-structured implementation of SimTestAppOptions
The new SimTestAppOptions type provides a clean implementation for handling simulation-specific options.
507-509: LGTM: Consistent usage of SimTestAppOptions
The createApp function correctly uses SimTestAppOptions with the simulation flag, maintaining consistency across all test scenarios.
| @@ -196,7 +196,7 @@ func TestAppImportExport(t *testing.T) { | |||
| require.NoError(t, os.RemoveAll(newDir)) | |||
| }() | |||
|
|
|||
| newApp := createApp(logger, db, encfg, fauxMerkleModeOpt) | |||
| newApp := createApp(logger, newDB, encfg, fauxMerkleModeOpt) | |||
There was a problem hiding this comment.
Add error handling for InitChain in TestAppImportExport
While error handling for InitChain was added in TestAppSimulationAfterImport, similar error handling is missing in TestAppImportExport. Consider adding consistent error handling across both test functions.
Apply this pattern to TestAppImportExport:
newApp := createApp(logger, newDB, encfg, fauxMerkleModeOpt)
require.Equal(t, "IrisApp", newApp.Name())
+
+_, err = newApp.InitChain(&abci.RequestInitChain{
+ AppStateBytes: exported.AppState,
+ ChainId: AppChainID,
+})
+require.NoError(t, err)Also applies to: 369-378
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/ante/decorators.go (2)
66-68: Add a short comment explaining what "SimulateTest" does.
Documenting the intent of this field will help future maintainers understand why it's needed and how it should be used.
71-74: Validate parameter naming consistency.
Currently, "simulateTest" is a bit ambiguous. Consider naming it "simulationTest" to match the field name on line 67 or to reflect its usage more clearly.app/app.go (1)
205-205: Validate consistent naming in HandlerOptions.
The HandlerOptions struct has a field named "SimulationTest" while the local variable is "simulationTest". Although functional, consider consistent naming for clarity.app/sim_test.go (1)
481-490: Add usage examples for SimTestAppOptions.
This new type effectively enables simulation-specific options. Adding a short code comment or example can help convey how these options can be extended if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/ante/decorators.go(3 hunks)app/ante/handler_options.go(2 hunks)app/app.go(2 hunks)app/params/params.go(1 hunks)app/sim_bench_test.go(3 hunks)app/sim_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/params/params.go
- app/ante/handler_options.go
- app/sim_bench_test.go
🔇 Additional comments (5)
app/ante/decorators.go (1)
84-86: Ensure coverage for simulation bypass logic.
When "SimulateTest" is true, the decorator immediately calls the next handler. Consider adding a unit test that verifies transactions are indeed bypassed here.
app/app.go (1)
181-186: Provide extra caution for type casting.
The application uses appOpts.Get(params.SimulationTest) and expects a bool. Ensure that any misconfiguration (e.g., a string instead of a bool) is handled gracefully or documented as an expected usage constraint.
app/sim_test.go (3)
199-199: Add error checks after newApp initialization.
Although the subsequent test code calls InitGenesis later, consider verifying that newApp is properly instantiated. For instance, a quick check for null keepers or other uninitialized fields can help detect misconfigurations.
369-378: Retain consistent error handling across test functions.
As recommended in a previous comment, ensure both TestAppImportExport and TestAppSimulationAfterImport have the same style of error handling for InitChain. This fosters consistency and reduces future debugging complexities.
507-509: Coordinate simulation flag across your codebase.
Even though "SimulationTest" is set to true here, ensure it’s properly referenced and validated in all relevant simulation tests. This helps maintain consistency and avoids confusion about default vs. simulated runs.
Closed: #2978 #2979 #2980 #2981
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores