Migrate trigger module from KV store to collections.#2546
Migrate trigger module from KV store to collections.#2546nagarajdivine wants to merge 14 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces raw KV store usage in x/trigger with cosmossdk.io/collections: Keeper now accepts a runtime KVStoreService, exposes typed collections (triggers, event listeners, queue, metadata), and many storage APIs now return errors; keys/codecs, genesis, migrations, msg/query servers, and tests adapted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Msg as MsgServer (CreateTrigger)
participant Keeper as TriggerKeeper (collections)
participant NextID as NextTriggerID (Item)
participant Triggers as TriggersMap (Map)
participant Events as EventListeners (KeySet)
Note over Msg,Keeper: CreateTrigger — collection-backed flow
Msg->>Keeper: NewTriggerWithID(ctx, owner, event, actions)
Keeper->>NextID: Get(ctx)
alt NotFound
NextID-->>Keeper: NotFound / treat as 0
else Found
NextID-->>Keeper: currentID
end
Keeper->>NextID: Set(ctx, currentID+1)
Keeper->>Triggers: Set(ctx, id, trigger)
Triggers-->>Keeper: ok / error
Keeper->>Events: Set(ctx, (eventHash, order, id))
Events-->>Keeper: ok / error
Keeper-->>Msg: (trigger, error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
x/trigger/keeper/query_server.go (1)
46-54: CollectionPaginate usage and ID binding look goodUsing
query.CollectionPaginateoverk.TriggersMapwith a transformer that setstrigger.Id = keypreserves the previous API contract where the ID field matches the storage key. The internal error is correctly wrapped as anInternalgRPC status, and the response assembly is straightforward. If you want to simplify later, you could drop the explicit type parameters and let the compiler infer them, but it's optional.Also applies to: 56-61, 64-64
x/trigger/keeper/event_listener_test.go (1)
173-237: New collections-based listener tests are solid; minor consistency nitThe new collections tests cover set/get, iteration, and single-listener removal nicely and validate the
ErrEventNotFoundpath after removal. One minor consistency improvement: at Line 234 you ignore the error fromtrigger1.GetTriggerEventI(), whereas earlier in the same test you assert on it. For clarity and future-proofing, consider:- eventI, _ := trigger1.GetTriggerEventI() + eventI, err := trigger1.GetTriggerEventI() + s.Require().NoError(err, "failed to get TriggerEventI for removed trigger1")Otherwise this block looks good.
x/trigger/keeper/trigger_test.go (1)
248-282: Trigger ID sequence test is appropriate, with minor enhancement opportunity
TestTriggerIDSequencecorrectly validates initialNextTriggerIDand the increments after twoNewTriggerWithIDcalls. For robustness, you might later add a check that the created triggers are also persisted as expected, but for verifying the sequence logic this test is already useful.x/trigger/types/codec.go (1)
4-9: FixedBytes32KeyCodec correctly enforces and preserves 32‑byte keysThe codec’s binary behavior (strict 32‑byte length check, fixed
Size, copy in/out) looks correct for preserving the legacy[32-byte-hash]prefix.StringifyandKeyTypeare straightforward, and using the same logic for terminal and non‑terminal usage is appropriate for composite keys.The only potential gap is
EncodeJSON/DecodeJSONreturning “not implemented” errors. If anything in the future tries to JSON‑encode event listener keys (e.g., debug tooling, introspection), this will surface as a runtime error. Optional improvement:
- Implement JSON as hex string encoding/decoding, or
- Add a brief comment noting these are intentionally unsupported and not used anywhere.
Also applies to: 47-111
x/trigger/types/keys.go (1)
157-165: Duplicate event-name hashing logic across types and keeper
GetEventNameBytesnormalizes and hashes event names, andkeeper/event_listener.goadds a separategetEventNameHashwith effectively identical logic. This duplication is brittle and risks drift if one implementation changes.Consider reusing
GetEventNameBytesfrom the keeper instead of re-implementing the hash there (or moving the helper to a shared location intypesand calling it from both places).x/trigger/keeper/event_listener.go (2)
113-121:RemoveAllEventListenersForTriggeronly removes one listener despite the method name
RemoveAllEventListenersForTriggercurrently loads the trigger and callsRemoveEventListeneronce:trigger, err := k.GetTrigger(ctx, triggerID) ... _, err = k.RemoveEventListener(ctx, trigger)If a trigger can have more than one associated event-listener entry (now or in future—e.g. multiple orders per event), this will only remove a single entry, contradicting the “All” in the method name.
If the invariant really is “one listener per trigger”, consider either:
- Renaming this to
RemoveEventListenerForTrigger, or- Adding a brief comment documenting that only a single listener can exist per trigger.
If multiple listeners per trigger are possible, this should iterate and remove all matching entries instead.
133-141: Duplicate event-name hashing withtypes.GetEventNameBytes
getEventNameHashnormalizes and hashes the event name usingstrings.ToLower(strings.TrimSpace())and SHA-256. This is effectively the same astypes.GetEventNameBytesinx/trigger/types/keys.go, but with a different panic message.To avoid divergence and keep hashing semantics centralized, prefer reusing
types.GetEventNameByteshere instead of maintaining a second copy of the logic.For example:
-func getEventNameHash(name string) []byte { - eventName := strings.ToLower(strings.TrimSpace(name)) - if len(eventName) == 0 { - panic("invalid event name: cannot be empty") - } - hash := sha256.Sum256([]byte(eventName)) - return hash[:] -} +func getEventNameHash(name string) []byte { + return types.GetEventNameBytes(name) +}x/trigger/keeper/queue.go (2)
13-17:QueueTriggersilently dropsEnqueueerrors
Enqueuehas been upgraded to return anerror, butQueueTriggerstill ignores it:func (k Keeper) QueueTrigger(ctx sdk.Context, trigger types.Trigger) { item := types.NewQueuedTrigger(...) k.Enqueue(ctx, item) // error ignored }Given that the
collections.Map.Set/Item.Setoperations can now fail, silently discarding the error means a failure to enqueue a trigger is completely invisible to callers.Consider either:
- Propagating the error up the call chain by changing
QueueTrigger’s signature, or- At least logging and/or panicking on a non-nil error to avoid silent data loss.
Also applies to: 44-71
109-119: Prefer reusingGetQueueLengthsemantics inQueueIsEmpty
QueueIsEmptymanually interpretscollections.ErrNotFoundas “length == 0” while treating other errors as “not empty”:length, err := k.QueueLength.Get(ctx) if err != nil { if errors.Is(err, collections.ErrNotFound) { return true } return false } return length == 0You’ve already encapsulated the “not found => 0 length” behavior in
GetQueueLength. Using that here would keep semantics in one place and avoid divergence:func (k Keeper) QueueIsEmpty(ctx sdk.Context) bool { - length, err := k.QueueLength.Get(ctx) + length, err := k.GetQueueLength(ctx) if err != nil { - if errors.Is(err, collections.ErrNotFound) { - return true - } - return false + // optionally: log or panic, rather than silently reporting "not empty" + return false } return length == 0 }Also applies to: 224-233
x/trigger/keeper/trigger.go (2)
19-32:RemoveTriggerhides underlying storage errors
RemoveTriggernow checks existence and then removes, but converts anyHas/Removeerrors into a barefalse:exists, err := k.TriggersMap.Has(ctx, id) if err != nil { return false } if exists { if err := k.TriggersMap.Remove(ctx, id); err != nil { return false } return true } return falseThis makes it impossible for callers to distinguish “not found” from “I/O or codec error”, which may complicate debugging real failures.
Consider either:
- Changing the signature to
(bool, error)and returning the underlying error, or- At least logging the error before returning
false.
78-87:HasTriggershould preferTriggersMap.HasoverGet
HasTriggercurrently does a fullGet:_, err := k.TriggersMap.Get(ctx, id) if errors.Is(err, collections.ErrNotFound) { return false, nil } if err != nil { return false, err } return true, nilFor a simple existence check this is unnecessarily expensive;
Map.Hasis designed for this and avoids decoding the value.A lighter-weight implementation:
func (k Keeper) HasTrigger(ctx sdk.Context, id uint64) (bool, error) { - _, err := k.TriggersMap.Get(ctx, id) - if errors.Is(err, collections.ErrNotFound) { - return false, nil - } - if err != nil { - return false, err - } - return true, nil + has, err := k.TriggersMap.Has(ctx, id) + if err != nil { + return false, err + } + return has, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.changelog/unreleased/features/2499-trigger-module-collections.md(1 hunks)app/app.go(1 hunks)x/trigger/keeper/event_detector_test.go(1 hunks)x/trigger/keeper/event_listener.go(2 hunks)x/trigger/keeper/event_listener_test.go(1 hunks)x/trigger/keeper/genesis.go(1 hunks)x/trigger/keeper/keeper.go(2 hunks)x/trigger/keeper/keeper_test.go(1 hunks)x/trigger/keeper/migrations_v2.go(1 hunks)x/trigger/keeper/msg_server.go(3 hunks)x/trigger/keeper/query_server.go(3 hunks)x/trigger/keeper/queue.go(2 hunks)x/trigger/keeper/queue_test.go(1 hunks)x/trigger/keeper/trigger.go(3 hunks)x/trigger/keeper/trigger_test.go(2 hunks)x/trigger/simulation/genesis.go(1 hunks)x/trigger/types/codec.go(2 hunks)x/trigger/types/keys.go(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keys.go:3-12
Timestamp: 2025-08-15T20:59:51.949Z
Learning: In Cosmos SDK, key prefixes only need to be unique within a given module's store, not across the entire application. Each module operates within its own isolated key-value store namespace, so the same byte prefixes can be safely reused across different modules without causing collisions.
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: app/upgrades.go:52-53
Timestamp: 2025-09-05T19:28:28.271Z
Learning: The asset module in the Provenance blockchain doesn't maintain its own state storage and therefore doesn't require a StoreKey in upgrade configurations. It delegates all storage operations to the NFT module, unlike other modules like ledger and registry which have their own state stores.
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:57-80
Timestamp: 2024-10-08T18:12:51.935Z
Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.
Applied to files:
x/trigger/keeper/migrations_v2.go
📚 Learning: 2025-08-15T20:59:51.949Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keys.go:3-12
Timestamp: 2025-08-15T20:59:51.949Z
Learning: In Cosmos SDK, key prefixes only need to be unique within a given module's store, not across the entire application. Each module operates within its own isolated key-value store namespace, so the same byte prefixes can be safely reused across different modules without causing collisions.
Applied to files:
x/trigger/keeper/query_server.go
📚 Learning: 2025-08-15T22:52:24.461Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keeper.go:74-173
Timestamp: 2025-08-15T22:52:24.461Z
Learning: In the ledger keeper NewKeeper function in x/ledger/keeper/keeper.go, SpicyLemon prefers to allow bankKeeper and registryKeeper dependencies to be nil rather than adding validation that would panic on nil values. The design intentionally supports nil keepers for flexibility.
Applied to files:
app/app.gox/trigger/keeper/keeper.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
app/app.gox/trigger/keeper/queue.gox/trigger/keeper/keeper.go
📚 Learning: 2025-08-15T20:34:01.110Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/codec.go:9-10
Timestamp: 2025-08-15T20:34:01.110Z
Learning: In Go Cosmos SDK projects, sdk.Msg interface embeds proto.Message interface, making it valid to copy elements from []sdk.Msg to []proto.Message even though the slice types are different. The pattern of declaring AllRequestMsgs as []sdk.Msg for type assertions while copying to []proto.Message for RegisterImplementations is a common and correct pattern.
Applied to files:
x/trigger/types/codec.go
📚 Learning: 2025-08-15T20:34:01.110Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/codec.go:9-10
Timestamp: 2025-08-15T20:34:01.110Z
Learning: The pattern of declaring AllRequestMsgs as []sdk.Msg and then copying to []proto.Message for RegisterImplementations is a standard, correct pattern used throughout the Provenance codebase. This works because sdk.Msg interface embeds proto.Message interface, making the copy operation valid. This pattern appears consistently across 15+ modules and compiles without errors.
Applied to files:
x/trigger/types/codec.go
📚 Learning: 2024-10-02T03:37:18.486Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server_test.go:1225-1228
Timestamp: 2024-10-02T03:37:18.486Z
Learning: When test cases are run using `s.T().Run` instead of `s.Run`, use `require` with `t` instead of `s.Require()`, to ensure that assertions apply to the correct test instance.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
Applied to files:
.changelog/unreleased/features/2499-trigger-module-collections.md
📚 Learning: 2024-10-02T01:43:01.360Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/marker/types/send_restrictions.go:11-11
Timestamp: 2024-10-02T01:43:01.360Z
Learning: In `x/marker/types/send_restrictions.go`, the `transferAgentKey` is only used for storing data in ephemeral contexts, so changing its value does not impact backward compatibility.
Applied to files:
x/trigger/types/keys.go
📚 Learning: 2025-08-15T20:19:31.726Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/simulation/decoder.go:8-14
Timestamp: 2025-08-15T20:19:31.726Z
Learning: The asset module in Provenance doesn't use collections-based storage like ledger/registry modules. Instead, it delegates storage to the NFT module, so a collections-based store decoder isn't applicable here. The module should use its own types.StoreKey rather than exchange.StoreKey in RegisterStoreDecoder.
Applied to files:
x/trigger/keeper/keeper.go
🧬 Code graph analysis (11)
x/trigger/keeper/queue_test.go (3)
x/trigger/keeper/keeper_test.go (1)
KeeperTestSuite(28-41)x/trigger/types/trigger.pb.go (6)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/tx.pb.go (3)
MsgDestroyTriggerRequest(145-150)MsgDestroyTriggerRequest(154-154)MsgDestroyTriggerRequest(155-157)
x/trigger/keeper/keeper_test.go (1)
x/trigger/types/keys.go (2)
ModuleName(14-14)GetTriggerKey(95-102)
x/trigger/keeper/query_server.go (3)
x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/query.pb.go (3)
QueryTriggersResponse(172-177)QueryTriggersResponse(181-181)QueryTriggersResponse(182-184)
app/app.go (1)
x/trigger/keeper/keeper.go (1)
NewKeeper(30-58)
x/trigger/keeper/trigger_test.go (3)
x/trigger/keeper/keeper_test.go (1)
KeeperTestSuite(28-41)x/trigger/types/trigger.pb.go (3)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)x/trigger/types/tx.pb.go (3)
MsgDestroyTriggerRequest(145-150)MsgDestroyTriggerRequest(154-154)MsgDestroyTriggerRequest(155-157)
x/trigger/keeper/queue.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)x/trigger/types/errors.go (1)
ErrQueueIndexNotFound(10-10)
x/trigger/keeper/event_listener_test.go (5)
x/trigger/keeper/keeper_test.go (1)
KeeperTestSuite(28-41)x/trigger/types/trigger.pb.go (9)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)BlockTimeEvent(216-219)BlockTimeEvent(223-223)BlockTimeEvent(224-226)Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/tx.pb.go (3)
MsgDestroyTriggerRequest(145-150)MsgDestroyTriggerRequest(154-154)MsgDestroyTriggerRequest(155-157)x/trigger/types/trigger.go (1)
NewTrigger(149-156)x/trigger/types/errors.go (1)
ErrEventNotFound(9-9)
x/trigger/keeper/event_listener.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (2)
ErrEventNotFound(9-9)ErrTriggerNotFound(8-8)
x/trigger/keeper/keeper.go (3)
x/trigger/types/trigger.pb.go (6)
Trigger(33-42)Trigger(46-46)Trigger(47-49)QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)x/trigger/types/keys.go (6)
TriggerKeyPrefix(59-59)EventListenerKeyPrefix(61-61)QueueKeyPrefix(63-63)NextTriggerIDKey(65-65)QueueStartIndexKey(67-67)QueueLengthKey(69-69)x/trigger/types/codec.go (1)
EventListenerKeyCodec(115-121)
x/trigger/keeper/trigger.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)
x/trigger/keeper/msg_server.go (1)
x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: go-test-multi-db (simple, goleveldb, ubuntu-latest)
- GitHub Check: go-test-single-db (nondeterminism, ubuntu-latest)
- GitHub Check: runsim (after-import, goleveldb, ubuntu-latest)
- GitHub Check: runsim (multi-seed-short, goleveldb, ubuntu-latest)
- GitHub Check: go-test-single-db (benchmark-invariants, ubuntu-latest)
- GitHub Check: runsim (import-export, goleveldb, ubuntu-latest)
- GitHub Check: go-test-multi-db (benchmark, goleveldb, ubuntu-latest)
- GitHub Check: test-race (04)
- GitHub Check: test-race (02)
- GitHub Check: test-race (03)
- GitHub Check: tests (04)
- GitHub Check: test-race (00)
- GitHub Check: tests (01)
- GitHub Check: test-race (01)
- GitHub Check: tests (02)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: Build OSX
- GitHub Check: Build Linux
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: heighliner-docker
🔇 Additional comments (17)
x/trigger/simulation/genesis.go (1)
179-184: Good cleanup: nolint removal with explicit safety check.The removal of the nolint directive is appropriate here. The bounds check on line 179 ensures
triggerID ≤ math.MaxIntbefore the conversion, making theint(triggerID)cast on line 183 explicitly safe. Replacing a lint suppression with proper bounds validation is a solid practice..changelog/unreleased/features/2499-trigger-module-collections.md (1)
1-1: LGTM!The changelog entry is clear and properly references the related issue.
x/trigger/keeper/genesis.go (1)
11-19: LGTM!The error handling for
getTriggerIDandgetQueueStartIndexis appropriate for genesis export operations. Panicking on error is consistent with the existing error handling pattern in this function.x/trigger/keeper/migrations_v2.go (1)
28-36: LGTM!The migration to
StoreService.OpenKVStoreand the error handling for iterator creation are well-implemented. Returning early on iterator creation failure is appropriate for a migration operation.x/trigger/keeper/keeper_test.go (1)
103-119: LGTM!This backward compatibility test is essential for validating that the collections-based storage can read data stored in the old KV format. The test approach is sound: storing with the old key format and reading with the new collections-based method ensures a smooth migration path.
x/trigger/keeper/msg_server.go (2)
39-42: LGTM!The error handling for
NewTriggerWithIDis properly implemented. Capturing and propagating the error ensures that trigger creation failures are correctly surfaced to the caller.
61-63: LGTM!The error mapping from
collections.ErrNotFoundtotypes.ErrTriggerNotFoundis good practice. This provides a proper abstraction layer, preventing internal collection errors from leaking into the public API.x/trigger/keeper/queue_test.go (2)
289-308: LGTM!The test properly validates basic queue operations with the collections-based implementation, including enqueue, peek, and dequeue operations.
310-336: LGTM!The test effectively validates FIFO ordering and queue length tracking across multiple enqueue/dequeue operations.
app/app.go (1)
645-645: LGTM!The TriggerKeeper initialization correctly uses
runtime.NewKVStoreServiceto wrap the store key, aligning with the collections-based storage migration. This pattern is consistent with other keeper initializations in the codebase.x/trigger/keeper/query_server.go (1)
5-5: Error mapping for missing triggers looks correctMapping
collections.ErrNotFoundtotypes.ErrTriggerNotFoundkeeps the public query semantics stable after the collections migration. Returning the module error from a gRPC query is consistent with other Cosmos modules and will be translated appropriately upstream. No changes needed.Also applies to: 10-10, 29-32
x/trigger/keeper/event_listener_test.go (1)
239-259: End-to-end RemoveAllEventListenersForTrigger behavior verifiedThis test cleanly exercises the “remove all listeners for a trigger” path and confirms that subsequent
GetEventListenercalls returnErrEventNotFound. It’s a good complement to the single-listener removal test and validates the collections-backed key scheme.x/trigger/types/codec.go (1)
113-121: EventListenerKeyCodec matches the documented legacy key layoutComposing
FixedBytes32KeyCodecwithcollections.Uint64Keytwice viacollections.TripleKeyCodecaligns with the documented legacy format[32-byte-hash][8-byte-order][8-byte-triggerID]. This should allow collections to read and write keys compatible with the pre‑collections trigger event listener store while still giving you ordered iteration by (hash, order, triggerID). Looks good.x/trigger/keeper/event_listener.go (1)
16-51: Collections-based EventListeners usage looks correctThe migration of event listeners to a
collections.KeySet[Triple[[]byte,uint64,uint64]]looks sound:
- Keys are consistently composed via
Join3(getEventNameHash(...), order, triggerID).RemoveEventListenerproperly checks existence withHasbefore removing and surfaces errors.GetEventListenermapsErrNotFoundtotypes.ErrEventNotFoundand then fetches the trigger from the main trigger map, which keeps the listener store as an index only.IterateEventListenersusesNewPrefixedTripleRangewith the event-name hash, skips stale entries where the trigger is missing, and respects the handler’s stop flag.I don’t see functional issues here; this is a good fit for collections.
Also applies to: 53-104
x/trigger/keeper/queue.go (1)
121-143: Iterator usage over Queue looks goodThe collections-based iteration in
GetAllQueueItemsanditerateQueueis straightforward and error-aware:
Queue.Iterate(ctx, nil)to cover the full range.- Proper
defer iter.Close().- Decode-time errors from
Key()/Value()are surfaced immediately.- Handlers control early termination via the
stopflag.This aligns well with collections’ iterator patterns.
Also applies to: 180-204, 206-238
x/trigger/keeper/trigger.go (2)
14-44: Core collections migration for triggers looks correctThe main trigger storage changes are consistent and error-aware:
SetTriggerdelegates toTriggersMap.Setand returns the error.GetTriggerusesTriggersMap.Get, mappingcollections.ErrNotFoundtotypes.ErrTriggerNotFoundand propagating other errors.IterateTriggersusesTriggersMap.Iterate(ctx, nil)anditerator.KeyValue()to feed typed triggers to the handler.GetAllTriggerssimply wrapsIterateTriggersto build a slice.This is a clean migration off raw KV access and preserves module-specific error types.
Also applies to: 46-76
89-117: Trigger ID handling is internally consistent but should be double-checked against existing stateThe new
NextTriggerIDlogic treats the stored value as “current ID”:
NewTriggerWithID:
- Reads
currentIDfromNextTriggerID, defaulting to 0 on not found.- Sets
nextID := currentID + 1.- Writes
NextTriggerID = nextIDand returns a trigger withId = nextID.GetNextTriggerID:
- Reads
currentIDand returnscurrentID + 1(or 1 if missing).setTriggerIDwritestriggerID - 1toNextTriggerID, consistent with the above (“store current so next is triggerID”).getTriggerIDreturns the stored current ID (or 0 if none).Within this file, those semantics line up and should yield IDs 1, 2, 3, … without gaps under normal use.
Given the PR’s requirement of “no state migration”, it’s worth verifying that the old implementation also treated the stored value as “current ID” (as opposed to “next ID to use”). If legacy state persisted “next ID”, you might need a one-time adjustment or a compatibility shim when reading
NextTriggerIDfor the first time.Also applies to: 119-146
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
x/trigger/keeper/trigger_test.go (1)
113-117: Don’t discardNewTriggerWithIDerror in test
NewTriggerWithIDnow returns(Trigger, error), but this test ignoreserr:trigger, _ := s.app.TriggerKeeper.NewTriggerWithID(s.ctx, tc.expected.Owner, tc.expected.Event, tc.expected.Actions)If an internal error occurs, the test will still compare
triggerand may pass or fail for the wrong reason. Capture and assert the error before the equality:- trigger, _ := s.app.TriggerKeeper.NewTriggerWithID(s.ctx, tc.expected.Owner, tc.expected.Event, tc.expected.Actions) - s.Equal(tc.expected, trigger, "should have correct trigger from NewTrigger") + trigger, err := s.app.TriggerKeeper.NewTriggerWithID(s.ctx, tc.expected.Owner, tc.expected.Event, tc.expected.Actions) + s.Require().NoError(err, "NewTriggerWithID should not return an error") + s.Equal(tc.expected, trigger, "should have correct trigger from NewTrigger")x/trigger/keeper/queue.go (1)
23-44: Fix queue metadata handling:QueueStartIndexstill uninitialized,QueuePeekandDequeuebypass helpersOn a fresh store,
EnqueuesetsQueueLengthbut never initializesQueueStartIndex.QueuePeekandDequeuethen readQueueStartIndexdirectly via.Get, leading to:
QueuePeek:QueueIsEmptyis false (length == 1), butQueueStartIndex.Getreturnscollections.ErrNotFound, so it returnsnileven though an item exists.Dequeue: same situation, but it panics on theErrNotFound.You already have helpers
getQueueStartIndexandGetQueueLengththat default missing metadata to 0; using them here fixes the bug and keeps behavior consistent.Suggested fix:
func (k Keeper) QueuePeek(ctx sdk.Context) *types.QueuedTrigger { - isEmpty := k.QueueIsEmpty(ctx) - - if isEmpty { - return nil - } - startIndex, err := k.QueueStartIndex.Get(ctx) - if err != nil { - if errors.Is(err, collections.ErrNotFound) { - return nil - } - return nil - } + if k.QueueIsEmpty(ctx) { + return nil + } + startIndex, err := k.getQueueStartIndex(ctx) + if err != nil { + // optional: log the error before treating as empty + return nil + } item, err := k.Queue.Get(ctx, startIndex) if err != nil { if errors.Is(err, collections.ErrNotFound) { return nil } return nil } return &item } func (k Keeper) Dequeue(ctx sdk.Context) { if k.QueueIsEmpty(ctx) { panic("unable to dequeue from empty queue.") } - // Get the start index - startIndex, err := k.QueueStartIndex.Get(ctx) - if err != nil { - panic(err) - } - - // Get the queue length - length, err := k.QueueLength.Get(ctx) - if err != nil { - panic(err) - } + startIndex, err := k.getQueueStartIndex(ctx) + if err != nil { + panic(err) + } + length, err := k.GetQueueLength(ctx) + if err != nil { + panic(err) + } // Remove the first item if err := k.Queue.Remove(ctx, startIndex); err != nil { panic(err) } // Update queue metadata newStart := startIndex + 1 newLength := length - 1 if err := k.QueueStartIndex.Set(ctx, newStart); err != nil { panic(err) } if err := k.QueueLength.Set(ctx, newLength); err != nil { panic(err) } }This makes a newly initialized queue behave correctly and avoids panics due to missing metadata.
Also applies to: 76-110
🧹 Nitpick comments (4)
x/trigger/keeper/event_listener_test.go (1)
174-237: Harden event-listener tests by checkingGetTriggerEventIerrors and aligning setupIn the new collections tests you correctly assert on errors from
SetTrigger,SetEventListener,IterateEventListeners, andRemoveEventListener, but still ignore the error fromGetTriggerEventI(e.g.,eventI, _ := trigger1.GetTriggerEventI()and the similar call inTestCollectionsRemoveAllEventListenersForTrigger). It would be more robust to capture andRequire().NoErrorthese, and to update the older tests in this file to also handle the new error-returningSetTrigger/SetEventListenersignatures instead of discarding them, so setup failures can’t silently pass.A possible tweak here:
- eventI, _ := trigger1.GetTriggerEventI() + eventI, err := trigger1.GetTriggerEventI() + s.Require().NoError(err, "failed to get TriggerEventI for removed trigger1")(and likewise for the other call sites and older tests’ setup).
x/trigger/keeper/trigger_registry.go (1)
10-36: Good step with logging; consider whether callers should see register/unregister failuresAdding error logging around
SetTrigger,SetEventListener, andRemoveEventListeneris an improvement over the previous silent failures. Depending on howRegisterTrigger/UnregisterTriggerare used, you may eventually want these to surface an error or status back to callers instead of only logging, so that failed registration/unregistration cannot be mistaken for success. For now, the behavior is backward‑compatible, so this can stay a follow‑up consideration.x/trigger/keeper/event_listener.go (1)
106-141: Consider reusingtypes.GetEventNameBytesinstead of duplicating hashing logic
getEventNameHashreplicates the normalization and sha256 hashing already implemented intypes.GetEventNameBytes. To avoid drift if that logic ever changes, consider delegating:func getEventNameHash(name string) []byte { return types.GetEventNameBytes(name) }This keeps all event-name hashing rules centralized in the types package.
x/trigger/keeper/queue.go (1)
13-20: Queue helpers and iteration mostly look good; consider small cleanups
QueueTriggernow logs failures fromEnqueue, which is a sensible non-breaking improvement.QueueIsEmpty,GetAllQueueItems,GetQueueItem,SetQueueItem,RemoveQueueIndex,IterateQueue, and theget/sethelpers for start index and length are consistent with the collections-based design and error semantics.Minor optional cleanups:
- In
GetAllQueueItems, you don't neediter.Key()if the key is unused; relying oniter.Value()is enough.- In
QueueIsEmpty, you might want to log unexpected errors instead of silently treating them as "empty".These are non-blocking and can be deferred.
Also applies to: 112-143, 145-205, 206-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
x/trigger/keeper/event_detector_test.go(1 hunks)x/trigger/keeper/event_listener.go(2 hunks)x/trigger/keeper/event_listener_test.go(1 hunks)x/trigger/keeper/genesis.go(2 hunks)x/trigger/keeper/keeper.go(2 hunks)x/trigger/keeper/migrations_v2.go(2 hunks)x/trigger/keeper/queue.go(2 hunks)x/trigger/keeper/queue_test.go(1 hunks)x/trigger/keeper/trigger_registry.go(1 hunks)x/trigger/keeper/trigger_test.go(2 hunks)x/trigger/types/codec.go(2 hunks)x/trigger/types/keys.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/trigger/keeper/event_detector_test.go
- x/trigger/keeper/migrations_v2.go
🧰 Additional context used
🧠 Learnings (18)
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 1993
File: x/marker/client/cli/cli_test.go:1090-1090
Timestamp: 2024-10-08T18:12:51.935Z
Learning: SpicyLemon prefers allowing tests to fail naturally if the setup is incorrect rather than explicitly checking and handling such errors within the test functions. They also recommend using `s.Require().NotNil(...)` instead of `s.T().Fatal(...)` for assertions in test functions.
Applied to files:
x/trigger/keeper/event_listener_test.gox/trigger/keeper/trigger_test.go
📚 Learning: 2024-10-10T23:19:54.565Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades_test.go:610-610
Timestamp: 2024-10-10T23:19:54.565Z
Learning: In `app/upgrades_test.go`, we prefer to keep TODO comments (like `TODO[viridian]`) to handle code deletions manually, and do not want to implement automated processes for code removal, especially for unit tests.
Applied to files:
x/trigger/keeper/event_listener_test.gox/trigger/keeper/trigger_test.gox/trigger/types/keys.go
📚 Learning: 2025-08-15T20:57:04.455Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/entries_test.go:26-31
Timestamp: 2025-08-15T20:57:04.455Z
Learning: SpicyLemon prefers string-based error assertions in tests using empty string to indicate "no error expected" and non-empty string for expected error messages. Use assertions.AssertErrorValue instead of errors.Is() comparisons because errors.Is() with s.Require().True() provides poor failure messages that only say "Should be true" without showing expected vs actual error content.
Applied to files:
x/trigger/keeper/event_listener_test.gox/trigger/keeper/trigger_test.go
📚 Learning: 2025-08-15T20:34:01.110Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/codec.go:9-10
Timestamp: 2025-08-15T20:34:01.110Z
Learning: In Go Cosmos SDK projects, sdk.Msg interface embeds proto.Message interface, making it valid to copy elements from []sdk.Msg to []proto.Message even though the slice types are different. The pattern of declaring AllRequestMsgs as []sdk.Msg for type assertions while copying to []proto.Message for RegisterImplementations is a common and correct pattern.
Applied to files:
x/trigger/types/codec.go
📚 Learning: 2025-08-15T20:34:01.110Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/codec.go:9-10
Timestamp: 2025-08-15T20:34:01.110Z
Learning: The pattern of declaring AllRequestMsgs as []sdk.Msg and then copying to []proto.Message for RegisterImplementations is a standard, correct pattern used throughout the Provenance codebase. This works because sdk.Msg interface embeds proto.Message interface, making the copy operation valid. This pattern appears consistently across 15+ modules and compiles without errors.
Applied to files:
x/trigger/types/codec.go
📚 Learning: 2024-10-02T03:04:32.929Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4_test.go:253-479
Timestamp: 2024-10-02T03:04:32.929Z
Learning: In Go tests within this project, it's acceptable to have similar helper functions like `newUUID`, `newScopeID`, `newSpecID`, and `newScope` defined within each test. Tests may tailor these functions to specific needs rather than sharing them, to avoid breaking other tests when changes are made and to keep them non-public.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2025-08-15T20:01:55.078Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: app/upgrades_test.go:0-0
Timestamp: 2025-08-15T20:01:55.078Z
Learning: SpicyLemon prefers upgrade tests to fail when expected data is missing rather than skip, using s.Require().NoError() and s.Require().NotEmpty() to enforce hard requirements for upgrade data files in test scenarios.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2024-10-02T03:37:18.486Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server_test.go:1225-1228
Timestamp: 2024-10-02T03:37:18.486Z
Learning: When test cases are run using `s.T().Run` instead of `s.Run`, use `require` with `t` instead of `s.Require()`, to ensure that assertions apply to the correct test instance.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2024-10-02T03:42:47.070Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/query_server_test.go:940-945
Timestamp: 2024-10-02T03:42:47.070Z
Learning: In the `TestScopeQuery` function, the conditional checks following the equality assertion are designed to help identify differences when `tc.expResp` and `actResp` are not equal and both are not `nil`. If they are equal, or either is `nil`, the failure message from the equality assertion suffices, and further checks are not needed.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/account_data_test.go:11-11
Timestamp: 2024-10-08T18:12:51.935Z
Learning: Do not raise issues about the `keeper` package still being in use after import removal in test files when there are no unresolved references.
Applied to files:
x/trigger/keeper/trigger_test.gox/trigger/keeper/keeper.go
📚 Learning: 2025-08-15T22:52:24.461Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keeper.go:74-173
Timestamp: 2025-08-15T22:52:24.461Z
Learning: In the ledger keeper NewKeeper function in x/ledger/keeper/keeper.go, SpicyLemon prefers to allow bankKeeper and registryKeeper dependencies to be nil rather than adding validation that would panic on nil values. The design intentionally supports nil keepers for flexibility.
Applied to files:
x/trigger/keeper/keeper.gox/trigger/keeper/genesis.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/trigger/keeper/keeper.gox/trigger/keeper/queue.gox/trigger/keeper/genesis.go
📚 Learning: 2024-10-02T02:06:21.664Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:57-80
Timestamp: 2024-10-02T02:06:21.664Z
Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.
Applied to files:
x/trigger/keeper/keeper.go
📚 Learning: 2025-08-15T20:19:31.726Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/simulation/decoder.go:8-14
Timestamp: 2025-08-15T20:19:31.726Z
Learning: The asset module in Provenance doesn't use collections-based storage like ledger/registry modules. Instead, it delegates storage to the NFT module, so a collections-based store decoder isn't applicable here. The module should use its own types.StoreKey rather than exchange.StoreKey in RegisterStoreDecoder.
Applied to files:
x/trigger/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: iramiller
Repo: provenance-io/provenance PR: 2160
File: x/metadata/client/cli/tx.go:1196-1212
Timestamp: 2024-10-08T18:12:51.935Z
Learning: When checking for indentation issues in multi-line strings assigned to the `Long` field in Go code, verify that the source lines are actually indented before flagging them.
Applied to files:
x/trigger/keeper/queue_test.go
📚 Learning: 2024-10-02T01:43:01.360Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/marker/types/send_restrictions.go:11-11
Timestamp: 2024-10-02T01:43:01.360Z
Learning: In `x/marker/types/send_restrictions.go`, the `transferAgentKey` is only used for storing data in ephemeral contexts, so changing its value does not impact backward compatibility.
Applied to files:
x/trigger/types/keys.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils_test.go:478-493
Timestamp: 2024-10-08T18:12:51.935Z
Learning: In Go tests, modifying byte slices directly using arithmetic operations like addition and subtraction is acceptable, even if it causes overflow, as overflow on `uint8` just wraps around. Avoid flagging such usage, and avoid suggesting bitwise operations which can be harder to read and understand.
Applied to files:
x/trigger/types/keys.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/trigger/types/keys.go
🧬 Code graph analysis (8)
x/trigger/keeper/trigger_registry.go (2)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)
x/trigger/keeper/event_listener_test.go (5)
x/trigger/keeper/keeper_test.go (1)
KeeperTestSuite(28-41)x/trigger/types/trigger.pb.go (9)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)BlockTimeEvent(216-219)BlockTimeEvent(223-223)BlockTimeEvent(224-226)Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/tx.pb.go (3)
MsgDestroyTriggerRequest(145-150)MsgDestroyTriggerRequest(154-154)MsgDestroyTriggerRequest(155-157)x/trigger/types/trigger.go (1)
NewTrigger(149-156)x/trigger/types/errors.go (1)
ErrEventNotFound(9-9)
x/trigger/keeper/event_listener.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/errors.go (2)
ErrEventNotFound(9-9)ErrTriggerNotFound(8-8)
x/trigger/keeper/trigger_test.go (3)
x/trigger/keeper/keeper_test.go (1)
KeeperTestSuite(28-41)x/trigger/types/trigger.pb.go (3)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)x/trigger/types/tx.pb.go (3)
MsgDestroyTriggerRequest(145-150)MsgDestroyTriggerRequest(154-154)MsgDestroyTriggerRequest(155-157)
x/trigger/keeper/keeper.go (3)
x/trigger/types/trigger.pb.go (6)
Trigger(33-42)Trigger(46-46)Trigger(47-49)QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)x/trigger/types/keys.go (6)
TriggerKeyPrefix(59-59)EventListenerKeyPrefix(61-61)QueueKeyPrefix(63-63)NextTriggerIDKey(65-65)QueueStartIndexKey(67-67)QueueLengthKey(69-69)x/trigger/types/codec.go (1)
EventListenerKeyCodec(115-121)
x/trigger/keeper/queue.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)x/trigger/types/errors.go (1)
ErrQueueIndexNotFound(10-10)
x/trigger/keeper/queue_test.go (3)
x/trigger/keeper/keeper_test.go (1)
KeeperTestSuite(28-41)x/trigger/types/trigger.pb.go (6)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/tx.pb.go (3)
MsgDestroyTriggerRequest(145-150)MsgDestroyTriggerRequest(154-154)MsgDestroyTriggerRequest(155-157)
x/trigger/keeper/genesis.go (2)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/genesis.pb.go (3)
GenesisState(27-39)GenesisState(43-43)GenesisState(44-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: tests (04)
- GitHub Check: test-race (00)
- GitHub Check: tests (03)
- GitHub Check: test-race (03)
- GitHub Check: tests (00)
- GitHub Check: test-race (02)
- GitHub Check: test-race (04)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: test-race (01)
- GitHub Check: build-linux
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: docker
- GitHub Check: heighliner-docker
- GitHub Check: ghcr-publish
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
x/trigger/keeper/trigger_test.go (1)
208-282: New collections-based trigger tests provide solid coverage
TestTriggerCollectionsOperationsandTestTriggerIDSequenceexercise the collections-backedSetTrigger,GetTrigger,HasTrigger,RemoveTrigger, andGetNextTriggerID/NewTriggerWithIDpaths, asserting on errors and expected IDs. This is good targeted coverage of the new storage model.x/trigger/keeper/genesis.go (1)
13-34: Centralizedmusthelper and error-aware genesis wiring look goodWiring all genesis reads/writes through
getTriggerID/getQueueStartIndex/GetAll*plus themusthelper gives consistent, contextual panics for collections-backed state initialization, while preserving the “fail fast” behavior on invalid genesis or storage errors. The updatedNewGenesisStateconstruction also cleanly reflects the new queue/triggers layout. LGTM.Also applies to: 42-62
x/trigger/keeper/keeper.go (1)
15-27: Keeper wiring to collections and StoreService assignment look correctThe keeper now correctly stores the
store.KVStoreService, builds a collections schema, and initializes all maps/items with the expected prefixes, matching the documented key layout and avoiding a migration. No issues spotted in the struct layout orNewKeeperwiring.Also applies to: 30-58
x/trigger/types/codec.go (1)
47-121: Event listener key codecs correctly preserve the legacy key layout
FixedBytes32KeyCodecandEventListenerKeyCodecimplement the collections key codec interface as expected and encode keys as[32-byte-hash][8-byte-order][8-byte-triggerID], matching the legacy event-listener key structure when combined with the 0x02 prefix. The explicit JSON-not-implemented errors are reasonable here.x/trigger/types/keys.go (1)
57-69: Prefix migration to collections.Prefix maintains on-disk key compatibilityThe switch to
collections.NewPrefix(...)for all trigger-related prefixes, combined with using.Bytes()(or explicit[]byteconversion) in the helpers, preserves the documented key layout and keeps both collections and legacy raw-key helpers usable without a migration.Also applies to: 85-131
x/trigger/keeper/event_listener.go (1)
16-68: Collections-based event listener storage and iteration look soundUsing a
KeySetover the triple(eventHash, order, triggerID)plusNewPrefixedTripleRangeyields the correct per-event iteration semantics, andGetEventListener/IterateEventListenersappropriately rely onGetTriggerand treatErrTriggerNotFoundas a soft failure. Overall logic and error handling here look solid.Also applies to: 70-104
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
x/trigger/keeper/event_listener.go (2)
36-47: Consider simplifying the existence check.The
Hascheck beforeRemoveis defensive but potentially redundant. The collections API'sRemovemethod should indicate whether the key existed. However, the current explicit approach is clear and safe.
125-125: Incorrect or outdated nolint comment.The comment
//nolint:revive // safe conversionsuggests a type conversion, butcountis already declared asuint64, so no conversion occurs. This comment should be removed or clarified if it addresses a different linting issue.Apply this diff:
- err := k.IterateEventListeners(ctx, eventName, func(trigger triggertypes.Trigger) (bool, error) { //nolint:revive // safe conversion + err := k.IterateEventListeners(ctx, eventName, func(trigger triggertypes.Trigger) (bool, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/trigger/keeper/event_listener.go(2 hunks)x/trigger/simulation/genesis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/trigger/simulation/genesis.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:57-80
Timestamp: 2024-10-08T18:12:51.935Z
Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:57-80
Timestamp: 2024-10-02T02:06:21.664Z
Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.
🧬 Code graph analysis (1)
x/trigger/keeper/event_listener.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (2)
ErrEventNotFound(9-9)ErrTriggerNotFound(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: tests (03)
- GitHub Check: tests (04)
- GitHub Check: test-race (04)
- GitHub Check: test-race (02)
- GitHub Check: tests (01)
- GitHub Check: tests (02)
- GitHub Check: test-race (01)
- GitHub Check: tests (00)
- GitHub Check: test-race (03)
- GitHub Check: test-race (00)
- GitHub Check: heighliner-docker
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: build-linux
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
x/trigger/keeper/event_listener.go (6)
3-13: LGTM: Imports align with collections migration.The imports correctly reflect the migration to collections-based storage, including the necessary crypto, error handling, and collections packages.
16-24: LGTM: Clean migration to collections API.The function correctly uses the hashed composite key approach and returns errors appropriately.
53-67: LGTM: Correct separation of concerns.The explicit
Hascheck before fetching the trigger correctly distinguishes between "event listener entry missing" and "trigger missing," returning the appropriate error type.
105-110: LGTM: Clean helper function.The implementation is straightforward and correctly delegates to the collections API.
134-134: Event name case normalization is intentional and consistently applied.The verification confirms that lowercase normalization of event names is deliberate design, not an oversight. The same normalization (
strings.ToLower(strings.TrimSpace(name))) is applied in bothgetEventNameHash(event_listener.go:134) andGetEventNameBytes(keys.go:159), ensuring all event listener lookups use consistent hashing. Tests validate this behavior works correctly. There are no collisions or unexpected case-sensitivity issues because all event name operations uniformly normalize at the storage/lookup layer.
86-93: Defensive handling of orphaned event listener entries is necessary and correctly implemented.The code correctly skips triggers that are not found (lines 89-91). Verification confirms orphaned event listener entries are indeed possible: if
RemoveEventListenerfails duringUnregisterTrigger(trigger_registry.go:28-33), the listener persists while its trigger is deleted. The defensive check properly handles this edge case.However, orphaned entries can accumulate if removal failures occur repeatedly. Consider whether a periodic cleanup mechanism is warranted to prevent unbounded growth of orphaned entries in storage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
x/trigger/keeper/event_listener.go (1)
126-134: Function name is misleading.The function is named
RemoveAllEventListenersForTriggerbut only removes a single event listener. Since aTriggerhas only oneEventfield, this is functionally correct, but the name suggests it would remove multiple entries.Consider renaming to
RemoveEventListenerForTriggerto match the actual behavior.Apply this diff:
-// RemoveAllEventListenersForTrigger removes all event listeners for a specific trigger -func (k Keeper) RemoveAllEventListenersForTrigger(ctx sdk.Context, triggerID uint64) error { +// RemoveEventListenerForTrigger removes the event listener for a specific trigger +func (k Keeper) RemoveEventListenerForTrigger(ctx sdk.Context, triggerID uint64) error {
🧹 Nitpick comments (2)
x/trigger/keeper/event_listener.go (2)
99-104: Silently skipping missing triggers may hide inconsistencies.When a trigger is not found (line 100-101), the code continues iteration without logging or alerting. This could occur if an event listener key exists but its corresponding trigger was deleted, indicating data inconsistency. While this defensive approach prevents iteration failures, it may mask bugs during development or migration.
Consider logging a warning when this occurs to aid debugging.
Example:
trigger, err := k.GetTrigger(ctx, triggerID) if err != nil { if errors.Is(err, triggertypes.ErrTriggerNotFound) { + // Log warning about orphaned event listener + ctx.Logger().Warn("event listener references non-existent trigger", "triggerID", triggerID, "eventName", eventName) continue } return err }
146-154: Consider using a more specific error for invalid input.Returning
ErrEventNotFoundfor an empty event name (line 150) is semantically misleading. This error typically means "event not found in storage," but an empty string is actually invalid input rather than a missing entry.Consider wrapping the error with more context:
func getEventNameHash(name string) ([]byte, error) { eventName := strings.ToLower(strings.TrimSpace(name)) if len(eventName) == 0 { - return nil, triggertypes.ErrEventNotFound + return nil, triggertypes.ErrEventNotFound.Wrap("event name cannot be empty") } hash := sha256.Sum256([]byte(eventName)) return hash[:], nil }This provides clearer context for debugging while maintaining error type consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/trigger/keeper/event_listener.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `GetLockedCoins` function in `x/hold/keeper/locked_coins.go` must conform to the `banktypes.GetLockedCoinsFn` type, which does not allow returning an error. In cases where `k.GetHoldCoins` encounters an error, using `panic` is considered appropriate due to the severity of such errors.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2025-08-15T20:39:34.095Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/util.go:68-102
Timestamp: 2025-08-15T20:39:34.095Z
Learning: In the Provenance codebase, functions with "Must" prefix (like MustGetMarkerAddress) can panic and should only be used when you know they won't panic. Instead, use the twin function that returns an error (like GetMarkerAddress) and handle the error properly.
Applied to files:
x/trigger/keeper/event_listener.go
🧬 Code graph analysis (1)
x/trigger/keeper/event_listener.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (2)
ErrEventNotFound(9-9)ErrTriggerNotFound(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: tests (01)
- GitHub Check: tests (04)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: tests (02)
- GitHub Check: test-race (03)
- GitHub Check: test-race (01)
- GitHub Check: test-race (00)
- GitHub Check: test-race (04)
- GitHub Check: test-race (02)
- GitHub Check: ghcr-publish
- GitHub Check: heighliner-docker
- GitHub Check: docker
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
🔇 Additional comments (6)
x/trigger/keeper/event_listener.go (6)
4-8: LGTM: Imports are appropriate.The new imports support the collections-based implementation and event name hashing.
16-27: LGTM: SetEventListener correctly stores hashed composite keys.The function properly hashes the event name, constructs a composite key, and stores it in the EventListeners KeySet.
30-56: LGTM: RemoveEventListener has correct semantics.The function appropriately checks existence before removal and returns
(false, nil)when the listener doesn't exist, which is good defensive programming.
59-76: LGTM: GetEventListener correctly handles KeySet design.The function appropriately checks the EventListeners KeySet for existence, then fetches the actual trigger from the TriggersMap. This is correct since EventListeners stores only relationships, not trigger data.
116-124: LGTM: HasEventListener is a useful helper.This function provides a clean way to check event listener existence without fetching the trigger.
136-144: LGTM: GetEventListenerCount is correct but iterates all entries.The function correctly counts event listeners by iterating all matching entries. While this has O(n) complexity, it's acceptable given the collections KeySet API doesn't provide a direct count mechanism.
|
The import-export sim is failing: https://github.com/provenance-io/provenance/actions/runs/19579700988/job/56074364016?pr=2546 That test starts a new chain, runs some Txs, stops the chain, exports genesis, then creates a new chain by importing that genesis file. It then checks that both the chains have the exact same state. It's failing, which usually means that state is changing between export and import genesis. You'll need to fix that. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
x/trigger/keeper/event_listener_test.go (2)
201-209: Drop or use the unusedexpectErrfieldThe
expectErrfield in the table forTestCollectionsSetAndGetEventListeneris never read. Either assert onerrusing this flag or remove the field to keep the test table minimal and easier to maintain.
239-258: Align test name withRemoveEventListenerForTriggerIDbehaviorThe test is still named
TestCollectionsRemoveAllEventListenersForTrigger, but it exercisesRemoveEventListenerForTriggerID, which removes a single listener for one trigger ID. Renaming the test to match the method name would reduce confusion when grepping for this behavior.x/trigger/keeper/query_server.go (1)
26-35: Wrap non‑ErrNotFounderrors inTriggerByIDwith more contextYou correctly translate
collections.ErrNotFoundtotypes.ErrTriggerNotFound, but other errors are returned as‑is. For consistency with the rest of the keeper (where errors are wrapped with context) and easier debugging, consider wrapping here too, e.g. including the trigger ID in the message before returning.x/trigger/keeper/event_listener.go (4)
17-35: Centralize key construction viaGetEventListenerKey
SetEventListenerusestriggertypes.GetEventListenerKey, butRemoveEventListenerbuilds keys withcollections.Join3directly. For consistency and to avoid future representation drift, consider usingGetEventListenerKeyeverywhere you construct an event-listener key (including inRemoveEventListener,GetEventListener, andHasEventListener).Also applies to: 38-64
38-64: Wrap propagated errors with context for easier debuggingMost of these methods (
RemoveEventListener,GetEventListener,IterateEventListeners,HasEventListener,RemoveEventListenerForTriggerID,GetEventListenerCount) return underlying errors directly, whileSetEventListenerwraps them with helpful context. Adding similar context (e.g., trigger ID, event prefix) before returning would make it much easier to trace failures in production.Also applies to: 66-84, 86-122, 124-152
86-122: Double-check the decision to silently skip missing triggers in iterationIn
IterateEventListeners, youcontinuewhenGetTriggerreturnsErrTriggerNotFound. That’s defensively safe, but it also silently hides inconsistencies between the event-listener index and the trigger store. Consider at least wrapping this with logging or surfacing a different error if missing triggers should never happen.
134-142: Update comment to matchRemoveEventListenerForTriggerIDbehaviorThe comment still says “RemoveAllEventListenersForTrigger removes all event listeners for a specific trigger”, but the function is named
RemoveEventListenerForTriggerIDand delegates toRemoveEventListenerfor a single trigger. Update the comment (and any external references) so the name and description align.x/trigger/types/keys.go (1)
67-69: FixQueueLengthPrefixcomment typoThe comment says
QueueLengthPrefixbut the identifier isQueueLengthKey. Updating the comment to match the actual name will avoid confusion when reading or grepping the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
x/trigger/keeper/event_listener.go(2 hunks)x/trigger/keeper/event_listener_test.go(1 hunks)x/trigger/keeper/genesis.go(2 hunks)x/trigger/keeper/query_server.go(3 hunks)x/trigger/simulation/genesis.go(1 hunks)x/trigger/types/keys.go(2 hunks)x/trigger/types/keys_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/trigger/simulation/genesis.go
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `GetLockedCoins` function in `x/hold/keeper/locked_coins.go` must conform to the `banktypes.GetLockedCoinsFn` type, which does not allow returning an error. In cases where `k.GetHoldCoins` encounters an error, using `panic` is considered appropriate due to the severity of such errors.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2025-08-15T20:39:34.095Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/util.go:68-102
Timestamp: 2025-08-15T20:39:34.095Z
Learning: In the Provenance codebase, functions with "Must" prefix (like MustGetMarkerAddress) can panic and should only be used when you know they won't panic. Instead, use the twin function that returns an error (like GetMarkerAddress) and handle the error properly.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-10T23:13:56.722Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: x/ibchooks/types/params.go:22-26
Timestamp: 2024-10-10T23:13:56.722Z
Learning: In the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `Validate` method without modifying the error messages, even if they may lack additional context.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:106-109
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In the `AddScopeDataAccess` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 1993
File: x/marker/client/cli/cli_test.go:1090-1090
Timestamp: 2024-10-08T18:12:51.935Z
Learning: SpicyLemon prefers allowing tests to fail naturally if the setup is incorrect rather than explicitly checking and handling such errors within the test functions. They also recommend using `s.Require().NotNil(...)` instead of `s.T().Fatal(...)` for assertions in test functions.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2024-10-10T23:19:54.565Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades_test.go:610-610
Timestamp: 2024-10-10T23:19:54.565Z
Learning: In `app/upgrades_test.go`, we prefer to keep TODO comments (like `TODO[viridian]`) to handle code deletions manually, and do not want to implement automated processes for code removal, especially for unit tests.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2025-08-15T20:57:04.455Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/entries_test.go:26-31
Timestamp: 2025-08-15T20:57:04.455Z
Learning: SpicyLemon prefers string-based error assertions in tests using empty string to indicate "no error expected" and non-empty string for expected error messages. Use assertions.AssertErrorValue instead of errors.Is() comparisons because errors.Is() with s.Require().True() provides poor failure messages that only say "Should be true" without showing expected vs actual error content.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2025-08-15T22:52:24.461Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keeper.go:74-173
Timestamp: 2025-08-15T22:52:24.461Z
Learning: In the ledger keeper NewKeeper function in x/ledger/keeper/keeper.go, SpicyLemon prefers to allow bankKeeper and registryKeeper dependencies to be nil rather than adding validation that would panic on nil values. The design intentionally supports nil keepers for flexibility.
Applied to files:
x/trigger/keeper/genesis.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/trigger/keeper/genesis.go
📚 Learning: 2024-10-02T01:43:01.360Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/marker/types/send_restrictions.go:11-11
Timestamp: 2024-10-02T01:43:01.360Z
Learning: In `x/marker/types/send_restrictions.go`, the `transferAgentKey` is only used for storing data in ephemeral contexts, so changing its value does not impact backward compatibility.
Applied to files:
x/trigger/types/keys_test.go
📚 Learning: 2024-10-02T03:06:21.997Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-02T03:06:21.997Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/trigger/types/keys_test.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/trigger/types/keys.go
📚 Learning: 2025-08-15T20:59:51.949Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keys.go:3-12
Timestamp: 2025-08-15T20:59:51.949Z
Learning: In Cosmos SDK, key prefixes only need to be unique within a given module's store, not across the entire application. Each module operates within its own isolated key-value store namespace, so the same byte prefixes can be safely reused across different modules without causing collisions.
Applied to files:
x/trigger/keeper/query_server.go
🧬 Code graph analysis (4)
x/trigger/keeper/event_listener.go (5)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/keys.go (1)
GetEventListenerKey(73-78)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (2)
ErrEventNotFound(9-9)ErrTriggerNotFound(8-8)
x/trigger/keeper/genesis.go (2)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/genesis.pb.go (3)
GenesisState(27-39)GenesisState(43-43)GenesisState(44-46)
x/trigger/types/keys.go (2)
x/trigger/simulation/genesis.go (1)
TriggerID(23-23)x/trigger/types/trigger.go (1)
TriggerID(16-16)
x/trigger/keeper/query_server.go (4)
x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/query.pb.go (3)
QueryTriggersResponse(172-177)QueryTriggersResponse(181-181)QueryTriggersResponse(182-184)x/trigger/simulation/genesis.go (1)
Triggers(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: tests (04)
- GitHub Check: test-race (04)
- GitHub Check: test-race (02)
- GitHub Check: test-race (03)
- GitHub Check: test-race (00)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: test-race (01)
- GitHub Check: tests (00)
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: golangci-lint
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: heighliner-docker
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
x/trigger/keeper/genesis.go (1)
13-33: ExportGenesis: collections-based access pattern looks consistentUsing
getTriggerID,getQueueStartIndex,GetAllTriggers, andGetAllQueueItemshere lines up with the new collections-based state layout, and panicking on unexpected errors during genesis export is reasonable since the module should never be in a partially valid state at this point. The construction of theGenesisStateviatypes.NewGenesisState(triggerID, queueStartIndex, triggers, queue)also keeps the export logic nicely centralized.x/trigger/keeper/event_listener_test.go (1)
168-170: RemoveEventListener test now correctly asserts on errorsCapturing the error and asserting
NoErrorbefore checkingsuccessgives good coverage of the new(bool, error)signature and will catch regressions in the keeper implementation.x/trigger/keeper/query_server.go (1)
46-61: Collection-based pagination wiring looks correctUsing
query.CollectionPaginateoverk.TriggersMapand rebindingtrigger.Id = keyensures the response IDs align with the collection keys while leveraging collections-native pagination. The error is appropriately surfaced as an internal gRPC status.x/trigger/types/keys_test.go (2)
13-26: Good reuse ofGetEventNameBytesand newTripleaccessorsThe test now derives
eventHashviaGetEventNameBytesand checksK1/K2/K3on thecollections.Triple, which keeps the test aligned with the production hashing and key layout and avoids hand-rolled byte slicing.
29-34: Prefix and hash assertions correctly reflect new key layout
TestGetEventListenerPrefixstill validates that the first byte is the event-listener prefix and the following 32 bytes match the event hash, which is the key compatibility we care about after migrating to collections.x/trigger/types/keys.go (2)
58-70: Collections prefix migration preserves key layoutSwitching the various
*_Prefix/*Keyvars tocollections.NewPrefixand using.Bytes()in the helpers keeps the on-disk key bytes compatible with the previous scheme while making these prefixes usable with collections. The updated helpers (GetTriggerKey,GetEventListenerPrefix, queue keys) look consistent.Also applies to: 85-122
72-78:GetEventListenerKeytriple helper fits the collections-backed designHaving
GetEventListenerKeyreturn acollections.Triple[[]byte, uint64, uint64]aligns with theEventListenerskeyset type and avoids manual byte concatenation. The panic on emptyeventHashis acceptable here given callers now validate input before calling this helper and tests assert the panic message.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
x/trigger/keeper/event_listener.go (1)
153-161: Invalid event names still useErrEventNotFound; consider a dedicated validation error
getEventNameHashreturnstriggertypes.ErrEventNotFoundwhen the normalized event name is empty, which conflates invalid input with a genuinely missing listener; this can make it harder for callers to distinguish “buggy request” from “no listener registered”.If you revisit this area, introducing something like
ErrInvalidEventName(or reusing a general invalid-input error) for empty/whitespace names and reservingErrEventNotFoundfor true “not present in store” cases would make error handling clearer.
🧹 Nitpick comments (5)
x/trigger/simulation/decoder.go (1)
41-43: Validate whether the length check should apply to the key or the value.Here you guard on
len(kvA.Key) < 1+types.TriggerIDLength, but the actual decoding useskvA.Value/kvB.ValueviaGetTriggerIDFromBytes. IfTriggerIDLengthrefers to the encodeduint64length (8 bytes), it’s more typical that the value has that length while the key is just the prefix (length 1). In that common layout this condition would always fire (1 < 9), and you still wouldn’t protect against malformed values.Please double‑check the actual layout of the NextTriggerID entry:
- If the ID lives in the value (prefix‑only key), you probably want to validate
kvA.Value/kvB.Valueinstead, e.g.:- if len(kvA.Key) < 1+types.TriggerIDLength { - return fmt.Sprintf("invalid trigger key length: %d", len(kvA.Key)) - } + if len(kvA.Value) != types.TriggerIDLength || len(kvB.Value) != types.TriggerIDLength { + return fmt.Sprintf( + "invalid trigger ID value lengths: A=%d B=%d", + len(kvA.Value), len(kvB.Value), + ) + }
- If the ID is actually encoded into the key with layout
<prefix><id-bytes>, then you probably want to slice the key forGetTriggerIDFromBytesinstead of the value.Either way, worth confirming to avoid silently misclassifying valid entries or leaving the real malformed data path unchecked.
x/trigger/keeper/keeper_test.go (1)
34-34: Clarify the purpose of the unusedkeeperfield.The
keeperfield is added but doesn't appear to be initialized inSetupTestor used in the test methods (which uses.app.TriggerKeeperinstead). Is this field intended for future use, or should it be removed to avoid confusion?x/trigger/keeper/genesis.go (2)
13-38: Genesis export logic and queue-length check look correct; consider hard‑failing on queue metadata errorsThe switch to using collection helpers with panics on error for trigger ID, queue start, and trigger/queue reads is appropriate for genesis export and should prevent silent partial exports. The queue-length vs actual-queue warning is useful, but if
GetQueueLengthever returns a real error (e.g. missing metadata), treating that as fatal (panic) instead of silently skipping the check would make corruption more obvious during simulations and debugging.
49-79: InitGenesis ordering fixes the queue‑metadata issue; panic messages could be slightly improvedInitializing
QueueLengthto 0 before enqueuing, then usingEnqueueplusSetTrigger/SetEventListener, gives a clean, deterministic reconstruction of state and should address the earlier import–export mismatch. As a small polish, you could switch thepanic(fmt.Sprintf(...))calls topanic(fmt.Errorf("context: %w", err))(or log + panic) for more structured error chaining and consistency with other keepers.x/trigger/keeper/event_listener.go (1)
17-35: Event listener set/remove/has logic looks correct; consider wrapping propagated errors for contextThe switch to hashed keys via
getEventNameHash+GetEventListenerKeyand using theEventListenersKeySet is sound, and the boolean return fromRemoveEventListenernicely distinguishes “not found” from real errors. To aid debugging, you might mirror theSetEventListenerstyle and wrap errors inRemoveEventListener,HasEventListener, and theHasEventListenercallsites (e.g. “failed checking/removing event listener for trigger %d: %w”) instead of returning raw errors.Based on learnings, wrapping errors with context is preferred in this codebase.
Also applies to: 38-64, 123-131
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
x/trigger/keeper/event_listener.go(2 hunks)x/trigger/keeper/genesis.go(3 hunks)x/trigger/keeper/keeper_test.go(2 hunks)x/trigger/simulation/decoder.go(1 hunks)x/trigger/simulation/genesis.go(1 hunks)x/trigger/types/keys.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/trigger/simulation/genesis.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/trigger/keeper/keeper_test.gox/trigger/keeper/genesis.go
📚 Learning: 2025-08-15T22:52:24.461Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keeper.go:74-173
Timestamp: 2025-08-15T22:52:24.461Z
Learning: In the ledger keeper NewKeeper function in x/ledger/keeper/keeper.go, SpicyLemon prefers to allow bankKeeper and registryKeeper dependencies to be nil rather than adding validation that would panic on nil values. The design intentionally supports nil keepers for flexibility.
Applied to files:
x/trigger/keeper/keeper_test.gox/trigger/keeper/genesis.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: In Cosmos SDK v0.50.x, the bank keeper interface methods use `context.Context` rather than `sdk.Context`. The BankKeeper interface should use `context.Context` for methods like SendCoins, HasBalance, GetBalance, etc.
Applied to files:
x/trigger/keeper/keeper_test.go
📚 Learning: 2025-08-15T20:39:34.095Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/util.go:68-102
Timestamp: 2025-08-15T20:39:34.095Z
Learning: In the Provenance codebase, functions with "Must" prefix (like MustGetMarkerAddress) can panic and should only be used when you know they won't panic. Instead, use the twin function that returns an error (like GetMarkerAddress) and handle the error properly.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `GetLockedCoins` function in `x/hold/keeper/locked_coins.go` must conform to the `banktypes.GetLockedCoinsFn` type, which does not allow returning an error. In cases where `k.GetHoldCoins` encounters an error, using `panic` is considered appropriate due to the severity of such errors.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-10T23:13:56.722Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: x/ibchooks/types/params.go:22-26
Timestamp: 2024-10-10T23:13:56.722Z
Learning: In the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `Validate` method without modifying the error messages, even if they may lack additional context.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:106-109
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In the `AddScopeDataAccess` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/trigger/types/keys.go
🧬 Code graph analysis (5)
x/trigger/keeper/keeper_test.go (2)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/keys.go (2)
ModuleName(14-14)GetTriggerKey(86-93)
x/trigger/simulation/decoder.go (1)
x/trigger/types/keys.go (1)
TriggerIDLength(23-23)
x/trigger/keeper/event_listener.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/keys.go (1)
GetEventListenerKey(73-78)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (2)
ErrEventNotFound(9-9)ErrTriggerNotFound(8-8)
x/trigger/keeper/genesis.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/genesis.pb.go (3)
GenesisState(27-39)GenesisState(43-43)GenesisState(44-46)x/trigger/simulation/genesis.go (3)
QueueStart(24-24)QueuedTriggers(28-28)Triggers(27-27)
x/trigger/types/keys.go (2)
x/trigger/simulation/genesis.go (1)
TriggerID(23-23)x/trigger/types/trigger.go (1)
TriggerID(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: test-race (02)
- GitHub Check: test-race (04)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: tests (04)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: test-race (03)
- GitHub Check: test-race (01)
- GitHub Check: test-race (00)
- GitHub Check: build-linux
- GitHub Check: Build OSX
- GitHub Check: Build Linux
- GitHub Check: Analyze (go)
- GitHub Check: docker
- GitHub Check: ghcr-publish
- GitHub Check: heighliner-docker
- GitHub Check: golangci-lint
🔇 Additional comments (3)
x/trigger/keeper/keeper_test.go (1)
103-119: Encoding compatibility verified; extend backward compatibility coverage for EventListeners.Collections.Uint64Key encodes uint64 as 8-byte big-endian integer, matching the old format used in the test. The backward compatibility test for
TriggersMapis sound and correctly validates storage layer compatibility.However, expand coverage to include:
EventListeners: Uses
types.EventListenerKeyCodec()(custom codec) rather than standardUint64Key. This requires explicit backward compatibility testing since it has a different encoding scheme than the standard uint64 codecs.Queue: While it uses
collections.Uint64Key(same encoding asTriggersMap), it's worth adding a backward compatibility test for completeness and confidence.Genesis logic: This test validates that collections can read old-format data, but the reported import-export simulation failure may stem from issues in genesis export/import logic itself (e.g., in
ExportGenesisorInitGenesis), which this test doesn't directly verify.x/trigger/types/keys.go (1)
57-70: Prefix migration tocollections.Prefixand.Bytes()usage look consistentThe move to
collections.NewPrefixfor all trigger/queue prefixes and the corresponding use of.Bytes()in the key helpers keeps the on-disk key layout stable while making the collections integration type-safe. This should satisfy the earlier concern aboutcollections.Prefixvs[]byteand remove compilation issues in the helpers.Also applies to: 80-83, 90-122
x/trigger/keeper/event_listener.go (1)
66-121: Iteration and counting over hashed event listeners are implemented correctlyUsing
getEventNameHashpluscollections.NewPrefixedTripleRangeto scope iteration, then resolving to full triggers viaGetTrigger, gives a clean abstraction over the underlying key structure. Skipping entries whose trigger has been deleted while continuing iteration is a reasonable resilience tradeoff and makesIterateEventListenersandGetEventListenerCountsafe to use even with minor store inconsistencies.Also applies to: 143-151
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
x/trigger/keeper/queue.go (2)
23-44: Queue metadata bug:QueuePeekbreaks on fresh store—use helper methodThis is the same critical issue flagged in the previous review. On a freshly initialized queue,
QueueStartIndexis never set byEnqueue, soQueueStartIndex.Get(ctx)at line 29 returnscollections.ErrNotFound, causingQueuePeekto returnnileven when the queue has items.The helper method
getQueueStartIndex(lines 198–207) already exists and correctly defaults to 0 onErrNotFound. Replace the direct.Get()call with the helper.This bug likely contributes to the import-export simulation failure noted in the PR objectives.
72-101: Queue metadata bug:Dequeuepanics on fresh store—use helper methodsThis is the same critical issue flagged in the previous review. On a freshly initialized queue, both
QueueStartIndex.Get(ctx)(line 77) andQueueLength.Get(ctx)(line 82) returncollections.ErrNotFound, causing panics instead of defaulting to 0.Helper methods
getQueueStartIndex(lines 198–207) andGetQueueLength(lines 215–224) already exist and correctly default to 0 onErrNotFound. Replace the direct.Get()calls with these helpers.This bug likely contributes to the import-export simulation failure noted in the PR objectives.
x/trigger/keeper/event_listener.go (1)
154-161: Use a more appropriate error for invalid event namesThis issue was flagged in the previous review.
getEventNameHashreturnstriggertypes.ErrEventNotFoundwhen the normalized event name is empty (line 157). This conflates "invalid input" (validation failure) with "listener not found in store" (query failure), making debugging harder.Consider introducing
ErrInvalidEventNameor reusing an existing validation error for empty/invalid event names, reservingErrEventNotFoundfor cases where a listener genuinely doesn't exist in the store.
🧹 Nitpick comments (4)
x/trigger/keeper/event_listener.go (2)
38-64: Consider wrapping errors inRemoveEventListenerBased on learnings, error wrapping helps reconstruct what went wrong from error messages. While lines 20, 25, and 31 in
SetEventListenerwrap errors with context (trigger ID, operation),RemoveEventListenerreturns bare errors fromGetTriggerEventI(line 41),getEventNameHash(line 46), andEventListeners.Remove(line 60).For consistency and debuggability, consider wrapping these errors with context like
fmt.Errorf("failed to remove event listener for trigger %d: %w", trigger.Id, err).Based on learnings, wrapping errors provides better context for debugging.
133-141:RemoveEventListenerForTriggerIDdiscards removal statusThe function calls
RemoveEventListener(line 139) which returns(bool, error)but only propagates the error, discarding the boolean that indicates whether a listener was actually removed. If callers need to distinguish "removed successfully" from "listener didn't exist", this information is lost.If the boolean return is not needed by callers, this is fine. Otherwise, consider either returning
(bool, error)from this function or documenting that it treats "not found" as success.x/trigger/keeper/trigger.go (2)
117-119: Confusing ID management insetTriggerIDThe function stores
triggerID-1with the comment "Store as current, so next is triggerID". This off-by-one management is error-prone and conflicts withNewTriggerWithIDwhich storesnextIDdirectly after incrementing (line 103).For clarity, consider storing the "next ID to use" directly in
NextTriggerIDand removing the -1 offset, updating getters/setters accordingly. This makes the semantics clearer: "NextTriggerID contains the next ID that will be assigned".
78-88:HasTriggerimplementation is inefficientThe function calls
TriggersMap.Get(line 80) to check existence, which retrieves and deserializes the entire trigger. Collections typically provide a cheaperHasmethod that only checks for key existence.Consider using
k.TriggersMap.Has(ctx, id)directly instead ofGetfollowed by error checking. This avoids unnecessary deserialization and improves performance, especially if this method is called frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
x/trigger/keeper/event_listener.go(2 hunks)x/trigger/keeper/queue.go(2 hunks)x/trigger/keeper/trigger.go(3 hunks)x/trigger/simulation/decoder.go(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `GetLockedCoins` function in `x/hold/keeper/locked_coins.go` must conform to the `banktypes.GetLockedCoinsFn` type, which does not allow returning an error. In cases where `k.GetHoldCoins` encounters an error, using `panic` is considered appropriate due to the severity of such errors.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2025-08-15T20:39:34.095Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/util.go:68-102
Timestamp: 2025-08-15T20:39:34.095Z
Learning: In the Provenance codebase, functions with "Must" prefix (like MustGetMarkerAddress) can panic and should only be used when you know they won't panic. Instead, use the twin function that returns an error (like GetMarkerAddress) and handle the error properly.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-10T23:13:56.722Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: x/ibchooks/types/params.go:22-26
Timestamp: 2024-10-10T23:13:56.722Z
Learning: In the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `Validate` method without modifying the error messages, even if they may lack additional context.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:106-109
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In the `AddScopeDataAccess` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/trigger/keeper/queue.go
🧬 Code graph analysis (4)
x/trigger/simulation/decoder.go (2)
x/trigger/types/keys.go (9)
NextTriggerIDKey(65-65)GetTriggerIDFromBytes(137-142)QueueStartIndexKey(67-67)GetQueueIndexFromBytes(125-127)QueueLengthKey(69-69)TriggerIDLength(23-23)EventListenerKeyPrefix(61-61)QueueKeyPrefix(63-63)QueueIndexLength(24-24)x/trigger/types/trigger.pb.go (6)
Trigger(33-42)Trigger(46-46)Trigger(47-49)QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)
x/trigger/keeper/event_listener.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/keys.go (1)
GetEventListenerKey(73-78)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (2)
ErrEventNotFound(9-9)ErrTriggerNotFound(8-8)
x/trigger/keeper/queue.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)x/trigger/types/errors.go (1)
ErrQueueIndexNotFound(10-10)
x/trigger/keeper/trigger.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: go-test-single-db (nondeterminism, ubuntu-latest)
- GitHub Check: go-test-multi-db (benchmark, goleveldb, ubuntu-latest)
- GitHub Check: go-test-single-db (benchmark-invariants, ubuntu-latest)
- GitHub Check: runsim (multi-seed-short, goleveldb, ubuntu-latest)
- GitHub Check: go-test-multi-db (simple, goleveldb, ubuntu-latest)
- GitHub Check: test-race (04)
- GitHub Check: test-race (01)
- GitHub Check: tests (03)
- GitHub Check: test-race (03)
- GitHub Check: test-race (02)
- GitHub Check: test-race (00)
- GitHub Check: tests (02)
- GitHub Check: tests (04)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: heighliner-docker
- GitHub Check: docker
- GitHub Check: ghcr-publish
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
x/trigger/types/keys.go (1)
72-78: Update panic message to accurately describe theeventHashparameter.The panic message at line 75 still refers to "invalid event name: " when the function parameter is
eventHash []byte. This was flagged in a previous review but remains unchanged.Apply this diff to fix the message:
func GetEventListenerKey(eventHash []byte, order uint64, id TriggerID) collections.Triple[[]byte, uint64, uint64] { if len(eventHash) == 0 { - panic("invalid event name: ") + panic("invalid event hash: empty") } return collections.Join3(eventHash, order, id) }x/trigger/simulation/decoder.go (3)
34-37: Replace unsafe slicing withbytes.HasPrefixto prevent panic.Line 34 uses
kvA.Key[:1]which will panic ifkvA.Keyis empty. The length validation at lines 35-37 executes only after the case matches, so it cannot prevent the panic. This issue was flagged in a previous review but remains unresolved.Apply this diff:
- case bytes.Equal(kvA.Key[:1], types.TriggerKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.TriggerKeyPrefix.Bytes()): if len(kvA.Key) < 1+types.TriggerIDLength { return fmt.Sprintf("invalid Trigger key length: %d", len(kvA.Key)) }
58-67: Replace unsafe slicing withbytes.HasPrefixto prevent panic.Line 58 uses
kvA.Key[:1]which will panic ifkvA.Keyis empty. The length validation at lines 62-67 executes only after the case matches, so it cannot prevent the panic. This issue was flagged in a previous review but remains unresolved.Apply this diff:
- case bytes.Equal(kvA.Key[:1], types.EventListenerKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.EventListenerKeyPrefix.Bytes()): triggerIDA := extractTriggerIDFromEventListenerKey(kvA.Key) triggerIDB := extractTriggerIDFromEventListenerKey(kvB.Key)
71-74: Replace unsafe slicing withbytes.HasPrefixto prevent panic.Line 71 uses
kvA.Key[:1]which will panic ifkvA.Keyis empty. The length validation at lines 72-74 executes only after the case matches, so it cannot prevent the panic. This issue was flagged in a previous review but remains unresolved.Apply this diff:
- case bytes.Equal(kvA.Key[:1], types.QueueKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.QueueKeyPrefix.Bytes()): if len(kvA.Key) < 1+types.QueueIndexLength { return fmt.Sprintf("invalid Queue key length: %d", len(kvA.Key)) }x/trigger/keeper/trigger.go (1)
20-32:RemoveTriggersilently swallows storage errors, preventing diagnosis of import-export failures.When
TriggersMap.Has(lines 21-23) orTriggersMap.Remove(lines 26-27) fail, the function returnsfalsewithout propagating the error or logging it. Callers cannot distinguish "trigger not found" from "storage operation failed".Given that:
- Other methods in this file return errors (
SetTrigger,GetTrigger,IterateTriggers)- The PR objectives mention an import-export simulation failure with differing state
- Silent error swallowing makes storage issues impossible to diagnose
Change the signature to return
(bool, error)and propagate errors, or at minimum log errors before returningfalse.Apply this diff to propagate errors:
-func (k Keeper) RemoveTrigger(ctx sdk.Context, id types.TriggerID) bool { +func (k Keeper) RemoveTrigger(ctx sdk.Context, id types.TriggerID) (bool, error) { exists, err := k.TriggersMap.Has(ctx, id) if err != nil { - return false + return false, err } if exists { if err := k.TriggersMap.Remove(ctx, id); err != nil { - return false + return false, err } - return true + return true, nil } - return false + return false, nil }Note: This change requires updating all callers to handle the new
(bool, error)return signature.
🧹 Nitpick comments (2)
x/trigger/keeper/genesis.go (2)
4-4: Preferfmt.Errorf(with%w) overfmt.Sprintfin panic paths for richer error contextRight now the InitGenesis error paths all do
panic(fmt.Sprintf("...: %v", err)), which turns the original error into a plain string and loses its type/causes. Usingfmt.Errorfwith%wpreserves the error value and wraps context, which is generally more useful when inspecting panics.Consider:
-import ( - "fmt" - - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/provenance-io/provenance/x/trigger/types" -) +import ( + "fmt" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/provenance-io/provenance/x/trigger/types" +) @@ - if err := k.setTriggerID(ctx, data.TriggerId); err != nil { - panic(fmt.Sprintf("Failed to set trigger ID: %v", err)) - } + if err := k.setTriggerID(ctx, data.TriggerId); err != nil { + panic(fmt.Errorf("failed to set trigger ID: %w", err)) + } @@ - if err := k.setQueueStartIndex(ctx, data.QueueStart); err != nil { - panic(fmt.Sprintf("Failed to set queue start index: %v", err)) - } + if err := k.setQueueStartIndex(ctx, data.QueueStart); err != nil { + panic(fmt.Errorf("failed to set queue start index: %w", err)) + } @@ - if err := k.setQueueLength(ctx, queueLength); err != nil { - panic(fmt.Sprintf("Failed to set queue length: %v", err)) - } + if err := k.setQueueLength(ctx, queueLength); err != nil { + panic(fmt.Errorf("failed to set queue length: %w", err)) + } @@ - if err := k.Enqueue(ctx, queuedTrigger); err != nil { - panic(fmt.Sprintf("Failed to enqueue trigger: %v", err)) - } + if err := k.Enqueue(ctx, queuedTrigger); err != nil { + panic(fmt.Errorf("failed to enqueue trigger from genesis: %w", err)) + } @@ - if err := k.SetTrigger(ctx, trigger); err != nil { - panic(fmt.Sprintf("Failed to set trigger %d: %v", trigger.Id, err)) - } - if err := k.SetEventListener(ctx, trigger); err != nil { - panic(fmt.Sprintf("Failed to set event listener for trigger %d: %v", trigger.Id, err)) - } + if err := k.SetTrigger(ctx, trigger); err != nil { + panic(fmt.Errorf("failed to set trigger %d from genesis: %w", trigger.Id, err)) + } + if err := k.SetEventListener(ctx, trigger); err != nil { + panic(fmt.Errorf("failed to set event listener for trigger %d from genesis: %w", trigger.Id, err)) + }You could optionally apply the same pattern in
ExportGenesisfor thegetTriggerID/getQueueStartIndexpanics for consistency.Also applies to: 49-52, 55-57, 59-62, 65-67, 71-76
59-62: Double-checkQueueLengthinitialization vsEnqueuebehavior to avoid possible double-countingHere you set:
queueLength := uint64(len(data.QueuedTriggers)) if err := k.setQueueLength(ctx, queueLength); err != nil { ... } for _, queuedTrigger := range data.QueuedTriggers { if err := k.Enqueue(ctx, queuedTrigger); err != nil { ... } }If
EnqueueusesQueueLengthas the current size and increments it internally (which is a common pattern with queue metadata), pre-seedingQueueLengthtolen(data.QueuedTriggers)before the loop could leave the stored length at2 * len(QueuedTriggers)after InitGenesis. IfEnqueuenow ignoresQueueLengthor no longer updates it, this is fine—but then the pre-set tolen(...)may be unnecessary.Please confirm the current
Enqueue/Dequeueimplementation:
- If
EnqueueincrementsQueueLength, initialize it to0(or letEnqueuehandle missing as zero) before populating the queue; or- If
Enqueueno longer usesQueueLength, consider either setting it touint64(len(data.QueuedTriggers))after the loop or documenting that it’s a compatibility-only value.Also applies to: 65-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
x/trigger/keeper/genesis.go(3 hunks)x/trigger/keeper/trigger.go(3 hunks)x/trigger/simulation/decoder.go(2 hunks)x/trigger/types/keys.go(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils_test.go:478-493
Timestamp: 2024-10-08T18:12:51.935Z
Learning: In Go tests, modifying byte slices directly using arithmetic operations like addition and subtraction is acceptable, even if it causes overflow, as overflow on `uint8` just wraps around. Avoid flagging such usage, and avoid suggesting bitwise operations which can be harder to read and understand.
Applied to files:
x/trigger/simulation/decoder.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/trigger/simulation/decoder.gox/trigger/types/keys.go
📚 Learning: 2025-08-15T22:52:24.461Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keeper.go:74-173
Timestamp: 2025-08-15T22:52:24.461Z
Learning: In the ledger keeper NewKeeper function in x/ledger/keeper/keeper.go, SpicyLemon prefers to allow bankKeeper and registryKeeper dependencies to be nil rather than adding validation that would panic on nil values. The design intentionally supports nil keepers for flexibility.
Applied to files:
x/trigger/keeper/genesis.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/trigger/keeper/genesis.go
📚 Learning: 2024-10-10T23:13:56.722Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: x/ibchooks/types/params.go:22-26
Timestamp: 2024-10-10T23:13:56.722Z
Learning: In the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `Validate` method without modifying the error messages, even if they may lack additional context.
Applied to files:
x/trigger/types/keys.go
📚 Learning: 2025-08-15T20:39:34.095Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/util.go:68-102
Timestamp: 2025-08-15T20:39:34.095Z
Learning: In the Provenance codebase, functions with "Must" prefix (like MustGetMarkerAddress) can panic and should only be used when you know they won't panic. Instead, use the twin function that returns an error (like GetMarkerAddress) and handle the error properly.
Applied to files:
x/trigger/types/keys.go
🧬 Code graph analysis (4)
x/trigger/simulation/decoder.go (2)
x/trigger/types/keys.go (9)
NextTriggerIDKey(65-65)QueueStartIndexKey(67-67)QueueLengthKey(69-69)TriggerKeyPrefix(59-59)TriggerIDLength(23-23)EventListenerKeyPrefix(61-61)QueueKeyPrefix(63-63)QueueIndexLength(24-24)ModuleName(14-14)x/trigger/types/trigger.pb.go (6)
Trigger(33-42)Trigger(46-46)Trigger(47-49)QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)
x/trigger/keeper/genesis.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/genesis.pb.go (3)
GenesisState(27-39)GenesisState(43-43)GenesisState(44-46)x/trigger/simulation/genesis.go (3)
QueueStart(24-24)QueuedTriggers(28-28)Triggers(27-27)
x/trigger/keeper/trigger.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/simulation/genesis.go (1)
TriggerID(23-23)x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)
x/trigger/types/keys.go (2)
x/trigger/simulation/genesis.go (1)
TriggerID(23-23)x/trigger/types/trigger.go (1)
TriggerID(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: runsim (after-import, goleveldb, ubuntu-latest)
- GitHub Check: runsim (import-export, goleveldb, ubuntu-latest)
- GitHub Check: go-test-multi-db (benchmark, goleveldb, ubuntu-latest)
- GitHub Check: runsim (multi-seed-short, goleveldb, ubuntu-latest)
- GitHub Check: go-test-single-db (benchmark-invariants, ubuntu-latest)
- GitHub Check: go-test-single-db (nondeterminism, ubuntu-latest)
- GitHub Check: go-test-multi-db (simple, goleveldb, ubuntu-latest)
- GitHub Check: tests (02)
- GitHub Check: tests (04)
- GitHub Check: test-race (04)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: test-race (02)
- GitHub Check: tests (01)
- GitHub Check: test-race (01)
- GitHub Check: test-race (03)
- GitHub Check: test-race (00)
- GitHub Check: Build OSX
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: golangci-lint
- GitHub Check: heighliner-docker
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
x/trigger/keeper/genesis.go (1)
13-21: ExportGenesis: stricter metadata handling and queue-length sanity check look goodPanicking on
getTriggerID/getQueueStartIndexerrors and validatingQueueLengthagainst the actual queue size avoids silently exporting an inconsistent genesis and adds a useful warning signal without changing behavior.Also applies to: 33-38
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
x/trigger/keeper/trigger.go (1)
20-31:RemoveTriggerstill collapses storage errors intofalse(same concern as earlier review)
TriggersMap.HasandTriggersMap.Removeerrors currently causeRemoveTriggerto returnfalse, which is indistinguishable from “trigger didn’t exist” and hides real storage failures—this was already flagged in a prior review and remains unresolved.Consider changing the signature to
(bool, error)and returningfalse, erron any storage error, withtrue, nilwhen a trigger is successfully removed andfalse, nilwhen it wasn’t present. At minimum, log the errors so they’re visible during debugging (especially for the import‑export sim).
🧹 Nitpick comments (1)
x/trigger/keeper/trigger.go (1)
78-88:HasTriggershould preferHasoverGetand align ontypes.TriggerIDFunctionally this works, but you can simplify and avoid decoding values by using
TriggersMap.Has:-func (k Keeper) HasTrigger(ctx sdk.Context, id uint64) (bool, error) { - _, err := k.TriggersMap.Get(ctx, id) - if errors.Is(err, collections.ErrNotFound) { - return false, nil - } - if err != nil { - return false, err - } - return true, nil -} +func (k Keeper) HasTrigger(ctx sdk.Context, id types.TriggerID) (bool, error) { + return k.TriggersMap.Has(ctx, id) +}This is a small consistency and efficiency improvement, and it keeps the public API using
types.TriggerIDlike the other methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/trigger/keeper/trigger.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
🧬 Code graph analysis (1)
x/trigger/keeper/trigger.go (5)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/simulation/genesis.go (1)
TriggerID(23-23)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: test-race (03)
- GitHub Check: test-race (01)
- GitHub Check: test-race (00)
- GitHub Check: test-race (04)
- GitHub Check: tests (01)
- GitHub Check: test-race (02)
- GitHub Check: tests (02)
- GitHub Check: tests (03)
- GitHub Check: tests (04)
- GitHub Check: tests (00)
- GitHub Check: build-linux
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: ghcr-publish
- GitHub Check: heighliner-docker
- GitHub Check: docker
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
🔇 Additional comments (3)
x/trigger/keeper/trigger.go (3)
3-17: SetTrigger collections usage looks correct; ensure all callers handle the new error returnSwitching
SetTriggertoTriggersMap.Setwith anerrorreturn is appropriate and consistent with the collections API. Please double‑check that all call sites now check/propagate this error so failures in the underlying store don’t get silently ignored.
35-44: GetTrigger’s error mapping toErrTriggerNotFoundis appropriateUsing
TriggersMap.Getand mappingcollections.ErrNotFoundtotypes.ErrTriggerNotFoundwhile propagating all other errors preserves the module‑specific error contract and surfaces storage issues cleanly. This looks correct and aligned with collections semantics.
47-66: IterateTriggers correctly uses the collections iteratorUsing
TriggersMap.Iterate(ctx, nil)withiterator.KeyValue()and deferringiterator.Close()is the expected pattern for a full‑range scan, and the callback error/stop handling looks correct. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
x/trigger/keeper/trigger_test.go (1)
115-116: Assert onNewTriggerWithIDerrors instead of discarding
NewTriggerWithIDreturns(types.Trigger, error), but the test usestrigger, _ := ..., so any internal error is silently ignored and the equality assertion may still run on a zero trigger.Capture and assert on the error before comparing:
- trigger, _ := s.app.TriggerKeeper.NewTriggerWithID(s.ctx, tc.expected.Owner, tc.expected.Event, tc.expected.Actions) - s.Equal(tc.expected, trigger, "should have correct trigger from NewTrigger") + trigger, err := s.app.TriggerKeeper.NewTriggerWithID(s.ctx, tc.expected.Owner, tc.expected.Event, tc.expected.Actions) + s.Require().NoError(err, "NewTriggerWithID should not return an error") + s.Equal(tc.expected, trigger, "should have correct trigger from NewTrigger")Using
Requirehere aligns with suite preferences for failing fast on setup errors. Based on learnings, SpicyLemon prefers this style.x/trigger/keeper/trigger.go (1)
20-32:RemoveTriggerstill swallows storage errors; prefer(bool, error)
RemoveTriggerreturnsfalseon any error fromTriggersMap.HasorTriggersMap.Remove, making it indistinguishable from "ID does not exist" and hiding underlying store issues (e.g., during import/export or I/O failures).Consider changing the signature and propagating errors:
-func (k Keeper) RemoveTrigger(ctx sdk.Context, id types.TriggerID) bool { - exists, err := k.TriggersMap.Has(ctx, id) - if err != nil { - return false - } - if exists { - if err := k.TriggersMap.Remove(ctx, id); err != nil { - return false - } - return true - } - return false -} +func (k Keeper) RemoveTrigger(ctx sdk.Context, id types.TriggerID) (bool, error) { + exists, err := k.TriggersMap.Has(ctx, id) + if err != nil { + return false, err + } + if !exists { + return false, nil + } + if err := k.TriggersMap.Remove(ctx, id); err != nil { + return false, err + } + return true, nil +}Callers (including tests) would then assert on both the boolean and the error. This keeps RemoveTrigger consistent with SetTrigger/GetTrigger in terms of error visibility.
🧹 Nitpick comments (2)
x/trigger/keeper/trigger.go (2)
78-88: UseHasandtypes.TriggerIDinHasTriggerfor clarity and efficiency
HasTriggercurrently does a fullGet/decode just to check existence and takes a rawuint64:func (k Keeper) HasTrigger(ctx sdk.Context, id uint64) (bool, error) { _, err := k.TriggersMap.Get(ctx, id) ... }Consider:
- Using
TriggersMap.Hasinstead ofGetto avoid unnecessary decoding.- Accepting
id types.TriggerIDfor consistency with other methods.For example:
func (k Keeper) HasTrigger(ctx sdk.Context, id types.TriggerID) (bool, error) { return k.TriggersMap.Has(ctx, id) }This keeps the method lightweight and aligned with the rest of the keeper API.
90-137: Implement recommended InitGenesis and round‑trip testing to ensure consistent NextTriggerID key presenceVerification confirms the concern:
ExportGenesiscallsgetTriggerID(ctx), which returns1without creating theNextTriggerIDkey when it's absent;InitGenesiscallssetTriggerID(ctx, data.TriggerId), which always writes the key. On a fresh chain that never allocated a trigger, exporting yieldsTriggerId: 1with no key in the store, but importing re‑creates the key—breaking the invariant that import-export should preserve store state byte-for-byte.The
Validate()function (x/trigger/types/genesis.go) preventsTriggerId: 0but does not guard against this key-presence inconsistency. No round-trip tests currently exist.Recommended:
- Ensure
InitGenesisalways callssetTriggerID(ctx, data.TriggerId)before processing triggers (it already does) to guarantee the key is present after import.- Add a keeper-level unit test:
- Create a state with empty triggers, export it, then immediately import it back
- Assert that
NextTriggerIDkey is present and matches the exported value both before and after the round-trip- Repeat with a non-empty trigger set to confirm ID consistency across the cycle
This will prevent subtle differences in key presence from silently breaking the import-export simulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/trigger/keeper/trigger.go(3 hunks)x/trigger/keeper/trigger_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 1993
File: x/marker/client/cli/cli_test.go:1090-1090
Timestamp: 2024-10-08T18:12:51.935Z
Learning: SpicyLemon prefers allowing tests to fail naturally if the setup is incorrect rather than explicitly checking and handling such errors within the test functions. They also recommend using `s.Require().NotNil(...)` instead of `s.T().Fatal(...)` for assertions in test functions.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2025-08-15T20:57:04.455Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/entries_test.go:26-31
Timestamp: 2025-08-15T20:57:04.455Z
Learning: SpicyLemon prefers string-based error assertions in tests using empty string to indicate "no error expected" and non-empty string for expected error messages. Use assertions.AssertErrorValue instead of errors.Is() comparisons because errors.Is() with s.Require().True() provides poor failure messages that only say "Should be true" without showing expected vs actual error content.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2025-08-15T20:01:55.078Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: app/upgrades_test.go:0-0
Timestamp: 2025-08-15T20:01:55.078Z
Learning: SpicyLemon prefers upgrade tests to fail when expected data is missing rather than skip, using s.Require().NoError() and s.Require().NotEmpty() to enforce hard requirements for upgrade data files in test scenarios.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2024-10-10T23:19:54.565Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades_test.go:610-610
Timestamp: 2024-10-10T23:19:54.565Z
Learning: In `app/upgrades_test.go`, we prefer to keep TODO comments (like `TODO[viridian]`) to handle code deletions manually, and do not want to implement automated processes for code removal, especially for unit tests.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2024-10-02T03:37:18.486Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server_test.go:1225-1228
Timestamp: 2024-10-02T03:37:18.486Z
Learning: When test cases are run using `s.T().Run` instead of `s.Run`, use `require` with `t` instead of `s.Require()`, to ensure that assertions apply to the correct test instance.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2024-10-02T03:42:47.070Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/query_server_test.go:940-945
Timestamp: 2024-10-02T03:42:47.070Z
Learning: In the `TestScopeQuery` function, the conditional checks following the equality assertion are designed to help identify differences when `tc.expResp` and `actResp` are not equal and both are not `nil`. If they are equal, or either is `nil`, the failure message from the equality assertion suffices, and further checks are not needed.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/account_data_test.go:11-11
Timestamp: 2024-10-08T18:12:51.935Z
Learning: Do not raise issues about the `keeper` package still being in use after import removal in test files when there are no unresolved references.
Applied to files:
x/trigger/keeper/trigger_test.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/trigger/keeper/trigger.go
🧬 Code graph analysis (2)
x/trigger/keeper/trigger_test.go (3)
x/trigger/keeper/keeper_test.go (1)
KeeperTestSuite(27-41)x/trigger/types/trigger.pb.go (3)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)x/trigger/types/tx.pb.go (3)
MsgDestroyTriggerRequest(145-150)MsgDestroyTriggerRequest(154-154)MsgDestroyTriggerRequest(155-157)
x/trigger/keeper/trigger.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: runsim (import-export, goleveldb, ubuntu-latest)
- GitHub Check: go-test-multi-db (simple, goleveldb, ubuntu-latest)
- GitHub Check: runsim (after-import, goleveldb, ubuntu-latest)
- GitHub Check: runsim (multi-seed-short, goleveldb, ubuntu-latest)
- GitHub Check: go-test-multi-db (benchmark, goleveldb, ubuntu-latest)
- GitHub Check: tests (04)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: tests (03)
- GitHub Check: test-race (03)
- GitHub Check: test-race (04)
- GitHub Check: test-race (00)
- GitHub Check: test-race (02)
- GitHub Check: test-race (01)
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: ghcr-publish
- GitHub Check: heighliner-docker
- GitHub Check: docker
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
x/trigger/keeper/trigger_test.go (1)
208-246: New collections round‑trip test looks good
TestTriggerCollectionsOperationsexercisesSetTrigger,GetTrigger,HasTrigger, andRemoveTriggerend‑to‑end against the collections-backed keeper and usesRequirefor critical checks, which is a solid addition for the new API.x/trigger/keeper/trigger.go (2)
14-17:SetTriggererror surface via collections is appropriateSwitching
SetTriggerto delegate toTriggersMap.Setand return anerroris the right direction for propagating storage failures to callers.
35-76: Collections-basedGetTrigger/IterateTriggers/GetAllTriggerslook correct
GetTriggercorrectly mapscollections.ErrNotFoundtotypes.ErrTriggerNotFoundwhile propagating other errors.IterateTriggersusesTriggersMap.Iterateand passes decoded values into the handler, closing the iterator viadefer.GetAllTriggerssimply accumulates viaIterateTriggers, which is straightforward.No issues spotted here.
nagarajdivine
force-pushed
the
feat/trigger-module-collections
branch
from
bc1877c to
fd735fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/trigger/keeper/query_server.go (1)
21-35: Returnstatus.Error(codes.NotFound, ...)directly for consistency in QueryServer error handling.The code mixes error handling patterns: line 23 uses
status.Error(codes.InvalidArgument, ...)with explicit gRPC codes, but line 30 returnstypes.ErrTriggerNotFounddirectly. For QueryServer methods, explicitly usestatus.Error(codes.NotFound, "trigger not found")to ensure consistent and predictable gRPC status codes across all error paths.
♻️ Duplicate comments (7)
x/trigger/simulation/decoder.go (1)
34-36: Fix unsafekvA.Key[:1]slicing (panic on empty key)Even with later length checks, the slice happens first and can panic if
kvA.Keyis empty. Usebytes.HasPrefix.- case bytes.Equal(kvA.Key[:1], types.TriggerKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.TriggerKeyPrefix.Bytes()): @@ - case bytes.Equal(kvA.Key[:1], types.EventListenerKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.EventListenerKeyPrefix.Bytes()): @@ - case bytes.Equal(kvA.Key[:1], types.QueueKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.QueueKeyPrefix.Bytes()):Also applies to: 58-59, 71-74
x/trigger/keeper/event_listener.go (2)
153-160: Don’t use ErrEventNotFound for invalid event nameEmpty/whitespace input is a caller bug, not “not found”. Return a distinct validation error (or add one in
x/trigger/types/errors.go) and wrap it here.
133-141: Fix misleading doc: function removes one listener, not “all”Update the comment to match behavior (and Go doc convention):
-// RemoveEventListenerForTriggerID removes all event listeners for a specific trigger +// RemoveEventListenerForTriggerID removes the event listener for the specified trigger.x/trigger/keeper/trigger.go (2)
20-32: RemoveTrigger still swallows storage errors (hard to debug failures)At minimum log; ideally return
(bool, error)like other methods.
113-137: Guard against TriggerID == 0 to keep ID semantics consistent
getTriggerIDtreats missing as1, butsetTriggerIDwill happily persist0. IfTriggerIdin genesis can be0,NewTriggerWithIDcan produceId: 0.Suggested patch:
func (k Keeper) setTriggerID(ctx sdk.Context, triggerID types.TriggerID) error { + if triggerID == 0 { + return errors.New("triggerID must be > 0") + } return k.NextTriggerID.Set(ctx, triggerID) }x/trigger/keeper/queue.go (1)
22-44: Critical: QueuePeek/Dequeue bypass “missing => 0” helpers; fresh queue can peek nil / dequeue panicUse
getQueueStartIndex+GetQueueLength, and initializeQueueStartIndexon first enqueue.func (k Keeper) QueuePeek(ctx sdk.Context) *types.QueuedTrigger { isEmpty := k.QueueIsEmpty(ctx) @@ - startIndex, err := k.QueueStartIndex.Get(ctx) - if err != nil { - if errors.Is(err, collections.ErrNotFound) { - return nil - } - return nil - } + startIndex, err := k.getQueueStartIndex(ctx) + if err != nil { + return nil + } @@ func (k Keeper) Enqueue(ctx sdk.Context, item types.QueuedTrigger) error { - startIndex, err := k.QueueStartIndex.Get(ctx) - if err != nil && !errors.Is(err, collections.ErrNotFound) { - return err - } - if errors.Is(err, collections.ErrNotFound) { - startIndex = 0 - } + startIndex, err := k.getQueueStartIndex(ctx) + if err != nil { + return err + } + // Ensure metadata exists once we have items. + if err := k.QueueStartIndex.Set(ctx, startIndex); err != nil { + return err + } @@ func (k Keeper) Dequeue(ctx sdk.Context) { @@ - startIndex, err := k.QueueStartIndex.Get(ctx) - if err != nil { - panic(err) - } - - length, err := k.QueueLength.Get(ctx) - if err != nil { - panic(err) - } + startIndex, err := k.getQueueStartIndex(ctx) + if err != nil { + panic(err) + } + length, err := k.GetQueueLength(ctx) + if err != nil { + panic(err) + }Also applies to: 47-69, 71-100
x/trigger/types/keys.go (1)
76-82: Fix panic text + stale comment: this function takeseventHash, not “event name”.
The panic message (Line 79) and the doc comment (Line 76) don’t reflect the parameter anymore.-// GetEventListenerKey converts an event name, order, and trigger ID into an event registry key format. +// GetEventListenerKey converts an event hash, order, and trigger ID into an event registry key format. func GetEventListenerKey(eventHash []byte, order uint64, id TriggerID) collections.Triple[[]byte, uint64, uint64] { if len(eventHash) == 0 { - panic("invalid event name: ") + panic("invalid event hash: empty") } return collections.Join3(eventHash, order, id) }
🧹 Nitpick comments (7)
x/trigger/types/codec.go (1)
47-111: The JSON methods are never called in the codebase; verify if this is a real concern before implementing.
EncodeJSONandDecodeJSONcurrently return errors and are never invoked anywhere. If these methods are only used as part of theEventListenerKeyCodecfor backward compatibility with the old KVStore key format, and generic collections tooling doesn't attempt to call JSON methods on all KeyCodecs, then implementing them may not be necessary. Consider either removing these methods if they're not required by theKeyCodecinterface, documenting why they're intentionally unimplemented, or confirming that generic debug/CLI/export helpers actually depend on them before implementing.x/trigger/simulation/decoder.go (1)
58-70: EventListener key decoding looks brittle (fixed offsets)
extractTriggerIDFromEventListenerKeyassumes a fixed layout (key[41:49]). With collections, the[]byteelement may be length-prefixed, so offsets may change. Consider decoding via the same codec used for the key (e.g.,types.EventListenerKeyCodec()), or at least parse the last 8 bytes for triggerID if the encoding guarantees that.Also applies to: 99-108
x/trigger/keeper/genesis.go (1)
49-77: Preferpanic(fmt.Errorf(...))overpanic(fmt.Sprintf(...))Panicking with a formatted string drops the wrapped error. This preserves
%wcontext:- panic(fmt.Sprintf("Failed to set trigger ID: %v", err)) + panic(fmt.Errorf("failed to set trigger ID: %w", err))x/trigger/keeper/trigger.go (1)
78-88: UseTriggersMap.Hasfor HasTrigger (avoid Get+decode)func (k Keeper) HasTrigger(ctx sdk.Context, id uint64) (bool, error) { - _, err := k.TriggersMap.Get(ctx, id) - if errors.Is(err, collections.ErrNotFound) { - return false, nil - } - if err != nil { - return false, err - } - return true, nil + return k.TriggersMap.Has(ctx, id) }x/trigger/types/keys.go (3)
57-70: Minor doc/name nit: “QueueLengthPrefix” comment doesn’t matchQueueLengthKey.
Line 68’s comment saysQueueLengthPrefixbut the identifier isQueueLengthKey, which is a bit confusing for future readers.
84-126: Avoidappend(prefix.Bytes(), ...)patterns ifBytes()can share backing storage.
IfPrefix.Bytes()returns a slice backed by shared storage,appendcan (in edge cases) mutate data you expect to be immutable. Safer to build into a fresh buffer forGetEventListenerPrefix,GetTriggerKey,GetQueueKey, etc.func GetEventListenerPrefix(eventHash []byte) []byte { - prefix := EventListenerKeyPrefix.Bytes() - return append(prefix, eventHash...) + prefix := EventListenerKeyPrefix.Bytes() + out := make([]byte, 0, len(prefix)+len(eventHash)) + out = append(out, prefix...) + out = append(out, eventHash...) + return out }
128-149: Double-check “return 0 on short slice” is a safe sentinel.
Returning0on malformed/truncated keys can silently collapse different invalid inputs into a valid-looking ID/index (especially if0is meaningful anywhere). If this is only for defensive decoding/logging paths, it’s fine; otherwise consider returning(val, ok)or an error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.changelog/unreleased/features/2499-trigger-module-collections.md(1 hunks)app/app.go(1 hunks)x/trigger/keeper/event_detector_test.go(1 hunks)x/trigger/keeper/event_listener.go(2 hunks)x/trigger/keeper/event_listener_test.go(1 hunks)x/trigger/keeper/genesis.go(3 hunks)x/trigger/keeper/keeper.go(2 hunks)x/trigger/keeper/keeper_test.go(2 hunks)x/trigger/keeper/migrations_v2.go(1 hunks)x/trigger/keeper/msg_server.go(3 hunks)x/trigger/keeper/query_server.go(3 hunks)x/trigger/keeper/queue.go(2 hunks)x/trigger/keeper/queue_test.go(1 hunks)x/trigger/keeper/trigger.go(3 hunks)x/trigger/keeper/trigger_registry.go(1 hunks)x/trigger/keeper/trigger_test.go(2 hunks)x/trigger/simulation/decoder.go(2 hunks)x/trigger/simulation/genesis.go(1 hunks)x/trigger/types/codec.go(2 hunks)x/trigger/types/keys.go(3 hunks)x/trigger/types/keys_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/trigger/simulation/genesis.go
🚧 Files skipped from review as they are similar to previous changes (5)
- x/trigger/keeper/msg_server.go
- .changelog/unreleased/features/2499-trigger-module-collections.md
- x/trigger/keeper/trigger_registry.go
- x/trigger/keeper/trigger_test.go
- x/trigger/keeper/queue_test.go
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
📚 Learning: 2025-08-15T20:34:01.110Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/codec.go:9-10
Timestamp: 2025-08-15T20:34:01.110Z
Learning: In Go Cosmos SDK projects, sdk.Msg interface embeds proto.Message interface, making it valid to copy elements from []sdk.Msg to []proto.Message even though the slice types are different. The pattern of declaring AllRequestMsgs as []sdk.Msg for type assertions while copying to []proto.Message for RegisterImplementations is a common and correct pattern.
Applied to files:
x/trigger/types/codec.go
📚 Learning: 2025-08-15T20:34:01.110Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/codec.go:9-10
Timestamp: 2025-08-15T20:34:01.110Z
Learning: The pattern of declaring AllRequestMsgs as []sdk.Msg and then copying to []proto.Message for RegisterImplementations is a standard, correct pattern used throughout the Provenance codebase. This works because sdk.Msg interface embeds proto.Message interface, making the copy operation valid. This pattern appears consistently across 15+ modules and compiles without errors.
Applied to files:
x/trigger/types/codec.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 1993
File: x/marker/client/cli/cli_test.go:1090-1090
Timestamp: 2024-06-10T19:25:28.209Z
Learning: SpicyLemon prefers allowing tests to fail naturally if the setup is incorrect rather than explicitly checking and handling such errors within the test functions. They also recommend using `s.Require().NotNil(...)` instead of `s.T().Fatal(...)` for assertions in test functions.
Applied to files:
x/trigger/keeper/event_listener_test.gox/trigger/keeper/event_detector_test.go
📚 Learning: 2024-10-10T23:19:54.565Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades_test.go:610-610
Timestamp: 2024-10-10T23:19:54.565Z
Learning: In `app/upgrades_test.go`, we prefer to keep TODO comments (like `TODO[viridian]`) to handle code deletions manually, and do not want to implement automated processes for code removal, especially for unit tests.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2025-08-15T20:57:04.455Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/entries_test.go:26-31
Timestamp: 2025-08-15T20:57:04.455Z
Learning: SpicyLemon prefers string-based error assertions in tests using empty string to indicate "no error expected" and non-empty string for expected error messages. Use assertions.AssertErrorValue instead of errors.Is() comparisons because errors.Is() with s.Require().True() provides poor failure messages that only say "Should be true" without showing expected vs actual error content.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2025-08-15T22:52:24.461Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keeper.go:74-173
Timestamp: 2025-08-15T22:52:24.461Z
Learning: In the ledger keeper NewKeeper function in x/ledger/keeper/keeper.go, SpicyLemon prefers to allow bankKeeper and registryKeeper dependencies to be nil rather than adding validation that would panic on nil values. The design intentionally supports nil keepers for flexibility.
Applied to files:
x/trigger/keeper/genesis.gox/trigger/keeper/keeper_test.goapp/app.gox/trigger/keeper/keeper.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/trigger/keeper/genesis.gox/trigger/keeper/keeper_test.goapp/app.gox/trigger/keeper/trigger.gox/trigger/keeper/queue.gox/trigger/keeper/keeper.go
📚 Learning: 2024-10-02T18:03:10.008Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils_test.go:478-493
Timestamp: 2024-10-02T18:03:10.008Z
Learning: In Go tests, modifying byte slices directly using arithmetic operations like addition and subtraction is acceptable, even if it causes overflow, as overflow on `uint8` just wraps around. Avoid flagging such usage, and avoid suggesting bitwise operations which can be harder to read and understand.
Applied to files:
x/trigger/simulation/decoder.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/trigger/simulation/decoder.gox/trigger/types/keys.go
📚 Learning: 2024-10-02T03:06:21.997Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-02T03:06:21.997Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/trigger/types/keys_test.go
📚 Learning: 2024-10-02T02:06:21.664Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:57-80
Timestamp: 2024-10-02T02:06:21.664Z
Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.
Applied to files:
x/trigger/keeper/migrations_v2.gox/trigger/keeper/keeper.go
📚 Learning: 2025-08-15T20:39:34.095Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/util.go:68-102
Timestamp: 2025-08-15T20:39:34.095Z
Learning: In the Provenance codebase, functions with "Must" prefix (like MustGetMarkerAddress) can panic and should only be used when you know they won't panic. Instead, use the twin function that returns an error (like GetMarkerAddress) and handle the error properly.
Applied to files:
x/trigger/keeper/event_listener.gox/trigger/types/keys.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `GetLockedCoins` function in `x/hold/keeper/locked_coins.go` must conform to the `banktypes.GetLockedCoinsFn` type, which does not allow returning an error. In cases where `k.GetHoldCoins` encounters an error, using `panic` is considered appropriate due to the severity of such errors.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-10T23:13:56.722Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: x/ibchooks/types/params.go:22-26
Timestamp: 2024-10-10T23:13:56.722Z
Learning: In the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `Validate` method without modifying the error messages, even if they may lack additional context.
Applied to files:
x/trigger/keeper/event_listener.gox/trigger/types/keys.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:106-109
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In the `AddScopeDataAccess` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: In Cosmos SDK v0.50.x, the bank keeper interface methods use `context.Context` rather than `sdk.Context`. The BankKeeper interface should use `context.Context` for methods like SendCoins, HasBalance, GetBalance, etc.
Applied to files:
x/trigger/keeper/keeper_test.go
📚 Learning: 2024-10-02T01:43:01.360Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/marker/types/send_restrictions.go:11-11
Timestamp: 2024-10-02T01:43:01.360Z
Learning: In `x/marker/types/send_restrictions.go`, the `transferAgentKey` is only used for storing data in ephemeral contexts, so changing its value does not impact backward compatibility.
Applied to files:
app/app.go
📚 Learning: 2025-08-15T20:59:51.949Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keys.go:3-12
Timestamp: 2025-08-15T20:59:51.949Z
Learning: In Cosmos SDK, key prefixes only need to be unique within a given module's store, not across the entire application. Each module operates within its own isolated key-value store namespace, so the same byte prefixes can be safely reused across different modules without causing collisions.
Applied to files:
x/trigger/keeper/query_server.go
📚 Learning: 2025-08-15T20:19:31.726Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/simulation/decoder.go:8-14
Timestamp: 2025-08-15T20:19:31.726Z
Learning: The asset module in Provenance doesn't use collections-based storage like ledger/registry modules. Instead, it delegates storage to the NFT module, so a collections-based store decoder isn't applicable here. The module should use its own types.StoreKey rather than exchange.StoreKey in RegisterStoreDecoder.
Applied to files:
x/trigger/keeper/keeper.go
🧬 Code graph analysis (6)
x/trigger/keeper/event_listener_test.go (4)
x/trigger/keeper/keeper_test.go (1)
KeeperTestSuite(27-41)x/trigger/types/trigger.pb.go (9)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)BlockTimeEvent(216-219)BlockTimeEvent(223-223)BlockTimeEvent(224-226)Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/trigger.go (1)
NewTrigger(149-156)x/trigger/types/errors.go (1)
ErrEventNotFound(9-9)
x/trigger/keeper/genesis.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/genesis.pb.go (3)
GenesisState(27-39)GenesisState(43-43)GenesisState(44-46)x/trigger/simulation/genesis.go (3)
QueueStart(24-24)QueuedTriggers(28-28)Triggers(27-27)
x/trigger/types/keys_test.go (1)
x/trigger/types/keys.go (3)
GetEventNameBytes(159-166)GetEventListenerKey(77-82)GetEventListenerPrefix(84-87)
app/app.go (2)
x/trigger/keeper/keeper.go (1)
NewKeeper(30-59)x/trigger/types/keys.go (1)
StoreKey(17-17)
x/trigger/keeper/queue.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)x/exchange/keeper/export_test.go (1)
Iterate(85-85)x/trigger/types/errors.go (1)
ErrQueueIndexNotFound(10-10)
x/trigger/types/keys.go (2)
x/trigger/simulation/genesis.go (1)
TriggerID(23-23)x/trigger/types/trigger.go (1)
TriggerID(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: tests (03)
- GitHub Check: tests (04)
- GitHub Check: tests (00)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: test-race (01)
- GitHub Check: test-race (04)
- GitHub Check: test-race (00)
- GitHub Check: test-race (02)
- GitHub Check: test-race (03)
- GitHub Check: build-linux
- GitHub Check: Build OSX
- GitHub Check: Build Linux
- GitHub Check: golangci-lint
- GitHub Check: docker
- GitHub Check: ghcr-publish
- GitHub Check: heighliner-docker
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
x/trigger/types/codec.go (1)
113-121: Key layout compatibility looks correct.The composed key format
[32-byte-hash][8-byte-order][8-byte-triggerID]matches the stated legacy layout, and using a fixed-width codec should preserve ordering semantics.app/app.go (1)
642-646: Keeper wiring update to KVStoreService looks correct.Passing
runtime.NewKVStoreService(keys[triggertypes.StoreKey])matches the updatedtriggerkeeper.NewKeepersignature and aligns with the collections schema builder usage.x/trigger/keeper/event_detector_test.go (1)
258-266: Good: test setup now checksNewTriggerWithIDerrors.This makes failures in trigger construction explicit and avoids cascading false negatives.
x/trigger/keeper/query_server.go (1)
38-62: Collections pagination change looks solid.Using
query.CollectionPaginateoverk.TriggersMapshould provide deterministic ordering by key, and settingtrigger.Id = keykeeps the response consistent with the storage key.x/trigger/keeper/migrations_v2.go (1)
21-23: The legacy prefix0x04is safe and does not overlap with any active trigger keys.Verification confirms that
GasLimitKeyPrefix(0x04) is unique among trigger module collections. The current active prefixes are: 0x01 (TriggersMap), 0x02 (EventListeners), 0x03 (Queue), 0x05 (NextTriggerID), 0x06 (QueueStartIndex), and 0x07 (QueueLength). The 0x04 prefix is documented as a legacy/deleted gas limit key intypes/keys.goand is not used by any other collection. The migration deletion usingIterator(GasLimitKeyPrefix, storetypes.PrefixEndBytes(GasLimitKeyPrefix))will safely delete only gas limit keys without affecting other triggers.x/trigger/keeper/keeper_test.go (1)
103-119: [rewritten comment]
[classification tag]x/trigger/keeper/event_listener_test.go (1)
168-170: Good: don’t ignoreRemoveEventListenererrorThis matches the updated
(bool, error)signature and prevents silent failures.x/trigger/keeper/keeper.go (1)
30-58: Keeper collections wiring looks solidSchemaBuilder usage and explicit
StoreServiceassignment are consistent with the collections migration.
nagarajdivine
force-pushed
the
feat/trigger-module-collections
branch
from
fd735fa to
60dd65d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (16)
x/trigger/simulation/decoder.go (3)
34-37: Unsafe key slicing can panic; usebytes.HasPrefixinstead.The expression
kvA.Key[:1]at line 34 will panic ifkvA.Keyis empty. The length validation at lines 35-37 executes only after the case condition matches, so it cannot prevent the panic. Usebytes.HasPrefix(kvA.Key, types.TriggerKeyPrefix.Bytes())for safe, idiomatic prefix matching.🔎 Apply this diff to fix the unsafe slicing:
- case bytes.Equal(kvA.Key[:1], types.TriggerKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.TriggerKeyPrefix.Bytes()): if len(kvA.Key) < 1+types.TriggerIDLength { return fmt.Sprintf("invalid Trigger key length: %d", len(kvA.Key)) }
58-67: Unsafe slicing can cause panic; usebytes.HasPrefixinstead.The expression
kvA.Key[:1]at line 58 will panic ifkvA.Keyis empty. Although length validation exists at lines 62-67, it executes only after the case condition matches. Usebytes.HasPrefix(kvA.Key, types.EventListenerKeyPrefix.Bytes())instead.🔎 Apply this diff:
- case bytes.Equal(kvA.Key[:1], types.EventListenerKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.EventListenerKeyPrefix.Bytes()): triggerIDA := extractTriggerIDFromEventListenerKey(kvA.Key) triggerIDB := extractTriggerIDFromEventListenerKey(kvB.Key) if len(kvA.Key) < 49 {
71-74: Usebytes.HasPrefixfor safe prefix matching.The expression
kvA.Key[:1]at line 71 will panic ifkvA.Keyis empty. Although length validation exists at lines 72-74, it executes only after the case condition matches. Usebytes.HasPrefix(kvA.Key, types.QueueKeyPrefix.Bytes())instead.🔎 Apply this diff:
- case bytes.Equal(kvA.Key[:1], types.QueueKeyPrefix.Bytes()): + case bytes.HasPrefix(kvA.Key, types.QueueKeyPrefix.Bytes()): if len(kvA.Key) < 1+types.QueueIndexLength { return fmt.Sprintf("invalid queue key length: %d", len(kvA.Key)) }x/trigger/keeper/queue.go (3)
23-44: Critical bug: QueuePeek returns nil on fresh store despite non-empty queue.On a freshly initialized queue (or after metadata is cleared):
EnqueuedefaultsstartIndexto 0 locally but never persistsQueueStartIndexto the storeQueuePeekcorrectly checksQueueIsEmpty()(returns false), then callsQueueStartIndex.Get()which returnscollections.ErrNotFound- The function returns
nilinstead of the queued item at index 0Fix: Use the existing helper
k.getQueueStartIndex(ctx)which defaults to 0 onErrNotFound.🔎 Apply this diff:
func (k Keeper) QueuePeek(ctx sdk.Context) *types.QueuedTrigger { - isEmpty := k.QueueIsEmpty(ctx) - - if isEmpty { + if k.QueueIsEmpty(ctx) { return nil } - startIndex, err := k.QueueStartIndex.Get(ctx) - if err != nil { - if errors.Is(err, collections.ErrNotFound) { - return nil - } - return nil - } + startIndex, err := k.getQueueStartIndex(ctx) + if err != nil { + // if metadata is corrupt, treat as empty + return nil + } item, err := k.Queue.Get(ctx, startIndex)
72-101: Critical bug: Dequeue panics on fresh store despite non-empty queue.
Dequeuedirectly callsQueueStartIndex.Get()andQueueLength.Get()at lines 77 and 82, which panic onErrNotFoundwhen metadata isn't initialized. This breaks dequeue operations on freshly initialized queues.Fix: Use the existing helpers
k.getQueueStartIndex(ctx)andk.GetQueueLength(ctx)which default to 0 onErrNotFound.🔎 Apply this diff:
func (k Keeper) Dequeue(ctx sdk.Context) { if k.QueueIsEmpty(ctx) { panic("unable to dequeue from empty queue.") } - startIndex, err := k.QueueStartIndex.Get(ctx) - if err != nil { - panic(err) - } - - length, err := k.QueueLength.Get(ctx) - if err != nil { - panic(err) - } + startIndex, err := k.getQueueStartIndex(ctx) + if err != nil { + panic(err) + } + length, err := k.GetQueueLength(ctx) + if err != nil { + panic(err) + }
47-69: Critical bug: Enqueue never initializes QueueStartIndex in the store.On a fresh store,
EnqueuedefaultsstartIndexto 0 locally (lines 52-54) but never persistsQueueStartIndexto the store. This causesQueuePeekandDequeueto fail on non-empty queues because they directly callQueueStartIndex.Get()which returnsErrNotFound.Fix: Initialize
QueueStartIndexwhen it doesn't exist.🔎 Apply this diff:
func (k Keeper) Enqueue(ctx sdk.Context, item types.QueuedTrigger) error { startIndex, err := k.QueueStartIndex.Get(ctx) if err != nil && !errors.Is(err, collections.ErrNotFound) { return err } if errors.Is(err, collections.ErrNotFound) { startIndex = 0 + if err := k.QueueStartIndex.Set(ctx, startIndex); err != nil { + return err + } } length, err := k.QueueLength.Get(ctx)x/trigger/keeper/event_listener_test.go (2)
234-236: Avoid ignoring the error fromGetTriggerEventI.The error from
trigger1.GetTriggerEventI()is being discarded. This should be captured and asserted to catch potential regressions.🔎 Apply this diff:
- eventI, _ := trigger1.GetTriggerEventI() + eventI, err := trigger1.GetTriggerEventI() + s.Require().NoError(err, "failed to get TriggerEventI for trigger1")
239-259: Test name doesn't match behavior.The test is named
TestCollectionsRemoveAllEventListenersForTriggerbut callsRemoveEventListenerForTriggerID, which removes a single listener per trigger (not "all" listeners in the plural sense). Also, line 256 ignores the error fromGetTriggerEventI().🔎 Apply this diff:
-func (s *KeeperTestSuite) TestCollectionsRemoveAllEventListenersForTrigger() { +func (s *KeeperTestSuite) TestCollectionsRemoveEventListenerForTriggerID() { @@ - eventI, _ := trigger.GetTriggerEventI() + eventI, err := trigger.GetTriggerEventI() + s.Require().NoError(err, "failed to get TriggerEventI for trigger")x/trigger/types/keys_test.go (1)
17-24: Clean up duplicate assertions and use correct types.Lines 17-18 and 19-20 assert the same thing (both check
eventHashagainstK1()). Additionally,K2()andK3()returnuint64, so comparisons should useuint64literals instead ofint()casts for type consistency.🔎 Apply this diff:
assert.EqualValues(t, eventHash, key1.K1(), "should have correct event hash for key1") assert.EqualValues(t, eventHash, key2.K1(), "should have correct event hash for key2") - assert.EqualValues(t, eventHash, key1.K1(), "should have correct name bytes in GetEventListenerKey for key1") - assert.EqualValues(t, eventHash, key2.K1(), "should have correct name bytes in GetEventListenerKey for key2") - assert.EqualValues(t, int(5), key1.K2(), "should have correct order bytes in GetEventListenerKey for key1") - assert.EqualValues(t, int(2), key2.K2(), "should have correct order bytes in GetEventListenerKey for key2") - assert.EqualValues(t, int(1), key1.K3(), "should have correct trigger id bytes in GetEventListenerKey for key1") - assert.EqualValues(t, int(0), key2.K3(), "should have correct trigger id bytes in GetEventListenerKey for key2") + assert.EqualValues(t, uint64(5), key1.K2(), "should have correct order bytes in GetEventListenerKey for key1") + assert.EqualValues(t, uint64(2), key2.K2(), "should have correct order bytes in GetEventListenerKey for key2") + assert.EqualValues(t, uint64(1), key1.K3(), "should have correct trigger id bytes in GetEventListenerKey for key1") + assert.EqualValues(t, uint64(0), key2.K3(), "should have correct trigger id bytes in GetEventListenerKey for key2")x/trigger/keeper/migrations_v2.go (2)
12-19: Migration errors are still being swallowed.
Migrate1To2always returnsnileven ifDeleteGasLimitsencounters errors. This can leave partially-migrated state and lead to inconsistent node behavior. The function should propagate errors fromDeleteGasLimits.🔎 Apply this diff:
-func (m Migrator) DeleteGasLimits(ctx sdk.Context, logger log.Logger) { +func (m Migrator) DeleteGasLimits(ctx sdk.Context, logger log.Logger) error { @@ iter, err := store.Iterator(GasLimitKeyPrefix, storetypes.PrefixEndBytes(GasLimitKeyPrefix)) if err != nil { logger.Error("Failed to create iterator", "err", err) - return + return err } @@ + return nil }func (m Migrator) Migrate1To2(ctx sdk.Context) error { logger := m.keeper.Logger(ctx) logger.Info("Starting migration of x/trigger from 1 to 2.") - m.DeleteGasLimits(ctx, logger) + if err := m.DeleteGasLimits(ctx, logger); err != nil { + return err + } logger.Info("Done migrating x/trigger from 1 to 2.") return nil }
45-48: Copy iterator keys before storing them.Depending on iterator implementation,
iter.Key()may reference a buffer that gets reused onNext(), which can causetoDeleteto contain corrupted or duplicated keys.🔎 Apply this diff:
var toDelete [][]byte for ; iter.Valid(); iter.Next() { - toDelete = append(toDelete, iter.Key()) + k := iter.Key() + kCopy := make([]byte, len(k)) + copy(kCopy, k) + toDelete = append(toDelete, kCopy) }x/trigger/keeper/genesis.go (1)
59-68: Critical: Queue length double-counting causes import-export simulation failure.This is the root cause of the failing CI. The code pre-sets
QueueLengthtolen(data.QueuedTriggers), then callsEnqueuefor each item. SinceEnqueueincrementsQueueLengthinternally, the final length becomes2Ninstead ofN.On export: queue has N items with
QueueLength = N
On import:setQueueLength(N)+ NEnqueuecalls =QueueLength = 2NThis divergence causes the import-export simulation to detect different state between chains.
🔎 Apply this diff to fix:
Option 1: Initialize to 0 and let Enqueue manage the length:
- queueLength := uint64(len(data.QueuedTriggers)) - if err := k.setQueueLength(ctx, queueLength); err != nil { + // Initialize queue length to 0; Enqueue will increment it per item + if err := k.setQueueLength(ctx, 0); err != nil { panic(fmt.Sprintf("Failed to set queue length: %v", err)) }Option 2: Set length and write items directly without Enqueue:
If you need to avoid callingEnqueue(to preserve exact indices), you'll need to write queue items directly to storage without callingEnqueue, then setQueueLengthtoNonce afterward.x/trigger/keeper/event_listener.go (2)
38-64: Wrap errors with context in RemoveEventListener.The function returns raw errors from
trigger.GetTriggerEventI(),getEventNameHash(),EventListeners.Has(), andEventListeners.Remove()without context at lines 41, 46, 52, and 60. This makes debugging difficult when failures occur.🔎 Apply this diff to wrap errors with context:
event, err := trigger.GetTriggerEventI() if err != nil { - return false, err + return false, fmt.Errorf("could not get trigger event for trigger %d: %w", trigger.Id, err) } eventHash, err := getEventNameHash(event.GetEventPrefix()) if err != nil { - return false, err + return false, fmt.Errorf("could not compute event hash for trigger %d: %w", trigger.Id, err) } key := triggertypes.GetEventListenerKey(eventHash, event.GetEventOrder(), trigger.GetId()) exists, err := k.EventListeners.Has(ctx, key) if err != nil { - return false, err + return false, fmt.Errorf("failed checking event listener existence for trigger %d: %w", trigger.Id, err) } if !exists { return false, nil } err = k.EventListeners.Remove(ctx, key) if err != nil { - return false, err + return false, fmt.Errorf("failed removing event listener for trigger %d: %w", trigger.Id, err) }Based on learnings, error wrapping helps reconstruct what went wrong from error messages.
153-161: Use a more appropriate error for invalid event names.
getEventNameHashreturnstriggertypes.ErrEventNotFound(line 157) when the normalized event name is empty. This conflates two distinct scenarios: invalid input (empty/whitespace name) versus a missing listener in storage. The sameErrEventNotFoundis returned byGetEventListenerwhen a listener doesn't exist, making it impossible to distinguish these cases during debugging.Consider introducing
ErrInvalidEventNameor similar for input validation failures, reservingErrEventNotFoundfor cases where the listener genuinely doesn't exist in the store.🔎 Suggested approach:
Add a new error constant to
x/trigger/types/errors.go:ErrInvalidEventName = cerrs.Register(ModuleName, X, "invalid event name")Then update
getEventNameHash:func getEventNameHash(name string) ([]byte, error) { eventName := strings.ToLower(strings.TrimSpace(name)) if len(eventName) == 0 { - return nil, triggertypes.ErrEventNotFound + return nil, triggertypes.ErrInvalidEventName.Wrap("event name cannot be empty") } hash := sha256.Sum256([]byte(eventName)) return hash[:], nil }x/trigger/keeper/trigger.go (1)
19-32: RemoveTrigger still swallows errors, breaking error observability.When
TriggersMap.Has(line 21) orTriggersMap.Remove(line 26) fail, the function returnsfalsewithout propagating the error. This makes it impossible for callers to distinguish "trigger didn't exist" from "storage operation failed"—a critical distinction for debugging the import-export simulation failure mentioned in the PR objectives.Change the signature to
(bool, error)and propagate errors, or at minimum log errors before returningfalse.🔎 Apply this diff to propagate errors:
-func (k Keeper) RemoveTrigger(ctx sdk.Context, id types.TriggerID) bool { +func (k Keeper) RemoveTrigger(ctx sdk.Context, id types.TriggerID) (bool, error) { exists, err := k.TriggersMap.Has(ctx, id) if err != nil { - return false + return false, err } if exists { if err := k.TriggersMap.Remove(ctx, id); err != nil { - return false + return false, err } - return true + return true, nil } - return false + return false, nil }Update all callers to handle the new
(bool, error)return.x/trigger/types/keys.go (1)
76-82: Fix misleading panic message in GetEventListenerKey.The panic message at line 79 says
"invalid event name: "but the parameter iseventHash []byte, not an event name. This is confusing for debugging.🔎 Apply this diff:
func GetEventListenerKey(eventHash []byte, order uint64, id TriggerID) collections.Triple[[]byte, uint64, uint64] { if len(eventHash) == 0 { - panic("invalid event name: ") + panic("invalid event hash: empty") } return collections.Join3(eventHash, order, id) }
🧹 Nitpick comments (7)
x/trigger/keeper/queue.go (1)
103-110: Prefer using the helper method for consistency.Line 105 calls
k.QueueLength.Get(ctx)directly. For consistency with the rest of the queue implementation and to ensure proper not-found handling, consider using thek.GetQueueLength(ctx)helper instead.🔎 Apply this diff:
func (k Keeper) QueueIsEmpty(ctx sdk.Context) bool { - length, err := k.QueueLength.Get(ctx) - if err != nil { - return errors.Is(err, collections.ErrNotFound) - } - return length == 0 + length, err := k.GetQueueLength(ctx) + if err != nil { + return true // treat errors as empty + } + return length == 0 }x/trigger/keeper/event_listener.go (4)
66-83: Wrap errors with context in GetEventListener.Errors from
getEventNameHash()(line 70) andEventListeners.Has()(line 75) are returned without contextual information. Add wrapping to identify which event name and trigger ID failed.🔎 Apply this diff to wrap errors:
eventHash, err := getEventNameHash(eventName) if err != nil { - return triggertypes.Trigger{}, err + return triggertypes.Trigger{}, fmt.Errorf("could not hash event name %q: %w", eventName, err) } key := triggertypes.GetEventListenerKey(eventHash, order, triggerID) exists, err := k.EventListeners.Has(ctx, key) if err != nil { - return triggertypes.Trigger{}, err + return triggertypes.Trigger{}, fmt.Errorf("failed checking event listener for trigger %d: %w", triggerID, err) }
85-121: Wrap errors with context in IterateEventListeners.Errors are returned without context at lines 89, 95, 102, and 110. Add contextual information to help identify which event name or operation failed.
🔎 Apply this diff to wrap errors:
eventHash, err := getEventNameHash(eventName) if err != nil { - return err + return fmt.Errorf("could not hash event name %q: %w", eventName, err) } rng := collections.NewPrefixedTripleRange[[]byte, uint64, uint64](eventHash) iter, err := k.EventListeners.Iterate(ctx, rng) if err != nil { - return err + return fmt.Errorf("failed to iterate event listeners for %q: %w", eventName, err) } defer iter.Close() for ; iter.Valid(); iter.Next() { key, err := iter.Key() if err != nil { - return err + return fmt.Errorf("failed to read iterator key for event %q: %w", eventName, err) } triggerID := key.K3() trigger, err := k.GetTrigger(ctx, triggerID) if err != nil { if errors.Is(err, triggertypes.ErrTriggerNotFound) { continue } - return err + return fmt.Errorf("failed to get trigger %d for event %q: %w", triggerID, eventName, err) }
123-131: Wrap error with context in HasEventListener.The error from
getEventNameHash()at line 127 lacks context about which event name failed.🔎 Apply this diff:
eventHash, err := getEventNameHash(eventName) if err != nil { - return false, err + return false, fmt.Errorf("could not hash event name %q: %w", eventName, err) }
133-141: Wrap error with context in RemoveEventListenerForTriggerID.The error from
GetTrigger()at line 137 should include the trigger ID to clarify which trigger couldn't be retrieved.🔎 Apply this diff:
trigger, err := k.GetTrigger(ctx, triggerID) if err != nil { - return err + return fmt.Errorf("failed to get trigger %d: %w", triggerID, err) }x/trigger/keeper/trigger.go (2)
78-88: Consider using TriggersMap.Has for efficiency.The function calls
TriggersMap.Getand discards the value, whenTriggersMap.Haswould be more efficient and expressive for checking existence.🔎 Apply this diff:
func (k Keeper) HasTrigger(ctx sdk.Context, id uint64) (bool, error) { - _, err := k.TriggersMap.Get(ctx, id) - if errors.Is(err, collections.ErrNotFound) { - return false, nil - } - if err != nil { - return false, err - } - return true, nil + return k.TriggersMap.Has(ctx, id) }
113-116: Add guard against zero trigger ID.While the
-1offset issue from the previous review has been correctly removed, the function should still guard againsttriggerID == 0, which would be invalid and could indicate a logic error upstream.🔎 Apply this diff:
func (k Keeper) setTriggerID(ctx sdk.Context, triggerID types.TriggerID) error { + if triggerID == 0 { + return fmt.Errorf("triggerID must be > 0") + } return k.NextTriggerID.Set(ctx, triggerID) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.changelog/unreleased/features/2499-trigger-module-collections.md(1 hunks)app/app.go(1 hunks)x/trigger/keeper/event_detector_test.go(1 hunks)x/trigger/keeper/event_listener.go(2 hunks)x/trigger/keeper/event_listener_test.go(1 hunks)x/trigger/keeper/genesis.go(3 hunks)x/trigger/keeper/keeper.go(2 hunks)x/trigger/keeper/keeper_test.go(2 hunks)x/trigger/keeper/migrations_v2.go(1 hunks)x/trigger/keeper/msg_server.go(3 hunks)x/trigger/keeper/query_server.go(3 hunks)x/trigger/keeper/queue.go(2 hunks)x/trigger/keeper/queue_test.go(1 hunks)x/trigger/keeper/trigger.go(3 hunks)x/trigger/keeper/trigger_registry.go(1 hunks)x/trigger/keeper/trigger_test.go(2 hunks)x/trigger/simulation/decoder.go(2 hunks)x/trigger/simulation/genesis.go(1 hunks)x/trigger/types/codec.go(2 hunks)x/trigger/types/keys.go(3 hunks)x/trigger/types/keys_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changelog/unreleased/features/2499-trigger-module-collections.md
🚧 Files skipped from review as they are similar to previous changes (3)
- x/trigger/keeper/queue_test.go
- app/app.go
- x/trigger/keeper/trigger_test.go
🧰 Additional context used
🧠 Learnings (30)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 1993
File: x/marker/client/cli/cli_test.go:1090-1090
Timestamp: 2024-06-10T19:25:28.209Z
Learning: SpicyLemon prefers allowing tests to fail naturally if the setup is incorrect rather than explicitly checking and handling such errors within the test functions. They also recommend using `s.Require().NotNil(...)` instead of `s.T().Fatal(...)` for assertions in test functions.
Applied to files:
x/trigger/keeper/event_detector_test.gox/trigger/keeper/event_listener_test.go
📚 Learning: 2024-10-10T23:19:54.565Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades_test.go:610-610
Timestamp: 2024-10-10T23:19:54.565Z
Learning: In `app/upgrades_test.go`, we prefer to keep TODO comments (like `TODO[viridian]`) to handle code deletions manually, and do not want to implement automated processes for code removal, especially for unit tests.
Applied to files:
x/trigger/keeper/event_listener_test.gox/trigger/keeper/migrations_v2.gox/trigger/types/keys_test.go
📚 Learning: 2025-08-15T20:57:04.455Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/entries_test.go:26-31
Timestamp: 2025-08-15T20:57:04.455Z
Learning: SpicyLemon prefers string-based error assertions in tests using empty string to indicate "no error expected" and non-empty string for expected error messages. Use assertions.AssertErrorValue instead of errors.Is() comparisons because errors.Is() with s.Require().True() provides poor failure messages that only say "Should be true" without showing expected vs actual error content.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2024-10-02T03:42:47.070Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/query_server_test.go:940-945
Timestamp: 2024-10-02T03:42:47.070Z
Learning: In the `TestScopeQuery` function, the conditional checks following the equality assertion are designed to help identify differences when `tc.expResp` and `actResp` are not equal and both are not `nil`. If they are equal, or either is `nil`, the failure message from the equality assertion suffices, and further checks are not needed.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2024-10-02T03:37:18.486Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server_test.go:1225-1228
Timestamp: 2024-10-02T03:37:18.486Z
Learning: When test cases are run using `s.T().Run` instead of `s.Run`, use `require` with `t` instead of `s.Require()`, to ensure that assertions apply to the correct test instance.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2025-08-15T20:01:55.078Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: app/upgrades_test.go:0-0
Timestamp: 2025-08-15T20:01:55.078Z
Learning: SpicyLemon prefers upgrade tests to fail when expected data is missing rather than skip, using s.Require().NoError() and s.Require().NotEmpty() to enforce hard requirements for upgrade data files in test scenarios.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2024-10-10T23:13:56.722Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: x/ibchooks/types/params.go:22-26
Timestamp: 2024-10-10T23:13:56.722Z
Learning: In the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `Validate` method without modifying the error messages, even if they may lack additional context.
Applied to files:
x/trigger/keeper/event_listener_test.gox/trigger/keeper/event_listener.gox/trigger/types/keys.go
📚 Learning: 2025-09-05T19:27:01.045Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/keeper/msg_server_test.go:49-56
Timestamp: 2025-09-05T19:27:01.045Z
Learning: In test files, prefer using testutil.EventsBuilder and assertions.AssertEqualEvents over custom helper functions for event and attribute lookup to maintain consistency with established testing patterns in the Provenance codebase.
Applied to files:
x/trigger/keeper/event_listener_test.gox/trigger/types/keys_test.go
📚 Learning: 2024-10-02T03:39:04.537Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server_test.go:1248-1251
Timestamp: 2024-10-02T03:39:04.537Z
Learning: In subtests, it's acceptable to use `require` and `assert` functions directly instead of `s.Require()` and `s.Assert()` methods.
Applied to files:
x/trigger/keeper/event_listener_test.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:57-80
Timestamp: 2024-10-08T18:12:51.935Z
Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.
Applied to files:
x/trigger/keeper/migrations_v2.gox/trigger/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:57.974Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 1874
File: cmd/dbmigrate/utils/migrator_test.go:10-10
Timestamp: 2024-10-08T18:12:57.974Z
Learning: The `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned to be removed from the project soon, as indicated by the user. Therefore, suggestions for changes or tests related to this directory are unnecessary.
Applied to files:
x/trigger/keeper/migrations_v2.go
📚 Learning: 2024-10-08T18:12:57.975Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:262-263
Timestamp: 2024-10-08T18:12:57.975Z
Learning: In the `MigrateValueOwner` method, use `sdkerrors.ErrLogic` when representing unexpected backend errors, as the request itself is valid.
Applied to files:
x/trigger/keeper/migrations_v2.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2033
File: app/upgrades.go:303-310
Timestamp: 2024-10-08T18:12:51.935Z
Learning: There are no rollback mechanisms used in the migration functions of the `app/upgrades.go` file, and error handling is managed by logging and returning errors as per the project's standards.
Applied to files:
x/trigger/keeper/migrations_v2.go
📚 Learning: 2025-08-15T20:34:01.110Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/codec.go:9-10
Timestamp: 2025-08-15T20:34:01.110Z
Learning: In Go Cosmos SDK projects, sdk.Msg interface embeds proto.Message interface, making it valid to copy elements from []sdk.Msg to []proto.Message even though the slice types are different. The pattern of declaring AllRequestMsgs as []sdk.Msg for type assertions while copying to []proto.Message for RegisterImplementations is a common and correct pattern.
Applied to files:
x/trigger/types/codec.go
📚 Learning: 2025-08-15T20:34:01.110Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/codec.go:9-10
Timestamp: 2025-08-15T20:34:01.110Z
Learning: The pattern of declaring AllRequestMsgs as []sdk.Msg and then copying to []proto.Message for RegisterImplementations is a standard, correct pattern used throughout the Provenance codebase. This works because sdk.Msg interface embeds proto.Message interface, making the copy operation valid. This pattern appears consistently across 15+ modules and compiles without errors.
Applied to files:
x/trigger/types/codec.go
📚 Learning: 2024-10-02T18:03:10.008Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils_test.go:478-493
Timestamp: 2024-10-02T18:03:10.008Z
Learning: In Go tests, modifying byte slices directly using arithmetic operations like addition and subtraction is acceptable, even if it causes overflow, as overflow on `uint8` just wraps around. Avoid flagging such usage, and avoid suggesting bitwise operations which can be harder to read and understand.
Applied to files:
x/trigger/simulation/decoder.gox/trigger/simulation/genesis.gox/trigger/types/keys_test.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/trigger/simulation/decoder.gox/trigger/types/keys.go
📚 Learning: 2025-08-15T20:59:51.949Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keys.go:3-12
Timestamp: 2025-08-15T20:59:51.949Z
Learning: In Cosmos SDK, key prefixes only need to be unique within a given module's store, not across the entire application. Each module operates within its own isolated key-value store namespace, so the same byte prefixes can be safely reused across different modules without causing collisions.
Applied to files:
x/trigger/keeper/query_server.go
📚 Learning: 2025-08-15T22:52:24.461Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/keeper.go:74-173
Timestamp: 2025-08-15T22:52:24.461Z
Learning: In the ledger keeper NewKeeper function in x/ledger/keeper/keeper.go, SpicyLemon prefers to allow bankKeeper and registryKeeper dependencies to be nil rather than adding validation that would panic on nil values. The design intentionally supports nil keepers for flexibility.
Applied to files:
x/trigger/keeper/genesis.gox/trigger/keeper/keeper_test.gox/trigger/keeper/keeper.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/trigger/keeper/keeper_test.gox/trigger/keeper/trigger.gox/trigger/keeper/keeper.gox/trigger/keeper/queue.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: In Cosmos SDK v0.50.x, the bank keeper interface methods use `context.Context` rather than `sdk.Context`. The BankKeeper interface should use `context.Context` for methods like SendCoins, HasBalance, GetBalance, etc.
Applied to files:
x/trigger/keeper/keeper_test.go
📚 Learning: 2025-08-15T20:39:34.095Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/util.go:68-102
Timestamp: 2025-08-15T20:39:34.095Z
Learning: In the Provenance codebase, functions with "Must" prefix (like MustGetMarkerAddress) can panic and should only be used when you know they won't panic. Instead, use the twin function that returns an error (like GetMarkerAddress) and handle the error properly.
Applied to files:
x/trigger/keeper/event_listener.gox/trigger/types/keys.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `GetLockedCoins` function in `x/hold/keeper/locked_coins.go` must conform to the `banktypes.GetLockedCoinsFn` type, which does not allow returning an error. In cases where `k.GetHoldCoins` encounters an error, using `panic` is considered appropriate due to the severity of such errors.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:106-109
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In the `AddScopeDataAccess` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/trigger/keeper/event_listener.go
📚 Learning: 2025-09-05T19:26:30.744Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/keeper/msg_server_test.go:38-46
Timestamp: 2025-09-05T19:26:30.744Z
Learning: In Provenance tests, prefer using testutil.EventsBuilder to construct expected events and assertions.AssertEqualEvents for precise event matching. Use assertions.AssertEventsContains only when there are many uncontrolled events from SDK modules that you don't care about. Most tests should be controlled enough to define all expected events precisely.
Applied to files:
x/trigger/types/keys_test.go
📚 Learning: 2024-10-02T03:06:21.997Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-02T03:06:21.997Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/trigger/types/keys_test.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4_test.go:253-479
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In Go tests within this project, it's acceptable to have similar helper functions like `newUUID`, `newScopeID`, `newSpecID`, and `newScope` defined within each test. Tests may tailor these functions to specific needs rather than sharing them, to avoid breaking other tests when changes are made and to keep them non-public.
Applied to files:
x/trigger/types/keys_test.go
📚 Learning: 2024-10-24T19:50:09.457Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2198
File: app/app_test.go:448-528
Timestamp: 2024-10-24T19:50:09.457Z
Learning: In `app/app_test.go`, when testing that the encoder is correctly wired to handle a governance `Proposal` with a `ParameterChangeProposal`, it's sufficient to have a basic test case without adding additional test cases, since the types and encoders are defined and maintained in external libraries outside our control.
Applied to files:
x/trigger/types/keys_test.go
📚 Learning: 2025-08-15T20:19:31.726Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/simulation/decoder.go:8-14
Timestamp: 2025-08-15T20:19:31.726Z
Learning: The asset module in Provenance doesn't use collections-based storage like ledger/registry modules. Instead, it delegates storage to the NFT module, so a collections-based store decoder isn't applicable here. The module should use its own types.StoreKey rather than exchange.StoreKey in RegisterStoreDecoder.
Applied to files:
x/trigger/keeper/keeper.go
🧬 Code graph analysis (11)
x/trigger/simulation/decoder.go (2)
x/trigger/types/keys.go (7)
NextTriggerIDKey(65-65)QueueStartIndexKey(67-67)QueueLengthKey(69-69)EventListenerKeyPrefix(61-61)QueueKeyPrefix(63-63)QueueIndexLength(24-24)ModuleName(14-14)x/trigger/types/trigger.pb.go (6)
Trigger(33-42)Trigger(46-46)Trigger(47-49)QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)
x/trigger/keeper/query_server.go (4)
x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/query.pb.go (3)
QueryTriggersResponse(172-177)QueryTriggersResponse(181-181)QueryTriggersResponse(182-184)x/trigger/simulation/genesis.go (1)
Triggers(27-27)
x/trigger/keeper/keeper_test.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
BlockHeightEvent(170-173)BlockHeightEvent(177-177)BlockHeightEvent(178-180)x/trigger/types/keys.go (2)
ModuleName(14-14)GetTriggerKey(90-97)
x/trigger/keeper/trigger.go (5)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/simulation/genesis.go (1)
TriggerID(23-23)x/trigger/types/trigger.go (1)
TriggerID(16-16)x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)
x/trigger/keeper/msg_server.go (1)
x/trigger/types/errors.go (1)
ErrTriggerNotFound(8-8)
x/trigger/keeper/trigger_registry.go (2)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)
x/trigger/keeper/event_listener.go (4)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
Trigger(33-42)Trigger(46-46)Trigger(47-49)x/trigger/types/keys.go (1)
GetEventListenerKey(77-82)x/trigger/types/errors.go (2)
ErrEventNotFound(9-9)ErrTriggerNotFound(8-8)
x/trigger/types/keys_test.go (1)
x/trigger/types/keys.go (3)
GetEventNameBytes(159-166)GetEventListenerKey(77-82)GetEventListenerPrefix(84-87)
x/trigger/keeper/keeper.go (3)
x/trigger/types/trigger.pb.go (6)
Trigger(33-42)Trigger(46-46)Trigger(47-49)QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)x/trigger/types/keys.go (6)
TriggerKeyPrefix(59-59)EventListenerKeyPrefix(61-61)QueueKeyPrefix(63-63)NextTriggerIDKey(65-65)QueueStartIndexKey(67-67)QueueLengthKey(69-69)x/trigger/types/codec.go (1)
EventListenerKeyCodec(115-121)
x/trigger/keeper/queue.go (3)
x/trigger/keeper/keeper.go (1)
Keeper(15-28)x/trigger/types/trigger.pb.go (3)
QueuedTrigger(106-113)QueuedTrigger(117-117)QueuedTrigger(118-120)x/trigger/types/errors.go (1)
ErrQueueIndexNotFound(10-10)
x/trigger/types/keys.go (2)
x/trigger/simulation/genesis.go (1)
TriggerID(23-23)x/trigger/types/trigger.go (1)
TriggerID(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: tests (00)
- GitHub Check: test-race (00)
- GitHub Check: test-race (03)
- GitHub Check: tests (02)
- GitHub Check: test-race (04)
- GitHub Check: tests (04)
- GitHub Check: tests (03)
- GitHub Check: test-race (01)
- GitHub Check: test-race (02)
- GitHub Check: tests (01)
- GitHub Check: build-linux
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: heighliner-docker
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
🔇 Additional comments (35)
x/trigger/keeper/event_detector_test.go (1)
261-262: LGTM! Error handling properly implemented.The error from
NewTriggerWithIDis now correctly captured and asserted, ensuring test setup fails fast if trigger creation fails. This addresses the previous review feedback and follows the preferred pattern of usings.Require().NoError()for test assertions.x/trigger/simulation/decoder.go (1)
99-115: LGTM! Helper functions are well-implemented.Both
extractTriggerIDFromEventListenerKeyandsafeDecodeUint64include proper bounds checking before accessing byte slices, preventing panics. The defensive approach of returning 0 for invalid inputs is appropriate for simulation decoding.x/trigger/types/codec.go (2)
47-111: LGTM! FixedBytes32KeyCodec is correctly implemented.The codec properly validates key and buffer lengths before operations, preventing panics. The fixed 32-byte size is correctly enforced in Encode, Decode, and Size methods. The NonTerminal variants correctly delegate to terminal methods (appropriate for fixed-size keys), and the JSON stubs are intentionally unimplemented as documented.
113-121: LGTM! EventListenerKeyCodec correctly composes the triple key.The codec correctly uses TripleKeyCodec with FixedBytes32KeyCodec for the event hash and two Uint64Keys for order and triggerID. The total key size (1-byte prefix + 32 + 8 + 8 = 49 bytes) aligns with the length checks in decoder.go.
x/trigger/keeper/queue.go (1)
197-229: LGTM! Helper methods correctly handle not-found cases.The helper methods
getQueueStartIndexandGetQueueLengthproperly default to 0 when metadata isn't found, making them safe for fresh store initialization. These should be consistently used throughout the queue implementation instead of direct.Get()calls.x/trigger/simulation/genesis.go (1)
179-183: LGTM! Comment properly documents safe conversion.The comment correctly explains that the uint64 to int conversion is safe because of the explicit boundary check at lines 179-182. The
nolint:gosecdirective is appropriate and well-justified.x/trigger/keeper/trigger_registry.go (2)
10-25: LGTM! Improved error handling with appropriate logging.The error handling now captures and logs failures from
SetTriggerandSetEventListenerwith helpful context (triggerID). Continuing execution after logging is appropriate for registration operations, making this a good defensive programming improvement over the previous silent error discarding.
27-37: LGTM! Consistent error handling pattern.The error handling for
RemoveEventListenerfollows the same pattern asRegisterTrigger, providing good visibility into failures while allowing the operation to continue. This consistency improves maintainability.x/trigger/keeper/keeper_test.go (1)
103-119: LGTM! Important backward compatibility test.This test correctly validates that the new collections-based
GetTriggercan read data stored with the old key format. This is crucial for the "no state migration required" requirement mentioned in the PR objectives. The test setup and assertions are appropriate.x/trigger/keeper/query_server.go (2)
20-35: LGTM! Correct error mapping for public API.The error handling correctly maps the internal
collections.ErrNotFoundto the publictypes.ErrTriggerNotFound, while propagating other errors unchanged. This is the appropriate pattern for query handlers.
37-62: LGTM! Correct migration to collections-based pagination.The implementation correctly uses
query.CollectionPaginateover theTriggersMap, properly sets thetrigger.Idfrom the map key in the mapping function, and constructs the response correctly. The error handling is appropriate.Note: Past review suggested removing explicit type parameters from the
CollectionPaginatecall (they can be inferred), but the code is functionally correct either way.x/trigger/keeper/event_listener_test.go (1)
168-172: LGTM!The error handling for
RemoveEventListenerhas been properly addressed. The test now captures and asserts on the error value.x/trigger/keeper/msg_server.go (2)
39-44: LGTM!The error handling for
NewTriggerWithIDis properly implemented, propagating errors before proceeding with registration.
60-66: LGTM!Good practice to map the internal
collections.ErrNotFoundto the publictypes.ErrTriggerNotFounderror type for a cleaner API surface.x/trigger/keeper/genesis.go (2)
12-21: LGTM!Error handling for
getTriggerIDandgetQueueStartIndexis properly implemented with panics on failure, which is appropriate for genesis operations.
33-38: Good defensive check for queue length consistency.This warning log helps detect potential state inconsistencies during export. Consider whether this should be a panic instead of a warning if consistency is critical.
x/trigger/keeper/keeper.go (1)
30-59: LGTM! StoreService initialization issue resolved.The
StoreServicefield is now properly initialized at line 40, addressing the critical issue from the previous review. All collections are correctly wired with appropriate prefixes, and the schema building follows standard Cosmos SDK patterns.x/trigger/keeper/event_listener.go (2)
16-35: LGTM! Error handling properly implemented.All errors are appropriately wrapped with contextual information including the trigger ID, making debugging easier.
143-151: LGTM!The implementation correctly delegates to
IterateEventListenerswith a simple counting callback.x/trigger/keeper/trigger.go (6)
14-17: LGTM!Clean delegation to the collections-based storage with proper error propagation.
34-44: LGTM!Proper error translation from collections.ErrNotFound to the module-specific error type.
46-67: LGTM!Proper iterator usage with defer close and error propagation.
118-128: LGTM! ID increment logic is now correct.With the removal of the
-1offset insetTriggerID, this function now correctly returns the current ID and stores the next ID for future use. This should resolve the export/import stability issues mentioned in the PR objectives.
130-137: LGTM!Correct implementation with proper default value handling.
90-111: LGTM! NewTriggerWithID correctly uses the ID sequence.With the fixes to
setTriggerID(removing the-1offset), this function now correctly obtains incrementing IDs. The pattern of callinggetNextTriggerIDensures proper ID allocation and should maintain export/import stability.x/trigger/types/keys.go (10)
58-70: LGTM! Prefix declarations correctly migrated to collections.All prefixes properly use
collections.NewPrefixwith appropriate numeric identifiers.
84-87: LGTM! Correctly uses .Bytes() conversion.Properly converts the
collections.Prefixto[]bytefor the return value.
89-97: LGTM! Correctly uses .Bytes() conversion.
99-102: LGTM! Correctly uses .Bytes() conversion.
104-107: LGTM! Correctly uses .Bytes() conversion.
109-116: LGTM! Correctly uses .Bytes() conversion.
118-121: LGTM! Correctly uses .Bytes() conversion.
123-126: LGTM! Correctly uses .Bytes() conversion.
128-134: LGTM! Good defensive length check.The function safely handles short byte slices by returning 0.
143-149: LGTM! Good defensive length check.The function safely handles short byte slices by returning 0. Past review verified all call sites pass exactly 8 bytes.
nagarajdivine
force-pushed
the
feat/trigger-module-collections
branch
from
60dd65d to
35950e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@x/trigger/keeper/queue.go`:
- Around line 47-69: Enqueue duplicates the "ErrNotFound => 0" handling; replace
the manual QueueStartIndex/Get and QueueLength/Get logic with the existing
helper calls (getQueueStartIndex and GetQueueLength) to obtain startIndex and
length, propagate any errors those helpers return, then compute position and set
the item and updated length as before using k.Queue.Set and k.QueueLength.Set;
ensure you remove the duplicated ErrNotFound checks and use the helper return
values consistently in the Enqueue function.
- Around line 14-20: QueueTrigger currently swallows errors from Enqueue which
can permanently lose triggers (UnregisterTrigger is called before QueueTrigger
in DetectBlockEvents); change QueueTrigger to return an error (update its
signature to func (k Keeper) QueueTrigger(ctx sdk.Context, trigger
types.Trigger) error), remove the silent swallow and instead return the Enqueue
error (or wrap it with context) instead of just logging, and update callers
(notably DetectBlockEvents) to handle the returned error appropriately
(propagate, retry, or panic per caller policy); keep the ctx.Logger().Error for
logging but do not suppress returning the underlying error from Enqueue.
In `@x/trigger/keeper/trigger_registry.go`:
- Around line 28-36: UnregisterTrigger currently swallows RemoveEventListener
errors; change UnregisterTrigger to return an error and propagate the
RemoveEventListener error instead of just logging it: call k.RemoveTrigger(ctx,
trigger.GetId()) after attempting to remove the listener or, better, attempt
k.RemoveEventListener(ctx, trigger) first and if it returns an error return that
error (do not silently log), otherwise call k.RemoveTrigger and return nil;
update the UnregisterTrigger signature and all callers accordingly to handle the
returned error and reference the existing RemoveEventListener and RemoveTrigger
methods.
- Around line 10-24: RegisterTrigger currently swallows errors from SetTrigger
and SetEventListener; change RegisterTrigger to return an error (e.g., func (k
Keeper) RegisterTrigger(ctx sdk.Context, trigger triggertypes.Trigger) error),
propagate and return any error from k.SetTrigger(...) or
k.SetEventListener(...), and update the caller CreateTrigger in msg_server.go to
check RegisterTrigger's error and return a failing response when an error occurs
so the transaction is aborted; reference SetTrigger, SetEventListener,
RegisterTrigger, and CreateTrigger when making these changes.
In `@x/trigger/simulation/decoder.go`:
- Around line 58-69: The EventListener branch currently slices kvA.Key[:1]
unsafely, calls extractTriggerIDFromEventListenerKey before validating key
lengths, and never compares kvA.Value/kvB.Value; fix by first checking key
lengths (ensure len(kvA.Key) and len(kvB.Key) are >= expected), use a safe
prefix check (e.g. bytes.HasPrefix or check len>0 before slicing) instead of
kvA.Key[:1], then extract trigger IDs with extractTriggerIDFromEventListenerKey,
and include comparison of the stored values (kvA.Value vs kvB.Value) — decode
them if there is a helper or include their hex/base64 representation in the
returned message so value differences are detected by the decoder.
🧹 Nitpick comments (7)
x/trigger/types/keys.go (1)
128-148: Defensive length checks are a good addition.Returning 0 for undersized byte slices instead of panicking is a safer approach. However, silently returning 0 could mask bugs at call sites — callers won't know they got a zero-value due to bad input vs. a legitimately stored zero.
x/trigger/types/keys_test.go (1)
28-34: Prefix type comparison in assertions may be fragile.Lines 32 and 38 compare
collections.Prefix(e.g.,EventListenerKeyPrefix) against[]byteslices. WhileEqualValuesshould coerce types, ifcollections.Prefixchanges its underlying representation, these tests could silently pass or fail unexpectedly. Consider comparing against.Bytes()explicitly for clarity.x/trigger/keeper/msg_server.go (1)
60-63: Good error mapping fromcollections.ErrNotFoundtotypes.ErrTriggerNotFound.Consider wrapping the trigger ID for debuggability:
Suggestion
if errors.Is(err, collections.ErrNotFound) { - return nil, types.ErrTriggerNotFound + return nil, fmt.Errorf("%w: id %d", types.ErrTriggerNotFound, msg.GetId()) }x/trigger/keeper/event_listener.go (1)
86-121:IterateEventListenerssilently skips triggers withErrTriggerNotFound.This is a reasonable defensive choice for orphaned event listener keys, but it means data inconsistencies are silently ignored. Consider at least logging a warning when a trigger referenced by an event listener key doesn't exist.
x/trigger/keeper/trigger.go (1)
78-88:HasTriggerusesGetinstead ofHas— unnecessary deserialization overhead.
TriggersMap.Has(ctx, id)would check existence without deserializing the value.Proposed fix
func (k Keeper) HasTrigger(ctx sdk.Context, id uint64) (bool, error) { - _, err := k.TriggersMap.Get(ctx, id) - if errors.Is(err, collections.ErrNotFound) { - return false, nil - } - if err != nil { - return false, err - } - return true, nil + return k.TriggersMap.Has(ctx, id) }x/trigger/keeper/queue.go (1)
120-124: UnnecessaryKey()call inGetAllQueueItems— result is discarded.Line 121 reads the key but never uses it. Remove to avoid wasted work.
Proposed fix
for ; iter.Valid(); iter.Next() { - _, err := iter.Key() - if err != nil { - return nil, err - } value, err := iter.Value() if err != nil { return nil, errx/trigger/simulation/decoder.go (1)
99-108: Magic numbers and dead code inextractTriggerIDFromEventListenerKey.
The magic number
49appears in three places (lines 62, 65, 100) and41at line 103. These should be derived from named constants to remain in sync if the key layout changes. For example:const eventListenerKeyMinLen = 1 + 32 + 8 + 8 // prefix + eventTypeHash + order + triggerID const triggerIDOffset = 1 + 32 + 8Line 104 (
if len(triggerIDBytes) < 8) is dead code —key[41:49]always produces exactly 8 bytes after thelen(key) >= 49guard at line 100.The key structure (1 prefix + 32 hash + 8 order + 8 triggerID) should be verified against the actual collections composite key definition.
#!/bin/bash # Verify the EventListener key structure in types/keys.go fd 'keys.go' --type=f --exec grep -n -A20 'EventListener' {}
nagarajdivine
force-pushed
the
feat/trigger-module-collections
branch
from
35950e4 to
96ecfd0
Compare
nagarajdivine
force-pushed
the
feat/trigger-module-collections
branch
from
96ecfd0 to
aff2541
Compare
2 participants
Description
closes: Switch to collections in the trigger module. #2499
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
Changelog
New Features
Bug Fixes
Chores
Tests