R4R: Redelegation Querier #2559

sunnya97 · 2018-10-23T00:17:33Z

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-23T00:17:50Z

Codecov Report

Merging #2559 into develop will decrease coverage by 0.01%.
The diff coverage is 76.78%.

@@             Coverage Diff             @@
##           develop    #2559      +/-   ##
===========================================
- Coverage    55.05%   55.04%   -0.02%     
===========================================
  Files          133      133              
  Lines         9430     9438       +8     
===========================================
+ Hits          5192     5195       +3     
- Misses        3921     3923       +2     
- Partials       317      320       +3

jackzampolin · 2018-10-23T01:21:56Z

Let's push some of that REALLY LONG URL into query params. I think there has been some conversation on the issue to reflect this.

client/lcd/lcd_test.go

sunnya97 · 2018-10-23T21:08:48Z

I'm going to switch this PR to implement Option 2 from #2182 (comment).

I think it makes the whole thing a lot cleaner.

cwgoes

Minor comments, substance looks fine.

x/stake/keeper/query_utils.go

cwgoes · 2018-11-05T11:14:26Z

x/stake/querier/queryable.go

+
+	var redels []types.Redelegation
+
+	if !params.DelegatorAddr.Empty() && !params.SrcValidatorAddr.Empty() && !params.DstValidatorAddr.Empty() {


Maybe a switch would be easier to read here.

Not sure, it's three different options that are being checked in weird configs

if !option1 && !option2 && !option3 else if option1 && !option2 && option 3 else

How do I write that cleanly in a switch statement? 3 tiered switch statements?

x/stake/client/rest/query.go

alexanderbez · 2018-11-05T15:08:30Z

@jackzampolin I think the endpoint structure was resolved. Any other feedback?

jackzampolin · 2018-11-05T16:05:29Z

Did we get the documentation updated? Doesn't look like the swagger.yaml file got entries added for these new routes yet.

melekes

🍾 🥇 🐎

melekes · 2018-11-07T14:07:17Z

client/lcd/lcd_test.go

+	endpoint := "/stake/redelegations?"
+
+	if !delegatorAddr.Empty() {
+		endpoint += fmt.Sprintf("delegator=%s&", delegatorAddr)


consider using https://golang.org/pkg/strings/#Builder

hmm interesting - yeah seems like it would make the code a bit smaller - but I don't see builders often so might just be more confusing for new devs... but then again maybe we should all learn!

I think in this situation, it is simple enough that we can just use the += for the test. But I agree, in more important places, where we want to reduce memory copying, a String builder is good to use.

x/stake/client/rest/utils.go

faboweb

Let's hold off until our next meeting with this change

fedekunze

/stake/validators/{validatorAddr}/redelegations and /stake/delegators/{validatorAddr}/redelegations should stay the same, and we should only add query params to the latter. Alternativerly we could add query params for outgoing or incoming redelegations to the validator redelegations endpoint.

fedekunze · 2018-11-29T11:06:01Z

client/lcd/lcd_test.go

+	var res *http.Response
+	var body string
+
+	endpoint := "/stake/redelegations?"


I still think this is a terrible design, we should only add the query parameters to /stake/delegators/{delegatorAddr}/redelegations, not changing the current structure. cc: @faboweb

The thing I don't like about /stake/delegators/{delegatorAddr}/redelegations is that it's trying to put a fake structure on data that is not actually structured like in the state. Redelegations are not attributes of a delegator, but rather delegator, srcValidator, and dstValidator are attributes of a redelegation.

I would agree with @fedekunze here. I don't think how we have the data structured internally matters to users. Presenting the data in a way that is logically consistent (delegators have redelegations) is a great idea.

What are /stake/delegators/<>/redelegations vs /stake/validators/<>/redelegations?

I think over-emphasis on the URL design of API endpoints often leads to premature design constraints that end up causing more headache down the line. It is simpler to model the API after what is modeled in the implementation.

REST isn't just about pretty URLs... there's nothing wrong with query parameters, esp given a context where we will want to provide filters etc. I favor Sunny's approach here, it's faster to execute and less brittle going into the future.

The end users here are developers who can deal with query parameters. We're not talking about website URLs, which is a different story.

It is simpler to model the API after what is modeled in the implementation.

I think simple to implement is not what we want with an API. An API should be consistent for quiet some time so developers don't need to adjust to constant changes (this is frustrating from experience). We should come up with a design that reflects the expectation of the developers that want to use the API. Let's invite more of them to stakeholder meetings and discuss together with them the API design.

fedekunze · 2018-11-29T11:06:23Z

client/lcd/lcd_test.go

@@ -1254,17 +1273,6 @@ func getValidatorUnbondingDelegations(t *testing.T, port string, validatorAddr s
 	return ubds
 }

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


this should stay the same, see comment above

fedekunze · 2018-11-29T11:07:00Z

client/lcd/swagger-ui/swagger.yaml

@@ -753,15 +753,25 @@ paths:
          description: Invalid delegator address
        500:
          description: Internal Server Error
-  /stake/delegators/{delegatorAddr}/redelegations:
+  /stake/redelegations:


why would you want to get all the redelegations in the state ? there's no use case for that

Maybe not in Voyager? But that totally seems like something some block explorer might want.

but you can still go through the list of validators and do that

and through the /txs endpoint

and through the /txs endpoint

Either through that or with the GET /stake/validators/{validatorAddr}/redelegations endpoint

cwgoes · 2018-12-12T12:37:59Z

Checking on this PR, I think we had come to the conclusion to leave the endpoints as-is.

@sunnya97 Am I right? Should we dismiss the outstanding reviews and merge this?

cc also @fedekunze @faboweb - we're not trying to make Voyager's life terrible, but we think structuring the REST like you describe will lead to confusion, because that's not how the underlying data is related.

fedekunze · 2018-12-12T15:01:45Z

because that's not how the underlying data is related

That might be the case, but then then most of the endpoints we've implemented and discussed multiple times on #2113 and on the individual issues will fall under the same argument.

jackzampolin · 2018-12-12T16:47:32Z

@cwgoes This is failing build and has merge conflicts so we should get that fixed first too.

cwgoes · 2018-12-12T16:53:07Z

That might be the case, but then then most of the endpoints we've implemented and discussed multiple times on #2113 and on the individual issues will fall under the same argument.

OK. Maybe this merits a dedicated discussion between Voyager / SDK, lest we simply come to opposite conclusions in separate meetings.

alexanderbez · 2018-12-12T16:54:25Z

Something we can discuss next Tuesday @cwgoes ?

discussed on SDK <> Voyager meeting

jackzampolin · 2018-12-13T01:14:38Z

This just needs a rebase then can be merged!

Discussed at SDK-Voyager meeting.

sunnya97 · 2018-12-17T13:42:39Z

That might be the case, but then then most of the endpoints we've implemented and discussed multiple times on #2113 and on the individual issues will fall under the same argument.

Agreed. I personally think we should probably rethink these.

faboweb · 2018-12-17T14:32:27Z

structuring the REST like you describe will lead to confusion, because that's not how the underlying data is related

this is actually the purpose! the API does not reflect the underlying data structure as it serves the purpose of making the data understandable and convenient to use. Imo the API should reflect more the use-cases of the people using it.

fedekunze · 2018-12-18T22:02:32Z

PENDING.md

@@ -16,11 +16,16 @@ BREAKING CHANGES
 FEATURES

 * Gaia REST API (`gaiacli advanced rest-server`)
+    * [gaia-lite] [\#2182] Added LCD endpoint for querying redelegations


This should have been in breaking changes cc: @sunnya97 @jackzampolin. I'll update it

fedekunze · 2018-12-18T22:02:50Z

PENDING.md

@@ -16,11 +16,16 @@ BREAKING CHANGES
 FEATURES

 * Gaia REST API (`gaiacli advanced rest-server`)
+    * [gaia-lite] [\#2182] Added LCD endpoint for querying redelegations
+    * [gov] [\#2479](https://github.com/cosmos/cosmos-sdk/issues/2479) Added governance parameter


not related to this PR

fedekunze · 2018-12-18T22:02:55Z

PENDING.md

@@ -16,11 +16,16 @@ BREAKING CHANGES
 FEATURES

 * Gaia REST API (`gaiacli advanced rest-server`)
+    * [gaia-lite] [\#2182] Added LCD endpoint for querying redelegations
+    * [gov] [\#2479](https://github.com/cosmos/cosmos-sdk/issues/2479) Added governance parameter
+    query REST endpoints.


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

sunnya97 requested review from fedekunze and jackzampolin October 23, 2018 00:17

sunnya97 changed the title ~~WIP: Redelegation Querier~~ R4R: Redelegation Querier Oct 23, 2018

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

sunnya97 assigned cwgoes and fedekunze Oct 23, 2018

jackzampolin requested changes Oct 23, 2018

View reviewed changes

client/lcd/lcd_test.go Outdated Show resolved Hide resolved

sunnya97 force-pushed the sunny/redelegationquerier branch from 9074212 to 163e628 Compare October 23, 2018 20:06

sunnya97 force-pushed the sunny/redelegationquerier branch from 100dcd0 to 011e180 Compare November 4, 2018 06:02

sunnya97 requested a review from jaekwon as a code owner November 4, 2018 06:02

cwgoes suggested changes Nov 5, 2018

View reviewed changes

melekes reviewed Nov 7, 2018

View reviewed changes

sunnya97 added 7 commits November 9, 2018 13:22

added querier redelegation

52325f8

LCD route

da690ce

PENDING

75feb13

use query params

c4ff53b

rebased and fixed bugs

5b5f421

single endpoint

bfe6994

in progress

b60db92

Remove merge artifact from PENDING.md

fa18461

cwgoes approved these changes Nov 29, 2018

View reviewed changes

faboweb previously requested changes Nov 29, 2018

View reviewed changes

fedekunze previously requested changes Nov 29, 2018

View reviewed changes

sunnya97 added 2 commits December 6, 2018 23:09

merged develop

482b664

merge conflicts

16281b4

sunnya97 and others added 2 commits December 17, 2018 21:58

Merge branch 'develop' into sunny/redelegationquerier

93fd680

fixed merge conflicts

5fd59ed

jackzampolin added the will-merge label Dec 17, 2018

jackzampolin merged commit f4fb360 into develop Dec 18, 2018

jackzampolin deleted the sunny/redelegationquerier branch December 18, 2018 21:43

fedekunze reviewed Dec 18, 2018

View reviewed changes


		var redels []types.Redelegation

		if !params.DelegatorAddr.Empty() && !params.SrcValidatorAddr.Empty() && !params.DstValidatorAddr.Empty() {

R4R: Redelegation Querier #2559

R4R: Redelegation Querier #2559

Conversation

sunnya97 commented Oct 23, 2018 • edited Loading

codecov bot commented Oct 23, 2018 • edited Loading

Codecov Report

jackzampolin commented Oct 23, 2018

sunnya97 commented Oct 23, 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

alexanderbez commented Nov 5, 2018

jackzampolin commented Nov 5, 2018

melekes 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

faboweb left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnya97 Nov 30, 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

cwgoes commented Dec 12, 2018 • edited Loading

fedekunze commented Dec 12, 2018 • edited Loading

jackzampolin commented Dec 12, 2018

cwgoes commented Dec 12, 2018

alexanderbez commented Dec 12, 2018

jackzampolin commented Dec 13, 2018

sunnya97 commented Dec 17, 2018

faboweb commented Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnya97 commented Oct 23, 2018 •

edited

Loading

codecov bot commented Oct 23, 2018 •

edited

Loading

sunnya97 commented Oct 23, 2018 •

edited

Loading

sunnya97 Nov 30, 2018 •

edited

Loading

cwgoes commented Dec 12, 2018 •

edited

Loading

fedekunze commented Dec 12, 2018 •

edited

Loading

faboweb commented Dec 17, 2018 •

edited

Loading