R4R: keeper custom queries #1918

sunnya97 · 2018-08-03T21:32:34Z

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Updated all relevant documentation (docs/)
Updated all relevant code comments
Wrote tests
Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
Updated cmd/gaia and examples/

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-08-06T07:07:15Z

Codecov Report

Merging #1918 into develop will decrease coverage by 1.63%.
The diff coverage is 6.63%.

@@            Coverage Diff             @@
##           develop   #1918      +/-   ##
==========================================
- Coverage    63.83%   62.2%   -1.64%     
==========================================
  Files          113     115       +2     
  Lines         6684    6869     +185     
==========================================
+ Hits          4267    4273       +6     
- Misses        2133    2312     +179     
  Partials       284     284

sunnya97 · 2018-08-06T09:10:35Z

Only added to x/gov/rest for now to demonstrate. Will add to CLI in separate PR. Other modules should mimic.

cwgoes · 2018-08-08T10:54:11Z

@sunnya97 What's the rationale for a separate QueryRouter - could we just use the single Router and have default-empty query endpoints? Presumably we want module names to correspond to routes in the same way for both transaction and query handling.

sunnya97 · 2018-08-08T20:12:47Z

Maybe not all modules have queriers, so instead of declaring a querier that just throws, they could just not add their route to the queryrouter. Forcing them to implement an empty querier just because they have a msghandler seems like conflation of concerns.

Also, the router points to the handler. There's nothing actually saying that there needs to be a 1-to-1 mapping from handler to keeper. So not sure how to get to the querier from the handler.

Modules (keepers) don't have names yet. They will need to at some point, though, to do the new errors with strings.

melekes · 2018-08-14T07:59:49Z

baseapp/baseapp.go

+	db          dbm.DB               // common DB backend
+	cms         sdk.CommitMultiStore // Main (uncached) state
+	router      Router               // handle any kind of message
+	queryrouter QueryRouter          // router for redirecting query calls


Maybe it's bike-shedding, but queryRouter is more readable

don't think it's bikeshedding - we should stick to the CamelCase convention

melekes · 2018-08-14T08:01:19Z

baseapp/baseapp.go

@@ -362,6 +366,22 @@ func handleQueryP2P(app *BaseApp, path []string, req abci.RequestQuery) (res abc
 	return sdk.ErrUnknownRequest(msg).QueryResult()
 }

+func handleQueryCustom(app *BaseApp, path []string, req abci.RequestQuery) (res abci.ResponseQuery) {
+	// "/custom" prefix for keeper queries
+	querier := app.queryrouter.Route(path[1])


what's path[1]?

Added comment

melekes · 2018-08-14T08:01:31Z

baseapp/baseapp.go

+	// "/custom" prefix for keeper queries
+	querier := app.queryrouter.Route(path[1])
+	ctx := app.checkState.ctx
+	resBytes, err := querier(ctx, path[2:], req)


what's path[2:]?

yeah let's add some more comments as to what the path is supposed to be

melekes · 2018-08-14T08:05:57Z

baseapp/queryrouter.go

+
+// QueryRouter provides queryables for each query path.
+type QueryRouter interface {
+	AddRoute(r string, h sdk.Querier) (rtr QueryRouter)


h - handler, right?

what's rtr?

router I think

I think the rtr is fine personally

I'd change r --> route and h --> handler

This is fine IMO

melekes · 2018-08-14T08:09:35Z

baseapp/queryrouter.go

+}
+
+type queryrouter struct {
+	routes []queryroute


why not make it a map[string]queryroute?

Idk, this is the way the normal router is, and so I just copied that. I think it may have had to do with allowing for more complex routes in the future (like overriding "gov/proposal" to go somewhere different thatn "gov/*"). @jaekwon

Mmm I'd be fine w/ either map[string]queryroute or not, I think QueryRouter might/should be simpler. But either way duplicate registration should panic.

Ok, switched

melekes · 2018-08-14T08:28:19Z

x/gov/client/rest/rest.go

@@ -476,54 +478,35 @@ func queryProposalsWithParameterFn(cdc *wire.Codec) http.HandlerFunc {
 				return
 			}
 		}
+		if len(strNumLatest) != 0 {
+			numLatest, err = strconv.ParseInt(strNumLatest, 10, 64)


Would be good to create a helper function. I am sure this is not the only case where we parse int64. No?

func parseInt64OrReturnBadRequest(s string, w http. ResponseWriter) (n int64, ok bool) { var err error n, err = strconv.ParseInt(s, 10, 64) if err != nil { w.WriteHeader(http.StatusBadRequest) err := fmt.Errorf("'%s' is not a valid int64", s) w.Write([]byte(err.Error())) return 0, false } return n, true }

melekes · 2018-08-14T08:28:55Z

x/gov/depositsvotes.go

@@ -15,13 +15,45 @@ type Vote struct {
 	Option     VoteOption     `json:"option"`      //  option from OptionSet chosen by the voter
 }

+// Returns whether 2 votes are equal
+func (voteA Vote) Equals(voteB Vote) bool {
+	if voteA.Voter.Equals(voteB.Voter) &&


return voteA.Voter.Equals(voteB.Voter) && voteA.ProposalID == voteB.ProposalID && voteA.Option == voteB.Option

melekes · 2018-08-14T08:29:21Z

x/gov/depositsvotes.go

 // Deposit
 type Deposit struct {
 	Depositer  sdk.AccAddress `json:"depositer"`   //  Address of the depositer
 	ProposalID int64          `json:"proposal_id"` //  proposalID of the proposal
 	Amount     sdk.Coins      `json:"amount"`      //  Deposit amount
 }

+// Returns whether 2 deposits are equal
+func (depositA Deposit) Equals(depositB Deposit) bool {
+	if depositA.Depositer.Equals(depositB.Depositer) &&


return depositA.Depositer.Equals(depositB.Depositer) && depositA.ProposalID == depositB.ProposalID && depositA.Amount.IsEqual(depositB.Amount)

melekes · 2018-08-14T08:33:55Z

x/gov/keeper.go

@@ -131,6 +178,19 @@ func (keeper Keeper) getNewProposalID(ctx sdk.Context) (proposalID int64, err sd
 	return proposalID, nil
 }

+// Peeks the next available ProposalID without incrementing it


this looks wrong because I clearly can see an increment. We should make it clear that this func mutates store.

Oops, good catch. Fixed.

jaekwon · 2018-08-16T02:08:21Z

baseapp/baseapp.go

+func handleQueryCustom(app *BaseApp, path []string, req abci.RequestQuery) (res abci.ResponseQuery) {
+	// path[0] should be "custom" because "/custom" prefix is required for keeper queries.
+	// the queryRouter routes using path[1]. For example, in the path "custom/gov/proposal", queryRouter routes using "gov"
+	querier := app.queryRouter.Route(path[1])


If it doesn't exist, a useful error message...

jaekwon · 2018-08-16T02:08:54Z

baseapp/baseapp.go

+	resBytes, err := querier(ctx, path[2:], req)
+	if err != nil {
+		return abci.ResponseQuery{
+			Code: uint32(err.ABCICode()),


Is there a ResponseQuery.Log where we can put the err.String() in? That's for nondeterministic debug info like this.

jaekwon · 2018-08-16T02:10:56Z

baseapp/queryrouter.go

+}
+
+// AddRoute - TODO add description
+func (rtr *queryrouter) AddRoute(r string, h sdk.Querier) QueryRouter {


sanity check duplicates

jaekwon · 2018-08-16T02:11:47Z

baseapp/setters.go

@@ -74,6 +74,12 @@ func (app *BaseApp) Router() Router {
 	}
 	return app.router
 }
+func (app *BaseApp) QueryRouter() QueryRouter {


What does sealing do on the baseapp and do we need to seal the query router as well? anything else we're missing? docs would be helpful!

I'm not sure why we have the seal it. I just saw that the router was sealed, so I sealed the QueryRouter as well

Hmm, I think we should restrict sealing to fields that actually need to be sealed for clarity, otherwise readers of the code will have a hard time figuring out what our intent was (as apparently we ourselves have a hard time figuring out already!). As I understand, this is just defensive coding - I believe @mossid wrote the original PR - and we only need to do it for write-once fields or fields that should not be read from after a certain event in the app lifecycle.

Also, don't we want to query the QueryRouter after the app is sealed?

jaekwon · 2018-08-16T02:13:35Z

types/account.go

+}
+
+// Returns boolean for whether an AccAddress is empty
+func (bz ValAddress) Empty() bool {


vs nil? What do we do for Empty() and Equals() if any are nil?

Oh, forgot that they can be nil. Fixed.

jaekwon · 2018-08-16T02:19:01Z

x/gov/queryable.go

+
+func queryProposal(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Keeper) (res []byte, err sdk.Error) {
+	var params QueryProposalParams
+	err2 := keeper.cdc.UnmarshalBinary(req.Data, &params)


Would JSON be better? It would be double encoded but might still be easier to deal with.

Yeah, it might be easier to encode for clients, but yeah, then it would be a JSON-encoded blob inside a amino-encoded blob (the QueryRequest).

I think we should go for JSON here, encoding efficiency isn't too much of a concern.

jaekwon · 2018-08-16T02:19:17Z

Have some questions and suggestions but otherwise LGTM.

fedekunze · 2018-08-17T09:47:26Z

x/gov/client/rest/util.go

@@ -100,3 +102,15 @@ func signAndBuild(w http.ResponseWriter, cliCtx context.CLIContext, baseReq base

 	w.Write(output)
 }
+
+func parseInt64OrReturnBadRequest(s string, w http.ResponseWriter) (n int64, ok bool) {


Should this be accessible from cosmos-sdk/client instead ?

Most of this util.go file should move to there, but I'll do that in a separate PR.

fedekunze · 2018-08-17T09:54:11Z

docs/sdk/modules.md

@@ -6,7 +6,7 @@

 The Cosmos SDK has all the necessary pre-built modules to add functionality on top of a `BaseApp`, which is the template to build a blockchain dApp in Cosmos. In this context, a `module` is a fundamental unit in the Cosmos SDK.

-Each module is an extension of the `BaseApp`'s functionalities that defines transactions, handles application state and manages the state transition logic. Each module also contains handlers for messages and transactions, as well as REST and CLI for secure user interactions.
+Each module is an extension of the `BaseApp`'s functionalities that defines transactions, handles application state and manages the state transition logic. Each module also contains handlers for messages and transactions, queriers for handling query requests, as well as REST and CLI for secure user interactions.


I'd add a more detailed description on how Queriers work. Otherwise please submit a separate issue

mossid · 2018-08-20T01:43:21Z

Wouldn't it be better to unmarshal req.Data to interface{} in baseapp and type match in queryhandler, just like as normal handler does? Or, we can type assert in each of the subpath.

cwgoes

Thanks; concept ACK, left comments.

cwgoes · 2018-08-20T15:25:25Z

baseapp/queryrouter.go

+
+// QueryRouter provides queryables for each query path.
+type QueryRouter interface {
+	AddRoute(r string, h sdk.Querier) (rtr QueryRouter)


This is fine IMO

cwgoes · 2018-08-20T15:27:09Z

baseapp/setters.go

@@ -74,6 +74,12 @@ func (app *BaseApp) Router() Router {
 	}
 	return app.router
 }
+func (app *BaseApp) QueryRouter() QueryRouter {


Hmm, I think we should restrict sealing to fields that actually need to be sealed for clarity, otherwise readers of the code will have a hard time figuring out what our intent was (as apparently we ourselves have a hard time figuring out already!). As I understand, this is just defensive coding - I believe @mossid wrote the original PR - and we only need to do it for write-once fields or fields that should not be read from after a certain event in the app lifecycle.

cwgoes · 2018-08-20T15:27:32Z

baseapp/setters.go

@@ -74,6 +74,12 @@ func (app *BaseApp) Router() Router {
 	}
 	return app.router
 }
+func (app *BaseApp) QueryRouter() QueryRouter {


Also, don't we want to query the QueryRouter after the app is sealed?

cwgoes · 2018-08-20T15:28:05Z

types/account.go

+		return true
+	}
+	bz2 := AccAddress{}
+	return (bytes.Compare(bz.Bytes(), bz2.Bytes()) == 0)


Extra parentheses

cwgoes · 2018-08-20T15:28:13Z

types/account.go

+	if bz.Empty() && bz2.Empty() {
+		return true
+	}
+	return (bytes.Compare(bz.Bytes(), bz2.Bytes()) == 0)


Extra parentheses

cwgoes · 2018-08-20T15:31:30Z

x/gov/queryable.go

+	}
+
+	if proposal.GetStatus() == StatusDepositPeriod {
+		return keeper.cdc.MustMarshalBinaryBare(EmptyTallyResult()), nil


(nit) Can we keep a result value and marshal once at the end? A bit cleaner.

cwgoes · 2018-08-20T15:32:43Z

x/gov/keeper.go

+			continue
+		}
+
+		if validProposalStatus(status) {


If it isn't, can we throw an error instead of failing silently?

That's how it knows whether its filtering for a status or not. An "empty filter status" returns false on validProposalStatus

Ah hmm, it would still be nice to be explicit but I guess it's OK for now.

cwgoes · 2018-08-20T15:33:07Z

x/gov/keeper.go

+		}
+
+		proposal := keeper.GetProposal(ctx, proposalID)
+		if proposal == nil {


Is this expected? Might the proposal have been deleted? Maybe we should add a DeletedProposal record or something so we can inform the client.

Only proposals that don't make it to the voting period (not get enough deposit) get deleted from state. Don't think its necessary to add DeletedProposal record to state for them.

cwgoes · 2018-08-20T15:33:24Z

x/gov/keeper.go

+
+		if depositerAddr != nil && len(depositerAddr) != 0 {
+			_, found := keeper.GetDeposit(ctx, proposalID, depositerAddr)
+			if !found {


Is this expected?

Basically, this is for filtering. In this case, depositerAddr is what you're trying to filter by. If it is empty, that means you're not trying to filter by that.

cwgoes · 2018-08-20T15:33:39Z

x/gov/keeper.go

+	for proposalID := maxProposalID - numLatest; proposalID < maxProposalID; proposalID++ {
+		if voterAddr != nil && len(voterAddr) != 0 {
+			_, found := keeper.GetVote(ctx, proposalID, voterAddr)
+			if !found {


Is this expected? Do we ever have a proposal but not votes?

Same as above

cwgoes

utACK

sunnya97 requested review from cwgoes, ebuchman and rigelrozanski as code owners August 3, 2018 21:32

sunnya97 force-pushed the sunny/keeper_queries branch from 1f2cc30 to 31dd45f Compare August 6, 2018 06:55

sunnya97 requested a review from zramsay as a code owner August 6, 2018 09:09

sunnya97 changed the title ~~WIP: keeper custom queries~~ R4R: keeper custom queries Aug 6, 2018

sunnya97 assigned jaekwon, sunnya97, rigelrozanski and cwgoes Aug 6, 2018

sunnya97 requested a review from jaekwon August 6, 2018 09:11

sunnya97 added the ready-for-review label Aug 6, 2018

sunnya97 force-pushed the sunny/keeper_queries branch from 979114b to 5d7b0b2 Compare August 6, 2018 21:55

melekes reviewed Aug 14, 2018

View reviewed changes

sunnya97 force-pushed the sunny/keeper_queries branch from b1bcbe4 to 31cd4af Compare August 16, 2018 01:03

jaekwon reviewed Aug 16, 2018

View reviewed changes

sunnya97 force-pushed the sunny/keeper_queries branch from 1ad8b56 to 307fff1 Compare August 17, 2018 07:42

fedekunze reviewed Aug 17, 2018

View reviewed changes

sunnya97 mentioned this pull request Aug 17, 2018

Add tally endpoint for Gov CLI and REST #2080

Closed

2 tasks

cwgoes suggested changes Aug 20, 2018

View reviewed changes

sunnya97 added 10 commits August 22, 2018 00:11

custom queriables

1d1a956

added querier to gov module

804baa7

moved queryrouter to behind baseapp seal

cc1d1fb

added to gov rest

1311117

PENDING

0eed9d1

Address Anton's comments

0134c3b

addressed Jae's comments

97f7b88

run queries against cachewrapped commit state, not checkstate

44bf69e

added tally query route to gov

50dd53f

address Chris's comments

5ae20d2

sunnya97 force-pushed the sunny/keeper_queries branch from fae83c5 to 5ae20d2 Compare August 22, 2018 07:15

fixed GetLastProposalID function

7f43d3b

cwgoes approved these changes Aug 22, 2018

View reviewed changes

cwgoes merged commit b23fe15 into develop Aug 22, 2018

cwgoes deleted the sunny/keeper_queries branch August 22, 2018 10:51

fedekunze mentioned this pull request Aug 23, 2018

Improve performance of /stake/delegators/{delegatorAddr} luniehq/lunie#1133

Closed

alessio mentioned this pull request Mar 10, 2020

types/rest: add unit tests #5779

Merged

11 tasks

neutron-robot mentioned this pull request Feb 28, 2024

[Snyk] Upgrade postcss from 8.4.18 to 8.4.34 neutron-org/cosmos-sdk#26

Closed

neutron-robot mentioned this pull request Mar 4, 2024

[Snyk] Upgrade postcss from 8.4.18 to 8.4.35 neutron-org/cosmos-sdk#29

Closed

R4R: keeper custom queries #1918

R4R: keeper custom queries #1918

Conversation

sunnya97 commented Aug 3, 2018 • edited Loading

codecov bot commented Aug 6, 2018 • edited Loading

Codecov Report

sunnya97 commented Aug 6, 2018

cwgoes commented Aug 8, 2018

sunnya97 commented Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnya97 Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaekwon Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnya97 Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaekwon commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossid commented Aug 20, 2018 • edited Loading

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

sunnya97 commented Aug 3, 2018 •

edited

Loading

codecov bot commented Aug 6, 2018 •

edited

Loading

sunnya97 commented Aug 8, 2018 •

edited

Loading

sunnya97 Aug 15, 2018 •

edited

Loading

melekes Aug 14, 2018 •

edited

Loading

jaekwon Aug 16, 2018 •

edited

Loading

sunnya97 Aug 17, 2018 •

edited

Loading

mossid commented Aug 20, 2018 •

edited

Loading