Fix incentives query filtering #1759
Conversation
issue-pagination
Issue pagination
x/incentives/keeper/grpc_query.go
Outdated
| return false, nil | ||
| } | ||
| if accumulate { | ||
| gauges = append(gauges, newGauges...) |
There was a problem hiding this comment.
Shouldn't we be appending a single element, i.e. newGauges[i] instead of the entire slice each time?
There was a problem hiding this comment.
yes sure, my mistake
x/incentives/keeper/grpc_query.go
Outdated
| } | ||
| for i := 0; i < len(newGauges); i++ { |
There was a problem hiding this comment.
| } | |
| for i := 0; i < len(newGauges); i++ { | |
| } | |
| for i, g := range newGauges { |
There was a problem hiding this comment.
Thanks for the PR!
Since your touching this area, would you be open to de-duplicating logic across:
Gauges, ActiveGauges, UpcomingGauges, ActiveGaugesByDenom, etc?
Basically make a generic method that takes in the prefix, and an optional parameter for 'denom', and re-uses all of the logic here? It would be a very welcome 100 line deletion here :)
Also can you add a comment above the
newGauges, err := q.getGaugeFromIDJsonBytes(ctx, value)
line saying to the extent of "This may return multiple gauges at once if two gauges start at the same time. For now this is treated as an edge case that is not of importance"
thanks for the suggestion. |
x/incentives/keeper/grpc_query.go
Outdated
| // For now this is treated as an edge case that is not of importance | ||
| newGauges, err := q.getGaugeFromIDJsonBytes(ctx, value) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
| panic(err) | |
| return false, err |
x/incentives/keeper/grpc_query.go
Outdated
| // For now this is treated as an edge case that is not of importance | ||
| newGauges, err := q.getGaugeFromIDJsonBytes(ctx, value) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
| panic(err) | |
| return false, err |
Yes I did |
* fix filtering logic * add test * format * fix: append element in loop * use range * optimize code, make filtering into a separate function * format * add to changelog (cherry picked from commit b464728) # Conflicts: # x/incentives/keeper/grpc_query_test.go
* Fix incentives query filtering (#1759) Co-authored-by: Hieu Vu <[email protected]>
Closes: #1218
What is the purpose of the change
Fix gauges query filter: