fix: poa unbond all validator delegations on removal#141
Draft
kpitapeersyst wants to merge 1 commit into
Draft
Conversation
📝 WalkthroughWalkthroughThe POA keeper's ExecuteRemoveValidator now fetches all delegations for a validator, decodes delegator Bech32 addresses, and unbonds each delegation individually. A StakingKeeper interface method GetValidatorDelegations was added and mocks/tests updated to exercise delegation-driven unbonding. ChangesDelegation-Based Unbonding Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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
force-pushed
the
fix/poa-remove-validator-state
branch
from
6143a41 to
7f71d01
Compare
![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.go (1)
257-264: 💤 Low valueOptional: consolidate the parallel
delegations/delegatorAddrsslices.Index-aligned parallel slices are easy to break in future refactors. A small local struct (or storing the pair) makes the relationship explicit without changing behavior.
♻️ Proposed refactor
- delegatorAddrs := make([]sdk.AccAddress, 0, len(delegations)) - for _, del := range delegations { - delAddr, err := sdk.AccAddressFromBech32(del.DelegatorAddress) - if err != nil { - return errors.Wrapf(err, "invalid delegator address %q for validator %q", del.DelegatorAddress, validatorAddress) - } - delegatorAddrs = append(delegatorAddrs, delAddr) - } + type pendingUnbond struct { + delAddr sdk.AccAddress + shares math.LegacyDec + bech32 string + } + pending := make([]pendingUnbond, 0, len(delegations)) + for _, del := range delegations { + delAddr, err := sdk.AccAddressFromBech32(del.DelegatorAddress) + if err != nil { + return errors.Wrapf(err, "invalid delegator address %q for validator %q", del.DelegatorAddress, validatorAddress) + } + pending = append(pending, pendingUnbond{delAddr: delAddr, shares: del.Shares, bech32: del.DelegatorAddress}) + } @@ - for i, del := range delegations { - if _, err := k.sk.Unbond(ctx, delegatorAddrs[i], valAddress, del.Shares); err != nil { - return errors.Wrapf(err, "failed to unbond delegation from %q to validator %q", del.DelegatorAddress, validatorAddress) + for _, p := range pending { + if _, err := k.sk.Unbond(ctx, p.delAddr, valAddress, p.shares); err != nil { + return errors.Wrapf(err, "failed to unbond delegation from %q to validator %q", p.bech32, validatorAddress) } }Also applies to: 288-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/poa/keeper/keeper.go` around lines 257 - 264, The code builds index-aligned parallel slices delegations and delegatorAddrs which is error-prone; refactor to a single local struct slice (e.g., type delegatorPair struct { delegation stakingtypes.Delegation; addr sdk.AccAddress }) and populate that in the loop that currently calls sdk.AccAddressFromBech32 for each del and appends to delegatorAddrs (refer to variables delegations, delegatorAddrs, sdk.AccAddressFromBech32 and validatorAddress); update subsequent usage sites (the block around lines 288-295 mentioned in the comment) to iterate the new slice of pairs instead of relying on two parallel slices, preserving the existing error handling and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@x/poa/keeper/keeper.go`:
- Around line 257-264: The code builds index-aligned parallel slices delegations
and delegatorAddrs which is error-prone; refactor to a single local struct slice
(e.g., type delegatorPair struct { delegation stakingtypes.Delegation; addr
sdk.AccAddress }) and populate that in the loop that currently calls
sdk.AccAddressFromBech32 for each del and appends to delegatorAddrs (refer to
variables delegations, delegatorAddrs, sdk.AccAddressFromBech32 and
validatorAddress); update subsequent usage sites (the block around lines 288-295
mentioned in the comment) to iterate the new slice of pairs instead of relying
on two parallel slices, preserving the existing error handling and return
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54f19a55-80f7-4dd6-92e9-f9d36fd0cac3
📒 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
🚧 Files skipped from review as they are similar to previous changes (1)
- x/poa/testutil/expected_keepers_mock.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: poa unbond all validator delegations on removal
Motivation 💡
Changes 🛠
GetValidatorDelegationsto the expected staking keeper interface and generated mock.ExecuteRemoveValidatorunit tests for self-delegation, foreign delegators, invalid delegator addresses, and unbond failures.Summary by CodeRabbit
Bug Fixes
Tests