Migrated sanction module to collections for state storage.#2570
Migrated sanction module to collections for state storage.#2570nagarajdivine wants to merge 10 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:
📝 WalkthroughWalkthroughMigrates x/sanction from direct KVStore access to cosmossdk.io/collections: adds collection-backed maps and key codecs, changes Keeper to accept a StoreService and context-based APIs, updates queries/tests, adds prefix/key codecs, improves error handling, and includes backward-compatibility tests for old KV layouts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as gRPC Client
participant GRPC as sanction gRPC
participant Keeper as sanction Keeper
participant StoreSvc as runtime.KVStoreService
participant DB as underlying KV DB
Client->>GRPC: ListTemporaryEntries(req)
GRPC->>Keeper: TemporaryEntries(ctx, req)
alt address filter provided
Keeper->>Keeper: temporaryEntriesForAddress(ctx, addr, pagination)
Keeper->>StoreSvc: OpenKVStore(ctx)
StoreSvc->>DB: open store
Keeper->>Keeper: walk TemporaryEntries collection with prefix
Keeper-->>Keeper: collect, apply offset/limit, compute NextKey/Total
else unfiltered
Keeper->>Keeper: CollectionPaginate(k.TemporaryEntriesStore, pagination)
Keeper->>StoreSvc: OpenKVStore(ctx)
StoreSvc->>DB: open store
Keeper->>Keeper: collection pagination returns page + total/nextKey
end
Keeper-->>GRPC: build response with Entries + Pagination
GRPC-->>Client: return paginated result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🧹 Nitpick comments (5)
x/trigger/simulation/genesis.go (1)
179-185: Int cast is safe after themath.MaxIntguardThe explicit
triggerID > uint64(math.MaxInt)panic ensures the subsequenttriggerIDInt := int(triggerID)is safe on all supported architectures. From a correctness standpoint, removing the//nolint:gosec G115is fine.If gosec still complains about G115 despite the guard, consider adding a brief comment explaining that the prior range check guarantees safety, or reintroducing a narrowly scoped
nolintwith that explanation.x/sanction/keeper/export_test.go (1)
47-50: Comment is stale - says "StoreKey" but method is now "KVStore".The comment on line 47 still references "storekey" but the method has been renamed to
KVStore.-// StoreKey, for unit tests, exposes this keeper's storekey. +// KVStore, for unit tests, returns a KVStore from the keeper's StoreService. func (k Keeper) KVStore(ctx sdk.Context) store.KVStore { return k.StoreService.OpenKVStore(ctx) }x/sanction/keeper/keeper.go (3)
99-100: Typo: Comment has line break in the middle of "Proposal".There's an accidental line break splitting "Proposal" across two lines.
- // Proposa - // l index: (uint64, AccAddress) -> []byte with custom codec + // Proposal index: (uint64, AccAddress) -> []byte with custom codec // Collections adds 0x03 prefix, codec handles [propID][len][addr]
266-297: Consider using prefix range forDeleteGovPropTempEntries.Similar to
getLatestTempEntry, this method walks the entireProposalIndexto find entries for a specificgovPropID. Since theProposalIndexkey is(govPropID, addr), you could use a prefix range based ongovPropIDto only iterate relevant entries.However, this is less critical than
getLatestTempEntrysince deletion happens less frequently than reads.
300-342: Consider using prefix range forDeleteAddrTempEntries.This method walks the entire
TemporaryEntriesStorefor each address. Since the key is(addr, govPropID), you could use a prefix range based onaddrto only iterate relevant entries. This is especially important when deleting entries for multiple addresses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.changelog/unreleased/features/2498-sanction-module-collections.md(1 hunks)app/app.go(1 hunks)internal/protocompat/protocompat.go(1 hunks)x/attribute/client/cli/tx.go(1 hunks)x/exchange/client/cli/flags.go(2 hunks)x/ledger/client/cli/tx.go(4 hunks)x/ledger/keeper/genesis.go(3 hunks)x/marker/client/cli/tx.go(2 hunks)x/marker/types/si.go(1 hunks)x/name/client/cli/tx.go(1 hunks)x/sanction/codec.go(2 hunks)x/sanction/keeper/export_test.go(3 hunks)x/sanction/keeper/genesis.go(2 hunks)x/sanction/keeper/genesis_test.go(1 hunks)x/sanction/keeper/gov_hooks.go(1 hunks)x/sanction/keeper/grpc_query.go(4 hunks)x/sanction/keeper/keeper.go(7 hunks)x/sanction/keeper/keeper_test.go(25 hunks)x/sanction/keeper/keys.go(2 hunks)x/sanction/keeper/suite_test.go(3 hunks)x/sanction/simulation/genesis.go(1 hunks)x/trigger/simulation/genesis.go(1 hunks)
🧰 Additional context used
🧠 Learnings (35)
📓 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/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.
📚 Learning: 2025-12-10T23:09:58.272Z
Learnt from: iramiller
Repo: provenance-io/provenance PR: 2568
File: .changelog/unreleased/features/2568-add-provlabs-wasm-contract-to-upgrade.md:0-0
Timestamp: 2025-12-10T23:09:58.272Z
Learning: In the provenance-io/provenance repo, ProvLabs is the parent entity and Nuva is the client. In formal contexts (changelogs, documentation, and log messages), reference ProvLabs as the entity providing support for the code contribution, and avoid using Nuva in these contexts.
Applied to files:
.changelog/unreleased/features/2498-sanction-module-collections.md
📚 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/sanction/keeper/genesis_test.goapp/app.gox/sanction/keeper/keys.gox/sanction/keeper/keeper_test.gox/sanction/keeper/export_test.gox/sanction/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: 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/sanction/keeper/genesis_test.goapp/app.gox/sanction/keeper/keeper_test.gox/sanction/keeper/export_test.gox/sanction/keeper/keeper.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/sanction/keeper/genesis_test.gox/attribute/client/cli/tx.gox/sanction/keeper/keeper_test.gox/sanction/keeper/gov_hooks.go
📚 Learning: 2024-10-02T01:58:51.519Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/expected_keepers.go:39-49
Timestamp: 2024-10-02T01:58:51.519Z
Learning: - In v0.50.x of the SDK, they have started the transition to using `context.Context` instead of `sdk.Context`, and `sdk.Context` satisfies `context.Context`.
Applied to files:
x/sanction/keeper/genesis_test.gox/sanction/keeper/export_test.go
📚 Learning: 2024-10-02T02:51:17.452Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/bank_test.go:81-91
Timestamp: 2024-10-02T02:51:17.452Z
Learning: In test helper functions like `setBalances` in `x/metadata/keeper/bank_test.go`, it's acceptable to handle zero or negative amounts without raising an error, as it suits the test's purposes.
Applied to files:
x/sanction/keeper/genesis_test.gox/sanction/keeper/keeper_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/sanction/keeper/genesis_test.goapp/app.gox/ledger/keeper/genesis.gox/sanction/keeper/keeper_test.gox/sanction/keeper/export_test.gox/sanction/keeper/keeper.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: Taztingo
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The user has updated the governance parameter structure in `cmd/provenanced/cmd/init.go` to `govGenState.Params.MinDeposit` and added a TODO to verify this change as it relates to a different task.
Applied to files:
x/sanction/keeper/genesis_test.gox/attribute/client/cli/tx.gox/sanction/simulation/genesis.gox/sanction/keeper/keeper_test.gox/sanction/keeper/gov_hooks.gox/marker/client/cli/tx.gox/sanction/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 1868
File: x/exchange/keeper/payments.go:24-34
Timestamp: 2024-10-08T18:12:51.936Z
Learning: The `parsePaymentStoreValue` function in `x/exchange/keeper/payments.go` returns `nil, nil` when the input byte slice is empty, as a design decision to simplify handling for the caller. This behavior is based on the rationale provided by a user, emphasizing ease of use over explicit error signaling for empty inputs.
Applied to files:
x/sanction/keeper/genesis_test.gox/sanction/keeper/keeper_test.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/attribute/client/cli/tx.gox/exchange/client/cli/flags.gox/marker/types/si.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/attribute/client/cli/tx.goapp/app.gox/ledger/keeper/genesis.gox/sanction/keeper/keys.gox/sanction/keeper/keeper_test.gox/ledger/client/cli/tx.gox/sanction/keeper/export_test.gox/marker/client/cli/tx.gox/sanction/keeper/keeper.go
📚 Learning: 2024-10-21T17:12:02.352Z
Learnt from: iramiller
Repo: provenance-io/provenance PR: 2194
File: cmd/provenanced/cmd/root.go:225-225
Timestamp: 2024-10-21T17:12:02.352Z
Learning: When adding flags in `cmd/provenanced/cmd/root.go`, use `flags.AddTxFlagsToCmd(cmd)` for each command to add the appropriate flags, instead of individually adding flags to the root command.
Applied to files:
x/exchange/client/cli/flags.go
📚 Learning: 2025-08-15T19:49:19.255Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/genesis.go:273-283
Timestamp: 2025-08-15T19:49:19.255Z
Learning: SpicyLemon prefers using strconv.ParseInt with explicit bit size parameter (e.g., strconv.ParseInt(str, 10, 32)) over strconv.Atoi followed by manual bounds checking when converting to specific integer types like int32, as it's more idiomatic and automatically enforces bounds.
Applied to files:
x/exchange/client/cli/flags.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/sanction/simulation/genesis.gox/sanction/keeper/keeper_test.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/sanction/keeper/keys.gox/sanction/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/sanction/keeper/keys.gointernal/protocompat/protocompat.gox/sanction/codec.gox/sanction/keeper/grpc_query.gox/sanction/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/mocks_test.go:425-427
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In this codebase, when using `sdk.AccAddress` as map keys, casting to `string` is preferred over calling `.String()` for performance reasons.
Applied to files:
x/sanction/keeper/keys.gox/sanction/codec.gox/sanction/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/sanction/keeper/genesis.go
📚 Learning: 2024-10-02T02:29:56.018Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils.go:138-143
Timestamp: 2024-10-02T02:29:56.018Z
Learning: In this codebase, methods like `GetAcc()` and `GetSignerAcc()` are intended to return `nil` if the address is invalid, and address validation is performed elsewhere. These methods should return a single value without an error.
Applied to files:
x/sanction/keeper/genesis.gox/sanction/keeper/keeper.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/sanction/keeper/suite_test.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/sanction/keeper/suite_test.gox/sanction/keeper/keeper_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/sanction/keeper/suite_test.gox/sanction/keeper/keeper_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/sanction/keeper/keeper_test.go
📚 Learning: 2024-10-10T23:08:15.256Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2175
File: .golangci.yml:87-87
Timestamp: 2024-10-10T23:08:15.256Z
Learning: The `depguard` definitions in `.golangci.yml` are prefixes (unless they end with `$`), so the entry `- cosmossdk.io/client/v2` covers imports from `cosmossdk.io/client/v2` and its subpackages like `autocli`. Changing imports from `cosmossdk.io/client/v2/autocli` to `cosmossdk.io/client/v2` in `app/app.go` would result in unresolved references because the required code resides in the `autocli` package.
Applied to files:
x/sanction/keeper/keeper_test.gox/sanction/keeper/grpc_query.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/sanction/keeper/keeper_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 `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/sanction/keeper/keeper_test.gox/sanction/keeper/keeper.go
📚 Learning: 2024-10-02T17:24:43.269Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/mocks_test.go:579-589
Timestamp: 2024-10-02T17:24:43.269Z
Learning: When mocking methods like `MintCoins` and `BurnCoins` in tests, using slices to store error strings and consuming them sequentially is the preferred approach, as it offers precise control and simplicity without unnecessary complexity.
Applied to files:
x/sanction/keeper/keeper_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/sanction/keeper/keeper_test.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils_test.go:1837-1889
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In Go tests where test cases are defined within a test case slice and iterated with `t.Run`, instances defined in the test case struct are not shared between subtests, ensuring test independence.
Applied to files:
x/sanction/keeper/keeper_test.go
📚 Learning: 2024-10-04T16:17:48.021Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2167
File: scripts/protocgen.sh:13-14
Timestamp: 2024-10-04T16:17:48.021Z
Learning: In `scripts/protocgen.sh`, the regex pattern `'option go_package.*provenance'` is sufficient for selecting the necessary proto files and does not need modification.
Applied to files:
internal/protocompat/protocompat.go
📚 Learning: 2025-09-19T19:57:02.914Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/entries.go:62-71
Timestamp: 2025-09-19T19:57:02.914Z
Learning: In Cosmos SDK v0.50.x, sdk.Context satisfies the context.Context interface and can be passed directly to methods expecting context.Context without wrapping via sdk.WrapSDKContext().
Applied to files:
x/sanction/keeper/export_test.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils.go:306-308
Timestamp: 2024-10-08T18:12:51.936Z
Learning: When using `sdk.AccAddress` as map keys in `AuthzCache`, casting to `string(addr)` is acceptable and preferred over `addr.String()` due to performance considerations and the keys being used internally.
Applied to files:
x/sanction/codec.gox/sanction/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: 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/sanction/codec.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/sanction/keeper/keeper.go
🧬 Code graph analysis (8)
x/sanction/keeper/genesis_test.go (2)
x/sanction/keeper/keeper.go (1)
Keeper(19-50)x/sanction/keeper/keys.go (1)
ParamNameImmediateUnsanctionMinDeposit(35-35)
app/app.go (2)
x/sanction/keeper/keeper.go (1)
NewKeeper(52-124)x/sanction/keys.go (1)
StoreKey(8-8)
x/sanction/simulation/genesis.go (1)
x/sanction/sanction.pb.go (4)
TEMP_STATUS_SANCTIONED(37-37)TemporaryEntry(124-131)TemporaryEntry(135-135)TemporaryEntry(136-138)
x/sanction/keeper/suite_test.go (1)
x/sanction/keeper/keeper.go (1)
Keeper(19-50)
x/sanction/keeper/keeper_test.go (6)
x/sanction/keeper/keeper.go (1)
Keeper(19-50)x/sanction/keeper/keys.go (7)
ParamNameImmediateSanctionMinDeposit(34-34)ParamNameImmediateUnsanctionMinDeposit(35-35)CreateSanctionedAddrKey(85-87)CreateTemporaryKey(111-118)CreateProposalTempIndexKey(185-187)SanctionB(129-129)UnsanctionB(131-131)x/sanction/codec.go (3)
AccAddressKey(26-26)TemporaryKeyCodec(93-93)ProposalIndexKeyCodec(190-190)app/app.go (1)
App(243-324)x/sanction/sanction.pb.go (1)
TEMP_STATUS_SANCTIONED(37-37)x/ibchooks/ibc_middleware_test.go (1)
Status(41-45)
x/sanction/keeper/export_test.go (1)
x/sanction/keeper/keeper.go (1)
Keeper(19-50)
x/sanction/codec.go (1)
internal/provutils/pair.go (1)
Pair(6-9)
x/marker/types/si.go (1)
x/marker/types/si.pb.go (2)
SIPrefix(25-25)SIPrefix(124-126)
⏰ 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 (00)
- GitHub Check: tests (01)
- GitHub Check: test-race (03)
- GitHub Check: tests (03)
- GitHub Check: tests (04)
- GitHub Check: test-race (01)
- GitHub Check: tests (00)
- GitHub Check: test-race (04)
- GitHub Check: test-race (02)
- GitHub Check: tests (02)
- GitHub Check: build-linux
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: ghcr-publish
- GitHub Check: heighliner-docker
- GitHub Check: docker
🔇 Additional comments (39)
x/sanction/simulation/genesis.go (1)
47-47: LGTM! Appropriate lint suppression for simulation code.The
nolint:gosecdirectives are correctly applied with clear justification. SinceInt63nreturns positive values and the offsets ensure the results stay well within bothint64anduint64ranges, the casts are safe. The comments accurately explain that this is intentional simulation randomness.Also applies to: 53-53
internal/protocompat/protocompat.go (1)
8-8: LGTM!Removing the lint directive is appropriate—the import is legitimate and widely used in the codebase for proto compatibility handling.
x/marker/types/si.go (1)
127-134: LGTM!The bounds check on lines 127-129 ensures
expis within int32 range before the conversion on line 130, making the nolint directive unnecessary.x/marker/client/cli/tx.go (2)
1468-1482: LGTM!The
strconv.ParseUint(args[5], 10, 32)on line 1468 already constrains the value to fit within uint32, making the conversion on line 1482 safe without a lint directive.
1521-1535: LGTM!Similar to the earlier conversion,
strconv.ParseUint(args[5], 10, 32)on line 1521 ensures the value fits within uint32, making the conversion on line 1535 safe.x/attribute/client/cli/tx.go (1)
342-347: LGTM!The
strconv.ParseUint(args[0], 10, 32)on line 342 constrains the value to uint32 range, making the conversion on line 346 safe.x/ledger/client/cli/tx.go (4)
64-77: LGTM!The
strconv.ParseInt(args[3], 10, 32)ensures the value fits within int32 range, making the conversion safe.
315-325: LGTM!The
strconv.ParseInt(args[1], 10, 32)on line 315 ensures safe conversion to int32.
355-365: LGTM!The
strconv.ParseInt(args[1], 10, 32)on line 355 ensures safe conversion to int32.
395-405: LGTM!The
strconv.ParseInt(args[1], 10, 32)on line 395 ensures safe conversion to int32.x/sanction/keeper/gov_hooks.go (1)
100-102: Improved error handling with better context.The change adds proper error checking for
DeleteGovPropTempEntriesand panics with a descriptive error message that includes the proposal ID. This is consistent with the error handling pattern used elsewhere in the function (lines 87-88) and provides better debugging information if the operation fails.x/exchange/client/cli/flags.go (2)
336-340: LGTM!The
strconv.ParseUint(args[0], 10, 32)on line 336 constrains the value to uint32 range, making the conversion on line 340 safe.
588-595: LGTM!The
strconv.ParseUint(amountStr, 10, 32)on line 588 ensures the value fits within uint32 range, making the conversion on line 595 safe.x/ledger/keeper/genesis.go (3)
181-186: LGTM!The
strconv.ParseInt(l.Key.P2, 10, 32)on line 181 ensures the value fits within int32 range before the conversion on line 186.
197-202: LGTM!The
strconv.ParseInt(l.Key.P2, 10, 32)on line 197 ensures safe conversion to int32 on line 202.
213-218: LGTM!The
strconv.ParseInt(l.Key.P2, 10, 32)on line 213 ensures safe conversion to int32 on line 218.x/name/client/cli/tx.go (1)
275-277: LGTM! Safe uint32 conversions.The removal of
nolint:goseccomments is appropriate here. Thestrconv.ParseUintcalls withbitSize: 32ensure the values are within uint32 range before conversion, making these casts safe.x/sanction/codec.go (3)
25-89: LGTM! Well-implemented AccAddressKey codec.The codec correctly implements length-prefixed encoding for
sdk.AccAddress:
- Buffer size validation in Encode/Decode
- Proper length-prefix handling (1 byte for length)
- JSON marshaling via Bech32 strings
- Non-terminal methods properly delegate to terminal variants
91-186: LGTM! TemporaryKeyCodec properly handles composite keys.The codec correctly implements encoding for
Pair[sdk.AccAddress, uint64]:
- Proper buffer validation before reads/writes
- BigEndian encoding for proposal ID ensures correct ordering
- JSON representation is clear with named fields
EncodePrefixhelper enables efficient prefix queries by address
188-276: LGTM! ProposalIndexKeyCodec correctly inverts key order.The codec correctly implements encoding for
Pair[uint64, sdk.AccAddress]with proposal ID first:
- Proposal ID as leading component enables efficient queries by proposal
- Proper buffer validation throughout
- JSON representation matches binary ordering
- Consistent implementation style with other codecs
.changelog/unreleased/features/2498-sanction-module-collections.md (1)
1-1: LGTM! Clear and concise changelog entry.The entry accurately describes the migration and properly references the related issue.
app/app.go (1)
729-731: LGTM! Correct KVStoreService initialization.The change properly wraps the store key with
runtime.NewKVStoreServiceto match the updatedNewKeepersignature that expects astore.KVStoreServiceinstead of a raw*storetypes.KVStoreKey. This aligns with the collection-based storage migration.x/sanction/keeper/genesis_test.go (1)
535-535: LGTM! Updated to context-based DeleteParam API.The change from passing
storetos.SdkCtxaligns with the migration to context-based keeper APIs, where methods now acceptsdk.Contextinstead of direct store references.x/sanction/keeper/genesis.go (2)
68-73: LGTM! Proper error handling for collection-based iteration.The addition of error handling for
IterateSanctionedAddressesis appropriate. Panicking on iteration errors in genesis export is acceptable since these are critical operations that should not fail silently.
81-94: LGTM! Consistent error handling pattern.The error handling mirrors the pattern in
GetAllSanctionedAddresses, maintaining consistency across genesis export functions. Panicking on iteration errors is appropriate in this context.x/sanction/keeper/suite_test.go (3)
7-7: LGTM! Proper setup validation.The import addition supports the new store types, and the
StoreServicenil-check ensures the keeper is properly initialized before tests execute. Good defensive programming.Also applies to: 64-64
73-75: LGTM! Updated to StoreService-based access.The change from
s.SdkCtx.KVStore(s.Keeper.StoreKey())tos.Keeper.StoreService.OpenKVStore(s.SdkCtx)correctly reflects the migration to the StoreService pattern. The return type update tostore.KVStorematches the new implementation.
81-82: LGTM! Proper error handling for Iterator.The addition of error capture and assertion for
store.Iteratoris correct. This ensures the test fails early if iteration cannot be started, preventing misleading test results.x/sanction/keeper/keys.go (1)
6-6: LGTM! Prefix migration to collections types.The change from
[]bytetocollections.Prefixaligns with the collection-based storage migration while preserving the prefix values (0, 1, 2, 3) for backward compatibility. Thecollections.Prefixtype integrates seamlessly with the existing key construction functions.Also applies to: 27-30
x/sanction/keeper/grpc_query.go (2)
236-252: LGTM -prefixEndBytescorrectly handles prefix range termination.The implementation correctly increments bytes from the end to find the exclusive end of the prefix range, handling byte overflow (wrapping to 0) by continuing to the next byte. Returning
nilfor all-0xFF prefix is correct as it means "iterate to the end."
44-65: LGTM - Collection-based pagination for sanctioned addresses.The migration to
CollectionPaginateis correctly implemented, initializing the response with an empty slice and properly handling the results and pagination response.x/sanction/keeper/keeper_test.go (4)
870-884: Test setup uses direct store access while testing context-based API.The test correctly sets up data using the old KVStore pattern (lines 879-883) to verify backward compatibility, which is intentional based on the test name and purpose. The test then correctly uses the context-based
GetLatestTempEntry(s.SdkCtx, ...)to verify the new API can read old data.
2389-2431: Good backward compatibility test for key layouts.These tests verify that the new Collections codecs produce identical keys to the old KVStore approach, which is crucial for ensuring no state migration is required. The test correctly compares:
- Sanctioned address keys
- Temporary entry keys
- Proposal index keys
2434-2469: Good backward compatibility test for reading old data.This test writes data using the old KVStore pattern and verifies that Collections-based accessors can read it correctly. This is essential for the migration requirement of "no state migration needed."
2498-2558: Comprehensive complex scenario test.This test validates a realistic chain state scenario with mixed permanent and temporary sanctions, verifying that
IsSanctionedAddr,GetAllSanctionedAddresses, andGetAllTemporaryEntrieswork correctly with data written using the old pattern.x/sanction/keeper/export_test.go (1)
73-90: LGTM - Test helpers properly updated to context-based API.The test helper methods (
GetLatestTempEntry,GetParam,SetParam,DeleteParam) are correctly updated to acceptsdk.Contextand delegate to the corresponding private context-based methods.x/sanction/keeper/keeper.go (3)
419-438: Behavior change:SetParams(nil)now sets defaults instead of deleting.The previous behavior (per the comment on line 418) was that nil params would delete the params so defaults are used. The new behavior explicitly sets the default values in the store.
This is semantically different:
- Before: Params not in store →
GetParamsreturns defaults- Now: Params in store with default values →
GetParamsreturns those valuesWhile functionally equivalent for now, this could cause issues if default values change in the future (stored values would remain, not reflect new defaults).
Please verify this behavior change is intentional and aligns with the module's requirements.
52-124: LGTM - NewKeeper properly initializes collections-based storage.The keeper initialization correctly:
- Creates a SchemaBuilder
- Initializes all collection Maps with appropriate prefixes and codecs
- Builds and assigns the schema
- Appends the send restriction to the bank keeper
344-379: All callers of these iteration methods handle errors, but via panic—consider more graceful error handling.The two methods that have production callers—
IterateSanctionedAddressesandIterateTemporaryEntries—are both called inx/sanction/keeper/genesis.go(lines 68 and 81), and both properly check the error return withif err := method(...); err != nil { panic(...) }. However, converting returned errors to panics in export genesis functions undermines the purpose of returning errors rather than using must-style functions. Consider handling errors more gracefully in genesis operations. Additionally,IterateProposalIndexEntrieshas no production callers and its error return is unused in test code; verify whether this method is needed or should be removed.
nagarajdivine
force-pushed
the
feat/sanction-module-collections
branch
from
346654e to
1c4db62
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
x/sanction/keeper/grpc_query.go (1)
164-191: Total count is still incorrect when using key-based pagination.The issue raised in the previous review remains unresolved. When
startPropIDis set (key-based pagination), the code incrementstotalfor skipped entries (lines 168-171, 175-178) and again for entries that pass the filter (lines 182-184). Thetotalshould represent all entries for the address, counted once per entry, before applying the pagination filter.Apply this fix to count all entries once before filtering:
for ; iter.Valid(); iter.Next() { kv, err := iter.KeyValue() if err != nil { return nil, status.Error(codes.Internal, err.Error()) } pid := kv.Key.K2() + + // Count all entries for this address regardless of pagination + if countTotal { + total++ + } // :white_check_mark: FIX: Handle both forward and reverse key pagination if startPropID != nil { if !reverse && pid < *startPropID { // Forward mode: skip entries before startPropID - if countTotal { - total++ - } continue } if reverse && pid > *startPropID { // Reverse mode: skip entries after startPropID - if countTotal { - total++ - } continue } } - if countTotal { - total++ - } - items = append(items, &sanction.TemporaryEntry{x/sanction/keeper/keeper.go (1)
232-263: Performance issue:getLatestTempEntrystill walks the entire store.The issue raised in the previous review remains unresolved. This method walks all temporary entries in the store (O(n) across all addresses) and is called from
IsSanctionedAddr, a critical hot path invoked on every transaction. Use prefix-based iteration with reverse ordering to scan only entries for the specific address.Apply this fix to use prefix-based reverse iteration:
func (k Keeper) getLatestTempEntry(ctx sdk.Context, addr sdk.AccAddress) []byte { if len(addr) == 0 { return nil } - - var latestValue []byte - var latestPropID uint64 - found := false - - // Walk all temporary entries and find the one for this address with highest propID - err := k.TemporaryEntriesStore.Walk(ctx, nil, - func(key collections.Pair[sdk.AccAddress, uint64], value []byte) (stop bool, err error) { - // Only consider entries for this address - if !key.K1().Equals(addr) { - return false, nil - } - - propID := key.K2() - if !found || propID > latestPropID { - latestPropID = propID - latestValue = value - found = true - } - return false, nil - }) - - if err != nil { - return nil - } - - return latestValue + + // Use prefix-based iteration with reverse order to get the latest (highest propID) entry first + prefixBytes := make([]byte, 1+len(addr)) + prefixBytes[0] = byte(len(addr)) + copy(prefixBytes[1:], addr) + + prefixEnd := prefixEndBytes(prefixBytes) + + iter, err := k.TemporaryEntriesStore.IterateRaw(ctx, prefixBytes, prefixEnd, collections.OrderDescending) + if err != nil { + return nil + } + defer iter.Close() + + if iter.Valid() { + kv, err := iter.KeyValue() + if err != nil { + return nil + } + return kv.Value + } + + return nil }Note: You'll need to add a
prefixEndByteshelper (similar to the one ingrpc_query.go) to this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
x/sanction/keeper/genesis.go(2 hunks)x/sanction/keeper/gov_hooks.go(1 hunks)x/sanction/keeper/grpc_query.go(4 hunks)x/sanction/keeper/keeper.go(7 hunks)x/sanction/simulation/genesis.go(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 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: Taztingo
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The user has updated the governance parameter structure in `cmd/provenanced/cmd/init.go` to `govGenState.Params.MinDeposit` and added a TODO to verify this change as it relates to a different task.
Applied to files:
x/sanction/keeper/gov_hooks.gox/sanction/keeper/keeper.gox/sanction/simulation/genesis.go
📚 Learning: 2024-10-10T23:08:15.256Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2175
File: .golangci.yml:87-87
Timestamp: 2024-10-10T23:08:15.256Z
Learning: The `depguard` definitions in `.golangci.yml` are prefixes (unless they end with `$`), so the entry `- cosmossdk.io/client/v2` covers imports from `cosmossdk.io/client/v2` and its subpackages like `autocli`. Changing imports from `cosmossdk.io/client/v2/autocli` to `cosmossdk.io/client/v2` in `app/app.go` would result in unresolved references because the required code resides in the `autocli` package.
Applied to files:
x/sanction/keeper/grpc_query.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/sanction/keeper/grpc_query.gox/sanction/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/sanction/keeper/genesis.go
📚 Learning: 2024-10-02T02:29:56.018Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils.go:138-143
Timestamp: 2024-10-02T02:29:56.018Z
Learning: In this codebase, methods like `GetAcc()` and `GetSignerAcc()` are intended to return `nil` if the address is invalid, and address validation is performed elsewhere. These methods should return a single value without an error.
Applied to files:
x/sanction/keeper/genesis.gox/sanction/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/sanction/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/sanction/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: 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/sanction/keeper/keeper.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/sanction/keeper/keeper.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/sanction/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils.go:306-308
Timestamp: 2024-10-08T18:12:51.936Z
Learning: When using `sdk.AccAddress` as map keys in `AuthzCache`, casting to `string(addr)` is acceptable and preferred over `addr.String()` due to performance considerations and the keys being used internally.
Applied to files:
x/sanction/keeper/keeper.go
📚 Learning: 2024-10-02T16:55:10.172Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/mocks_test.go:425-427
Timestamp: 2024-10-02T16:55:10.172Z
Learning: In this codebase, when using `sdk.AccAddress` as map keys, casting to `string` is preferred over calling `.String()` for performance reasons.
Applied to files:
x/sanction/keeper/keeper.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/sanction/keeper/keeper.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/sanction/simulation/genesis.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/sanction/simulation/genesis.go
🧬 Code graph analysis (2)
x/sanction/keeper/grpc_query.go (5)
x/sanction/query.pb.go (6)
QuerySanctionedAddressesResponse(171-176)QuerySanctionedAddressesResponse(180-180)QuerySanctionedAddressesResponse(181-183)QueryTemporaryEntriesResponse(281-285)QueryTemporaryEntriesResponse(289-289)QueryTemporaryEntriesResponse(290-292)internal/provutils/pair.go (1)
Pair(6-9)x/sanction/sanction.pb.go (3)
TemporaryEntry(124-131)TemporaryEntry(135-135)TemporaryEntry(136-138)x/sanction/keeper/keys.go (1)
ToTempStatus(145-155)x/sanction/keeper/keeper.go (1)
Keeper(19-50)
x/sanction/keeper/genesis.go (2)
x/sanction/keeper/keeper.go (1)
Keeper(19-50)x/sanction/sanction.pb.go (3)
TemporaryEntry(124-131)TemporaryEntry(135-135)TemporaryEntry(136-138)
🪛 GitHub Actions: Lint
x/sanction/simulation/genesis.go
[error] 47-47: golangci-lint run: directive //nolint:gosec // G115: Int63n used for simulation randomness; casting to uint64 is safe and intentional. is unused for linter "gosec" (nolintlint)
🪛 GitHub Check: golangci-lint
x/sanction/simulation/genesis.go
[failure] 53-53:
directive //nolint:gosec // G115: Int63n used for simulation randomness; casting to uint64 is safe and intentional. is unused for linter "gosec" (nolintlint)
[failure] 47-47:
directive //nolint:gosec // G115: Int63n used for simulation randomness; casting to uint64 is safe and intentional. is unused for linter "gosec" (nolintlint)
⏰ 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). (16)
- GitHub Check: tests (04)
- GitHub Check: test-race (02)
- GitHub Check: tests (02)
- GitHub Check: tests (00)
- GitHub Check: tests (01)
- GitHub Check: tests (03)
- GitHub Check: test-race (01)
- GitHub Check: test-race (00)
- GitHub Check: test-race (03)
- GitHub Check: test-race (04)
- GitHub Check: build-linux
- GitHub Check: Build OSX
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: heighliner-docker
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
x/sanction/keeper/gov_hooks.go (1)
100-102: LGTM! Proper error handling for collections-based deletion.The error handling for
DeleteGovPropTempEntriesis appropriate. Panicking with a descriptive error message in a governance hook context ensures critical failures don't go unnoticed.x/sanction/keeper/genesis.go (2)
68-73: LGTM! Proper error handling for collections-based iteration.The error handling for
IterateSanctionedAddressesis appropriate. Panicking with a descriptive error message during genesis export ensures critical failures are caught.
81-94: LGTM! Proper error handling for collections-based iteration.The error handling for
IterateTemporaryEntriesis appropriate. Panicking with a descriptive error message during genesis export ensures critical failures are caught.x/sanction/keeper/grpc_query.go (5)
44-66: LGTM! Clean migration to collection-based pagination.The refactoring to use
query.CollectionPaginateoverk.SanctionedAddressesStoreis clean and properly handles errors. The response construction correctly initializes theAddressesslice to avoid nil values.
83-109: LGTM! Well-structured conditional query handling.The conditional logic cleanly separates address-filtered queries (delegated to
temporaryEntriesForAddress) from unfiltered queries (using standardCollectionPaginate). The unfiltered path properly constructs the response with error handling.
112-163: LGTM! Proper pagination setup and prefix construction.The pagination parameter parsing and prefix construction correctly match the
TemporaryKeyCodecformat. The handling ofpagination.Keyas an 8-byte uint64 for key-based pagination is appropriate.
193-226: LGTM! Correct post-processing and response construction.The reverse ordering, offset/limit slicing, and
NextKeycalculation are correctly implemented. The response properly constructs the pagination metadata (aside from thetotalcounting issue flagged separately).
236-252: LGTM! Standard prefix range helper.The
prefixEndByteshelper correctly computes the end boundary for prefix-based iteration by incrementing bytes from right to left.x/sanction/keeper/keeper.go (7)
52-124: LGTM! Well-structured collections migration in NewKeeper.The refactoring to use
store.KVStoreServiceand collections-based storage is well-implemented. The schema builder pattern with custom key codecs ensures backward compatibility. The panic on schema build failure is appropriate for constructor-level errors.
132-150: LGTM! Proper collections-based sanctioned address check.The logic correctly checks temporary entries first, then permanent sanctions using
SanctionedAddressesStore.Has. The error handling appropriately returns false on errors (aside from the performance concern ingetLatestTempEntryflagged separately).
154-175: LGTM! Proper error handling in SanctionAddresses.The collections-based updates are properly wrapped with error handling and descriptive error messages. The error propagation pattern is consistent throughout.
177-195: LGTM! Consistent error handling in UnsanctionAddresses.The collections-based removals properly handle errors with descriptive messages. The pattern mirrors
SanctionAddressesappropriately.
207-229: LGTM! Proper collections-based temporary entry addition.The function correctly updates both
TemporaryEntriesStoreandProposalIndexwith proper error handling. The use ofcollections.Joinfor composite keys is appropriate.
266-342: LGTM! Proper collections-based deletion with error handling.Both deletion methods correctly use the collect-then-delete pattern to avoid iterator invalidation. The error handling and propagation are appropriate with descriptive error messages.
346-458: LGTM! Collections-based iteration and parameter methods.The iteration methods correctly use
Walkwith appropriate error returns. The parameter methods properly interact withParameterStoreusing collection-based gets and sets with error handling. TheGetParamsmethod appropriately handles missing parameters by falling back to defaults.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/sanction/simulation/genesis.go(1 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.
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.
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: 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-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/sanction/simulation/genesis.go
📚 Learning: 2024-10-02T03:23:58.504Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/scope_test.go:1355-1364
Timestamp: 2024-10-02T03:23:58.504Z
Learning: In `scope_test.go`, numeric literals are intentionally used for the `Role` field in tests instead of named constants. This approach keeps the lines shorter and reduces false positives when searching for usages of specific party types, as the tests do not depend on the actual names of the roles.
Applied to files:
x/sanction/simulation/genesis.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/sanction/simulation/genesis.go
📚 Learning: 2024-10-10T23:18:06.683Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades.go:221-227
Timestamp: 2024-10-10T23:18:06.683Z
Learning: In the Go codebase, assigning functions to the blank identifier `_` is an acceptable practice to prevent linter warnings about unused functions. This approach is preferred over using `nolint` directives because it avoids linter complaints about unused `nolint` directives when the functions become used again.
Applied to files:
x/sanction/simulation/genesis.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: Taztingo
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The user has updated the governance parameter structure in `cmd/provenanced/cmd/init.go` to `govGenState.Params.MinDeposit` and added a TODO to verify this change as it relates to a different task.
Applied to files:
x/sanction/simulation/genesis.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/sanction/simulation/genesis.go
🪛 GitHub Actions: Lint
x/sanction/simulation/genesis.go
[error] 42-42: golangci-lint: directive //nolint:gosec is unused for linter "gosec" (nolintlint)
🪛 GitHub Check: golangci-lint
x/sanction/simulation/genesis.go
[failure] 42-42:
directive //nolint:gosec is unused for linter "gosec" (nolintlint)
⏰ 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 (02)
- GitHub Check: test-race (01)
- GitHub Check: test-race (03)
- GitHub Check: test-race (00)
- GitHub Check: tests (03)
- GitHub Check: test-race (04)
- GitHub Check: test-race (02)
- GitHub Check: tests (00)
- GitHub Check: tests (04)
- GitHub Check: tests (01)
- GitHub Check: build-linux
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: heighliner-docker
- GitHub Check: Build OSX
- GitHub Check: Build Linux
- GitHub Check: Analyze (go)
nagarajdivine
force-pushed
the
feat/sanction-module-collections
branch
from
1707318 to
6f2d100
Compare
nagarajdivine
force-pushed
the
feat/sanction-module-collections
branch
from
6f2d100 to
9c0785b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
x/sanction/keeper/suite_test.go (2)
96-98:⚠️ Potential issue | 🟡 Minor
store.Delete(key)error is silently ignored.
store.KVStore.Delete(fromcosmossdk.io/core/store) returns anerror. Ignoring it meansClearStatemay silently fail to delete entries, causing stale state to leak between tests.Proposed fix
for _, key := range keysToDelete { - store.Delete(key) + s.Require().NoError(store.Delete(key), "store.Delete") }
181-186:⚠️ Potential issue | 🟡 Minor
ReqOKDelPropTempdoesn't check the error returned byDeleteGovPropTempEntries.Per
gov_hooks.go(line 100),DeleteGovPropTempEntriesnow returns an error. This test helper only checks for panics but ignores the returned error. Consider usingRequireNotPanicsNoErrorinstead.Proposed fix
func (s *BaseTestSuite) ReqOKDelPropTemp(id uint64) { - s.Require().NotPanics(func() { - s.Keeper.DeleteGovPropTempEntries(s.SdkCtx, id) - }, "DeleteGovPropTempEntries(%d)", id) + s.T().Helper() + s.RequireNotPanicsNoError(func() error { + return s.Keeper.DeleteGovPropTempEntries(s.SdkCtx, id) + }, "DeleteGovPropTempEntries(%d)", id) }x/sanction/keeper/keeper_test.go (1)
878-884:⚠️ Potential issue | 🟡 Minor
store.Set(...)errors are silently discarded.
s.GetStore()now returnscosmossdk.io/core/store.KVStore, whoseSetmethod returns anerror. The ignored errors here could cause the test setup to silently fail, leading to misleading test results.Proposed fix
for _, id := range []uint64{18, 19, 55, 100000} { for _, addr := range []sdk.AccAddress{randAddr1, randAddr2, randAddr3, randAddr4} { val += 1 - store.Set(keeper.CreateTemporaryKey(addr, id), []byte{val}) - store.Set(keeper.CreateProposalTempIndexKey(id, addr), []byte{val}) + s.Require().NoError(store.Set(keeper.CreateTemporaryKey(addr, id), []byte{val}), "store.Set temp key") + s.Require().NoError(store.Set(keeper.CreateProposalTempIndexKey(id, addr), []byte{val}), "store.Set index key") } }
🤖 Fix all issues with AI agents
In `@x/sanction/keeper/export_test.go`:
- Around line 47-50: Update the stale doc comment to reflect the current
function name: change the comment above Keeper.KVStore from "StoreKey, for unit
tests, exposes this keeper's storekey." to something like "KVStore, for unit
tests, exposes this keeper's KV store." or similar; ensure the comment
references KVStore and describes that it returns the keeper's KV store by
calling k.StoreService.OpenKVStore(ctx).
In `@x/sanction/keeper/keeper.go`:
- Around line 419-425: The comment for Keeper.SetParams is inaccurate: when
params is nil the function writes default values into the store (using
sanction.Params with DefaultImmediateSanctionMinDeposit and
DefaultImmediateUnsanctionMinDeposit) rather than deleting stored params to rely
on implicit defaults. Update the docstring/comment for SetParams (and any
related public docs) to state that passing nil will persist default Params into
the store (initializing those fields) instead of removing entries, and mention
the exact defaults written (DefaultImmediateSanctionMinDeposit,
DefaultImmediateUnsanctionMinDeposit) so callers know the stored state after
calling SetParams(nil).
- Around line 144-149: In IsSanctionedAddr, don't silently swallow errors from
k.SanctionedAddressesStore.Has; instead log the error (using the keeper's
logger, e.g., k.Logger or equivalent) and return a conservative result (treat
the address as sanctioned) so an underlying store error cannot allow a
sanctioned address to transact; specifically, update the error branch after
k.SanctionedAddressesStore.Has(ctx, addr) to emit a clear error log including
the err and then return true (or propagate the error if the function signature
is changed to return an error).
- Around line 99-100: Fix the broken comment by joining the split word so it
reads a single coherent line like "// Proposal index: (uint64, AccAddress) ->
[]byte with custom codec" (replace the two lines "// Proposa" and "// l index"
with the single corrected comment) near the keeper struct or the related mapping
in keeper.go so the comment correctly documents the Proposal index.
- Around line 313-320: The current code uses TemporaryEntriesStore.Walk to scan
all entries and filters by key.K1().Equals(addr), causing O(n) work per address;
instead iterate only the entries for that address by using a prefix/range-based
iteration on TemporaryEntriesStore (e.g., WalkPrefix or an iterator created with
the address prefix) to enumerate keys for the given addr, appending to tempKeys
and indexKeys (use collections.Join(key.K2(), addr) as before) and remove the
per-entry equality check; update the block around TemporaryEntriesStore.Walk,
collections.Pair handling, and any code that builds tempKeys/indexKeys to use
the scoped prefix iterator so complexity becomes proportional to the number of
entries for that address rather than the whole store.
🧹 Nitpick comments (6)
x/sanction/codec.go (1)
39-53: Decoded address aliases the input buffer — consider copying.In
Decode, the returnedsdk.AccAddressis a sub-slice of the inputbuffer(line 51). If the caller ever reuses or mutates that buffer, the address will silently change. The same pattern repeats inTemporaryKeyCodec.Decode(line 123) andProposalIndexKeyCodec.Decode(line 221).This is typically safe within the collections framework but is a subtle footgun for any other caller.
🛡️ Defensive copy
- addr := sdk.AccAddress(buffer[1 : 1+addrLen]) + addr := make(sdk.AccAddress, addrLen) + copy(addr, buffer[1:1+addrLen])Apply similarly in
TemporaryKeyCodec.DecodeandProposalIndexKeyCodec.Decode.x/sanction/keeper/grpc_query.go (2)
146-151: All matching entries are loaded into memory before pagination.
temporaryEntriesForAddressiterates over all temporary entries for the address, collecting them intoitems, then applies offset/limit in memory. For addresses with many entries, this could be memory-intensive. In practice, the number of temporary entries per address is expected to be small, but it's worth noting.
234-252: ReplaceprefixEndByteswithstoretypes.PrefixEndBytesfrom the Cosmos SDK.The function duplicates
storetypes.PrefixEndBytesfromcosmossdk.io/store/types, which is already used throughout the codebase. Add the import and replace the function call at line 144 with the SDK utility.x/sanction/keeper/keeper.go (3)
357-366: Performance:IterateTemporaryEntriesshould use prefix range whenaddris provided.When a specific
addris passed, the Walk still scans all temporary entries. Use a prefix range to scope iteration:♻️ Suggested approach
func (k Keeper) IterateTemporaryEntries(ctx sdk.Context, addr sdk.AccAddress, cb func(addr sdk.AccAddress, govPropID uint64, isSanction bool) (stop bool)) error { - return k.TemporaryEntriesStore.Walk(ctx, nil, func(key collections.Pair[sdk.AccAddress, uint64], value []byte) (stop bool, err error) { - if len(addr) > 0 && !key.K1().Equals(addr) { - return false, nil - } - - isSanction := IsSanctionBz(value) - return cb(key.K1(), key.K2(), isSanction), nil - }) + var rng *collections.PairRange[sdk.AccAddress, uint64] + if len(addr) > 0 { + rng = collections.NewPrefixedPairRange[sdk.AccAddress, uint64](addr) + } + return k.TemporaryEntriesStore.Walk(ctx, rng, func(key collections.Pair[sdk.AccAddress, uint64], value []byte) (stop bool, err error) { + isSanction := IsSanctionBz(value) + return cb(key.K1(), key.K2(), isSanction), nil + }) }
371-379: Performance:IterateProposalIndexEntriesshould use prefix range whengovPropIDis provided.Same pattern — use
NewPrefixedPairRangeongovPropIDinstead of scanning everything.♻️ Suggested approach
func (k Keeper) IterateProposalIndexEntries(ctx sdk.Context, govPropID *uint64, cb func(govPropID uint64, addr sdk.AccAddress) (stop bool)) error { - return k.ProposalIndex.Walk(ctx, nil, func(key collections.Pair[uint64, sdk.AccAddress], _ []byte) (stop bool, err error) { - if govPropID != nil && key.K1() != *govPropID { - return false, nil - } - - return cb(key.K1(), key.K2()), nil - }) + var rng *collections.PairRange[uint64, sdk.AccAddress] + if govPropID != nil { + rng = collections.NewPrefixedPairRange[uint64, sdk.AccAddress](*govPropID) + } + return k.ProposalIndex.Walk(ctx, rng, func(key collections.Pair[uint64, sdk.AccAddress], _ []byte) (stop bool, err error) { + return cb(key.K1(), key.K2()), nil + }) }
270-278: UseNewPrefixedPairRangeto scopeProposalIndexwalk bygovPropID.Since
ProposalIndexis keyed by(govPropID, address), use a prefix range ongovPropIDto avoid scanning all entries. This pattern is already used elsewhere in the same module.♻️ Suggested approach
- err := k.ProposalIndex.Walk(ctx, nil, - func(key collections.Pair[uint64, sdk.AccAddress], _ []byte) (stop bool, err error) { - if key.K1() == govPropID { - addr := key.K2() - toRemove = append(toRemove, collections.Join(addr, govPropID)) - indexToRemove = append(indexToRemove, key) - } - return false, nil - }) + rng := collections.NewPrefixedPairRange[uint64, sdk.AccAddress](govPropID) + err := k.ProposalIndex.Walk(ctx, rng, + func(key collections.Pair[uint64, sdk.AccAddress], _ []byte) (stop bool, err error) { + addr := key.K2() + toRemove = append(toRemove, collections.Join(addr, govPropID)) + indexToRemove = append(indexToRemove, key) + return false, nil + })
nagarajdivine
force-pushed
the
feat/sanction-module-collections
branch
from
9c0785b to
c75e82e
Compare
…s backward compatible test cases.
nagarajdivine
force-pushed
the
feat/sanction-module-collections
branch
from
c75e82e to
b44d66d
Compare
1 participant
Description
closes: Switch to collections in the sanction module. #2498
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
Refactor
New Features
Bug Fixes
Tests