Migrate name module from KV store to collections.#2411
Open
nagarajdivine wants to merge 24 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughMigrate the name module from legacy KV storage to Cosmos SDK collections: replace raw KV access with a collections Schema, IndexedMap and multi-index, move params to a collections Item, add KV→collections migrator and tests, update key codecs/queries, adjust call sites/tests, and bump module consensus version to 3. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant NameKeeper
participant Collections as CollectionsStore
participant LegacyKV as LegacyKVStore
participant Migrator
App->>NameKeeper: NewKeeper(runtime.NewKVStoreService(...))
NameKeeper->>Collections: Build Schema, IndexedMap(nameRecords), params Item
App->>NameKeeper: SetNameRecord / UpdateNameRecord / DeleteRecord
NameKeeper->>Collections: IndexedMap.Set / Get / Remove
Collections-->>NameKeeper: AddrIndex updates / queries
Migrator->>LegacyKV: Iterate legacy name & param prefixes
LegacyKV-->>Migrator: legacy records & params
Migrator->>Collections: Insert migrated NameRecord(s) and Params
Collections-->>Migrator: confirm inserts
App->>NameKeeper: Resolve / ReverseLookup
NameKeeper->>Collections: nameRecords.Get / AddrIndex.Match / Walk (pagination)
NameKeeper-->>App: Return results (with continuation key if any)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
SpicyLemon
reviewed
Jul 30, 2025
SpicyLemon
left a comment
SpicyLemon
left a comment
Contributor
There was a problem hiding this comment.
Some preliminary comments.
nagarajdivine
force-pushed
the
feature/collections-name-module
branch
from
78a0593 to
9ff0d20
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
x/attribute/keeper/keeper_test.go (1)
167-176: Test case name doesn't match the error conditionThe test case name "should fail unable to normalize segment length too short" is misleading. The actual error is about having too many segments (4 segments in "example.cant.normalize.me" exceeds the MaxNameLevels of 3), not about segment length. The updated error message correctly reflects this.
{ - name: "should fail unable to normalize segment length too short", + name: "should fail unable to normalize - too many segments", attr: types.Attribute{ Name: "example.cant.normalize.me", Value: []byte("0123456789"), Address: s.user1, AttributeType: types.AttributeType_String, }, ownerAddr: s.user2Addr, errorMsg: "unable to normalize attribute name \"example.cant.normalize.me\": name has too many segments", },
♻️ Duplicate comments (2)
x/attribute/keeper/keeper_test.go (1)
854-859: Update expected error contents instead of using ContainsAs mentioned in the previous review, it's better to update the expected error contents than to switch to just checking if it contains what it used to be expecting.
_, err := s.app.AttributeKeeper.GetAttributes(s.ctx, s.user1, "blah") -s.Assert().Error(err, "GetAttributes") -s.Assert().Contains(err.Error(), "no address bound to name") +s.Assert().EqualError(err, "no address bound to name: name not bound", "GetAttributes") attributes, err := s.app.AttributeKeeper.GetAttributes(s.ctx, s.user1, "example.attribute") s.Assert().NoError(err, "GetAttributes should succeed")x/name/keeper/migrations.go (1)
25-81: Well-implemented migration logic!The migration correctly:
- Preserves original keys when moving data to collections
- Handles parameters, name records, and address indices
- Provides detailed logging for monitoring progress
- Properly cleans up iterators with defer statements
- Returns errors appropriately
The implementation ensures data integrity during the v2 to v3 upgrade.
Note: As mentioned in previous reviews, the
storeKeyis still needed here to access the legacy KV store data during migration, which is the correct approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.changelog/unreleased/features/2411-collections-name-module.md(1 hunks)app/app.go(1 hunks)app/app_test.go(1 hunks)app/upgrades_test.go(2 hunks)x/attribute/keeper/keeper_test.go(2 hunks)x/attribute/keeper/query_server_test.go(2 hunks)x/name/keeper/genesis.go(2 hunks)x/name/keeper/keeper.go(11 hunks)x/name/keeper/keeper_test.go(8 hunks)x/name/keeper/migration_test.go(1 hunks)x/name/keeper/migrations.go(2 hunks)x/name/keeper/params.go(1 hunks)x/name/keeper/query_server.go(2 hunks)x/name/module.go(2 hunks)x/name/types/keys.go(2 hunks)x/name/types/keys_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (51)
📓 Common learnings
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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.
📚 Learning: a github issue was created to track the migration of parameters for the metadata, marker, name, and ...
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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/2411-collections-name-module.md
📚 Learning: in `app/upgrades_test.go`, we prefer to keep todo comments (like `todo[viridian]`) to handle code de...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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:
app/upgrades_test.goapp/app_test.gox/name/keeper/migration_test.goapp/app.go
📚 Learning: the `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned t...
Learnt from: SpicyLemon
PR: provenance-io/provenance#1874
File: cmd/dbmigrate/utils/migrator_test.go:10-10
Timestamp: 2024-10-08T18:12:57.974Z
Learning: The `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned to be removed from the project soon, as indicated by the user. Therefore, suggestions for changes or tests related to this directory are unnecessary.
Applied to files:
app/upgrades_test.gox/name/keeper/migration_test.gox/name/keeper/migrations.go
📚 Learning: in `cli_test.go` test cases, the `expout` field may include indentation levels and field names with ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2197
File: x/metadata/client/cli/cli_test.go:1474-1480
Timestamp: 2024-10-23T18:46:02.430Z
Learning: In `cli_test.go` test cases, the `expOut` field may include indentation levels and field names with extra spaces to match the YAML output format. Including partial outputs in `expOut` is acceptable as long as they verify the necessary parts.
Applied to files:
app/upgrades_test.gox/name/keeper/keeper_test.go
📚 Learning: there are no rollback mechanisms used in the migration functions of the `app/upgrades.go` file, and ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2033
File: app/upgrades.go:303-310
Timestamp: 2024-06-13T21:10:37.185Z
Learning: There are no rollback mechanisms used in the migration functions of the `app/upgrades.go` file, and error handling is managed by logging and returning errors as per the project's standards.
Applied to files:
app/upgrades_test.goapp/app.go
📚 Learning: the upgrade injection logic in `app/app.go` is used only for testing purposes and not in production....
Learnt from: Taztingo
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The upgrade injection logic in `app/app.go` is used only for testing purposes and not in production.
Applied to files:
app/upgrades_test.goapp/app.go
📚 Learning: when checking for indentation issues in multi-line strings assigned to the `long` field in go code, ...
Learnt from: iramiller
PR: provenance-io/provenance#2160
File: x/metadata/client/cli/tx.go:1196-1212
Timestamp: 2024-09-23T22:46:26.850Z
Learning: When checking for indentation issues in multi-line strings assigned to the `Long` field in Go code, verify that the source lines are actually indented before flagging them.
Applied to files:
app/upgrades_test.go
📚 Learning: in go tests, modifying byte slices directly using arithmetic operations like addition and subtractio...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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:
app/upgrades_test.gox/name/types/keys_test.gox/name/types/keys.go
📚 Learning: when testing migration processes expected to handle a large number of items in production (e.g., 300...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: app/upgrades_test.go:1132-1161
Timestamp: 2024-10-02T03:20:17.677Z
Learning: When testing migration processes expected to handle a large number of items in production (e.g., 300,005 scopes), it's acceptable to create an equivalent number of items in unit tests to simulate real-world scenarios and observe performance and resource usage.
Applied to files:
app/upgrades_test.go
📚 Learning: when test cases are run using `s.t().run` instead of `s.run`, use `require` with `t` instead of `s.r...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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:
app/upgrades_test.gox/attribute/keeper/query_server_test.gox/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.go
📚 Learning: the `pio_testnet` environment variable is explicitly set to its default value in `pre_upgrade_test.g...
Learnt from: SpicyLemon
PR: provenance-io/provenance#1968
File: cmd/provenanced/cmd/pre_upgrade_test.go:101-103
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `PIO_TESTNET` environment variable is explicitly set to its default value in `pre_upgrade_test.go` to ensure test consistency across different local environments where this variable might be set differently.
Applied to files:
app/upgrades_test.gox/attribute/keeper/query_server_test.go
📚 Learning: spicylemon prefers allowing tests to fail naturally if the setup is incorrect rather than explicitly...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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:
app/upgrades_test.gox/attribute/keeper/query_server_test.gox/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.go
📚 Learning: the new queries `queryattributeaccountsresponse` and `queryaccountdataresponse` in the attribute mod...
Learnt from: Taztingo
PR: provenance-io/provenance#2003
File: internal/provwasm/stargate_whitelist.go:86-87
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The new queries `QueryAttributeAccountsResponse` and `QueryAccountDataResponse` in the Attribute module are tested by their respective modules, as confirmed by the presence of these queries in various test files such as `x/attribute/keeper/query_server_test.go`, `x/attribute/client/rest/grpc_query_test.go`, and `x/attribute/client/cli/cli_test.go`.
Applied to files:
x/attribute/keeper/query_server_test.gox/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.go
📚 Learning: in `app/app_test.go`, when testing that the encoder is correctly wired to handle a governance `propo...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/attribute/keeper/query_server_test.goapp/app_test.gox/name/types/keys_test.gox/name/keeper/keeper_test.go
📚 Learning: in the `app` package, the constant `paramsname` is defined in `app/app.go` and can be used in `app/u...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: app/upgrades.go:55-55
Timestamp: 2024-10-10T23:16:32.311Z
Learning: In the `app` package, the constant `paramsName` is defined in `app/app.go` and can be used in `app/upgrades.go`.
Applied to files:
x/attribute/keeper/query_server_test.gox/name/keeper/params.goapp/app.go
📚 Learning: in the `provenance` codebase, the constant `paramsname` is defined in `app/app.go` and is accessible...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: app/upgrades.go:41-41
Timestamp: 2024-10-10T23:14:29.308Z
Learning: In the `provenance` codebase, the constant `paramsName` is defined in `app/app.go` and is accessible in `app/upgrades.go` since they are in the same package.
Applied to files:
x/attribute/keeper/query_server_test.gox/name/keeper/params.goapp/app_test.goapp/app.go
📚 Learning: in go tests within this project, it's acceptable to have similar helper functions like `newuuid`, `n...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/attribute/keeper/query_server_test.goapp/app_test.gox/name/types/keys_test.gox/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.gox/name/keeper/migration_test.go
📚 Learning: in go tests where test cases are defined within a test case slice and iterated with `t.run`, instanc...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils_test.go:1837-1889
Timestamp: 2024-10-02T03:31:48.658Z
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/attribute/keeper/query_server_test.gox/name/keeper/keeper_test.go
📚 Learning: in the `testscopequery` function, the conditional checks following the equality assertion are design...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/attribute/keeper/query_server_test.gox/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.go
📚 Learning: in `app/app.go` of the provenance blockchain (go), the `txconfig` needs to be initialized in two sta...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2175
File: app/app.go:451-452
Timestamp: 2024-10-10T23:08:46.079Z
Learning: In `app/app.go` of the Provenance Blockchain (Go), the `txConfig` needs to be initialized in two stages because early components depend on a partially completed `txConfig`. The full setup of `txConfig` requires the `BankKeeper`, which isn't available until later in the initialization process. Therefore, `txConfig` is first initialized with essential options and then finalized after `BankKeeper` is created.
Applied to files:
x/attribute/keeper/query_server_test.goapp/app_test.gox/name/keeper/genesis.goapp/app.go
📚 Learning: in test helper functions like `setbalances` in `x/metadata/keeper/bank_test.go`, it's acceptable to ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/attribute/keeper/query_server_test.gox/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.gox/name/keeper/migration_test.gox/name/keeper/keeper.go
📚 Learning: when swapping in a logger maker for `simapp.setup(t)` within an immediately invoked anonymous functi...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/migrations_v4_test.go:182-187
Timestamp: 2024-10-02T03:03:14.942Z
Learning: When swapping in a logger maker for `simapp.Setup(t)` within an immediately invoked anonymous function, it's acceptable to use a `defer` inside that function to reset the logger maker after the function call.
Applied to files:
x/attribute/keeper/query_server_test.go
📚 Learning: in the codebase, the other `newparams` functions are intended to remain as they are and do not need ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: x/metadata/types/oslocatorparams.go:7-9
Timestamp: 2024-10-10T23:13:15.277Z
Learning: In the codebase, the other `NewParams` functions are intended to remain as they are and do not need to be renamed.
Applied to files:
x/name/keeper/params.go
📚 Learning: in the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `validate` method wi...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/params.gox/name/keeper/genesis.gox/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.go
📚 Learning: the `parsepaymentstorevalue` function in `x/exchange/keeper/payments.go` returns `nil, nil` when the...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/params.go
📚 Learning: the user has updated the governance parameter structure in `cmd/provenanced/cmd/init.go` to `govgens...
Learnt from: Taztingo
PR: provenance-io/provenance#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/name/keeper/params.gox/name/keeper/genesis.goapp/app.go
📚 Learning: in `x/marker/types/send_restrictions.go`, the `transferagentkey` is only used for storing data in ep...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/params.goapp/app.gox/name/types/keys.gox/name/keeper/keeper.go
📚 Learning: the `depguard` definitions in `.golangci.yml` are prefixes (unless they end with `$`), so the entry ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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:
app/app_test.gox/name/keeper/query_server.go
📚 Learning: in `scripts/protocgen.sh`, the regex pattern `'option go_package.*provenance'` is sufficient for sel...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2167
File: scripts/protocgen.sh:13-14
Timestamp: 2024-10-08T18:12:51.935Z
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:
app/app_test.go
📚 Learning: in `cmd/provenanced/cmd/root.go`, the `istestnetflagset` function manually parses the `--testnet` fl...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2177
File: cmd/provenanced/cmd/root.go:69-69
Timestamp: 2024-10-11T17:33:53.507Z
Learning: In `cmd/provenanced/cmd/root.go`, the `isTestnetFlagSet` function manually parses the `--testnet` flag from `os.Args` due to a bootstrap problem where flag definitions are not yet available when the application starts.
Applied to files:
app/app_test.go
📚 Learning: the variable `envprefix` is defined in `app/prefix.go`, so it is available for use throughout the co...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2175
File: app/app.go:311-311
Timestamp: 2024-10-11T17:35:59.823Z
Learning: The variable `EnvPrefix` is defined in `app/prefix.go`, so it is available for use throughout the codebase.
Applied to files:
x/name/keeper/query_server.gox/name/keeper/keeper.go
📚 Learning: - in v0.50.x of the sdk, they have started the transition to using `context.context` instead of `sdk...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: when unit tests exist for a key generation function, it's preferable to reuse that function in tests...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-08T18:12:51.936Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/name/types/keys_test.gox/name/keeper/keeper_test.go
📚 Learning: when mocking methods like `mintcoins` and `burncoins` in tests, using slices to store error strings ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.gox/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.go
📚 Learning: in `scope_test.go`, numeric literals are intentionally used for the `role` field in tests instead of...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.go
📚 Learning: in go unit tests, repetitive code is acceptable; refactoring for dry is not necessary....
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/exchange/keeper/mocks_test.go:427-431
Timestamp: 2024-10-02T03:16:46.324Z
Learning: In Go unit tests, repetitive code is acceptable; refactoring for DRY is not necessary.
Applied to files:
x/name/types/keys_test.go
📚 Learning: do not raise issues about the `keeper` package still being in use after import removal in test files...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.gox/name/keeper/migration_test.gox/name/keeper/keeper.go
📚 Learning: in this codebase, methods like `getacc()` and `getsigneracc()` are intended to return `nil` if the a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.gox/name/types/keys.gox/name/keeper/keeper.go
📚 Learning: when using `sdk.accaddress` as map keys in `authzcache`, casting to `string(addr)` is acceptable and...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils.go:306-308
Timestamp: 2024-10-02T02:20:49.611Z
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/name/keeper/keeper_test.gox/name/types/keys.gox/name/keeper/keeper.go
📚 Learning: in this codebase, when using `sdk.accaddress` as map keys, casting to `string` is preferred over cal...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.gox/name/types/keys.gox/name/keeper/keeper.go
📚 Learning: the expected return value for the `getsigners()` function in the `quarantine` module, when an addres...
Learnt from: SpicyLemon
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The expected return value for the `GetSigners()` function in the `quarantine` module, when an address is bad, is a list with a single `nil` entry. This behavior is intentionally tested and confirmed by user SpicyLemon.
Applied to files:
x/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.go
📚 Learning: the `getsigners()` function in the `quarantine` module is designed to include a `nil` entry in the r...
Learnt from: SpicyLemon
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `GetSigners()` function in the `quarantine` module is designed to include a `nil` entry in the result when it encounters a bad address. This behavior is intentionally tested in the unit tests.
Applied to files:
x/name/keeper/keeper_test.go
📚 Learning: the `createautoresponsetoaddrprefix` function in the quarantine module should have unit tests that a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/name/keeper/keeper_test.gox/attribute/keeper/keeper_test.go
📚 Learning: in subtests, it's acceptable to use `require` and `assert` functions directly instead of `s.require(...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/msg_server_test.go:1248-1251
Timestamp: 2024-10-02T03:39:04.537Z
Learning: In subtests, it's acceptable to use `require` and `assert` functions directly instead of `s.Require()` and `s.Assert()` methods.
Applied to files:
x/attribute/keeper/keeper_test.go
📚 Learning: the `v3writenewscope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migr...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migration_test.gox/name/keeper/migrations.goapp/app.gox/name/keeper/keeper.go
📚 Learning: in the `migratevalueowner` method, use `sdkerrors.errlogic` when representing unexpected backend err...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/msg_server.go:262-263
Timestamp: 2024-10-08T18:12:57.975Z
Learning: In the `MigrateValueOwner` method, use `sdkerrors.ErrLogic` when representing unexpected backend errors, as the request itself is valid.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in `setscopevalueowner`, no additional validation is needed for `fromaddr` and `toaddr` since they a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/scope.go:228-284
Timestamp: 2024-10-02T03:10:33.082Z
Learning: In `SetScopeValueOwner`, no additional validation is needed for `fromAddr` and `toAddr` since they are `sdk.AccAddress` types.
Applied to files:
x/name/types/keys.go
📚 Learning: the authority address validation is handled in the `validatebasic` method for the message in the `x/...
Learnt from: nullpointer0x00
PR: provenance-io/provenance#1984
File: x/ibcratelimit/msgs.go:29-29
Timestamp: 2024-10-08T18:12:51.936Z
Learning: The authority address validation is handled in the `ValidateBasic` method for the message in the `x/ibcratelimit` module, while the `Params` struct is responsible for validating the `ContractAddress` property.
Applied to files:
x/name/types/keys.go
📚 Learning: in the `balancevalueownertransformer` function in `x/metadata/keeper/bank.go`, it is intended to alw...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/bank.go:76-87
Timestamp: 2024-10-02T15:32:02.168Z
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/name/keeper/keeper.go
📚 Learning: - when defining interfaces that need to satisfy external modules (like `bankkeeper`), suggesting cha...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/expected_keepers.go:39-49
Timestamp: 2024-10-02T01:58:51.519Z
Learning: - When defining interfaces that need to satisfy external modules (like `BankKeeper`), suggesting changes to method signatures may not be appropriate as they are outside of our control.
Applied to files:
x/name/keeper/keeper.go
🧬 Code Graph Analysis (5)
x/name/keeper/params.go (2)
x/name/keeper/keeper.go (1)
Keeper(22-40)x/name/types/name.pb.go (3)
Params(29-38)Params(42-42)Params(43-45)
x/name/keeper/query_server.go (5)
x/name/keeper/keeper.go (1)
Keeper(22-40)x/name/types/query.pb.go (6)
QueryReverseLookupRequest(211-216)QueryReverseLookupRequest(220-220)QueryReverseLookupRequest(221-223)QueryReverseLookupResponse(252-257)QueryReverseLookupResponse(261-261)QueryReverseLookupResponse(262-264)x/name/types/errors.go (1)
ErrInvalidAddress(22-22)x/name/types/keys.go (1)
GetAddressKeyPrefix(66-73)x/name/types/name.pb.go (3)
NameRecord(102-109)NameRecord(112-112)NameRecord(113-115)
x/name/types/keys_test.go (1)
x/name/types/keys.go (1)
RawBytesKey(82-82)
x/name/keeper/migration_test.go (3)
x/name/types/keys.go (4)
StoreKey(20-20)NameParamStoreKey(32-32)GetNameKeyPrefix(36-39)GetAddressKeyPrefix(66-73)x/name/keeper/keeper.go (1)
NewKeeper(47-67)x/name/keeper/migrations.go (1)
NewMigrator(19-21)
x/name/keeper/migrations.go (3)
x/attribute/keeper/migrations.go (1)
Migrator(4-6)x/marker/keeper/migrations.go (1)
Migrator(4-6)x/name/types/keys.go (3)
NameParamStoreKey(32-32)NameKeyPrefix(28-28)AddressKeyPrefix(30-30)
🪛 GitHub Check: golangci-lint
x/name/keeper/params.go
[failure] 17-17:
Error return value of k.paramsStore.Set is not checked (errcheck)
x/name/keeper/query_server.go
[failure] 68-68:
G115: integer overflow conversion uint64 -> int (gosec)
x/name/keeper/keeper.go
[failure] 181-181:
appendAssign: append result not assigned to the same slice (gocritic)
[failure] 160-160:
appendAssign: append result not assigned to the same slice (gocritic)
[failure] 279-279:
appendAssign: append result not assigned to the same slice (gocritic)
[failure] 386-386:
appendAssign: append result not assigned to the same slice (gocritic)
[failure] 367-367:
appendAssign: append result not assigned to the same slice (gocritic)
🪛 GitHub Actions: Lint
x/name/keeper/params.go
[error] 17-17: golangci-lint: Error return value of k.paramsStore.Set is not checked (errcheck)
⏰ 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). (3)
- GitHub Check: runsim (multi-seed-short, goleveldb, ubuntu-latest)
- GitHub Check: runsim (import-export, goleveldb, ubuntu-latest)
- GitHub Check: runsim (after-import, goleveldb, ubuntu-latest)
🔇 Additional comments (23)
app/app_test.go (1)
33-33: LGTM! Import reordering looks good.The import statement has been reordered appropriately within the import block with no functional impact.
x/name/types/keys.go (2)
5-13: LGTM! Appropriate imports for the new codec implementation.The new imports support JSON operations and the collections codec interface needed for the name module migration to collections.
79-139: LGTM! Well-implemented raw bytes key codec.The
rawBytesKeyCodecimplementation correctly handles raw byte slice keys for the collections framework:
- Proper buffer size validation in
Encode- Consistent behavior between terminal and non-terminal methods
- Standard JSON marshaling using base64 encoding
- Clear error handling and type identification
The implementation aligns well with the name module's migration to collections-based storage.
.changelog/unreleased/features/2411-collections-name-module.md (1)
1-1: LGTM! Clear and concise changelog entry.The changelog entry properly documents the name module migration to collections with appropriate issue reference.
x/attribute/keeper/query_server_test.go (2)
22-22: LGTM! Appropriate import for name module integration.The import is needed to access
DefaultParams()for proper test setup.
47-47: LGTM! Good test setup enhancement.Initializing the NameKeeper parameters ensures consistent test state, especially important with the migration to collections-based parameter storage. This follows the same pattern as other keeper parameter initializations in the test setup.
app/upgrades_test.go (2)
1037-1043: Constant cleanup acknowledged – no further action neededThe only change is removal of the trailing spaces in the
LogMsgRunModuleMigrationsvalue.
No behavioural impact, aligns with the other message constants.
1056-1064: Commenting-out an expected log line hides a potential regressionThe Bouvardia main-net upgrade should emit a line when the name-module migration runs.
By commenting it out, the test now silently passes even if the migration log is missing.
Consider either:
- Re-enabling the assertion once the handler reliably logs the message, or
- Explicitly documenting (in code) why the message is not expected for the main upgrade but is for
bouvardia-rc1.- // "INF Migrating name module from KV store to collections (v2 to v3)...", + "INF Migrating name module from KV store to collections (v2 to v3)...",Leaving it commented risks future changes removing the log without detection.
app/app.go (1)
554-554: LGTM! NameKeeper initialization updated for collections migration.The addition of
runtime.NewKVStoreService(keys[nametypes.StoreKey])as the third parameter correctly provides the required store service for the collections framework. This change aligns with the broader migration from direct KV store usage to the collections package and follows the same pattern used by other keepers in the application.x/name/module.go (1)
128-133: LGTM! Migration registration is properly implementedThe migration registration follows the standard pattern with appropriate error handling and panic on failure. The consensus version bump to 3 correctly aligns with this migration.
x/name/types/keys_test.go (1)
122-173: Excellent test coverage for RawBytesKeyCodecThe test comprehensively covers all codec methods with various input scenarios including edge cases (empty key) and different key lengths. The assertions properly validate both return values and data integrity.
x/name/keeper/genesis.go (2)
14-14: LGTM! Clear documentation added.The comment clarifies that module parameters are set during initialization, which improves code readability.
28-48: Excellent refactoring to collections framework!The migration from legacy KV store iteration to the collections API is implemented correctly:
- Proper range construction using
StartInclusiveandEndExclusive- Clean iteration with the
Walkmethod- Appropriate error handling with panic on failure
The code is now more type-safe and consistent with the module's migration to collections.
x/name/keeper/keeper_test.go (4)
259-259: Correct error message update.The error message change from generic "value provided for name is invalid" to specific "segment of name is too short" reflects the improved validation in the normalized name handling.
292-295: Proper test setup for UpdateNameRecord.The test now correctly creates the name record before attempting to update it, which aligns with the keeper's expected behavior where UpdateNameRecord requires an existing record.
484-484: Updated expectations match new DeleteInvalidAddressIndexEntries behavior.The test expectations have been correctly updated to reflect the new collections-based implementation of DeleteInvalidAddressIndexEntries, which now logs the total count of valid entries instead of deletion details.
Also applies to: 491-494, 502-505
664-706: Excellent test coverage for collections storage consistency!This new test thoroughly validates that:
- Name records are correctly stored in the nameRecords collection
- Address indices are properly maintained in the addrIndex collection
- Both collections store identical record values
- Key construction and retrieval work as expected
The test provides valuable verification of the collections migration correctness.
x/name/keeper/migration_test.go (1)
1-122: Comprehensive migration test coverage!This test suite excellently validates the KV-to-collections migration:
- Properly sets up legacy store data with parameters and records
- Creates keeper with new collections schema
- Validates migration execution without errors
- Thoroughly verifies data integrity post-migration through multiple access patterns
The test ensures the migration preserves all data and maintains consistency across name records and address indices.
x/name/keeper/keeper.go (5)
22-67: Excellent migration to collections framework!The keeper refactoring properly:
- Defines typed collections for name records, address indices, and parameters
- Makes collections private as requested in previous reviews
- Initializes schema with appropriate error handling
- Uses RawBytesKey codec for maintaining key compatibility
The implementation follows best practices for the collections framework.
160-160: Static analysis false positives - ignore appendAssign warnings.The golangci-lint warnings about
appendAssignare false positives. The code correctly creates new slices for composite keys using patterns like:addrIndexKey := append(addrPrefix, nameKey...)This is the intended behavior for creating index keys by combining prefixes.
Also applies to: 181-181, 279-279, 367-367, 386-386
109-206: Correct implementation of name record CRUD operations.The refactored methods properly:
- Use collections Get/Set/Remove/Has methods
- Maintain consistency between nameRecords and addrIndex
- Handle address changes by removing old index entries
- Preserve all original functionality while improving type safety
396-461: Clean refactoring of DeleteInvalidAddressIndexEntries.The method has been successfully migrated to use collections:
- Uses Walk to iterate through address indices
- Properly extracts name keys from composite keys
- Validates entries against the nameRecords collection
- Maintains the same cleanup logic with improved implementation
463-469: Useful helper methods for direct collection access.The new
GetNameRecordandGetAddrIndexRecordmethods provide clean access to the collections, which is particularly useful for migration and testing scenarios.
SpicyLemon
reviewed
Jul 31, 2025
SpicyLemon
left a comment
SpicyLemon
left a comment
Contributor
There was a problem hiding this comment.
I haven't looked at the tests yet, but I'm certain that we can and should be using an IndexedMap for both the name records and the address index.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
x/name/keeper/query_server.go (1)
56-65: Fix potential integer overflow in pagination limitThe conversion from
uint64tointcan cause overflow on 32-bit systems or when the limit value exceedsmath.MaxInt.// Handle pagination parameters if request.Pagination != nil { if request.Pagination.Limit > 0 { - limit = int(request.Pagination.Limit) + // Cap limit to prevent overflow and use default if too large + if request.Pagination.Limit > uint64(query.DefaultLimit) { + limit = query.DefaultLimit + } else if request.Pagination.Limit > uint64(^uint(0)>>1) { + // Prevent overflow on 32-bit systems + limit = int(^uint(0) >> 1) + } else { + limit = int(request.Pagination.Limit) + } }x/name/keeper/migrations.go (3)
37-37: Replace MustUnmarshal to prevent chain halt.Using
MustUnmarshalin a migration can panic and halt the chain if unmarshaling fails. This is a critical issue that must be fixed.Apply this fix:
- m.keeper.cdc.MustUnmarshal(bz, ¶ms) + if err := m.keeper.cdc.Unmarshal(bz, ¶ms); err != nil { + return fmt.Errorf("failed to unmarshal params: %w", err) + }
42-42: Check error return from store.Delete.The error return value from
store.Deleteis not checked, which could mask failures during migration.Apply this fix:
- store.Delete(types.NameParamStoreKey) + if err := store.Delete(types.NameParamStoreKey); err != nil { + return fmt.Errorf("failed to delete old param key: %w", err) + }
28-63: Migration logic is fundamentally incomplete.Based on past review comments, this migration approach is flawed. The migration should:
- Use old key prefixes to iterate over existing name records
- Properly migrate name records from old KV format to new collections format
- Not update the same store while iterating over it
- Use a new key prefix approach as suggested in past reviews
The current implementation only migrates parameters and clears address index entries, but doesn't migrate the actual name records.
🧹 Nitpick comments (5)
x/name/keeper/query_server.go (1)
35-36: Remove unnecessary blank linefunc (k Keeper) ReverseLookup(c context.Context, request *types.QueryReverseLookupRequest) (*types.QueryReverseLookupResponse, error) { - ctx := sdk.UnwrapSDKContext(c)x/name/keeper/genesis.go (1)
32-35: Use underscore for unused parameterThe
nameparameter in the Walk callback is unused.- err := k.nameRecords.Walk(ctx, nil, func(name string, record types.NameRecord) (bool, error) { + err := k.nameRecords.Walk(ctx, nil, func(_ string, record types.NameRecord) (bool, error) { records = append(records, record) return false, nil })x/name/keeper/keeper_test.go (1)
665-710: Use test logging instead of PrintfReplace
fmt.Printfwith test logging for better test output control.// 6. Verify key lengths (optional logs) - fmt.Printf("NameKey (NameRecords) length: %d\n", len(nameKey)) - fmt.Printf("NameKey bytes: %X\n", nameKey) + s.T().Logf("NameKey (NameRecords) length: %d", len(nameKey)) + s.T().Logf("NameKey bytes: %X", nameKey)x/name/keeper/keeper.go (2)
72-72: Fix unused parameter in index function.The
nameparameter in the index function is unused. Either use it or rename it to_to indicate it's intentionally unused.Apply this fix:
- func(name string, record types.NameRecord) ([]byte, error) { + func(_ string, record types.NameRecord) ([]byte, error) {
340-340: Fix append assignment pattern.The
appendresult should be assigned to the same slice variable.Apply this fix:
- nameKey := append(types.NameKeyPrefix, nameHash...) + nameKey = append(types.NameKeyPrefix, nameHash...)Or better yet, use a separate variable:
- nameKey := append(types.NameKeyPrefix, nameHash...) + nameKey := make([]byte, len(types.NameKeyPrefix)+len(nameHash)) + copy(nameKey, types.NameKeyPrefix) + copy(nameKey[len(types.NameKeyPrefix):], nameHash)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/app.go(2 hunks)x/attribute/keeper/query_server_test.go(2 hunks)x/name/keeper/export_test.go(1 hunks)x/name/keeper/genesis.go(1 hunks)x/name/keeper/keeper.go(5 hunks)x/name/keeper/keeper_test.go(8 hunks)x/name/keeper/migration_test.go(1 hunks)x/name/keeper/migrations.go(2 hunks)x/name/keeper/query_server.go(1 hunks)x/name/types/keys.go(2 hunks)x/name/types/keys_test.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/attribute/keeper/query_server_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- app/app.go
- x/name/keeper/migration_test.go
- x/name/types/keys_test.go
🧰 Additional context used
🧠 Learnings (38)
📓 Common learnings
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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
PR: provenance-io/provenance#2176
File: app/upgrades.go:41-41
Timestamp: 2024-10-10T23:14:29.308Z
Learning: In the `provenance` codebase, the constant `paramsName` is defined in `app/app.go` and is accessible in `app/upgrades.go` since they are in the same package.
📚 Learning: in `x/marker/types/send_restrictions.go`, the `transferagentkey` is only used for storing data in ep...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/export_test.gox/name/keeper/migrations.gox/name/types/keys.gox/name/keeper/keeper.go
📚 Learning: in test helper functions like `setbalances` in `x/metadata/keeper/bank_test.go`, it's acceptable to ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/export_test.gox/name/keeper/keeper_test.gox/name/keeper/keeper.go
📚 Learning: the `v3writenewscope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migr...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/export_test.gox/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: the `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned t...
Learnt from: SpicyLemon
PR: provenance-io/provenance#1874
File: cmd/dbmigrate/utils/migrator_test.go:10-10
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned to be removed from the project soon, as indicated by the user. Therefore, suggestions for changes or tests related to this directory are unnecessary.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `validate` method wi...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper_test.go
📚 Learning: in the codebase, the other `newparams` functions are intended to remain as they are and do not need ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: x/metadata/types/oslocatorparams.go:7-9
Timestamp: 2024-10-10T23:13:15.277Z
Learning: In the codebase, the other `NewParams` functions are intended to remain as they are and do not need to be renamed.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in the `app` package, the constant `paramsname` is defined in `app/app.go` and can be used in `app/u...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: app/upgrades.go:55-55
Timestamp: 2024-10-10T23:16:32.311Z
Learning: In the `app` package, the constant `paramsName` is defined in `app/app.go` and can be used in `app/upgrades.go`.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in the `provenance` codebase, the constant `paramsname` is defined in `app/app.go` and is accessible...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: app/upgrades.go:41-41
Timestamp: 2024-10-10T23:14:29.308Z
Learning: In the `provenance` codebase, the constant `paramsName` is defined in `app/app.go` and is accessible in `app/upgrades.go` since they are in the same package.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in the `migratevalueowner` method, use `sdkerrors.errlogic` when representing unexpected backend err...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/msg_server.go:262-263
Timestamp: 2024-10-08T18:12:57.975Z
Learning: In the `MigrateValueOwner` method, use `sdkerrors.ErrLogic` when representing unexpected backend errors, as the request itself is valid.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: the user has updated the governance parameter structure in `cmd/provenanced/cmd/init.go` to `govgens...
Learnt from: Taztingo
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: in the `subset` function used in unit tests, it's acceptable for it to panic if an out-of-bounds ind...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/address_test.go:69-80
Timestamp: 2024-10-08T18:12:51.935Z
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/name/keeper/migrations.go
📚 Learning: in `app/upgrades_test.go`, we prefer to keep todo comments (like `todo[viridian]`) to handle code de...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: in go tests, modifying byte slices directly using arithmetic operations like addition and subtractio...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/query_server.go
📚 Learning: in go tests within this project, it's acceptable to have similar helper functions like `newuuid`, `n...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.gox/name/types/keys.go
📚 Learning: the new queries `queryattributeaccountsresponse` and `queryaccountdataresponse` in the attribute mod...
Learnt from: Taztingo
PR: provenance-io/provenance#2003
File: internal/provwasm/stargate_whitelist.go:86-87
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The new queries `QueryAttributeAccountsResponse` and `QueryAccountDataResponse` in the Attribute module are tested by their respective modules, as confirmed by the presence of these queries in various test files such as `x/attribute/keeper/query_server_test.go`, `x/attribute/client/rest/grpc_query_test.go`, and `x/attribute/client/cli/cli_test.go`.
Applied to files:
x/name/keeper/keeper_test.go
📚 Learning: do not raise issues about the `keeper` package still being in use after import removal in test files...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.gox/name/keeper/keeper.go
📚 Learning: in `app/app_test.go`, when testing that the encoder is correctly wired to handle a governance `propo...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.go
📚 Learning: in go tests where test cases are defined within a test case slice and iterated with `t.run`, instanc...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils_test.go:1837-1889
Timestamp: 2024-10-02T03:31:48.658Z
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/name/keeper/keeper_test.go
📚 Learning: spicylemon prefers allowing tests to fail naturally if the setup is incorrect rather than explicitly...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.go
📚 Learning: when test cases are run using `s.t().run` instead of `s.run`, use `require` with `t` instead of `s.r...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.go
📚 Learning: when mocking methods like `mintcoins` and `burncoins` in tests, using slices to store error strings ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.gox/name/keeper/keeper.go
📚 Learning: in this codebase, methods like `getacc()` and `getsigneracc()` are intended to return `nil` if the a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.gox/name/types/keys.gox/name/keeper/keeper.gox/name/keeper/genesis.go
📚 Learning: when using `sdk.accaddress` as map keys in `authzcache`, casting to `string(addr)` is acceptable and...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils.go:306-308
Timestamp: 2024-10-02T02:20:49.611Z
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/name/keeper/keeper_test.gox/name/types/keys.gox/name/keeper/keeper.go
📚 Learning: in this codebase, when using `sdk.accaddress` as map keys, casting to `string` is preferred over cal...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.gox/name/types/keys.gox/name/keeper/keeper.gox/name/keeper/genesis.go
📚 Learning: in the `testscopequery` function, the conditional checks following the equality assertion are design...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper_test.go
📚 Learning: in `cli_test.go` test cases, the `expout` field may include indentation levels and field names with ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2197
File: x/metadata/client/cli/cli_test.go:1474-1480
Timestamp: 2024-10-23T18:46:02.430Z
Learning: In `cli_test.go` test cases, the `expOut` field may include indentation levels and field names with extra spaces to match the YAML output format. Including partial outputs in `expOut` is acceptable as long as they verify the necessary parts.
Applied to files:
x/name/keeper/keeper_test.go
📚 Learning: the expected return value for the `getsigners()` function in the `quarantine` module, when an addres...
Learnt from: SpicyLemon
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The expected return value for the `GetSigners()` function in the `quarantine` module, when an address is bad, is a list with a single `nil` entry. This behavior is intentionally tested and confirmed by user SpicyLemon.
Applied to files:
x/name/keeper/keeper_test.go
📚 Learning: the `getsigners()` function in the `quarantine` module is designed to include a `nil` entry in the r...
Learnt from: SpicyLemon
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `GetSigners()` function in the `quarantine` module is designed to include a `nil` entry in the result when it encounters a bad address. This behavior is intentionally tested in the unit tests.
Applied to files:
x/name/keeper/keeper_test.go
📚 Learning: the `createautoresponsetoaddrprefix` function in the quarantine module should have unit tests that a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/name/keeper/keeper_test.go
📚 Learning: in `setscopevalueowner`, no additional validation is needed for `fromaddr` and `toaddr` since they a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/scope.go:228-284
Timestamp: 2024-10-02T03:10:33.082Z
Learning: In `SetScopeValueOwner`, no additional validation is needed for `fromAddr` and `toAddr` since they are `sdk.AccAddress` types.
Applied to files:
x/name/types/keys.go
📚 Learning: in the metadata keeper, panics are intentionally used when `authzcache` is not found in the context,...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils.go:421-427
Timestamp: 2024-10-02T02:34:13.721Z
Learning: In the metadata keeper, panics are intentionally used when `AuthzCache` is not found in the context, because many methods cannot return an error, and it's considered a programming error to call these methods without an `AuthzCache`.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: in the `balancevalueownertransformer` function in `x/metadata/keeper/bank.go`, it is intended to alw...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: - when defining interfaces that need to satisfy external modules (like `bankkeeper`), suggesting cha...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/expected_keepers.go:39-49
Timestamp: 2024-10-02T01:58:51.519Z
Learning: - When defining interfaces that need to satisfy external modules (like `BankKeeper`), suggesting changes to method signatures may not be appropriate as they are outside of our control.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: the variable `envprefix` is defined in `app/prefix.go`, so it is available for use throughout the co...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2175
File: app/app.go:311-311
Timestamp: 2024-10-11T17:35:59.823Z
Learning: The variable `EnvPrefix` is defined in `app/prefix.go`, so it is available for use throughout the codebase.
Applied to files:
x/name/keeper/keeper.gox/name/keeper/query_server.go
📚 Learning: when unit tests exist for a key generation function, it's preferable to reuse that function in tests...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-02T03:06:21.997Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: - in v0.50.x of the sdk, they have started the transition to using `context.context` instead of `sdk...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: the `depguard` definitions in `.golangci.yml` are prefixes (unless they end with `$`), so the entry ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
🧬 Code Graph Analysis (3)
x/name/keeper/migrations.go (3)
x/attribute/keeper/migrations.go (1)
Migrator(4-6)x/marker/keeper/migrations.go (1)
Migrator(4-6)x/name/types/keys.go (2)
NameParamStoreKey(32-32)AddressKeyPrefix(30-30)
x/name/keeper/keeper_test.go (4)
x/marker/types/expected_keepers.go (1)
NameKeeper(73-75)x/attribute/keeper/keeper_test.go (1)
KeeperTestSuite(24-39)x/name/types/keys.go (1)
GetNameKeyBytes(102-108)x/name/types/name.pb.go (3)
NameRecord(102-109)NameRecord(112-112)NameRecord(113-115)
x/name/keeper/keeper.go (6)
x/name/types/name.pb.go (3)
NameRecord(102-109)NameRecord(112-112)NameRecord(113-115)x/name/types/expected_keepers.go (1)
AttributeKeeper(14-17)x/name/types/keys.go (6)
AddressKeyPrefix(30-30)HashedStringKeyCodec(36-36)NameKeyPrefix(28-28)NameParamStoreKey(32-32)ModuleName(17-17)ValidateAddress(144-146)x/name/types/errors.go (5)
ErrInvalidAddress(22-22)ErrNameAlreadyBound(12-12)ErrNameNotBound(10-10)ErrNameInvalid(14-14)ErrNameHasTooManySegments(20-20)x/name/types/name.go (3)
NewNameRecord(14-20)NormalizeName(42-49)IsValidName(53-55)x/name/types/events.go (3)
NewEventNameBound(17-23)NewEventNameUpdate(33-39)NewEventNameUnbound(25-31)
🪛 GitHub Check: golangci-lint
x/name/keeper/migrations.go
[failure] 52-52:
Error return value of store.Set is not checked (errcheck)
[failure] 42-42:
Error return value of store.Delete is not checked (errcheck)
x/name/types/keys.go
[failure] 51-51:
unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _ (revive)
x/name/keeper/keeper.go
[failure] 72-72:
unused-parameter: parameter 'name' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 340-340:
appendAssign: append result not assigned to the same slice (gocritic)
[failure] 381-381:
Error return value of store.Delete is not checked (errcheck)
x/name/keeper/genesis.go
[failure] 32-32:
unused-parameter: parameter 'name' seems to be unused, consider removing or renaming it as _ (revive)
x/name/keeper/query_server.go
[failure] 35-35:
unnecessary leading newline (whitespace)
[failure] 59-59:
G115: integer overflow conversion uint64 -> int (gosec)
🪛 GitHub Actions: Lint
x/name/keeper/keeper.go
[error] 381-381: golangci-lint: Error return value of store.Delete is not checked (errcheck)
⏰ 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). (7)
- GitHub Check: runsim (import-export, goleveldb, ubuntu-latest)
- GitHub Check: runsim (multi-seed-short, goleveldb, ubuntu-latest)
- GitHub Check: runsim (after-import, goleveldb, ubuntu-latest)
- GitHub Check: test-race (00)
- GitHub Check: test-race (01)
- GitHub Check: tests (00)
- GitHub Check: tests (01)
🔇 Additional comments (16)
x/name/types/keys.go (3)
101-108: LGTM!The
GetNameKeyBytesfunction correctly constructs the name key by prefixing the SHA-256 hash with the name key prefix byte.
110-126: LGTM!The address key functions correctly implement length-prefixed address keys for the collections framework's indexed map functionality.
128-142: LGTM!The
computeNameHashfunction properly validates name segments and implements the hierarchical hashing logic for name resolution.x/name/keeper/export_test.go (1)
8-11: LGTM!The test helper method correctly delegates to the new
SetNameRecordAPI, aligning with the collections-based keeper implementation.x/name/keeper/query_server.go (1)
27-31: LGTM!The
Resolvemethod correctly uses the collections API to retrieve name records.x/name/keeper/genesis.go (1)
12-21: Verify skipping normalization is intentionalThe method sets name records directly without normalization checks. While this might be intentional for genesis import to preserve exact state, it could allow invalid names to be imported.
Please confirm that skipping normalization during genesis import is intentional and won't cause issues with invalid names.
x/name/keeper/keeper_test.go (2)
255-260: LGTM!The updated error message correctly reflects the new validation logic in the collections-based keeper.
291-331: LGTM!The test correctly uses the new
SetNameRecordAPI and properly asserts empty results usings.Require().Empty().x/name/keeper/migrations.go (1)
58-58: Clarify the commented out DeleteInvalidAddressIndexEntries call.This commented out call suggests incomplete migration logic. Should this be executed as part of the migration, or is it intentionally disabled?
x/name/keeper/keeper.go (7)
23-32: LGTM! Well-structured collections index definition.The
NameRecordIndexesstruct and itsIndexesListimplementation correctly follow the collections framework pattern for defining indexed maps.
34-54: LGTM! Keeper struct properly refactored for collections.The Keeper struct has been correctly refactored to use the collections framework with
storeServiceinstead ofstoreKey(addressing past review feedback), and includes the appropriate collections components.
56-118: LGTM! Excellent collections setup in NewKeeper.The collections initialization properly sets up the indexed map, address index, and parameter store. The schema building and error handling are appropriate.
151-284: LGTM! Excellent refactor of CRUD methods to collections.All the core methods (
SetNameRecord,UpdateNameRecord,GetRecordByName,NameExists,GetRecordsByAddress,DeleteRecord,IterateRecords) have been properly refactored to use the collections API. The logic is clean, error handling is appropriate, and the address index is used correctly.
386-391: LGTM! Clean helper methods for collections access.The
GetNameRecordandGetAddrIndexmethods provide appropriate access to the underlying collections components.
393-422: LGTM! CreateRootName properly updated for collections.The method has been correctly updated to use
NameExistsand maintains the proper logic for creating intermediate domains. The addition of the root name existence check is a good improvement.
1-423: Outstanding refactor to collections framework!This is a comprehensive and well-executed migration from manual KV store operations to the modern collections framework. The refactor:
- Properly implements IndexedMap with address indexing
- Maintains all existing functionality while improving type safety
- Uses appropriate error handling throughout
- Follows collections best practices
This represents a significant improvement in code maintainability and type safety for the name module.
SpicyLemon
reviewed
Aug 5, 2025
SpicyLemon
reviewed
Aug 5, 2025
SpicyLemon
reviewed
Aug 5, 2025
SpicyLemon
reviewed
Aug 6, 2025
nagarajdivine
force-pushed
the
feature/collections-name-module
branch
from
f70cafe to
7d63106
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
x/name/keeper/migrations.go (1)
25-87: Critical: Migration implementation is incomplete and potentially corrupted.The migration has several critical issues that were identified in previous reviews but remain unaddressed:
Same prefix for read/write: You're iterating over
types.NameKeyPrefixand writing to collections that also usetypes.NameKeyPrefix. This violates the fundamental rule of never writing to what you're iterating over.Old entries not deleted: After migration, the old KV store entries remain, leading to duplicate data.
Address index not handled: The old address index entries (prefix 0x05) are neither migrated nor cleaned up.
Parameters unnecessarily touched: As noted in past reviews, parameters don't need migration since they use the same key and encoding.
To fix this migration:
+// Define old prefixes (copy current values before changing them) +var ( + OldNameKeyPrefix = []byte{0x03} + OldAddressKeyPrefix = []byte{0x05} +) func (m Migrator) MigrateKVToCollections2to3(ctx sdk.Context) error { ctx.Logger().Info("Migrating name module from KV store to collections (v2 to v3)...") store := m.keeper.storeService.OpenKVStore(ctx) ctx.Logger().Info("Phase 1: Collecting existing name records...") var recordsToMigrate []struct { name string record types.NameRecord } + var oldKeys [][]byte // Iterate over name records. - namePrefix := types.NameKeyPrefix + namePrefix := OldNameKeyPrefix iter, err := store.Iterator(namePrefix, storetypes.PrefixEndBytes(namePrefix)) if err != nil { return fmt.Errorf("failed to create name records iterator: %w", err) } defer iter.Close() for ; iter.Valid(); iter.Next() { key := iter.Key() value := iter.Value() + oldKeys = append(oldKeys, key) if len(key) <= len(namePrefix) { continue } var record types.NameRecord if err := m.keeper.cdc.Unmarshal(value, &record); err != nil { ctx.Logger().Error("failed to unmarshal name record during migration", "key", fmt.Sprintf("%x", key), "error", err) continue } if record.Name == "" { ctx.Logger().Error("name record has empty name field", "key", fmt.Sprintf("%x", key)) continue } recordsToMigrate = append(recordsToMigrate, struct { name string record types.NameRecord }{ name: record.Name, record: record, }) } ctx.Logger().Info(fmt.Sprintf("Found %d name records to migrate", len(recordsToMigrate))) ctx.Logger().Info("Phase 2: Migrating records to collections...") for _, item := range recordsToMigrate { if err := m.keeper.nameRecords.Set(ctx, item.name, item.record); err != nil { ctx.Logger().Error("failed to set record in nameRecords", "name", item.name, "address", item.record.Address, "error", err) return fmt.Errorf("failed to migrate name record %s: %w", item.name, err) } ctx.Logger().Info("Record written to collections", "name", item.name, "address", item.record.Address) } ctx.Logger().Info(fmt.Sprintf("Successfully migrated %d name records to collections", len(recordsToMigrate))) + ctx.Logger().Info("Phase 3: Cleaning up old entries...") + // Delete old name records + for _, key := range oldKeys { + if err := store.Delete(key); err != nil { + return fmt.Errorf("failed to delete old name record: %w", err) + } + } + + // Delete old address index entries + addrIter, err := store.Iterator(OldAddressKeyPrefix, storetypes.PrefixEndBytes(OldAddressKeyPrefix)) + if err != nil { + return fmt.Errorf("failed to create address index iterator: %w", err) + } + defer addrIter.Close() + + var oldAddrKeys [][]byte + for ; addrIter.Valid(); addrIter.Next() { + oldAddrKeys = append(oldAddrKeys, addrIter.Key()) + } + + for _, key := range oldAddrKeys { + if err := store.Delete(key); err != nil { + return fmt.Errorf("failed to delete old address index: %w", err) + } + } + + ctx.Logger().Info(fmt.Sprintf("Deleted %d old name records and %d address index entries", len(oldKeys), len(oldAddrKeys))) + ctx.Logger().Info("Name module migration to collections (v2 to v3) completed successfully.") return nil }Important: Before deploying this migration, you must:
- Change the value of
types.NameKeyPrefixinx/name/types/keys.goto a new value (e.g.,[]byte{0x07})- Keep the old prefix values as constants in this migration file
- Ensure the collections framework uses the new prefix value
x/name/keeper/keeper.go (1)
293-366: Check error return from store.Delete.The error from
store.Deleteat line 363 should be checked to ensure cleanup operations complete successfully.Apply this fix:
for _, key := range toDelete { - store.Delete(key) + if err := store.Delete(key); err != nil { + logger.Error("failed to delete invalid address index entry", "key", fmt.Sprintf("%x", key), "error", err) + } }
🧹 Nitpick comments (1)
x/name/keeper/migration_test.go (1)
67-122: Test should verify cleanup of old entries.The test validates that data is migrated correctly but doesn't verify that old KV store entries are cleaned up after migration. This is important to ensure no duplicate data remains.
Add verification that old entries are deleted:
func (s *MigrationTestSuite) TestMigration() { // ... existing setup ... err = migrator.MigrateKVToCollections2to3(s.ctx) s.Require().NoError(err, "migration failed") + // Verify old entries are deleted + oldStore := s.ctx.KVStore(storeKey) + + // Check that old name record is deleted + nameKey, err := types.GetNameKeyBytes(name) + s.Require().NoError(err) + s.Require().False(oldStore.Has(nameKey), "old name record should be deleted") + + // Check that old address index is deleted + addrIndexKey, err := types.GetAddressKeyBytes(s.user1Addr, nameKey[1:]) + s.Require().NoError(err) + s.Require().False(oldStore.Has(addrIndexKey), "old address index should be deleted") // ... rest of verifications ... }Note: This verification will fail with the current migration implementation, which doesn't delete old entries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.changelog/unreleased/features/2411-collections-name-module.md(1 hunks)app/app.go(1 hunks)x/attribute/keeper/keeper_test.go(1 hunks)x/attribute/keeper/query_server_test.go(2 hunks)x/name/keeper/export_test.go(1 hunks)x/name/keeper/genesis.go(1 hunks)x/name/keeper/keeper.go(5 hunks)x/name/keeper/keeper_test.go(9 hunks)x/name/keeper/migration_test.go(1 hunks)x/name/keeper/migrations.go(2 hunks)x/name/keeper/params.go(1 hunks)x/name/keeper/query_server.go(2 hunks)x/name/module.go(2 hunks)x/name/types/keys.go(2 hunks)x/name/types/keys_test.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/app.go
🚧 Files skipped from review as they are similar to previous changes (9)
- .changelog/unreleased/features/2411-collections-name-module.md
- x/name/module.go
- x/name/keeper/genesis.go
- x/attribute/keeper/query_server_test.go
- x/name/keeper/export_test.go
- x/name/keeper/query_server.go
- x/attribute/keeper/keeper_test.go
- x/name/keeper/keeper_test.go
- x/name/keeper/params.go
🧰 Additional context used
🧠 Learnings (38)
📓 Common learnings
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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.
📚 Learning: in go tests within this project, it's acceptable to have similar helper functions like `newuuid`, `n...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.gox/name/keeper/migration_test.gox/name/types/keys.go
📚 Learning: when unit tests exist for a key generation function, it's preferable to reuse that function in tests...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-08T18:12:51.936Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/name/types/keys_test.gox/name/keeper/migration_test.gox/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: in `app/app_test.go`, when testing that the encoder is correctly wired to handle a governance `propo...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.gox/name/keeper/migration_test.go
📚 Learning: in go tests, modifying byte slices directly using arithmetic operations like addition and subtractio...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils_test.go:478-493
Timestamp: 2024-10-08T18:12:51.935Z
Learning: In Go tests, modifying byte slices directly using arithmetic operations like addition and subtraction is acceptable, even if it causes overflow, as overflow on `uint8` just wraps around. Avoid flagging such usage, and avoid suggesting bitwise operations which can be harder to read and understand.
Applied to files:
x/name/types/keys_test.gox/name/keeper/migrations.go
📚 Learning: when mocking methods like `mintcoins` and `burncoins` in tests, using slices to store error strings ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.gox/name/keeper/keeper.go
📚 Learning: in `scope_test.go`, numeric literals are intentionally used for the `role` field in tests instead of...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.go
📚 Learning: in this codebase, when using `sdk.accaddress` as map keys, casting to `string` is preferred over cal...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.gox/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: in `cli_test.go` test cases, the `expout` field may include indentation levels and field names with ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2197
File: x/metadata/client/cli/cli_test.go:1474-1480
Timestamp: 2024-10-23T18:46:02.430Z
Learning: In `cli_test.go` test cases, the `expOut` field may include indentation levels and field names with extra spaces to match the YAML output format. Including partial outputs in `expOut` is acceptable as long as they verify the necessary parts.
Applied to files:
x/name/types/keys_test.go
📚 Learning: when test cases are run using `s.t().run` instead of `s.run`, use `require` with `t` instead of `s.r...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.go
📚 Learning: in go tests where test cases are defined within a test case slice and iterated with `t.run`, instanc...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys_test.go
📚 Learning: in go unit tests, repetitive code is acceptable; refactoring for dry is not necessary....
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/exchange/keeper/mocks_test.go:427-431
Timestamp: 2024-10-02T03:16:46.324Z
Learning: In Go unit tests, repetitive code is acceptable; refactoring for DRY is not necessary.
Applied to files:
x/name/types/keys_test.go
📚 Learning: the `v3writenewscope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migr...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/migration_test.gox/name/keeper/keeper.go
📚 Learning: the `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned t...
Learnt from: SpicyLemon
PR: provenance-io/provenance#1874
File: cmd/dbmigrate/utils/migrator_test.go:10-10
Timestamp: 2024-10-08T18:12:57.974Z
Learning: The `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned to be removed from the project soon, as indicated by the user. Therefore, suggestions for changes or tests related to this directory are unnecessary.
Applied to files:
x/name/keeper/migrations.gox/name/keeper/migration_test.go
📚 Learning: in the file `x/ibchooks/types/params.go`, maintain the existing behavior in the `validate` method wi...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: in the codebase, the other `newparams` functions are intended to remain as they are and do not need ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: x/metadata/types/oslocatorparams.go:7-9
Timestamp: 2024-10-10T23:13:15.277Z
Learning: In the codebase, the other `NewParams` functions are intended to remain as they are and do not need to be renamed.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in the `app` package, the constant `paramsname` is defined in `app/app.go` and can be used in `app/u...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: app/upgrades.go:55-55
Timestamp: 2024-10-10T23:16:32.311Z
Learning: In the `app` package, the constant `paramsName` is defined in `app/app.go` and can be used in `app/upgrades.go`.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in the `provenance` codebase, the constant `paramsname` is defined in `app/app.go` and is accessible...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: app/upgrades.go:41-41
Timestamp: 2024-10-10T23:14:29.308Z
Learning: In the `provenance` codebase, the constant `paramsName` is defined in `app/app.go` and is accessible in `app/upgrades.go` since they are in the same package.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in the `migratevalueowner` method, use `sdkerrors.errlogic` when representing unexpected backend err...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/msg_server.go:262-263
Timestamp: 2024-10-08T18:12:57.975Z
Learning: In the `MigrateValueOwner` method, use `sdkerrors.ErrLogic` when representing unexpected backend errors, as the request itself is valid.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in `x/marker/types/send_restrictions.go`, the `transferagentkey` is only used for storing data in ep...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: the user has updated the governance parameter structure in `cmd/provenanced/cmd/init.go` to `govgens...
Learnt from: Taztingo
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: in the `subset` function used in unit tests, it's acceptable for it to panic if an out-of-bounds ind...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: in `app/upgrades_test.go`, we prefer to keep todo comments (like `todo[viridian]`) to handle code de...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/migration_test.go
📚 Learning: a github issue was created to track the migration of parameters for the metadata, marker, name, and ...
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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:
x/name/keeper/migrations.go
📚 Learning: do not raise issues about the `keeper` package still being in use after import removal in test files...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: the `createautoresponsetoaddrprefix` function in the quarantine module should have unit tests that a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: the `setscope` function in the `metadatakeeper` interface does not return an error....
Learnt from: nullpointer0x00
PR: provenance-io/provenance#2046
File: app/upgrades_test.go:760-841
Timestamp: 2024-06-21T13:43:19.231Z
Learning: The `SetScope` function in the `MetadataKeeper` interface does not return an error.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: in test helper functions like `setbalances` in `x/metadata/keeper/bank_test.go`, it's acceptable to ...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/migration_test.gox/name/keeper/keeper.go
📚 Learning: the `parsepaymentstorevalue` function in `x/exchange/keeper/payments.go` returns `nil, nil` when the...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: in the `deletescopeowner` method (and similar cases), redeclaring `err` with `:=` is appropriate whe...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/msg_server.go:206-209
Timestamp: 2024-10-02T03:54:36.749Z
Learning: In the `DeleteScopeOwner` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: when using `sdk.accaddress` as map keys in `authzcache`, casting to `string(addr)` is acceptable and...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: in the `balancevalueownertransformer` function in `x/metadata/keeper/bank.go`, it is intended to alw...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/bank.go:76-87
Timestamp: 2024-10-02T15:32:02.168Z
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/name/keeper/keeper.go
📚 Learning: - when defining interfaces that need to satisfy external modules (like `bankkeeper`), suggesting cha...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/expected_keepers.go:39-49
Timestamp: 2024-10-02T01:58:51.519Z
Learning: - When defining interfaces that need to satisfy external modules (like `BankKeeper`), suggesting changes to method signatures may not be appropriate as they are outside of our control.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: the variable `envprefix` is defined in `app/prefix.go`, so it is available for use throughout the co...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2175
File: app/app.go:311-311
Timestamp: 2024-10-11T17:35:59.823Z
Learning: The variable `EnvPrefix` is defined in `app/prefix.go`, so it is available for use throughout the codebase.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: in this codebase, methods like `getacc()` and `getsigneracc()` are intended to return `nil` if the a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: the `getlockedcoins` function in `x/hold/keeper/locked_coins.go` must conform to the `banktypes.getl...
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: the authority address validation is handled in the `validatebasic` method for the message in the `x/...
Learnt from: nullpointer0x00
PR: provenance-io/provenance#1984
File: x/ibcratelimit/msgs.go:29-29
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The authority address validation is handled in the `ValidateBasic` method for the message in the `x/ibcratelimit` module, while the `Params` struct is responsible for validating the `ContractAddress` property.
Applied to files:
x/name/types/keys.go
📚 Learning: in `setscopevalueowner`, no additional validation is needed for `fromaddr` and `toaddr` since they a...
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/scope.go:228-284
Timestamp: 2024-10-02T03:10:33.082Z
Learning: In `SetScopeValueOwner`, no additional validation is needed for `fromAddr` and `toAddr` since they are `sdk.AccAddress` types.
Applied to files:
x/name/types/keys.go
🧬 Code Graph Analysis (3)
x/name/keeper/migrations.go (3)
x/attribute/keeper/migrations.go (1)
Migrator(4-6)x/marker/keeper/migrations.go (1)
Migrator(4-6)x/name/types/keys.go (1)
NameKeyPrefix(30-30)
x/name/keeper/migration_test.go (4)
x/name/types/keys.go (5)
StoreKey(22-22)NameParamStoreKey(34-34)GetNameKeyBytes(113-119)GetAddressKeyPrefix(121-128)GetAddressKeyBytes(131-137)x/name/types/name.go (1)
NewNameRecord(14-20)x/name/keeper/keeper.go (1)
NewKeeper(47-105)x/name/keeper/migrations.go (1)
NewMigrator(19-21)
x/name/types/keys.go (4)
internal/provutils/pair.go (1)
Pair(6-9)x/name/types/name.pb.go (3)
NameRecord(102-109)NameRecord(112-112)NameRecord(113-115)x/attribute/types/keys.go (1)
GetNameKeyBytes(107-115)x/name/types/errors.go (1)
ErrNameInvalid(14-14)
⏰ 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). (15)
- GitHub Check: test-race (03)
- GitHub Check: test-race (02)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: test-race (00)
- GitHub Check: tests (00)
- GitHub Check: test-race (04)
- GitHub Check: test-race (01)
- GitHub Check: tests (04)
- GitHub Check: build-linux
- GitHub Check: Build OSX
- GitHub Check: Build Linux
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
🔇 Additional comments (8)
x/name/types/keys_test.go (2)
40-96: LGTM!Test updates correctly reflect the new key encoding function and improved error messages.
124-214: Excellent test coverage for the new codec!The test thoroughly validates all aspects of the
HashedStringKeyCodecincluding encoding, decoding, JSON operations, and hash uniqueness. Good coverage of edge cases.x/name/keeper/keeper.go (3)
46-105: Well-structured collections initialization!The keeper constructor correctly sets up the IndexedMap with address multi-index, following the collections framework patterns. The panic on schema build failure is appropriate for initialization.
148-196: Clean implementation of record management methods!Both
SetNameRecordandUpdateNameRecordcorrectly use the collections API with proper validation and event emission. The automatic index management through IndexedMap eliminates the need for manual index updates.
368-374: Useful helper methods for collections access.These methods appropriately expose the collections internals for migration and query operations.
x/name/types/keys.go (3)
37-46: Correct multi-index implementation!The
NameRecordIndexesproperly implements the collections index interface for address-based lookups.
48-110: Well-implemented custom key codec!The
HashedStringKeyCodeccorrectly implements all required methods for the collections framework. The use of SHA-256 hashing ensures consistent key sizes, and returning base64-encoded hashes on decode is the appropriate approach since hashes cannot be reversed.
112-158: Key generation functions maintain backward compatibility!The functions correctly preserve the existing key format while adding proper validation. The
computeNameHashfunction appropriately validates that name segments aren't empty, preventing invalid keys.
nagarajdivine
force-pushed
the
feature/collections-name-module
branch
from
aa47330 to
750c456
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
x/name/keeper/migrations.go (1)
37-44: Close the iterator before writing to the same storeThe deferred
iter.Close()keeps the iterator open while Phase 2 writes to the same key-space, violating SDK iterator safety rules and risking panic/corruption.
Calliter.Close()(or move Phase 2 into a new function) before anynameRecords.Setcalls.
🧹 Nitpick comments (1)
x/name/keeper/keeper.go (1)
241-255: Prefix filtering logic is incorrect
IterateRecordscomparesprefixtotypes.NameKeyPrefixinstead of using it directly, so any caller-supplied prefix other than the module’s constant is ignored.
Replace thebytes.Equalcheck with just:if prefix != nil && !bytes.HasPrefix([]byte(record.Name), prefix) { return false, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.changelog/unreleased/features/2411-collections-name-module.md(1 hunks)app/app.go(1 hunks)x/attribute/keeper/keeper_test.go(1 hunks)x/attribute/keeper/query_server_test.go(2 hunks)x/name/keeper/genesis.go(1 hunks)x/name/keeper/keeper.go(5 hunks)x/name/keeper/keeper_test.go(9 hunks)x/name/keeper/migration_test.go(1 hunks)x/name/keeper/migrations.go(2 hunks)x/name/keeper/params.go(1 hunks)x/name/keeper/query_server.go(2 hunks)x/name/module.go(2 hunks)x/name/types/keys.go(2 hunks)x/name/types/keys_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- .changelog/unreleased/features/2411-collections-name-module.md
- x/name/keeper/params.go
- x/attribute/keeper/query_server_test.go
- x/name/module.go
- app/app.go
- x/name/keeper/genesis.go
- x/attribute/keeper/keeper_test.go
- x/name/keeper/migration_test.go
- x/name/keeper/keeper_test.go
- x/name/types/keys_test.go
🧰 Additional context used
🧠 Learnings (33)
📓 Common learnings
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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.
📚 Learning: 2024-10-02T02:06:21.664Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:57.974Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#1874
File: cmd/dbmigrate/utils/migrator_test.go:10-10
Timestamp: 2024-10-08T18:12:57.974Z
Learning: The `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned to be removed from the project soon, as indicated by the user. Therefore, suggestions for changes or tests related to this directory are unnecessary.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-10T23:13:56.722Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: 2024-10-10T23:13:15.277Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: x/metadata/types/oslocatorparams.go:7-9
Timestamp: 2024-10-10T23:13:15.277Z
Learning: In the codebase, the other `NewParams` functions are intended to remain as they are and do not need to be renamed.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-10T23:16:32.311Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: app/upgrades.go:55-55
Timestamp: 2024-10-10T23:16:32.311Z
Learning: In the `app` package, the constant `paramsName` is defined in `app/app.go` and can be used in `app/upgrades.go`.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-10T23:14:29.308Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2176
File: app/upgrades.go:41-41
Timestamp: 2024-10-10T23:14:29.308Z
Learning: In the `provenance` codebase, the constant `paramsName` is defined in `app/app.go` and is accessible in `app/upgrades.go` since they are in the same package.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-08T18:12:57.975Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/msg_server.go:262-263
Timestamp: 2024-10-08T18:12:57.975Z
Learning: In the `MigrateValueOwner` method, use `sdkerrors.ErrLogic` when representing unexpected backend errors, as the request itself is valid.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-02T01:43:01.360Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: Taztingo
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: 2024-10-02T03:49:31.991Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: 2024-10-10T23:19:54.565Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils_test.go:478-493
Timestamp: 2024-10-08T18:12:51.935Z
Learning: In Go tests, modifying byte slices directly using arithmetic operations like addition and subtraction is acceptable, even if it causes overflow, as overflow on `uint8` just wraps around. Avoid flagging such usage, and avoid suggesting bitwise operations which can be harder to read and understand.
Applied to files:
x/name/keeper/migrations.gox/name/keeper/query_server.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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:
x/name/keeper/migrations.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-06-21T13:43:19.231Z
Learnt from: nullpointer0x00
PR: provenance-io/provenance#2046
File: app/upgrades_test.go:760-841
Timestamp: 2024-06-21T13:43:19.231Z
Learning: The `SetScope` function in the `MetadataKeeper` interface does not return an error.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-02T02:51:17.452Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: 2024-10-02T03:54:36.749Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/msg_server.go:206-209
Timestamp: 2024-10-02T03:54:36.749Z
Learning: In the `DeleteScopeOwner` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: 2024-10-11T17:35:59.823Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2175
File: app/app.go:311-311
Timestamp: 2024-10-11T17:35:59.823Z
Learning: The variable `EnvPrefix` is defined in `app/prefix.go`, so it is available for use throughout the codebase.
Applied to files:
x/name/keeper/query_server.gox/name/keeper/keeper.go
📚 Learning: 2024-10-02T01:58:51.519Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-10T23:08:15.256Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-02T02:34:13.721Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils.go:421-427
Timestamp: 2024-10-02T02:34:13.721Z
Learning: In the metadata keeper, panics are intentionally used when `AuthzCache` is not found in the context, because many methods cannot return an error, and it's considered a programming error to call these methods without an `AuthzCache`.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: 2024-10-02T16:55:10.172Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: 2024-10-02T15:32:02.168Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/bank.go:76-87
Timestamp: 2024-10-02T15:32:02.168Z
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/name/keeper/keeper.go
📚 Learning: 2024-10-02T01:58:51.519Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/expected_keepers.go:39-49
Timestamp: 2024-10-02T01:58:51.519Z
Learning: - When defining interfaces that need to satisfy external modules (like `BankKeeper`), suggesting changes to method signatures may not be appropriate as they are outside of our control.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: 2024-10-02T17:24:43.269Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-08T18:12:51.936Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: 2024-10-02T03:04:32.929Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/types/keys.go
🧬 Code Graph Analysis (1)
x/name/keeper/migrations.go (3)
x/attribute/keeper/migrations.go (1)
Migrator(4-6)x/marker/keeper/migrations.go (1)
Migrator(4-6)x/name/types/keys.go (1)
NameKeyPrefix(30-30)
⏰ 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). (15)
- GitHub Check: tests (02)
- GitHub Check: tests (04)
- GitHub Check: tests (00)
- GitHub Check: test-race (03)
- GitHub Check: test-race (02)
- GitHub Check: tests (01)
- GitHub Check: test-race (01)
- GitHub Check: tests (03)
- GitHub Check: test-race (04)
- GitHub Check: test-race (00)
- GitHub Check: build-linux
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
🔇 Additional comments (2)
x/name/types/keys.go (1)
67-75: EncodeJSON/DecodeJSON return raw name, not hash
EncodeJSON&DecodeJSONmarshal the input string, not the hashed representation produced byEncode.
Down-stream tooling expecting a JSON dump of the stored key will see unhashed names, which cannot be looked up directly.
Consider marshalling the base64-encoded hash (same value returned byDecode).x/name/keeper/keeper.go (1)
60-67: Address -> name join uses full address bytes
collections.Join([]byte(addr), name)copies the entire 20-byte address each time.
To keep index keys deterministic and length-prefixed (like old format) useaddress.MustLengthPrefix(addr)instead of the raw bytes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
x/name/keeper/query_server.go (1)
50-58: Reuse the address index instead of full scan
GetRecordsByAddressstill walks the wholenameRecordsmap and filters in-memory, making every reverse-lookup an O(N) operation.
You already builtAddrIndex; fetch the records directly viak.nameRecords.Indexes.AddrIndex.MatchExact(or expose a helper) to get O(k) behaviour.x/name/keeper/keeper.go (1)
209-221: Leverage theAddrIndexfor efficient look-ups
GetRecordsByAddressiterates every record and filters in Go.
SinceaddrIndexis defined, replace the walk with:idx := k.nameRecords.Indexes.AddrIndex err := idx.MatchExact(ctx, collections.Join(address.Bytes(), ""), func(_ collections.Pair[[]byte,string], _ string, rec types.NameRecord) (bool,error){ records = append(records, rec) return false,nil })This keeps the query O(k) and avoids large allocations.
🧹 Nitpick comments (4)
x/name/keeper/query_server.go (1)
60-79: Offset conversion still risks overflow
pageReq.Offsetis cast withsafeUint64ToInt, but only whenKeyis empty.
IfOffsetis > math.MaxInt on 32-bit builds the function silently clamps tomaxInt, skipping records and returning an empty page – hard to diagnose.Consider rejecting offsets that exceed
maxIntwithcodes.InvalidArgumentto prevent silent truncation.x/name/keeper/keeper.go (3)
148-160: Avoid double normalisation
SetNameRecordnormalisesname, then callsaddRecord, which normalises again.
Pass the already-normalised value toaddRecord(or add a variant that assumes pre-normalised) to remove redundant hashing and allocations.
165-178: Same double normalisation as above
UpdateNameRecordalso normalises twice. Refactor similarly.
240-256: Prefix comparison looks inverted
IterateRecordschecksif prefix!=nil && !bytes.Equal(prefix, types.NameKeyPrefix) { … HasPrefix(nameBytes, prefix) }
prefixshould be compared tonilonly— comparing toNameKeyPrefixis unnecessary and makes the condition harder to read. Drop the equality test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/name/keeper/keeper.go(5 hunks)x/name/keeper/query_server.go(2 hunks)
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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.
📚 Learning: 2024-10-02T18:03:10.008Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-02T16:55:10.172Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.gox/name/keeper/keeper.go
📚 Learning: 2024-10-02T02:20:49.611Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils.go:306-308
Timestamp: 2024-10-02T02:20:49.611Z
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/name/keeper/query_server.gox/name/keeper/keeper.go
📚 Learning: 2024-10-02T02:29:56.018Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.gox/name/keeper/keeper.go
📚 Learning: 2024-10-11T17:35:59.823Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2175
File: app/app.go:311-311
Timestamp: 2024-10-11T17:35:59.823Z
Learning: The variable `EnvPrefix` is defined in `app/prefix.go`, so it is available for use throughout the codebase.
Applied to files:
x/name/keeper/query_server.gox/name/keeper/keeper.go
📚 Learning: 2024-10-02T01:58:51.519Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-10T23:08:15.256Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-02T01:43:01.360Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2024-10-02T02:06:21.664Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2024-10-02T02:34:13.721Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils.go:421-427
Timestamp: 2024-10-02T02:34:13.721Z
Learning: In the metadata keeper, panics are intentionally used when `AuthzCache` is not found in the context, because many methods cannot return an error, and it's considered a programming error to call these methods without an `AuthzCache`.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2024-10-02T02:51:17.452Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2024-10-02T15:32:02.168Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/bank.go:76-87
Timestamp: 2024-10-02T15:32:02.168Z
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/name/keeper/keeper.go
📚 Learning: 2024-10-02T01:58:51.519Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/expected_keepers.go:39-49
Timestamp: 2024-10-02T01:58:51.519Z
Learning: - When defining interfaces that need to satisfy external modules (like `BankKeeper`), suggesting changes to method signatures may not be appropriate as they are outside of our control.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: 2024-10-02T17:24:43.269Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2024-10-02T03:54:36.749Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/msg_server.go:206-209
Timestamp: 2024-10-02T03:54:36.749Z
Learning: In the `DeleteScopeOwner` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/keeper.go
📚 Learning: 2025-07-29T18:06:56.313Z
Learnt from: arnabmitra
PR: provenance-io/provenance#2403
File: x/smartaccounts/keeper/keeper.go:310-348
Timestamp: 2025-07-29T18:06:56.313Z
Learning: In the smartaccounts module, duplicate credential checking is performed at the message server layer (in RegisterFido2Credential and RegisterCosmosCredential methods) before calling keeper methods like AddSmartAccountCredential, making additional duplicate checks in the keeper redundant.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: 2024-10-02T03:06:21.997Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-02T03:06:21.997Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/name/keeper/keeper.go
🧬 Code Graph Analysis (1)
x/name/keeper/query_server.go (3)
x/name/types/errors.go (2)
ErrNameNotBound(10-10)ErrInvalidAddress(22-22)x/name/types/query.pb.go (9)
QueryResolveResponse(156-161)QueryResolveResponse(165-165)QueryResolveResponse(166-168)QueryReverseLookupRequest(211-216)QueryReverseLookupRequest(220-220)QueryReverseLookupRequest(221-223)QueryReverseLookupResponse(252-257)QueryReverseLookupResponse(261-261)QueryReverseLookupResponse(262-264)x/name/keeper/keeper.go (1)
Keeper(24-42)
⏰ 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). (15)
- GitHub Check: test-race (03)
- GitHub Check: test-race (02)
- GitHub Check: test-race (04)
- GitHub Check: test-race (01)
- GitHub Check: tests (04)
- GitHub Check: tests (02)
- GitHub Check: tests (00)
- GitHub Check: tests (01)
- GitHub Check: test-race (00)
- GitHub Check: tests (03)
- GitHub Check: build-linux
- GitHub Check: Build OSX
- GitHub Check: Build Linux
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
x/name/keeper/query_server.go (1)
50-59: Avoid full in-memory scan; paginate directly from the index
GetRecordsByAddressloads all records for the address and the function then slices them for pagination.
On mainnet accounts with thousands of names this allocates large slices and increases response latency.Use the address index together with
collections.Paginateto stream only the requested page, e.g.:idx := k.nameRecords.Indexes.AddrIndex var pageNames []string err := collections.Paginate(ctx, idx, request.Pagination, func(_ collections.Pair[[]byte,string], rec types.NameRecord) (bool, error) { pageNames = append(pageNames, rec.Name) return false, nil })This keeps the operation O(page-size) instead of O(total-names).
Also applies to: 60-100
🧹 Nitpick comments (1)
x/name/keeper/query_server.go (1)
109-115: Nit: use math.MaxInt for clarity
maxInt := int(^uint(0) >> 1)works but is cryptic. Since Go 1.17 there’s a constant for this:-const maxInt = int(^uint(0) >> 1) +const maxInt = math.MaxIntRequires
import "math".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/name/keeper/query_server.go(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: nullpointer0x00
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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
PR: provenance-io/provenance#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.
📚 Learning: 2024-10-02T18:03:10.008Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-02T16:55:10.172Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-02T02:20:49.611Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2140
File: x/metadata/types/signer_utils.go:306-308
Timestamp: 2024-10-02T02:20:49.611Z
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/name/keeper/query_server.go
📚 Learning: 2024-10-02T02:29:56.018Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-11T17:35:59.823Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#2175
File: app/app.go:311-311
Timestamp: 2024-10-11T17:35:59.823Z
Learning: The variable `EnvPrefix` is defined in `app/prefix.go`, so it is available for use throughout the codebase.
Applied to files:
x/name/keeper/query_server.go
📚 Learning: 2024-10-02T01:58:51.519Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
📚 Learning: 2024-10-10T23:08:15.256Z
Learnt from: SpicyLemon
PR: provenance-io/provenance#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/name/keeper/query_server.go
⏰ 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). (14)
- GitHub Check: tests (03)
- GitHub Check: test-race (01)
- GitHub Check: test-race (04)
- GitHub Check: tests (04)
- GitHub Check: test-race (03)
- GitHub Check: test-race (02)
- GitHub Check: test-race (00)
- GitHub Check: tests (00)
- GitHub Check: tests (01)
- GitHub Check: tests (02)
- GitHub Check: build-linux
- GitHub Check: Build OSX
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
🔇 Additional comments (1)
x/name/keeper/query_server.go (1)
40-43: Return proper gRPC status for invalid addressesFor client input errors we should surface
codes.InvalidArgumentrather than a module-defined error so that gRPC callers can react correctly.-accAddr, err := sdk.AccAddressFromBech32(request.Address) -if err != nil { - return nil, types.ErrInvalidAddress -} +accAddr, err := sdk.AccAddressFromBech32(request.Address) +if err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) +}
SpicyLemon
changed the title
SpicyLemon
reviewed
Sep 8, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
SpicyLemon
reviewed
Sep 9, 2025
nagarajdivine
force-pushed
the
feature/collections-name-module
branch
from
283c3ad to
e0735d0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (17)
x/attribute/keeper/keeper_test.go (1)
854-856: Fix the backwards Contains assertion.The
Containsassertion has its arguments reversed. The function signature isContains(haystack, needle), so you're currently checking if the string"no address bound to name"contains the contents oferr.Error(), which is backwards.Based on learnings, prefer using string-based error assertions with
assertions.AssertErrorValueor simply check the full error message directly.🔎 Apply this diff to fix the assertion:
- _, err := s.app.AttributeKeeper.GetAttributes(s.ctx, s.user1, "blah") - s.Assert().Error(err, "GetAttributes") - s.Assert().Contains(err.Error(), "no address bound to name") + _, err := s.app.AttributeKeeper.GetAttributes(s.ctx, s.user1, "blah") + s.Assert().Error(err, "GetAttributes") + s.Assert().Contains("no address bound to name", err.Error(), "GetAttributes error should contain expected substring")x/name/keeper/migrations.go (3)
44-51: Log corrupt legacy records instead of silently skipping.Corrupt records are skipped without any telemetry, making it impossible to diagnose data issues during migration. Log the error with context before continuing.
🔎 Apply this diff to add logging:
for ; legacyIter.Valid(); legacyIter.Next() { var record types.NameRecord if err := m.keeper.cdc.Unmarshal(legacyIter.Value(), &record); err != nil { + ctx.Logger().Error("failed to unmarshal legacy name record; skipping", + "key", fmt.Sprintf("%X", legacyIter.Key()), "err", err) continue // skip bad records silently } recordsToMigrate = append(recordsToMigrate, record) }
60-71: Check and handle errors from Has and Get operations.The error returns from
store.Hasandstore.Getare being discarded. Handle them explicitly to avoid masking storage failures during migration.🔎 Apply this diff to handle errors properly:
// Step 3: Migrate params if they exist - if exists, _ := store.Has(LegacyNameParamStoreKey); exists { - bz, err := store.Get(LegacyNameParamStoreKey) - if err == nil { + has, err := store.Has(LegacyNameParamStoreKey) + if err != nil { + return fmt.Errorf("failed checking legacy params: %w", err) + } + if has { + bz, err := store.Get(LegacyNameParamStoreKey) + if err != nil { + return fmt.Errorf("failed reading legacy params: %w", err) + } var params types.Params if err := m.keeper.cdc.Unmarshal(bz, ¶ms); err == nil { if err := m.keeper.paramsStore.Set(ctx, params); err != nil { return fmt.Errorf("failed to migrate params: %w", err) } } - } }
73-76: Delete legacy data after successful migration.Legacy entries under
LegacyNameKeyPrefix(0x03) andLegacyAddressKeyPrefix(0x05) should be removed after migration to prevent dead state bloat and ensure a clean migration.🔎 Add cleanup logic after line 75:
ctx.Logger().Info(fmt.Sprintf("Successfully migrated %d name records to collections", len(recordsToMigrate))) ctx.Logger().Info("Name module migration to collections (v2 to v3) completed successfully.") + + // Step 4: Delete legacy name entries + legacyNameIter, err := store.Iterator(LegacyNameKeyPrefix, storetypes.PrefixEndBytes(LegacyNameKeyPrefix)) + if err != nil { + return fmt.Errorf("failed to create legacy name iterator for cleanup: %w", err) + } + defer legacyNameIter.Close() + for ; legacyNameIter.Valid(); legacyNameIter.Next() { + if err := store.Delete(legacyNameIter.Key()); err != nil { + return fmt.Errorf("failed deleting legacy name key %X: %w", legacyNameIter.Key(), err) + } + } + + // Step 5: Delete legacy address index entries + legacyAddrIter, err := store.Iterator(LegacyAddressKeyPrefix, storetypes.PrefixEndBytes(LegacyAddressKeyPrefix)) + if err != nil { + return fmt.Errorf("failed to create legacy addr index iterator for cleanup: %w", err) + } + defer legacyAddrIter.Close() + for ; legacyAddrIter.Valid(); legacyAddrIter.Next() { + if err := store.Delete(legacyAddrIter.Key()); err != nil { + return fmt.Errorf("failed deleting legacy addr index key %X: %w", legacyAddrIter.Key(), err) + } + } + return nilx/name/keeper/query_server.go (2)
32-36: Differentiate "not found" errors from storage errors.Converting all errors from
nameRecords.GettoErrNameNotBoundmasks genuine storage errors and makes debugging harder. Check specifically for "not found" conditions.🔎 Apply this diff to properly handle errors:
record, err := k.nameRecords.Get(ctx, name) - if err != nil { - return nil, err - } + switch { + case errors.Is(err, collections.ErrNotFound): + return nil, types.ErrNameNotBound + case err != nil: + return nil, status.Error(codes.Internal, err.Error()) + } return &types.QueryResolveResponse{Address: record.Address, Restricted: record.Restricted}, nilYou'll need to add the
errorsimport.
47-54: Prevent integer overflow in pagination limit conversion.Converting
uint64tointcan overflow on 32-bit systems or when the limit exceedsmath.MaxInt.🔎 Apply this diff to safely handle the conversion:
limit := request.Pagination.GetLimit() if limit == 0 { limit = query.DefaultLimit } + // Prevent overflow when converting to int + if limit > uint64(^uint(0)>>1) { + limit = query.DefaultLimit + } const maxLimit = 200 if limit > maxLimit { limit = maxLimit }x/name/keeper/migration_test.go (3)
46-47: Use legacy params key to properly test migration.The test writes params under the new collections prefix (
types.NameParamStoreKey) instead of the legacy one, so the migration won't actually migrate anything.🔎 Apply this diff to use the legacy key:
params := types.DefaultParams() - store.Set(types.NameParamStoreKey, s.cdc.MustMarshal(¶ms)) + store.Set(keeper.LegacyNameParamStoreKey, s.cdc.MustMarshal(¶ms))
49-58: Build legacy name key for proper migration testing.
types.GetNameKeyBytesuses the newNameKeyPrefix(0x07). The test must write the record under the legacy key format to properly test the migration.🔎 Apply this diff to construct the legacy key:
name := "test.provenance" - nameKey, err := types.GetNameKeyBytes(name) - s.Require().NoError(err) + hash, err := types.ComputeNameHash(name) + s.Require().NoError(err) + nameKey := append(append([]byte{}, keeper.LegacyNameKeyPrefix...), hash...) record := types.NameRecord{ Name: name, Address: s.user1, Restricted: true, } store.Set(nameKey, s.cdc.MustMarshal(&record))
70-76: Use legacy key format in TestMigration setup.Same issue: the test writes the record using the new key format, but the migration expects legacy keys.
🔎 Apply this diff to use the legacy key:
oldStore := s.ctx.KVStore(storeKey) - nameKey, err := types.GetNameKeyBytes(name) + hash, err := types.ComputeNameHash(name) s.Require().NoError(err, "failed to get name key bytes") - + nameKey := append(append([]byte{}, keeper.LegacyNameKeyPrefix...), hash...) + recordBz, err := s.cdc.Marshal(&record) s.Require().NoError(err, "failed to marshal name record") oldStore.Set(nameKey, recordBz)x/name/keeper/keeper.go (3)
220-228: Storage errors from Has are silently ignoredThe error from
k.nameRecords.Hasis discarded. While storage errors are rare, ignoring them could mask underlying issues. Consider logging the error at minimum.🔎 Apply this diff to at least log storage errors:
func (k Keeper) NameExists(ctx sdk.Context, name string) bool { normalizedName, err := k.Normalize(ctx, name) if err != nil { return false } - exists, _ := k.nameRecords.Has(ctx, normalizedName) + exists, err := k.nameRecords.Has(ctx, normalizedName) + if err != nil { + ctx.Logger().Error("NameExists storage error", "name", normalizedName, "err", err) + return false + } return exists }
279-280: Event should use normalizedName for consistencyThe event emission uses the raw
nameparameter while the storage operation usesnormalizedName. For consistency, the event should also usenormalizedName.🔎 Apply this diff:
- nameUnboundEvent := types.NewEventNameUnbound(record.Address, name, record.Restricted) + nameUnboundEvent := types.NewEventNameUnbound(record.Address, normalizedName, record.Restricted)
200-201: Event should use normalizedName for consistencySimilar to
DeleteRecord, the update event uses rawnamebut should usenormalizedNamefor consistency with the stored value.🔎 Apply this diff:
- nameUpdateEvent := types.NewEventNameUpdate(addr.String(), name, restrict) + nameUpdateEvent := types.NewEventNameUpdate(addr.String(), normalizedName, restrict)x/name/keeper/keeper_test.go (2)
23-23: Duplicate import of the same package causes compile errorThis is importing
x/name/typestwice under different aliases (nametypeson line 22 andtypeson line 23), which will cause a compilation error.🔎 Apply this diff to remove the duplicate import:
nametypes "github.com/provenance-io/provenance/x/name/types" - types "github.com/provenance-io/provenance/x/name/types"
523-534: Use consistent type alias and fix test logicTwo issues here:
- Uses
types.NameRecordbut after removing the duplicate import, this should benametypes.NameRecord.- The loop at lines 525-529 doesn't actually use the iterator's value - it just calls
GetRecordByNameagain with the same hardcodedname. This doesn't validate that the index correctly points to the record.🔎 Apply this diff to fix both issues:
- var recordByIndex *types.NameRecord + var recordByIndex *nametypes.NameRecord found := false for ; iter.Valid(); iter.Next() { - recordByIndex, err = s.app.NameKeeper.GetRecordByName(s.ctx, name) + pk, err := iter.PrimaryKey() + s.Require().NoError(err, "failed to get primary key from index") + record, err := s.app.NameKeeper.GetRecordByName(s.ctx, pk) s.Require().NoError(err, "failed to get name record from index") + recordByIndex = record found = true break }x/name/types/keys.go (3)
57-59: Decode should consume exactly sha256.Size bytesReturning
len(buffer)breaks multi-key decoding scenarios. The codec should consume exactlysha256.Sizebytes since that's whatEncodeproduces.🔎 Apply this diff:
func (c HashedStringKeyCodec) Decode(buffer []byte) (int, string, error) { - return len(buffer), base64.StdEncoding.EncodeToString(buffer), nil + if len(buffer) < sha256.Size { + return 0, "", fmt.Errorf("buffer too small for hashed key decode") + } + return sha256.Size, base64.StdEncoding.EncodeToString(buffer[:sha256.Size]), nil }
92-94: DecodeNonTerminal must consume exactly sha256.Size bytesSame issue as
Decode- usinglen(buffer)will swallow subsequent key parts in composite keys.🔎 Apply this diff:
func (c HashedStringKeyCodec) DecodeNonTerminal(buffer []byte) (int, string, error) { - return len(buffer), base64.StdEncoding.EncodeToString(buffer), nil + if len(buffer) < sha256.Size { + return 0, "", fmt.Errorf("buffer too small for hashed key decode") + } + return sha256.Size, base64.StdEncoding.EncodeToString(buffer[:sha256.Size]), nil }
131-138: Misleading comment: prefix is 0x07, not 0x03The comment states "same format as before (0x03 + sha256(name))" but
NameKeyPrefixis0x07. Either update the comment to reflect the actual prefix, or if this is meant to produce legacy keys for migration, use the legacy prefix constant.🔎 Apply this diff to fix the comment:
-// GetNameKeyBytes returns the name key in the same format as before (0x03 + sha256(name)) +// GetNameKeyBytes returns the name key bytes (0x07 prefix + sha256(name)) func GetNameKeyBytes(name string) ([]byte, error) {
🧹 Nitpick comments (2)
x/name/keeper/keeper_test.go (1)
327-327: Remove commented-out codeThis commented-out line should be removed rather than left as dead code.
🔎 Apply this diff:
- // expUser2Recs := nametypes.NameRecords{}x/name/types/keys.go (1)
100-108: ComputeHash silently accepts empty segments unlike ComputeNameHash
ComputeHash(used by the codec) doesn't validate empty segments, whileComputeNameHashreturns an error for them. This inconsistency could allow invalid names to be encoded via the codec that would fail viaComputeNameHash.Consider either:
- Having
ComputeHashcallComputeNameHashand handle the error, or- Adding the same empty-segment validation to
ComputeHash.🔎 Option 1 - Reuse ComputeNameHash:
func (c HashedStringKeyCodec) ComputeHash(name string) []byte { - comps := strings.Split(name, ".") - hsh := sha256.New() - for i := len(comps) - 1; i >= 0; i-- { - comp := strings.TrimSpace(comps[i]) - hsh.Write([]byte(comp)) - } - return hsh.Sum(nil) + hash, err := ComputeNameHash(name) + if err != nil { + // Return empty hash for invalid names - validation should happen elsewhere + return make([]byte, sha256.Size) + } + return hash }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.changelog/unreleased/features/2411-collections-name-module.md(1 hunks)app/app.go(1 hunks)internal/provwasm/simulation.go(1 hunks)x/attribute/keeper/keeper_test.go(1 hunks)x/attribute/keeper/mocks_test.go(1 hunks)x/attribute/keeper/query_server_test.go(2 hunks)x/attribute/simulation/operations.go(1 hunks)x/attribute/types/expected_keepers.go(1 hunks)x/name/client/cli/cli_test.go(1 hunks)x/name/keeper/export_test.go(1 hunks)x/name/keeper/genesis.go(1 hunks)x/name/keeper/keeper.go(5 hunks)x/name/keeper/keeper_test.go(7 hunks)x/name/keeper/migration_test.go(1 hunks)x/name/keeper/migrations.go(1 hunks)x/name/keeper/params.go(1 hunks)x/name/keeper/query_server.go(2 hunks)x/name/module.go(2 hunks)x/name/simulation/operations.go(1 hunks)x/name/types/keys.go(2 hunks)x/name/types/keys_test.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changelog/unreleased/features/2411-collections-name-module.md
🚧 Files skipped from review as they are similar to previous changes (9)
- x/attribute/keeper/query_server_test.go
- x/name/keeper/genesis.go
- x/name/keeper/export_test.go
- internal/provwasm/simulation.go
- x/name/keeper/params.go
- app/app.go
- x/name/client/cli/cli_test.go
- x/attribute/keeper/mocks_test.go
- x/name/module.go
🧰 Additional context used
🧠 Learnings (50)
📓 Common learnings
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/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.
📚 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/name/keeper/keeper_test.gox/name/types/keys_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/name/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/name/keeper/keeper_test.gox/name/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/name/keeper/keeper_test.gox/name/keeper/keeper.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/name/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/name/keeper/keeper_test.gox/name/types/keys_test.gox/attribute/keeper/keeper_test.go
📚 Learning: 2024-10-02T03:06:21.997Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4_test.go:619-630
Timestamp: 2024-10-02T03:06:21.997Z
Learning: When unit tests exist for a key generation function, it's preferable to reuse that function in tests instead of manually constructing byte slices.
Applied to files:
x/name/keeper/keeper_test.gox/name/types/keys_test.gox/name/keeper/migration_test.gox/name/types/keys.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/name/keeper/keeper_test.gox/name/types/keys_test.go
📚 Learning: 2025-08-15T20:59:09.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/expected_keeper.go:11-19
Timestamp: 2025-08-15T20:59:09.909Z
Learning: The BankKeeper interface methods in Cosmos SDK v0.50.x and Provenance use `context.Context` as the context parameter type, not `sdk.Context`. This applies to methods like SendCoins, HasBalance, GetBalance, SpendableCoin, HasSupply, etc. The expected keeper interfaces should consistently use `context.Context` to match the actual Cosmos SDK bank keeper signatures.
Applied to files:
x/name/keeper/keeper_test.gox/attribute/types/expected_keepers.gox/name/keeper/migrations.gox/name/keeper/keeper.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/name/keeper/keeper_test.gox/name/types/keys_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/name/keeper/keeper_test.gox/name/types/keys_test.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: The `CreateAutoResponseToAddrPrefix` function in the quarantine module should have unit tests that assert specific panic messages to ensure the tests fail for the correct reasons, specifically when the `MustLengthPrefix` function triggers a panic due to address length issues.
Applied to files:
x/name/keeper/keeper_test.gox/name/types/keys_test.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/address_test.go:69-80
Timestamp: 2024-10-08T18:12:51.935Z
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/name/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/name/keeper/keeper_test.gox/name/types/keys_test.go
📚 Learning: 2024-10-23T18:46:02.430Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2197
File: x/metadata/client/cli/cli_test.go:1474-1480
Timestamp: 2024-10-23T18:46:02.430Z
Learning: In `cli_test.go` test cases, the `expOut` field may include indentation levels and field names with extra spaces to match the YAML output format. Including partial outputs in `expOut` is acceptable as long as they verify the necessary parts.
Applied to files:
x/name/types/keys_test.go
📚 Learning: 2025-08-15T20:57:04.455Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/ledger/keeper/entries_test.go:26-31
Timestamp: 2025-08-15T20:57:04.455Z
Learning: SpicyLemon prefers string-based error assertions in tests using empty string to indicate "no error expected" and non-empty string for expected error messages. Use assertions.AssertErrorValue instead of errors.Is() comparisons because errors.Is() with s.Require().True() provides poor failure messages that only say "Should be true" without showing expected vs actual error content.
Applied to files:
x/name/types/keys_test.gox/attribute/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/name/types/keys_test.go
📚 Learning: 2024-10-02T03:16:46.324Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/exchange/keeper/mocks_test.go:427-431
Timestamp: 2024-10-02T03:16:46.324Z
Learning: In Go unit tests, repetitive code is acceptable; refactoring for DRY is not necessary.
Applied to files:
x/name/types/keys_test.go
📚 Learning: 2025-08-15T20:01:55.078Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: app/upgrades_test.go:0-0
Timestamp: 2025-08-15T20:01:55.078Z
Learning: SpicyLemon prefers upgrade tests to fail when expected data is missing rather than skip, using s.Require().NoError() and s.Require().NotEmpty() to enforce hard requirements for upgrade data files in test scenarios.
Applied to files:
x/name/types/keys_test.go
📚 Learning: 2024-10-10T23:19:54.565Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades_test.go:610-610
Timestamp: 2024-10-10T23:19:54.565Z
Learning: In `app/upgrades_test.go`, we prefer to keep TODO comments (like `TODO[viridian]`) to handle code deletions manually, and do not want to implement automated processes for code removal, especially for unit tests.
Applied to files:
x/name/types/keys_test.gox/name/keeper/migrations.go
📚 Learning: 2024-10-02T03:39:04.537Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server_test.go:1248-1251
Timestamp: 2024-10-02T03:39:04.537Z
Learning: In subtests, it's acceptable to use `require` and `assert` functions directly instead of `s.Require()` and `s.Assert()` methods.
Applied to files:
x/name/types/keys_test.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/name/types/keys_test.gox/name/keeper/query_server.gox/name/keeper/keeper.gox/name/types/keys.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/attribute/simulation/operations.gox/name/keeper/query_server.gox/name/simulation/operations.gox/name/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/types/signer_utils_test.go:478-493
Timestamp: 2024-10-08T18:12:51.935Z
Learning: In Go tests, modifying byte slices directly using arithmetic operations like addition and subtraction is acceptable, even if it causes overflow, as overflow on `uint8` just wraps around. Avoid flagging such usage, and avoid suggesting bitwise operations which can be harder to read and understand.
Applied to files:
x/name/keeper/query_server.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/name/keeper/query_server.gox/name/keeper/keeper.gox/name/types/keys.go
📚 Learning: 2025-08-15T20:39:34.095Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/types/util.go:68-102
Timestamp: 2025-08-15T20:39:34.095Z
Learning: In the Provenance codebase, functions with "Must" prefix (like MustGetMarkerAddress) can panic and should only be used when you know they won't panic. Instead, use the twin function that returns an error (like GetMarkerAddress) and handle the error properly.
Applied to files:
x/name/keeper/query_server.gox/name/keeper/migrations.gox/name/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/name/keeper/query_server.gox/attribute/keeper/keeper_test.gox/name/keeper/migrations.go
📚 Learning: 2025-09-24T22:38:42.707Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: proto/provenance/ledger/v1/query.proto:208-211
Timestamp: 2025-09-24T22:38:42.707Z
Learning: SpicyLemon prefers to defer adding pagination to query endpoints until actual usage patterns demonstrate the need, rather than adding it preemptively for theoretical large datasets that are unlikely to occur in practice based on domain knowledge.
Applied to files:
x/name/keeper/query_server.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/name/keeper/query_server.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/name/keeper/query_server.go
📚 Learning: 2024-10-08T18:12:51.935Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/migrations_v4.go:57-80
Timestamp: 2024-10-08T18:12:51.935Z
Learning: The `V3WriteNewScope` method in `x/metadata/keeper/migrations_v4.go` must exist until after the migration has been executed and will be deleted as part of the `viridian` cleanup.
Applied to files:
x/name/keeper/migrations.gox/name/keeper/migration_test.go
📚 Learning: 2024-10-10T23:13:15.277Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: x/metadata/types/oslocatorparams.go:7-9
Timestamp: 2024-10-10T23:13:15.277Z
Learning: In the codebase, the other `NewParams` functions are intended to remain as they are and do not need to be renamed.
Applied to files:
x/name/keeper/migrations.gox/name/keeper/migration_test.go
📚 Learning: 2024-10-10T23:16:32.311Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades.go:55-55
Timestamp: 2024-10-10T23:16:32.311Z
Learning: In the `app` package, the constant `paramsName` is defined in `app/app.go` and can be used in `app/upgrades.go`.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-10T23:14:29.308Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2176
File: app/upgrades.go:41-41
Timestamp: 2024-10-10T23:14:29.308Z
Learning: In the `provenance` codebase, the constant `paramsName` is defined in `app/app.go` and is accessible in `app/upgrades.go` since they are in the same package.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-02T23:22:26.909Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:262-263
Timestamp: 2024-10-02T23:22:26.909Z
Learning: In the `MigrateValueOwner` method, use `sdkerrors.ErrLogic` when representing unexpected backend errors, as the request itself is valid.
Applied to files:
x/name/keeper/migrations.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/name/keeper/migrations.gox/name/keeper/migration_test.gox/name/types/keys.go
📚 Learning: 2025-10-08T21:44:13.140Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2474
File: x/asset/client/cli/tx.go:302-308
Timestamp: 2025-10-08T21:44:13.140Z
Learning: When a variable is declared inside a Go loop body using `:=` (e.g., `coin, err := Parse(...)`), and its address is taken and stored beyond the iteration, Go allocates a fresh instance for each iteration. This is different from taking the address of the loop variable itself (declared in the `for` statement), which was problematic prior to Go 1.22. Code like `for _, x := range items { v := f(x); append(&v) }` is safe.
Applied to files:
x/name/keeper/migrations.gox/name/keeper/keeper.go
📚 Learning: 2025-09-12T20:27:37.057Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/client/cli/tx.go:264-270
Timestamp: 2025-09-12T20:27:37.057Z
Learning: When analyzing Go code for the "address of loop variable" issue, distinguish between loop variables (declared in the for statement) and variables declared inside the loop body with :=. Variables declared inside the loop body are fresh in each iteration and taking their address is safe.
Applied to files:
x/name/keeper/migrations.go
📚 Learning: 2024-10-08T18:12:57.974Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 1874
File: cmd/dbmigrate/utils/migrator_test.go:10-10
Timestamp: 2024-10-08T18:12:57.974Z
Learning: The `cmd/dbmigrate` directory and its contents, including the `migrator_test.go` file, are planned to be removed from the project soon, as indicated by the user. Therefore, suggestions for changes or tests related to this directory are unnecessary.
Applied to files:
x/name/keeper/migrations.gox/name/keeper/migration_test.gox/name/types/keys.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/name/keeper/migrations.go
📚 Learning: 2024-06-10T19:25:28.209Z
Learnt from: nullpointer0x00
Repo: provenance-io/provenance PR: 0
File: :0-0
Timestamp: 2024-06-10T19:25:28.209Z
Learning: A GitHub issue was created to track the migration of parameters for the metadata, marker, name, and msgFees modules as indicated by the TODO comments in the codebase. The issue is linked to PR #1932 and can be found at https://github.com/provenance-io/provenance/issues/1935.
Applied to files:
x/name/keeper/migrations.gox/name/types/keys.go
📚 Learning: 2025-08-15T20:19:31.726Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2422
File: x/asset/simulation/decoder.go:8-14
Timestamp: 2025-08-15T20:19:31.726Z
Learning: The asset module in Provenance doesn't use collections-based storage like ledger/registry modules. Instead, it delegates storage to the NFT module, so a collections-based store decoder isn't applicable here. The module should use its own types.StoreKey rather than exchange.StoreKey in RegisterStoreDecoder.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: 2025-09-05T19:28:28.271Z
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.
Applied to files:
x/name/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/name/keeper/keeper.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/name/keeper/keeper.go
📚 Learning: 2024-10-02T03:54:36.749Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:206-209
Timestamp: 2024-10-02T03:54:36.749Z
Learning: In the `DeleteScopeOwner` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/name/keeper/keeper.go
📚 Learning: 2024-10-08T18:12:51.936Z
Learnt from: SpicyLemon
Repo: provenance-io/provenance PR: 2140
File: x/metadata/keeper/msg_server.go:106-109
Timestamp: 2024-10-08T18:12:51.936Z
Learning: In the `AddScopeDataAccess` method (and similar cases), redeclaring `err` with `:=` is appropriate when previous `err` variables are scoped within `if` statements, as using `=` would result in a compilation error.
Applied to files:
x/name/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/name/keeper/keeper.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/name/keeper/keeper.go
🧬 Code graph analysis (6)
x/name/types/keys_test.go (3)
x/name/types/errors.go (1)
ErrNameInvalid(14-14)x/name/types/keys.go (2)
GetNameKeyBytes(132-138)HashedStringKeyCodec(46-46)x/attribute/types/keys.go (1)
GetNameKeyBytes(107-115)
x/name/keeper/query_server.go (4)
x/name/types/query.pb.go (6)
QueryReverseLookupRequest(211-216)QueryReverseLookupRequest(220-220)QueryReverseLookupRequest(221-223)QueryReverseLookupResponse(252-257)QueryReverseLookupResponse(261-261)QueryReverseLookupResponse(262-264)x/name/keeper/keeper.go (1)
Keeper(22-40)x/name/types/errors.go (1)
ErrInvalidAddress(22-22)internal/provutils/pair.go (1)
Pair(6-9)
x/attribute/keeper/keeper_test.go (1)
x/name/types/expected_keepers.go (1)
AttributeKeeper(14-17)
x/name/keeper/migrations.go (1)
x/name/keeper/keeper.go (1)
Keeper(22-40)
x/name/keeper/keeper.go (5)
x/name/types/name.pb.go (3)
NameRecord(102-109)NameRecord(112-112)NameRecord(113-115)x/name/types/keys.go (2)
NameRecordIndexes(37-39)ModuleName(18-18)x/name/types/errors.go (2)
ErrNameAlreadyBound(12-12)ErrNameNotBound(10-10)x/name/types/name.go (1)
NewNameRecord(14-20)x/name/types/events.go (3)
NewEventNameBound(17-23)NewEventNameUpdate(33-39)NewEventNameUnbound(25-31)
x/name/types/keys.go (4)
internal/provutils/pair.go (1)
Pair(6-9)x/name/types/name.pb.go (3)
NameRecord(102-109)NameRecord(112-112)NameRecord(113-115)x/name/types/errors.go (1)
ErrNameInvalid(14-14)x/attribute/types/keys.go (1)
GetNameKeyBytes(107-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: tests (00)
- GitHub Check: tests (01)
- GitHub Check: test-race (01)
- GitHub Check: test-race (03)
- GitHub Check: test-race (02)
- GitHub Check: tests (02)
- GitHub Check: tests (04)
- GitHub Check: test-race (00)
- GitHub Check: tests (03)
- GitHub Check: test-race (04)
- GitHub Check: build-linux
- GitHub Check: Build Linux
- GitHub Check: Build OSX
- GitHub Check: heighliner-docker
- GitHub Check: docker
- GitHub Check: ghcr-publish
- GitHub Check: Analyze (go)
- GitHub Check: golangci-lint
🔇 Additional comments (14)
x/attribute/keeper/keeper_test.go (1)
857-863: LGTM! Good addition of descriptive assertion messages.The descriptive message on the
NoErrorassertion improves test clarity and makes failures easier to diagnose.x/attribute/types/expected_keepers.go (1)
26-26: LGTM! Signature update aligns with collections migration.The removal of the
prefixparameter fromIterateRecordsis consistent with the migration to collections-based storage where iteration no longer requires explicit prefix filtering.x/name/simulation/operations.go (1)
180-186: LGTM! Iterator usage updated correctly.The removal of the prefix parameter from
IterateRecordsaligns with the new collections-based API. The callback logic remains unchanged and correct.x/attribute/simulation/operations.go (1)
266-269: LGTM! Iterator usage updated correctly.The removal of the prefix parameter from
IterateRecordsis consistent with the signature change in the NameKeeper interface.x/name/types/keys_test.go (1)
108-202: Excellent comprehensive test coverage for HashedStringKeyCodec.The new test thoroughly validates:
- Hashing determinism and encoding/decoding
- Terminal and non-terminal encoding variants
- JSON round-trip serialization
- Hash uniqueness across inputs
- Edge cases (empty strings, whitespace, special characters, long strings)
The use of nested
s.Runblocks for test organization is a good practice.x/name/keeper/keeper_test.go (3)
562-566: LGTM!The
sortNameRecordsByNamehelper correctly ensures deterministic ordering for test comparisons.
536-561: LGTM!This test properly validates the name record storage and address index retrieval flow.
379-381: LGTM!The
IterateRecordscall correctly uses the updated signature without the prefix argument.x/name/types/keys.go (1)
36-44: LGTM!The
NameRecordIndexesstruct andIndexesListmethod correctly implement thecollections.Indexesinterface for the address-based multi-index.x/name/keeper/keeper.go (5)
52-65: Verify index key codec matches primary key codecThe
addrIndexusescollections.StringKeyfor the second part of the pair (line 56-57), but the primarynameRecordscollection usesHashedStringKeyCodec{}(line 76). The index stores the raw name string while the primary collection stores the hashed version.This works because the index stores the primary key value (the raw name string), not the encoded key bytes. Just verify this is the intended behavior.
230-261: LGTM with minor note on error handlingThe implementation correctly uses
PrefixedPairRangewith theAddrIndexfor efficient address-based lookups, addressing previous concerns about iterating over all records.The silent
continueon errors (lines 252, 256) is acceptable for iteration robustness, but consider logging at debug level for troubleshooting.
204-218: Consider whether normalization is needed herePrevious review feedback suggested not normalizing in lookup methods since stored names are already normalized and callers should know the exact format. However, the current approach is more forgiving for API consumers.
If performance is a concern (since
Normalizerequires a state lookup), consider documenting the expectation that callers provide normalized names, or provide both normalized and exact-match lookup methods.
45-103: LGTM!The
NewKeeperfunction correctly:
- Uses
storeServiceinstead ofstoreKey(addressing prior feedback)- Creates the
IndexedMapwith proper key codecs- Sets up the address multi-index for efficient lookups
- Panics appropriately on schema build failure during initialization
318-320: LGTM!The
GetAddrIndexmethod correctly exposes the address index for use in tests and query operations.
nagarajdivine
force-pushed
the
feature/collections-name-module
branch
from
e0735d0 to
949df43
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@x/name/keeper/keeper_test.go`:
- Around line 523-533: The loop is re-fetching the record by the original name
instead of using the index primary key and the test also uses the wrong import
alias; change the loop to obtain pk := iter.PrimaryKey() and call
s.app.NameKeeper.GetRecordByName(s.ctx, pk) (or the correct lookup function that
accepts a primary key) so you verify index→record mapping, set recordByIndex to
that result, and assert equality; also replace the duplicate types alias by
using *nametypes.NameRecord for recordByIndex (and update imports) to avoid the
duplicate import alias.
In `@x/name/keeper/keeper.go`:
- Around line 249-258: The loop currently swallows errors from iter.PrimaryKey()
and k.nameRecords.Get which can hide corruption; update the loop in the function
using iter to surface failures instead of silently continuing: when
iter.PrimaryKey() returns an error return that error (or wrap it with context
mentioning the iterator) rather than continue, and likewise when
k.nameRecords.Get(ctx, primaryKey) returns an error return/wrap that error (or
log it and return) so the caller is informed; ensure you update the function
signature to propagate the error if necessary and adjust callers to handle the
new error return.
In `@x/name/keeper/migration_test.go`:
- Around line 1-16: The test file is missing the suite runner and the testing
import, so add the testing package to the imports and implement a top-level Test
function that calls suite.Run(t, new(MigrationTestSuite)); specifically, import
"testing" and add a TestMigrationTestSuite(t *testing.T) function which invokes
suite.Run with a new instance of MigrationTestSuite to ensure the suite
(MigrationTestSuite) executes under go test.
In `@x/name/keeper/query_server.go`:
- Around line 81-105: The loop currently calls iter.Next() in the for-footer and
ignores any error it returns; capture and handle errors from iter.Next() so
storage/iteration failures are propagated instead of silently swallowed. Replace
the for ; iter.Valid(); iter.Next() pattern with an explicit loop that checks
iter.Valid() each iteration and calls err := iter.Next() after processing the
current item (or capture the error result if your iterator API returns (bool,
error)); if err != nil return status.Error(codes.Internal, err.Error()). Ensure
you still call iter.PrimaryKey(), k.nameRecords.Get(...) and maintain the
existing skipMode/limit logic while propagating iter.Next() errors.
In `@x/name/types/keys.go`:
- Around line 100-108: The HashedStringKeyCodec.ComputeHash implementation
accepts empty/whitespace segments and writes zero-length bytes, causing
inconsistency with ComputeNameHash which rejects empty segments; fix by adding
the same empty-segment validation used in ComputeNameHash to ComputeHash (trim
each segment and return an error on empty segments) or refactor ComputeHash to
call ComputeNameHash and make the codec's Encode method propagate and return
that error instead of silently continuing—update references to
HashedStringKeyCodec.ComputeHash and the Encode method to handle/return errors
accordingly.
🧹 Nitpick comments (4)
x/name/types/keys_test.go (1)
139-161: Tests don't coverDecode/DecodeNonTerminalwith a buffer larger thansha256.Size.The current tests always pass a buffer of exactly
sha256.Size, so they won't catch the bug whereDecode/DecodeNonTerminalreturnlen(buffer)instead ofsha256.Size. Consider adding a test case where extra bytes are appended to the buffer and asserting that onlysha256.Sizebytes are consumed.s.Run("Decode with extra bytes", func() { extra := append(buffer[:n], []byte("extra data")...) read, _, err := codec.Decode(extra) s.Require().NoError(err) s.Equal(sha256.Size, read, "Decode should consume exactly sha256.Size bytes") })x/name/keeper/query_server.go (1)
107-110:NextKeyshould point to the next unpaginated entry, not the last returned entrySetting
nextKey = []byte(lastKey)means the next page request will re-encounterlastKey, skip past it via theskipModelogic, and waste a comparison. More importantly, iflastKeyis deleted between requests, the skip logic falls through and the first entry of the next page may be an item that was already beforelastKeylexicographically (sincepk < continueAfterNamewould skip it, butpk > continueAfterNamewould not—actually this is fine for the deleted case since skipMode exits).While functionally correct, the standard pattern is to set
NextKeyto the key of the next entry (the one that causedlimitto be exceeded). Consider peeking at the next primary key after the limit is reached:if uint64(len(names)) >= limit { + // Peek at next entry for NextKey + nextPK, pkErr := iter.PrimaryKey() + if pkErr == nil { + nextKey = []byte(nextPK) + } break }Then adjust the skip logic to start at (inclusive)
continueAfterNameinstead of after it. This avoids the skip scan entirely on subsequent pages.x/name/keeper/migration_test.go (1)
62-76: Redundant setup duplicates whatSetupTestalready wroteLines 65-66 re-derive
user1Addr/user1(already set inSetupTest), and lines 70-76 re-write the same record to the same key thatSetupTestwrote at line 58. This entire block can be removed sinceSetupTestalready populates the store.If the intent is to test with a different record or verify overwrite behavior, make it explicit with a distinct name/address.
x/name/keeper/keeper_test.go (1)
327-327: Remove commented-out code
// expUser2Recs := nametypes.NameRecords{}is dead code. Either remove it or, if the intent was to assert emptiness, use thes.Require().Empty(...)assertion that follows on line 330 (which already covers this).
nagarajdivine
force-pushed
the
feature/collections-name-module
branch
from
949df43 to
3a89185
Compare
… keeper with all test cases
…ed app.go for migration
…ade with test cases and fixed migration test
… fix migrations and test cases
…s map to index map with lint-fix
nagarajdivine
force-pushed
the
feature/collections-name-module
branch
from
781f3e1 to
ce83a36
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
closes: Switch to Collections in Name module. #2018
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
New Features
Bug Fixes
Tests
Documentation
Refactor