R4R: Redelegation endpoint and Querier #2336

fedekunze · 2018-09-13T23:27:03Z

Add GET redelegation endpoint
Add redelegation querier
refactor query.go to avoid duplicated code

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 #
Reviewed 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-09-14T10:39:14Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@6c9e71b). Click here to learn what that means.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##             develop    #2336   +/-   ##
==========================================
  Coverage           ?   61.66%           
==========================================
  Files              ?      122           
  Lines              ?     7500           
  Branches           ?        0           
==========================================
  Hits               ?     4625           
  Misses             ?     2553           
  Partials           ?      322

cwgoes · 2018-10-02T10:55:13Z

@fedekunze What's the status of this PR?

fedekunze · 2018-10-02T11:10:55Z

@cwgoes is done but I can't test it because of #2339

cwgoes

utACK, let's add the testcase once the upstream issue is fixed, thanks @fedekunze

fedekunze · 2018-10-09T22:07:02Z

note: lint is failing on develop

faboweb · 2018-10-10T16:39:08Z

Do we really need /stake/delegators/%s/redelegations/validator_from/%s/validator_to/%s? In which usecase would you use this query?

fedekunze · 2018-10-10T17:06:04Z

@faboweb the query is to query individual redelegations, same as the existing endpoints to query individual delegations and unbonding delegations

cwgoes · 2018-10-11T07:57:31Z

Merge develop, it will fix the linter. Not sure about test_cover.

faboweb · 2018-10-11T08:40:44Z

the query is to query individual redelegations

yes, but why? in which case do I query for all redelegations between one validator and another validator? maybe for analytics. but there I will probably download bigger data sets and do not use a single query for each request.
I would like to avoid to spam the API with endpoints that will never be used. This makes the API less readable and there is always code attached to those endpoints, wich we should avoid.

…edekunze/2182-redelegation-endpoint Pull

fedekunze · 2018-10-11T16:52:38Z

@faboweb the query redelegation endpoint was initially specified on the Gaia-lite refactor (#2113), I'd suggest to move the discussion there if you want to propose any change

fedekunze · 2018-10-11T19:13:05Z

@alexanderbez You said that maybe the error had to do with the queue ? Cuz if I run the unit tests locally they pass

jackzampolin · 2018-10-11T19:13:34Z

Lets try to get this in for 0.25? cc @cwgoes

cwgoes · 2018-10-11T20:03:08Z

Lets try to get this in for 0.25? cc @cwgoes

Needs test_cover fixed then a review.

jackzampolin · 2018-10-11T20:53:52Z

Is this affected by the non-determinism?

fedekunze · 2018-10-11T20:54:45Z

I should be affected

cwgoes · 2018-10-12T22:19:20Z

@fedekunze I think this test failure is unrelated to the CI nondeterminism, something about redelegation querying.

fedekunze · 2018-10-13T01:02:05Z

@cwgoes I'm getting the same error on other branch as well without the redelegation query:

REQUEST GET http://localhost:41766/auth/accounts/cosmos1vylug6qgf4n984fh5qrj5r9xqe8vagg3yaftgf
--- FAIL: TestBonding (4.06s)
	Error Trace:	lcd_test.go:577
	Error:      	Not equal: 
	            	expected: 40
	            	actual  : 100
	Test:       	TestBonding

alexanderbez · 2018-10-13T02:31:14Z

Yes, I believe the non-determinism is due to the new bonding/unbonding queue logic or at least correlated.

cwgoes · 2018-10-13T06:12:39Z

@cwgoes I'm getting the same error on other branch as well without the redelegation query:

Hmm, I see another error, e.g. https://circleci.com/gh/cosmos/cosmos-sdk/36899#tests/containers/2 -

ok  	github.com/cosmos/cosmos-sdk/x/stake	5.546s	coverage: 82.4% of statements
--- FAIL: TestQueryRedelegation (0.13s)
	Error Trace:	queryable_test.go:334
	Error:      	Should be true
	Test:       	TestQueryRedelegation
FAIL
coverage: 70.8% of statements
FAIL	github.com/cosmos/cosmos-sdk/x/stake/querier	0.773s

…edekunze/2182-redelegation-endpoint Pull

jackzampolin · 2018-10-16T20:26:37Z

Failing test @fedekunze

--- FAIL: TestQueryRedelegation (0.10s)
	Error Trace:	queryable_test.go:334
	Error:      	Should be true
	Test:       	TestQueryRedelegation
FAIL
coverage: 70.8% of statements
FAIL	github.com/cosmos/cosmos-sdk/x/stake/querier	0.665s
Exited with code 1

melekes · 2018-10-17T12:20:04Z

x/stake/client/rest/query.go

-
-		w.Header().Set("Content-Type", "application/json")
+		bech32ValidatorSrc := vars["validatorSrcAddr"]
+		bech32ValidatorDst := vars["validatorDstAddr"]

 		delegatorAddr, err := sdk.AccAddressFromBech32(bech32delegator)


Shouldn't we add

if err != nil { utils.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) return }

after each line? otherwise, err might end up overwritten by the subsequent calls

melekes · 2018-10-17T12:21:08Z

x/stake/client/rest/utils.go

+		bech32delegator := vars["delegatorAddr"]
+		bech32validator := vars["validatorAddr"]
+
+		delegatorAddr, err := sdk.AccAddressFromBech32(bech32delegator)


melekes · 2018-10-17T12:22:13Z

x/stake/client/rest/utils.go

+			return
+		}
+
+		w.Header().Set("Content-Type", "application/json")


I think this should be set per an API handler, not on individual endpoint level, no?

yeah agree, the standarize error responses PR (#2444) refactors all this by using utils.PostProcessResponse(w, cdc, res, cliCtx.Indent) which contains it

fedekunze · 2018-10-19T20:46:03Z

splitting and closing for #2537

Federico Kunze added 2 commits September 14, 2018 00:58

Add redelegation REST endpoint

5281799

Fixed utils and added tests

b8a547d

fedekunze added C:x/staking wip labels Sep 13, 2018

fedekunze self-assigned this Sep 13, 2018

fedekunze requested review from cwgoes, ebuchman and rigelrozanski as code owners September 13, 2018 23:27

Redelegation Querier tests

80427f0

Federico Kunze added 3 commits September 14, 2018 13:02

Fix HumanReadableString

4f1a4ff

Gaia-lite tests

39169ff

fix conflicts

334f998

Federico Kunze added 3 commits October 2, 2018 16:20

add NewQuerier tests

6dddbb0

Changelog, renaming and refactor

fcb1044

Fixed error in rest redelegation endpoint

96b997f

fedekunze changed the title ~~WIP: Add missing redelegation endpoint~~ R4R: Add missing redelegation endpoint Oct 2, 2018

fedekunze added ready-for-review and removed wip labels Oct 2, 2018

fedekunze changed the title ~~R4R: Add missing redelegation endpoint~~ R4R: Redelegation endpoint and Querier Oct 2, 2018

cwgoes approved these changes Oct 3, 2018

View reviewed changes

Merge branch 'develop' into fedekunze/2182-redelegation-endpoint

8bd3210

cwgoes force-pushed the fedekunze/2182-redelegation-endpoint branch from c4cc30f to 8bd3210 Compare October 3, 2018 17:07

Federico Kunze added 2 commits October 3, 2018 20:58

Increased coverage

e0c6811

Merge develop changes

57d71df

fedekunze changed the title ~~R4R: Redelegation endpoint and Querier~~ WIP: Redelegation endpoint and Querier Oct 3, 2018

fedekunze added the wip label Oct 3, 2018

fedekunze removed the wip label Oct 9, 2018

Update PENDING

0129010

Merge branch 'develop' of https://github.com/cosmos/cosmos-sdk into f…

444c4c7

…edekunze/2182-redelegation-endpoint Pull

jackzampolin approved these changes Oct 11, 2018

View reviewed changes

faboweb mentioned this pull request Oct 12, 2018

Implement Gaia-lite LCD Spec #2113

Closed

51 tasks

fedekunze requested a review from alexanderbez October 13, 2018 01:01

Merge branch 'develop' of https://github.com/cosmos/cosmos-sdk into f…

a38cc91

…edekunze/2182-redelegation-endpoint Pull

melekes reviewed Oct 17, 2018

View reviewed changes

fedekunze mentioned this pull request Oct 19, 2018

R4R: Staking REST utils refactor and querier tests (pt 1 redelegation PR) #2537

Merged

6 tasks

fedekunze closed this Oct 19, 2018

fedekunze deleted the fedekunze/2182-redelegation-endpoint branch October 19, 2018 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Redelegation endpoint and Querier #2336

R4R: Redelegation endpoint and Querier #2336

fedekunze commented Sep 13, 2018 •

edited

Loading

codecov bot commented Sep 14, 2018 •

edited

Loading

cwgoes commented Oct 2, 2018

fedekunze commented Oct 2, 2018

cwgoes left a comment

fedekunze commented Oct 9, 2018

faboweb commented Oct 10, 2018

fedekunze commented Oct 10, 2018

cwgoes commented Oct 11, 2018

faboweb commented Oct 11, 2018

fedekunze commented Oct 11, 2018

fedekunze commented Oct 11, 2018

jackzampolin commented Oct 11, 2018

cwgoes commented Oct 11, 2018

jackzampolin commented Oct 11, 2018

fedekunze commented Oct 11, 2018

cwgoes commented Oct 12, 2018

fedekunze commented Oct 13, 2018

alexanderbez commented Oct 13, 2018

cwgoes commented Oct 13, 2018

jackzampolin commented Oct 16, 2018

melekes Oct 17, 2018

melekes Oct 17, 2018

melekes Oct 17, 2018

fedekunze Oct 17, 2018

fedekunze commented Oct 19, 2018

R4R: Redelegation endpoint and Querier #2336

R4R: Redelegation endpoint and Querier #2336

Conversation

fedekunze commented Sep 13, 2018 • edited Loading

codecov bot commented Sep 14, 2018 • edited Loading

Codecov Report

cwgoes commented Oct 2, 2018

fedekunze commented Oct 2, 2018

cwgoes left a comment

Choose a reason for hiding this comment

fedekunze commented Oct 9, 2018

faboweb commented Oct 10, 2018

fedekunze commented Oct 10, 2018

cwgoes commented Oct 11, 2018

faboweb commented Oct 11, 2018

fedekunze commented Oct 11, 2018

fedekunze commented Oct 11, 2018

jackzampolin commented Oct 11, 2018

cwgoes commented Oct 11, 2018

jackzampolin commented Oct 11, 2018

fedekunze commented Oct 11, 2018

cwgoes commented Oct 12, 2018

fedekunze commented Oct 13, 2018

alexanderbez commented Oct 13, 2018

cwgoes commented Oct 13, 2018

jackzampolin commented Oct 16, 2018

melekes Oct 17, 2018

Choose a reason for hiding this comment

melekes Oct 17, 2018

Choose a reason for hiding this comment

melekes Oct 17, 2018

Choose a reason for hiding this comment

fedekunze Oct 17, 2018

Choose a reason for hiding this comment

fedekunze commented Oct 19, 2018

fedekunze commented Sep 13, 2018 •

edited

Loading

codecov bot commented Sep 14, 2018 •

edited

Loading