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

Return account queries with height #4536

Merged
merged 4 commits into from
Jun 13, 2019
Merged

Return account queries with height #4536

merged 4 commits into from
Jun 13, 2019

Conversation

colin-axner
Copy link
Contributor

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

closes: #4542


  • 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 11, 2019

Codecov Report

Merging #4536 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4536   +/-   ##
=======================================
  Coverage   53.49%   53.49%           
=======================================
  Files         256      256           
  Lines       16165    16165           
=======================================
  Hits         8648     8648           
  Misses       6870     6870           
  Partials      647      647

@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #4536 into master will increase coverage by <.01%.
The diff coverage is 10.34%.

@@            Coverage Diff            @@
##           master   #4536      +/-   ##
=========================================
+ Coverage   53.49%   53.5%   +<.01%     
=========================================
  Files         256     256              
  Lines       16165   16169       +4     
=========================================
+ Hits         8648    8651       +3     
- Misses       6870    6871       +1     
  Partials      647     647

@alessio
Copy link
Contributor

alessio commented Jun 11, 2019

Very good @colin-axner - how do you intend to test it?

@colin-axner
Copy link
Contributor Author

@alessio I was planning on manually testing via initializing a rest-server and doing some queries on either mainnet or a testnet, but I don't have access to any nodes at the moment. Also it doesn't look like there are any good places in the codebase to add a test for this?

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 generally idea, but the logic needs to change slightly and we should add a LCD test 👍

x/auth/client/rest/query.go Outdated Show resolved Hide resolved
client/context/query.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.

Thanks @colin-axner -- left a few more minor bits of feedback, but this looks great.

@@ -233,9 +243,10 @@ func (ctx CLIContext) verifyProof(queryPath string, resp abci.ResponseQuery) err

// queryStore performs a query from a Tendermint node with the provided a store
// name and path.
func (ctx CLIContext) queryStore(key cmn.HexBytes, storeName, endPath string) ([]byte, error) {
func (ctx CLIContext) queryStore(key cmn.HexBytes, storeName, endPath string) ([]byte, int64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

++

client/context/query.go Outdated Show resolved Hide resolved
client/context/query.go Outdated Show resolved Hide resolved
client/context/query.go Outdated Show resolved Hide resolved
client/context/query.go Outdated Show resolved Hide resolved
client/context/query.go Outdated Show resolved Hide resolved
client/context/query.go Outdated Show resolved Hide resolved
@colin-axner
Copy link
Contributor Author

ACK generally idea, but the logic needs to change slightly and we should add a LCD test

Can the LCD test be contained in this module or would it need to be a separate package like in gaia?

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 12, 2019

ACK generally idea, but the logic needs to change slightly and we should add a LCD test

Can the LCD test be contained in this module or would it need to be a separate package like in gaia?

ohhhhh since we moved LCD tests to gaia, this might be difficult to do. I wouldn't worry too much about testing then since all we're really doing is adding an extra return value.

@@ -12,6 +12,13 @@ import (
"github.com/cosmos/cosmos-sdk/x/auth/types"
)

// AccountWithHeight wraps the embedded Account
// with the height it was queried at
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// with the height it was queried at
// with the height it was queried at.

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 -- just one minor nitpick. Thanks @colin-axner!

// QueryWithData performs a query to a Tendermint node with the provided path
// and a data payload. It returns the result and height of the query upon success
// or an error if the query fails.
func (ctx CLIContext) QueryWithData(path string, data []byte) ([]byte, int64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, looks like QueryWithData is identical to Query! How about we simply remove QueryWithData?

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

@alexanderbez alexanderbez merged commit 95f3d32 into cosmos:master Jun 13, 2019
alessio pushed a commit that referenced this pull request Jul 1, 2019
Addition to #4536, no longer specific to account queries.
Allows for validator endpoints to return height in the response.

Closes: #4609
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update cli context to return height query was performed at
5 participants