R4R: split POST delegations endpoint #2997

fedekunze · 2018-12-04T18:51:47Z

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-12-04T18:58:58Z

Codecov Report

Merging #2997 into develop will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           develop    #2997      +/-   ##
===========================================
- Coverage    52.17%   52.14%   -0.04%     
===========================================
  Files          136      136              
  Lines         9619     9612       -7     
===========================================
- Hits          5019     5012       -7     
- Misses        4263     4264       +1     
+ Partials       337      336       -1

…edekunze/2191-split-post-delegations Rebase

client/lcd/swagger-ui/swagger.yaml

jackzampolin

A couple of comments, and a request for more test coverage.

client/lcd/lcd_test.go

PENDING.md

client/lcd/swagger-ui/swagger.yaml

x/stake/client/rest/tx.go


-			i++
-		}
+func postRedelegationsHandlerFn(cdc *codec.Codec, kb keys.Keybase, cliCtx context.CLIContext) http.HandlerFunc {


x/stake/client/rest/tx.go


-				txBldr = newBldr
-			}
+func postUnbondingDelegationsHandlerFn(cdc *codec.Codec, kb keys.Keybase, cliCtx context.CLIContext) http.HandlerFunc {


jackzampolin

Thanks of fixing that! Looks good otherwise!

cwgoes

I don't understand the PUT / POST distinction (but probably that's just me).

Otherwise tested ACK.

PENDING.md

alexanderbez

Did a partial review and left some comments. I just want to make sure we support simulation and generation only.

x/stake/client/rest/tx.go

Addressed. See comment for support simulation and generation only txs above

x/stake/client/rest/tx.go

alexanderbez · 2018-12-07T13:43:55Z

Willing to give this an approval pending once generate_only and sim is confirmed working 👍

jackzampolin · 2018-12-07T21:35:23Z

Maybe we add tests for those cases @alexanderbez mentioned

…edekunze/2191-split-post-delegations Merge develop

fedekunze · 2018-12-10T13:39:49Z

@alexanderbez updated the PR to test the generate_only

fedekunze · 2018-12-10T13:43:10Z

client/utils/rest.go

-	return nil
+	genOnlyStr := r.FormValue("generate_only")
+	if len(genOnlyStr) > 0 {
+		cliCtx.GenerateOnly, err = strconv.ParseBool(genOnlyStr)


previously we only checked if the generate_only had a value, not that it was actually true or false.
I think this is also the case for simulate. @alessio can you address that on #3056 as well ?

alexanderbez · 2018-12-10T14:23:32Z

x/stake/client/rest/tx.go


-		simulateGas, gas, err := client.ReadGasFlag(baseReq.Gas)
+		cliCtx, err := utils.ReadRESTReq(w, r, cdc, cliCtx, &req)


Ughhh, CLIContext has really evolved. I think we should consider renaming it. Maybe ClientContext?

oh I always thought it was Command Line Interface Context lol

It is! I just think it's more appropriate to call it a ClientContext at this point (e.g. look how it's being used here).

alexanderbez

Changes LGTM -- thanks @fedekunze!

fedekunze · 2018-12-10T17:07:15Z

this is going to have conflicting changes with #3060. cc: @alessio

jackzampolin

Going to get this in once the 0.28 release PRs are merged

jackzampolin · 2018-12-10T22:53:39Z

@fedekunze can you resolve the merge conflicts here?

fedekunze · 2018-12-11T10:36:30Z

updated, please review again

cwgoes

Tested ACK

pending

b55e748

fedekunze added the wip label Dec 4, 2018

fedekunze requested review from cwgoes, ebuchman and rigelrozanski as code owners December 4, 2018 18:51

Federico Kunze added 5 commits December 5, 2018 11:36

Split endpoints and use utils

f3d5279

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

9bea121

…edekunze/2191-split-post-delegations Rebase

fix lcd tests

dcd7ecd

change input msg types

5af8fd6

update and rearrange swagger file

b19d9ed

fedekunze added ready-for-review T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). and removed wip labels Dec 5, 2018

fedekunze changed the title ~~WIP: split POST delegations endpoint~~ R4R: split POST delegations endpoint Dec 5, 2018

fedekunze requested review from alexanderbez, faboweb and jackzampolin December 5, 2018 11:18

fedekunze commented Dec 5, 2018

View reviewed changes

client/lcd/swagger-ui/swagger.yaml Show resolved Hide resolved

jackzampolin reviewed Dec 5, 2018

View reviewed changes

Updated swagger

0ee236a

jackzampolin approved these changes Dec 5, 2018

View reviewed changes

cwgoes approved these changes Dec 6, 2018

View reviewed changes

PENDING.md Outdated Show resolved Hide resolved

Federico Kunze added 2 commits December 6, 2018 15:01

update delegations to use POST

5bd2544

update pending

8b794d2

alexanderbez mentioned this pull request Dec 6, 2018

Delegations REST endpoint require name and password when generate_only is passed #3020

Closed

4 tasks

alexanderbez previously requested changes Dec 6, 2018

View reviewed changes

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

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

x/stake/client/rest/tx.go Show resolved Hide resolved

Update tx.go

9286098

alexanderbez suggested changes Dec 7, 2018

View reviewed changes

x/stake/client/rest/tx.go Show resolved Hide resolved

Federico Kunze added 4 commits December 10, 2018 11:53

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

c4765b2

…edekunze/2191-split-post-delegations Merge develop

fix genOnly REST, closes #3020

3f744f8

fixes based on tests

6fd34a9

fix lint

03771a3

fedekunze requested a review from alessio December 10, 2018 13:39

fedekunze commented Dec 10, 2018

View reviewed changes

alexanderbez reviewed Dec 10, 2018

View reviewed changes

alexanderbez approved these changes Dec 10, 2018

View reviewed changes

jackzampolin added the will-merge label Dec 10, 2018

jackzampolin approved these changes Dec 10, 2018

View reviewed changes

Federico Kunze and others added 3 commits December 11, 2018 11:26

fix conflicts and update functions based on Alessio's PR

e4e537a

fix comments on cli_tests and added simulation update

b4a307b

Update cli_test.go

a90fca5

cwgoes approved these changes Dec 11, 2018

View reviewed changes

cwgoes merged commit 4ecbf0d into develop Dec 11, 2018

cwgoes deleted the fedekunze/2191-split-post-delegations branch December 11, 2018 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: split POST delegations endpoint #2997

R4R: split POST delegations endpoint #2997

fedekunze commented Dec 4, 2018 •

edited

Loading

codecov bot commented Dec 4, 2018 •

edited

Loading

jackzampolin left a comment

This comment was marked as resolved.

This comment was marked as resolved.

jackzampolin left a comment

cwgoes left a comment

alexanderbez left a comment

alexanderbez commented Dec 7, 2018

jackzampolin commented Dec 7, 2018

fedekunze commented Dec 10, 2018

fedekunze Dec 10, 2018 •

edited

Loading

alexanderbez Dec 10, 2018

fedekunze Dec 10, 2018

alexanderbez Dec 10, 2018

alexanderbez left a comment

fedekunze commented Dec 10, 2018 •

edited

Loading

jackzampolin left a comment

jackzampolin commented Dec 10, 2018

fedekunze commented Dec 11, 2018

cwgoes left a comment


		simulateGas, gas, err := client.ReadGasFlag(baseReq.Gas)
		cliCtx, err := utils.ReadRESTReq(w, r, cdc, cliCtx, &req)

R4R: split POST delegations endpoint #2997

R4R: split POST delegations endpoint #2997

Conversation

fedekunze commented Dec 4, 2018 • edited Loading

codecov bot commented Dec 4, 2018 • edited Loading

Codecov Report

jackzampolin left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

jackzampolin left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez commented Dec 7, 2018

jackzampolin commented Dec 7, 2018

fedekunze commented Dec 10, 2018

fedekunze Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez Dec 10, 2018

Choose a reason for hiding this comment

fedekunze Dec 10, 2018

Choose a reason for hiding this comment

alexanderbez Dec 10, 2018

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

fedekunze commented Dec 10, 2018 • edited Loading

jackzampolin left a comment

Choose a reason for hiding this comment

jackzampolin commented Dec 10, 2018

fedekunze commented Dec 11, 2018

cwgoes left a comment

Choose a reason for hiding this comment

fedekunze commented Dec 4, 2018 •

edited

Loading

codecov bot commented Dec 4, 2018 •

edited

Loading

fedekunze Dec 10, 2018 •

edited

Loading

fedekunze commented Dec 10, 2018 •

edited

Loading