add topN endpoint to supply module (disabled by default) #1800
Conversation
joe-bowman
requested review from
faddat,
ajansari95 and
arhamchordia
as code owners
WalkthroughThe pull request introduces a new feature to the Quicksilver supply module that allows querying the top N accounts by balance. This involves extending the proto definitions, adding a new gRPC query method, and implementing the corresponding keeper logic. The changes include creating new message types for the top N accounts request and response, adding a new RPC method in the Query service, and implementing the backend logic to retrieve and sort accounts based on their balances. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
x/supply/keeper/keeper.go (1)
85-122: Consider performance implications and memory usage.The current implementation loads all accounts into memory before sorting. For chains with a large number of accounts, this could lead to high memory usage and potential performance issues.
Consider these alternatives:
- Implement pagination to process accounts in batches
- Use a min-heap to maintain top N accounts while iterating
- Add caching with a configurable TTL to prevent frequent recalculations
Would you like me to provide an implementation for any of these approaches?
proto/quicksilver/supply/v1/query.proto (2)
18-20: Add documentation for the TopN endpoint.The RPC endpoint lacks documentation describing its purpose, parameters, and response format.
Add documentation comments:
+ // TopN returns the top N accounts sorted by total balance (liquid + staked). + // This endpoint is intended for use by CoinmarketCap. + // {n} parameter specifies the number of accounts to return (max: 100). rpc TopN(QueryTopNRequest) returns (QueryTopNResponse) { option (google.api.http).get = "/quicksilver/supply/v1/topn/{n}"; }
38-40: Add field constraints and documentation.The
nfield inQueryTopNRequestshould have constraints and documentation.Add field constraints:
message QueryTopNRequest { + // n specifies the number of top accounts to return (max: 100) + uint64 n = 1 [(gogoproto.moretags) = "validate:\"required,min=1,max=100\""]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
x/supply/types/query.pb.gois excluded by!**/*.pb.gox/supply/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (4)
proto/quicksilver/supply/v1/query.proto(2 hunks)x/supply/keeper/grpc_query.go(1 hunks)x/supply/keeper/keeper.go(2 hunks)x/supply/types/expected_keepers.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
x/supply/keeper/grpc_query.go
[high] 53-53: G115: integer overflow conversion uint64 -> int
(gosec)
🪛 GitHub Check: lint
x/supply/keeper/grpc_query.go
[failure] 53-53:
G115: integer overflow conversion uint64 -> int (gosec)
🪛 GitHub Actions: golangci-lint
x/supply/keeper/grpc_query.go
[error] 53-53: G115: Potential integer overflow in conversion from uint64 to int
🪛 Buf (1.47.2)
proto/quicksilver/supply/v1/query.proto
4-4: import "cosmos/base/v1beta1/coin.proto": file does not exist
(COMPILE)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: Analyze
🔇 Additional comments (1)
x/supply/types/expected_keepers.go (1)
25-25: LGTM! Clean interface extension.The addition of
IterateAllDelegationsto theStakingKeeperinterface is well-defined and follows existing patterns.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1800 +/- ##
==========================================
+ Coverage 63.02% 63.37% +0.34%
==========================================
Files 172 172
Lines 15377 15422 +45
==========================================
+ Hits 9691 9773 +82
+ Misses 4876 4838 -38
- Partials 810 811 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Can be merged once CI passes. |
…age type to x/supply
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Well spotted :)
…On Thu, 30 Jan 2025 at 07:54, Arham Chordia ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In x/supply/keeper/grpc_query.go
<#1800 (comment)>
:
> @@ -40,7 +40,21 @@ func (q Querier) Supply(c context.Context, _ *types.QuerySupplyRequest) (*types.
"quick1e22za5qrqqp488h5p7vw2pfx8v0y4u444ufeuw", // ingenuity
})
- return &types.QuerySupplyResponse{Supply: supply.Amount.Uint64(), CirculatingSupply: circulatingSupply.Uint64()}, nil
+ fmt.Println("supply", supply.Amount)
+ fmt.Println("circulatingSupply", circulatingSupply)
Can get rid of println
—
Reply to this email directly, view it on GitHub
<#1800 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABA7QYH52UASY3OWIL553TL2NHLCZAVCNFSM6AAAAABV6SHUQSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOBTGE3DCMZWHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
x/supply/keeper/grpc_query.go (1)
48-56:⚠️ Potential issueAdd input validation and fix potential integer overflow.
The TopN implementation needs input validation and proper type handling:
- Missing input validation for
req.N- Potential integer overflow when using large values of N
Apply this diff to fix both issues:
func (q Querier) TopN(c context.Context, req *types.QueryTopNRequest) (*types.QueryTopNResponse, error) { ctx := sdk.UnwrapSDKContext(c) if q.endpointEnabled { + // Validate input + if req.N == 0 { + return nil, fmt.Errorf("n must be greater than 0") + } + if req.N > 100 { + return nil, fmt.Errorf("n must not exceed 100") + } accounts := q.Keeper.TopN(ctx, q.stakingKeeper.BondDenom(ctx), req.N) return &types.QueryTopNResponse{Accounts: accounts}, nil } return nil, fmt.Errorf("endpoint disabled") }
🧹 Nitpick comments (2)
x/supply/keeper/grpc_query.go (1)
Line range hint
31-41: Consider moving excluded addresses to configuration.The list of excluded addresses is hard-coded. Consider moving this to a configuration file or chain parameters for better maintainability.
x/supply/keeper/keeper.go (1)
99-99: Add documentation for the TopN method.Consider adding documentation to describe:
- The purpose of the method
- The meaning of the parameters
- The structure of the returned accounts
- Any performance implications for large values of n
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumx/supply/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
docs/swagger.yml(1 hunks)proto/quicksilver/supply/v1/query.proto(2 hunks)x/supply/keeper/grpc_query.go(1 hunks)x/supply/keeper/grpc_query_test.go(1 hunks)x/supply/keeper/keeper.go(4 hunks)x/supply/keeper/msg_server_test.go(1 hunks)x/supply/types/expected_keepers.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/supply/types/expected_keepers.go
🧰 Additional context used
📓 Learnings (1)
x/supply/keeper/grpc_query_test.go (1)
Learnt from: arhamchordia
PR: quicksilver-zone/quicksilver#1803
File: x/supply/keeper/keeper_test.go:31-66
Timestamp: 2025-01-28T19:03:57.668Z
Learning: Avoid suggesting test cases that duplicate functionality already tested in msg_server_test.go, particularly for module account operations and balance transfers. Focus on unique keeper functionality instead.
🪛 Buf (1.47.2)
proto/quicksilver/supply/v1/query.proto
4-4: import "cosmos/base/v1beta1/coin.proto": file does not exist
(COMPILE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: Analyze
🔇 Additional comments (6)
x/supply/keeper/grpc_query_test.go (1)
1-112: LGTM! Comprehensive test coverage.The test suite provides excellent coverage:
- Tests both enabled and disabled states
- Validates excluded accounts behavior
- Verifies TopN functionality with realistic values
x/supply/keeper/msg_server_test.go (1)
48-75: LGTM! Good test coverage for invalid cases.The additional test cases for invalid and blocked addresses enhance the robustness of the test suite.
x/supply/keeper/keeper.go (3)
68-70: LGTM!Simple and clean implementation of the feature flag.
76-76: LGTM! Good optimization.Using
bytes.Equalfor address comparison is more efficient than string comparison.
99-139: LGTM! Well-structured implementation.The implementation is efficient and secure:
- Uses maps for O(1) lookups
- Properly filters module accounts
- Includes bounds checking to prevent panic
- Correctly aggregates balances from both direct holdings and delegations
docs/swagger.yml (1)
5249-5296: LGTM! Well-documented API specification.The Swagger specification for the new topN endpoint is complete and follows OpenAPI conventions:
- Clear endpoint path and method
- Proper parameter definition with type and format
- Well-structured response schema
- Standard error response handling
1. Summary
Add rest endpoint to supply module that returns topN accounts. Used by CoinmarketCap.
2.Type of change
Summary by CodeRabbit
New Features
/quicksilver/supply/v1/topn/{n}to retrieve top N accounts by balance.Technical Improvements
Tests
TopNfunctionality and various scenarios for theSupplyKeeper.