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

subspace queries + few more staking queries #969

Merged
merged 5 commits into from
May 10, 2018
Merged

Conversation

rigelrozanski
Copy link
Contributor

closes #871
partial #964

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md

@rigelrozanski rigelrozanski changed the title Rigel/range query range queries + few more staking queries May 8, 2018
@rigelrozanski rigelrozanski changed the title range queries + few more staking queries WIP range queries + few more staking queries May 8, 2018
@rigelrozanski rigelrozanski changed the title WIP range queries + few more staking queries subspace queries + few more staking queries May 8, 2018
@rigelrozanski rigelrozanski requested a review from cwgoes May 8, 2018 20:35
@codecov
Copy link

codecov bot commented May 8, 2018

Codecov Report

Merging #969 into develop will increase coverage by 0.23%.
The diff coverage is 91.66%.

@@             Coverage Diff             @@
##           develop     #969      +/-   ##
===========================================
+ Coverage     60.9%   61.13%   +0.23%     
===========================================
  Files           72       72              
  Lines         3512     3520       +8     
===========================================
+ Hits          2139     2152      +13     
+ Misses        1203     1198       -5     
  Partials       170      170

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Miscellaneous notes

types/store.go Outdated
//----------------------------------------

// key-value result for iterator queries
type KV struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome will update for this

// get the command to query a candidate
func GetCmdQueryCandidates(storeName string, cdc *wire.Codec) *cobra.Command {
cmd := &cobra.Command{
Use: "candidates",
Copy link
Contributor

Choose a reason for hiding this comment

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

What order are candidates returned in?

Maybe accept a limit (and offset) of the number of candidates to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Candidates are returned in the stored order (by candidate owner address) yeah we could build in a limit, just one more thing to parse for in the path though, which is kind of confusing, it would probably need to use a different symbol such as a dash, ex. subspace-100 not sure I love this idea. I think it's maybe a bit too much complexity that we don't need in the query, you can manually filter after you get the results - also not concerned about limiting so much as per this issue: #975

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, not a priority, agreed we want to avoid ugliness/complexity.

// get the command to query all the candidates bonded to a delegator
func GetCmdQueryDelegatorBonds(storeName string, cdc *wire.Codec) *cobra.Command {
cmd := &cobra.Command{
Use: "delegator-candidates",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, maybe limit-offset (not sure if necessary for bonds) and clarity on sort order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see response above

x/stake/msg.go Outdated
panic(err)
}
return b
cdc := wire.NewCodec()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to create a new codec each call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

struggling with how to use a basic cdc without needing to pass them all around. But, correct good point this can just be defined once here. I just don't like having to many cdc's lying around all over the place - have a solution in mind here though, shouldn't be too bad if we define it globally (with an init function too) right above this message.

Copy link
Contributor

Choose a reason for hiding this comment

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

A singleton for the file (or even just for all calls of this function) seems fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's the plan - I just want the cdc definitions to be as consolidated as possible - it's more just stylistic clarity I'm thinking of

@@ -176,7 +176,16 @@ func (st *iavlStore) Query(req abci.RequestQuery) (res abci.ResponseQuery) {
} else {
_, res.Value = tree.GetVersioned(key, height)
}

case "/subspace":
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to support limit-offset queries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a fan for the time being as per my upper comment - I think once we refine a way to pass in options instead of using the path, then I'm much more warm to this idea.

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

utACK for now, limit/offset maybe to be added in the future (also need to think about DoS vectors, make sure clients can't force expensive store reads).

@cwgoes cwgoes merged commit be975fc into develop May 10, 2018
@cwgoes cwgoes deleted the rigel/range-query branch May 10, 2018 15:33
@cwgoes
Copy link
Contributor

cwgoes commented May 10, 2018

Also ref #979 re: store queries, will be implemented in a separate PR.

@zmanian
Copy link
Member

zmanian commented May 10, 2018

@rigelrozanski is this backwards compatible with the current gaia-5000 testnet? Would love to be able to get this data for the current validator set

@rigelrozanski
Copy link
Contributor Author

unfortunately it's not because of unmarshalling format changes since then

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.

ABCI Range Query
3 participants