Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (3)
modules/symstaking/relay/client.go (1)
48-55: Disallow ambiguous relay client configuration.Line 53 silently prioritizes
KeyFileoverRPCAddress. If both are set, non-test nodes can accidentally run in mock mode.Proposed change
func NewClient(cfg Config) (*Client, error) { if cfg.RPCAddress == "" && cfg.KeyFile == "" { return nil, fmt.Errorf("either RPCAddress or KeyFile must be provided") } + if cfg.RPCAddress != "" && cfg.KeyFile != "" { + return nil, fmt.Errorf("RPCAddress and KeyFile are mutually exclusive") + } // If KeyFile is provided, use mock client if cfg.KeyFile != "" { return newMockClient(cfg.KeyFile) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/symstaking/relay/client.go` around lines 48 - 55, The current logic in the relay client constructor silently prioritizes cfg.KeyFile over cfg.RPCAddress (calling newMockClient when KeyFile is non-empty), which allows ambiguous configuration; change the validation so that if both cfg.KeyFile and cfg.RPCAddress are set the constructor returns an error (e.g., "cannot specify both KeyFile and RPCAddress"), otherwise keep the existing behavior: require at least one of cfg.RPCAddress or cfg.KeyFile, call newMockClient when only cfg.KeyFile is set, and instantiate the real RPC client when only cfg.RPCAddress is set; update the conditional checks around cfg.RPCAddress, cfg.KeyFile, and newMockClient accordingly.modules/symslashing/keeper/keeper.go (1)
48-51: Prefer returning constructor errors instead of panicking.Using
panichere makes startup failures harder to control and diverges from the sibling symstaking keeper constructor behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/symslashing/keeper/keeper.go` around lines 48 - 51, Replace the panic on schema build failure with error propagation: in the keeper constructor (the function that calls sb.Build()), replace the panic(err) after schema, err := sb.Build() with returning the error to the caller (e.g., return nil, err or the constructor's error signature), mirroring the symstaking keeper's constructor pattern; ensure sb.Build() failures are handled by the caller rather than terminating the process so the constructor returns a non-nil error when schema building fails.modules/symstaking/types/genesis.go (1)
46-49: RemoveLastValidatorSetstruct or replace[]anywith a concrete type.The
LastValidatorSetstruct at lines 46–49 defines a[]anyfield with protobuf tags, which is incompatible for serialization. This struct is not currently used inGenesisStateor elsewhere in the codebase. Either remove it if unnecessary, or if it's intended for future use, replace theUpdatesfield with a concrete, versioned type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/symstaking/types/genesis.go` around lines 46 - 49, The LastValidatorSet struct defines Updates as []any which breaks protobuf serialization; either delete the LastValidatorSet type if it's unused, or change its Updates field to a concrete, versioned type (e.g., []ValidatorUpdate or another existing proto-backed struct) and update the protobuf tags accordingly; locate the type named LastValidatorSet and its Updates field and either remove the entire type declaration or replace []any with the concrete struct type and regenerate any proto/codec bindings.
🤖 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/hooks.go`:
- Line 29: Guard against nil or empty consensus pubkeys by validating consPubKey
before calling consPubKey.Bytes(): add a small helper (e.g.,
validateAndEncodeConsPubKey or encodeConsPubKeyIfValid) that checks consPubKey
!= nil and len(consPubKey.Bytes()) > 0, returns (string, error) or empty+bool,
and use that helper in the three hooks that currently call consPubKey.Bytes()
directly (the places that set pubKeyHex). If the key is invalid, skip
creating/removing an attester key and handle/log the error/skip path
consistently in those hooks to avoid panics and accidental empty-key operations.
- Around line 50-53: The hook currently calls
h.keeper.BuildValidatorIndexMap(sdkCtx) for every attester add/remove which
clears and repopulates indexes each time (see GetAllAttesters and
BuildValidatorIndexMap in keeper.go), causing quadratic work; change the hook in
modules/network/keeper/hooks.go to either (A) accumulate attester add/remove
events and call h.keeper.BuildValidatorIndexMap(sdkCtx) a single time after the
batch, or (B) replace the per-event full rebuild with targeted updates by
calling or adding keeper methods that update only the affected entries (e.g.,
AddValidatorIndex/RemoveValidatorIndex or UpdateValidatorIndex) instead of full
rebuilds; apply the same fix to the other occurrence around lines 91-94.
- Around line 37-40: The hook currently silently returns nil when len(attesters)
>= MaxAttesters, which allows validator creation to proceed while skipping
registration; change this to return an explicit error instead of nil so upstream
validator creation fails when the attester cap is reached (or alternatively
enforce the cap before this hook is invoked). Specifically, update the branch
that references MaxAttesters, attesters, pubKeyHex and h.keeper.Logger(sdkCtx)
to log the condition and then return a descriptive error (using the module's
standard error constructor) rather than nil so the caller observes failure and
the modules cannot diverge.
In `@modules/symslashing/keeper/keeper.go`:
- Around line 121-129: The code currently logs failures from
json.Marshal(record) and k.InfractionRecords.Set(sdkCtx, pubKeyHex, recordBz)
but still returns overall success; change the logic in the function that calls
recordBz to return an error when marshal or storage fails instead of only
logging. Specifically, in the block around recordBz/json.Marshal and
k.InfractionRecords.Set, propagate the error returned by json.Marshal or by
k.InfractionRecords.Set up to the caller (do not swallow it), include the
request ID from the incoming ctx when constructing/annotating the returned error
so the caller can preserve request tracing, and ensure the function no longer
returns a success result on these failures. Use the existing symbols
(json.Marshal, recordBz, k.InfractionRecords.Set, sdk.UnwrapSDKContext,
k.Logger) to locate and update the code path.
In `@modules/symslashing/types/keys.go`:
- Line 29: ModuleCdc is declared but never initialized, which can cause nil
panics; initialize ModuleCdc in modules/symslashing/types/codec.go to a non-nil
codec instance (e.g., a proto codec with an interface registry) and perform the
same type/interface registrations you do for AminoCdc so all module types are
registered with ModuleCdc as well; update the codec.go init block or a dedicated
RegisterCodec/MakeCodec function to set ModuleCdc and register the module's
concrete types and interfaces (refer to symbols ModuleCdc, AminoCdc, and the
codec registration code in types/codec.go).
In `@modules/symslashing/types/params.go`:
- Around line 61-63: Implement proper validation in Params.Validate to reject
invalid slashing parameters: check that signed and tombstone-block windows
(fields on Params) are positive integers, MaxEvidenceAge and SlashWindow (or
equivalent window durations) are > 0, the DowntimeJailDuration (or JailDuration)
is non-negative, and that decimal/fraction fields (e.g.,
SlashFractionDoubleSign, SlashFractionDowntime, or similar) parse correctly as
sdk.Dec and lie within [0,1]. Return descriptive errors for each failing field
(use existing error helpers or fmt.Errorf) instead of nil; locate the checks
inside Params.Validate so genesis and governance boundaries fail fast.
In `@modules/symstaking/depinject.go`:
- Around line 30-36: ProvideModule currently returns a Keeper with k.hooks left
nil so module callbacks never run; update ProvideModule to assign the injected
hooks into the Keeper before returning (set k.hooks = <injected hooks> or wrap
the provided Hook implementation into the keeper's hooks field) and avoid
returning a nil wrapper. Specifically, inside ProvideModule (and any related
provision helpers) locate the Keeper construction and set its hooks field
(k.hooks) to the injected hooks implementation used by
modules/symstaking/keeper/abci.go (and referenced by
modules/network/keeper/hooks.go) so callbacks are wired during module
provisioning.
In `@modules/symstaking/genesis.go`:
- Around line 13-17: InitGenesis currently persists state immediately; first
call genState.Validate() at the start of InitGenesis and return a wrapped error
if it fails (e.g., return fmt.Errorf("validate genesis: %w", err)) before
invoking keeper methods like k.SetParams(ctx, genState.Params) so invalid
genesis is not written; update the InitGenesis function to perform this
validation step and short-circuit on error.
In `@modules/symstaking/keeper/abci.go`:
- Around line 126-135: The pubKeyToHex helper (and the other key conversion
helpers around lines 138-152) must stop silently returning empty strings for
unsupported key types or invalid encodings; change their signatures to return
(string, error), detect and return an explicit error for unsupported PublicKey
variants and any hex/encoding issues, and update callers (e.g., where validator
updates are built) to propagate/handle these errors so malformed validator
updates fail fast rather than being created with empty/invalid keys; reference
the pubKeyToHex function and the sibling conversion helpers when making these
changes.
- Around line 36-39: Move the call that persists the new epoch (SetCurrentEpoch)
so it only runs after the validator synchronization completes successfully: do
not call k.SetCurrentEpoch(ctx, currentEpoch) before the relay/fetch-and-sync
logic (the code that performs validator updates); instead, perform the relay
fetch and validator sync first and only call k.SetCurrentEpoch(...) after that
succeeds. Update both places where the epoch is currently advanced (the block
around the shown SetCurrentEpoch and the other analogous block at lines 42-46)
so failures in the relay/fetch/sync path return an error without updating
CurrentEpoch.
In `@modules/symstaking/keeper/keeper.go`:
- Around line 155-163: The iterator error paths in the loop around
k.LastValidatorSet.Iterate and iter.KeyValue() currently return silently; change
these to propagate or log the error instead of swallowing it — specifically,
when Iterate(ctx, nil) returns an error, return that error (or call
ctx.Logger()/k.Logger() to log with context) rather than plain return, and do
the same for iter.KeyValue() failures inside the loop; ensure the surrounding
function signature supports returning an error or that you emit a clear error
log including the iterator error and context (e.g., validator set key) so
reconciliation does not stop silently.
- Around line 189-194: Before building/signing the slash payload, validate
inputs: ensure height is non-negative and fits into 8 bytes (e.g. >=0 and <=
math.MaxUint64) and check validatorPubKey length equals 32 bytes; also validate
infractionType is within expected enum/range if applicable. Add these guards in
the same scope where message is constructed (around the code that uses
infractionType, encodeHeight(height) and validatorPubKey) and return/propagate
an error when checks fail instead of proceeding to append and sign the malformed
message.
In `@modules/symstaking/module.go`:
- Around line 98-107: The EndBlock implementation currently discards validator
updates and uses the wrong signature; change AppModule.EndBlock to return
([]abci.ValidatorUpdate, error) instead of only error, call
am.keeper.EndBlock(sdk.UnwrapSDKContext(ctx)) to get (updates, err), return
nil,err on error and return the updates and nil on success so the computed
validator set from keeper.EndBlock is propagated to the ABCI layer; ensure
imports/reference to abci.ValidatorUpdate are present and mirror the pattern
used in modules/staking and modules/migrationmngr.
In `@modules/symstaking/relay/client.go`:
- Around line 169-171: In SignMessage when c.mockMode is true you slice
message[:8] which panics for short payloads; fix by guarding the slice length
before formatting the mock ID: compute n := 8 and set n = len(message) if
len(message) < 8 (handle len==0 by using a fixed token like "empty" or zero
bytes), then call fmt.Sprintf("mock-request-%x", message[:n]) so the mock path
never slices past the message bounds; update the branch that checks c.mockMode
and the fmt.Sprintf call accordingly.
- Around line 145-153: The code currently ignores missing ed25519Key and
silently defaults malformed voting power to 1; instead fail fast by returning an
error when either condition occurs: if ed25519Key == nil return a descriptive
error indicating a missing Ed25519 key for the validator, and if
strconv.ParseInt(v.GetVotingPower(), 10, 64) returns an error return a
descriptive error that includes the offending voting power string from
v.GetVotingPower(); update the surrounding function (the caller that iterates
validators) to propagate this error rather than continuing so invalid relay
records surface immediately.
---
Nitpick comments:
In `@modules/symslashing/keeper/keeper.go`:
- Around line 48-51: Replace the panic on schema build failure with error
propagation: in the keeper constructor (the function that calls sb.Build()),
replace the panic(err) after schema, err := sb.Build() with returning the error
to the caller (e.g., return nil, err or the constructor's error signature),
mirroring the symstaking keeper's constructor pattern; ensure sb.Build()
failures are handled by the caller rather than terminating the process so the
constructor returns a non-nil error when schema building fails.
In `@modules/symstaking/relay/client.go`:
- Around line 48-55: The current logic in the relay client constructor silently
prioritizes cfg.KeyFile over cfg.RPCAddress (calling newMockClient when KeyFile
is non-empty), which allows ambiguous configuration; change the validation so
that if both cfg.KeyFile and cfg.RPCAddress are set the constructor returns an
error (e.g., "cannot specify both KeyFile and RPCAddress"), otherwise keep the
existing behavior: require at least one of cfg.RPCAddress or cfg.KeyFile, call
newMockClient when only cfg.KeyFile is set, and instantiate the real RPC client
when only cfg.RPCAddress is set; update the conditional checks around
cfg.RPCAddress, cfg.KeyFile, and newMockClient accordingly.
In `@modules/symstaking/types/genesis.go`:
- Around line 46-49: The LastValidatorSet struct defines Updates as []any which
breaks protobuf serialization; either delete the LastValidatorSet type if it's
unused, or change its Updates field to a concrete, versioned type (e.g.,
[]ValidatorUpdate or another existing proto-backed struct) and update the
protobuf tags accordingly; locate the type named LastValidatorSet and its
Updates field and either remove the entire type declaration or replace []any
with the concrete struct type and regenerate any proto/codec bindings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b925fb9e-9119-41c1-9c3a-8a032b9fd27d
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.summodules/symstaking/module/v1/module.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (20)
go.modmodules/network/keeper/bitmap.gomodules/network/keeper/hooks.gomodules/symslashing/keeper/keeper.gomodules/symslashing/types/codec.gomodules/symslashing/types/errors.gomodules/symslashing/types/keys.gomodules/symslashing/types/params.gomodules/symstaking/depinject.gomodules/symstaking/genesis.gomodules/symstaking/keeper/abci.gomodules/symstaking/keeper/keeper.gomodules/symstaking/module.gomodules/symstaking/relay/client.gomodules/symstaking/types/codec.gomodules/symstaking/types/errors.gomodules/symstaking/types/expected_keepers.gomodules/symstaking/types/genesis.gomodules/symstaking/types/keys.gomodules/symstaking/types/params.go
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
|
|
||
| // Get the hex-encoded pubkey as the attester address | ||
| pubKeyHex := hex.EncodeToString(consPubKey.Bytes()) |
Contributor
There was a problem hiding this comment.
Guard against nil or empty consensus pubkeys.
All three hooks call consPubKey.Bytes() directly. A nil value will panic, and an empty byte slice becomes "", which can create or remove an empty attester key. Validate the pubkey once before encoding it and reuse that helper in each hook.
Also applies to: 68-68, 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/network/keeper/hooks.go` at line 29, Guard against nil or empty
consensus pubkeys by validating consPubKey before calling consPubKey.Bytes():
add a small helper (e.g., validateAndEncodeConsPubKey or
encodeConsPubKeyIfValid) that checks consPubKey != nil and
len(consPubKey.Bytes()) > 0, returns (string, error) or empty+bool, and use that
helper in the three hooks that currently call consPubKey.Bytes() directly (the
places that set pubKeyHex). If the key is invalid, skip creating/removing an
attester key and handle/log the error/skip path consistently in those hooks to
avoid panics and accidental empty-key operations.
Comment on lines
+37
to
+40
| if len(attesters) >= MaxAttesters { | ||
| h.keeper.Logger(sdkCtx).Warn("attester set at maximum capacity, cannot add validator", | ||
| "pubkey", pubKeyHex, "max", MaxAttesters) | ||
| return nil // Don't fail, just log |
Contributor
There was a problem hiding this comment.
Don't turn MaxAttesters into a silent no-op.
Returning nil here lets validator creation succeed upstream while the network attester mirror is skipped, so the two modules can diverge exactly when the cap is reached. Please surface an error here, or enforce the cap before this hook runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/network/keeper/hooks.go` around lines 37 - 40, The hook currently
silently returns nil when len(attesters) >= MaxAttesters, which allows validator
creation to proceed while skipping registration; change this to return an
explicit error instead of nil so upstream validator creation fails when the
attester cap is reached (or alternatively enforce the cap before this hook is
invoked). Specifically, update the branch that references MaxAttesters,
attesters, pubKeyHex and h.keeper.Logger(sdkCtx) to log the condition and then
return a descriptive error (using the module's standard error constructor)
rather than nil so the caller observes failure and the modules cannot diverge.
Comment on lines
+50
to
+53
| if err := h.keeper.BuildValidatorIndexMap(sdkCtx); err != nil { | ||
| h.keeper.Logger(sdkCtx).Error("failed to rebuild validator index map", "error", err) | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
Batch index rebuilds instead of doing one per validator event.
GetAllAttesters already scans the full set, and BuildValidatorIndexMap then clears and repopulates both index collections for every attester (modules/network/keeper/keeper.go:169-186, 193-232). Running that on each add/remove makes relay sync quadratic and risks the same stalling MaxAttesters is meant to avoid. Rebuild once after the batch, or update only the affected entries here.
Also applies to: 91-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/network/keeper/hooks.go` around lines 50 - 53, The hook currently
calls h.keeper.BuildValidatorIndexMap(sdkCtx) for every attester add/remove
which clears and repopulates indexes each time (see GetAllAttesters and
BuildValidatorIndexMap in keeper.go), causing quadratic work; change the hook in
modules/network/keeper/hooks.go to either (A) accumulate attester add/remove
events and call h.keeper.BuildValidatorIndexMap(sdkCtx) a single time after the
batch, or (B) replace the per-event full rebuild with targeted updates by
calling or adding keeper methods that update only the affected entries (e.g.,
AddValidatorIndex/RemoveValidatorIndex or UpdateValidatorIndex) instead of full
rebuilds; apply the same fix to the other occurrence around lines 91-94.
Comment on lines
+121
to
+129
| recordBz, err := json.Marshal(record) | ||
| if err != nil { | ||
| k.Logger(sdk.UnwrapSDKContext(ctx)).Error("failed to marshal infraction record", "error", err) | ||
| } else { | ||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
| if err := k.InfractionRecords.Set(sdkCtx, pubKeyHex, recordBz); err != nil { | ||
| k.Logger(sdkCtx).Error("failed to store infraction record", "error", err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Do not return full success when infraction record persistence fails.
If marshal/store fails, the method currently logs and still returns success at Line 139. That hides a partial failure and loses audit trail guarantees.
🧾 Return partial-failure error while preserving request ID
recordBz, err := json.Marshal(record)
if err != nil {
- k.Logger(sdk.UnwrapSDKContext(ctx)).Error("failed to marshal infraction record", "error", err)
- } else {
- sdkCtx := sdk.UnwrapSDKContext(ctx)
- if err := k.InfractionRecords.Set(sdkCtx, pubKeyHex, recordBz); err != nil {
- k.Logger(sdkCtx).Error("failed to store infraction record", "error", err)
- }
- }
+ return requestID, fmt.Errorf("marshal infraction record: %w", err)
+ }
+ sdkCtx := sdk.UnwrapSDKContext(ctx)
+ if err := k.InfractionRecords.Set(sdkCtx, pubKeyHex, recordBz); err != nil {
+ return requestID, fmt.Errorf("store infraction record: %w", err)
+ }Also applies to: 139-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/symslashing/keeper/keeper.go` around lines 121 - 129, The code
currently logs failures from json.Marshal(record) and
k.InfractionRecords.Set(sdkCtx, pubKeyHex, recordBz) but still returns overall
success; change the logic in the function that calls recordBz to return an error
when marshal or storage fails instead of only logging. Specifically, in the
block around recordBz/json.Marshal and k.InfractionRecords.Set, propagate the
error returned by json.Marshal or by k.InfractionRecords.Set up to the caller
(do not swallow it), include the request ID from the incoming ctx when
constructing/annotating the returned error so the caller can preserve request
tracing, and ensure the function no longer returns a success result on these
failures. Use the existing symbols (json.Marshal, recordBz,
k.InfractionRecords.Set, sdk.UnwrapSDKContext, k.Logger) to locate and update
the code path.
modules/symslashing/types/keys.go
Outdated
| ) | ||
|
|
||
| // ModuleCdc references the global x/symslashing module codec. | ||
| var ModuleCdc codec.Codec |
Contributor
There was a problem hiding this comment.
Initialize ModuleCdc; it is currently nil.
Line 29 declares ModuleCdc without assignment. Given modules/symslashing/types/codec.go only initializes AminoCdc, any ModuleCdc usage can panic at runtime.
Proposed change
// ModuleCdc references the global x/symslashing module codec.
-var ModuleCdc codec.Codec
+var ModuleCdc codec.Codec = AminoCdc📝 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
| var ModuleCdc codec.Codec | |
| var ModuleCdc codec.Codec = AminoCdc |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/symslashing/types/keys.go` at line 29, ModuleCdc is declared but
never initialized, which can cause nil panics; initialize ModuleCdc in
modules/symslashing/types/codec.go to a non-nil codec instance (e.g., a proto
codec with an interface registry) and perform the same type/interface
registrations you do for AminoCdc so all module types are registered with
ModuleCdc as well; update the codec.go init block or a dedicated
RegisterCodec/MakeCodec function to set ModuleCdc and register the module's
concrete types and interfaces (refer to symbols ModuleCdc, AminoCdc, and the
codec registration code in types/codec.go).
Comment on lines
+155
to
+163
| iter, err := k.LastValidatorSet.Iterate(ctx, nil) | ||
| if err != nil { | ||
| return | ||
| } | ||
| defer iter.Close() | ||
| for ; iter.Valid(); iter.Next() { | ||
| kv, err := iter.KeyValue() | ||
| if err != nil { | ||
| return |
Contributor
There was a problem hiding this comment.
Iterator errors are swallowed, causing silent validator-set drift risk.
Line 157 and Line 163 return without logging or error propagation. If iteration fails, reconciliation stops silently.
🔎 Low-impact observability fix
func (k Keeper) IterateLastValidatorSet(ctx sdk.Context, cb func(pubkeyHex string, power int64) bool) {
iter, err := k.LastValidatorSet.Iterate(ctx, nil)
if err != nil {
+ k.Logger(ctx).Error("iterate last validator set failed", "error", err)
return
}
defer iter.Close()
for ; iter.Valid(); iter.Next() {
kv, err := iter.KeyValue()
if err != nil {
+ k.Logger(ctx).Error("read validator-set iterator kv failed", "error", err)
return
}
if cb(kv.Key, kv.Value) {
return
}
}
}📝 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
| iter, err := k.LastValidatorSet.Iterate(ctx, nil) | |
| if err != nil { | |
| return | |
| } | |
| defer iter.Close() | |
| for ; iter.Valid(); iter.Next() { | |
| kv, err := iter.KeyValue() | |
| if err != nil { | |
| return | |
| func (k Keeper) IterateLastValidatorSet(ctx sdk.Context, cb func(pubkeyHex string, power int64) bool) { | |
| iter, err := k.LastValidatorSet.Iterate(ctx, nil) | |
| if err != nil { | |
| k.Logger(ctx).Error("iterate last validator set failed", "error", err) | |
| return | |
| } | |
| defer iter.Close() | |
| for ; iter.Valid(); iter.Next() { | |
| kv, err := iter.KeyValue() | |
| if err != nil { | |
| k.Logger(ctx).Error("read validator-set iterator kv failed", "error", err) | |
| return | |
| } | |
| if cb(kv.Key, kv.Value) { | |
| return | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/symstaking/keeper/keeper.go` around lines 155 - 163, The iterator
error paths in the loop around k.LastValidatorSet.Iterate and iter.KeyValue()
currently return silently; change these to propagate or log the error instead of
swallowing it — specifically, when Iterate(ctx, nil) returns an error, return
that error (or call ctx.Logger()/k.Logger() to log with context) rather than
plain return, and do the same for iter.KeyValue() failures inside the loop;
ensure the surrounding function signature supports returning an error or that
you emit a clear error log including the iterator error and context (e.g.,
validator set key) so reconciliation does not stop silently.
Comment on lines
+189
to
+194
| // Format: infraction_type (1 byte) | height (8 bytes) | pubkey (32 bytes) | ||
| message := make([]byte, 0, 41) | ||
| message = append(message, byte(infractionType)) | ||
| message = append(message, encodeHeight(height)...) | ||
| message = append(message, validatorPubKey...) | ||
|
|
Contributor
There was a problem hiding this comment.
Validate slash payload inputs before signing.
The format on Line 189 assumes fixed fields, but there’s no guard for negative height or invalid pubkey length before building/signing the message.
✅ Suggested input guards
func (k Keeper) SlashWithInfractionReason(
ctx context.Context,
validatorPubKey []byte,
infractionType types.Infraction,
height int64,
) (string, error) {
+ if height < 0 {
+ return "", fmt.Errorf("invalid height %d", height)
+ }
+ if len(validatorPubKey) != 32 {
+ return "", fmt.Errorf("invalid validator pubkey length %d (expected 32)", len(validatorPubKey))
+ }
params := k.GetParams(sdk.UnwrapSDKContext(ctx))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/symstaking/keeper/keeper.go` around lines 189 - 194, Before
building/signing the slash payload, validate inputs: ensure height is
non-negative and fits into 8 bytes (e.g. >=0 and <= math.MaxUint64) and check
validatorPubKey length equals 32 bytes; also validate infractionType is within
expected enum/range if applicable. Add these guards in the same scope where
message is constructed (around the code that uses infractionType,
encodeHeight(height) and validatorPubKey) and return/propagate an error when
checks fail instead of proceeding to append and sign the malformed message.
Comment on lines
+98
to
+107
| func (am AppModule) EndBlock(ctx context.Context) error { | ||
| updates, err := am.keeper.EndBlock(sdk.UnwrapSDKContext(ctx)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Note: The validator updates need to be passed to CometBFT. | ||
| // In the standard Cosmos SDK, this is handled via the ABCI application. | ||
| // For now, we just update the internal state. | ||
| _ = updates // Validator updates will be returned via the app module manager | ||
| return nil |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this codebase has any handoff path for validator updates from symstaking.
rg -n -C3 'symstaking|ValidatorUpdate|EndBlock\(|EndBlocker|FinalizeBlock|ModuleManager|SetOrderEndBlockers'Repository: evstack/ev-abci
Length of output: 47053
🏁 Script executed:
rg -A5 "type AppModule interface|type HasEndBlocker" modules/ pkg/Repository: evstack/ev-abci
Length of output: 41
🏁 Script executed:
rg "EndBlock\(" modules/symstaking/module.go modules/staking/module.go modules/migrationmngr/module.go -A2 -B1Repository: evstack/ev-abci
Length of output: 1063
Fix EndBlock signature to return validator updates, not discard them.
The EndBlock method signature is incompatible with the Cosmos SDK module interface. It returns only error when it should return ([]abci.ValidatorUpdate, error) (as shown in modules/staking and modules/migrationmngr). This prevents the keeper's computed validator updates from reaching the ABCI layer, breaking consensus validator-set coordination.
🛠️ Correct signature and return updates
-func (am AppModule) EndBlock(ctx context.Context) error {
+func (am AppModule) EndBlock(ctx context.Context) ([]abci.ValidatorUpdate, error) {
updates, err := am.keeper.EndBlock(sdk.UnwrapSDKContext(ctx))
if err != nil {
- return err
+ return nil, err
}
- // Note: The validator updates need to be passed to CometBFT.
- // In the standard Cosmos SDK, this is handled via the ABCI application.
- // For now, we just update the internal state.
- _ = updates // Validator updates will be returned via the app module manager
- return nil
+ return updates, nil
}📝 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
| func (am AppModule) EndBlock(ctx context.Context) error { | |
| updates, err := am.keeper.EndBlock(sdk.UnwrapSDKContext(ctx)) | |
| if err != nil { | |
| return err | |
| } | |
| // Note: The validator updates need to be passed to CometBFT. | |
| // In the standard Cosmos SDK, this is handled via the ABCI application. | |
| // For now, we just update the internal state. | |
| _ = updates // Validator updates will be returned via the app module manager | |
| return nil | |
| func (am AppModule) EndBlock(ctx context.Context) ([]abci.ValidatorUpdate, error) { | |
| updates, err := am.keeper.EndBlock(sdk.UnwrapSDKContext(ctx)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return updates, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/symstaking/module.go` around lines 98 - 107, The EndBlock
implementation currently discards validator updates and uses the wrong
signature; change AppModule.EndBlock to return ([]abci.ValidatorUpdate, error)
instead of only error, call am.keeper.EndBlock(sdk.UnwrapSDKContext(ctx)) to get
(updates, err), return nil,err on error and return the updates and nil on
success so the computed validator set from keeper.EndBlock is propagated to the
ABCI layer; ensure imports/reference to abci.ValidatorUpdate are present and
mirror the pattern used in modules/staking and modules/migrationmngr.
Comment on lines
+145
to
+153
| if ed25519Key == nil { | ||
| continue | ||
| } | ||
|
|
||
| // Parse voting power from string | ||
| power, err := strconv.ParseInt(v.GetVotingPower(), 10, 64) | ||
| if err != nil { | ||
| power = 1 // Default power | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on malformed relay validator records.
Lines 145-153 currently skip missing Ed25519 keys and coerce bad voting power to 1. This can produce incorrect validator weights instead of surfacing bad relay data.
Proposed change
- for _, v := range validators {
+ for i, v := range validators {
// Find the Ed25519 key with the validator key tag (tag 43 by default)
keys := v.GetKeys()
var ed25519Key []byte
@@
- if ed25519Key == nil {
- continue
- }
+ if ed25519Key == nil {
+ return nil, fmt.Errorf("validator[%d] missing ed25519 key", i)
+ }
// Parse voting power from string
power, err := strconv.ParseInt(v.GetVotingPower(), 10, 64)
if err != nil {
- power = 1 // Default power
+ return nil, fmt.Errorf("validator[%d] invalid voting power %q: %w", i, v.GetVotingPower(), err)
+ }
+ if power < 0 {
+ return nil, fmt.Errorf("validator[%d] has negative voting power: %d", i, power)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/symstaking/relay/client.go` around lines 145 - 153, The code
currently ignores missing ed25519Key and silently defaults malformed voting
power to 1; instead fail fast by returning an error when either condition
occurs: if ed25519Key == nil return a descriptive error indicating a missing
Ed25519 key for the validator, and if strconv.ParseInt(v.GetVotingPower(), 10,
64) returns an error return a descriptive error that includes the offending
voting power string from v.GetVotingPower(); update the surrounding function
(the caller that iterates validators) to propagate this error rather than
continuing so invalid relay records surface immediately.
Comment on lines
+169
to
+171
| if c.mockMode { | ||
| // Return a mock request ID | ||
| return fmt.Sprintf("mock-request-%x", message[:8]), nil |
Contributor
There was a problem hiding this comment.
Prevent panic in mock SignMessage for short payloads.
Line 171 uses message[:8] without length checks; messages shorter than 8 bytes panic.
Proposed change
if c.mockMode {
// Return a mock request ID
- return fmt.Sprintf("mock-request-%x", message[:8]), nil
+ n := len(message)
+ if n > 8 {
+ n = 8
+ }
+ return fmt.Sprintf("mock-request-%x", message[:n]), nil
}📝 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
| if c.mockMode { | |
| // Return a mock request ID | |
| return fmt.Sprintf("mock-request-%x", message[:8]), nil | |
| if c.mockMode { | |
| // Return a mock request ID | |
| n := len(message) | |
| if n > 8 { | |
| n = 8 | |
| } | |
| return fmt.Sprintf("mock-request-%x", message[:n]), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/symstaking/relay/client.go` around lines 169 - 171, In SignMessage
when c.mockMode is true you slice message[:8] which panics for short payloads;
fix by guarding the slice length before formatting the mock ID: compute n := 8
and set n = len(message) if len(message) < 8 (handle len==0 by using a fixed
token like "empty" or zero bytes), then call fmt.Sprintf("mock-request-%x",
message[:n]) so the mock path never slices past the message bounds; update the
branch that checks c.mockMode and the fmt.Sprintf call accordingly.
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.
Overview
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores