Gauges queries #8563
Gauges queries #8563
Conversation
najeal
requested review from
mattverse,
p0mvn and
AlpinYukseloglu
as code owners
WalkthroughThe changes enhance the querying capabilities of the incentives module by introducing new RPC methods to retrieve internal and external gauges, as well as gauges linked to specific pool IDs. Additionally, new filtering functions and comprehensive tests accompany these features, improving overall functionality and usability for external integrators. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant QueryService
participant Keeper
participant Database
User->>CLI: Request for Internal Gauges
CLI->>QueryService: Call InternalGauges()
QueryService->>Keeper: GetInternalGauges()
Keeper->>Database: Fetch internal gauges
Database-->>Keeper: Return internal gauges
Keeper-->>QueryService: Return gauges
QueryService-->>CLI: Return gauge data
CLI-->>User: Display internal gauges
sequenceDiagram
participant User
participant CLI
participant QueryService
participant Keeper
participant Database
User->>CLI: Request for Gauges By Pool ID
CLI->>QueryService: Call GaugesByPoolID()
QueryService->>Keeper: GetGaugesFromPoolID(poolID)
Keeper->>Database: Fetch gauges by pool ID
Database-->>Keeper: Return gauges
Keeper-->>QueryService: Return gauges
QueryService-->>CLI: Return gauge data
CLI-->>User: Display gauges for pool ID
Assessment against linked issues
Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (9)
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 (
|
x/incentives/types/gauge.go
Outdated
| func (gauge Gauge) IsInternalGauge() bool { | ||
| return gauge.IsNoLockGauge() && strings.HasPrefix(gauge.DistributeTo.GetDenom(), NoLockInternalPrefix) | ||
| } | ||
|
|
||
| func (gauge Gauge) IsExternalGauge() bool { | ||
| return gauge.IsNoLockGauge() && strings.HasPrefix(gauge.DistributeTo.GetDenom(), NoLockExternalPrefix) |
There was a problem hiding this comment.
AFAIR internal / external gauges does not have to be no lock gauge, (no lock gauge for only CL Pools)
There was a problem hiding this comment.
It's always InternalNoLock or ExternalNoLock from what I understand in Keeper.CreateGauge
osmosis/x/incentives/keeper/gauge.go
Line 126 in fa2168d
But I may be wrong
There was a problem hiding this comment.
osmosis/x/incentives/keeper/gauge.go
Line 157 in fa2168d
I may also be wrong, but please look at this line
There was a problem hiding this comment.
🤔 I may have missunderstood what defines an External/Internal Gauge.
Hence my filtering on DistrTo.Denom looks wrong.
If I can have more information of what defines External/Internal Gauge I could correct it. Do you have this information?
The same for IsLinkToPool function, I am not sure to filter the right way.
Can you give me your opinion? 🙂
Co-authored-by: Matt, Park <[email protected]>
|
Also a kindly reminder to re generate protobuf |
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
|
I updated the way to fetch internal gauges, taking into account the Balancer pools. From what I have understood: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
x/incentives/keeper/gauge.go (1)
Line range hint
36-53:
Review:getGaugesFromIteratorAndFilterFunction.The implementation of
getGaugesFromIteratorAndFiltercorrectly iterates over the iterator and applies the filter function to each gauge. Error handling is done usingpanic, which might be appropriate for the context, but consider logging the error for better traceability.Consider adding logging for errors to improve traceability:
if err != nil { ctx.Logger().Error(fmt.Sprintf("Error unmarshalling gauge IDs: %v", err)) panic(err) }
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
x/incentives/keeper/gauge.go
Outdated
| type GaugeFilterFn func(*types.Gauge) bool | ||
|
|
||
| // getGaugesFromIterator iterates over everything in a gauge's iterator, until it reaches the end. Return all gauges iterated over. | ||
| func (k Keeper) getGaugesFromIteratorAndFilter(ctx sdk.Context, iterator db.Iterator, filter GaugeFilterFn) []types.Gauge { |
There was a problem hiding this comment.
Do we need this change right now? It seems like we're not calling this anymore
| store := ctx.KVStore(q.Keeper.storeKey) | ||
| valStore := prefix.NewStore(store, keyPrefix) | ||
|
|
||
| pageRes, err := query.FilteredPaginate(valStore, pageReq, func(key []byte, value []byte, accumulate bool) (bool, error) { |
There was a problem hiding this comment.
Should we not filter than paginate? I'm curious if we paginate then filter inside, we wouldn't be getting correct pagination (e.g user wants 100 results, we do a filter inside and only returns 30 results). I might be wrong on this
There was a problem hiding this comment.
Huum the way I understand how FilteredPaginate works it will continue to iterate until it reaches the 100 results.
I just missed one important point: to return true at the end of the closure when new element is added to the result.
When the closure returns true the FilteredPaginate function increments the numbered of hit elements and verifies that num_hit < request.Limit before continuing to iterate.
I will make the update.
| } | ||
| return pageRes, gauges, nil | ||
| } | ||
|
|
||
| // filterByPrefixAndDenom filters gauges based on a given key prefix and denom | ||
| func (q Querier) filterByPrefixAndDenom(ctx sdk.Context, prefixType []byte, denom string, pagination *query.PageRequest) (*query.PageResponse, []types.Gauge, error) { |
There was a problem hiding this comment.
This function looks not correct if I am right about the way FilteredPaginate works. What do you think @mattverse
There was a problem hiding this comment.
which part are you sus about?
There was a problem hiding this comment.
Iterating over newGauges the closure returns false if the denom is not the expected one.
In some situations we can miss a gauge
If len(newGauges) == 2
Index 0 : denom is not expecting one
Index 1 : denom is the expecting one
we are going to exit at first iteration without adding the second gauge in the response.
There was a problem hiding this comment.
yeah, this is an edge case as mentioned in the comment that we need to fix :(
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
x/incentives/types/gauge.go (1)
69-79: Method implementation is correct but consider enhancing documentation.The
IsInternalGaugemethod correctly checks for internal gauge prefixes. Consider adding a comment to clarify that the method can check multiple prefixes, enhancing understanding for future maintainers.x/incentives/keeper/grpc_query.go (1)
328-356: Method implementation is correct but consider enhancing documentation.The
getGaugesWithFiltermethod effectively retrieves gauges based on a filter function and pagination parameters. Consider adding a comment to clarify the edge case handling when multiple gauges start at the same time.
|
@mattverse conflicts are resolved 👍 |
|
We don't have a changelog for this, but for the sake of faster iterations merging this for now and I'll create a PR with the changelog right after |
Closes: #6314
What is the purpose of the change
The purpose of the change is to make external integration easier by adding new queries:
Testing and Verifying
This change added tests and can be verified as follows:
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)