Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

R4R: generalize query response with height #4573

Merged
merged 19 commits into from
Jul 1, 2019
Merged

R4R: generalize query response with height #4573

merged 19 commits into from
Jul 1, 2019

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Jun 17, 2019

Addition to #4536, no longer specific to account queries. Allows for validator endpoints to return height in the response. closes #4609

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


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
Copy link

codecov bot commented Jun 17, 2019

Codecov Report

Merging #4573 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4573      +/-   ##
==========================================
+ Coverage   52.65%   52.66%   +0.01%     
==========================================
  Files         261      261              
  Lines       16410    16410              
==========================================
+ Hits         8640     8642       +2     
+ Misses       7124     7122       -2     
  Partials      646      646

@codecov
Copy link

codecov bot commented Jun 17, 2019

Codecov Report

Merging #4573 into master will increase coverage by 0.1%.
The diff coverage is 64%.

@@            Coverage Diff            @@
##           master    #4573     +/-   ##
=========================================
+ Coverage   53.62%   53.72%   +0.1%     
=========================================
  Files         260      260             
  Lines       16177    16197     +20     
=========================================
+ Hits         8675     8702     +27     
+ Misses       6856     6846     -10     
- Partials      646      649      +3

types/rest/rest.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate some test cases

types/rest/rest.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is API breaking for all endpoints that will do this. Is this intended? If so, we must make sure swagger is also updated.

@alexanderbez alexanderbez added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). ready-for-review labels Jun 18, 2019
@alexanderbez alexanderbez added the S:blocked Status: Blocked label Jun 20, 2019
@colin-axner colin-axner reopened this Jun 21, 2019
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. There is nothing that actually sets the height @colin-axner.

e.g.

	res, _, err := ar.querier.QueryWithData(fmt.Sprintf("custom/%s/%s", QuerierRoute, QueryAccount), bs)
	if err != nil {
		return nil, err
	}

Where do we set the height when returning the result?

@colin-axner
Copy link
Contributor Author

@alexanderbez ohh yea... There are a few other places this happens as well where a helper function does the data retrieval and just returns the result/error. Seems to me, if you want height to be returned in a response, then it it needs to be passed all the way down to the client. This would mean changing GetAccount to return height, as well as any other data retrieval functions to always return result, height, error. Should I update it to this format?

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 24, 2019

@colin-axner not sure I completely follow, but this is what needs to happen I think:

  1. Remove the recently added AccountWithHeight custom type and logic (essentially revert the PR)
  2. Update all relevant query REST handlers as follows:
res, _, err := cliCtx.QueryWithData(route, bz)
if err != nil {
    rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
    return
}

rest.PostProcessResponse(w, cliCtx, res)

Now becomes:

res, height, err := cliCtx.QueryWithData(route, bz)
if err != nil {
    rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
    return
}

cliCtx = cliCtx.WithHeight(height)
rest.PostProcessResponse(w, cliCtx, res)

I know (2) will be a huge PIA, but this is how we get the responses to have the height.

Does this make sense?

@colin-axner
Copy link
Contributor Author

@alexanderbez Both those have been done, I think the issue is here where you have:

account, err := accGetter.GetAccount(addr)
if err != nil {
	rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error())
	return
}

rest.PostProcessResponse(w, cliCtx, account)

because GetAccount() doesn't return height and therefore (2) can't be done to set the height in context

@alexanderbez
Copy link
Contributor

@colin-axner phenomenal, just checked and you're correct -- my mistake!

Wrt to the account query, I guess this is a special case huh? No biggie, let's update AccountRetriever to also include a GetAccountWithHeight method.

@colin-axner
Copy link
Contributor Author

colin-axner commented Jun 25, 2019

@alexanderbez updated, the only issue is not all of the gov endpoints will return the query height. I can't find a work around for querying with tags without rewriting a lot of code. If these endpoints query for a specific height then the height will be included in the response. If they are querying using the latest height then the height won't be included because it is not set.

Specifically in QueryByTxsTags can't return height because:

resTxs, err := node.TxSearch(query, prove, page, limit)
if err != nil {
	return nil, err
}

does not return height.

Furthermore, a lot of the gov endpoint code is intermixed with searching by tags like the following:

if !(propStatus == types.StatusVotingPeriod || propStatus == types.StatusDepositPeriod) {
	res, err = gcutils.QueryDepositsByTxQuery(cliCtx, params) // calls QueryByTxsTags
} else {
	res, _, err = cliCtx.QueryWithData("custom/gov/deposits", bz)
}

@fedekunze
Copy link
Collaborator

@colin-axner yeah, we need to refactor those queries

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

types/rest/rest_test.go Outdated Show resolved Hide resolved
Co-Authored-By: Federico Kunze <[email protected]>
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK -- great job @colin-axner!

wrt to the specific governance queries, there is nothing we can do. Those specific type of queries were requested by the community and are "special" because they're ad-hoc and do tag (event) based queries underneath the hood. By definition the results can all have various heights. This PR is good as-is 👍

@fedekunze
Copy link
Collaborator

is this PR blocked?

@colin-axner colin-axner mentioned this pull request Jun 28, 2019
5 tasks
@alexanderbez alexanderbez removed the S:blocked Status: Blocked label Jun 30, 2019
@alexanderbez
Copy link
Contributor

Nope, just need another ACK from @alessio

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something major needs fixing

"github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top level packages must not depend on x sub packages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offf, good catch -- I missed this!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably remove this test all together @colin-axner and move it to the LCD tests in Gaia.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don’t need LCD tests for queries as those are covered by the contract tests

Copy link
Contributor Author

@colin-axner colin-axner Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, should I remove and move the tests elsewhere or should I refactor the test to use a mock version of account?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the latter

"github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offf, good catch -- I missed this!

"github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably remove this test all together @colin-axner and move it to the LCD tests in Gaia.

@alexanderbez alexanderbez requested a review from alessio July 1, 2019 16:43
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

@alessio alessio merged commit 8d8fd9d into cosmos:master Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Returning height in JSON Response
4 participants