PSE Score query implementation#25
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a new gRPC query endpoint for retrieving delegator scores in the PSE module, along with a significant internal refactoring of the delegation time entry storage structure. The changes enable efficient querying of delegator scores by reorganizing the key structure to support prefix-based lookups by delegator address.
Key Changes
- Added a new
ScoregRPC query endpoint that returns the total score (accumulated + current period) for a delegator - Refactored
DelegationTimeEntrieskey structure from(ValidatorAddress, DelegatorAddress)to(DelegatorAddress, ValidatorAddress)to enable efficient prefix queries - Implemented
CalculateDelegatorScoremethod that combines accumulated scores with real-time calculation from active delegations
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/tx/pse/v1/query.proto | Defines the new Score query RPC with request/response messages and HTTP endpoint mapping |
| x/pse/types/query.pb.go | Generated protobuf code for new query types with proper marshaling/unmarshaling logic |
| x/pse/types/query.pb.gw.go | Generated gRPC-gateway handlers for HTTP REST API support |
| x/pse/keeper/keeper.go | Updates DelegationTimeEntries key codec to reflect the refactored key structure |
| x/pse/keeper/delegation.go | Adds CalculateDelegatorScore function and updates existing methods to use new key order |
| x/pse/keeper/distribute.go | Updates key extraction logic to match the new key structure |
| x/pse/keeper/grpc.go | Implements the Score query handler with address validation and score calculation |
| x/pse/keeper/grpc_query_test.go | Comprehensive test suite covering multiple scenarios including edge cases |
| x/pse/keeper/hooks_test.go | Updates test assertions to use the refactored key structure |
| docs/api.md | Documents the new query endpoint with request/response specifications |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for ; iter.Valid(); iter.Next() { | ||
| kv, err := iter.KeyValue() | ||
| if err != nil { | ||
| return sdkmath.Int{}, err |
There was a problem hiding this comment.
Error handling during iteration should close the iterator before returning. If iter.KeyValue() returns an error, the iterator is not closed, potentially causing a resource leak. Consider using iter.Close() before returning the error.
if err != nil {
iter.Close()
return sdkmath.Int{}, err
}| if err != nil { | ||
| return sdkmath.Int{}, err |
There was a problem hiding this comment.
Error handling for calculateAddedScore should close the iterator before returning. If calculateAddedScore returns an error, the iterator is not closed, potentially causing a resource leak. Consider using iter.Close() before returning the error.
if err != nil {
iter.Close()
return sdkmath.Int{}, err
}31d79f1 to
f96f051
Compare
miladz68
left a comment
There was a problem hiding this comment.
@miladz68 reviewed 7 of 9 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
miladz68
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @masihyeganeh, @TxCorpi0x, and @ysv)
x/pse/keeper/grpc_query_test.go line 1 at r2 (raw file):
package keeper_test
I think we should add an integration test as well.
ysv
left a comment
There was a problem hiding this comment.
@ysv reviewed 7 of 9 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x)
x/pse/keeper/grpc_query_test.go line 1 at r2 (raw file):
Previously, miladz68 (milad) wrote…
I think we should add an integration test as well.
yes, would be nice to have it
Either here or as separate PR if this one is blocking
TxCorpi0x
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @masihyeganeh and @miladz68)
x/pse/keeper/grpc_query_test.go line 1 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
yes, would be nice to have it
Either here or as separate PR if this one is blocking
Let's merge this one so i can raise the next queries, then in a separate PR I will add integration tests to cover all of queries.
I added check list item to the task so I don't forget it
ysv
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @masihyeganeh, @miladz68, and @TxCorpi0x)
miladz68
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @masihyeganeh and @TxCorpi0x)
Description
This pull request introduces a new gRPC query endpoint for retrieving a delegator's current score in the PSE module, along with significant internal refactoring to the way delegation time entries are stored and accessed. The changes also include comprehensive tests for the new query and update the documentation accordingly.
API and Query Enhancements
Scoreto theQueryservice, allowing clients to fetch the current total score of a delegator. This includes both the accumulated score snapshot and any uncalculated scores from active delegations since the last distribution. (proto/tx/pse/v1/query.proto,x/pse/keeper/grpc.go,docs/api.md)QueryScoreRequestandQueryScoreResponse, and documented their usage in the API reference. (proto/tx/pse/v1/query.proto,docs/api.md)Internal Data Structure Refactoring
DelegationTimeEntriesfromcollections.Pair[sdk.ValAddress, sdk.AccAddress]tocollections.Pair[sdk.AccAddress, sdk.ValAddress]throughout the keeper, distribution logic, and tests. This change enables efficient prefix queries for all delegations by a specific delegator, improving performance and simplifying code. (x/pse/keeper/keeper.go,x/pse/keeper/delegation.go,x/pse/keeper/distribute.go,x/pse/keeper/hooks_test.go)Score Calculation Logic
CalculateDelegatorScoremethod to the keeper, which computes the total score for a delegator by combining the accumulated snapshot and the current period score calculated on-demand from active delegations. (x/pse/keeper/delegation.go)Testing
grpc_query_test.goto cover various scenarios for theScorequery, including: no score, accumulated score, active delegation, combination of accumulated and current period scores, multiple delegations, and invalid address handling. (x/pse/keeper/grpc_query_test.go)These changes together provide a robust, efficient, and well-tested mechanism for querying delegator scores in the PSE module.
Reviewers checklist:
Authors checklist
This change is