fix: Store authority in AttesterInfo and check for authz; proto-gen#362
Open
fix: Store authority in AttesterInfo and check for authz; proto-gen#362
authority in AttesterInfo and check for authz; proto-gen#362Conversation
Contributor
📝 WalkthroughWalkthroughSwitches attester identity from Validator to Authority, introduces authority-based authorization checks and early vote-length validation in Attest, updates proto and tests to reflect the field rename, and applies broad dependency version updates in go.mod. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
* main: fix(network): add vote payload validation in Attest handler (#360)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/network/keeper/msg_server.go (1)
205-233:⚠️ Potential issue | 🟠 Major
LeaveAttesterSetdoes not delete theAttesterInforecord.
RemoveAttesterSetMemberremoves the entry fromAttesterSetbut leaves the correspondingAttesterInforecord in the store. A staleAttesterInfoentry poses no immediate exploit (theGetValidatorIndexcheck inAttestacts as a secondary gate), but it leaks storage and could confuse future queries or tooling that iteratesAttesterInfo.Consider adding a
DeleteAttesterInfo(ctx, msg.ConsensusAddress)(or equivalent collectionRemove) call alongsideRemoveAttesterSetMember.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server.go` around lines 205 - 233, The LeaveAttesterSet handler currently removes the member from the AttesterSet but leaves the AttesterInfo record; update LeaveAttesterSet to also delete the AttesterInfo entry for msg.ConsensusAddress (e.g., call a keeper method like DeleteAttesterInfo or RemoveAttesterInfo after RemoveAttesterSetMember) and handle and wrap any error from that deletion (similar to how RemoveAttesterSetMember errors are wrapped) before emitting the event so the store won't retain stale AttesterInfo records.
🧹 Nitpick comments (2)
go.mod (1)
1-3:go 1.25.6is valid but one minor behind — consider bumping togo 1.26.0.Go 1.26 arrived in February 2026 and is now the latest stable release.
go1.25.6was released on 2026-01-15 and is a valid version, so there is no correctness issue, but upgrading thegodirective keeps you on an actively maintained release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 1 - 3, Update the Go version in the module directive: change the existing "go 1.25.6" directive in go.mod to "go 1.26.0" so the module builds against the latest stable Go release; ensure you run "go mod tidy" or a quick build to verify no toolchain breakages after updating the "go" directive.modules/network/keeper/msg_server_test.go (1)
159-197:TestAttestVotePayloadValidationstill uses inline setup — minor inconsistency withnewTestServer.All other test functions now use
newTestServer, butTestAttestVotePayloadValidationstill hand-rolls the same boilerplate. This is low priority but worth aligning for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server_test.go` around lines 159 - 197, Replace the inline test setup in TestAttestVotePayloadValidation with a call to the shared helper newTestServer to keep consistency: remove the manual NewMockStakingKeeper / MakeTestEncodingConfig / NewKeeper / msgServer / CreateMultiStore / ctx construction and instead call newTestServer(...) to obtain the msgServer (or server), keeper and sdk.Context (and any test addresses), then reuse those in the rest of the test when building params, attester state, and calling server.Attest; ensure the same preconditions (SetParams, SetAttesterSetMember, SetAttesterInfo, BuildValidatorIndexMap) are applied to the returned keeper/context so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/network/keeper/msg_server_test.go`:
- Around line 310-322: Update the "not_in_set" case in TestAttest to expect the
authorization error instead of collections.ErrNotFound: change the expErr from
collections.ErrNotFound to sdkerrors.ErrUnauthorized in the test case for
types.MsgAttest (the same adjustment done for LeaveAttesterSet), so the test
asserts the intended "not in attester set" unauthorized path when
Keeper/production sentinel is fixed.
- Around line 225-237: The test case "not_in_set" in msg_server_test.go
documents the current bug instead of asserting intended behavior; after fixing
production code to use collections.ErrNotFound in Keeper logic, update the
"not_in_set" test (the table entry named "not_in_set" that constructs a
MsgLeaveAttesterSet with Authority and ConsensusAddress set to
ownerAddr.String()) to expect sdkerrors.ErrInvalidRequest instead of
collections.ErrNotFound, remove or update the NOTE comment, and change expErr to
sdkerrors.ErrInvalidRequest so the test matches the fixed error path.
In `@modules/network/keeper/msg_server.go`:
- Around line 46-55: The branch checking for sdkerrors.ErrNotFound after calling
k.AttesterInfo.Get is dead because collections.Map.Get returns
collections.ErrNotFound, not sdkerrors.ErrNotFound; update the error check to
compare against collections.ErrNotFound (e.g., use errors.Is(err,
collections.ErrNotFound)) and keep the existing
sdkerr.Wrapf(sdkerrors.ErrUnauthorized, ...) return for the "consensus address
not in attester set" case; ensure the import for collections.ErrNotFound is
added if missing and leave the fallback sdkerr.Wrapf(err, "attester set") for
other errors.
- Around line 209-218: In LeaveAttesterSet, fix the unreachable "not-in-set"
branch by checking for collections.ErrNotFound (the sentinel returned by
k.AttesterInfo.Get) instead of sdkerrors.ErrNotFound; update the error check in
the block that calls k.AttesterInfo.Get to use errors.Is(err,
collections.ErrNotFound) and keep the existing
sdkerr.Wrapf(sdkerrors.ErrInvalidRequest, ...) return for that case so callers
receive the intended ErrInvalidRequest message when the consensus address is
missing; ensure the change references k.AttesterInfo.Get and the
LeaveAttesterSet handler.
---
Outside diff comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 205-233: The LeaveAttesterSet handler currently removes the member
from the AttesterSet but leaves the AttesterInfo record; update LeaveAttesterSet
to also delete the AttesterInfo entry for msg.ConsensusAddress (e.g., call a
keeper method like DeleteAttesterInfo or RemoveAttesterInfo after
RemoveAttesterSetMember) and handle and wrap any error from that deletion
(similar to how RemoveAttesterSetMember errors are wrapped) before emitting the
event so the store won't retain stale AttesterInfo records.
---
Nitpick comments:
In `@go.mod`:
- Around line 1-3: Update the Go version in the module directive: change the
existing "go 1.25.6" directive in go.mod to "go 1.26.0" so the module builds
against the latest stable Go release; ensure you run "go mod tidy" or a quick
build to verify no toolchain breakages after updating the "go" directive.
In `@modules/network/keeper/msg_server_test.go`:
- Around line 159-197: Replace the inline test setup in
TestAttestVotePayloadValidation with a call to the shared helper newTestServer
to keep consistency: remove the manual NewMockStakingKeeper /
MakeTestEncodingConfig / NewKeeper / msgServer / CreateMultiStore / ctx
construction and instead call newTestServer(...) to obtain the msgServer (or
server), keeper and sdk.Context (and any test addresses), then reuse those in
the rest of the test when building params, attester state, and calling
server.Attest; ensure the same preconditions (SetParams, SetAttesterSetMember,
SetAttesterInfo, BuildValidatorIndexMap) are applied to the returned
keeper/context so behavior is unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.summodules/network/module/v1/module.pb.gois excluded by!**/*.pb.gomodules/network/types/attester.pb.gois excluded by!**/*.pb.gomodules/network/types/query.pb.gw.gois excluded by!**/*.pb.gw.gomodules/network/types/tx.pb.gois excluded by!**/*.pb.gomodules/proto/buf.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
go.modmodules/network/keeper/msg_server.gomodules/network/keeper/msg_server_test.gomodules/proto/evabci/network/v1/attester.proto
tac0turtle
reviewed
Feb 25, 2026
tac0turtle
reviewed
Feb 25, 2026
randygrok
reviewed
Feb 25, 2026
randygrok
reviewed
Feb 25, 2026
randygrok
reviewed
Feb 25, 2026
Contributor
randygrok
left a comment
randygrok
left a comment
There was a problem hiding this comment.
some comments left, important to fix
randygrok
reviewed
Feb 25, 2026
randygrok
reviewed
Feb 25, 2026
Contributor
Author
|
Good feedback! Thank you |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 223-235: The current assertValidValidatorAuthority relies solely
on AttesterInfo.Get and can authorize addresses that were removed from
AttesterSet but left in AttesterInfo; update assertValidValidatorAuthority to
also check membership via k.AttesterSet.Has(ctx, consensusAddress) and return an
unauthorized error if Has is false, or alternatively modify
RemoveAttesterSetMember to also delete the corresponding k.AttesterInfo entry so
AttesterInfo.Get cannot be used alone for auth; locate the function
assertValidValidatorAuthority and the methods AttesterInfo.Get, AttesterSet.Has,
and RemoveAttesterSetMember to implement the chosen fix.
Contributor
Author
|
@oderabbitai review again |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/network/keeper/msg_server.go (1)
233-235: Consider a more descriptive error message for authority mismatch.The current message
"address %s"doesn't clearly indicate that it's an authority mismatch. A more explicit message would help with debugging and auditing.💡 Suggested improvement
if v.Authority != authority { - return sdkerr.Wrapf(sdkerrors.ErrUnauthorized, "address %s", authority) + return sdkerr.Wrapf(sdkerrors.ErrUnauthorized, "authority mismatch for consensus address %s: expected %s, got %s", consensusAddress, v.Authority, authority) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/network/keeper/msg_server.go` around lines 233 - 235, The unauthorized error for the authority check is too vague; update the return in the authority mismatch branch (the if comparing v.Authority and authority in the msg server handler) to return a descriptive wrapped sdkerrors.ErrUnauthorized that includes both the expected authority (v.Authority) and the caller/provided authority (authority) so the message clearly states "authority mismatch" or "unauthorized: expected <expected>, got <actual>" to aid debugging and auditing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modules/network/keeper/msg_server.go`:
- Around line 233-235: The unauthorized error for the authority check is too
vague; update the return in the authority mismatch branch (the if comparing
v.Authority and authority in the msg server handler) to return a descriptive
wrapped sdkerrors.ErrUnauthorized that includes both the expected authority
(v.Authority) and the caller/provided authority (authority) so the message
clearly states "authority mismatch" or "unauthorized: expected <expected>, got
<actual>" to aid debugging and auditing.
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
3 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.
Overview
Replaces #358
Summary by CodeRabbit
Chores
Refactor
API
Tests