feat: change AddValidator RemoveValiator to add/remove attestor
#165
Conversation
WalkthroughUpdates introduce consensus power roles for validators (attestor vs. sequencer), modify add/remove validator flows to set and check ConsPower, adjust tests to reflect the new removal authorization behavior, fix a comment typo, and add Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant MS as MsgServer
participant K as Keeper/Store
participant ABCI as ABCI Updates
rect rgb(240,248,255)
note over C,MS: Add validator flow
C->>MS: MsgAddValidator
MS->>MS: Construct Validator (default SequencerConsPower)
MS->>MS: Override ConsPower = AttestorConsPower
MS->>K: Save validator
K-->>MS: OK
MS->>ABCI: Request validator set update
ABCI-->>MS: Applied
end
rect rgb(250,240,230)
note over C,MS: Remove validator flow with role check
C->>MS: MsgRemoveValidator{valAddr}
MS->>K: Get validator by address
alt ConsPower != AttestorConsPower
MS-->>C: Error (invalid request: only attestor removable)
else ConsPower == AttestorConsPower
MS->>MS: Set ConsPower = 0
MS->>K: Remove/mark validator
K-->>MS: OK
MS->>ABCI: Request validator set update
ABCI-->>MS: Applied
MS-->>C: Success
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
x/opchild/keeper/msg_server.go (2)
211-217: Consider initializing ConsPower during construction.The current implementation creates a validator with
SequencerConsPower(line 211) then immediately overwrites it withAttestorConsPower(line 217). This two-step approach works but creates a momentary inconsistency.Consider one of these alternatives:
Option 1: Set ConsPower directly after construction
validator, err := types.NewValidator(valAddr, pk, req.Moniker) if err != nil { return nil, err } - -// attestor consensus power is always 3 -validator.ConsPower = types.AttestorConsPower +validator.ConsPower = types.AttestorConsPower // AddValidator always creates attestorsOption 2: Add a constructor variant
Add aNewAttestorValidatorhelper intypes/validator.go:func NewAttestorValidator(operator sdk.ValAddress, pubKey cryptotypes.PubKey, moniker string) (Validator, error) { v, err := NewValidator(operator, pubKey, moniker) if err != nil { return Validator{}, err } v.ConsPower = AttestorConsPower return v, nil }
216-217: Consider adding documentation for the attestor/sequencer distinction.The code introduces a critical architectural distinction between attestors (power 3, removable) and sequencers (power 1, permanent), but this is not documented. This could lead to confusion about:
- When to use
AddValidator(creates attestors) vs directSetValidator(creates sequencers)- Why some validators can't be removed
- The purpose of the two power levels
Consider adding:
1. Package-level documentation in
types/validator.go:// Validator Roles: // // Sequencers (ConsPower=1): Core validators that cannot be removed via RemoveValidator. // These are typically bootstrap validators or system-critical validators. // // Attestors (ConsPower=3): Dynamically managed validators that can be added and removed // via AddValidator/RemoveValidator messages. These provide additional validation capacity.2. Function-level documentation:
// AddValidator creates a new attestor validator with ConsPower=AttestorConsPower. // Attestors can later be removed via RemoveValidator. func (ms MsgServer) AddValidator(ctx context.Context, req *types.MsgAddValidator) (*types.MsgAddValidatorResponse, error) { // RemoveValidator removes an attestor validator. Only validators with ConsPower=AttestorConsPower // can be removed. Sequencers (ConsPower=SequencerConsPower) cannot be removed via this method. func (ms MsgServer) RemoveValidator(ctx context.Context, req *types.MsgRemoveValidator) (*types.MsgRemoveValidatorResponse, error) {Also applies to: 255-257
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
.gitignore(1 hunks)x/opchild/keeper/msg_server.go(2 hunks)x/opchild/keeper/msg_server_test.go(3 hunks)x/opchild/types/errors.go(1 hunks)x/opchild/types/validator.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: golangci-lint
- GitHub Check: Run test and upload codecov
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
.gitignore (1)
56-56: LGTM!Adding
.gocache/to.gitignoreis appropriate for excluding Go build cache artifacts.x/opchild/types/validator.go (2)
29-33: LGTM! Clear role distinction introduced.The new constants
AttestorConsPowerandSequencerConsPowerestablish a clear distinction between validator roles based on consensus power. The documentation is helpful.
51-51: LGTM! Constructor updated to use constant.Replacing the hardcoded
1withSequencerConsPowerimproves maintainability and makes the default role explicit.x/opchild/types/errors.go (1)
37-37: LGTM! Typo fixed.Correcting "Antehandler" to "AnteHandler" aligns with standard Cosmos SDK terminology.
x/opchild/keeper/msg_server.go (1)
255-257: Authorization check correctly enforces attestor-only removal.The logic correctly restricts removal to validators with
AttestorConsPower, preventing accidental removal of sequencers. The error message clearly communicates the restriction.However, consider enhancing the error message to be more explicit:
-} else if val.ConsPower != types.AttestorConsPower { - return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "only attestor can be removed") +} else if val.ConsPower != types.AttestorConsPower { + return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "only attestor can be removed; validator has ConsPower=%d, expected %d", val.ConsPower, types.AttestorConsPower) }This would help developers debug issues when removal fails unexpectedly.
x/opchild/keeper/msg_server_test.go (2)
71-110: LGTM! Test correctly validates attestor add/remove flow.The test updates properly verify the new validator role model:
- Initial state: Sequencer
val1created via directSetValidator(power 1)- Add attestor:
addMsgcreates attestorval2viaAddValidator(power 3) → 2 validators- Remove attestor:
removeMsgremovesval2→ 1 validator remains (val1)The assertions correctly verify:
- Line 94: 2 validators after add
- Line 109-110: 1 validator remains with Moniker "val1" after removal
226-249: LGTM! Comprehensive test coverage for removal authorization.This test correctly validates both success and failure cases:
- Lines 226-231: Verifies that attempting to remove a sequencer (ConsPower = 1) fails with an error
- Lines 233-236: Creates an attestor with explicit
ConsPower = types.AttestorConsPower- Lines 242-249: Verifies that the attestor can be successfully removed
The explicit setting of
ConsPoweron line 236 clearly demonstrates the distinction between sequencers and attestors in the test.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
==========================================
+ Coverage 51.69% 51.73% +0.03%
==========================================
Files 62 62
Lines 5186 5190 +4
==========================================
+ Hits 2681 2685 +4
Misses 1977 1977
Partials 528 528
🚀 New features to boost your workflow:
|
| OperatorAddress: operator.String(), | ||
| ConsensusPubkey: pkAny, | ||
| ConsPower: 1, | ||
| ConsPower: SequencerConsPower, |
There was a problem hiding this comment.
it might be good to implement both NewSequencer and NewAttestor
2 participants
Description
This PR is related with initia-labs/cometbft#50 PR.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit