test: enable the simulator for the provider module #2005
Conversation
This reverts commit 62dfd1e.
github-actions
bot
added
C:Testing
WalkthroughWalkthroughThe changes introduce a new GitHub Actions workflow for running simulation tests, add new simulation-related functionalities in the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant GitHubActions
participant GoEnvironment
participant SimulationTests
Developer->>GitHubActions: Push code changes to branch
GitHubActions->>GitHubActions: Trigger Simulation Workflow
GitHubActions->>GoEnvironment: Setup Go version 1.22
GitHubActions->>GitHubActions: Checkout code
GitHubActions->>GoEnvironment: Cache dependencies
GitHubActions->>SimulationTests: Run `make sim-full`
SimulationTests-->>GitHubActions: Simulation test results
GitHubActions-->>Developer: Display results
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional context usedPath-based instructions (1)
Additional comments not posted (5)
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 Configration File (
|
p-offtermatt
changed the base branch from
feat/inactive-vals-v50
to
ph/inactive-vals-endblocker
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (11)
.github/workflows/simulation.yml (1)
1-1: Ensure the workflow name is descriptive.The name "Simulation" is clear, but consider adding more context if this workflow is specific to certain tests or modules.
x/ccv/no_valupdates_staking/module.go (1)
1-1: Consider renaming the package to avoid conflicts.The package name
stakingmight conflict with the native staking module. Consider renaming it to something more specific likeno_valupdates_staking.app/provider/sim_test.go (1)
1-1: Consider using a more descriptive package name.The package name
app_testis generic. Consider using a more descriptive name that reflects the purpose of the tests, such asprovider_simulation_test.proto/interchain_security/ccv/provider/v1/genesis.proto (1)
52-53: Ensure the new field is documented.The new field
last_provider_consensus_validatorsshould be documented to explain its purpose and usage.// last_provider_consensus_validators defines the last provider consensus validators. repeated ConsensusValidator last_provider_consensus_validators = 14 [ (gogoproto.nullable) = false ];app/sovereign/export.go (1)
Line range hint
14-161: Consider handling errors more gracefully instead of using panic.Using
panicfor error handling can cause the application to crash. Consider handling errors more gracefully and providing meaningful error messages.- if err != nil { - panic(err) - } + if err != nil { + log.Fatalf("Error: %v", err) + }proto/interchain_security/ccv/provider/v1/provider.proto (1)
255-257: Add documentation for the new parameter.The new parameter
max_provider_consensus_validatorsshould have clear documentation explaining its purpose and usage.+ // The maximal number of validators that will be passed + // to the consensus engine on the provider. int64 max_provider_consensus_validators = 11;tests/e2e/steps_inactive_vals.go (3)
1-1: Ensure package name consistency.The package name
mainis generally reserved for the entry point of an application. Consider using a more descriptive package name for test files.
10-19: Improve function documentation.The function documentation is clear but can be improved by specifying the expected outcomes for each step in the test.
83-113: Clarify power calculation logic.The comment about Carol needing more than 2/3rds of power is correct but could be expanded to explain the specific calculation used to determine the amount staked.
tests/e2e/main.go (2)
Line range hint
22-22: Consider adding default values for flags.Adding default values for flags can help avoid potential issues when the flags are not set by the user.
Line range hint
164-196: Improve error messages for missing test cases.The function logs fatal errors when test cases are not found. Consider improving the error messages to provide more context and guidance for the user.
| defer func() { | ||
| require.NoError(t, db.Close()) | ||
| require.NoError(t, os.RemoveAll(dir)) | ||
| }() |
There was a problem hiding this comment.
Handle potential errors in deferred functions.
The deferred functions should handle potential errors from db.Close() and os.RemoveAll(dir) to ensure proper resource cleanup.
defer func() {
if err := db.Close(); err != nil {
t.Errorf("failed to close database: %v", err)
}
if err := os.RemoveAll(dir); err != nil {
t.Errorf("failed to remove directory: %v", 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.
| defer func() { | |
| require.NoError(t, db.Close()) | |
| require.NoError(t, os.RemoveAll(dir)) | |
| }() | |
| defer func() { | |
| if err := db.Close(); err != nil { | |
| t.Errorf("failed to close database: %v", err) | |
| } | |
| if err := os.RemoveAll(dir); err != nil { | |
| t.Errorf("failed to remove directory: %v", err) | |
| } | |
| }() |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
.github/workflows/simulation.yml (2)
1-1: Add a description for the workflow.Consider adding a
descriptionfield under thenameto provide more context about the workflow's purpose.
2-10: Specify event types for better control.While
pull_requestandpushevents are specified, it is a good practice to define specific event types (e.g.,pull_request: [opened, synchronize]) to avoid unnecessary workflow runs.
:thumbs-up: Agreed. I want to keep the scope of this PR small, since this is just focussed on helping test the inactive validators feature, but in general I agree. |
p-offtermatt
changed the base branch from
ph/inactive-vals-endblocker
to
feat/inactive-vals-v50
p-offtermatt
changed the base branch from
feat/inactive-vals-v50
to
ph/inactive-vals-endblocker
* Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add new MaxProviderConsensusValidators param * Add validation for MaxProviderConsensusValidators * Add no_valupdates_staking module * Add function to get MaxProviderConsensusValidators param * Start returning validators in EndBlock * Fix tests * Revert cosmetic change * Revert cosmetic changes * Revert formatting * Add genutil replacer module * Revert formatting * Revert formatting in tests/integration * Revert minor formatting * Fix type * Change wrapped staking to conform to EndBlocker interface * Fix typo * Revert "Fix typo" This reverts commit 62dfd1e. * Add e2e test for inactive vals * Start fixing e2e test * Revert formatting changes * Remove more formatting * Revert extra formatting * Re-wire provider/app.go to use wrapped modules * Remove consumer rewards check * Add simulator test * Add randomly generated parameters for provider in sim * Add invariant * Add simulation to Makefile and github workflow * Use simcli instead of just passing true
…er chains (#2079) * refactor!: Refactor the validator set storage and add provider consensus validator storage (#1990) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add key to key test * Add unit test for LastTotalProviderConsensusPower * Address comments * feat!: Introduce the MaxProviderConsensusValidators param (#1992) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add new MaxProviderConsensusValidators param * Add validation for MaxProviderConsensusValidators * Add function to get MaxProviderConsensusValidators param * refactor!: Refactor the validator set storage and add provider consensus validator storage (#1990) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add key to key test * Add unit test for LastTotalProviderConsensusPower * Address comments * feat!: Wire the provider module to return ValidatorUpdates, instead of the staking module (#1993) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add new MaxProviderConsensusValidators param * Add validation for MaxProviderConsensusValidators * Add no_valupdates_staking module * Add function to get MaxProviderConsensusValidators param * Start returning validators in EndBlock * Fix tests * Revert cosmetic change * Revert cosmetic changes * Revert formatting * Add genutil replacer module * Revert formatting * Revert formatting in tests/integration * Revert minor formatting * Fix type * Change wrapped staking to conform to EndBlocker interface * Fix typo * Revert "Fix typo" This reverts commit 62dfd1e. * Add e2e test for inactive vals * Start fixing e2e test * Revert formatting changes * Remove more formatting * Revert extra formatting * Re-wire provider/app.go to use wrapped modules * Remove consumer rewards check * Add inactive provider vals testcase to nightly * Adjust comment * Address comments * Fix nightly test name * feat: Initialize the max validators parameter for existing consumers (#2012) * Add initialization for validator cap * Remove migration test * Fix inconsistent naming * test: enable the simulator for the provider module (#2005) * Refactor validator set storage * Add comment for getTotalPower * Add provider consensus validator set storage * Add new MaxProviderConsensusValidators param * Add validation for MaxProviderConsensusValidators * Add no_valupdates_staking module * Add function to get MaxProviderConsensusValidators param * Start returning validators in EndBlock * Fix tests * Revert cosmetic change * Revert cosmetic changes * Revert formatting * Add genutil replacer module * Revert formatting * Revert formatting in tests/integration * Revert minor formatting * Fix type * Change wrapped staking to conform to EndBlocker interface * Fix typo * Revert "Fix typo" This reverts commit 62dfd1e. * Add e2e test for inactive vals * Start fixing e2e test * Revert formatting changes * Remove more formatting * Revert extra formatting * Re-wire provider/app.go to use wrapped modules * Remove consumer rewards check * Add simulator test * Add randomly generated parameters for provider in sim * Add invariant * Add simulation to Makefile and github workflow * Use simcli instead of just passing true * feat!: Let consumer chains choose a minimum stake and validator rank (#2035) * Add minimum stake key * Add MinValidatorRank prefix * Add keeper and tests for new parameters * Utilize MinStake and MaxRank parameters in computing next validators * Mention MinStake and MaxRank in adr * Add test for FulfillsMinStake * Handle multiple validators with same power * Add min stake and max rank to docs * Add minStake and maxRank to proposals * Check for untyped equality * Handle minStake and maxRank in Msgs * Add integration test for min stake and max rank * Rename test and testfile * Update docs/docs/adrs/adr-017-allowing-inactive-validators.md * Add changelog entries for maxrank and minstake * Address comments * Clarify which feature is disabled by setting maxrank * Test validator powers cap and validator set cap into int param testing function * feat!: Rewire dependencies on the staking module (#2056) * Change wiring for mint and gov to use ProviderKeeper instead of StakingKeeper * Add test for IterateBondedValidatorsByPower * Rewire GovKeeper * Fix docstrings * Test other modified functions * Start writing some testing scenarios * Add TotalBondedTokens to expected staking keeper interface * feat: Calculate Top N based on active validators only (#2070) * Add test for inactive vals with top N * Add test case to predefined tests * Fix bonded/active validator distinction * Fix relay test to set max provider consensus vals correctly * feat!: Add a parameter that determines whether consumer chains allow inactive validators to validate them (#2066) * Introduce new AllowInactiveValidators param for consumer chains * Add AllowInactiveValidators param to tests * Set MaxProviderConsensusValidators in tests * Add migration to initialize inactive vals * Add changelog entries for inactive vals param * Add property-based test for inactive vals * Add docs for inactive vals param * Set AllowInactiveVals parameter in e2e test * test: Add e2e tests for inactive vals (#2064) * Start adding e2e test for governance * Debug gov with inactive vals test * Outline for test scenarios where they are tested * Add MaxRank steps * Add e2e tests for min stake and max rank * Revert formatting change * Refactor stepsOptIn * Use adjusted config for e2e tests * Write for more scenarios where they are tested * Add test for mint * Add docstrings for e2e steps * Delete hanging changelog entry * Address comments * Address more comments * Add migration for param * Fix allow inactive validators param test * Fix tests * Add LastProviderConsensusValidatorKey to fully defined keys * Fix key for validator set updates * Add info about genesis/endblock ordering * Add unit test for ProviderValidatorUpdates * Add example to proto definition of max_rank * Remove max rank * Remove references to max rank * Start adding an extension to the simulator * Make invariant fail early when param is 0 * Reorder InitGenesis to put Crisis last * Remove canary * Swap equals for not equals * Disable invariant check when max validators != max provider consensus validators * Make the simulator use a random seed * Remove TODO * Remove decoder * Run go mod tidy * Add migration in UPGRADING.md * Fix tests * Put random seed generation into golang code * Rename simulation jobs * Update UPGRADING.md Co-authored-by: Marius Poke <[email protected]> * Update UPGRADING.md Co-authored-by: Marius Poke <[email protected]> * Update x/ccv/provider/keeper/genesis.go Co-authored-by: Marius Poke <[email protected]> * Mention simulation tests in testing.md * Address some comments * Remake protos * Panic when LastActiveBondedValidators fails * Address some comments * Address comments * Reorder tests * Adjust stake_multiplier to stakeMultiplier * Address comments * Add error logging * Fix reference to bank blocked addrs in simulation * Change hasToValidate to only take into account active validators * Update docs/docs/adrs/adr-017-allowing-inactive-validators.md Co-authored-by: insumity <[email protected]> * Clarify: Slash happens on provider Co-authored-by: insumity <[email protected]> --------- Co-authored-by: Marius Poke <[email protected]> Co-authored-by: insumity <[email protected]>
Description
Closes: Part of inactive validators #1913
The simulator integration is very basic for now.
I just wanted it to test for panics and basic discrepancies with the inactive validators feature.
Note that this does not seriously check any of the CCV functionality; that functionality relies on IBC and multiple chains, which are not supported in the simulator.
You can run
make sim-fullto check out the simulator locally.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...
Reviewers 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.
I have...
Summary by CodeRabbit
New Features
Refactor
bankBlockedAddrsinto a new function.Bug Fixes
Tests