implement the topic label registry#918
implement the topic label registry#918zale144 wants to merge 1 commit intoalek/engn-5126-1-core-protos-topic-config-extensionsfrom
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf Linter / buf (pull_request).
|
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
cubic analysis
3 issues found across 7 files
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/proto/emissions/v10/topic.proto">
<violation number="1" location="x/emissions/proto/emissions/v10/topic.proto:116">
P2: According to linked Linear issue ENGN-5151, the registry should be keyed by `(topic_id, nonce)` and must be "compatible with existing epoch-scoped pruning patterns." The existing `Nonce` type uses `int64 block_height`, but this field is `uint64 epoch_id`. Consider using `int64` (or referencing the existing `Nonce` type directly) to ensure compatibility with existing pruning infrastructure, and aligning the field name with `nonce` to match the spec.</violation>
</file>
<file name="x/emissions/keeper/keeper.go">
<violation number="1" location="x/emissions/keeper/keeper.go:5068">
P2: Inconsistent error-checking pattern: the rest of the keeper uses `errors.Is(err, collections.ErrNotFound)` but this function uses `errorsmod.IsOf(err, collections.ErrNotFound)`. Use the established `errors.Is` pattern for consistency and to avoid subtle mismatch risks when errors are wrapped.</violation>
</file>
<file name="x/emissions/keeper/keeper_test.go">
<violation number="1" location="x/emissions/keeper/keeper_test.go:6172">
P2: The outer `newFixture()` call in the test loop is wasteful—every test case internally calls `newFixture()` again and either shadows the passed-in `ctx`/`k` (cases 2–5) or only uses the inner `topicId`/`nonce` (case 1). This creates an unused topic per test run. Simplify by either passing all four fixture values into `tc.run` (and removing the inner `newFixture()` calls), or removing the outer `newFixture()` and letting each test case fully manage its own setup.</violation>
</file>
Linked issue analysis
Linked issue: ENGN-5151: 2. Epoch label registry - types & storage
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Define EpochLabelRegistry keyed by (`topic_id`, `nonce`) | Added proto message and Keeper map keyed by Pair(topicId,nonce) |
| ✅ | KV layout and marshal/unmarshal helpers | Added TopicLabelRegistry key and generated proto marshal/unmarshal code |
| ✅ | Provide read helpers to fetch registry for (topic, nonce) | GetEpochLabelRegistry and lookup helpers added |
| ✅ | Provide write helpers to persist registry (explicit writes only) | RegisterEpochLabel persists registry via topicLabelRegistry.Set |
| ✅ | No mutations occur automatically (mutations only on explicit calls) | Get returns default without setting; mutations only via explicit Register or pruning |
| ✅ | Compatible with existing epoch-scoped pruning patterns | Prune integrated: pruneTopicLabelRegistry uses Clear and is called in pruning flow |
| ✅ | Registry can be stored and retrieved for (topic, nonce) (validated by tests) | Unit tests register labels and verify retrieval and lookups |
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| message EpochLabelRegistry { | ||
| uint64 topic_id = 1; | ||
| uint64 epoch_id = 2; |
There was a problem hiding this comment.
P2: According to linked Linear issue ENGN-5151, the registry should be keyed by (topic_id, nonce) and must be "compatible with existing epoch-scoped pruning patterns." The existing Nonce type uses int64 block_height, but this field is uint64 epoch_id. Consider using int64 (or referencing the existing Nonce type directly) to ensure compatibility with existing pruning infrastructure, and aligning the field name with nonce to match the spec.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/proto/emissions/v10/topic.proto, line 116:
<comment>According to linked Linear issue ENGN-5151, the registry should be keyed by `(topic_id, nonce)` and must be "compatible with existing epoch-scoped pruning patterns." The existing `Nonce` type uses `int64 block_height`, but this field is `uint64 epoch_id`. Consider using `int64` (or referencing the existing `Nonce` type directly) to ensure compatibility with existing pruning infrastructure, and aligning the field name with `nonce` to match the spec.</comment>
<file context>
@@ -105,3 +105,14 @@ message TopicIdWeightPair {
+
+message EpochLabelRegistry {
+ uint64 topic_id = 1;
+ uint64 epoch_id = 2;
+ repeated TopicLabel labels = 3;
+}
</file context>
| ) (types.EpochLabelRegistry, error) { | ||
| registry, err := k.topicLabelRegistry.Get(ctx, collections.Join(topicId, nonce)) | ||
| if err != nil { | ||
| if errorsmod.IsOf(err, collections.ErrNotFound) { |
There was a problem hiding this comment.
P2: Inconsistent error-checking pattern: the rest of the keeper uses errors.Is(err, collections.ErrNotFound) but this function uses errorsmod.IsOf(err, collections.ErrNotFound). Use the established errors.Is pattern for consistency and to avoid subtle mismatch risks when errors are wrapped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/keeper.go, line 5068:
<comment>Inconsistent error-checking pattern: the rest of the keeper uses `errors.Is(err, collections.ErrNotFound)` but this function uses `errorsmod.IsOf(err, collections.ErrNotFound)`. Use the established `errors.Is` pattern for consistency and to avoid subtle mismatch risks when errors are wrapped.</comment>
<file context>
@@ -5043,3 +5055,124 @@ func (k *Keeper) updateTopicWeightAfterStakeChange(
+) (types.EpochLabelRegistry, error) {
+ registry, err := k.topicLabelRegistry.Get(ctx, collections.Join(topicId, nonce))
+ if err != nil {
+ if errorsmod.IsOf(err, collections.ErrNotFound) {
+ return types.EpochLabelRegistry{
+ TopicId: topicId,
</file context>
| if errorsmod.IsOf(err, collections.ErrNotFound) { | |
| if errors.Is(err, collections.ErrNotFound) { |
|
|
||
| for _, tc := range tests { | ||
| s.Run(tc.name, func() { | ||
| ctx, k, _, _ := newFixture() |
There was a problem hiding this comment.
P2: The outer newFixture() call in the test loop is wasteful—every test case internally calls newFixture() again and either shadows the passed-in ctx/k (cases 2–5) or only uses the inner topicId/nonce (case 1). This creates an unused topic per test run. Simplify by either passing all four fixture values into tc.run (and removing the inner newFixture() calls), or removing the outer newFixture() and letting each test case fully manage its own setup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At x/emissions/keeper/keeper_test.go, line 6172:
<comment>The outer `newFixture()` call in the test loop is wasteful—every test case internally calls `newFixture()` again and either shadows the passed-in `ctx`/`k` (cases 2–5) or only uses the inner `topicId`/`nonce` (case 1). This creates an unused topic per test run. Simplify by either passing all four fixture values into `tc.run` (and removing the inner `newFixture()` calls), or removing the outer `newFixture()` and letting each test case fully manage its own setup.</comment>
<file context>
@@ -6061,3 +6062,115 @@ func (s *KeeperTestSuite) TestRemoveTopicFromPreviousTopicWeights() {
+
+ for _, tc := range tests {
+ s.Run(tc.name, func() {
+ ctx, k, _, _ := newFixture()
+ tc.run(ctx, k)
+ })
</file context>
Purpose of Changes and their Description
add the topic label registry types and store implementation
Link(s) to Ticket(s) or Issue(s) resolved by this PR
https://linear.app/alloralabs/issue/ENGN-5151/2-epoch-label-registry-types-and-storage
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
Implements an epoch-scoped topic label registry with new proto types, KV storage, and simple register/lookup helpers. Preps classification-based topics per ENGN-5151 without integrating mutation flows yet.
Written for commit e28ffab. Summary will update on new commits.