Open
Conversation
|
Too many files changed for review. ( |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
26 issues found across 131 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 (unresolved 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/migrations/v4/migrate.go">
<violation number="1" location="x/emissions/migrations/v4/migrate.go:62">
P3: Use a `var` declaration for this zero-value struct that is populated via unmarshalling, which eliminates the need for the `//nolint:exhaustruct` directive.
(Based on your team's feedback about using var declarations for zero-value structs populated later.) [FEEDBACK_USED]</violation>
</file>
<file name="x/emissions/keeper/topic.go">
<violation number="1" location="x/emissions/keeper/topic.go:202">
P2: Copy-paste error in error message: says `"error getting topic fee revenue"` but this function retrieves the total sum of previous topic weights, not topic fee revenue. This will produce misleading log messages during debugging.</violation>
</file>
<file name="x/emissions/keeper/score_utils.go">
<violation number="1" location="x/emissions/keeper/score_utils.go:154">
P2: Address validation in `SetLowestReputerScoreEma` will reject the zero-value `Score` returned by `GetLowestScoreFromAllReputers` when all score lookups fail (since `Address` will be empty). The `Update*` methods chain these calls without handling this edge case. Consider either skipping the `Set` call when no valid scores were found, or having `GetLowestScoreFromAll*` return a distinct error/flag when all lookups fail.</violation>
</file>
<file name="x/emissions/keeper/queryserver/query_server.go">
<violation number="1" location="x/emissions/keeper/queryserver/query_server.go:8">
P3: Use a zero-value `var` declaration to avoid the `//nolint:exhaustruct` directive. For example:
```go
var (
_qs queryServer
_ types.QueryServiceServer = _qs
)
This creates a zero-value struct without a composite literal, so exhaustruct won't flag it.
(Based on your team's feedback about using var declarations instead of composite literals to avoid exhaustruct nolint directives.) [FEEDBACK_USED]
(Based on your team's feedback about using var declarations for zero-value structs populated later.) [FEEDBACK_USED]
(Based on your team's feedback about using var declarations to avoid exhaustruct warnings and nolint directives.) [FEEDBACK_USED]
After calling SetReputerScoreEma for the initial score, update previousEmaScore to reflect the newly stored value.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if pagination != nil { | ||
| if len(pagination.Key) > 0 { | ||
| cursor = binary.BigEndian.Uint64(pagination.Key) |
There was a problem hiding this comment.
P0: Potential panic: binary.BigEndian.Uint64 requires at least 8 bytes, but only len > 0 is checked. A pagination key with 1–7 bytes will cause an index-out-of-range panic, which could crash the node from a malformed client request.
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 pagination key with 1–7 bytes will cause an index-out-of-range panic, which could crash the node from a malformed client request.</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>
| topicId uint64, | ||
| ) (removal types.DelegateStakeRemovalInfo, found bool, err error) { | ||
| rng := NewTriplePrefixedQuadrupleRange[ActorId, ActorId, TopicId, BlockHeight](delegator, reputer, topicId) | ||
| iter, err := k.delegateStakeRemovalsByActor.Iterate(ctx, rng) |
There was a problem hiding this comment.
P1: Missing defer iter.Close() — iterator resource leak. Every other Iterate call in this file is followed by defer iter.Close(), but this one is not. This can leak resources, especially under repeated calls.
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 512:
<comment>Missing `defer iter.Close()` — iterator resource leak. Every other `Iterate` call in this file is followed by `defer iter.Close()`, but this one is not. This can leak resources, especially under repeated calls.</comment>
<file context>
@@ -0,0 +1,953 @@
+ topicId uint64,
+) (removal types.DelegateStakeRemovalInfo, found bool, err error) {
+ rng := NewTriplePrefixedQuadrupleRange[ActorId, ActorId, TopicId, BlockHeight](delegator, reputer, topicId)
+ iter, err := k.delegateStakeRemovalsByActor.Iterate(ctx, rng)
+ if err != nil {
+ return types.DelegateStakeRemovalInfo{}, false, errorsmod.Wrap(err, "error iterating over delegate stake removals by actor")
</file context>
| return k.previousPercentageRewardToStakedReputers.Set(ctx, percentageRewardToStakedReputers) | ||
| } | ||
|
|
||
| func (k *ScoresKeeper) GetPreviousPercentageRewardToStakedReputers(ctx context.Context) (alloraMath.Dec, error) { |
There was a problem hiding this comment.
P1: GetPreviousPercentageRewardToStakedReputers doesn't handle collections.ErrNotFound, unlike every other getter in this file. When the value hasn't been set yet, callers will receive an unexpected error instead of a sensible default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/scores.go, line 628:
<comment>`GetPreviousPercentageRewardToStakedReputers` doesn't handle `collections.ErrNotFound`, unlike every other getter in this file. When the value hasn't been set yet, callers will receive an unexpected error instead of a sensible default.</comment>
<file context>
@@ -0,0 +1,704 @@
+ return k.previousPercentageRewardToStakedReputers.Set(ctx, percentageRewardToStakedReputers)
+}
+
+func (k *ScoresKeeper) GetPreviousPercentageRewardToStakedReputers(ctx context.Context) (alloraMath.Dec, error) {
+ return k.previousPercentageRewardToStakedReputers.Get(ctx)
+}
</file context>
Comment on lines
+181
to
186
| topicExists, err := qs.tk.TopicExists(ctx, req.TopicId) | ||
| if !topicExists { | ||
| return nil, status.Errorf(codes.NotFound, "topic %v not found", req.TopicId) | ||
| } else if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
P1: Bug: error check is evaluated after !topicExists, so if TopicExists returns an error, topicExists defaults to false and the code returns a misleading "topic not found" instead of the actual error. Check err != nil first. This same pattern appears in all three GetTopicInitial*EmaScore functions.
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 is evaluated after `!topicExists`, so if `TopicExists` returns an error, `topicExists` defaults to `false` and the code returns a misleading "topic not found" instead of the actual error. Check `err != nil` first. This same pattern appears in all three `GetTopicInitial*EmaScore` functions.</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>
Suggested change
| topicExists, err := qs.tk.TopicExists(ctx, req.TopicId) | |
| if !topicExists { | |
| return nil, status.Errorf(codes.NotFound, "topic %v not found", req.TopicId) | |
| } else if err != nil { | |
| return nil, err | |
| } | |
| topicExists, err := qs.tk.TopicExists(ctx, req.TopicId) | |
| if err != nil { | |
| return nil, err | |
| } else if !topicExists { | |
| return nil, status.Errorf(codes.NotFound, "topic %v not found", req.TopicId) | |
| } |
| func (qs queryServer) GetCurrentLowestInfererScore(ctx context.Context, req *types.GetCurrentLowestInfererScoreRequest) (_ *types.GetCurrentLowestInfererScoreResponse, err error) { | ||
| defer metrics.RecordMetrics("GetCurrentLowestInfererScore", time.Now(), &err) | ||
| lowestInfererScore, found, err := qs.k.GetLowestInfererScoreEma(ctx, req.TopicId) | ||
| lowestInfererScore, found, err := qs.sck.GetLowestInfererScoreEma(ctx, req.TopicId) |
There was a problem hiding this comment.
P1: Bug: when !found is true (and err is nil), errorsmod.Wrap(err, ...) returns nil because err is nil at this point (already checked in the prior branch). The function returns (nil, nil) — no response and no error — which is incorrect. Use status.Errorf or errorsmod.New instead of wrapping a nil error. Same issue in GetCurrentLowestForecasterScore and GetCurrentLowestReputerScore.
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 103:
<comment>Bug: when `!found` is true (and `err` is nil), `errorsmod.Wrap(err, ...)` returns `nil` because `err` is nil at this point (already checked in the prior branch). The function returns `(nil, nil)` — no response and no error — which is incorrect. Use `status.Errorf` or `errorsmod.New` instead of wrapping a nil error. Same issue in `GetCurrentLowestForecasterScore` and `GetCurrentLowestReputerScore`.</comment>
<file context>
@@ -99,7 +100,7 @@ func (qs queryServer) GetWorkerInferenceScoresAtBlock(ctx context.Context, req *
func (qs queryServer) GetCurrentLowestInfererScore(ctx context.Context, req *types.GetCurrentLowestInfererScoreRequest) (_ *types.GetCurrentLowestInfererScoreResponse, err error) {
defer metrics.RecordMetrics("GetCurrentLowestInfererScore", time.Now(), &err)
- lowestInfererScore, found, err := qs.k.GetLowestInfererScoreEma(ctx, req.TopicId)
+ lowestInfererScore, found, err := qs.sck.GetLowestInfererScoreEma(ctx, req.TopicId)
if err != nil {
return nil, errorsmod.Wrap(err, "error getting lowest inferer score EMA")
</file context>
| if errors.Is(err, collections.ErrNotFound) { | ||
| topic, err := k.topicKeeper.GetTopic(ctx, topicId) | ||
| if err != nil { | ||
| return types.TimestampedValue{ |
There was a problem hiding this comment.
P3: Inconsistent error-path return: GetInfererNetworkRegret returns TimestampedValue{BlockHeight: 0, Value: alloraMath.ZeroDec()} on error, while every other Get method returns types.TimestampedValue{}. Consider aligning with the other methods for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/regrets.go, line 76:
<comment>Inconsistent error-path return: `GetInfererNetworkRegret` returns `TimestampedValue{BlockHeight: 0, Value: alloraMath.ZeroDec()}` on error, while every other Get method returns `types.TimestampedValue{}`. Consider aligning with the other methods for consistency.</comment>
<file context>
@@ -0,0 +1,403 @@
+ if errors.Is(err, collections.ErrNotFound) {
+ topic, err := k.topicKeeper.GetTopic(ctx, topicId)
+ if err != nil {
+ return types.TimestampedValue{
+ BlockHeight: 0,
+ Value: alloraMath.ZeroDec(),
</file context>
| @@ -58,7 +59,7 @@ func MigrateStore(ctx sdk.Context, emissionsKeeper keeper.Keeper) error { | |||
| // migrate params for this new version | |||
| // 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 | |||
There was a problem hiding this comment.
P3: Use var oldParams oldV4Types.Params instead of a composite literal with a //nolint:exhaustruct directive. Since this struct is immediately populated via proto.Unmarshal, a var declaration is cleaner and avoids the need for the nolint comment.
(Based on your team's feedback about using var declarations for zero-value structs populated later.)
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>Use `var oldParams oldV4Types.Params` instead of a composite literal with a `//nolint:exhaustruct` directive. Since this struct is immediately populated via `proto.Unmarshal`, a `var` declaration is cleaner and avoids the need for the nolint comment.
(Based on your team's feedback about using var declarations for zero-value structs populated later.) </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 | |
| var oldParams oldV4Types.Params // empty struct used by cosmos-sdk Unmarshal below |
| // 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) |
There was a problem hiding this comment.
P3: Typo in method name: WeightstsKeeper() should be WeightsKeeper(). The method has an extra "ts" in the name ("WeightstsKeeper" vs "WeightsKeeper"). Since this PR is restructuring the keeper, this is a good opportunity to fix the method name in test_utils.go and all call sites.
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 method has an extra "ts" in the name ("Weights**ts**Keeper" vs "WeightsKeeper"). Since this PR is restructuring the keeper, this is a good opportunity to fix the method name in `test_utils.go` and all call sites.</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>
| @@ -108,14 +97,30 @@ func RemoveStakes( | |||
| return nil | |||
| } | |||
|
|
|||
| type RemoveDelegateStakeFn func( | |||
| ctx context.Context, | |||
| stakeRemovalBlockHeight emissionskeeper.BlockHeight, | |||
There was a problem hiding this comment.
P3: Inconsistent package used for BlockHeight type: RemoveDelegateStakeFn uses emissionskeeper.BlockHeight while all other function types in this file use emissionstypes.BlockHeight. Use emissionstypes.BlockHeight consistently.
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 102:
<comment>Inconsistent package used for `BlockHeight` type: `RemoveDelegateStakeFn` uses `emissionskeeper.BlockHeight` while all other function types in this file use `emissionstypes.BlockHeight`. Use `emissionstypes.BlockHeight` consistently.</comment>
<file context>
@@ -108,14 +97,30 @@ func RemoveStakes(
+type RemoveDelegateStakeFn func(
+ ctx context.Context,
+ stakeRemovalBlockHeight emissionskeeper.BlockHeight,
+ topicId emissionskeeper.TopicId,
+ delegator, reputer emissionskeeper.ActorId,
</file context>
Suggested change
| stakeRemovalBlockHeight emissionskeeper.BlockHeight, | |
| stakeRemovalBlockHeight emissionstypes.BlockHeight, |
Comment on lines
+22
to
+23
| //nolint:exhaustruct | ||
| var _ types.MsgServiceServer = msgServer{} |
There was a problem hiding this comment.
P3: Use (*msgServer)(nil) instead of msgServer{} for the interface compliance check. This avoids the exhaustruct warning without needing the //nolint directive.
(Based on your team's feedback about using var declarations to avoid exhaustruct warnings and nolint directives.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/msgserver/msg_server.go, line 22:
<comment>Use `(*msgServer)(nil)` instead of `msgServer{}` for the interface compliance check. This avoids the `exhaustruct` warning without needing the `//nolint` directive.
(Based on your team's feedback about using var declarations to avoid exhaustruct warnings and nolint directives.) </comment>
<file context>
@@ -1,20 +1,42 @@
}
-var _ types.MsgServiceServer = msgServer{k: keeper.Keeper{}}
+//nolint:exhaustruct
+var _ types.MsgServiceServer = msgServer{}
</file context>
Suggested change
| //nolint:exhaustruct | |
| var _ types.MsgServiceServer = msgServer{} | |
| var _ types.MsgServiceServer = (*msgServer)(nil) |
There was a problem hiding this comment.
26 issues found across 131 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 (unresolved 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/migrations/v4/migrate.go">
<violation number="1" location="x/emissions/migrations/v4/migrate.go:62">
P3: Use a `var` declaration for this zero-value struct that is populated via unmarshalling, which eliminates the need for the `//nolint:exhaustruct` directive.
(Based on your team's feedback about using var declarations for zero-value structs populated later.) [FEEDBACK_USED]</violation>
</file>
<file name="x/emissions/keeper/topic.go">
<violation number="1" location="x/emissions/keeper/topic.go:202">
P2: Copy-paste error in error message: says `"error getting topic fee revenue"` but this function retrieves the total sum of previous topic weights, not topic fee revenue. This will produce misleading log messages during debugging.</violation>
</file>
<file name="x/emissions/keeper/score_utils.go">
<violation number="1" location="x/emissions/keeper/score_utils.go:154">
P2: Address validation in `SetLowestReputerScoreEma` will reject the zero-value `Score` returned by `GetLowestScoreFromAllReputers` when all score lookups fail (since `Address` will be empty). The `Update*` methods chain these calls without handling this edge case. Consider either skipping the `Set` call when no valid scores were found, or having `GetLowestScoreFromAll*` return a distinct error/flag when all lookups fail.</violation>
</file>
<file name="x/emissions/keeper/queryserver/query_server.go">
<violation number="1" location="x/emissions/keeper/queryserver/query_server.go:8">
P3: Use a zero-value `var` declaration to avoid the `//nolint:exhaustruct` directive. For example:
```go
var (
_qs queryServer
_ types.QueryServiceServer = _qs
)
This creates a zero-value struct without a composite literal, so exhaustruct won't flag it.
(Based on your team's feedback about using var declarations instead of composite literals to avoid exhaustruct nolint directives.) [FEEDBACK_USED]
(Based on your team's feedback about using var declarations for zero-value structs populated later.) [FEEDBACK_USED]
(Based on your team's feedback about using var declarations to avoid exhaustruct warnings and nolint directives.) [FEEDBACK_USED]
After calling SetReputerScoreEma for the initial score, update previousEmaScore to reflect the newly stored value.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if pagination != nil { | ||
| if len(pagination.Key) > 0 { | ||
| cursor = binary.BigEndian.Uint64(pagination.Key) |
There was a problem hiding this comment.
P0: Potential panic: binary.BigEndian.Uint64 requires at least 8 bytes, but only len > 0 is checked. A pagination key with 1–7 bytes will cause an index-out-of-range panic, which could crash the node from a malformed client request.
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 pagination key with 1–7 bytes will cause an index-out-of-range panic, which could crash the node from a malformed client request.</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>
| topicId uint64, | ||
| ) (removal types.DelegateStakeRemovalInfo, found bool, err error) { | ||
| rng := NewTriplePrefixedQuadrupleRange[ActorId, ActorId, TopicId, BlockHeight](delegator, reputer, topicId) | ||
| iter, err := k.delegateStakeRemovalsByActor.Iterate(ctx, rng) |
There was a problem hiding this comment.
P1: Missing defer iter.Close() — iterator resource leak. Every other Iterate call in this file is followed by defer iter.Close(), but this one is not. This can leak resources, especially under repeated calls.
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 512:
<comment>Missing `defer iter.Close()` — iterator resource leak. Every other `Iterate` call in this file is followed by `defer iter.Close()`, but this one is not. This can leak resources, especially under repeated calls.</comment>
<file context>
@@ -0,0 +1,953 @@
+ topicId uint64,
+) (removal types.DelegateStakeRemovalInfo, found bool, err error) {
+ rng := NewTriplePrefixedQuadrupleRange[ActorId, ActorId, TopicId, BlockHeight](delegator, reputer, topicId)
+ iter, err := k.delegateStakeRemovalsByActor.Iterate(ctx, rng)
+ if err != nil {
+ return types.DelegateStakeRemovalInfo{}, false, errorsmod.Wrap(err, "error iterating over delegate stake removals by actor")
</file context>
| return k.previousPercentageRewardToStakedReputers.Set(ctx, percentageRewardToStakedReputers) | ||
| } | ||
|
|
||
| func (k *ScoresKeeper) GetPreviousPercentageRewardToStakedReputers(ctx context.Context) (alloraMath.Dec, error) { |
There was a problem hiding this comment.
P1: GetPreviousPercentageRewardToStakedReputers doesn't handle collections.ErrNotFound, unlike every other getter in this file. When the value hasn't been set yet, callers will receive an unexpected error instead of a sensible default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/scores.go, line 628:
<comment>`GetPreviousPercentageRewardToStakedReputers` doesn't handle `collections.ErrNotFound`, unlike every other getter in this file. When the value hasn't been set yet, callers will receive an unexpected error instead of a sensible default.</comment>
<file context>
@@ -0,0 +1,704 @@
+ return k.previousPercentageRewardToStakedReputers.Set(ctx, percentageRewardToStakedReputers)
+}
+
+func (k *ScoresKeeper) GetPreviousPercentageRewardToStakedReputers(ctx context.Context) (alloraMath.Dec, error) {
+ return k.previousPercentageRewardToStakedReputers.Get(ctx)
+}
</file context>
Comment on lines
+181
to
186
| topicExists, err := qs.tk.TopicExists(ctx, req.TopicId) | ||
| if !topicExists { | ||
| return nil, status.Errorf(codes.NotFound, "topic %v not found", req.TopicId) | ||
| } else if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
P1: Bug: error check is evaluated after !topicExists, so if TopicExists returns an error, topicExists defaults to false and the code returns a misleading "topic not found" instead of the actual error. Check err != nil first. This same pattern appears in all three GetTopicInitial*EmaScore functions.
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 is evaluated after `!topicExists`, so if `TopicExists` returns an error, `topicExists` defaults to `false` and the code returns a misleading "topic not found" instead of the actual error. Check `err != nil` first. This same pattern appears in all three `GetTopicInitial*EmaScore` functions.</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>
Suggested change
| topicExists, err := qs.tk.TopicExists(ctx, req.TopicId) | |
| if !topicExists { | |
| return nil, status.Errorf(codes.NotFound, "topic %v not found", req.TopicId) | |
| } else if err != nil { | |
| return nil, err | |
| } | |
| topicExists, err := qs.tk.TopicExists(ctx, req.TopicId) | |
| if err != nil { | |
| return nil, err | |
| } else if !topicExists { | |
| return nil, status.Errorf(codes.NotFound, "topic %v not found", req.TopicId) | |
| } |
| func (qs queryServer) GetCurrentLowestInfererScore(ctx context.Context, req *types.GetCurrentLowestInfererScoreRequest) (_ *types.GetCurrentLowestInfererScoreResponse, err error) { | ||
| defer metrics.RecordMetrics("GetCurrentLowestInfererScore", time.Now(), &err) | ||
| lowestInfererScore, found, err := qs.k.GetLowestInfererScoreEma(ctx, req.TopicId) | ||
| lowestInfererScore, found, err := qs.sck.GetLowestInfererScoreEma(ctx, req.TopicId) |
There was a problem hiding this comment.
P1: Bug: when !found is true (and err is nil), errorsmod.Wrap(err, ...) returns nil because err is nil at this point (already checked in the prior branch). The function returns (nil, nil) — no response and no error — which is incorrect. Use status.Errorf or errorsmod.New instead of wrapping a nil error. Same issue in GetCurrentLowestForecasterScore and GetCurrentLowestReputerScore.
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 103:
<comment>Bug: when `!found` is true (and `err` is nil), `errorsmod.Wrap(err, ...)` returns `nil` because `err` is nil at this point (already checked in the prior branch). The function returns `(nil, nil)` — no response and no error — which is incorrect. Use `status.Errorf` or `errorsmod.New` instead of wrapping a nil error. Same issue in `GetCurrentLowestForecasterScore` and `GetCurrentLowestReputerScore`.</comment>
<file context>
@@ -99,7 +100,7 @@ func (qs queryServer) GetWorkerInferenceScoresAtBlock(ctx context.Context, req *
func (qs queryServer) GetCurrentLowestInfererScore(ctx context.Context, req *types.GetCurrentLowestInfererScoreRequest) (_ *types.GetCurrentLowestInfererScoreResponse, err error) {
defer metrics.RecordMetrics("GetCurrentLowestInfererScore", time.Now(), &err)
- lowestInfererScore, found, err := qs.k.GetLowestInfererScoreEma(ctx, req.TopicId)
+ lowestInfererScore, found, err := qs.sck.GetLowestInfererScoreEma(ctx, req.TopicId)
if err != nil {
return nil, errorsmod.Wrap(err, "error getting lowest inferer score EMA")
</file context>
| if errors.Is(err, collections.ErrNotFound) { | ||
| topic, err := k.topicKeeper.GetTopic(ctx, topicId) | ||
| if err != nil { | ||
| return types.TimestampedValue{ |
There was a problem hiding this comment.
P3: Inconsistent error-path return: GetInfererNetworkRegret returns TimestampedValue{BlockHeight: 0, Value: alloraMath.ZeroDec()} on error, while every other Get method returns types.TimestampedValue{}. Consider aligning with the other methods for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/regrets.go, line 76:
<comment>Inconsistent error-path return: `GetInfererNetworkRegret` returns `TimestampedValue{BlockHeight: 0, Value: alloraMath.ZeroDec()}` on error, while every other Get method returns `types.TimestampedValue{}`. Consider aligning with the other methods for consistency.</comment>
<file context>
@@ -0,0 +1,403 @@
+ if errors.Is(err, collections.ErrNotFound) {
+ topic, err := k.topicKeeper.GetTopic(ctx, topicId)
+ if err != nil {
+ return types.TimestampedValue{
+ BlockHeight: 0,
+ Value: alloraMath.ZeroDec(),
</file context>
| @@ -58,7 +59,7 @@ func MigrateStore(ctx sdk.Context, emissionsKeeper keeper.Keeper) error { | |||
| // migrate params for this new version | |||
| // 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 | |||
There was a problem hiding this comment.
P3: Use var oldParams oldV4Types.Params instead of a composite literal with a //nolint:exhaustruct directive. Since this struct is immediately populated via proto.Unmarshal, a var declaration is cleaner and avoids the need for the nolint comment.
(Based on your team's feedback about using var declarations for zero-value structs populated later.)
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>Use `var oldParams oldV4Types.Params` instead of a composite literal with a `//nolint:exhaustruct` directive. Since this struct is immediately populated via `proto.Unmarshal`, a `var` declaration is cleaner and avoids the need for the nolint comment.
(Based on your team's feedback about using var declarations for zero-value structs populated later.) </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 | |
| var oldParams oldV4Types.Params // empty struct used by cosmos-sdk Unmarshal below |
| // 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) |
There was a problem hiding this comment.
P3: Typo in method name: WeightstsKeeper() should be WeightsKeeper(). The method has an extra "ts" in the name ("WeightstsKeeper" vs "WeightsKeeper"). Since this PR is restructuring the keeper, this is a good opportunity to fix the method name in test_utils.go and all call sites.
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 method has an extra "ts" in the name ("Weights**ts**Keeper" vs "WeightsKeeper"). Since this PR is restructuring the keeper, this is a good opportunity to fix the method name in `test_utils.go` and all call sites.</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>
| @@ -108,14 +97,30 @@ func RemoveStakes( | |||
| return nil | |||
| } | |||
|
|
|||
| type RemoveDelegateStakeFn func( | |||
| ctx context.Context, | |||
| stakeRemovalBlockHeight emissionskeeper.BlockHeight, | |||
There was a problem hiding this comment.
P3: Inconsistent package used for BlockHeight type: RemoveDelegateStakeFn uses emissionskeeper.BlockHeight while all other function types in this file use emissionstypes.BlockHeight. Use emissionstypes.BlockHeight consistently.
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 102:
<comment>Inconsistent package used for `BlockHeight` type: `RemoveDelegateStakeFn` uses `emissionskeeper.BlockHeight` while all other function types in this file use `emissionstypes.BlockHeight`. Use `emissionstypes.BlockHeight` consistently.</comment>
<file context>
@@ -108,14 +97,30 @@ func RemoveStakes(
+type RemoveDelegateStakeFn func(
+ ctx context.Context,
+ stakeRemovalBlockHeight emissionskeeper.BlockHeight,
+ topicId emissionskeeper.TopicId,
+ delegator, reputer emissionskeeper.ActorId,
</file context>
Suggested change
| stakeRemovalBlockHeight emissionskeeper.BlockHeight, | |
| stakeRemovalBlockHeight emissionstypes.BlockHeight, |
Comment on lines
+22
to
+23
| //nolint:exhaustruct | ||
| var _ types.MsgServiceServer = msgServer{} |
There was a problem hiding this comment.
P3: Use (*msgServer)(nil) instead of msgServer{} for the interface compliance check. This avoids the exhaustruct warning without needing the //nolint directive.
(Based on your team's feedback about using var declarations to avoid exhaustruct warnings and nolint directives.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/msgserver/msg_server.go, line 22:
<comment>Use `(*msgServer)(nil)` instead of `msgServer{}` for the interface compliance check. This avoids the `exhaustruct` warning without needing the `//nolint` directive.
(Based on your team's feedback about using var declarations to avoid exhaustruct warnings and nolint directives.) </comment>
<file context>
@@ -1,20 +1,42 @@
}
-var _ types.MsgServiceServer = msgServer{k: keeper.Keeper{}}
+//nolint:exhaustruct
+var _ types.MsgServiceServer = msgServer{}
</file context>
Suggested change
| //nolint:exhaustruct | |
| var _ types.MsgServiceServer = msgServer{} | |
| var _ types.MsgServiceServer = (*msgServer)(nil) |
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.
Purpose of Changes and their Description
Link(s) to Ticket(s) or Issue(s) resolved by this PR
Are these changes tested and documented?
Unreleasedsection ofCHANGELOG.md?Still Left Todo
Fill this out if this is a Draft PR so others can help.
Summary by cubic
Restructured the emissions keeper into focused sub-keepers and rewired msg/query servers to use them, improving readability, testability, and separation of concerns with no intended behavior changes.
Refactors
Migration
Written for commit 23a7d31. Summary will update on new commits.