R4R: Querier - Get all delegations to validator #2565

sunnya97 · 2018-10-23T08:26:34Z

closes #2027

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 entries in PENDING.md with issue #
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 · 2018-10-23T08:41:04Z

Codecov Report

Merging #2565 into develop will increase coverage by 0.09%.
The diff coverage is 87.09%.

@@             Coverage Diff             @@
##           develop    #2565      +/-   ##
===========================================
+ Coverage    56.65%   56.75%   +0.09%     
===========================================
  Files          156      156              
  Lines         9783     9814      +31     
===========================================
+ Hits          5543     5570      +27     
- Misses        3863     3865       +2     
- Partials       377      379       +2

fedekunze

Thanks @sunnya97 ! let's add the command to the docs so we don't forget later on. Also the single redelegation query is still under discussion so I'd leave it for another PR

x/stake/querier/queryable.go

x/stake/client/cli/query.go

x/stake/keeper/delegation.go

x/stake/querier/queryable_test.go

alexanderbez

Thanks @sunnya97 -- left some minor feedback 👍

cmd/gaia/cmd/gaiacli/main.go

x/stake/client/cli/query.go

x/stake/querier/queryable.go

x/stake/keeper/delegation.go

sunnya97 · 2018-10-23T19:28:04Z

Oops, I probably accidentally branched off my redelegations PR instead of develop. Let's get that one merged to develop first, then I'll rebase this off of develop and fix any conflicts.

fedekunze · 2018-10-23T20:13:18Z

I’d instead remove every line that has to do with redelegation to avoid blocking this PR from being merged

fedekunze

utACK. Need to resolve/address @alexanderbez's renaming suggestions

fedekunze · 2018-10-24T08:41:38Z

@sunnya97 can you add the cmd to docs/sdk/clients.md ? (see 905718f for reference).

Also it'd be cool if you could add a test case for keeper.GetValidatorDelegations to increase coverage

fedekunze · 2018-10-30T12:14:51Z

@sunnya97 status on this ?

alexanderbez

utACK.

cwgoes

utACK - leaving to @jaekwon to merge

x/stake/client/rest/query.go

x/stake/keeper/delegation_test.go

Co-Authored-By: sunnya97 <sunnya97@gmail.com>

rigelrozanski

So the code looks good (few minor comments)
but I am wondering why we want this function at all. It's an extra query which might be nice to have I suppose however I can't see any protocol reason as to why we would ever need to know the list of all delegators for a validator, which was why this was never included to begin with. So I mean I'm okay with the idea of having it, but I want us to really consider whether or not we need this code, and maybe drop it if it doesn't seem absolutely necessary to have

x/stake/client/cli/query.go

rigelrozanski · 2018-11-06T14:47:23Z

x/stake/keeper/delegation.go

+func (k Keeper) GetValidatorDelegations(ctx sdk.Context, valAddr sdk.ValAddress) (delegations []types.Delegation) {
+	store := ctx.KVStore(k.storeKey)
+	iterator := sdk.KVStorePrefixIterator(store, DelegationKey)
+	defer iterator.Close()


because this is such a simple function, we should probably just close the iterator after the for loop and avoid using a defer call

rigelrozanski · 2018-11-06T14:48:20Z

x/stake/querier/queryable.go

+
+	res, errRes = codec.MarshalJSONIndent(cdc, delegations)
+	if errRes != nil {
+		return nil, sdk.ErrInternal(sdk.AppendMsgToErr("could not marshal result to JSON", errRes.Error()))


split this line up?

x/stake/querier/queryable.go

melekes

🍫 🍵

melekes · 2018-11-07T11:55:05Z

client/lcd/lcd_test.go

@@ -1206,6 +1210,17 @@ func getValidator(t *testing.T, port string, validatorAddr sdk.ValAddress) stake
 	return validator
 }

+func getValidatorDelegations(t *testing.T, port string, validatorAddr sdk.ValAddress) []stake.Delegation {
+	res, body := Request(t, port, "GET", fmt.Sprintf("/stake/validators/%s/delegations", validatorAddr.String()), nil)


I feel like there should be a helper function like StakeValidatorsDelegationsPath(validatorAddr.String())

https://guides.rubyonrails.org/routing.html#path-and-url-helpers

In Ruby, you can use magic to create such functions on the fly. In Go, we'd need to write them ourselves. Still, there may be a benefit in having them.

sunnya97 · 2018-11-08T17:52:43Z

@rigelrozanski This seems like a very useful query for a validator to know what delegations there are to him. Maybe they want to build a UI interface specifically for their delegators?

fedekunze · 2018-11-08T20:47:19Z

@rigelrozanski it'll be useful to have this endpoint. Check this use case from voyager: https://github.com/cosmos/voyager/issues/1015

jackzampolin · 2018-11-12T18:57:46Z

We need a swagger.yaml update to add the new route here @sunnya97

…/github.com/cosmos/cosmos-sdk into sunny/get_all_delegations_validator_querier Merge

sunnya97 requested review from cwgoes, ebuchman and rigelrozanski as code owners October 23, 2018 08:26

sunnya97 requested a review from fedekunze October 23, 2018 08:26

sunnya97 assigned rigelrozanski, cwgoes and fedekunze Oct 23, 2018

sunnya97 changed the title ~~WIP: Querier - Get all delegations to validator~~ R4R: Querier - Get all delegations to validator Oct 23, 2018

sunnya97 added C:x/staking lcd C:CLI labels Oct 23, 2018

fedekunze suggested changes Oct 23, 2018

View reviewed changes

alexanderbez reviewed Oct 23, 2018

View reviewed changes

cmd/gaia/cmd/gaiacli/main.go Outdated Show resolved Hide resolved

x/stake/client/cli/query.go Show resolved Hide resolved

x/stake/querier/queryable.go Outdated Show resolved Hide resolved

x/stake/keeper/delegation.go Outdated Show resolved Hide resolved

fedekunze approved these changes Oct 23, 2018

View reviewed changes

fedekunze mentioned this pull request Oct 23, 2018

R4R: Cmds for validator unbondings and redelegations #2569

Merged

6 tasks

sunnya97 force-pushed the sunny/get_all_delegations_validator_querier branch from 8cf3e23 to 9961f64 Compare October 23, 2018 20:42

fedekunze requested a review from jackzampolin October 30, 2018 12:15

sunnya97 requested review from jaekwon and zramsay as code owners November 4, 2018 05:53

sunnya97 added 4 commits November 3, 2018 22:53

added querier redelegation

d378db3

added validatorDelegations querier endpoint

9c6ef3e

LCD and CLI

2d42973

cli fixes

f8b030a

sunnya97 force-pushed the sunny/get_all_delegations_validator_querier branch from 0d76f0f to 4c3ac3f Compare November 4, 2018 05:58

alexanderbez approved these changes Nov 4, 2018

View reviewed changes

cwgoes approved these changes Nov 4, 2018

View reviewed changes

Merge branch 'develop' into sunny/get_all_delegations_validator_querier

dfddac3

fedekunze reviewed Nov 5, 2018

View reviewed changes

x/stake/client/rest/query.go Outdated Show resolved Hide resolved

x/stake/keeper/delegation_test.go Outdated Show resolved Hide resolved

fedekunze and others added 2 commits November 5, 2018 16:42

Update x/stake/client/rest/query.go

080a781

Co-Authored-By: sunnya97 <sunnya97@gmail.com>

addressed fede's comment

cc652a3

rigelrozanski reviewed Nov 6, 2018

View reviewed changes

melekes reviewed Nov 7, 2018

View reviewed changes

jackzampolin approved these changes Nov 9, 2018

View reviewed changes

jackzampolin added will-merge and removed ready-for-review labels Nov 9, 2018

Merge branch 'develop' into sunny/get_all_delegations_validator_querier

4d1246d

jackzampolin mentioned this pull request Nov 9, 2018

Game of Stakes / gaia-9002 release checklist #2754

Closed

27 tasks

update swagger.yaml

182f111

fedekunze mentioned this pull request Nov 12, 2018

remove 'err' names from function return arguments in queriers #2788

Closed

jackzampolin and others added 6 commits November 12, 2018 14:09

Merge branch 'develop' into sunny/get_all_delegations_validator_querier

b255a7d

use newQuery...Params

7dfeb7d

Merge branch 'sunny/get_all_delegations_validator_querier' of https:/…

a6af76e

…/github.com/cosmos/cosmos-sdk into sunny/get_all_delegations_validator_querier Merge

Merge branch 'develop' into sunny/get_all_delegations_validator_querier

695395a

add back in CLI command after rebase

4ebe99b

Fix CLI tests

f38f57d

jackzampolin merged commit 23e8415 into develop Nov 13, 2018

jackzampolin deleted the sunny/get_all_delegations_validator_querier branch November 13, 2018 19:42

abelliumnt mentioned this pull request Dec 5, 2018

R4R: Add unit tests and add valueable PRs from cosmos-sdk irisnet/irishub#758

Merged

snyk-bot mentioned this pull request Jul 6, 2021

[Snyk] Upgrade @vuepress/plugin-google-analytics from 1.3.1 to 1.8.2 terrorizer1980/cosmos-sdk#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Querier - Get all delegations to validator #2565

R4R: Querier - Get all delegations to validator #2565

sunnya97 commented Oct 23, 2018 •

edited by fedekunze

Loading

codecov bot commented Oct 23, 2018 •

edited

Loading

fedekunze left a comment

alexanderbez left a comment

sunnya97 commented Oct 23, 2018 •

edited

Loading

fedekunze commented Oct 23, 2018

fedekunze left a comment

fedekunze commented Oct 24, 2018 •

edited

Loading

fedekunze commented Oct 30, 2018

alexanderbez left a comment

cwgoes left a comment

rigelrozanski left a comment

rigelrozanski Nov 6, 2018

rigelrozanski Nov 6, 2018

melekes left a comment

melekes Nov 7, 2018

sunnya97 commented Nov 8, 2018 •

edited

Loading

fedekunze commented Nov 8, 2018

jackzampolin commented Nov 12, 2018

R4R: Querier - Get all delegations to validator #2565

R4R: Querier - Get all delegations to validator #2565

Conversation

sunnya97 commented Oct 23, 2018 • edited by fedekunze Loading

codecov bot commented Oct 23, 2018 • edited Loading

Codecov Report

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

sunnya97 commented Oct 23, 2018 • edited Loading

fedekunze commented Oct 23, 2018

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze commented Oct 24, 2018 • edited Loading

fedekunze commented Oct 30, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

rigelrozanski left a comment

Choose a reason for hiding this comment

rigelrozanski Nov 6, 2018

Choose a reason for hiding this comment

rigelrozanski Nov 6, 2018

Choose a reason for hiding this comment

melekes left a comment

Choose a reason for hiding this comment

melekes Nov 7, 2018

Choose a reason for hiding this comment

sunnya97 commented Nov 8, 2018 • edited Loading

fedekunze commented Nov 8, 2018

jackzampolin commented Nov 12, 2018

sunnya97 commented Oct 23, 2018 •

edited by fedekunze

Loading

codecov bot commented Oct 23, 2018 •

edited

Loading

sunnya97 commented Oct 23, 2018 •

edited

Loading

fedekunze commented Oct 24, 2018 •

edited

Loading

sunnya97 commented Nov 8, 2018 •

edited

Loading