Adding upcoming gauges query #1195

xBalbinus · 2022-04-05T16:38:29Z

Closes: #1158

Description

Added new request definitions in the proto. UpcomingGaugesPerDenom, UpcomingGaugesPerDenomRequest, UpcomingGaugesPerDenomResponse

Made proto-all

Added a keeper method in x/incentives/keeper/grpc_query.go .

Ditto for adding a test for this in grpc_query_test.go: note: I implemented tests as well for querying active upcoming gauges per denom. At the moment, they're not table driven but still diving deeper in and will implement soon.

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

proto/osmosis/incentives/query.proto

ValarDragon · 2022-04-05T16:49:54Z

Nice work! I think the mytestnet folder got accidentally committed

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

ValarDragon · 2022-04-05T17:05:57Z

x/incentives/keeper/grpc_query.go

+	pageRes, err := query.FilteredPaginate(valStore, req.Pagination, func(key []byte, value []byte, accumulate bool) (bool, error) {
+		upcomingGauges := k.GetUpcomingGauges(ctx)
+		for _, gauge := range upcomingGauges {
+			if gauge.DistributeTo.Denom == req.Denom {
+				gauges = append(gauges, gauge)
+			}
+		}
+		return true, nil
+	})
+	if err != nil {
+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
+	return &types.UpcomingGaugesPerDenomResponse{Data: gauges, Pagination: pageRes}, nil


On reviewing this PR, I'm realizing that every filtered paginate usage in this package is wrong. I think its fine to merge this as is though, since its not a real performance overhead / can be fixed later. (We should make an issue to track it)

Filtered paginate's method of working is actually pretty awkward / maybe we need to change it at the SDK level. It gives you the value in []bytes, that this code should then de-marshal, and check if it meets the required constraints. If accumulate is true, only then add it to the gauges result. But this is a high amount of overhead tbh. I think its probably worth getting this refactored in the SDK to be simple, with go 1.18 Generics.

I'll add more detail on this / convert it to an issue, doesn't block this PR!

ValarDragon

Nice job on your first code change PR!!

LGTM! (It has the same behavior as ActiveGaugesPerDenom)

We should make two follow on issues:

One brainstorming ways for fixing the erroneous pagination behavior (either we change things directly here, or add better pagination code scaffolding)
Steps for getting a better table driven test framework here. (We need to make it easy to make upcoming, present, and completed gauges in the test suite)

ValarDragon · 2022-04-06T20:47:39Z

x/incentives/keeper/grpc_query_test.go

+	suite.SetupTest()
+
+		// initial check
+	res, err := suite.app.IncentivesKeeper.ActiveGaugesPerDenom(sdk.WrapSDKContext(suite.ctx), &types.ActiveGaugesPerDenomRequest{})


oh you have to make a querier struct now for this to work now. (An update happened on main.

Change to suite.querier,ActiveGaugesPerDenom and I think that should work in all places where this is erroring.

ValarDragon · 2022-04-06T20:47:53Z

Can you run make format to fix the lint errors

xBalbinus · 2022-04-06T20:50:45Z

Sweet, got you, checking it out!

…

On Wed, Apr 6, 2022 at 4:48 PM Dev Ojha ***@***.***> wrote: Can you run make format to fix the lint errors — Reply to this email directly, view it on GitHub <#1195 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASH4FPWSR4FOUEEVUGTTIA3VDX2AJANCNFSM5STK5W3Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ValarDragon · 2022-04-07T03:31:54Z

PR LGTM, its having the liveness test timeout, but this confuses me, since this doesn't even touch the state machine? Any ideas @alexanderbez ?

alexanderbez

Looks great @xBalbinus!

I left some minor feedback. I'm also taking a look at the liveness test.

proto/osmosis/incentives/query.proto

alexanderbez · 2022-04-07T20:53:51Z

x/incentives/client/cli/query.go

+	cmd := &cobra.Command{
+		Use:   "upcoming-gauges-per-denom [denom]",
+		Short: "Query scheduled gauges per denom",
+		Long: strings.TrimSpace(


nit: for concise/straight forward CLI handlers like this, I'd argue Long is not necessary as it does not offer any additional help or insight.

bump @xBalbinus

x/incentives/keeper/grpc_query.go

alexanderbez · 2022-04-07T21:07:04Z

Re liveness failures:

➜ starport chain serve --reset-once -v -c ./starport.ci.yml
[STARPORT] Cosmos SDK's version is: stargate - v0.45.1
[STARPORT] 
[STARPORT] 🛠️  Building proto...
[STARPORT] cannot build app:
[STARPORT] 
[STARPORT]      cannot build app:
[STARPORT] 
[STARPORT]      error while running command /var/folders/jl/532qskgn2y32m_qg3p4l8q440000gn/T/protoc832737936: osmosis/incentives/query.proto:136:54: Field number 1 has already been used in "osmosis.incentives.UpcomingGaugesPerDenomRequest" by field "denom".
[STARPORT] : exit status 1

Just an FYI, if liveness fails or timesout, you can always run it locally to find out why:

$ starport chain serve --reset-once -v -c ./starport.ci.yml

alexanderbez · 2022-04-07T21:08:18Z

proto/osmosis/incentives/query.proto

+  string denom = 1;
+  cosmos.base.query.v1beta1.PageRequest pagination = 1;


We gotta be more careful with reviewing proto structs. I missed this too.

Suggested change

string denom = 1;

cosmos.base.query.v1beta1.PageRequest pagination = 1;

string denom = 1;

cosmos.base.query.v1beta1.PageRequest pagination = 2;

Oop! Updated.

Yeah your totally right. Is there a way we can lint for this more easily? I don't get why make proto-all even compiled with this as an issue.

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

xBalbinus · 2022-04-07T21:42:37Z

make proto-format done; changes committed! Thanks @alexanderbez and @ValarDragon for your input & help.

x/incentives/keeper/grpc_query_test.go

codecov-commenter · 2022-04-12T15:47:19Z

Codecov Report

Merging #1195 (3190691) into main (b6d342a) will decrease coverage by 0.82%.
The diff coverage is 3.71%.

@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
- Coverage   20.90%   20.08%   -0.83%     
==========================================
  Files         196      203       +7     
  Lines       25425    26786    +1361     
==========================================
+ Hits         5316     5379      +63     
- Misses      19118    20409    +1291     
- Partials      991      998       +7

Impacted Files	Coverage Δ
x/claim/abci.go	`0.00% <ø> (ø)`
x/claim/client/cli/query.go	`34.45% <ø> (ø)`
x/claim/client/cli/tx.go	`0.00% <ø> (ø)`
x/claim/handler.go	`0.00% <ø> (ø)`
x/claim/keeper/claim.go	`64.33% <ø> (ø)`
x/claim/keeper/grpc_query.go	`2.50% <ø> (ø)`
x/claim/keeper/hooks.go	`28.00% <ø> (ø)`
x/claim/keeper/keeper.go	`87.50% <ø> (ø)`
x/claim/keeper/params.go	`81.81% <ø> (ø)`
x/claim/module.go	`58.06% <ø> (ø)`
... and 102 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40d7094...3190691. Read the comment docs.

x/incentives/keeper/grpc_query.go

…uery.go

x/incentives/keeper/grpc_query.go

alexanderbez

🚀🚀🚀🚀🚀🚀🚀🚀🚀

alexanderbez · 2022-04-13T10:29:24Z

I'm confused as to how CI is passing. Starport liveness test reports:

[STARPORT] # github.com/osmosis-labs/osmosis/v7/x/incentives/keeper
[STARPORT] ../../x/incentives/keeper/grpc_query.go:173:47: unknown field 'Data' in struct literal of type "github.com/osmosis-labs/osmosis/v7/x/incentives/types".UpcomingGaugesPerDenomResponse
[STARPORT] : exit status 2

xBalbinus · 2022-04-13T12:21:45Z

Should we fix the liveliness tests first before committing? What might be causing this?

alexanderbez · 2022-04-13T13:09:55Z

Should we fix the liveliness tests first before committing? What might be causing this?

@xBalbinus Yes, we need to fix it. The liveness test does two things primarily -- test the chain builds and runs. The error above tells you:

[STARPORT] ../../x/incentives/keeper/grpc_query.go:173:47: unknown field 'Data' in struct literal of type "github.com/osmosis-labs/osmosis/v7/x/incentives/types".UpcomingGaugesPerDenomResponse

xBalbinus · 2022-04-13T22:23:06Z

I just pushed out some fixes / changes & make all has been doing well for me (all tests passed). Let me know if the liveliness is still an issue.

ValarDragon · 2022-04-14T13:12:53Z

So to fix that liveness error, can you do make proto-all, and then replace the usage of Data on line 173, with UpcomingGauges which is what should get written after make proto-all is ran? That should hopefully fix the liveness test

* Add queries for getting Upcoming gauges by denom * rudimentary tests for denomquery * Update proto/osmosis/incentives/query.proto Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * removed mytestnet * fixed query tests * fixed query tests * Update proto/osmosis/incentives/query.proto pagination number Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * Update x/incentives/keeper/grpc_query.go Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * implemented feedback * amended minor bug with prefixStore * investigating BeginDistribution issues * investigating grpc_query_test.go * Fix grpc query test * fixed grpc tests, added nil checks for grpc query, removed long for query.go * additional grpcquery fixes (double if removed) * liveliness error fix * gofmt grpc_query_test.go * liveliness test fixes * liveliness test fixes Co-authored-by: Xiangan He <xiangan@polychain.capital> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Dev Ojha <dojha@berkeley.edu> (cherry picked from commit eb39af7) # Conflicts: # docs/core/proto-docs.md # go.mod # go.sum # proto/osmosis/gamm/v1beta1/query.proto # x/gamm/types/query.pb.go # x/incentives/client/cli/query.go # x/incentives/keeper/grpc_query.go

* Adding upcoming gauges query (#1195) * Add queries for getting Upcoming gauges by denom * rudimentary tests for denomquery * Update proto/osmosis/incentives/query.proto Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * removed mytestnet * fixed query tests * fixed query tests * Update proto/osmosis/incentives/query.proto pagination number Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * Update x/incentives/keeper/grpc_query.go Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * implemented feedback * amended minor bug with prefixStore * investigating BeginDistribution issues * investigating grpc_query_test.go * Fix grpc query test * fixed grpc tests, added nil checks for grpc query, removed long for query.go * additional grpcquery fixes (double if removed) * liveliness error fix * gofmt grpc_query_test.go * liveliness test fixes * liveliness test fixes Co-authored-by: Xiangan He <xiangan@polychain.capital> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Dev Ojha <dojha@berkeley.edu> (cherry picked from commit eb39af7) # Conflicts: # docs/core/proto-docs.md # go.mod # go.sum # proto/osmosis/gamm/v1beta1/query.proto # x/gamm/types/query.pb.go # x/incentives/client/cli/query.go # x/incentives/keeper/grpc_query.go * fixing conflicts * Revert "fixing conflicts" This reverts commit 28022f1. * Fix merge conflicts * changed queriers to app.IncentiveKeepers Co-authored-by: Xiangan He <76530366+xBalbinus@users.noreply.github.com> Co-authored-by: Xiangan He <xiangan@polychain.capital> Co-authored-by: Dev Ojha <dojha@berkeley.edu>

xBalbinus added 2 commits March 29, 2022 10:33

Add queries for getting Upcoming gauges by denom

3d85cbe

rudimentary tests for denomquery

5c44cb2

xBalbinus requested a review from ValarDragon April 5, 2022 16:38

ValarDragon reviewed Apr 5, 2022

View reviewed changes

proto/osmosis/incentives/query.proto Outdated Show resolved Hide resolved

xBalbinus changed the title ~~Xiangan/gaugequeries~~ Adding upcoming gauges query Apr 5, 2022

xBalbinus and others added 3 commits April 5, 2022 12:52

Update proto/osmosis/incentives/query.proto

beaba13

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

removed mytestnet

5920589

removed mynetworks

7774b43

ValarDragon reviewed Apr 5, 2022

View reviewed changes

ValarDragon approved these changes Apr 6, 2022

View reviewed changes

ValarDragon reviewed Apr 6, 2022

View reviewed changes

xBalbinus added 3 commits April 6, 2022 21:56

fixed query methods to be defined with querier

0599bc1

fixed query tests

523298f

fixed query tests

4f1e16e

alexanderbez requested changes Apr 7, 2022

View reviewed changes

alexanderbez added C:gRPC Issues and PRs related to the gRPC service and HTTP gateway. C:x/incentives labels Apr 7, 2022

alexanderbez requested changes Apr 7, 2022

View reviewed changes

xBalbinus and others added 5 commits April 7, 2022 17:09

Update proto/osmosis/incentives/query.proto pagination number

55b7e56

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/incentives/keeper/grpc_query.go

49aab2b

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

implemented feedback

96d92a2

implemented feedback & formatting

709a4ef

amended minor bug with prefixStore

3b62895

ValarDragon reviewed Apr 7, 2022

View reviewed changes

x/incentives/keeper/grpc_query_test.go Outdated Show resolved Hide resolved

ValarDragon mentioned this pull request Apr 7, 2022

Nil pointer guarding for queries #1224

Closed

ValarDragon added the A:backport/v7.x Do not use. backport patches to v7.x branch label Apr 7, 2022

xBalbinus and others added 3 commits April 8, 2022 13:52

investigating BeginDistribution issues

37e44a0

investigating grpc_query_test.go

7dd6e7d

Fix grpc query test

fbca4df

alexanderbez reviewed Apr 12, 2022

View reviewed changes

x/incentives/keeper/grpc_query.go Show resolved Hide resolved

xBalbinus added 2 commits April 12, 2022 12:19

fixed grpc tests, added nil checks for grpc query, removed long for q…

1590ea8

…uery.go

fixed grpc tests, added nil checks for grpc query, removed long for q…

0aa2070

…uery.go

ValarDragon reviewed Apr 12, 2022

View reviewed changes

x/incentives/keeper/grpc_query.go Outdated Show resolved Hide resolved

additional grpcquery fixes (double if removed)

f02d7d2

alexanderbez approved these changes Apr 13, 2022

View reviewed changes

liveliness error fix

d9f85c3

github-actions bot added the C:CLI label Apr 13, 2022

gofmt grpc_query_test.go

04677c9

ValarDragon and others added 3 commits April 14, 2022 15:59

Merge branch 'main' into xiangan/gaugequeries

2a1acb8

liveliness test fixes

a3737b8

liveliness test fixes

3190691

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Apr 15, 2022

ValarDragon merged commit eb39af7 into main Apr 15, 2022

ValarDragon deleted the xiangan/gaugequeries branch April 15, 2022 02:33

mergify bot mentioned this pull request Apr 15, 2022

Adding upcoming gauges query (backport #1195) #1257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding upcoming gauges query #1195

Adding upcoming gauges query #1195

xBalbinus commented Apr 5, 2022

ValarDragon commented Apr 5, 2022

ValarDragon Apr 5, 2022

ValarDragon left a comment

ValarDragon Apr 6, 2022

ValarDragon commented Apr 6, 2022

xBalbinus commented Apr 6, 2022 via email

ValarDragon commented Apr 7, 2022

alexanderbez left a comment

alexanderbez Apr 7, 2022

alexanderbez Apr 12, 2022

alexanderbez commented Apr 7, 2022 •

edited

alexanderbez Apr 7, 2022 •

edited

xBalbinus Apr 7, 2022

ValarDragon Apr 7, 2022

xBalbinus commented Apr 7, 2022

codecov-commenter commented Apr 12, 2022 •

edited

alexanderbez left a comment

alexanderbez commented Apr 13, 2022

xBalbinus commented Apr 13, 2022

alexanderbez commented Apr 13, 2022 •

edited

xBalbinus commented Apr 13, 2022

ValarDragon commented Apr 14, 2022

		string denom = 1;
		cosmos.base.query.v1beta1.PageRequest pagination = 1;

Adding upcoming gauges query #1195

Adding upcoming gauges query #1195

Conversation

xBalbinus commented Apr 5, 2022

Description

ValarDragon commented Apr 5, 2022

ValarDragon Apr 5, 2022

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

ValarDragon Apr 6, 2022

Choose a reason for hiding this comment

ValarDragon commented Apr 6, 2022

xBalbinus commented Apr 6, 2022 via email

ValarDragon commented Apr 7, 2022

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Apr 7, 2022

Choose a reason for hiding this comment

alexanderbez Apr 12, 2022

Choose a reason for hiding this comment

alexanderbez commented Apr 7, 2022 • edited

alexanderbez Apr 7, 2022 • edited

Choose a reason for hiding this comment

xBalbinus Apr 7, 2022

Choose a reason for hiding this comment

ValarDragon Apr 7, 2022

Choose a reason for hiding this comment

xBalbinus commented Apr 7, 2022

codecov-commenter commented Apr 12, 2022 • edited

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez commented Apr 13, 2022

xBalbinus commented Apr 13, 2022

alexanderbez commented Apr 13, 2022 • edited

xBalbinus commented Apr 13, 2022

ValarDragon commented Apr 14, 2022

alexanderbez commented Apr 7, 2022 •

edited

alexanderbez Apr 7, 2022 •

edited

codecov-commenter commented Apr 12, 2022 •

edited

alexanderbez commented Apr 13, 2022 •

edited