Migrate hold module from KV store to collections.#2554
Conversation
📝 WalkthroughWalkthroughThe hold module was migrated from direct KVStore access to collections-based storage: new codecs and a Schema-backed Map were added, the Keeper now accepts a KVStoreService and exposes Holds map, gRPC pagination was reworked to use in-memory collection processing, and tests/initializers were updated to the new store types and APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPC as gRPC Server
participant Keeper
participant StoreSvc as KVStoreService
participant Holds as Collections.Map
Client->>GRPC: GetAllHolds(request + pagination)
GRPC->>Keeper: GetAllHolds(ctx, req)
Keeper->>StoreSvc: open map / use Schema
StoreSvc->>Holds: access map API
Keeper->>Holds: iterate all entries (collect into memory)
Holds-->>Keeper: entries (addr, denom, amount) / per-entry errors
Keeper->>Keeper: apply offset/limit or startKey, compute NextKey
Keeper-->>GRPC: response(hands[], next_key, total, status)
GRPC-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/hold/keeper/keeper_test.go (1)
182-195: Error fromstore.Iteratoris not checked before use.The error returned from
store.Iteratoris captured but not checked before entering the loop. If iterator creation fails,itercould be nil, causing a panic when accessingiter.Valid().Apply this diff to check the error:
iter, err := store.Iterator(nil, nil) + s.Require().NoError(err, "store.Iterator(nil, nil)") defer func() { if iter != nil { iter.Close() } }()
🧹 Nitpick comments (3)
x/hold/codec.go (1)
159-168: Non-terminal methods forDenomKeyCodecare not safe if used non-terminally.Since
DenomKeyCodecuses raw bytes without a length prefix, theDecodeNonTerminalmethod will consume the entire remaining buffer. This only works correctly if the denom is always the last (terminal) part of a composite key.If the composite key structure is
(Address, Denom)where Address is first and Denom is terminal, this is fine. However, theNonTerminalmethods as implemented could be misleading or cause issues if someone attempts to useDenomKeyCodecas a non-terminal key component in the future.Consider either:
- Adding a comment clarifying this codec must only be used as the terminal key component, or
- Having
EncodeNonTerminal/DecodeNonTerminalreturn an error or panic to prevent misuse.x/hold/keeper/grpc_query.go (1)
168-173: Manual reversal adds O(n) time complexity.Consider using a reverse iterator if the collections library supports it (e.g.,
IterateReverseor passing a range with reverse ordering) instead of collecting all entries and reversing in-memory.x/hold/keeper/keeper.go (1)
34-34: Use camelCase for parameter names.Go convention is to use camelCase for parameter names.
store_serviceshould bestoreService.-func NewKeeper(cdc codec.BinaryCodec, store_service store.KVStoreService, accountKeeper hold.AccountKeeper, bankKeeper hold.BankKeeper) Keeper { +func NewKeeper(cdc codec.BinaryCodec, storeService store.KVStoreService, accountKeeper hold.AccountKeeper, bankKeeper hold.BankKeeper) Keeper {Also update the usage on line 35 and 48:
- sb := collections.NewSchemaBuilder(store_service) + sb := collections.NewSchemaBuilder(storeService)- StoreService: store_service, + StoreService: storeService,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changelog/unreleased/features/2419-hold-module-collections.md(1 hunks)app/app.go(1 hunks)x/hold/codec.go(2 hunks)x/hold/keeper/export_test.go(1 hunks)x/hold/keeper/genesis_test.go(5 hunks)x/hold/keeper/grpc_query.go(2 hunks)x/hold/keeper/grpc_query_test.go(6 hunks)x/hold/keeper/invariants_test.go(19 hunks)x/hold/keeper/keeper.go(6 hunks)x/hold/keeper/keeper_test.go(12 hunks)
🧰 Additional context used
🧠 Learnings (22)
📓 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: 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.
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: 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/2419-hold-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/hold/keeper/genesis_test.gox/hold/keeper/grpc_query.gox/hold/keeper/invariants_test.gox/hold/keeper/keeper_test.gox/hold/keeper/grpc_query_test.gox/hold/keeper/keeper.gox/hold/keeper/export_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/hold/keeper/genesis_test.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/hold/keeper/genesis_test.gox/hold/keeper/invariants_test.gox/hold/keeper/grpc_query_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/hold/keeper/genesis_test.go
📚 Learning: 2024-10-02T03:49:31.991Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/address_test.go:69-80
Timestamp: 2024-10-02T03:49:31.991Z
Learning: In the `subSet` function used in unit tests, it's acceptable for it to panic if an out-of-bounds index is provided, as this indicates a test case was set up incorrectly.
Applied to files:
x/hold/keeper/genesis_test.gox/hold/keeper/grpc_query_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/hold/keeper/genesis_test.gox/hold/keeper/grpc_query.gox/hold/keeper/invariants_test.gox/hold/keeper/keeper_test.gox/hold/keeper/grpc_query_test.gox/hold/keeper/keeper.gox/hold/keeper/export_test.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/mocks_test.go:579-589
Timestamp: 2024-10-08T18:12:51.935Z
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/hold/keeper/genesis_test.gox/hold/keeper/invariants_test.gox/hold/keeper/keeper_test.gox/hold/keeper/grpc_query_test.gox/hold/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:
app/app.gox/hold/keeper/grpc_query.gox/hold/keeper/grpc_query_test.gox/hold/keeper/keeper.gox/hold/keeper/export_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.gox/hold/keeper/export_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/hold/keeper/grpc_query.gox/hold/keeper/invariants_test.gox/hold/keeper/keeper_test.gox/hold/keeper/keeper.gox/hold/keeper/export_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: 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/hold/keeper/grpc_query.gox/hold/keeper/invariants_test.gox/hold/keeper/keeper_test.gox/hold/keeper/grpc_query_test.gox/hold/keeper/keeper.gox/hold/keeper/export_test.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/hold/codec.gox/hold/keeper/keeper_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/hold/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/hold/codec.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/hold/keeper/invariants_test.gox/hold/keeper/grpc_query_test.gox/hold/keeper/keeper.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/hold/keeper/invariants_test.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/hold/keeper/invariants_test.gox/hold/keeper/keeper_test.gox/hold/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 `WithVestingLockedBypass` function in `x/hold/keeper/invariants.go` is specifically used to handle scenarios involving vesting and locked coins deep within third-party library code. It follows a pattern used by other injectable functions to allow bypassing specific portions of those functions.
Applied to files:
x/hold/keeper/invariants_test.gox/hold/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/hold/keeper/keeper_test.gox/hold/keeper/export_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/hold/keeper/grpc_query_test.go
🧬 Code graph analysis (6)
x/hold/keeper/genesis_test.go (1)
x/hold/keeper/keeper_test.go (1)
TestSuite(33-50)
app/app.go (12)
x/marker/types/key.go (1)
StoreKey(17-17)x/ibchooks/types/keys.go (1)
StoreKey(5-5)x/quarantine/keys.go (1)
StoreKey(18-18)x/msgfees/types/keys.go (1)
StoreKey(8-8)x/name/types/keys.go (1)
StoreKey(17-17)x/attribute/types/keys.go (1)
StoreKey(19-19)x/sanction/keys.go (1)
StoreKey(8-8)x/ibcratelimit/keys.go (1)
StoreKey(8-8)x/oracle/types/keys.go (1)
StoreKey(10-10)x/exchange/keys.go (1)
StoreKey(16-16)x/hold/keys.go (1)
StoreKey(8-8)x/trigger/types/keys.go (1)
StoreKey(15-15)
x/hold/keeper/grpc_query.go (5)
internal/provutils/pair.go (1)
Pair(6-9)x/hold/query.pb.go (6)
GetAllHoldsRequest(115-118)GetAllHoldsRequest(122-122)GetAllHoldsRequest(123-125)GetAllHoldsResponse(154-159)GetAllHoldsResponse(163-163)GetAllHoldsResponse(164-166)x/hold/keeper/keeper.go (1)
Keeper(24-32)x/hold/hold.pb.go (3)
AccountHold(30-35)AccountHold(39-39)AccountHold(40-42)x/hold/codec.go (22)
AddressKeyCodec(56-56)AddressKeyCodec(58-71)AddressKeyCodec(73-84)AddressKeyCodec(86-88)AddressKeyCodec(90-92)AddressKeyCodec(94-100)AddressKeyCodec(102-104)AddressKeyCodec(106-108)AddressKeyCodec(110-112)AddressKeyCodec(114-116)AddressKeyCodec(118-120)DenomKeyCodec(123-123)DenomKeyCodec(125-131)DenomKeyCodec(133-135)DenomKeyCodec(137-139)DenomKeyCodec(141-143)DenomKeyCodec(145-149)DenomKeyCodec(151-153)DenomKeyCodec(155-157)DenomKeyCodec(159-161)DenomKeyCodec(163-165)DenomKeyCodec(167-169)
x/hold/keeper/invariants_test.go (1)
x/hold/keeper/keeper_test.go (1)
TestSuite(33-50)
x/hold/keeper/keeper_test.go (1)
testutil/assertions/general.go (1)
RequireNotPanicsNoErrorf(55-57)
x/hold/keeper/export_test.go (1)
x/hold/keeper/keeper.go (1)
Keeper(24-32)
⏰ 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-single-db (benchmark-invariants, ubuntu-latest)
- GitHub Check: runsim (multi-seed-short, goleveldb, ubuntu-latest)
- GitHub Check: go-test-single-db (nondeterminism, ubuntu-latest)
- GitHub Check: runsim (after-import, goleveldb, ubuntu-latest)
- GitHub Check: go-test-multi-db (simple, goleveldb, ubuntu-latest)
- GitHub Check: go-test-multi-db (benchmark, goleveldb, ubuntu-latest)
- GitHub Check: runsim (import-export, goleveldb, ubuntu-latest)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: tests (04)
- GitHub Check: tests (03)
- GitHub Check: test-race (03)
- GitHub Check: test-race (04)
- GitHub Check: test-race (01)
- GitHub Check: test-race (02)
- GitHub Check: test-race (00)
- GitHub Check: Build OSX
- GitHub Check: Build Linux
- GitHub Check: golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: heighliner-docker
- GitHub Check: docker
- GitHub Check: ghcr-publish
🔇 Additional comments (21)
.changelog/unreleased/features/2419-hold-module-collections.md (1)
1-1: Verify the correct issue number reference.The PR description states this closes issue #2491, but the changelog entry and filename reference issue #2419. Please verify which issue number is correct and update accordingly.
x/hold/codec.go (2)
22-53: LGTM!The
IntValueCodecimplementation correctly handles encoding/decoding ofsdkmath.Intvalues. The empty byte check inDecodereturningZeroInt()is a good defensive measure.
55-120: LGTM!The
AddressKeyCodecimplementation is correct. Using a 1-byte length prefix makes the encoding self-delimiting, which allows theNonTerminalvariants to correctly delegate to the terminal implementations.app/app.go (1)
615-617: LGTM!The
HoldKeeperinitialization correctly wraps the store key withruntime.NewKVStoreService(), consistent with other keepers in this file that have migrated to collections-based storage.x/hold/keeper/genesis_test.go (2)
6-6: LGTM!The import change from
storetypestocosmossdk.io/core/storealigns with the collections migration pattern.
153-153: LGTM!The setup function signature change to use
store.KVStoreis consistent across all test cases and matches the new storage interface.x/hold/keeper/invariants_test.go (2)
7-8: LGTM!The import addition for
cosmossdk.io/core/storecorrectly supports the updatedKVStoreinterface used in the test setup functions.
49-49: LGTM!The setup function signature consistently uses
store.KVStoreacross all test cases, maintaining uniformity with the collections migration.x/hold/keeper/export_test.go (1)
28-31: LGTM!The signature change from
store storetypes.KVStoretoctx sdk.Contextcorrectly aligns with the new collections-based storage pattern in the keeper. The internal call now properly delegates to the context-awaresetHoldCoinAmountmethod.x/hold/keeper/grpc_query_test.go (2)
3-10: LGTM!The import path update from
cosmossdk.io/store/typestocosmossdk.io/core/storecorrectly aligns with the new KVStoreService-based access pattern used throughout the keeper migration.
107-118: LGTM!The
standardSetupfunction and all test case setup closures correctly use the updatedstore.KVStoretype, maintaining consistency with the new storage access pattern.x/hold/keeper/grpc_query.go (3)
292-310: LGTM!The
compareKeyshelper correctly implements lexicographic comparison: addresses are compared byte-wise first, then denominations are compared as strings. This matches the expected key ordering for the collections map.
98-122: LGTM!The pagination key decoding using
pairCodeccorrectly validates that all bytes were consumed (bytesRead != len(pageKey)), which prevents malformed keys from being accepted.
124-166: Address-based pagination requires loading all entries into memory; this is an intentional design trade-off.The implementation loads all hold entries into memory because the response must group entries by address and aggregate coins per denom. Since the underlying data structure stores individual (address, denom) pairs, standard iterator-based pagination that counts entries cannot be used for address-level pagination. The code comment at line 71 documents this decision: "It was easier to do it this way than shoehorn a solution into a call to FilteredPaginate."
This approach has O(n) memory complexity and could be a concern for chains with very large numbers of holds, but it's a documented architectural trade-off required by the address-grouping requirement, not an oversight.
x/hold/keeper/keeper_test.go (2)
148-162: LGTM!The test helpers are correctly updated to use the new
StoreService.OpenKVStore(ctx)pattern and passctxtoSetHoldCoinAmount. ThesetHoldCoinAmountRawhelper still uses direct store access for injecting malformed test data, which is appropriate for testing error handling paths.
226-342: LGTM!The test cases are correctly updated to use the new
store.KVStoretype in setup closures and the context-basedSetHoldCoinAmountcall. Test coverage for various scenarios (add, update, delete, negative amounts, empty denoms) is maintained.x/hold/keeper/keeper.go (5)
58-65: LGTM!Panicking on schema build failure during initialization is acceptable since this indicates a programming error that should be caught during development. The
AppendLockedCoinsGettercall correctly registers the hold module's locked coins function with the bank keeper.
71-94: LGTM!The
setHoldCoinAmountandgetHoldCoinAmountmethods correctly usecollections.Joinfor key construction and properly handle thecollections.ErrNotFoundcase by returning zero values, maintaining backward compatibility with the previous behavior.
253-277: LGTM!The
GetHoldCoinsimplementation correctly usescollections.NewPrefixedPairRangeto iterate only over entries for the given address, which is more efficient than scanning all entries. Error handling for individual entry read failures is properly accumulated and returned.
317-344: LGTM!The
IterateAllHoldsimplementation correctly iterates over all entries withnilrange, handles per-entry errors gracefully by continuing iteration and accumulating errors, and respects the process callback's stop signal.
38-55: Key encoding is compatible with existing state.The custom
AddressKeyCodecandDenomKeyCodecproduce byte-for-byte identical output to the originalCreateHoldCoinKeyfunction:
AddressKeyCodecencodes with a 1-byte length prefix followed by address bytes, matchingaddress.MustLengthPrefix()DenomKeyCodecencodes denom as raw bytes without a length prefixPairKeyCodeccombines them sequentially withKeyPrefixHoldCoin(0x00) as the store prefixNo state migration is required—the PR objective is valid.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
x/hold/keeper/keeper_test.go (1)
182-195: Error fromstore.Iteratorshould be checked before use.The error returned by
store.Iterator(nil, nil)on line 182 is assigned toerrbut not checked before the loop starts. If iterator creation fails,itercould be nil, causing a panic wheniter.Valid()is called.Apply this diff to handle the error:
iter, err := store.Iterator(nil, nil) + s.Require().NoError(err, "store.Iterator(nil, nil)") defer func() { if iter != nil { iter.Close() } }()
🧹 Nitpick comments (1)
x/hold/keeper/keeper_test.go (1)
153-157: Unusedstoreparameter should be removed.The
storeparameter is no longer used since the function now callss.keeper.SetHoldCoinAmount(s.ctx, ...)which uses the keeper's internal store service.Apply this diff to remove the unused parameter:
-func (s *TestSuite) requireSetHoldCoinAmount(store store.KVStore, addr sdk.AccAddress, denom string, amount sdkmath.Int) { +func (s *TestSuite) requireSetHoldCoinAmount(addr sdk.AccAddress, denom string, amount sdkmath.Int) { assertions.RequireNotPanicsNoErrorf(s.T(), func() error { return s.keeper.SetHoldCoinAmount(s.ctx, addr, denom, amount) }, "setHoldCoinAmount(%s, %s%s)", s.getAddrName(addr), amount, denom) }This will also require updating all call sites (e.g., lines 250, 263, 276, etc.) to remove the
storeargument.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/hold/keeper/keeper.go(6 hunks)x/hold/keeper/keeper_test.go(12 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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.
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: 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/hold/keeper/keeper_test.gox/hold/keeper/keeper.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/hold/keeper/keeper_test.gox/hold/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/hold/keeper/keeper_test.gox/hold/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/hold/keeper/keeper_test.gox/hold/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/hold/keeper/keeper_test.gox/hold/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:162-166
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In Go, iterating over a `nil` slice with `range` is safe and behaves like iterating over an empty slice; no nil check is needed before iterating over a slice.
Applied to files:
x/hold/keeper/keeper_test.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/mocks_test.go:579-589
Timestamp: 2024-10-08T18:12:51.935Z
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/hold/keeper/keeper_test.gox/hold/keeper/keeper.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/hold/keeper/keeper_test.gox/hold/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/hold/keeper/keeper_test.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/hold/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/hold/keeper/keeper.go
🧬 Code graph analysis (1)
x/hold/keeper/keeper.go (4)
internal/provutils/pair.go (1)
Pair(6-9)x/hold/codec.go (22)
AddressKeyCodec(56-56)AddressKeyCodec(58-71)AddressKeyCodec(73-84)AddressKeyCodec(86-88)AddressKeyCodec(90-92)AddressKeyCodec(94-100)AddressKeyCodec(102-104)AddressKeyCodec(106-108)AddressKeyCodec(110-112)AddressKeyCodec(114-116)AddressKeyCodec(118-120)DenomKeyCodec(123-123)DenomKeyCodec(125-131)DenomKeyCodec(133-135)DenomKeyCodec(137-139)DenomKeyCodec(141-143)DenomKeyCodec(145-149)DenomKeyCodec(151-153)DenomKeyCodec(155-157)DenomKeyCodec(159-161)DenomKeyCodec(163-165)DenomKeyCodec(167-169)x/hold/keys.go (1)
ModuleName(5-5)x/hold/keeper/keys.go (1)
KeyPrefixHoldCoin(17-17)
⏰ 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: test-race (00)
- GitHub Check: test-race (03)
- GitHub Check: tests (02)
- GitHub Check: test-race (02)
- GitHub Check: tests (04)
- GitHub Check: tests (03)
- GitHub Check: test-race (04)
- GitHub Check: tests (00)
- GitHub Check: test-race (01)
- GitHub Check: tests (01)
- GitHub Check: build-linux
- GitHub Check: Build OSX
- GitHub Check: Analyze (go)
- GitHub Check: ghcr-publish
- GitHub Check: docker
- GitHub Check: heighliner-docker
- GitHub Check: golangci-lint
🔇 Additional comments (10)
x/hold/keeper/keeper.go (6)
71-84: LGTM!The migration from direct store access to collections-based storage is implemented correctly. The function properly uses
collections.Joinfor composite key creation and handles zero amounts by removing the entry.
87-94: LGTM!The function correctly handles
collections.ErrNotFoundby returning zero, preserving the expected behavior where a missing entry means no hold exists.
238-250: LGTM!The function correctly distinguishes between "not found" (returns zero coin, no error) and actual errors (returns zero coin with wrapped error), maintaining the expected API contract.
253-277: LGTM!The iteration logic with
NewPrefixedPairRangecorrectly scopes to a single address. The error handling approach—continuing iteration while collecting errors—is appropriate for this use case.
283-311: LGTM!The iteration pattern with early termination support is correctly implemented. The error handling properly collects errors while allowing iteration to continue.
317-344: LGTM!The full iteration implementation is correct. Using
nilrange for full iteration and extracting both address and denom for error context provides good debugging information.x/hold/keeper/keeper_test.go (4)
213-215: LGTM!The error handling for
store.Iteratoris correctly implemented here, properly asserting no error before proceeding.
329-344: LGTM!The test execution correctly uses the new context-based API
s.keeper.SetHoldCoinAmount(s.ctx, ...)instead of the old store-based approach.
148-150: LGTM!The helper correctly uses the new
StoreService.OpenKVStore(ctx)pattern for accessing the KV store.
160-162: LGTM!This helper correctly uses direct store access to set raw byte values, which is necessary for testing error handling with malformed data like
"badvalue".
nagarajdivine
force-pushed
the
feat/hold-module-collections
branch
from
3868110 to
90390cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/hold/keeper/keeper_test.go (1)
178-200: Check iterator creation error before using iterator.The error from
store.Iterator(nil, nil)at Line 182 is captured but never checked before callingiter.Valid()at Line 189. If iterator creation fails and returns a nil iterator, this will panic.Apply this diff:
func (s *TestSuite) clearHoldState() { store := s.getStore() var keys [][]byte iter, err := store.Iterator(nil, nil) + s.Require().NoError(err, "store.Iterator(nil, nil)") defer func() { if iter != nil { iter.Close() } }() for ; iter.Valid(); iter.Next() { s.Require().NoError(iter.Error(), "iter.Error()") keys = append(keys, iter.Key()) } err = iter.Close() iter = nil s.Require().NoError(err, "iter.Close()") for _, key := range keys { store.Delete(key) } }
♻️ Duplicate comments (1)
x/hold/keeper/keeper.go (1)
34-66: Potential nil pointer dereference whenbankKeeperis nil.Line 64 calls
bankKeeper.AppendLockedCoinsGetter(...)without checking ifbankKeeperis nil. Based on learnings, nil keepers are intentionally allowed for flexibility in this codebase, so this call could panic.Apply this diff to add a nil check:
k.Schema = schema + if bankKeeper != nil { bankKeeper.AppendLockedCoinsGetter(k.GetLockedCoins) + } return k }
🧹 Nitpick comments (2)
x/hold/keeper/grpc_query.go (1)
124-166: Consider memory implications of loading all entries into memory.The current implementation loads all hold entries into
allEntriesbefore applying pagination. For chains with many accounts holding funds, this could consume significant memory.Consider if there's a way to avoid loading all entries when not using reverse pagination - forward iteration with early termination would be more memory-efficient for the common case.
x/hold/codec.go (1)
125-135: Consider adding length prefix for non-terminal encoding.
EncodeNonTerminalshould ideally use a length-prefixed encoding (similar toAddressKeyCodec) so that the decoder knows where the denom ends when it's not the final component. Currently it delegates toEncodewhich writes raw bytes without a delimiter.If the denom is always the terminal component in practice, this works correctly. But the
NonTerminalmethod name implies it should support non-terminal usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changelog/unreleased/features/2419-hold-module-collections.md(1 hunks)app/app.go(1 hunks)x/hold/codec.go(2 hunks)x/hold/keeper/export_test.go(1 hunks)x/hold/keeper/genesis_test.go(5 hunks)x/hold/keeper/grpc_query.go(2 hunks)x/hold/keeper/grpc_query_test.go(6 hunks)x/hold/keeper/invariants_test.go(19 hunks)x/hold/keeper/keeper.go(6 hunks)x/hold/keeper/keeper_test.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .changelog/unreleased/features/2419-hold-module-collections.md
- x/hold/keeper/grpc_query_test.go
- x/hold/keeper/export_test.go
🧰 Additional context used
🧠 Learnings (21)
📓 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.
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: 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/hold/keeper/invariants_test.gox/hold/keeper/genesis_test.gox/hold/keeper/keeper.goapp/app.gox/hold/keeper/grpc_query.gox/hold/keeper/keeper_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: 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/hold/keeper/invariants_test.gox/hold/keeper/keeper.goapp/app.gox/hold/keeper/grpc_query.gox/hold/keeper/keeper_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/hold/keeper/invariants_test.gox/hold/keeper/genesis_test.gox/hold/keeper/keeper.gox/hold/keeper/grpc_query.gox/hold/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/hold/keeper/invariants_test.gox/hold/keeper/keeper.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/hold/keeper/invariants_test.gox/hold/keeper/genesis_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/hold/keeper/invariants_test.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/hold/keeper/invariants_test.gox/hold/keeper/genesis_test.gox/hold/keeper/keeper.gox/hold/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/hold/keeper/invariants_test.gox/hold/keeper/keeper.gox/hold/keeper/grpc_query.gox/hold/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.go:138-143
Timestamp: 2024-10-08T18:12:51.936Z
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/hold/keeper/invariants_test.gox/hold/keeper/keeper.gox/hold/keeper/grpc_query.gox/hold/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 `WithVestingLockedBypass` function in `x/hold/keeper/invariants.go` is specifically used to handle scenarios involving vesting and locked coins deep within third-party library code. It follows a pattern used by other injectable functions to allow bypassing specific portions of those functions.
Applied to files:
x/hold/keeper/invariants_test.gox/hold/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/hold/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/hold/codec.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/hold/codec.gox/hold/keeper/keeper_test.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/hold/keeper/genesis_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/hold/keeper/genesis_test.go
📚 Learning: 2024-10-02T03:49:31.991Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/address_test.go:69-80
Timestamp: 2024-10-02T03:49:31.991Z
Learning: In the `subSet` function used in unit tests, it's acceptable for it to panic if an out-of-bounds index is provided, as this indicates a test case was set up incorrectly.
Applied to files:
x/hold/keeper/genesis_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/hold/keeper/keeper.goapp/app.gox/hold/keeper/grpc_query.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/hold/keeper/keeper.gox/hold/keeper/keeper_test.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/bank.go:76-87
Timestamp: 2024-10-08T18:12:51.935Z
Learning: In the `balanceValueOwnerTransformer` function in `x/metadata/keeper/bank.go`, it is intended to always return an `AccMDLink`, even if `mdAddr` is nil due to an error from `MetadataAddressFromDenom`.
Applied to files:
x/hold/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:162-166
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In Go, iterating over a `nil` slice with `range` is safe and behaves like iterating over an empty slice; no nil check is needed before iterating over a slice.
Applied to files:
x/hold/keeper/keeper_test.go
🧬 Code graph analysis (4)
x/hold/keeper/invariants_test.go (1)
x/hold/keeper/keeper_test.go (1)
TestSuite(33-50)
x/hold/keeper/keeper.go (2)
x/hold/codec.go (23)
AddressKeyCodec(56-56)AddressKeyCodec(58-71)AddressKeyCodec(73-84)AddressKeyCodec(86-88)AddressKeyCodec(90-92)AddressKeyCodec(94-100)AddressKeyCodec(102-104)AddressKeyCodec(106-108)AddressKeyCodec(110-112)AddressKeyCodec(114-116)AddressKeyCodec(118-120)DenomKeyCodec(123-123)DenomKeyCodec(125-131)DenomKeyCodec(133-135)DenomKeyCodec(137-139)DenomKeyCodec(141-143)DenomKeyCodec(145-149)DenomKeyCodec(151-153)DenomKeyCodec(155-157)DenomKeyCodec(159-161)DenomKeyCodec(163-165)DenomKeyCodec(167-169)IntValueCodec(22-22)x/hold/keeper/keys.go (1)
KeyPrefixHoldCoin(17-17)
app/app.go (1)
x/hold/keys.go (1)
StoreKey(8-8)
x/hold/keeper/grpc_query.go (4)
internal/provutils/pair.go (1)
Pair(6-9)x/hold/keeper/keeper.go (1)
Keeper(24-32)x/hold/hold.pb.go (3)
AccountHold(30-35)AccountHold(39-39)AccountHold(40-42)x/hold/codec.go (22)
AddressKeyCodec(56-56)AddressKeyCodec(58-71)AddressKeyCodec(73-84)AddressKeyCodec(86-88)AddressKeyCodec(90-92)AddressKeyCodec(94-100)AddressKeyCodec(102-104)AddressKeyCodec(106-108)AddressKeyCodec(110-112)AddressKeyCodec(114-116)AddressKeyCodec(118-120)DenomKeyCodec(123-123)DenomKeyCodec(125-131)DenomKeyCodec(133-135)DenomKeyCodec(137-139)DenomKeyCodec(141-143)DenomKeyCodec(145-149)DenomKeyCodec(151-153)DenomKeyCodec(155-157)DenomKeyCodec(159-161)DenomKeyCodec(163-165)DenomKeyCodec(167-169)
⏰ 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: tests (04)
- GitHub Check: tests (03)
- GitHub Check: test-race (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: test-race (04)
- GitHub Check: test-race (01)
- GitHub Check: test-race (00)
- GitHub Check: tests (00)
- 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 (11)
x/hold/keeper/genesis_test.go (2)
6-6: LGTM: Import updated to align with new store abstraction.The import change from
storetypestocosmossdk.io/core/storeis consistent with the PR-wide migration to KVStoreService-based storage patterns.
151-154: LGTM: Test function signatures correctly updated.The
setupfunction parameter type updated fromstoretypes.KVStoretostore.KVStoreis consistent across all test cases and aligns with the new storage abstraction used in the keeper.app/app.go (1)
615-617: LGTM: HoldKeeper correctly initialized with KVStoreService.The change to use
runtime.NewKVStoreService(keys[hold.StoreKey])follows the same pattern used by other keepers in this file (e.g.,ConsensusParamsKeeper,AccountKeeper,BankKeeper), ensuring consistency with the collections-based storage migration.x/hold/keeper/invariants_test.go (2)
7-8: LGTM: Import updated for new store abstraction.The import change from
storetypestocosmossdk.io/core/storeis consistent with the migration pattern across the module.
49-49: LGTM: Test setup function signatures consistently updated.All test case
setupfunctions have been updated to usestore.KVStoreparameter type, maintaining consistency with the new storage abstraction throughout the test file.x/hold/keeper/grpc_query.go (3)
22-27: LGTM: Clean struct design for pagination processing.The
entryWithKeystruct appropriately captures both successful entries and error states during iteration, enabling unified processing in the pagination loop.
54-67: LGTM: Good defensive nil checks with sensible defaults.Properly handles nil request and nil pagination by initializing sensible defaults with
CountTotal: true.
292-311: LGTM: Correct lexicographic key comparison.The
compareKeysfunction properly compares address bytes first, then denomination strings, providing correct lexicographic ordering for the composite key.x/hold/codec.go (2)
22-53: LGTM: IntValueCodec implementation is correct.The codec properly handles binary and JSON encoding/decoding for
sdkmath.Int, with a sensible default ofZeroInt()for empty byte slices.
56-120: LGTM: AddressKeyCodec implementation is robust.Good validation for nil addresses and empty buffers. The 1-byte length prefix encoding correctly supports addresses up to 255 bytes. NonTerminal variants properly delegate to base methods.
x/hold/keeper/keeper_test.go (1)
207-225: LGTM! Iterator error properly checked.The error from
store.Iteratoris now properly checked before using the iterator, addressing the concern from the previous review.
nagarajdivine
force-pushed
the
feat/hold-module-collections
branch
from
90390cb to
721c742
Compare
nagarajdivine
force-pushed
the
feat/hold-module-collections
branch
from
721c742 to
05d52b2
Compare
nagarajdivine
force-pushed
the
feat/hold-module-collections
branch
from
05d52b2 to
53c716b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
x/hold/codec.go (1)
58-70:AddressKeyCodec.Encodevalidates only for an empty buffer, not for insufficient capacity.The guard at line 62 (
len(buffer) == 0) does not protect against the case where the buffer exists but is shorter than1 + len(addr). In that scenariocopy(buffer[1:], addr)silently writes zero bytes while the function returns1 + len(addr)as bytes written — a silent, incorrect result. The collections framework always pre-allocates viaSize()so this won't fire in practice, but the guard should match the actual requirement.🛡️ Suggested defensive fix
- if len(buffer) == 0 { - return 0, fmt.Errorf("buffer is empty") - } + if len(buffer) < 1+len(addr) { + return 0, fmt.Errorf("buffer too small: need %d bytes, got %d", 1+len(addr), len(buffer)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/hold/codec.go` around lines 58 - 70, AddressKeyCodec.Encode currently only checks for an empty buffer but must ensure the buffer has capacity for the length prefix plus the address; change the guard to verify len(buffer) >= 1+len(addr) and return an error (e.g., collections.ErrEncoding or a descriptive fmt.Errorf) when the buffer is too small so that copy(buffer[1:], addr) cannot silently write zero bytes while the function returns a larger count; update the error path in AddressKeyCodec.Encode accordingly and keep behavior for nil addr and addr length > 255 the same.x/hold/keeper/grpc_query.go (1)
22-27:entryErrorsmap is dead code;hasErrfield is redundant.Two cleanup opportunities in the local data structures:
entryErrors map[int]error(lines 125 and 133) is populated at lines 142, 147, and 150 but is never consulted during the pagination processing loop — the same error values are already stored inallEntries[i].errand accessed exclusively viaentry.err. Remove the map entirely.
hasErr boolinentryWithKeyis always set alongside a non-nilerr, making it redundant with anerr != nilcheck.♻️ Proposed cleanup
type entryWithKey struct { key collections.Pair[sdk.AccAddress, string] amount sdkmath.Int - hasErr bool err error }-var entryErrors map[int]error ... -entryErrors = make(map[int]error) -entryIndex := 0 for ; iterator.Valid(); iterator.Next() { kv, err := iterator.KeyValue() if err != nil { iterKey, keyErr := iterator.Key() if keyErr == nil { - entryErrors[entryIndex] = fmt.Errorf("failed to read amount of %s for account %s: %w", - iterKey.K2(), iterKey.K1(), err) - allEntries = append(allEntries, entryWithKey{ - key: iterKey, - hasErr: true, - err: entryErrors[entryIndex], - }) + allEntries = append(allEntries, entryWithKey{ + key: iterKey, + err: fmt.Errorf("failed to read amount of %s for account %s: %w", iterKey.K2(), iterKey.K1(), err), + }) } else { - entryErrors[entryIndex] = fmt.Errorf("failed to read entry: %w", err) - allEntries = append(allEntries, entryWithKey{ - hasErr: true, - err: entryErrors[entryIndex], - }) + allEntries = append(allEntries, entryWithKey{ + err: fmt.Errorf("failed to read entry: %w", err), + }) } - entryIndex++ continue } allEntries = append(allEntries, entryWithKey{ key: kv.Key, amount: kv.Value, - hasErr: false, }) - entryIndex++ }Replace all
entry.hasErrchecks in the processing loop withentry.err != nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/hold/keeper/grpc_query.go` around lines 22 - 27, Remove the dead entryErrors map and the redundant hasErr field from entryWithKey: delete the entryErrors map and all code that populates it, remove the hasErr bool from the entryWithKey struct, stop setting hasErr when assigning entry.err, and replace every check of entry.hasErr in the pagination/processing loop with entry.err != nil (continue using allEntries[i].err or entry.err for error handling). Ensure no remaining references to entryErrors or hasErr remain and tests/compilation reflect these deletions.x/hold/keeper/keeper_test.go (1)
152-161:storeparameter name shadows thestorepackage import in helper functions.Both
requireSetHoldCoinAmount(line 152) andsetHoldCoinAmountRaw(line 159) declare a parameter namedstorethat shadows the importedstorepackage within their bodies. This compiles correctly because neither body needs to reference the package itself, but it is a naming hazard that may confuse linters and future readers. Consider renaming tokvStoreorst.Additionally,
requireSetHoldCoinAmountaccepts thestore store.KVStoreparameter but never uses it — it drives the write throughs.keeper.SetHoldCoinAmount(s.ctx, ...). The parameter exists to maintain a uniform call-site signature withsetHoldCoinAmountRaw; a brief comment clarifying this intent would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/hold/keeper/keeper_test.go` around lines 152 - 161, Rename the shadowing parameter named "store" in both helper functions to a non-conflicting identifier (e.g., kvStore or st) to avoid shadowing the imported store package; update the parameter name in setHoldCoinAmountRaw (function setHoldCoinAmountRaw) and in requireSetHoldCoinAmount (function requireSetHoldCoinAmount) accordingly. Also add a one-line comment to requireSetHoldCoinAmount explaining that the kvStore parameter is intentionally unused and kept to preserve a uniform call-site signature with setHoldCoinAmountRaw, and leave the function body calling s.keeper.SetHoldCoinAmount(s.ctx, ...) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changelog/unreleased/features/2419-hold-module-collections.md:
- Line 1: The changelog entry references issue `#2419` but the PR closes `#2491`;
update the changelog file so both the filename and the issue link inside the
content use the correct issue number (change the "2419" in the filename and the
"[`#2419`](...)" text to "2491" and adjust the URL to /issues/2491) so the
filename, the markdown text "Migrated hold module to collections for state
storage", and the linked issue number are consistent with the PR.
In `@x/hold/keeper/grpc_query.go`:
- Around line 124-165: The code in paginateAllHolds collects all entries into
allEntries via k.Holds.Iterate which materializes the entire dataset; change the
forward (non-reverse) pagination path to start the iterator at startKey using a
ranged iterator (collections.Range or equivalent) so you only scan from startKey
forward and stop once you have filled the requested page/limit instead of
appending every entry to allEntries; update logic in paginateAllHolds to stream
matching entries into the page result as you iterate and stop when page size is
reached, and for the reverse case retain early stopping (do not build the full
allEntries either) so you at least break out once the page is satisfied,
ensuring iterator.Close() is still deferred and entry error handling (the
entryWithKey/error mapping) is applied on-the-fly rather than storing everything
in allEntries.
---
Duplicate comments:
In `@x/hold/codec.go`:
- Around line 159-165: DecodeNonTerminal currently calls DenomKeyCodec.Decode
which consumes the entire buffer, so it cannot be used safely in non-terminal
key positions; update DenomKeyCodec to either implement a non-terminal-safe
format (e.g., change EncodeNonTerminal to write a length prefix and make
DecodeNonTerminal read that length and return only the denom bytes) or
explicitly reject non-terminal usage by having
EncodeNonTerminal/DecodeNonTerminal return an error indicating non-terminal
positions are unsupported; ensure you modify both
DenomKeyCodec.EncodeNonTerminal and DenomKeyCodec.DecodeNonTerminal (and any
related Encode/Decode helpers) so their behavior is consistent and documented in
the method comments.
In `@x/hold/keeper/keeper.go`:
- Line 64: The call to bankKeeper.AppendLockedCoinsGetter(k.GetLockedCoins) will
panic if bankKeeper is nil; update the initialization in keeper.go to check for
nil before calling AppendLockedCoinsGetter — e.g., only call
bankKeeper.AppendLockedCoinsGetter(k.GetLockedCoins) when bankKeeper != nil (or
when the provided BankKeeper implements an interface check), so tests can pass
nil keepers without dereferencing them; locate the call using the symbols
bankKeeper.AppendLockedCoinsGetter and k.GetLockedCoins and add the nil guard
around that invocation.
---
Nitpick comments:
In `@x/hold/codec.go`:
- Around line 58-70: AddressKeyCodec.Encode currently only checks for an empty
buffer but must ensure the buffer has capacity for the length prefix plus the
address; change the guard to verify len(buffer) >= 1+len(addr) and return an
error (e.g., collections.ErrEncoding or a descriptive fmt.Errorf) when the
buffer is too small so that copy(buffer[1:], addr) cannot silently write zero
bytes while the function returns a larger count; update the error path in
AddressKeyCodec.Encode accordingly and keep behavior for nil addr and addr
length > 255 the same.
In `@x/hold/keeper/grpc_query.go`:
- Around line 22-27: Remove the dead entryErrors map and the redundant hasErr
field from entryWithKey: delete the entryErrors map and all code that populates
it, remove the hasErr bool from the entryWithKey struct, stop setting hasErr
when assigning entry.err, and replace every check of entry.hasErr in the
pagination/processing loop with entry.err != nil (continue using
allEntries[i].err or entry.err for error handling). Ensure no remaining
references to entryErrors or hasErr remain and tests/compilation reflect these
deletions.
In `@x/hold/keeper/keeper_test.go`:
- Around line 152-161: Rename the shadowing parameter named "store" in both
helper functions to a non-conflicting identifier (e.g., kvStore or st) to avoid
shadowing the imported store package; update the parameter name in
setHoldCoinAmountRaw (function setHoldCoinAmountRaw) and in
requireSetHoldCoinAmount (function requireSetHoldCoinAmount) accordingly. Also
add a one-line comment to requireSetHoldCoinAmount explaining that the kvStore
parameter is intentionally unused and kept to preserve a uniform call-site
signature with setHoldCoinAmountRaw, and leave the function body calling
s.keeper.SetHoldCoinAmount(s.ctx, ...) as-is.
| @@ -0,0 +1 @@ | |||
| * Migrated hold module to collections for state storage [#2419](https://github.com/provenance-io/provenance/issues/2419). | |||
There was a problem hiding this comment.
Issue number in changelog does not match the PR's linked issue.
The changelog filename and content both reference issue #2419, but the PR description states it closes issue #2491. Please verify and correct the issue number (the filename would also need renaming to match).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changelog/unreleased/features/2419-hold-module-collections.md at line 1,
The changelog entry references issue `#2419` but the PR closes `#2491`; update the
changelog file so both the filename and the issue link inside the content use
the correct issue number (change the "2419" in the filename and the
"[`#2419`](...)" text to "2491" and adjust the URL to /issues/2491) so the
filename, the markdown text "Migrated hold module to collections for state
storage", and the linked issue number are consistent with the PR.
| var allEntries []entryWithKey | ||
| var entryErrors map[int]error | ||
|
|
||
| if len(key) != 0 { | ||
| iterator := getIterator(prefixStore, key, reverse) | ||
| defer iterator.Close() //nolint:errcheck // close error safe to ignore in this context. | ||
| iterator, err := k.Holds.Iterate(ctx, nil) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to create iterator: %v", err) | ||
| } | ||
| defer iterator.Close() //nolint:errcheck // close error safe to ignore in this context. | ||
|
|
||
| for ; iterator.Valid(); iterator.Next() { | ||
| if err := iterator.Error(); err != nil { | ||
| return nil, err | ||
| entryErrors = make(map[int]error) | ||
| entryIndex := 0 | ||
|
|
||
| // Collect all entries | ||
| for ; iterator.Valid(); iterator.Next() { | ||
| kv, err := iterator.KeyValue() | ||
| if err != nil { | ||
| iterKey, keyErr := iterator.Key() | ||
| if keyErr == nil { | ||
| entryErrors[entryIndex] = fmt.Errorf("failed to read amount of %s for account %s: %w", | ||
| iterKey.K2(), iterKey.K1(), err) | ||
| allEntries = append(allEntries, entryWithKey{ | ||
| key: iterKey, | ||
| hasErr: true, | ||
| err: entryErrors[entryIndex], | ||
| }) | ||
| } else { | ||
| entryErrors[entryIndex] = fmt.Errorf("failed to read entry: %w", err) | ||
| allEntries = append(allEntries, entryWithKey{ | ||
| hasErr: true, | ||
| err: entryErrors[entryIndex], | ||
| }) | ||
| } | ||
| entryIndex++ | ||
| continue | ||
| } | ||
|
|
||
| ikey := iterator.Key() | ||
| addr, denom := ParseHoldCoinKeyUnprefixed(ikey) | ||
| if !addr.Equals(lastAddr) { | ||
| if uint64(len(resp.Holds)) >= limit { | ||
| resp.Pagination.NextKey = ikey | ||
| break | ||
| allEntries = append(allEntries, entryWithKey{ | ||
| key: kv.Key, | ||
| amount: kv.Value, | ||
| hasErr: false, | ||
| }) | ||
| entryIndex++ |
There was a problem hiding this comment.
paginateAllHolds performs a full collection scan into memory before applying pagination.
All hold entries across all accounts are materialized into allEntries (line 124) before any pagination logic runs. This is O(n) in both time and heap allocation, where n is the total number of hold coin entries in the module. Concurrent GetAllHolds queries compound the pressure. On a chain with a large number of hold entries, this:
- Allocates unbounded heap per request.
- Can exceed query timeout limits at scale.
- Is a potential amplification vector for resource exhaustion.
For forward, non-reverse, key-based pagination the iterator could be started directly at startKey using a ranged collections.Range starting at startKey, avoiding the full scan. The reverse case is harder to optimize this way, but it would at least halve memory usage for the common forward case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/hold/keeper/grpc_query.go` around lines 124 - 165, The code in
paginateAllHolds collects all entries into allEntries via k.Holds.Iterate which
materializes the entire dataset; change the forward (non-reverse) pagination
path to start the iterator at startKey using a ranged iterator
(collections.Range or equivalent) so you only scan from startKey forward and
stop once you have filled the requested page/limit instead of appending every
entry to allEntries; update logic in paginateAllHolds to stream matching entries
into the page result as you iterate and stop when page size is reached, and for
the reverse case retain early stopping (do not build the full allEntries either)
so you at least break out once the page is satisfied, ensuring iterator.Close()
is still deferred and entry error handling (the entryWithKey/error mapping) is
applied on-the-fly rather than storing everything in allEntries.
1 participant
Description
closes: Switch to collections in the hold module. #2491
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