fix: check unbonding delegations in ExecuteAddValidator#126
fix: check unbonding delegations in ExecuteAddValidator#126kpitapeersyst wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughThe POA keeper's AddValidator flow now queries unbonding delegations by delegator account with ChangesPOA unbonding lookup change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kpitapeersyst
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
x/poa/keeper/keeper_test.go (1)
29-29: TightenGetUnbondingDelegationsexpectations to assert the bugfix contract.The keeper implementation calls
GetUnbondingDelegations(ctx, accAddress, gomath.MaxUint16)with concrete delegator and maxRetrieve values. Usinggomock.Any()for both parameters in test expectations weakens coverage of this critical behavior. Prefer asserting the concrete delegator address anduint16(gomath.MaxUint16)in test cases.Note: Line 29 resides in the shared setup function and uses
.AnyTimes(), which may warrant careful review if tightened to avoid unintended side effects on other tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/poa/keeper/keeper_test.go` at line 29, Tighten the mock expectation for stakingKeeper.EXPECT().GetUnbondingDelegations to assert the concrete delegator address and the maxRetrieve value used by the keeper: replace gomock.Any() for the delegator and maxRetrieve args with the actual accAddress and uint16(gomath.MaxUint16) (or the exact value cast used in the keeper), and avoid using .AnyTimes() if that would impact other tests—scope the expectation to the specific test or use .Times(1) so the contract GetUnbondingDelegations(ctx, accAddress, uint16(gomath.MaxUint16)) is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@x/poa/keeper/keeper_test.go`:
- Line 29: Tighten the mock expectation for
stakingKeeper.EXPECT().GetUnbondingDelegations to assert the concrete delegator
address and the maxRetrieve value used by the keeper: replace gomock.Any() for
the delegator and maxRetrieve args with the actual accAddress and
uint16(gomath.MaxUint16) (or the exact value cast used in the keeper), and avoid
using .AnyTimes() if that would impact other tests—scope the expectation to the
specific test or use .Times(1) so the contract GetUnbondingDelegations(ctx,
accAddress, uint16(gomath.MaxUint16)) is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a98ac44-95db-4d20-8ac0-f7d4c60e3ea6
📒 Files selected for processing (4)
x/poa/keeper/keeper.gox/poa/keeper/keeper_test.gox/poa/testutil/expected_keepers_mock.gox/poa/types/expected_keepers.go
|
CI failure is unrelated to this PR, will be resolved once #143 is merged. |
2 participants
fix: check unbonding delegations from delegator perspective in ExecuteAddValidator
Motivation 💡
In
ExecuteAddValidator, the unbonding-delegation guard calledGetUnbondingDelegationsFromValidator(ctx, valAddress), which returns delegations where the given address acts as a validator. A candidate being added as a new validator has never been a validator before, so the call always returned an empty slice and the!unbondingBalance.IsZero()check on the following lines could never fire. The intent of the check (block addresses that already hold staking power via in-flight unbonding) was never being enforced, giving a false sense of safety. While the ante handler currently blocksMsgUndelegate, genesis state can still inject unbonding delegations, so the guard needs to actually work.Changes 🛠
GetUnbondingDelegationsFromValidator(ctx, valAddress)withGetUnbondingDelegations(ctx, accAddress, gomath.MaxUint16)inx/poa/keeper/keeper.goso the check looks at the candidate as a delegator instead of as a validatorGetUnbondingDelegationsto theStakingKeeperinterface inx/poa/types/expected_keepers.gox/poa/testutil/expected_keepers_mock.goto include the new methodx/poa/keeper/keeper_test.goto mock and assert againstGetUnbondingDelegationsConsiderations 🤔
gomath.MaxUint16is used asmaxRetrieveto get every unbonding delegation for the account.Summary by CodeRabbit
Dependencies 📦
Refactor
Tests
Chores