Skip to content

feat(poa): edit validator#26172

Open
vladjdk wants to merge 6 commits intomainfrom
vlad/edit-validator
Open

feat(poa): edit validator#26172
vladjdk wants to merge 6 commits intomainfrom
vlad/edit-validator

Conversation

@vladjdk
Copy link
Copy Markdown
Member

@vladjdk vladjdk commented Mar 24, 2026

Add MsgEditValidator allowing validator operators to update their metadata and operator

@github-actions
Copy link
Copy Markdown
Contributor

@vladjdk your pull request is missing a changelog!

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 24, 2026

Greptile Summary

This PR adds MsgEditValidator, allowing a validator operator to update their moniker, description, and operator address (the last of which effectively transfers validator ownership). The proto, generated code, keeper, CLI, events, and test layers are all present. The overall structure is solid, but there is a critical gap in the message server handler that causes metadata validation to be silently skipped.

Key issues found:

  • Missing req.Validate() in EditValidator handler (msg_server.go)CreateValidator at line 92 calls req.Validate(s.keeper.authKeeper.AddressCodec()) before proceeding; EditValidator has no equivalent call. As a result ValidatorMetadata.ValidateBasic() is never reached, allowing empty monikers and overlong fields to be stored on-chain. This also means the "fails with empty moniker" test in msg_server_test.go will not actually return an error and will fail at require.Error(t, err).
  • CLI nil-safetyres.Validator.Metadata is dereferenced without a nil check; a validator stored with nil metadata (possible due to the above) would cause a panic in the CLI.
  • Misleading variable name in EditValidatorMetadataMatchExact returns the primary consensus-address key; naming it compositeKey instead of consAddr is inconsistent with GetValidatorByOperatorAddress and can confuse readers.

Confidence Score: 2/5

  • Not safe to merge — the handler skips all metadata validation, invalid data can be stored on-chain, and a unit test is guaranteed to fail.
  • The missing req.Validate() call in EditValidator is a concrete, provable bug: it bypasses all ValidatorMetadata.ValidateBasic() checks (empty moniker, length limits), and the existing unit test that exercises the empty-moniker path will fail when run. The feature itself is well-designed and the surrounding code (proto, keeper lookup, index update, CLI query-first pattern) is correct, but the validation gap must be fixed before merging.
  • enterprise/poa/x/poa/keeper/msg_server.go and enterprise/poa/x/poa/keeper/msg_server_test.go need the most attention.

Important Files Changed

Filename Overview
enterprise/poa/x/poa/keeper/msg_server.go Adds EditValidator handler, but critically omits req.Validate() — the call that every other handler (including CreateValidator) makes to trigger ValidatorMetadata.ValidateBasic(). Empty monikers and overlong fields are silently stored on-chain.
enterprise/poa/x/poa/keeper/validator.go Adds EditValidatorMetadata — correctly guards with ValidateOperatorAndConsensusPubKeyDifferent and relies on the indexed map framework to update the secondary OperatorAddress index on Set. Minor: misleading compositeKey variable name should be consAddr.
enterprise/poa/x/poa/keeper/msg_server_test.go Comprehensive test coverage for EditValidator, but the "fails with empty moniker" test will fail at runtime because req.Validate() is never called in the handler and no error is ever returned for empty monikers.
enterprise/poa/x/poa/client/cli/tx.go Implements the query-first pattern correctly (fetches current validator state before applying flag overrides), but does not nil-check res.Validator.Metadata before dereferencing it.
enterprise/poa/x/poa/types/poa.go Adds well-structured Validate(ac address.Codec) method on MsgEditValidator — validates sender, delegates to ValidatorMetadata.ValidateBasic(), and validates the operator address separately. Unfortunately this method is never invoked from the msg server.
enterprise/poa/x/poa/types/poa_test.go Thorough unit tests for MsgEditValidator.Validate() covering all edge cases correctly.
enterprise/poa/proto/cosmos/poa/v1/tx.proto Adds EditValidator RPC and MsgEditValidator/MsgEditValidatorResponse message types correctly, with appropriate signer annotation and nullable=false on the metadata field.
enterprise/poa/x/poa/types/events.go Adds EventTypeEditValidator constant, consistent with existing event type naming.

Reviews (2): Last reviewed commit: "Merge branch 'main' into vlad/edit-valid..." | Re-trigger Greptile

Comment thread enterprise/poa/x/poa/keeper/validator.go
Comment thread enterprise/poa/x/poa/client/cli/tx.go
Comment thread enterprise/poa/x/poa/keeper/msg_server.go
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 24, 2026

Codecov Report

❌ Patch coverage is 40.47619% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.41%. Comparing base (ecd58b1) to head (dd7c761).
⚠️ Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
enterprise/poa/x/poa/client/cli/tx.go 0.00% 47 Missing ⚠️
enterprise/poa/x/poa/keeper/validator.go 81.81% 2 Missing ⚠️
enterprise/poa/x/poa/keeper/msg_server.go 94.44% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #26172      +/-   ##
==========================================
+ Coverage   61.37%   61.41%   +0.03%     
==========================================
  Files         951      967      +16     
  Lines       62446    63464    +1018     
==========================================
+ Hits        38328    38977     +649     
- Misses      24118    24487     +369     
Files with missing lines Coverage Δ
enterprise/poa/x/poa/types/poa.go 98.13% <100.00%> (ø)
enterprise/poa/x/poa/keeper/msg_server.go 92.79% <94.44%> (ø)
enterprise/poa/x/poa/keeper/validator.go 85.49% <81.81%> (ø)
enterprise/poa/x/poa/client/cli/tx.go 0.00% <0.00%> (ø)

... and 12 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vladjdk
Copy link
Copy Markdown
Member Author

vladjdk commented Mar 24, 2026

@greptile re-review

@aljo242 aljo242 added the backport/v0.54.x Backport PR's to release/v0.54.x branch label Mar 24, 2026
Comment thread enterprise/poa/x/poa/keeper/msg_server.go
Comment thread enterprise/poa/x/poa/client/cli/tx.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/v0.54.x Backport PR's to release/v0.54.x branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants