feat(poa): add validator self-removal message#146
Merged
AdriaCarrera merged 7 commits intoJun 23, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesSelfRemoveValidator feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@x/poa/keeper/keeper.go`:
- Around line 293-297: In the validator address parsing logic, change from
parsing validatorAddress as an account address using sdk.AccAddressFromBech32
and then coercing to validator address, to directly parsing it as a validator
address using sdk.ValAddressFromBech32. Assign the result to valAddress
directly, then derive the accAddress from the validator address bytes if needed
by calling sdk.AccAddress on the valAddress bytes. This ensures the proper
bech32 format (valoper prefix) is accepted and self-removal for standard
validator addresses works correctly.
- Around line 299-302: In the GetValidator error handling block around line
299-302, the current code returns ErrAddressIsNotAValidator for all errors from
k.sk.GetValidator, which masks real staking keeper failures. Instead, check if
the error is specifically a not-found error (typically using
sdk.ErrUnknownAddress or similar) and only then return
ErrAddressIsNotAValidator; for all other errors, return the original error
unchanged to preserve important debugging information about store or query
issues.
In `@x/poa/keeper/msg_server_remove_validator_self.go`:
- Around line 10-18: The RemoveValidatorSelf message handler is missing an
authority validation check before mutating the validator set. Add an explicit
authority gate check in the RemoveValidatorSelf method that verifies
msg.Authority equals k.authority before calling ExecuteRemoveValidatorSelf. If
the authority does not match, return an appropriate error (typically an
unauthorized or permission denied error) to enforce the POA module's authority
gate invariant that requires authority verification before any validator set
mutations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74b0d139-8c9c-4763-834d-6feec76e4257
⛔ Files ignored due to path filters (1)
x/poa/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
proto/poa/tx.protox/poa/keeper/keeper.gox/poa/keeper/msg_server_remove_validator_self.gox/poa/keeper/msg_server_remove_validator_self_test.gox/poa/keeper/msg_server_remove_validator_test.gox/poa/types/codec.gox/poa/types/errors.gox/poa/types/events.gox/poa/types/message_remove_validator_self.go
Comment on lines
+299
to
+302
| validator, err := k.sk.GetValidator(ctx, valAddress) | ||
| if err != nil { | ||
| return types.ErrAddressIsNotAValidator | ||
| } |
There was a problem hiding this comment.
Do not collapse all GetValidator failures into “not a validator”.
Returning ErrAddressIsNotAValidator for every error masks real staking keeper failures (store/query issues) and can mislead callers. Only map the specific not-found case; propagate other errors unchanged.
Proposed fix
validator, err := k.sk.GetValidator(ctx, valAddress)
if err != nil {
- return types.ErrAddressIsNotAValidator
+ if errors.IsOf(err, stakingtypes.ErrNoValidatorFound) {
+ return types.ErrAddressIsNotAValidator
+ }
+ return err
}🤖 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 299 - 302, In the GetValidator error
handling block around line 299-302, the current code returns
ErrAddressIsNotAValidator for all errors from k.sk.GetValidator, which masks
real staking keeper failures. Instead, check if the error is specifically a
not-found error (typically using sdk.ErrUnknownAddress or similar) and only then
return ErrAddressIsNotAValidator; for all other errors, return the original
error unchanged to preserve important debugging information about store or query
issues.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
x/poa/keeper/keeper.go (2)
293-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse
validatorAddressas a validator address, not an account address.At Line 293,
sdk.AccAddressFromBech32can reject valid validator-form bech32 input and break self-removal for normal validator addresses. Parse withsdk.ValAddressFromBech32first.Proposed fix
- accAddress, err := sdk.AccAddressFromBech32(validatorAddress) + valAddress, err := sdk.ValAddressFromBech32(validatorAddress) if err != nil { return err } - valAddress := sdk.ValAddress(accAddress) + accAddress := sdk.AccAddress(valAddress)🤖 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 293 - 297, In the code block where validatorAddress is being processed, the issue is that sdk.AccAddressFromBech32 is being used to parse validatorAddress, which expects an account-format bech32 address. Instead, parse validatorAddress directly as a validator address using sdk.ValAddressFromBech32, which will properly handle validator-format bech32 addresses (like those with "val" prefix). Remove the intermediate step that converts from account address to validator address using sdk.ValAddress, and assign the result directly from sdk.ValAddressFromBech32 to valAddress.
299-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not convert every
GetValidatorfailure into “not a validator.”At Line 300-302, all errors are mapped to
types.ErrAddressIsNotAValidator. This hides real staking/store failures and makes debugging harder. Only map the specific not-found case; return other errors unchanged.Proposed fix
validator, err := k.sk.GetValidator(ctx, valAddress) if err != nil { ctx.Logger().Warn("Error getting validator", "error", err) - return types.ErrAddressIsNotAValidator + if errors.IsOf(err, stakingtypes.ErrNoValidatorFound) { + return types.ErrAddressIsNotAValidator + } + return err }🤖 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 299 - 303, The error handling in the GetValidator call block is converting all errors to types.ErrAddressIsNotAValidator, which masks real failures in the staking keeper or store. Instead of catching all errors at line 300-302 with the k.sk.GetValidator call on valAddress, you need to differentiate between a specific not-found error (which should map to types.ErrAddressIsNotAValidator) and other error types that represent actual failures and should be returned unchanged. Check if the returned error is specifically a "not found" or "not a validator" type error before converting it; otherwise, return the original error to preserve debugging information and avoid hiding real staking/store failures.
🤖 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.
Duplicate comments:
In `@x/poa/keeper/keeper.go`:
- Around line 293-297: In the code block where validatorAddress is being
processed, the issue is that sdk.AccAddressFromBech32 is being used to parse
validatorAddress, which expects an account-format bech32 address. Instead, parse
validatorAddress directly as a validator address using sdk.ValAddressFromBech32,
which will properly handle validator-format bech32 addresses (like those with
"val" prefix). Remove the intermediate step that converts from account address
to validator address using sdk.ValAddress, and assign the result directly from
sdk.ValAddressFromBech32 to valAddress.
- Around line 299-303: The error handling in the GetValidator call block is
converting all errors to types.ErrAddressIsNotAValidator, which masks real
failures in the staking keeper or store. Instead of catching all errors at line
300-302 with the k.sk.GetValidator call on valAddress, you need to differentiate
between a specific not-found error (which should map to
types.ErrAddressIsNotAValidator) and other error types that represent actual
failures and should be returned unchanged. Check if the returned error is
specifically a "not found" or "not a validator" type error before converting it;
otherwise, return the original error to preserve debugging information and avoid
hiding real staking/store failures.
AdriaCarrera
requested changes
Jun 19, 2026
|
|
||
| // MsgRemoveValidatorSelf defines a message for a validator to remove itself | ||
| message MsgRemoveValidatorSelf { | ||
| option (cosmos.msg.v1.signer) = "validator_address"; |
| message MsgRemoveValidatorResponse {} | ||
|
|
||
| // MsgRemoveValidatorSelf defines a message for a validator to remove itself | ||
| message MsgRemoveValidatorSelf { |
Contributor
There was a problem hiding this comment.
Rename to SelfRemoveValidator
| return types.ErrAddressIsNotAValidator | ||
| } | ||
|
|
||
| // Prevent the last bonded validator from removing itself. If it is not bonded, the |
Contributor
There was a problem hiding this comment.
This logic is not needed
Contributor
There was a problem hiding this comment.
Check if preventing last validator removal is present in x/staking
Contributor
Author
There was a problem hiding this comment.
Checked x/staking, and there is no last-validator guard. Removed the one that I added.
| "github.com/xrplevm/node/v10/x/poa/types" | ||
| ) | ||
|
|
||
| func (k msgServer) RemoveValidatorSelf(goCtx context.Context, msg *types.MsgRemoveValidatorSelf) (*types.MsgRemoveValidatorSelfResponse, error) { |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
x/poa/keeper/keeper.go (2)
293-298:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse self-removal input as validator bech32, not account bech32.
Line 293 currently uses
sdk.AccAddressFromBech32, which rejects valid validator-operator formatted inputs and can break self-removal for normal validator addresses.Suggested fix
- accAddress, err := sdk.AccAddressFromBech32(validatorAddress) + valAddress, err := sdk.ValAddressFromBech32(validatorAddress) if err != nil { return err } - valAddress := sdk.ValAddress(accAddress) + accAddress := sdk.AccAddress(valAddress) + _ = accAddress#!/bin/bash # Verify address parsing choices on self-remove path rg -n -C3 'ExecuteSelfRemoveValidator|AccAddressFromBech32|ValAddressFromBech32' x/poa/keeper/keeper.go rg -n -C2 'TestMsgServer_SelfRemoveValidator|validatorAddress|MsgSelfRemoveValidator' x/poa/keeper/msg_server_self_remove_validator_test.go🤖 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 293 - 298, The address parsing in the self-removal validator code path is currently using sdk.AccAddressFromBech32 to parse validatorAddress, which incorrectly attempts to parse a validator-formatted bech32 address as an account address. Fix this by directly parsing validatorAddress as a validator address using sdk.ValAddressFromBech32 instead, and assign the result directly to valAddress, eliminating the intermediate accAddress conversion step. This change should be made in the code block handling validatorAddress parsing around the ExecuteSelfRemoveValidator or MsgSelfRemoveValidator logic.
299-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not collapse all staking lookup failures into “not a validator”.
Line 302 maps every
GetValidatorerror toErrAddressIsNotAValidator, which hides real keeper/storage failures and reduces debuggability.Suggested fix
validator, err := k.sk.GetValidator(ctx, valAddress) if err != nil { ctx.Logger().Warn("Error getting validator", "error", err) - return types.ErrAddressIsNotAValidator + if errors.IsOf(err, stakingtypes.ErrNoValidatorFound) { + return types.ErrAddressIsNotAValidator + } + return err }#!/bin/bash # Verify current GetValidator error mapping sites rg -n -C3 'GetValidator\(ctx, valAddress\)|ErrAddressIsNotAValidator' x/poa/keeper/keeper.go🤖 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 299 - 303, The GetValidator call in the validator lookup block returns all errors (keeper failures, storage issues, etc.) as ErrAddressIsNotAValidator, which obscures real system errors. Instead of mapping every error from k.sk.GetValidator(ctx, valAddress) to types.ErrAddressIsNotAValidator, distinguish between cases where the validator truly does not exist versus actual keeper/storage errors, and handle them appropriately by either propagating the real error for debugging or returning the appropriate error type only when the validator is genuinely not found.
🤖 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.
Inline comments:
In `@proto/poa/tx.proto`:
- Around line 53-57: Add an authority field to the MsgSelfRemoveValidator
message definition to enable authority validation in the message handler. Insert
a new field after the existing address field in MsgSelfRemoveValidator that
captures the authority address, following the same pattern used for the address
field with appropriate cosmos protobuf annotations for scalar types. This will
allow the handler to perform the required msg.Authority == k.authority check
before any validator-set mutations.
---
Duplicate comments:
In `@x/poa/keeper/keeper.go`:
- Around line 293-298: The address parsing in the self-removal validator code
path is currently using sdk.AccAddressFromBech32 to parse validatorAddress,
which incorrectly attempts to parse a validator-formatted bech32 address as an
account address. Fix this by directly parsing validatorAddress as a validator
address using sdk.ValAddressFromBech32 instead, and assign the result directly
to valAddress, eliminating the intermediate accAddress conversion step. This
change should be made in the code block handling validatorAddress parsing around
the ExecuteSelfRemoveValidator or MsgSelfRemoveValidator logic.
- Around line 299-303: The GetValidator call in the validator lookup block
returns all errors (keeper failures, storage issues, etc.) as
ErrAddressIsNotAValidator, which obscures real system errors. Instead of mapping
every error from k.sk.GetValidator(ctx, valAddress) to
types.ErrAddressIsNotAValidator, distinguish between cases where the validator
truly does not exist versus actual keeper/storage errors, and handle them
appropriately by either propagating the real error for debugging or returning
the appropriate error type only when the validator is genuinely not found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d314bd96-a6e3-413e-9a6d-18b55008d39e
⛔ Files ignored due to path filters (1)
x/poa/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
proto/poa/tx.protox/poa/keeper/keeper.gox/poa/keeper/msg_server_self_remove_validator.gox/poa/keeper/msg_server_self_remove_validator_test.gox/poa/types/codec.gox/poa/types/events.gox/poa/types/message_self_remove_validator.go
AdriaCarrera
requested changes
Jun 22, 2026
AdriaCarrera
left a comment
AdriaCarrera
left a comment
Contributor
There was a problem hiding this comment.
Add integration tests in poa testsuite
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/integration/poa_test.go`:
- Around line 647-654: The assertion with require.Contains on the err.Error()
call does not guard against err being nil. If the Validator query unexpectedly
succeeds, err will be nil and calling err.Error() will cause a panic instead of
a clean test failure. Before calling err.Error() in the require.Contains
statement, first assert that err is not nil using require.Error to ensure the
validator query actually failed as expected before checking the error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d736e6d0-2bc0-48e1-844d-a38aea32c302
📒 Files selected for processing (1)
tests/integration/poa_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@tests/integration/poa_test.go`:
- Around line 763-770: The test is dereferencing err without first asserting it
is not nil, which can cause a panic and hide the actual test failure. Before
calling err.Error() in the require.Contains assertion on the line after the
Validator call, add a guard using require.Error(s.T(), err) to ensure the error
is not nil and the Validator call actually failed as expected.
- Around line 701-705: The code checks that the validators slice is non-empty
but does not validate that tc.valIndex is within the valid range of the
validators slice before indexing into it on the line where validator is
assigned. Add a bounds check using require.Less or require.True to ensure that
tc.valIndex is less than len(validators) after confirming validators is
non-empty, preventing potential panics when test cases use indices like 1 on
single-validator setups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 626fb00b-9bd9-460c-9aed-45e99765fd7f
📒 Files selected for processing (1)
tests/integration/poa_test.go
Comment on lines
+701
to
+705
| validators := s.Network().GetValidators() | ||
| require.NotZero(s.T(), len(validators)) | ||
|
|
||
| validator := validators[tc.valIndex] | ||
| valAddr, err := sdktypes.ValAddressFromBech32(validator.OperatorAddress) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate tc.valIndex bounds before indexing validators.
Line 702 only checks non-empty validators, but cases use index 1; this can panic on a single-validator setup.
Suggested fix
validators := s.Network().GetValidators()
require.NotZero(s.T(), len(validators))
+ require.Greater(s.T(), len(validators), tc.valIndex)
validator := validators[tc.valIndex]🤖 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 `@tests/integration/poa_test.go` around lines 701 - 705, The code checks that
the validators slice is non-empty but does not validate that tc.valIndex is
within the valid range of the validators slice before indexing into it on the
line where validator is assigned. Add a bounds check using require.Less or
require.True to ensure that tc.valIndex is less than len(validators) after
confirming validators is non-empty, preventing potential panics when test cases
use indices like 1 on single-validator setups.
Comment on lines
+763
to
+770
| _, err = s.Network().GetStakingClient().Validator( | ||
| s.Network().GetContext(), | ||
| &stakingtypes.QueryValidatorRequest{ | ||
| ValidatorAddr: nonValAddr.String(), | ||
| }, | ||
| ) | ||
| require.Contains(s.T(), err.Error(), fmt.Sprintf("validator %s not found", nonValAddr.String())) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard err before checking the not-found message in non-validator precheck.
Line 769 dereferences err without asserting it exists, which can panic and hide the real failure.
Suggested fix
_, err = s.Network().GetStakingClient().Validator(
s.Network().GetContext(),
&stakingtypes.QueryValidatorRequest{
ValidatorAddr: nonValAddr.String(),
},
)
+ require.Error(s.T(), err)
require.Contains(s.T(), err.Error(), fmt.Sprintf("validator %s not found", nonValAddr.String()))📝 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.
Suggested change
| _, err = s.Network().GetStakingClient().Validator( | |
| s.Network().GetContext(), | |
| &stakingtypes.QueryValidatorRequest{ | |
| ValidatorAddr: nonValAddr.String(), | |
| }, | |
| ) | |
| require.Contains(s.T(), err.Error(), fmt.Sprintf("validator %s not found", nonValAddr.String())) | |
| _, err = s.Network().GetStakingClient().Validator( | |
| s.Network().GetContext(), | |
| &stakingtypes.QueryValidatorRequest{ | |
| ValidatorAddr: nonValAddr.String(), | |
| }, | |
| ) | |
| require.Error(s.T(), err) | |
| require.Contains(s.T(), err.Error(), fmt.Sprintf("validator %s not found", nonValAddr.String())) |
🤖 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 `@tests/integration/poa_test.go` around lines 763 - 770, The test is
dereferencing err without first asserting it is not nil, which can cause a panic
and hide the actual test failure. Before calling err.Error() in the
require.Contains assertion on the line after the Validator call, add a guard
using require.Error(s.T(), err) to ensure the error is not nil and the Validator
call actually failed as expected.
…n' of github.com:aluque-peersyst/xrplevm-node into feat/add-validator-self-remove-message
…unbonding refactor
AdriaCarrera
approved these changes
Jun 23, 2026
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
2 participants
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.
Add message for validators to remove themselves from the validator set
Summary by CodeRabbit
self_remove_validatorevent on successful self-removal.