coderabbitai[bot]
commented
3 months ago Walkthrough
## Walkthrough The changes primarily involve replacing calls to the `GetLastValidators` method with the `IterateLastValidatorPowers` method across various files. This update ensures safer and more efficient retrieval of validator information, preventing potential panics during block processing. The modifications span multiple packages, including test utilities and various keeper files, to maintain consistency and enhance testing capabilities. ## Changes | Files & Paths | Change Summary | |---------------|----------------| | `testutil/ibc_testing/generic_setup.go` | Replaced `providerApp.GetTestStakingKeeper().GetLastValidators` with `providerKeeper.GetLastBondedValidators`. | | `testutil/keeper/expectations.go`, `testutil/keeper/unit_test_helpers.go`, `testutil/keeper/mocks.go` | Updated mock setups and expectations for last bonded validators, including new parameters for flexibility. Removed `GetLastValidators` from mocks. | | `x/ccv/provider/keeper/grpc_query.go`, `x/ccv/provider/keeper/keeper.go`, `x/ccv/provider/keeper/partial_set_security.go`, `x/ccv/provider/keeper/proposal.go`, `x/ccv/provider/keeper/relay.go`, `x/ccv/provider/keeper/validator_set_update.go` | Introduced `GetLastBondedValidators` function and replaced calls to `stakingKeeper.GetLastValidators` with `k.GetLastBondedValidators`. | | `x/ccv/provider/keeper/proposal_handler_test.go`, `x/ccv/provider/keeper/relay_test.go` | Updated tests to use `SetupMocksForLastBondedValidatorsExpectation` for setting up mocks for last bonded validators. | | `x/ccv/types/expected_keepers.go` | Removed `GetLastValidators` method from `StakingKeeper` interface. | | `tests/integration/unbonding.go` | Replaced `GetLastValidators` with `GetLastBondedValidators` in `CCVTestSuite`. | | `x/ccv/consumer/keeper/changeover_test.go`, `x/ccv/consumer/keeper/keeper.go`, `x/ccv/consumer/keeper/keeper_test.go` | Updated references and mocks from `stakingKeeper` to `consumerKeeper` and used `GetLastBondedValidators` instead of `GetLastValidators`. | | `x/ccv/types/utils.go` | Added `GetLastBondedValidatorsUtil` function for retrieving bonded validators. | ## Sequence Diagram(s) ```mermaid sequenceDiagram participant Client participant ProviderKeeper participant StakingKeeper Client->>ProviderKeeper: Call GetLastBondedValidators() ProviderKeeper->>StakingKeeper: IterateLastValidatorPowers() StakingKeeper-->>ProviderKeeper: Return validators ProviderKeeper-->>Client: Return bonded validators ``` ## Assessment against linked issues | Objective | Addressed | Explanation | |-----------|-----------|-------------| | Replace calls to `GetLastValidators` with `IterateLastValidatorPowers` (#1947) | ✅ | |Recent review details
**Configuration used: .coderabbit.yml** **Review profile: CHILL**Commits
Files that changed from the base of the PR and between 34d63b2b59d448469df472713131adc255aea8ab and 85a17423e58f5562940bd74f2915a9c47413c445.Files selected for processing (6)
* tests/integration/unbonding.go (3 hunks) * x/ccv/consumer/keeper/changeover_test.go (4 hunks) * x/ccv/consumer/keeper/keeper.go (2 hunks) * x/ccv/consumer/keeper/keeper_test.go (1 hunks) * x/ccv/provider/keeper/validator_set_update.go (2 hunks) * x/ccv/types/utils.go (2 hunks)Additional context used
Path-based instructions (6)
x/ccv/types/utils.go (1)
Pattern `**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/ccv/consumer/keeper/changeover_test.go (2)
Pattern `**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations. --- Pattern `**/*_test.go`: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/ccv/provider/keeper/validator_set_update.go (1)
Pattern `**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/ccv/consumer/keeper/keeper_test.go (2)
Pattern `**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations. --- Pattern `**/*_test.go`: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/unbonding.go (2)
Pattern `**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations. --- Pattern `tests/integration/*`: "Assess the e2e test code assessing sufficient code coverage for the changes associated in the pull request"x/ccv/consumer/keeper/keeper.go (1)
Pattern `**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
x/ccv/consumer/keeper/changeover_test.go (1)
`104-110`: The updated mock setup using `SetupMocksForLastBondedValidatorsExpectation` is correctly implemented to handle the new functionality. Ensure that the test cases adequately cover scenarios where the `IterateLastValidatorPowers` might return different numbers of validators than expected.x/ccv/provider/keeper/validator_set_update.go (1)
`194-198`: The implementation of `GetLastBondedValidators` correctly delegates to the utility function `GetLastBondedValidatorsUtil`, ensuring consistency and reusability of the logic. This is a good use of abstraction to avoid code duplication.x/ccv/consumer/keeper/keeper_test.go (1)
`187-192`: The updated mock setup using `SetupMocksForLastBondedValidatorsExpectation` in the test `TestGetLastSovereignValidators` is correctly implemented. Consider adding a test case to handle scenarios where no validators are returned by the `IterateLastValidatorPowers` to ensure robustness.tests/integration/unbonding.go (1)
Line range hint `470-512`: The test `TestTooManyLastValidators` effectively checks the stability of the system when a validator is jailed and then unjailed, ensuring no panics occur due to state inconsistencies. This is crucial for maintaining the integrity of the validator set updates.x/ccv/consumer/keeper/keeper.go (1)
`694-698`: The implementation of `GetLastBondedValidators` utilizes a utility function to fetch validators, which is a good use of abstraction. However, ensure that the utility function handles all edge cases, especially around the handling of `MaxValidators`.Verification successful
--- The utility function `GetLastBondedValidatorsUtil` handles edge cases appropriately, including the handling of `MaxValidators`. It correctly stops iteration when the maximum number of validators is reached, truncates the list of validators to the actual number found, and logs and skips invalid addresses and missing validators. -Scripts executed
The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Description: Verify the utility function handles all edge cases. # Test: Search for the utility function usage and ensure it's robust. rg --type go $'GetLastBondedValidatorsUtil' ``` Length of output: 430 --- --- Script: ```shell #!/bin/bash # Extract the implementation of GetLastBondedValidatorsUtil from x/ccv/types/utils.go ast-grep --lang go --pattern $'func GetLastBondedValidatorsUtil(ctx sdk.Context, stakingKeeper StakingKeeper, logger log.Logger) []stakingtypes.Validator { $$$ }' -- x/ccv/types/utils.go ``` Length of output: 2219
sainoe
Description
Closes: #1947
This replaces the call to stakingKeeper.GetLastValidators with calls to providerKeeper.GetLastBondedValidators, which in turn is a wrapper around stakingKeeper.IterateLastValidatorPowers
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...
[x] Included the correct type prefix in the PR title
[x] Added
!to the type prefix if the change is state-machine breaking[x] Confirmed this PR does not introduce changes requiring state migrations, OR migration code has been added to consumer and/or provider modules
[x] Targeted the correct branch (see PR Targeting)
[x] Provided a link to the relevant issue or specification
[x] Followed the guidelines for building SDK modules
[x] Included the necessary unit and integration tests
[ ] Added a changelog entry to
CHANGELOG.mdNo changelog needed since this is just a refactor?[x] Included comments for documenting Go code
[x] Updated the relevant documentation or specification
[x] Reviewed "Files changed" and left comments if necessary
[x] Confirmed all CI checks have passed
[x] If this PR is library API breaking, bump the go.mod version string of the repo, and follow through on a new major release
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...
!the type prefix if the change is state-machine breakingSummary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor