test(integration): port x/staking tests to server v2 #22813
Conversation
📝 WalkthroughWalkthroughThe pull request introduces comprehensive changes to the integration testing framework for the staking module, focusing on migrating tests to a new version (v2) of the server architecture. The modifications involve updating context management, refactoring test utilities, and standardizing test structures across multiple files. Key changes include transitioning from Changes
Sequence DiagramsequenceDiagram
participant Test as Integration Test
participant Fixture as Test Fixture
participant Keeper as Staking Keeper
participant Context as Context Manager
Test->>Fixture: Initialize with new context
Fixture->>Context: Create context
Fixture->>Keeper: Setup with new context
Test->>Keeper: Perform staking operations
Keeper-->>Test: Return results
Possibly related PRs
Suggested Labels
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
akhilkumarpilli
changed the title
akhilkumarpilli
requested review from
facundomedica,
testinginprod,
JulianToledano,
ziscky and
a team
as code owners
akhilkumarpilli
removed request for
a team,
ziscky,
JulianToledano,
facundomedica and
testinginprod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (21)
tests/integration/v2/staking/unbonding_test.go (3)
Line range hint
25-51: SimplifymockHooksfunction and ensure mock expectations are metThe
mockHooksfunction sets up extensive mock expectations. Ensure that all expected calls are necessary for the tests and that redundant or unnecessary expectations are removed to simplify the test setup.
53-87: Check return values inSetupUnbondingTestsThe updated
SetupUnbondingTestsfunction now returnsbondDenom,addrDels, andaddrVals. Ensure that all return values are used appropriately in the calling functions to prevent unused variables.
Line range hint
98-122: Ensure context is correctly passed indoUnbondingDelegationChanging the
ctxparameter tocontext.Contextrequires a thorough check to ensure all context-dependent calls withindoUnbondingDelegationare correctly using the updated context.tests/integration/v2/staking/deterministic_test.go (1)
44-51: Initialize all fields indeterministicFixturestructEnsure that all necessary fields are initialized in
deterministicFixture, and remove any unused fields to improve code clarity and maintainability.tests/integration/v2/staking/slash_test.go (1)
276-298: Potential redundancy in slashing logicIn
TestSlashWithUnbondingDelegation, consider reviewing the successive slashing operations to ensure they are necessary and that they accurately reflect the intended test case without redundancy.tests/integration/v2/staking/validator_test.go (2)
1-1: Consider using a separate test package to improve encapsulationRenaming the package from
keeper_testtostakingmay grant tests access to unexported identifiers, which can lead to tests depending on internal implementation details. Using a distinct test package (e.g.,staking_test) helps ensure tests only use exported APIs.
Line range hint
892-898: Simplify theapplyValidatorSetUpdateshelper functionThe
applyValidatorSetUpdatesfunction can be improved by returning the error instead of handling it internally. This allows for better error handling in the calling functions.Consider modifying the function signature to return
(updates []module.ValidatorUpdate, err error)and handle the error in the test functions.tests/integration/v2/staking/module_test.go (1)
13-20: Ensure module accounts are properly initialized in testsThe test checks for the existence of module accounts but doesn't verify their balances or permissions. It might be beneficial to assert that the module accounts have the expected initial state.
Consider adding assertions to check the account balances and permissions:
// Check BondedPoolName account balance and permissions bondedBalance := f.bankKeeper.GetAllBalances(f.ctx, acc.GetAddress()) require.True(t, bondedBalance.IsZero(), "Bonded pool should start with zero balance") // Check NotBondedPoolName account balance and permissions notBondedBalance := f.bankKeeper.GetAllBalances(f.ctx, acc.GetAddress()) require.True(t, notBondedBalance.IsZero(), "Not bonded pool should start with zero balance")x/staking/keeper/test_common.go (1)
83-138: Avoid code duplication betweenTestingUpdateValidatorandTestingUpdateValidatorV2Both functions share a significant amount of code. To reduce duplication and improve maintainability, consider refactoring common logic into a shared helper function.
Apply this diff to refactor the common code:
+func updateValidatorHelper(keeper *Keeper, ctx context.Context, validator types.Validator) error { + err := keeper.SetValidator(ctx, validator) + if err != nil { + return err + } + // ... move common logic here ... + return nil +} + func TestingUpdateValidator(keeper *Keeper, ctx sdk.Context, validator types.Validator, apply bool) types.Validator { - // existing code ... + err := updateValidatorHelper(keeper, ctx, validator) + // handle error ... } func TestingUpdateValidatorV2(keeper *Keeper, ctx context.Context, validator types.Validator, apply bool) (types.Validator, []module.ValidatorUpdate) { - // existing code ... + err := updateValidatorHelper(keeper, ctx, validator) + // handle error and proceed ... }tests/integration/v2/staking/vote_extensions_test.go (1)
66-69: Consider extracting validator setup into a helper functionThe validator setup sequence is repeated across multiple test files. Consider extracting it into a helper function to improve maintainability.
+func setupValidator(t *testing.T, f *Fixture, v types.Validator, privKey cryptotypes.PrivKey) error { + v.Tokens = math.NewInt(1000000) + v.Status = stakingtypes.Bonded + if err := f.stakingKeeper.SetValidator(f.ctx, v); err != nil { + return err + } + if err := f.stakingKeeper.SetValidatorByConsAddr(f.ctx, v); err != nil { + return err + } + if err := f.stakingKeeper.SetNewValidatorByPowerIndex(f.ctx, v); err != nil { + return err + } + _, err := f.stakingKeeper.Delegate(f.ctx, sdk.AccAddress(privKey.PubKey().Address()), + v.Tokens, stakingtypes.Unbonded, v, true) + return err +}tests/integration/v2/staking/delegation_test.go (2)
Line range hint
89-91: Enhance error message for max entries validationThe error message could be more descriptive by including the actual max entries value.
-assert.Error(t, err, "too many unbonding delegation entries for (delegator, validator) tuple") +assert.Error(t, err, fmt.Sprintf("too many unbonding delegation entries (max: %d) for (delegator, validator) tuple", maxEntries))
Line range hint
76-85: Consider extracting balance assertion into a helper functionThe balance checking logic is repeated multiple times. Consider extracting it into a helper function to reduce code duplication.
+func assertPoolBalances(t *testing.T, f *Fixture, ctx context.Context, + oldBonded, oldNotBonded, expectedBondedDiff, expectedNotBondedDiff math.Int) { + bondDenom, err := f.stakingKeeper.BondDenom(ctx) + assert.NilError(t, err) + newBonded := f.bankKeeper.GetBalance(ctx, f.stakingKeeper.GetBondedPool(ctx).GetAddress(), bondDenom).Amount + newNotBonded := f.bankKeeper.GetBalance(ctx, f.stakingKeeper.GetNotBondedPool(ctx).GetAddress(), bondDenom).Amount + assert.Assert(math.IntEq(t, newBonded, oldBonded.Add(expectedBondedDiff))) + assert.Assert(math.IntEq(t, newNotBonded, oldNotBonded.Add(expectedNotBondedDiff))) +}tests/integration/v2/staking/common_test.go (4)
72-77: Consider adding validation for numAddrs parameterThe function should validate that numAddrs is positive to prevent potential issues with negative or zero values.
func generateAddresses(f *fixture, numAddrs int) ([]sdk.AccAddress, []sdk.ValAddress) { + if numAddrs <= 0 { + panic("numAddrs must be positive") + } addrDels := simtestutil.AddTestAddrsIncremental(f.bankKeeper, f.stakingKeeper, f.ctx, numAddrs, math.NewInt(10000)) addrVals := simtestutil.ConvertAddrsToValAddrs(addrDels)
79-114: Consider extracting magic numbers into constantsThe function is well-implemented, but the hardcoded value 5 for the number of validators should be extracted into a named constant for better maintainability.
+const ( + defaultTestValidatorCount = 5 +) func createValidators( t *testing.T, f *fixture, powers []int64, ) ([]sdk.AccAddress, []sdk.ValAddress, []types.Validator) { t.Helper() - addrs := simtestutil.AddTestAddrsIncremental(f.bankKeeper, f.stakingKeeper, f.ctx, 5, f.stakingKeeper.TokensFromConsensusPower(f.ctx, 300)) + addrs := simtestutil.AddTestAddrsIncremental(f.bankKeeper, f.stakingKeeper, f.ctx, defaultTestValidatorCount, f.stakingKeeper.TokensFromConsensusPower(f.ctx, 300))
116-118: Consider avoiding global state for mockUsing a global variable for the mock staking hook could lead to test interference. Consider passing the mock as a parameter or using a factory function.
-var mockStakingHook = types.StakingHooksWrapper{} - -func ProvideMockStakingHook() types.StakingHooksWrapper { - return mockStakingHook +func NewMockStakingHook() types.StakingHooksWrapper { + return types.StakingHooksWrapper{} }
120-176: Add documentation for isGenesisSkip parameterThe function is well-implemented, but the
isGenesisSkipparameter lacks documentation explaining its purpose and impact on the test setup.+// initFixture initializes a new test fixture with the specified configuration. +// Parameters: +// - tb: testing.TB interface for test assertions +// - isGenesisSkip: when true, skips genesis initialization +// - stakingHooks: optional staking hooks for custom behavior func initFixture(tb testing.TB, isGenesisSkip bool, stakingHooks ...types.StakingHooksWrapper) *fixture {tests/integration/v2/staking/genesis_test.go (3)
23-24: Consider removing t.Parallel() from bootstrapGenesisTestSince
bootstrapGenesisTestis a helper function, the decision to run tests in parallel should be made by the test functions that use it, not the helper itself.func bootstrapGenesisTest(t *testing.T, numAddrs int) (*fixture, []sdk.AccAddress) { t.Helper() - t.Parallel() f := initFixture(t, true)
139-140: Consider adding test cases for edge casesThe test for pool balance mismatch only covers basic scenarios. Consider adding test cases for edge cases like zero tokens or maximum token values.
Line range hint
184-205: Extract magic numbers into named constantsThe test uses magic numbers (200, 100) for validator set sizes. These should be extracted into named constants for better maintainability and clarity.
+const ( + largeValidatorSetSize = 200 + highPowerValidatorCount = 100 +) func TestInitGenesisLargeValidatorSet(t *testing.T) { - size := 200 + size := largeValidatorSetSize assert.Assert(t, size > 100) f, addrs := bootstrapGenesisTest(t, 200) genesisValidators, err := f.stakingKeeper.GetAllValidators(f.ctx) assert.NilError(t, err) params, err := f.stakingKeeper.Params.Get(f.ctx) assert.NilError(t, err) delegations := []types.Delegation{} validators := make([]types.Validator, size) bondedPoolAmt := math.ZeroInt() for i := range validators { validators[i], err = types.NewValidator( sdk.ValAddress(addrs[i]).String(), PKs[i], types.NewDescription(fmt.Sprintf("#%d", i), "", "", "", "", &types.Metadata{}), ) assert.NilError(t, err) validators[i].Status = types.Bonded tokens := f.stakingKeeper.TokensFromConsensusPower(f.ctx, 1) - if i < 100 { + if i < highPowerValidatorCount { tokens = f.stakingKeeper.TokensFromConsensusPower(f.ctx, 2) }tests/integration/v2/app.go (1)
104-104: Consider renaming parameter for better clarityThe parameter name should be
tbinstead oftto better reflect its typetesting.TBand follow Go conventions.-func DefaultStartUpConfig(t testing.TB) StartupConfig { +func DefaultStartUpConfig(tb testing.TB) StartupConfig {🧰 Tools
🪛 golangci-lint (1.62.2)
104-104: parameter testing.TB should have name tb
(thelper)
tests/integration/v2/staking/grpc_query_test.go (1)
115-120: Consider adding test case for pagination edge cases.While the test coverage is good, consider adding test cases for pagination edge cases (empty results, maximum limit).
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
tests/integration/staking/app_config.go(0 hunks)tests/integration/staking/keeper/common_test.go(0 hunks)tests/integration/staking/module_test.go(0 hunks)tests/integration/v2/app.go(2 hunks)tests/integration/v2/services.go(1 hunks)tests/integration/v2/staking/common_test.go(1 hunks)tests/integration/v2/staking/delegation_test.go(5 hunks)tests/integration/v2/staking/deterministic_test.go(25 hunks)tests/integration/v2/staking/genesis_test.go(11 hunks)tests/integration/v2/staking/grpc_query_test.go(24 hunks)tests/integration/v2/staking/module_test.go(1 hunks)tests/integration/v2/staking/msg_server_test.go(14 hunks)tests/integration/v2/staking/slash_test.go(14 hunks)tests/integration/v2/staking/unbonding_test.go(8 hunks)tests/integration/v2/staking/validator_bench_test.go(9 hunks)tests/integration/v2/staking/validator_test.go(20 hunks)tests/integration/v2/staking/vote_extensions_test.go(6 hunks)testutil/configurator/configurator.go(1 hunks)x/staking/keeper/cons_pubkey.go(1 hunks)x/staking/keeper/test_common.go(2 hunks)
💤 Files with no reviewable changes (3)
- tests/integration/staking/app_config.go
- tests/integration/staking/module_test.go
- tests/integration/staking/keeper/common_test.go
🧰 Additional context used
📓 Path-based instructions (17)
tests/integration/v2/staking/module_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/delegation_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
testutil/configurator/configurator.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/v2/staking/validator_bench_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/app.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/genesis_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/vote_extensions_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/services.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
x/staking/keeper/test_common.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/staking/keeper/cons_pubkey.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/v2/staking/msg_server_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/grpc_query_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/common_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/validator_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/slash_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/unbonding_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/staking/deterministic_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
tests/integration/v2/app.go
104-104: parameter testing.TB should have name tb
(thelper)
🔇 Additional comments (32)
tests/integration/v2/staking/msg_server_test.go (9)
1-1: Package name update aligns with module restructuring
Changing the package name to staking reflects the focus on staking functionalities and improves code organization.
26-26: Verify the impact of adding the false parameter in initFixture
The initFixture function now accepts a boolean parameter. Ensure that passing false here correctly initializes the fixture without unintended side effects.
28-28: Consistent context usage with ctx := f.ctx
Reassigning ctx to f.ctx ensures consistent context management. Confirm that f.ctx is properly initialized and replaces all instances where f.sdkCtx was previously used.
61-61: Ensure HeaderInfo is present in context before accessing
When accessing integration.HeaderInfoFromContext(ctx).Time, verify that HeaderInfo is correctly set in the context to prevent potential nil pointer dereferences.
184-184: Verify the initialization with initFixture(t, false)
Passing false to initFixture in TestRotateConsPubKey might affect the test setup. Ensure that this change aligns with the intended test scenarios.
218-219: Update context header correctly before EndBlocker call
Updating the context with integration.SetHeaderInfo before invoking EndBlocker is appropriate. Confirm that the new height is accurately calculated to reflect the state changes.
231-231: Adapt malleate function signature to new context
Changing the malleate function to return context.Context aligns with the updated context management. Ensure all implementations return the correct context instance.
Line range hint 241-247: Ensure proper context handling in test cases
In the test case "successful consensus pubkey rotation", verify that the returned context from malleate is correctly used throughout the test to maintain context integrity.
Line range hint 313-331: Accurate context updates for unbonding period simulation
When simulating time passage for unbonding periods, ensure that integration.SetHeaderInfo accurately updates both Height and Time. Confirm that the added UnbondingTime correctly reflects the intended test scenario.
tests/integration/v2/staking/unbonding_test.go (2)
1-1: Package name updated to staking
The package name change improves code organization and reflects the module's focus.
Line range hint 162-183: Validate context usage in doValidatorUnbonding
Confirm that changing ctx to context.Context in doValidatorUnbonding does not affect the validator unbonding logic and that all context interactions remain valid.
tests/integration/v2/staking/deterministic_test.go (4)
1-1: Package name changed to staking
Updating the package name aligns with the module's naming conventions.
71-100: Simplify initDeterministicFixture function
The initDeterministicFixture function has been significantly refactored. Verify that all dependencies are correctly initialized and that removed components are no longer needed for the tests.
324-328: Ensure gas usage is asserted correctly
In assertNonZeroGas, make sure that the gas used is correctly measured and that zero gas usage is appropriately handled to prevent false positives in tests.
329-353: Review test logic in TestValidator
After renaming from TestGRPCValidator to TestValidator, ensure that the test still covers all necessary cases and that the refactored query function operates as intended.
tests/integration/v2/staking/slash_test.go (4)
1-1: Package name updated to staking
Changing the package name enhances code organization and module clarity.
35-35: Check the effect of passing false to initFixture
In bootstrapSlashTest, verify that initializing the fixture with false correctly sets up the test environment without unintended side effects.
Line range hint 650-733: Security concern in TestFixAvoidFullSlashPenalty
The test simulates a scenario where an attacker attempts to avoid a full slash penalty. Ensure that the test accurately reflects potential security vulnerabilities and that the slashing logic effectively mitigates such attacks.
121-130:
Ensure module accounts are properly funded
When funding the bondedPool module account, confirm that the transfer of startCoins is successful and that the account is correctly updated in the accountKeeper.
tests/integration/v2/staking/validator_test.go (2)
46-46: Verify if SetModuleAccount returns an error
Ensure that f.accountKeeper.SetModuleAccount does not return an error that needs to be handled. If it does, the error should be checked to prevent silent failures.
Run the following script to check the signature of SetModuleAccount:
33-33: Verify the impact of changing initFixture function parameters
The initFixture function is now called with true as an argument. Ensure that all usages of initFixture across the codebase are updated accordingly and that the new parameter value is correctly handled within the function.
Run the following script to check for other usages of initFixture:
x/staking/keeper/test_common.go (1)
93-117:
Ensure iterator is properly closed even if an error occurs
In the loop over the iterator, if an error occurs in store.Delete, the iterator may not be closed due to the panic. Consider deferring the iterator close immediately after its creation.
Apply this diff to ensure the iterator is closed:
iterator, err := store.Iterator(types.ValidatorsByPowerIndexKey, storetypes.PrefixEndBytes(types.ValidatorsByPowerIndexKey))
if err != nil {
panic(err)
}
+defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
// existing code...
}Likely invalid or redundant comment.
tests/integration/v2/services.go (1)
113-113: Verify the increased gas limit necessity
The gas limit has been increased by 10x from 100,000 to 1,000,000. While this allows for more complex operations, it might mask potential gas optimization issues.
✅ Verification successful
Let me analyze the gas limit usage patterns across the codebase to better understand the context of this change.
Let me analyze the test files and any gas-related test failures to understand if this gas limit increase was necessary.
Let me check the history of this change and its usage in tests to better understand the context.
Gas limit increase appears reasonable for integration tests
The gas limit increase from 100,000 to 1,000,000 is in an integration test services file, which is used for testing infrastructure. Since this is a test-only change and integration tests often need to handle complex scenarios that consume more gas than simple unit tests, the increased limit is reasonable to prevent artificial test failures. The change doesn't affect production code or actual transaction gas limits.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any tests were failing due to out of gas errors before this change
git log -p HEAD^1 | rg -B 5 -A 5 "out of gas|gas limit exceeded"
Length of output: 0
Script:
#!/bin/bash
# Search for gas-related test failures or configurations
rg -i "gas.*limit|out.*of.*gas" -g '*.go' -B 3 -A 3
# Search for gas limit usage patterns
ast-grep --pattern 'const gasLimit = $_'
Length of output: 80253
Script:
#!/bin/bash
# Search for test files that might be using this gas limit
rg -l "gasLimit.*=.*1.*000.*000" -g "*.go" -g "!vendor/*"
# Search for any gas-related test failures in recent commits
rg -B 3 -A 3 "test.*fail.*gas|fail.*test.*gas" -g "*.go"
Length of output: 151
Script:
#!/bin/bash
# Get the git history for the specific file and line to understand the change context
git log -L 113,113:tests/integration/v2/services.go
# Look for any test files that use this service
rg -B 3 -A 3 "services\.go|NewTestService" -g "*.go" -g "tests/*"
Length of output: 67
tests/integration/v2/staking/vote_extensions_test.go (1)
104-108: Provide more context for the skipped test
The skip message is vague. Please add:
- Why the integration app context cannot be used
- What changes are needed to support it
- Link to any relevant issues tracking this
tests/integration/v2/staking/common_test.go (2)
61-63: LGTM: Proper initialization of power reduction value
The initialization sets up a consistent power reduction value using a big integer calculation, which is essential for accurate testing of staking operations.
45-59: LGTM: Well-structured test fixture
The fixture struct properly encapsulates all necessary dependencies for staking tests, including keepers, context, and codec.
x/staking/keeper/cons_pubkey.go (1)
Line range hint 266-279: LGTM: Proper context type update
The function has been correctly updated to use context.Context instead of sdk.Context, maintaining consistency with the module's context handling approach.
tests/integration/v2/app.go (1)
348-351: LGTM!
The changes to use testing.TB and proper test helper pattern are well implemented.
testutil/configurator/configurator.go (1)
168-168: LGTM! Verify permission usage.
The addition of "minter" permission for the staking module is appropriate. Let's verify its usage in the staking module.
✅ Verification successful
The search results show that the "minter" permission is used appropriately in the codebase:
- The mint module uses this permission to mint new tokens and manage inflation
- The staking module is listed alongside other core modules (distribution, mint) that require minting permissions
- The permission is properly validated and checked in the auth and bank modules
LGTM! Permission usage verified.
The addition of "minter" permission for the staking module is consistent with the codebase's permission model and is used alongside other core modules that require token minting capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how the minter permission is used in the staking module
rg -A 5 "minter" --type go
Length of output: 47192
tests/integration/v2/staking/grpc_query_test.go (3)
1-1: LGTM! Package name change aligns with module structure.
The package name change from keeper_test to staking better reflects the module organization.
32-32: LGTM! Well-structured test setup and execution.
The test properly uses the fixture pattern and includes comprehensive test cases. The parallel test execution is correctly implemented.
Also applies to: 36-36, 94-94
684-701: LGTM! Comprehensive pool and params testing.
The test cases properly verify both pool state and parameters, with good error handling and assertions.
| var testStakingKeeper = &stakingkeeper.Keeper{} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Global variable testStakingKeeper should be thread-safe
Defining testStakingKeeper as a global variable may cause issues in parallel tests. Consider using a local variable or ensure thread safety when running tests with t.Parallel().
| f := initFixture(t, false, types.StakingHooksWrapper{StakingHooks: mockHooks(t, &hookCalled, &ubdeID)}) | ||
| // set global staking keeper value which will be used in mock hooks | ||
| testStakingKeeper = f.stakingKeeper | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Thread safety concern with global testStakingKeeper in parallel tests
Setting testStakingKeeper within the test may lead to race conditions when tests run in parallel. Consider refactoring to avoid global state mutation.
Apply this diff to avoid using a global variable:
-f := initFixture(t, false, types.StakingHooksWrapper{StakingHooks: mockHooks(t, &hookCalled, &ubdeID)})
-// set global staking keeper value which will be used in mock hooks
-testStakingKeeper = f.stakingKeeper
+f := initFixture(t, false, types.StakingHooksWrapper{StakingHooks: mockHooks(t, &hookCalled, &ubdeID, f.stakingKeeper)})And update the mockHooks function to accept stakingKeeper as a parameter instead of using the global variable.
Committable suggestion skipped: line range outside the PR's diff.
| nextCliffVal, _ = nextCliffVal.RemoveDelShares(math.LegacyNewDecFromInt(shares)) | ||
| _ = keeper.TestingUpdateValidator(f.stakingKeeper, f.sdkCtx, nextCliffVal, true) | ||
| _, _ = keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, nextCliffVal, true) |
There was a problem hiding this comment.
Handle the error returned by TestingUpdateValidatorV2
The error from TestingUpdateValidatorV2 is being ignored. Proper error handling ensures that test failures are caught.
Apply this diff to handle the error:
-_, _ = keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, nextCliffVal, true)
+_, _, err := keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, nextCliffVal, true)
+assert.NilError(t, err)Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| addrDels, addrVals := generateAddresses(f, numAddrs) | ||
|
|
||
| bondDenom, err := f.stakingKeeper.BondDenom(f.sdkCtx) | ||
| bondDenom, err := f.stakingKeeper.BondDenom(f.ctx) |
There was a problem hiding this comment.
Handle error returned by BondDenom
The call to f.stakingKeeper.BondDenom(f.ctx) returns an error which should be handled to avoid potential runtime issues.
Apply this diff to handle the error:
-bondDenom, err := f.stakingKeeper.BondDenom(f.ctx)
+bondDenom, err := f.stakingKeeper.BondDenom(f.ctx)
+assert.NilError(tb, 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.
| bondDenom, err := f.stakingKeeper.BondDenom(f.ctx) | |
| bondDenom, err := f.stakingKeeper.BondDenom(f.ctx) | |
| assert.NilError(tb, err) |
| assert.NilError(t, err) | ||
| // apply TM updates | ||
| applyValidatorSetUpdates(t, f.sdkCtx, f.stakingKeeper, -1) | ||
| applyValidatorSetUpdates(t, f.ctx, f.stakingKeeper, -1) |
There was a problem hiding this comment.
Ensure applyValidatorSetUpdates handles errors
The applyValidatorSetUpdates function could return an error which should be handled to avoid masking test failures.
Apply this diff to handle the error:
-applyValidatorSetUpdates(t, f.ctx, f.stakingKeeper, -1)
+updates, err := applyValidatorSetUpdates(t, f.ctx, f.stakingKeeper, -1)
+assert.NilError(t, err)
+_ = updates // Use updates if necessaryCommittable suggestion skipped: line range outside the PR's diff.
| @@ -117,7 +120,7 @@ | |||
| valAddr := validators[valIdx].OperatorAddress | |||
| addr, err := sdk.ValAddressFromBech32(valAddr) | |||
| assert.NilError(t, err) | |||
| val, _ := f.stakingKeeper.GetValidator(f.sdkCtx, addr) | |||
| val, _ := f.stakingKeeper.GetValidator(f.ctx, addr) | |||
There was a problem hiding this comment.
Check the existence of the validator after retrieval
The second return value of GetValidator indicates if the validator exists. Ignoring it might lead to nil pointer dereferences if the validator is not found.
Apply this diff to check the validator's existence:
-val, _ := f.stakingKeeper.GetValidator(f.ctx, addr)
+val, found := f.stakingKeeper.GetValidator(f.ctx, addr)
+assert.Assert(t, found, "validator not found")Committable suggestion skipped: line range outside the PR's diff.
| assert.NilError(t, err) | ||
| assert.Equal(t, params.MaxValidators, uint32(len(resValidators))) | ||
| validators[1] = keeper.TestingUpdateValidator(f.stakingKeeper, f.sdkCtx, validators[1], true) | ||
| validators[1], _ = keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, validators[1], true) | ||
| assert.Assert(ValEq(t, validators[0], resValidators[0])) |
There was a problem hiding this comment.
Assign the result of TestingUpdateValidatorV2 to the correct variables
Ensure that you are capturing both the updated validator and any validator updates from TestingUpdateValidatorV2.
Apply this diff to properly assign the returned values:
-validators[1], _ = keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, validators[1], true)
+validators[1], updates, err := keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, validators[1], true)
+assert.NilError(t, err)
+// Use updates if necessaryCommittable suggestion skipped: line range outside the PR's diff.
| val, _ = val.AddTokensFromDel(delTokens) | ||
|
|
||
| val = keeper.TestingUpdateValidator(f.stakingKeeper, f.sdkCtx, val, true) | ||
| val, _ = keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, val, true) |
There was a problem hiding this comment.
Handle the error returned by TestingUpdateValidatorV2
Ignoring the error from TestingUpdateValidatorV2 may cause unexpected behaviors in tests. It's important to handle the error to ensure the validator update was successful.
Apply this diff to handle the error:
-val, _ = keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, val, true)
+val, _, err := keeper.TestingUpdateValidatorV2(f.stakingKeeper, f.ctx, val, true)
+assert.NilError(t, err)Committable suggestion skipped: line range outside the PR's diff.
| store := k.KVStoreService.OpenKVStore(f.ctx) | ||
|
|
||
| iterator := storetypes.KVStorePrefixIterator(store, types.DelegationKey) | ||
| iterator, err := store.Iterator(types.DelegationKey, storetypes.PrefixEndBytes(types.DelegationKey)) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Handle possible errors when opening the store iterator
The call to store.Iterator can return an error that should be handled to prevent unexpected panics during benchmarking.
Apply this diff to handle the error:
-iterator, err := store.Iterator(types.DelegationKey, storetypes.PrefixEndBytes(types.DelegationKey))
-if err != nil {
- panic(err)
-}
+iterator, err := store.Iterator(types.DelegationKey, storetypes.PrefixEndBytes(types.DelegationKey))
+if err != nil {
+ b.Fatalf("Failed to create iterator: %v", err)
+}
defer iterator.Close()📝 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.
| store := k.KVStoreService.OpenKVStore(f.ctx) | |
| iterator := storetypes.KVStorePrefixIterator(store, types.DelegationKey) | |
| iterator, err := store.Iterator(types.DelegationKey, storetypes.PrefixEndBytes(types.DelegationKey)) | |
| if err != nil { | |
| panic(err) | |
| } | |
| store := k.KVStoreService.OpenKVStore(f.ctx) | |
| iterator, err := store.Iterator(types.DelegationKey, storetypes.PrefixEndBytes(types.DelegationKey)) | |
| if err != nil { | |
| b.Fatalf("Failed to create iterator: %v", err) | |
| } |
| err := k.DelegationsByValidator.Walk(f.ctx, rng, func(key collections.Pair[sdk.ValAddress, sdk.AccAddress], _ []byte) (stop bool, err error) { | ||
| valAddr, delAddr := key.K1(), key.K2() | ||
|
|
||
| delegation, err := k.Delegations.Get(f.sdkCtx, collections.Join(delAddr, valAddr)) | ||
| delegation, err := k.Delegations.Get(f.ctx, collections.Join(delAddr, valAddr)) | ||
| if err != nil { | ||
| return true, err | ||
| } | ||
|
|
||
| // remove old operator addr from delegation | ||
| if err := k.RemoveDelegation(f.sdkCtx, delegation); err != nil { | ||
| if err := k.RemoveDelegation(f.ctx, delegation); err != nil { | ||
| return true, err | ||
| } | ||
|
|
||
| delegation.ValidatorAddress = newValAddr.String() | ||
| // add with new operator addr | ||
| if err := k.SetDelegation(f.sdkCtx, delegation); err != nil { | ||
| if err := k.SetDelegation(f.ctx, delegation); err != nil { | ||
| return true, err | ||
| } |
There was a problem hiding this comment.
Check for errors when updating delegations
In the updateValidatorDelegations function, errors within the Walk method should be properly handled to avoid masking potential issues.
Apply this diff to handle errors within Walk:
err := k.DelegationsByValidator.Walk(f.ctx, rng, func(key collections.Pair[sdk.ValAddress, sdk.AccAddress], _ []byte) (stop bool, err error) {
// existing code...
-})
+})
if err != nil {
- panic(err)
+ b.Fatalf("Failed to update delegations: %v", err)
}Committable suggestion skipped: line range outside the PR's diff.
x/staking/keeper/test_common.go
Outdated
| @@ -76,3 +78,61 @@ func TestingUpdateValidator(keeper *Keeper, ctx sdk.Context, validator types.Val | |||
|
|
|||
| return validator | |||
| } | |||
|
|
|||
| // TestingUpdateValidatorV2 updates a validator in v2 for testing | |||
| func TestingUpdateValidatorV2(keeper *Keeper, ctx context.Context, validator types.Validator, apply bool) (types.Validator, []module.ValidatorUpdate) { | |||
There was a problem hiding this comment.
nit, appmodulev2.ValidatorUpdate instead of importing types/modules
| @@ -263,7 +263,7 @@ func (k Keeper) GetBlockConsPubKeyRotationHistory(ctx context.Context) ([]types. | |||
| } | |||
|
|
|||
| // GetValidatorConsPubKeyRotationHistory iterates over all the rotated history objects in the state with the given valAddr and returns. | |||
| func (k Keeper) GetValidatorConsPubKeyRotationHistory(ctx sdk.Context, operatorAddress sdk.ValAddress) ([]types.ConsPubKeyRotationHistory, error) { | |||
| func (k Keeper) GetValidatorConsPubKeyRotationHistory(ctx context.Context, operatorAddress sdk.ValAddress) ([]types.ConsPubKeyRotationHistory, error) { | |||
There was a problem hiding this comment.
We should keep track of those changes, as they should be backported.
There was a problem hiding this comment.
I will raise a backport PR manually with these changes. Will it works?
There was a problem hiding this comment.
Raised backport PR: #22863
|
|
||
| err := baseapp.ValidateVoteExtensions(f.sdkCtx, f.stakingKeeper, eci) | ||
| assert.NilError(t, err) | ||
| // TODO: uncomment below code once ValidateVoteExtensions supports app context |
There was a problem hiding this comment.
fixed with de7a18b
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/integration/v2/staking/vote_extensions_test.go (3)
36-41: Document the boolean parameter in initFixtureThe purpose of the boolean parameter passed to
initFixtureis not clear. Consider adding a comment explaining its significance.
Line range hint
44-70: Refactor validator setup for better maintainabilityConsider the following improvements:
- Extract the validator setup logic into a helper function
- Define constants for magic numbers like
1000000tokens- Consider parameterizing the
chainIDfor better test flexibility+const ( + defaultValidatorTokens = 1000000 +) + +func setupValidators(t *testing.T, f *Fixture, numVals int) ([]stakingtypes.Validator, []cryptotypes.PrivKey) { + privKeys := make([]cryptotypes.PrivKey, numVals) + for i := 0; i < numVals; i++ { + privKeys[i] = ed25519.GenPrivKey() + } + + vals := make([]stakingtypes.Validator, numVals) + for i, v := range privKeys { + // ... rest of the validator setup logic ... + } + return vals, privKeys +}
Line range hint
71-107: Improve vote extension data structure and documentationConsider these improvements:
- Use a more structured approach for vote extension data instead of simple string concatenation
- Clarify the height calculation comment to explain the relationship between vote extension signing and validation heights
-voteExt := []byte("something" + v.OperatorAddress) +// Create a structured vote extension +type VoteExtData struct { + ValidatorAddr string + Metadata string +} +voteExtData := VoteExtData{ + ValidatorAddr: v.OperatorAddress, + Metadata: "something", +} +voteExt, err := proto.Marshal(&voteExtData)baseapp/abci_utils.go (1)
Line range hint
60-137: LGTM! Well-structured validation function with improved testabilityThe function is well-designed with:
- Clear parameter separation improving testability
- Comprehensive error handling
- Proper security validation of vote extensions
The separation of parameters from context makes this function more testable and reusable. This pattern could be applied to other validation functions in the codebase for similar benefits.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
baseapp/abci_utils.go(2 hunks)tests/integration/v2/staking/vote_extensions_test.go(6 hunks)x/staking/keeper/test_common.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/integration/v2/staking/vote_extensions_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/staking/keeper/test_common.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/abci_utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
tests/integration/v2/staking/vote_extensions_test.go (2)
Line range hint 1-24: LGTM: Package structure and imports are well organized
The transition from keeper_test to staking package aligns well with the integration test structure, and the imports are properly organized.
Line range hint 109-156: LGTM: Helper functions are well-implemented
The helper functions for marshaling and sorting follow Go best practices and are properly implemented.
x/staking/keeper/test_common.go (1)
7-7: LGTM!
The import is correctly aliased and follows the Go import conventions.
baseapp/abci_utils.go (2)
18-18: LGTM!
The import is correctly placed and follows the Go import conventions.
50-58: LGTM!
Good refactoring to separate parameter handling from core validation logic. The function correctly delegates to ValidateVoteExtensionsWithParams while maintaining backward compatibility.
| // TestingUpdateValidatorV2 updates a validator in v2 for testing | ||
| func TestingUpdateValidatorV2(keeper *Keeper, ctx context.Context, validator types.Validator, apply bool) (types.Validator, []appmodulev2.ValidatorUpdate) { | ||
| err := keeper.SetValidator(ctx, validator) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| // Remove any existing power key for validator. | ||
| store := keeper.KVStoreService.OpenKVStore(ctx) | ||
| deleted := false | ||
|
|
||
| iterator, err := store.Iterator(types.ValidatorsByPowerIndexKey, storetypes.PrefixEndBytes(types.ValidatorsByPowerIndexKey)) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| defer iterator.Close() | ||
|
|
||
| bz, err := keeper.validatorAddressCodec.StringToBytes(validator.GetOperator()) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| for ; iterator.Valid(); iterator.Next() { | ||
| valAddr := types.ParseValidatorPowerRankKey(iterator.Key()) | ||
| if bytes.Equal(valAddr, bz) { | ||
| if deleted { | ||
| panic("found duplicate power index key") | ||
| } else { | ||
| deleted = true | ||
| } | ||
|
|
||
| if err = store.Delete(iterator.Key()); err != nil { | ||
| panic(err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if err = keeper.SetValidatorByPowerIndex(ctx, validator); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| var updates []appmodulev2.ValidatorUpdate | ||
|
|
||
| if apply { | ||
| updates, err = keeper.ApplyAndReturnValidatorSetUpdates(ctx) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| } | ||
|
|
||
| validator, err = keeper.GetValidator(ctx, sdk.ValAddress(bz)) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return validator, updates | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing code duplication with TestingUpdateValidator
The function duplicates most of its logic from TestingUpdateValidator. Consider extracting the common logic into a shared private function to improve maintainability.
Here's a suggested refactoring:
+func testingUpdateValidatorCommon(keeper *Keeper, ctx context.Context, validator types.Validator) error {
+ err := keeper.SetValidator(ctx, validator)
+ if err != nil {
+ return err
+ }
+
+ // Remove any existing power key for validator.
+ store := keeper.KVStoreService.OpenKVStore(ctx)
+ deleted := false
+
+ iterator, err := store.Iterator(types.ValidatorsByPowerIndexKey, storetypes.PrefixEndBytes(types.ValidatorsByPowerIndexKey))
+ if err != nil {
+ return err
+ }
+ defer iterator.Close()
+
+ bz, err := keeper.validatorAddressCodec.StringToBytes(validator.GetOperator())
+ if err != nil {
+ return err
+ }
+
+ for ; iterator.Valid(); iterator.Next() {
+ valAddr := types.ParseValidatorPowerRankKey(iterator.Key())
+ if bytes.Equal(valAddr, bz) {
+ if deleted {
+ return fmt.Errorf("found duplicate power index key")
+ }
+ deleted = true
+ if err = store.Delete(iterator.Key()); err != nil {
+ return err
+ }
+ }
+ }
+
+ return keeper.SetValidatorByPowerIndex(ctx, validator)
+}
+
func TestingUpdateValidatorV2(keeper *Keeper, ctx context.Context, validator types.Validator, apply bool) (types.Validator, []appmodulev2.ValidatorUpdate) {
- err := keeper.SetValidator(ctx, validator)
- if err != nil {
- panic(err)
- }
-
- // Remove any existing power key for validator.
- store := keeper.KVStoreService.OpenKVStore(ctx)
- deleted := false
-
- iterator, err := store.Iterator(types.ValidatorsByPowerIndexKey, storetypes.PrefixEndBytes(types.ValidatorsByPowerIndexKey))
- if err != nil {
- panic(err)
- }
- defer iterator.Close()
-
- bz, err := keeper.validatorAddressCodec.StringToBytes(validator.GetOperator())
- if err != nil {
- panic(err)
- }
-
- for ; iterator.Valid(); iterator.Next() {
- valAddr := types.ParseValidatorPowerRankKey(iterator.Key())
- if bytes.Equal(valAddr, bz) {
- if deleted {
- panic("found duplicate power index key")
- } else {
- deleted = true
- }
-
- if err = store.Delete(iterator.Key()); err != nil {
- panic(err)
- }
- }
- }
-
- if err = keeper.SetValidatorByPowerIndex(ctx, validator); err != nil {
+ bz, err := keeper.validatorAddressCodec.StringToBytes(validator.GetOperator())
+ if err != nil {
panic(err)
}
+
+ if err := testingUpdateValidatorCommon(keeper, ctx, validator); err != nil {
+ panic(err)
+ }
var updates []appmodulev2.ValidatorUpdate
if apply {
updates, err = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
if err != nil {
panic(err)
}
}
validator, err = keeper.GetValidator(ctx, sdk.ValAddress(bz))
if err != nil {
panic(err)
}
return validator, updates
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/integration/v2/staking/msg_server_test.go (2)
Line range hint
26-61: Consider adding initial state validationWhile the test setup is correct, consider adding assertions to validate the initial state:
- Validator's initial status
- Initial module account balances
- Initial unbonding delegation state
Line range hint
74-182: Consider additional test scenariosThe test cases provide good coverage, but consider adding:
- Test for multiple concurrent unbonding delegations
- Test for maximum unbonding entries limit
- Test for zero amount cancellation
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/v2/staking/msg_server_test.go(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/integration/v2/staking/msg_server_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (2)
tests/integration/v2/staking/msg_server_test.go (2)
Line range hint 1-22: LGTM: Clean package structure and imports
The package name change and import organization align well with the v2 framework standards.
218-223: Validate state transitions explicitly
The EndBlocker and Commit sequence should include explicit validation of:
- Validator set changes
- Consensus state updates
- Block height progression
This helps catch potential race conditions in state transitions.
| ctxHeader := integration.HeaderInfoFromContext(ctx) | ||
| newHeight = ctxHeader.Height + 1 | ||
| newCtx := integration.SetHeaderInfo(ctx, header.Info{Height: newHeight + 1, Time: ctxHeader.Time.Add(params.UnbondingTime)}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve time management safety
The time manipulation could be made safer by:
- Using constants for time increments
- Adding validation for time progression
- Ensuring no negative time jumps
Consider extracting time management into a helper function to ensure consistency.
- ctxHeader := integration.HeaderInfoFromContext(ctx)
- newHeight = ctxHeader.Height + 1
- newCtx := integration.SetHeaderInfo(ctx, header.Info{Height: newHeight + 1, Time: ctxHeader.Time.Add(params.UnbondingTime)})
+ func advanceTimeToUnbonding(ctx context.Context, params types.Params) context.Context {
+ ctxHeader := integration.HeaderInfoFromContext(ctx)
+ newHeight := ctxHeader.Height + 1
+ newTime := ctxHeader.Time.Add(params.UnbondingTime)
+ if newTime.Before(ctxHeader.Time) {
+ panic("invalid time progression")
+ }
+ return integration.SetHeaderInfo(ctx, header.Info{
+ Height: newHeight + 1,
+ Time: newTime,
+ })
+ }
+ newCtx := advanceTimeToUnbonding(ctx, params)Committable suggestion skipped: line range outside the PR's diff.
| @@ -110,7 +110,7 @@ func GasMeterFactory(ctx context.Context) func() gas.Meter { | |||
| } | |||
|
|
|||
| func (s storeService) OpenKVStore(ctx context.Context) corestore.KVStore { | |||
| const gasLimit = 100_000 | |||
| const gasLimit = 1_000_000 | |||
There was a problem hiding this comment.
Few of deterministic tests are failing with out of gas issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integration/v2/app.go (1)
107-107: Rename parameter to follow Go conventionsThe parameter type change from
*testing.Ttotesting.TBis a good improvement as it allows the function to be used with both tests and benchmarks. However, the parameter should be namedtbto follow Go conventions for test helpers.-func DefaultStartUpConfig(t testing.TB) StartupConfig { +func DefaultStartUpConfig(tb testing.TB) StartupConfig {🧰 Tools
🪛 golangci-lint (1.62.2)
107-107: parameter testing.TB should have name tb
(thelper)
tests/integration/v2/staking/slash_test.go (1)
Line range hint
646-734: Consider adding more assertions in TestFixAvoidFullSlashPenalty.While the test successfully reproduces and fixes issue #20641, consider adding assertions to verify:
- The final state of delegations after slashing
- The impact on other delegators (testAcc1, testAcc2)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/integration/v2/app.go(2 hunks)tests/integration/v2/services.go(1 hunks)tests/integration/v2/staking/msg_server_test.go(13 hunks)tests/integration/v2/staking/slash_test.go(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/v2/services.go
- tests/integration/v2/staking/msg_server_test.go
🧰 Additional context used
📓 Path-based instructions (2)
tests/integration/v2/staking/slash_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/integration/v2/app.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
tests/integration/v2/app.go
107-107: parameter testing.TB should have name tb
(thelper)
🔇 Additional comments (8)
tests/integration/v2/app.go (2)
Line range hint 354-362: LGTM! Parameter type update is consistent
The changes look good. The function correctly implements the test helper pattern and the parameter type change to testing.TB is consistent with the earlier changes.
Line range hint 300-350: LGTM! Block height synchronization is properly implemented
The addition of block height synchronization for the integration context is well implemented. It correctly:
- Checks for the presence of integration context
- Updates the height only when the context is present
- Maintains consistency between app state and context state
🧰 Tools
🪛 golangci-lint (1.62.2)
107-107: parameter testing.TB should have name tb
(thelper)
tests/integration/v2/staking/slash_test.go (6)
Line range hint 1-29: LGTM: Package organization and imports are well structured.
The package name change from keeper_test to staking better reflects the module-based organization pattern.
Line range hint 31-65: LGTM: Helper functions properly migrated to use context.Context.
The helper functions have been successfully updated to use the new context type while maintaining their original functionality.
Also applies to: 737-741
Line range hint 66-396: LGTM: Comprehensive test coverage for slashing with unbonding delegations.
The test cases thoroughly cover the slashing behavior with unbonding delegations, including edge cases and multiple slashing scenarios.
Line range hint 397-559: LGTM: Well-structured tests for slashing with redelegations.
The test cases effectively verify the slashing behavior with redelegations, including proper balance checks and validator status verification.
560-630: LGTM: Thorough testing of combined slashing scenarios.
The test cases properly verify the slashing behavior when both unbonding delegations and redelegations are present.
631-645: LGTM: Good coverage of slashing amount calculations.
The test cases verify both successful slashing amounts and edge cases where validators are not found.
Description
ref: #20799
ModuleAccPermissionsin configurator'sAuthModule()to add minting permission for staking moduleValidateVoteExtensionsto support integration v2 app framework contextGetValidatorConsPubKeyRotationHistorymethod in staking keeper to usecontext.ContextAuthor 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
Bug Fixes
Refactor
Documentation