feat(storage): add AllFilesByOwner query endpoint - #552
feat(storage): add AllFilesByOwner query endpoint#552TheMarstonConnell wants to merge 5 commits into
Conversation
Add a new query endpoint to retrieve all files owned by a specific address. This is useful for clients that need to list their own files without scanning the entire file store. The query uses pagination and filters files by the owner field. Changes: - Add AllFilesByOwner RPC to Query service in proto - Implement AllFilesByOwner keeper method with owner filtering Note: Proto regeneration is required after merging. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new gRPC/HTTP query AllFilesByOwner to proto/canine_chain/storage/query.proto with request QueryAllFilesByOwner and response QueryAllFilesByOwnerResponse, exposed via GET /jackal/canine-chain/storage/files/owner/{owner}. In the keeper, implements AllFilesByOwner(c context.Context, req *types.QueryAllFilesByOwner) which validates Bech32 owner addresses, applies pagination via new internal pagination helpers, and filters stored UnifiedFile entries by owner. A unit test TestAllFilesByOwner was added to validate multi-owner results, pagination, invalid/empty owner errors, and non-existent owner behavior. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@x/storage/keeper/grpc_query_active_deals.go`:
- Around line 119-121: The handler currently only checks for empty req.Owner;
add a bech32 address format validation (e.g., call sdk.AccAddressFromBech32 or
equivalent) in the same function in grpc_query_active_deals.go (the routine
handling the ActiveDeals query where req.Owner is checked) and return
status.Error(codes.InvalidArgument, "invalid owner address") when parsing fails;
keep the existing empty check, validate req.Owner after that, and do not proceed
if parsing returns an error so malformed addresses produce a clear error
message.
- Around line 126-144: Pagination here is incorrect because query.Paginate
counts scanned items, not filtered matches; update the handler (where you use
query.Paginate on prefix.NewStore with
types.KeyPrefix(types.FilePrimaryKeyPrefix) and filter by req.Owner after
k.cdc.Unmarshal) to either (preferred) add and use an owner-indexed prefix
(e.g., a FilesByOwner prefix/index keyed by owner) and paginate that store
directly so pagination and pageRes.Total reflect matching files, or
(alternative) replace query.Paginate with manual iteration over the primary
store: seek to the prefix, iterate with the iterator, unmarshal via
k.cdc.Unmarshal, collect only items where file.Owner == req.Owner until you hit
the requested limit/offset, and build a PaginationResult whose Total equals the
matching count before returning QueryAllFilesByOwnerResponse; apply the same
change to FilesFromNote which currently filters inside the paginate callback.
Replace buf-generated code with manual additions to avoid gogoproto import compatibility issues. The buf tool generates code using github.com/cosmos/gogoproto which conflicts with the existing codebase using github.com/gogo/protobuf. Changes: - Add AllFilesByOwner to QueryServer and QueryClient interfaces - Add gRPC handler and service registration - Add HTTP gateway handlers for REST API endpoint Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
d4562ef to
1c69d9c
Compare
- Add bech32 address format validation for owner parameter - Fix pagination by using manual iteration instead of query.Paginate with filtering (query.Paginate counts all items, not filtered matches) The new implementation correctly handles pagination offset and limit while filtering, and returns accurate Total count of matching files. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Addressed both review comments in commit 8eda86d:
|
Comprehensive test coverage including: - Basic query returning files for specified owner only - Multiple owners with correct filtering - Pagination with offset and limit - Empty owner returns error - Invalid owner address format returns error - Non-existent owner returns empty list - Verify Total count is accurate for filtered results Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@x/storage/keeper/grpc_query_active_deals_test.go`:
- Around line 147-298: Add a cursor-based pagination subtest inside
TestAllFilesByOwner that mirrors the existing limit/offset checks but uses
query.PageRequest.NextKey to paginate: call suite.queryClient.AllFilesByOwner
with a first request that sets Limit (e.g., 2) and nil Offset, capture
res.Pagination.NextKey, then issue a second request with Pagination.NextKey set
to that value and verify combined results equal the full set and
res.Pagination.Total remains the same; reference the TestAllFilesByOwner test,
the suite.queryClient.AllFilesByOwner call, types.QueryAllFilesByOwner, and
query.PageRequest.NextKey/Limit to locate where to add this check.
| func (suite *KeeperTestSuite) TestAllFilesByOwner() { | ||
| suite.SetupSuite() | ||
|
|
||
| testAddresses, err := testutil.CreateTestAddresses("cosmos", 3) | ||
| suite.Require().NoError(err) | ||
|
|
||
| owner1 := testAddresses[0] | ||
| owner2 := testAddresses[1] | ||
| depoAccount := testAddresses[2] | ||
|
|
||
| coins := sdk.NewCoins(sdk.NewCoin("ujkl", sdk.NewInt(100000000000))) | ||
| testAcc, _ := sdk.AccAddressFromBech32(owner1) | ||
| err = suite.bankKeeper.SendCoinsFromModuleToAccount(suite.ctx, types.ModuleName, testAcc, coins) | ||
| suite.Require().NoError(err) | ||
|
|
||
| suite.storageKeeper.SetParams(suite.ctx, types.Params{ | ||
| DepositAccount: depoAccount, | ||
| ProofWindow: 50, | ||
| ChunkSize: 1024, | ||
| PriceFeed: "jklprice", | ||
| MissesToBurn: 3, | ||
| MaxContractAgeInBlocks: 100, | ||
| PricePerTbPerMonth: 8, | ||
| CollateralPrice: 2, | ||
| CheckWindow: 11, | ||
| ReferralCommission: 25, | ||
| PolRatio: 40, | ||
| }) | ||
|
|
||
| // Create 5 files for owner1 | ||
| for i := 0; i < 5; i++ { | ||
| merkle := []byte(fmt.Sprintf("merkle_owner1_%d", i)) | ||
| suite.storageKeeper.SetFile(suite.ctx, types.UnifiedFile{ | ||
| Merkle: merkle, | ||
| Owner: owner1, | ||
| Start: int64(i), | ||
| Expires: 0, | ||
| FileSize: 1024, | ||
| ProofInterval: 400, | ||
| ProofType: 0, | ||
| Proofs: make([]string, 0), | ||
| MaxProofs: 3, | ||
| Note: "{}", | ||
| }) | ||
| } | ||
|
|
||
| // Create 3 files for owner2 | ||
| for i := 0; i < 3; i++ { | ||
| merkle := []byte(fmt.Sprintf("merkle_owner2_%d", i)) | ||
| suite.storageKeeper.SetFile(suite.ctx, types.UnifiedFile{ | ||
| Merkle: merkle, | ||
| Owner: owner2, | ||
| Start: int64(i), | ||
| Expires: 0, | ||
| FileSize: 2048, | ||
| ProofInterval: 400, | ||
| ProofType: 0, | ||
| Proofs: make([]string, 0), | ||
| MaxProofs: 3, | ||
| Note: "{}", | ||
| }) | ||
| } | ||
|
|
||
| // Test: Query files for owner1 - should return 5 files | ||
| pg := query.PageRequest{ | ||
| Offset: 0, | ||
| Reverse: false, | ||
| Limit: 100, | ||
| } | ||
|
|
||
| res, err := suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{ | ||
| Pagination: &pg, | ||
| Owner: owner1, | ||
| }) | ||
| suite.Require().NoError(err) | ||
| suite.Require().Equal(5, len(res.Files)) | ||
| suite.Require().Equal(uint64(5), res.Pagination.Total) | ||
|
|
||
| // Verify all returned files belong to owner1 | ||
| for _, file := range res.Files { | ||
| suite.Require().Equal(owner1, file.Owner) | ||
| } | ||
|
|
||
| // Test: Query files for owner2 - should return 3 files | ||
| res, err = suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{ | ||
| Pagination: &pg, | ||
| Owner: owner2, | ||
| }) | ||
| suite.Require().NoError(err) | ||
| suite.Require().Equal(3, len(res.Files)) | ||
| suite.Require().Equal(uint64(3), res.Pagination.Total) | ||
|
|
||
| // Verify all returned files belong to owner2 | ||
| for _, file := range res.Files { | ||
| suite.Require().Equal(owner2, file.Owner) | ||
| } | ||
|
|
||
| // Test: Pagination with limit | ||
| pgLimit := query.PageRequest{ | ||
| Offset: 0, | ||
| Limit: 2, | ||
| } | ||
| res, err = suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{ | ||
| Pagination: &pgLimit, | ||
| Owner: owner1, | ||
| }) | ||
| suite.Require().NoError(err) | ||
| suite.Require().Equal(2, len(res.Files)) | ||
| suite.Require().Equal(uint64(5), res.Pagination.Total) // Total should still be 5 | ||
|
|
||
| // Test: Pagination with offset | ||
| pgOffset := query.PageRequest{ | ||
| Offset: 3, | ||
| Limit: 100, | ||
| } | ||
| res, err = suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{ | ||
| Pagination: &pgOffset, | ||
| Owner: owner1, | ||
| }) | ||
| suite.Require().NoError(err) | ||
| suite.Require().Equal(2, len(res.Files)) // 5 total - 3 offset = 2 remaining | ||
| suite.Require().Equal(uint64(5), res.Pagination.Total) | ||
|
|
||
| // Test: Empty owner should return error | ||
| _, err = suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{ | ||
| Pagination: &pg, | ||
| Owner: "", | ||
| }) | ||
| suite.Require().Error(err) | ||
| suite.Require().Contains(err.Error(), "owner address is required") | ||
|
|
||
| // Test: Invalid owner address format should return error | ||
| _, err = suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{ | ||
| Pagination: &pg, | ||
| Owner: "invalid-address", | ||
| }) | ||
| suite.Require().Error(err) | ||
| suite.Require().Contains(err.Error(), "invalid owner address format") | ||
|
|
||
| // Test: Non-existent owner should return empty list | ||
| // Use a known valid bech32 address that won't match any files we created | ||
| nonExistentOwner := "cosmos1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqnrql8a" | ||
| res, err = suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{ | ||
| Pagination: &pg, | ||
| Owner: nonExistentOwner, | ||
| }) | ||
| suite.Require().NoError(err) | ||
| suite.Require().Equal(0, len(res.Files)) | ||
| suite.Require().Equal(uint64(0), res.Pagination.Total) | ||
|
|
||
| suite.reset() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-structured test with comprehensive coverage.
The test thoroughly covers the AllFilesByOwner query endpoint including:
- Basic owner filtering with multiple owners
- Pagination behavior (limit and offset)
- Error cases (empty owner, invalid format)
- Non-existent owner edge case
The test follows existing patterns in the file and properly verifies that Pagination.Total reflects the filtered count rather than page size.
Minor suggestion: Consider adding a test case for cursor-based pagination using NextKey to ensure the query handler supports both pagination styles consistently with other endpoints like OpenFiles.
📝 Optional: Add cursor-based pagination test
suite.Require().Equal(2, len(res.Files)) // 5 total - 3 offset = 2 remaining
suite.Require().Equal(uint64(5), res.Pagination.Total)
+ // Test: Cursor-based pagination using NextKey
+ pgCursor := query.PageRequest{
+ Limit: 2,
+ CountTotal: true,
+ }
+ res, err = suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{
+ Pagination: &pgCursor,
+ Owner: owner1,
+ })
+ suite.Require().NoError(err)
+ suite.Require().Equal(2, len(res.Files))
+ suite.Require().NotNil(res.Pagination.NextKey) // Should have more pages
+
+ // Fetch next page using NextKey
+ pgCursor.Key = res.Pagination.NextKey
+ res, err = suite.queryClient.AllFilesByOwner(context.Background(), &types.QueryAllFilesByOwner{
+ Pagination: &pgCursor,
+ Owner: owner1,
+ })
+ suite.Require().NoError(err)
+ suite.Require().Equal(2, len(res.Files))
+
// Test: Empty owner should return error🤖 Prompt for AI Agents
In `@x/storage/keeper/grpc_query_active_deals_test.go` around lines 147 - 298, Add
a cursor-based pagination subtest inside TestAllFilesByOwner that mirrors the
existing limit/offset checks but uses query.PageRequest.NextKey to paginate:
call suite.queryClient.AllFilesByOwner with a first request that sets Limit
(e.g., 2) and nil Offset, capture res.Pagination.NextKey, then issue a second
request with Pagination.NextKey set to that value and verify combined results
equal the full set and res.Pagination.Total remains the same; reference the
TestAllFilesByOwner test, the suite.queryClient.AllFilesByOwner call,
types.QueryAllFilesByOwner, and query.PageRequest.NextKey/Limit to locate where
to add this check.
|
Regarding the cursor-based pagination suggestion: The Cursor-based pagination with filtering is complex to implement correctly because you'd need to track position across filtered results. The current offset-based approach is consistent with the existing filtered query patterns in this module. If cursor-based pagination becomes a requirement for this endpoint, it would warrant a separate PR to implement properly (potentially with a secondary index for efficient cursor tracking). |
Extract common patterns into reusable helpers: - extractPaginationParams: extracts reverse/limit/offset with defaults - filterFilesWithPagination: generic filtered iteration with pagination Simplifies AllFilesByOwner, OpenFiles, and EndangeredFiles from ~50 lines each to ~10 lines each by reusing the common filter pattern. Removes 61 lines of duplicated code. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/storage/keeper/grpc_query_active_deals.go (1)
93-137: Consider refactoringFilesFromNoteto use the new pagination helper.This function still uses
query.Paginatewith in-callback filtering, which has the same pagination issues that were fixed inAllFilesByOwner,OpenFiles, andEndangeredFiles. TheTotalin the response reflects all files scanned, not just those matching the note filter, and page sizes may be inconsistent.♻️ Proposed refactor to use filterFilesWithPagination
func (k Keeper) FilesFromNote(c context.Context, req *types.QueryFilesFromNote) (*types.QueryFilesFromNoteResponse, error) { if req == nil { return nil, status.Error(codes.InvalidArgument, "invalid request") } - var files []types.UnifiedFile ctx := sdk.UnwrapSDKContext(c) + params := extractPaginationParams(req.Pagination) - store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefix(types.FilePrimaryKeyPrefix)) - - pageRes, err := query.Paginate(store, req.Pagination, func(_ []byte, value []byte) error { - var file types.UnifiedFile - if err := k.cdc.Unmarshal(value, &file); err != nil { - return err - } - + files, pageRes := k.filterFilesWithPagination(ctx, params, func(file *types.UnifiedFile) bool { var kv map[string]any - err := json.Unmarshal([]byte(file.Note), &kv) + err := json.Unmarshal([]byte(file.Note), &kv) if err != nil { - return nil + return false } r, exists := kv[req.Key] if !exists { - return nil + return false } s, ok := r.(string) if !ok { - return nil + return false } - if s != req.Value { - return nil - } - - files = append(files, file) - return nil + return s == req.Value }) - if err != nil { - return nil, status.Error(codes.Internal, err.Error()) - } return &types.QueryFilesFromNoteResponse{Files: files, Pagination: pageRes}, nil }
🤖 Fix all issues with AI agents
In `@x/storage/keeper/grpc_query_active_deals.go`:
- Around line 44-48: The iteration in k.IterateFilesByMerkle silently skips
entries when k.cdc.Unmarshal(val, &file) fails; change this to log the unmarshal
error before returning false so data issues are visible—use the keeper's logger
(or context logger) to emit a descriptive message including the error and any
identifying bytes (e.g., the val or merkle key) in the k.IterateFilesByMerkle
callback when k.cdc.Unmarshal returns an error for types.UnifiedFile.
| k.IterateFilesByMerkle(ctx, params.reverse, func(_ []byte, val []byte) bool { | ||
| var file types.UnifiedFile | ||
| if err := k.cdc.Unmarshal(val, &file); err != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging unmarshal errors instead of silently skipping.
When Unmarshal fails, the iteration silently continues. This provides resilience but could mask data corruption issues. Consider adding a log statement to aid debugging.
📝 Optional: Add error logging
k.IterateFilesByMerkle(ctx, params.reverse, func(_ []byte, val []byte) bool {
var file types.UnifiedFile
if err := k.cdc.Unmarshal(val, &file); err != nil {
+ k.Logger(ctx).Error("failed to unmarshal file during pagination", "error", err)
return false
}📝 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.
| k.IterateFilesByMerkle(ctx, params.reverse, func(_ []byte, val []byte) bool { | |
| var file types.UnifiedFile | |
| if err := k.cdc.Unmarshal(val, &file); err != nil { | |
| return false | |
| } | |
| k.IterateFilesByMerkle(ctx, params.reverse, func(_ []byte, val []byte) bool { | |
| var file types.UnifiedFile | |
| if err := k.cdc.Unmarshal(val, &file); err != nil { | |
| k.Logger(ctx).Error("failed to unmarshal file during pagination", "error", err) | |
| return false | |
| } |
🤖 Prompt for AI Agents
In `@x/storage/keeper/grpc_query_active_deals.go` around lines 44 - 48, The
iteration in k.IterateFilesByMerkle silently skips entries when
k.cdc.Unmarshal(val, &file) fails; change this to log the unmarshal error before
returning false so data issues are visible—use the keeper's logger (or context
logger) to emit a descriptive message including the error and any identifying
bytes (e.g., the val or merkle key) in the k.IterateFilesByMerkle callback when
k.cdc.Unmarshal returns an error for types.UnifiedFile.
Summary
Add a new query endpoint
AllFilesByOwnerto retrieve all files owned by a specific wallet address. This enables efficient file listing for clients without scanning the entire storage module.Changes
AllFilesByOwnerRPC endpoint to the Query service inquery.protoAllFilesByOwnerkeeper method with owner address filtering and pagination supportAPI Endpoint
Request
Response
Test plan
make proto-genequivalent)go build ./...)canined query storage all-files-by-owner <address>🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.