switch to collections in the attribute module.#2677
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe attribute module is being refactored to use Cosmos SDK collections for state storage instead of direct KVStore access. A no-op migration (v2→v3) is introduced to track the schema change. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/attribute/keeper/keeper.go (1)
265-286:⚠️ Potential issue | 🟠 MajorBuild
origKeyfrom the normalized original name.
normalizedOrigNameis computed, butoriginalAttribute.Nameis never updated beforeBuildAttrTriple(originalAttribute). If the caller passes a valid non-normalized original name, this lookup misses the stored row even though the normalized names matched.Suggested fix
if normalizedName != normalizedOrigName { return fmt.Errorf("update and original names must match %s : %s", normalizedName, normalizedOrigName) } + originalAttribute.Name = normalizedOrigName updateAttribute.Name = normalizedName if ownerAcc := k.authKeeper.GetAccount(ctx, owner); ownerAcc == nil { return fmt.Errorf("no account found for owner address %q", owner.String()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/attribute/keeper/keeper.go` around lines 265 - 286, The code computes normalizedOrigName via k.nameKeeper.Normalize but still calls types.BuildAttrTriple(originalAttribute) using the un-normalized originalAttribute.Name, causing a lookup miss; update originalAttribute.Name = normalizedOrigName (or otherwise construct the triple using normalizedOrigName) before calling types.BuildAttrTriple so origKey is built from the normalized original name and subsequent lookups succeed; ensure any later uses of originalAttribute reflect this change and keep the call sites: normalizedOrigName, originalAttribute, types.BuildAttrTriple, and origKey in mind while editing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/attribute/keeper/keeper.go`:
- Around line 609-625: The expiration index entry is being removed
unconditionally even when k.attributes.Remove fails, which loses the retry
handle; change the logic so k.expirationIndex.Remove(ctx, entry.expKey) is only
invoked after a successful attribute deletion (i.e., when attr was fetched
(k.attributes.Get returned nil) and k.attributes.Remove returned nil). Keep the
existing log paths for Get/Remove failures and, on a failed k.attributes.Remove,
do not call k.expirationIndex.Remove so the expiration sentinel remains for
retry; retain the EmitTypedEvent and DecAttrNameAddressLookup calls in the
successful-removal branch.
- Around line 143-150: IterateRecords currently ignores the caller-supplied
prefix by calling k.attributes.Walk(ctx, nil, ...); update it to derive the walk
range from the provided prefix the same way prefixScan does and pass that range
instead of nil. Locate the IterateRecords method and replace the nil range
argument to k.attributes.Walk with the start/end (or range) values produced by
the prefix->range helper used by prefixScan so Walk only iterates keys under the
supplied prefix; keep the handler invocation and error handling the same.
- Around line 486-500: The purge loop removes the primary attribute rows and
decrements name-address counts but never clears the corresponding entries in
expirationIndex, leaving orphaned expiry sentinels; after successfully removing
the attribute with k.attributes.Remove(ctx, key) and updating counts via
k.DecAttrNameAddressLookup(ctx, name, acct), also remove the matching expiration
index entry from k.expirationIndex (e.g. call k.expirationIndex.Remove(ctx, key)
or the equivalent method your implementation exposes) and handle/return any
error from that call so expired-index sentinels are not left behind.
- Around line 216-238: The current incNameAddrCount and decNameAddrCount swallow
all errors from nameAddrCounts.Get and treat them as "not found"; change them to
only treat collections.ErrNotFound as missing and propagate any other errors.
Specifically, in incNameAddrCount, call k.nameAddrCounts.Get and if err != nil
then if errors.Is(err, collections.ErrNotFound) set current = 0 else return err;
then Set the incremented value. In decNameAddrCount, call Get and if err != nil
then if errors.Is(err, collections.ErrNotFound) return nil (nothing to
decrement) else return err; otherwise apply the existing current<=1 removal or
Set(current-1). Use errors.Is(err, collections.ErrNotFound) exactly and keep the
existing calls to k.nameAddrCounts.Get, Set, and Remove and the
DecAttrNameAddressLookup logging behavior.
In `@x/attribute/keeper/params.go`:
- Around line 11-16: The current handler treats any error from k.params.Get(ctx)
as "params missing" and returns types.Params{MaxValueLength:
types.DefaultMaxValueLength}; change this to only use the default when the Get
error is the known "missing entry"/not-found condition (e.g., check for
sdkerrors.ErrNotFound or store's IsNotFound error depending on your params store
implementation) and otherwise surface the unexpected error (propagate/return it
or wrap and fail loudly) instead of silently returning defaults; update the
function signature to return an error if needed and keep references to
k.params.Get, types.Params, and types.DefaultMaxValueLength to locate and patch
the code.
In `@x/attribute/keeper/query_server.go`:
- Around line 150-156: The current nil-page handling sets pageReq to a non-nil
PageRequest, which together with the earlier local limit := query.DefaultLimit
causes unpaginated requests to be truncated to the default page size; instead,
detect pageReq == nil and only set CountTotal = true (or leave pageReq nil) so
you do not override the intended unlimited behavior — update the nil check
around pageReq in the query handlers (symbols: limit, offset, countTotal,
pageReq, and the request handlers Attribute, Attributes, Scan,
AttributeAccounts) to avoid creating a PageRequest that forces the default limit
(e.g., if pageReq == nil set countTotal=true or set limit to math.MaxUint64
rather than assigning pageReq = &query.PageRequest{CountTotal:true}).
- Around line 110-116: AttributeAccounts currently dereferences req
unconditionally (req.AttributeName) which will panic on a nil gRPC request; add
an initial nil check at the top of Keeper.AttributeAccounts (for
types.QueryAttributeAccountsRequest) and return a proper InvalidArgument gRPC
error (e.g., using status.Error(codes.InvalidArgument, "invalid request") or
sdkerrors.Wrap) when req == nil, then proceed to use req.AttributeName, keeping
the rest of the nameHashRange/nameAddrPageWalk logic unchanged.
---
Outside diff comments:
In `@x/attribute/keeper/keeper.go`:
- Around line 265-286: The code computes normalizedOrigName via
k.nameKeeper.Normalize but still calls types.BuildAttrTriple(originalAttribute)
using the un-normalized originalAttribute.Name, causing a lookup miss; update
originalAttribute.Name = normalizedOrigName (or otherwise construct the triple
using normalizedOrigName) before calling types.BuildAttrTriple so origKey is
built from the normalized original name and subsequent lookups succeed; ensure
any later uses of originalAttribute reflect this change and keep the call sites:
normalizedOrigName, originalAttribute, types.BuildAttrTriple, and origKey in
mind while editing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9415147e-f803-49f8-b58d-fac361a94099
📒 Files selected for processing (9)
app/app.gox/attribute/keeper/keeper.gox/attribute/keeper/migration_test.gox/attribute/keeper/migrations.gox/attribute/keeper/params.gox/attribute/keeper/query_server.gox/attribute/module.gox/attribute/types/codec.gox/attribute/types/keys_test.go
| func (k Keeper) IterateRecords(ctx sdk.Context, prefix []byte, handle Handler) error { | ||
| // Init an attribute record iterator | ||
| store := ctx.KVStore(k.storeKey) | ||
| iterator := storetypes.KVStorePrefixIterator(store, prefix) | ||
| defer iterator.Close() //nolint:errcheck // close error safe to ignore in this context. | ||
|
|
||
| // Iterate over records, processing callbacks. | ||
| for ; iterator.Valid(); iterator.Next() { | ||
| record := types.Attribute{} | ||
| // get proto objects for legacy prefix with legacy amino codec. | ||
| if err := k.cdc.Unmarshal(iterator.Value(), &record); err != nil { | ||
| return err | ||
| } | ||
| if err := handle(record); err != nil { | ||
| return err | ||
| return k.attributes.Walk(ctx, nil, func(_ types.AttrTriple, record types.Attribute) (stop bool, err error) { | ||
| if err = handle(record); err != nil { | ||
| return true, err | ||
| } | ||
| } | ||
| return nil | ||
| return false, nil | ||
| }) |
There was a problem hiding this comment.
Preserve the prefix filter in IterateRecords.
Walk(ctx, nil, ...) now ignores the caller-supplied prefix, so any caller asking for a subset will receive every attribute instead. Please derive the walk range from prefix here, the same way prefixScan does.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/attribute/keeper/keeper.go` around lines 143 - 150, IterateRecords
currently ignores the caller-supplied prefix by calling k.attributes.Walk(ctx,
nil, ...); update it to derive the walk range from the provided prefix the same
way prefixScan does and pass that range instead of nil. Locate the
IterateRecords method and replace the nil range argument to k.attributes.Walk
with the start/end (or range) values produced by the prefix->range helper used
by prefixScan so Walk only iterates keys under the supplied prefix; keep the
handler invocation and error handling the same.
| func (k Keeper) incNameAddrCount(ctx sdk.Context, name string, addrBytes []byte) error { | ||
| key := types.BuildNameAddrPair(name, addrBytes) | ||
| current, err := k.nameAddrCounts.Get(ctx, key) | ||
| if err != nil { | ||
| current = 0 | ||
| } | ||
| return k.nameAddrCounts.Set(ctx, key, current+1) | ||
| } | ||
|
|
||
| // DecAttrNameAddressLookup decrements the name to account lookups and removes value if decremented to 0 | ||
| func (k Keeper) DecAttrNameAddressLookup(ctx sdk.Context, name string, addrBytes []byte) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.AttributeNameAddrKeyPrefix(name, addrBytes) | ||
| bz := store.Get(key) | ||
| if bz != nil { | ||
| value := binary.BigEndian.Uint64(bz) | ||
| if value <= uint64(1) { | ||
| store.Delete(key) | ||
| } else { | ||
| store.Set(key, sdk.Uint64ToBigEndian(value-1)) | ||
| } | ||
| if err := k.decNameAddrCount(ctx, name, addrBytes); err != nil { | ||
| k.Logger(ctx).Error("DecAttrNameAddressLookup failed", "error", err) | ||
| } | ||
| } | ||
|
|
||
| func (k Keeper) decNameAddrCount(ctx sdk.Context, name string, addrBytes []byte) error { | ||
| key := types.BuildNameAddrPair(name, addrBytes) | ||
| current, err := k.nameAddrCounts.Get(ctx, key) | ||
| if err != nil || current <= 1 { | ||
| return k.nameAddrCounts.Remove(ctx, key) | ||
| } | ||
| return k.nameAddrCounts.Set(ctx, key, current-1) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "keeper.go" | grep attribute | head -5Repository: provenance-io/provenance
Length of output: 50
🏁 Script executed:
# First, let's find the file and examine the context
find . -type f -path "*/attribute/keeper/*.go" | head -10Repository: provenance-io/provenance
Length of output: 423
🏁 Script executed:
# Search for nameAddrCounts definition
rg "nameAddrCounts" --type go -B 2 -A 2 | head -50Repository: provenance-io/provenance
Length of output: 3541
🏁 Script executed:
# Look for collections package and Get() usage patterns
rg "collections\." --type go -A 3 | grep -E "(\.Get|\.Remove|\.Set)" | head -30Repository: provenance-io/provenance
Length of output: 1783
🏁 Script executed:
# Check what error types the collections Get() can return
find . -type f -name "*.go" -path "*/collections/*" | head -5Repository: provenance-io/provenance
Length of output: 50
🏁 Script executed:
# Look for collections package imports and find error handling patterns
rg "collections\." --type go -B 1 -A 1 | grep -E "(not.*found|NotFound|ErrNotFound|collections\.ErrNotFound)" | head -20Repository: provenance-io/provenance
Length of output: 695
🏁 Script executed:
# Search for AccountsByAttribute to understand the risk of drift
rg "AccountsByAttribute" --type go -B 3 -A 10 | head -60Repository: provenance-io/provenance
Length of output: 3974
🏁 Script executed:
# Look for how other Get() calls in the codebase handle errors from collections
rg "\.Get\(ctx," --type go -B 1 -A 3 | grep -E "(if err.*nil|ErrNotFound|collections)" | head -40Repository: provenance-io/provenance
Length of output: 1071
🏁 Script executed:
# Find the collections package to understand Get() error semantics
find . -type d -name collections | head -5Repository: provenance-io/provenance
Length of output: 50
🏁 Script executed:
# Look for error handling patterns with collections.Get() - checking if they distinguish not-found
rg "\.Get\(.*\)" --type go -A 2 | grep -B 1 "ErrNotFound\|collections.ErrNotFound" | head -30Repository: provenance-io/provenance
Length of output: 304
🏁 Script executed:
# Find AccountsByAttribute implementation
rg "func.*AccountsByAttribute" --type go -A 20Repository: provenance-io/provenance
Length of output: 1533
🏁 Script executed:
# Also check how nameAddrCounts is used throughout the keeper
rg "nameAddrCounts\." --type go -B 2 -A 2Repository: provenance-io/provenance
Length of output: 1601
Distinguish not-found errors from other failures.
Both incNameAddrCount and decNameAddrCount should only treat collections.ErrNotFound as "missing"—other errors (decode failures, store corruption) are distinct and should not be silently treated as zero or trigger removal. Collapsing all errors creates a mismatch: the nameAddrCounts map can lose entries or counts while the attributes map remains intact, causing AccountsByAttribute to drift from the actual stored attributes. Use errors.Is(err, collections.ErrNotFound) to distinguish, matching the pattern used elsewhere in the codebase (e.g., ledger and registry keepers).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/attribute/keeper/keeper.go` around lines 216 - 238, The current
incNameAddrCount and decNameAddrCount swallow all errors from nameAddrCounts.Get
and treat them as "not found"; change them to only treat collections.ErrNotFound
as missing and propagate any other errors. Specifically, in incNameAddrCount,
call k.nameAddrCounts.Get and if err != nil then if errors.Is(err,
collections.ErrNotFound) set current = 0 else return err; then Set the
incremented value. In decNameAddrCount, call Get and if err != nil then if
errors.Is(err, collections.ErrNotFound) return nil (nothing to decrement) else
return err; otherwise apply the existing current<=1 removal or Set(current-1).
Use errors.Is(err, collections.ErrNotFound) exactly and keep the existing calls
to k.nameAddrCounts.Get, Set, and Remove and the DecAttrNameAddressLookup
logging behavior.
| var keysToRemove []types.AttrTriple | ||
| if walkErr := k.attributes.Walk(ctx, rng, func(key types.AttrTriple, attr types.Attribute) (stop bool, err error) { | ||
| if bytes.Equal(key.AddrBytes, []byte(acct)) && attr.Name == name { | ||
| keysToRemove = append(keysToRemove, key) | ||
| } | ||
| return false, nil | ||
| }); walkErr != nil { | ||
| return walkErr | ||
| } | ||
| for _, key := range keysToRemove { | ||
| if err = k.attributes.Remove(ctx, key); err != nil { | ||
| return err | ||
| } | ||
| k.DecAttrNameAddressLookup(ctx, name, acct) | ||
| } |
There was a problem hiding this comment.
Purge needs to remove expirationIndex entries too.
This path removes the primary attribute row and decrements nameAddrCounts, but it never clears expirationIndex. That leaves orphaned expiry sentinels behind, so later expiry sweeps waste limit budget on already-purged records and log false missing-attribute errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/attribute/keeper/keeper.go` around lines 486 - 500, The purge loop removes
the primary attribute rows and decrements name-address counts but never clears
the corresponding entries in expirationIndex, leaving orphaned expiry sentinels;
after successfully removing the attribute with k.attributes.Remove(ctx, key) and
updating counts via k.DecAttrNameAddressLookup(ctx, name, acct), also remove the
matching expiration index entry from k.expirationIndex (e.g. call
k.expirationIndex.Remove(ctx, key) or the equivalent method your implementation
exposes) and handle/return any error from that call so expired-index sentinels
are not left behind.
| attr, getErr := k.attributes.Get(ctx, entry.attrKey) | ||
| if getErr == nil { | ||
| if removeErr := k.attributes.Remove(ctx, entry.attrKey); removeErr == nil { | ||
| k.DecAttrNameAddressLookup(ctx, attr.Name, attr.GetAddressBytes()) | ||
| if emitErr := ctx.EventManager().EmitTypedEvent(types.NewEventAttributeExpired(attr)); emitErr != nil { | ||
| ctx.Logger().Error(fmt.Sprintf("failed to emit typed event %v", emitErr)) | ||
| } | ||
| count++ | ||
| } else { | ||
| ctx.Logger().Error(fmt.Sprintf("unable to unmarshal attribute to delete key: %v error: %v", attrKey, err)) | ||
| ctx.Logger().Error(fmt.Sprintf("unable to remove attribute: %v error: %v", entry.attrKey, removeErr)) | ||
| } | ||
| } else { | ||
| ctx.Logger().Error(fmt.Sprintf("unable to get attribute for expiry key: %v error: %v", entry.expKey, getErr)) | ||
| } | ||
|
|
||
| // delete the expiration lookup key | ||
| store.Delete(expirationKey) | ||
| if limit != 0 && count >= limit { | ||
| break | ||
| if removeExpErr := k.expirationIndex.Remove(ctx, entry.expKey); removeExpErr != nil { | ||
| ctx.Logger().Error(fmt.Sprintf("unable to remove expiration entry: %v error: %v", entry.expKey, removeExpErr)) | ||
| } |
There was a problem hiding this comment.
Don't drop the expiry sentinel when primary deletion failed.
expirationIndex.Remove runs unconditionally, even when k.attributes.Remove failed. That loses the only retry handle for this expired record, so a transient remove error can leave an expired attribute in state permanently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/attribute/keeper/keeper.go` around lines 609 - 625, The expiration index
entry is being removed unconditionally even when k.attributes.Remove fails,
which loses the retry handle; change the logic so k.expirationIndex.Remove(ctx,
entry.expKey) is only invoked after a successful attribute deletion (i.e., when
attr was fetched (k.attributes.Get returned nil) and k.attributes.Remove
returned nil). Keep the existing log paths for Get/Remove failures and, on a
failed k.attributes.Remove, do not call k.expirationIndex.Remove so the
expiration sentinel remains for retry; retain the EmitTypedEvent and
DecAttrNameAddressLookup calls in the successful-removal branch.
| params, err := k.params.Get(ctx) | ||
| if err != nil { | ||
| return types.Params{ | ||
| MaxValueLength: types.DefaultMaxValueLength, | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't treat every params read failure as "use defaults".
This now falls back to DefaultMaxValueLength for any k.params.Get failure, not just the "params not set yet" case. A bad on-disk params encoding or wrong collection key would be silently masked, which is especially dangerous for a no-migration storage rewrite. Please only default the missing-entry case and fail loudly on unexpected read errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/attribute/keeper/params.go` around lines 11 - 16, The current handler
treats any error from k.params.Get(ctx) as "params missing" and returns
types.Params{MaxValueLength: types.DefaultMaxValueLength}; change this to only
use the default when the Get error is the known "missing entry"/not-found
condition (e.g., check for sdkerrors.ErrNotFound or store's IsNotFound error
depending on your params store implementation) and otherwise surface the
unexpected error (propagate/return it or wrap and fail loudly) instead of
silently returning defaults; update the function signature to return an error if
needed and keep references to k.params.Get, types.Params, and
types.DefaultMaxValueLength to locate and patch the code.
| func (k Keeper) AttributeAccounts(c context.Context, req *types.QueryAttributeAccountsRequest) (*types.QueryAttributeAccountsResponse, error) { | ||
| if req == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "invalid request") | ||
| } | ||
| ctx := sdk.UnwrapSDKContext(c) | ||
| accounts := make([]string, 0) | ||
| store := ctx.KVStore(k.storeKey) | ||
| keyPrefix := types.AttributeNameKeyPrefix(req.AttributeName) | ||
| attributeStore := prefix.NewStore(store, keyPrefix) | ||
|
|
||
| pageRes, err := query.FilteredPaginate(attributeStore, req.Pagination, func(key []byte, _ []byte, accumulate bool) (bool, error) { | ||
| addressLength := int32(key[0]) | ||
| address := sdk.AccAddress(key[1 : addressLength+1]) | ||
| if slices.Contains(accounts, address.String()) { | ||
| return false, nil | ||
| } | ||
| if accumulate { | ||
| accounts = append(accounts, address.String()) | ||
| } | ||
| return true, nil | ||
| }) | ||
| var nameHash [32]byte | ||
| copy(nameHash[:], types.GetNameKeyBytes(req.AttributeName)) | ||
|
|
||
| rng, endBound := nameHashRange(nameHash) | ||
| accounts, pageRes, err := nameAddrPageWalk(ctx, k.nameAddrCounts, rng, endBound, req.Pagination) |
There was a problem hiding this comment.
Guard AttributeAccounts before dereferencing req.
Line 113 reads req.AttributeName unconditionally, so a nil gRPC request now panics instead of returning InvalidArgument like the other query handlers.
Suggested fix
func (k Keeper) AttributeAccounts(c context.Context, req *types.QueryAttributeAccountsRequest) (*types.QueryAttributeAccountsResponse, error) {
+ if req == nil {
+ return nil, status.Error(codes.InvalidArgument, "invalid request")
+ }
+ if req.AttributeName == "" {
+ return nil, status.Error(codes.InvalidArgument, "empty attribute name")
+ }
ctx := sdk.UnwrapSDKContext(c)
var nameHash [32]byte
copy(nameHash[:], types.GetNameKeyBytes(req.AttributeName))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (k Keeper) AttributeAccounts(c context.Context, req *types.QueryAttributeAccountsRequest) (*types.QueryAttributeAccountsResponse, error) { | |
| if req == nil { | |
| return nil, status.Error(codes.InvalidArgument, "invalid request") | |
| } | |
| ctx := sdk.UnwrapSDKContext(c) | |
| accounts := make([]string, 0) | |
| store := ctx.KVStore(k.storeKey) | |
| keyPrefix := types.AttributeNameKeyPrefix(req.AttributeName) | |
| attributeStore := prefix.NewStore(store, keyPrefix) | |
| pageRes, err := query.FilteredPaginate(attributeStore, req.Pagination, func(key []byte, _ []byte, accumulate bool) (bool, error) { | |
| addressLength := int32(key[0]) | |
| address := sdk.AccAddress(key[1 : addressLength+1]) | |
| if slices.Contains(accounts, address.String()) { | |
| return false, nil | |
| } | |
| if accumulate { | |
| accounts = append(accounts, address.String()) | |
| } | |
| return true, nil | |
| }) | |
| var nameHash [32]byte | |
| copy(nameHash[:], types.GetNameKeyBytes(req.AttributeName)) | |
| rng, endBound := nameHashRange(nameHash) | |
| accounts, pageRes, err := nameAddrPageWalk(ctx, k.nameAddrCounts, rng, endBound, req.Pagination) | |
| func (k Keeper) AttributeAccounts(c context.Context, req *types.QueryAttributeAccountsRequest) (*types.QueryAttributeAccountsResponse, error) { | |
| if req == nil { | |
| return nil, status.Error(codes.InvalidArgument, "invalid request") | |
| } | |
| if req.AttributeName == "" { | |
| return nil, status.Error(codes.InvalidArgument, "empty attribute name") | |
| } | |
| ctx := sdk.UnwrapSDKContext(c) | |
| var nameHash [32]byte | |
| copy(nameHash[:], types.GetNameKeyBytes(req.AttributeName)) | |
| rng, endBound := nameHashRange(nameHash) | |
| accounts, pageRes, err := nameAddrPageWalk(ctx, k.nameAddrCounts, rng, endBound, req.Pagination) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/attribute/keeper/query_server.go` around lines 110 - 116, AttributeAccounts
currently dereferences req unconditionally (req.AttributeName) which will panic
on a nil gRPC request; add an initial nil check at the top of
Keeper.AttributeAccounts (for types.QueryAttributeAccountsRequest) and return a
proper InvalidArgument gRPC error (e.g., using
status.Error(codes.InvalidArgument, "invalid request") or sdkerrors.Wrap) when
req == nil, then proceed to use req.AttributeName, keeping the rest of the
nameHashRange/nameAddrPageWalk logic unchanged.
| limit := uint64(query.DefaultLimit) | ||
| offset := uint64(0) | ||
| countTotal := false | ||
|
|
||
| if pageReq == nil { | ||
| pageReq = &query.PageRequest{CountTotal: true} | ||
| } |
There was a problem hiding this comment.
Nil pagination now truncates these queries to the default page size.
Both helpers start with limit := query.DefaultLimit and then manufacture a non-nil PageRequest when the caller omitted pagination. That means unpaginated Attribute, Attributes, Scan, and AttributeAccounts requests only return the first page once the result set grows past the default limit.
Also applies to: 231-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/attribute/keeper/query_server.go` around lines 150 - 156, The current
nil-page handling sets pageReq to a non-nil PageRequest, which together with the
earlier local limit := query.DefaultLimit causes unpaginated requests to be
truncated to the default page size; instead, detect pageReq == nil and only set
CountTotal = true (or leave pageReq nil) so you do not override the intended
unlimited behavior — update the nil check around pageReq in the query handlers
(symbols: limit, offset, countTotal, pageReq, and the request handlers
Attribute, Attributes, Scan, AttributeAccounts) to avoid creating a PageRequest
that forces the default limit (e.g., if pageReq == nil set countTotal=true or
set limit to math.MaxUint64 rather than assigning pageReq =
&query.PageRequest{CountTotal:true}).
1 participant
Description
closes: Switch to collections in the attribute module. #2489
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
Refactor
Chores
Tests