keeper package refactor

[zale144]

Purpose of Changes and their Description

Link(s) to Ticket(s) or Issue(s) resolved by this PR

Are these changes tested and documented?

• If tested, please describe how. If not, why tests are not needed.
• If documented, please describe where. If not, describe why docs are not needed.
• Added to Unreleased section of CHANGELOG.md?

Still Left Todo

Fill this out if this is a Draft PR so others can help.

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf Linter / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	⏩ skipped	Feb 19, 2026, 10:41 AM

[greptile-apps]

Too many files changed for review. (137 files found, 100 file limit)

[cubic-dev-ai]

28 issues found across 137 files

Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="x/emissions/keeper/topic.go">

<violation number="1" location="x/emissions/keeper/topic.go:138">
P2: Missing `topicId` validation in `SetLastMedianInferences`. Every other setter in this file validates the topic ID before writing to storage, but this one only validates the Dec value. This could allow an invalid topic ID (e.g., 0) to be written.</violation>

<violation number="2" location="x/emissions/keeper/topic.go:156">
P2: Missing `topicId` validation in `SetMadInferences`, inconsistent with other setters in this file.</violation>

<violation number="3" location="x/emissions/keeper/topic.go:202">
P2: Copy-paste error in error message: says "error getting topic fee revenue" but should describe total sum of previous topic weights. This will produce misleading error messages during debugging.</violation>
</file>

<file name="x/emissions/module/stake_removals.go">

<violation number="1" location="x/emissions/module/stake_removals.go:17">
P3: Inconsistent package references for `BlockHeight` type alias across the new function types. `GetStakeRemovalsUpUntilBlockFn` and `GetDelegateStakeRemovalsUpUntilBlockFn` reference `emissionstypes.BlockHeight`, while `RemoveReputerStakeFn` and `RemoveDelegateStakeFn` reference `emissionskeeper.BlockHeight` (via `emissionskeeper.TopicId`/`emissionskeeper.ActorId`). Consider using a single source for all these type aliases consistently — preferring `emissionstypes` since that's the canonical types package.</violation>
</file>

<file name="x/emissions/keeper/reputer_loss.go">

<violation number="1" location="x/emissions/keeper/reputer_loss.go:215">
P1: Bug: `previousEmaScore` is not updated after setting the initial EMA score for a first-time reputer. The local variable still holds the default/zero value from `GetReputerScoreEma`, which causes incorrect score comparisons later when checking whether this reputer should replace the lowest-scoring active reputer. After calling `SetReputerScoreEma`, update the local variable to reflect the new score.</violation>

<violation number="2" location="x/emissions/keeper/reputer_loss.go:335">
P1: Potential nil pointer dereference: `*valueBundle.GetValueBundle()` will panic if `GetValueBundle()` returns nil. Protobuf getter methods return nil when the embedded message field is unset. Add a nil check before dereferencing.</violation>
</file>

<file name="x/emissions/keeper/params.go">

<violation number="1" location="x/emissions/keeper/params.go:54">
P0: Potential panic: `binary.BigEndian.Uint64` requires at least 8 bytes, but only `len > 0` is checked. A malformed pagination key with 1–7 bytes will cause a runtime panic (index out of range). Validate the key length is exactly 8, or return an error for invalid lengths.</violation>
</file>

<file name="x/emissions/keeper/queryserver/query_server_score.go">

<violation number="1" location="x/emissions/keeper/queryserver/query_server_score.go:181">
P1: Bug: error check must come before the `!topicExists` check. If `TopicExists` returns an error, `topicExists` will be the zero value (`false`), so the `!topicExists` branch fires first — swallowing the real error and returning a misleading "topic not found" gRPC status. Check `err != nil` first. This same issue exists in `GetTopicInitialForecasterEmaScore` and `GetTopicInitialReputerEmaScore`.</violation>
</file>

<file name="x/emissions/migrations/v3/migrate.go">

<violation number="1" location="x/emissions/migrations/v3/migrate.go:55">
P2: The `// nolint:` directive (with a leading space) is invalid and will not suppress the linter. golangci-lint requires no space between `//` and `nolint` — see the [nolint directive docs](https://golangci-lint.run/docs/linters/false-positives/). Compare with the correct format used on line 276: `//nolint:exhaustruct`.</violation>
</file>

<file name="x/emissions/keeper/nonces.go">

<violation number="1" location="x/emissions/keeper/nonces.go:246">
P1: Nil check on `nonce` is placed after `nonce.Validate()` has already been called, making it either dead code or incorrectly ordered. Move the nil check before `nonce.Validate()` to guard against nil pointer issues.</violation>

<violation number="2" location="x/emissions/keeper/nonces.go:279">
P2: `PruneWorkerNonces` returns an error when no nonces are found (`collections.ErrNotFound`), while `PruneReputerNonces` uses the helper `GetUnfulfilledReputerNonces` which gracefully returns empty nonces. Use `k.GetUnfulfilledWorkerNonces(ctx, topicId)` for consistency, treating 'nothing to prune' as a no-op rather than an error.</violation>

<violation number="3" location="x/emissions/keeper/nonces.go:423">
P1: Missing nil check on `nonce` before accessing `nonce.ReputerNonce.BlockHeight`. Other methods in this file (e.g., `IsReputerNonceUnfulfilled`) check `if n == nil { continue }` before accessing fields. This could panic if a nil entry exists in the nonces slice.</violation>
</file>

<file name="x/emissions/keeper/mint.go">

<violation number="1" location="x/emissions/keeper/mint.go:12">
P2: All methods in this file use value receivers `(k Keeper)` while the rest of the keeper methods consistently use pointer receivers `(k *Keeper)`. This causes an unnecessary copy of the large `Keeper` struct on every call and breaks consistency. Use `(k *Keeper)` to match the project convention.</violation>
</file>

<file name="x/emissions/keeper/banking.go">

<violation number="1" location="x/emissions/keeper/banking.go:55">
P2: Duplicate method: `GetBalance` (line 27) and `GetBankBalance` (line 55) have identical signatures and implementations — both delegate to `k.bankKeeper.GetBalance`. Consider removing one to avoid confusion. If both are needed (e.g., to satisfy different interfaces), add a comment clarifying why.</violation>
</file>

<file name="x/emissions/keeper/scores.go">

<violation number="1" location="x/emissions/keeper/scores.go:43">
P2: Incorrect comments on `infererScoreEmas`, `forecasterScoreEmas`, and `reputerScoreEmas`: comments say `map of (topic, block_height, worker) -> score` but the actual key type is `collections.Pair[TopicId, ActorId]`. These should say `map of (topic, worker/reputer) -> score`.</violation>

<violation number="2" location="x/emissions/keeper/scores.go:87">
P2: Misleading comment: "If the new score is older than the current score, don't update" but the function unconditionally sets the score without any age check. Either the comment is stale or the described guard logic is missing. The same issue appears on `SetReputerScoreEma`.</violation>

<violation number="3" location="x/emissions/keeper/scores.go:112">
P2: Inconsistent error handling: `GetReputerScoreEma` returns a populated default `Score` alongside a non-nil error, while the analogous `GetInfererScoreEma` and `GetForecasterScoreEma` return an empty `types.Score{}` on error. Returning a meaningful-looking value with an error is misleading and should be consistent with the other getters.</violation>

<violation number="4" location="x/emissions/keeper/scores.go:628">
P1: `GetPreviousPercentageRewardToStakedReputers` does not handle `collections.ErrNotFound`, unlike every other getter in this file. If the value has never been set, this will propagate a raw `ErrNotFound` to callers instead of returning a sensible default (e.g., `alloraMath.ZeroDec()`).</violation>
</file>

<file name="x/emissions/migrations/v4/migrate.go">

<violation number="1" location="x/emissions/migrations/v4/migrate.go:62">
P2: Adding a space between `//` and `nolint` breaks the linter directive. `golangci-lint` only recognizes `//nolint:` (no space after `//`). With `// nolint:`, this becomes a plain comment and the `exhaustruct` lint warning will fire. Note that line 231 in this same file correctly uses `//nolint:exhaustruct`.</violation>
</file>

<file name="x/emissions/keeper/regrets.go">

<violation number="1" location="x/emissions/keeper/regrets.go:43">
P2: Copy-paste error in comment: says `oneOutInferer` but the field stores `oneOutForecaster`. This is `latestOneOutForecasterInfererNetworkRegrets`, so the key description should be `(topicId, oneOutForecaster, inferer)`.</violation>

<violation number="2" location="x/emissions/keeper/regrets.go:76">
P2: Inconsistent error-path return values: `GetInfererNetworkRegret` explicitly returns `TimestampedValue{BlockHeight: 0, Value: alloraMath.ZeroDec()}` on both error paths, while all seven other getter methods return `types.TimestampedValue{}` (zero value). Consider aligning for consistency.</violation>
</file>

<file name="x/emissions/proto/emissions/v10/topic.proto">

<violation number="1" location="x/emissions/proto/emissions/v10/topic.proto:110">
P2: `TopicLabel.id` uses `uint32`, which is inconsistent with every other ID field in the emissions proto definitions (all `uint64`). This creates a type mismatch in generated Go code, requiring explicit conversions and risking silent truncation. Consider using `uint64` for consistency unless there's a strong reason to limit label IDs to 32 bits.</violation>
</file>

<file name="x/emissions/migrations/v5/migrate.go">

<violation number="1" location="x/emissions/migrations/v5/migrate.go:62">
P2: `// nolint: exhaustruct` is not a valid nolint directive. golangci-lint requires no space between `//` and `nolint` (i.e., `//nolint:exhaustruct`). With the added space, this becomes a regular comment and the linter will report `exhaustruct` violations on this line.</violation>
</file>

<file name="x/emissions/keeper/inference_synthesis/weights_test.go">

<violation number="1" location="x/emissions/keeper/inference_synthesis/weights_test.go:154">
P2: Typo in method name: `WeightstsKeeper()` should be `WeightsKeeper()`. The extra "ts" in "Weightsts" appears to be a typo that was introduced in both the helper definition (`test_utils.go:146`) and all call sites. The code compiles since it's consistent, but the name is misleading.</violation>
</file>

<file name="x/emissions/migrations/v6/migrate.go">

<violation number="1" location="x/emissions/migrations/v6/migrate.go:65">
P2: `// nolint:` (with space) is not recognized as a valid golangci-lint suppression directive. This was changed from the correct `//nolint:` format. The `exhaustruct` linter will fire on this line since the directive is now treated as a plain comment.</violation>
</file>

<file name="x/emissions/keeper/staking.go">

<violation number="1" location="x/emissions/keeper/staking.go:288">
P3: Inconsistent error wrapping: errors from `stakeRemovalsByActor.Set` are returned bare (`return err`) while all other errors in the same function are wrapped with `errorsmod.Wrap`. This makes debugging harder since the error context is lost. Apply consistent wrapping.</violation>

<violation number="2" location="x/emissions/keeper/staking.go:512">
P1: Missing `defer iter.Close()` after creating the iterator. This is a resource leak — all other iterator-using functions in this file properly close their iterators with `defer iter.Close()`. The cosmos SDK `Iterator` holds a reference to underlying store resources that must be explicitly released.</violation>
</file>

<file name="x/emissions/keeper/score_utils.go">

<violation number="1" location="x/emissions/keeper/score_utils.go:154">
P1: Bug: `SetLowestReputerScoreEma` validates `lowestScore.Address` with `ValidateBech32`, but callers can pass a zero-value `types.Score` with an empty `Address` when `GetLowestScoreFromAllReputers` finds no valid scores (all lookups error or the address list is empty). This will cause the Set call to fail unexpectedly.

Consider either: (a) having `GetLowestScoreFromAllReputers` return an error when no scores are found, letting the caller decide whether to skip the Set, or (b) skipping the Bech32 validation when the address is empty (and treating it as a sentinel/reset value). The same issue applies to the Inferer and Forecaster variants.</violation>
</file>

---

Since this is your first cubic review, here's how it works:

• cubic automatically reviews your code and comments on bugs and improvements
• Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
• Add one-off context when rerunning by tagging @cubic-dev-ai with guidance or docs links (including llms.txt)
• Ask questions if you need clarification on any suggestion

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P0: Potential panic: binary.BigEndian.Uint64 requires at least 8 bytes, but only len > 0 is checked. A malformed pagination key with 1–7 bytes will cause a runtime panic (index out of range). Validate the key length is exactly 8, or return an error for invalid lengths.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/params.go, line 54:

<comment>Potential panic: `binary.BigEndian.Uint64` requires at least 8 bytes, but only `len > 0` is checked. A malformed pagination key with 1–7 bytes will cause a runtime panic (index out of range). Validate the key length is exactly 8, or return an error for invalid lengths.</comment>

<file context>
@@ -0,0 +1,65 @@
+
+	if pagination != nil {
+		if len(pagination.Key) > 0 {
+			cursor = binary.BigEndian.Uint64(pagination.Key)
+		}
+		if pagination.Limit > 0 {
</file context>

[spooktheducks]

why is this a []byte? the sdk can ser/de uint64s for us

[cubic-dev-ai]

@spooktheducks It’s a []byte primarily because that is the standard pattern for pagination keys in the Cosmos SDK. Using bytes allows the cursor to remain an opaque "offset" that can represent different things (like IAVL keys or database IDs) without the client needing to know the underlying type.

However, since you're specifically treating this as a uint64 cursor, you're right that defining it as a uint64 in the Protobuf definition would be much cleaner. It would let the SDK handle the serialization for you and eliminate the manual byte-shuffling.

I flagged this specific logic in my previous comment because, as it's currently written, it's dangerous. binary.BigEndian.Uint64 expects a slice of exactly 8 bytes. Because the code only checks len > 0, a client sending a malformed key (like 2 bytes) will trigger a runtime panic and crash the node. If you stick with []byte, you need to strictly validate the length or use a safer way to decode the cursor.

Include these links for more context on how to handle these settings and conventions:
→ Cubic Learnings & Preferences
→ Cubic Documentation

[cubic-dev-ai]

P1: Potential nil pointer dereference: *valueBundle.GetValueBundle() will panic if GetValueBundle() returns nil. Protobuf getter methods return nil when the embedded message field is unset. Add a nil check before dereferencing.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/reputer_loss.go, line 335:

<comment>Potential nil pointer dereference: `*valueBundle.GetValueBundle()` will panic if `GetValueBundle()` returns nil. Protobuf getter methods return nil when the embedded message field is unset. Add a nil check before dereferencing.</comment>

<file context>
@@ -0,0 +1,502 @@
+	if err != nil {
+		return types.LossBundle{}, err
+	}
+	return *valueBundle.GetValueBundle(), err
+}
+
</file context>

[spooktheducks]

yeah this could imply a DDoS vector potentially

[cubic-dev-ai]

Good point—thanks!

[cubic-dev-ai]

P1: Bug: previousEmaScore is not updated after setting the initial EMA score for a first-time reputer. The local variable still holds the default/zero value from GetReputerScoreEma, which causes incorrect score comparisons later when checking whether this reputer should replace the lowest-scoring active reputer. After calling SetReputerScoreEma, update the local variable to reflect the new score.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/reputer_loss.go, line 215:

<comment>Bug: `previousEmaScore` is not updated after setting the initial EMA score for a first-time reputer. The local variable still holds the default/zero value from `GetReputerScoreEma`, which causes incorrect score comparisons later when checking whether this reputer should replace the lowest-scoring active reputer. After calling `SetReputerScoreEma`, update the local variable to reflect the new score.</comment>

<file context>
@@ -0,0 +1,502 @@
+
+	// Check if the reputer is new and set initial EMA score
+	firstSubmission := false
+	if previousEmaScore.BlockHeight == 0 {
+		firstSubmission = true
+		initialEmaScore, err := k.scoresKeeper.GetTopicInitialReputerEmaScore(ctx, topic.Id)
</file context>

[spooktheducks]

big if true

[cubic-dev-ai]

Thanks for the feedback!

[cubic-dev-ai]

P1: Bug: error check must come before the !topicExists check. If TopicExists returns an error, topicExists will be the zero value (false), so the !topicExists branch fires first — swallowing the real error and returning a misleading "topic not found" gRPC status. Check err != nil first. This same issue exists in GetTopicInitialForecasterEmaScore and GetTopicInitialReputerEmaScore.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/queryserver/query_server_score.go, line 181:

<comment>Bug: error check must come before the `!topicExists` check. If `TopicExists` returns an error, `topicExists` will be the zero value (`false`), so the `!topicExists` branch fires first — swallowing the real error and returning a misleading "topic not found" gRPC status. Check `err != nil` first. This same issue exists in `GetTopicInitialForecasterEmaScore` and `GetTopicInitialReputerEmaScore`.</comment>

<file context>
@@ -177,14 +178,14 @@ func (qs queryServer) GetListeningCoefficient(ctx context.Context, req *types.Ge
 	defer metrics.RecordMetrics("GetTopicInitialInfererEmaScore", time.Now(), &err)
 
-	topicExists, err := qs.k.TopicExists(ctx, req.TopicId)
+	topicExists, err := qs.tk.TopicExists(ctx, req.TopicId)
 	if !topicExists {
 		return nil, status.Errorf(codes.NotFound, "topic %v not found", req.TopicId)
</file context>

[spooktheducks]

yes

[cubic-dev-ai]

Thanks for confirming!

[cubic-dev-ai]

P1: Missing nil check on nonce before accessing nonce.ReputerNonce.BlockHeight. Other methods in this file (e.g., IsReputerNonceUnfulfilled) check if n == nil { continue } before accessing fields. This could panic if a nil entry exists in the nonces slice.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/nonces.go, line 423:

<comment>Missing nil check on `nonce` before accessing `nonce.ReputerNonce.BlockHeight`. Other methods in this file (e.g., `IsReputerNonceUnfulfilled`) check `if n == nil { continue }` before accessing fields. This could panic if a nil entry exists in the nonces slice.</comment>

<file context>
@@ -0,0 +1,442 @@
+	// Filter Nonces based on block_height
+	filteredNonces := make([]*types.ReputerRequestNonce, 0)
+	for _, nonce := range nonces.Nonces {
+		if nonce.ReputerNonce.BlockHeight >= blockHeightThreshold {
+			filteredNonces = append(filteredNonces, nonce)
+		}
</file context>

[cubic-dev-ai]

P2: // nolint: exhaustruct is not a valid nolint directive. golangci-lint requires no space between // and nolint (i.e., //nolint:exhaustruct). With the added space, this becomes a regular comment and the linter will report exhaustruct violations on this line.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/migrations/v5/migrate.go, line 62:

<comment>`// nolint: exhaustruct` is not a valid nolint directive. golangci-lint requires no space between `//` and `nolint` (i.e., `//nolint:exhaustruct`). With the added space, this becomes a regular comment and the linter will report `exhaustruct` violations on this line.</comment>

<file context>
@@ -58,7 +59,7 @@ func MigrateStore(ctx sdk.Context, emissionsKeeper keeper.Keeper) error {
 // the changes are the addition of InitialRegretQuantile,PNormSafeDiv
 func MigrateParams(store storetypes.KVStore, cdc codec.BinaryCodec) error {
-	oldParams := oldV4Types.Params{} //nolint: exhaustruct // empty struct used by cosmos-sdk Unmarshal below
+	oldParams := oldV4Types.Params{} // nolint: exhaustruct // empty struct used by cosmos-sdk Unmarshal below
 	oldParamsBytes := store.Get(emissionstypes.ParamsKey)
 	if oldParamsBytes == nil {
</file context>

Suggested change

	oldParams := oldV4Types.Params{} // nolint: exhaustruct // empty struct used by cosmos-sdk Unmarshal below
	oldParams := oldV4Types.Params{} //nolint:exhaustruct // empty struct used by cosmos-sdk Unmarshal below

[cubic-dev-ai]

P2: Typo in method name: WeightstsKeeper() should be WeightsKeeper(). The extra "ts" in "Weightsts" appears to be a typo that was introduced in both the helper definition (test_utils.go:146) and all call sites. The code compiles since it's consistent, but the name is misleading.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/inference_synthesis/weights_test.go, line 154:

<comment>Typo in method name: `WeightstsKeeper()` should be `WeightsKeeper()`. The extra "ts" in "Weightsts" appears to be a typo that was introduced in both the helper definition (`test_utils.go:146`) and all call sites. The code compiles since it's consistent, but the name is misleading.</comment>

<file context>
@@ -151,7 +151,7 @@ func (s *WeightsTestSuite) TestStoreLatestNormalizedWeights() {
 		// Verify stored weights
 		for worker, expectedWeight := range weights.Inferers { // nolint: maprange // reason: order not relevant
-			storedWeight, err := s.EmissionsKeeper().GetLatestInfererWeight(s.Ctx(), topicId, worker)
+			storedWeight, err := s.WeightstsKeeper().GetLatestInfererWeight(s.Ctx(), topicId, worker)
 			s.Require().NoError(err)
 			s.Require().True(expectedWeight.Equal(storedWeight))
</file context>

[cubic-dev-ai]

P2: // nolint: (with space) is not recognized as a valid golangci-lint suppression directive. This was changed from the correct //nolint: format. The exhaustruct linter will fire on this line since the directive is now treated as a plain comment.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/migrations/v6/migrate.go, line 65:

<comment>`// nolint:` (with space) is not recognized as a valid golangci-lint suppression directive. This was changed from the correct `//nolint:` format. The `exhaustruct` linter will fire on this line since the directive is now treated as a plain comment.</comment>

<file context>
@@ -61,7 +62,7 @@ func MigrateParams(store storetypes.KVStore, cdc codec.BinaryCodec) error {
 	// ADDED:
 	//      GlobalWhitelistEnabled, TopicCreatorWhitelistEnabled
-	newParams := v6Types.Params{ //nolint: exhaustruct
+	newParams := v6Types.Params{ // nolint: exhaustruct
 		Version:                             oldParams.Version,
 		MaxSerializedMsgLength:              oldParams.MaxSerializedMsgLength,
</file context>

Suggested change

	newParams := v6Types.Params{ // nolint: exhaustruct
	newParams := v6Types.Params{ //nolint:exhaustruct

[cubic-dev-ai]

P3: Inconsistent package references for BlockHeight type alias across the new function types. GetStakeRemovalsUpUntilBlockFn and GetDelegateStakeRemovalsUpUntilBlockFn reference emissionstypes.BlockHeight, while RemoveReputerStakeFn and RemoveDelegateStakeFn reference emissionskeeper.BlockHeight (via emissionskeeper.TopicId/emissionskeeper.ActorId). Consider using a single source for all these type aliases consistently — preferring emissionstypes since that's the canonical types package.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/module/stake_removals.go, line 17:

<comment>Inconsistent package references for `BlockHeight` type alias across the new function types. `GetStakeRemovalsUpUntilBlockFn` and `GetDelegateStakeRemovalsUpUntilBlockFn` reference `emissionstypes.BlockHeight`, while `RemoveReputerStakeFn` and `RemoveDelegateStakeFn` reference `emissionskeeper.BlockHeight` (via `emissionskeeper.TopicId`/`emissionskeeper.ActorId`). Consider using a single source for all these type aliases consistently — preferring `emissionstypes` since that's the canonical types package.</comment>

<file context>
@@ -12,48 +12,37 @@ import (
-}
+type GetStakeRemovalsUpUntilBlockFn func(
+	ctx context.Context,
+	blockHeight emissionstypes.BlockHeight,
+	limit uint64,
+) (ret []emissionstypes.StakeRemovalInfo, anyLeft bool, err error)
</file context>

[cubic-dev-ai]

P3: Inconsistent error wrapping: errors from stakeRemovalsByActor.Set are returned bare (return err) while all other errors in the same function are wrapped with errorsmod.Wrap. This makes debugging harder since the error context is lost. Apply consistent wrapping.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/staking.go, line 288:

<comment>Inconsistent error wrapping: errors from `stakeRemovalsByActor.Set` are returned bare (`return err`) while all other errors in the same function are wrapped with `errorsmod.Wrap`. This makes debugging harder since the error context is lost. Apply consistent wrapping.</comment>

<file context>
@@ -0,0 +1,953 @@
+	byActorKey := collections.Join3(removalInfo.Reputer, removalInfo.TopicId, removalInfo.BlockRemovalCompleted)
+	err = k.stakeRemovalsByActor.Set(ctx, byActorKey)
+	if err != nil {
+		return err
+	}
+	types.EmitNewRequestStakeRemovalEvent(ctx, removalInfo.TopicId, removalInfo.Reputer, removalInfo.Reputer, removalInfo.Amount, removalInfo.BlockRemovalCompleted)
</file context>

[spooktheducks]

yeah this could imply a DDoS vector potentially

[spooktheducks]

yes

[spooktheducks]

this function feels like it should belong in a utils subpackage

[spooktheducks]

ignored error

[spooktheducks]

agree

[spooktheducks]

this is the proper way to line-break a function signature

[spooktheducks]

there's something incongruous about this:

1. do SetTopicReputer and InsertReputer both need to be public?
2. InsertReputer and RemoveReputer are not yin-yang -- SetTopicReputer and RemoveReputer are

[spooktheducks]

big if true

[spooktheducks]

or just do var oldParams oldtypes.Params

[spooktheducks]

yes

[spooktheducks]

PR #912 — Keeper Refactor: Code Review Report (Pass 1)

Repo: allora-network/allora-chain
PR: #912 — epoch label registry types storage - part 1: keeper refactor
Author: zale144
Stats: +14,677 / -12,426 across 137 files

Review Panel

Agent	Model	Findings
Correctness	claude-opus-4	3
Security	claude-opus-4	4
Architecture	gemini-2.5-pro	1
Style	claude-sonnet-4	5
Performance	claude-opus-4	3
Adversarial	claude-opus-4	7
Total		23 raw → 13 deduplicated

Severity Summary

Severity	Count
Critical	0
High	0
⭐ Confirmed (multi-agent convergence)	3
Medium	4
Low	4
Info	2

---

⭐ Confirmed Findings (Multi-Agent Convergence)

C1: Circular Dependency Nil-Pointer Risk — No Guard on Post-Construction Patching

Severity: Medium-High | Confidence: 0.95 | Agents: 4/6 (correctness, security, architecture, adversarial)

File: x/emissions/keeper/keeper.go:98-105

The NewKeeper() constructor creates sub-keepers with nil for circular deps, then patches them after:

stk := NewStakingKeeper(cdc, sb, nil, bk)  // topicKeeper = nil
nk := NewNonceKeeper(cdc, sb, nil, pk)     // topicKeeper = nil
tk := NewTopicKeeper(cdc, sb, pk, nk, stk)
stk.topicKeeper = tk  // patched after
nk.topicKeeper = tk   // patched after

Risk: If any code path calls a method on StakingKeeper or NonceKeeper that dereferences topicKeeper during the construction window (between creation and patching), it panics. Currently safe because no methods are called during construction, but this is fragile — any future constructor change that adds an init call will cause a chain halt.

Suggestion: Add a nil guard at the top of methods that use topicKeeper, or use a lazy-init pattern (sync.Once), or restructure to avoid circular deps entirely (e.g. pass topic accessors as interfaces).

---

C2: Value Receiver on Mint Delegation Methods Copies Entire Keeper Struct

Severity: Medium | Confidence: 0.90 | Agents: 3/6 (correctness, style, performance)

File: x/emissions/keeper/mint.go:12+

The mint delegation methods use value receiver (k Keeper) instead of pointer receiver (k *Keeper). Since Keeper now contains ~12 pointer fields to sub-keepers plus collections, each call copies the full struct.

Suggestion: Change to pointer receivers: func (k *Keeper) SetRewardCurrentBlockEmission(...) etc.

---

C3: Sub-Keeper Accessors Expose Mutable Internal State

Severity: Medium | Confidence: 0.80 | Agents: 2/6 (security, adversarial)

File: x/emissions/keeper/keeper.go:275+

Getter methods like GetTopicKeeper(), GetScoresKeeper() etc. return pointers to internal sub-keepers, allowing any caller in the same package (or external packages if exported) to mutate state directly, bypassing any future authorization or validation layers.

Suggestion: Consider returning read-only interfaces instead of concrete pointers, or keep these unexported and only expose the delegating methods on Keeper.

---

Medium Findings

M1: BankingKeeper Wraps Bank Operations Without Additional Authorization

Severity: Medium | Confidence: 0.70 | Agents: 2/6 (security, adversarial)

File: x/emissions/keeper/banking.go:32+

BankingKeeper is a thin wrapper around the bank module's SendCoins, SendCoinsFromModuleToModule, etc. It adds no module-level authorization checks. Currently safe because the bank module itself enforces permissions, but the wrapper creates the illusion of a security boundary that doesn't exist.

Suggestion: Either add meaningful validation (amount > 0, known module names) or document explicitly that authorization is delegated to the underlying bank module.

M2: Misleading Comments on ScoresKeeper EMA Fields

Severity: Medium | Confidence: 0.85 | Agents: 1/6 (style)

File: x/emissions/keeper/scores.go:49+

Comments on several EMA score fields say map of (topic, block_height, worker) -> score but the actual key type is collections.Pair[TopicId, ActorId] — keyed by ActorId, not block_height. Carried over from the old keeper but now more visible in the focused sub-keeper.

Suggestion: Fix comments to match actual key types.

M3: msgServer Holds Sub-Keeper Pointers Without Parent Keeper Access

Severity: Medium | Confidence: 0.65 | Agents: 1/6 (adversarial)

File: x/emissions/keeper/msgserver/msg_server.go:10+

The msgServer struct now holds individual sub-keeper pointers rather than a single *Keeper. If future code needs access to collections still on the parent Keeper (like networkInferences, countInfererInclusions), it would need to be restructured.

Suggestion: Either move all remaining collections to sub-keepers, or keep a reference to the parent Keeper alongside the sub-keeper pointers.

M4: Post-Construction Field Assignment Breaks Encapsulation Pattern

Severity: Medium | Confidence: 0.65 | Agents: 1/6 (adversarial)

File: x/emissions/keeper/keeper.go:104

Writing to unexported fields (stk.topicKeeper = tk) from outside the sub-keeper's constructor works because everything is in the same Go package, but it breaks the encapsulation that the sub-keeper pattern is trying to establish.

Suggestion: Provide SetTopicKeeper(*TopicKeeper) methods or use a builder pattern.

---

Low Findings

L1: TODO Left in Production Constructor

File: x/emissions/keeper/keeper.go:132 — // emissionsMintKeeper: NewEmissionsMintKeeper(), TODO

L2: Duplicate Methods — GetBalance and GetBankBalance

File: x/emissions/keeper/banking.go:30 — Both do the same thing.

L3: Constructor Functions Defined Before Struct Type Declarations

File: x/emissions/keeper/actor_penalties.go:11 — Minor but inconsistent with Go convention.

L4: Orphaned Collections Remain on Parent Keeper

File: x/emissions/keeper/keeper.go:82+ — countInfererInclusionsInTopicActiveSet, networkInferences, outlierResistantNetworkInferences still live on the parent Keeper rather than in a sub-keeper.

---

Info

I1: Genesis InitGenesis Changed from Direct Collection Access to Setter Methods

File: x/emissions/keeper/genesis.go:72 — Behavioral change: setters may have additional validation not present when writing directly to collections. Worth verifying no genesis state is rejected by new validation.

I2: Performance Impact is Negligible

Sub-keeper pointer indirection and delegation are trivially inlined by the Go compiler. No measurable performance regression expected from this refactor.

---

Semgrep

1 pre-existing finding: math/rand used instead of crypto/rand in x/emissions/keeper/actor_utils/sort.go:4 (not introduced by this PR).

---

Overall Assessment

This is a well-executed structural refactor that breaks down a 4,700-line monolith into focused domain keepers. The approach is sound and will make the codebase significantly more maintainable for the epoch label registry work.

No critical issues found. The highest-confidence finding (circular dependency nil-pointer risk) is currently safe but fragile — it's the kind of thing that bites you in 6 months when someone adds an init hook. The value receiver issue on mint.go is a straightforward fix.

The refactor is mechanically clean — tests were split alongside code, and the delegation pattern preserves the existing API surface. Recommend merging after addressing C1 (nil guard), C2 (value receivers), and M2 (comment fixes).

---

Automated review by brynskbot — 6-agent panel (opus/gemini/sonnet), pass 1

[spooktheducks]

⭐ Confirmed by 3/6 agents (correctness, style, performance)

Value Receiver Copies Entire Keeper Struct

This uses (k Keeper) instead of (k *Keeper). The Keeper now has ~12 pointer fields to sub-keepers — each call copies the full struct. Should be a pointer receiver.

[spooktheducks]

Misleading Comments on EMA Fields

Several comments say map of (topic, block_height, worker) -> score but the actual key type is collections.Pair[TopicId, ActorId] — keyed by ActorId, not block_height. Should be corrected.

[zale144]

closing in favor of #919