R4R: Implement fee distribution RESTful endpoints #3460

alessio · 2019-01-31T03:16:00Z

[@alessio]

Implement RESTful endpoints as per https://cosmos.network/rpc/#/ICS24
Implement delegator's withdraw-all-rewards cli command

[@rigelrozanski ]

Distribution querier endpoints change: "all_delegation_rewards" -> "total_delegation_rewards"
New query endpoint "delegator_validators"

Closes: #3476
Closes: #2358
Closes: #3490

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 · 2019-01-31T03:23:45Z

Codecov Report

Merging #3460 into develop will increase coverage by 1.72%.
The diff coverage is 31.25%.

@@             Coverage Diff             @@
##           develop    #3460      +/-   ##
===========================================
+ Coverage    52.95%   54.67%   +1.72%     
===========================================
  Files          137      139       +2     
  Lines        10746    10375     -371     
===========================================
- Hits          5691     5673      -18     
+ Misses        4705     4343     -362     
- Partials       350      359       +9

faboweb · 2019-01-31T16:05:52Z

Tested:
My account: cosmos1dhcyvz8vltlkstn53al76l6wmzrxv0s02teyzf
Validated to majority validator cosmosvaloper1dhcyvz8vltlkstn53al76l6wmzrxv0s00ld3w6

GET /distribution/parameters seems to work

GET /distribution/pool returns { "community_pool": null }

GET /distribution/delegators/cosmos1dhcyvz8vltlkstn53al76l6wmzrxv0s02teyzf/rewards returns null

GET /distribution/delegators/cosmos1dhcyvz8vltlkstn53al76l6wmzrxv0s02teyzf/rewards/cosmosvaloper1dhcyvz8vltlkstn53al76l6wmzrxv0s00ld3w6 returns null

x/distribution/client/cli/query.go

alessio · 2019-01-31T23:58:45Z

/distribution/pool (GET) is supposed to return val_pool and val_accum, which resemble some sort of validators specific information. I couldn't find any reference in the code other than the following results returned by grep:

alessio@bangalter:~/.../cosmos/cosmos-sdk$ grep -rw accum
CHANGELOG.md:  * [\#3181](https://github.com/cosmos/cosmos-sdk/issues/3181) Correctly reset total accum update height and jailed-validator bond height / unbonding height on export-for-zero-height
CHANGELOG.md: * [\#2573](https://github.com/cosmos/cosmos-sdk/issues/2573) [x/distribution] add accum invariance
CHANGELOG.md:      * [x/distribution] Add sanity checks for incorrect accum / total accum relations
CHANGELOG.md:    * #2573 [x/distribution] add accum invariance
CHANGELOG.md:    * #2573 [x/distribution] accum invariance bugfix
docs/spec/distribution/messages.md:### Update total validator accum
docs/spec/distribution/messages.md:The total amount of validator accum must be calculated in order to determine
docs/spec/distribution/messages.md:block. The accum is always additive to the existing accum. This term is to be
docs/spec/distribution/messages.md:The total amount of delegator accum must be updated in order to determine the
docs/spec/distribution/messages.md:other delegators for that validator. The accum is always additive to
docs/spec/distribution/messages.md:the existing accum. This term is to be updated each time a
docs/spec/distribution/messages.md:    accum = blocks * vdTokens
docs/spec/distribution/messages.md:    withdrawalTokens := g.Pool * accum / g.TotalValAccum 
docs/spec/distribution/messages.md:    accum = delegatorShares * blocks 
docs/spec/distribution/messages.md:    withdrawalTokens := vi.Pool * accum / vi.TotalDelAccum
docs/spec/distribution/messages.md:    vi.TotalDelAccum -= accum
docs/spec/distribution/messages.md:    vi.TotalDelAccum -= accum
docs/spec/distribution/state.md:    TotalValAccumUpdateHeight  int64    // last height which the total validator accum was updated
docs/spec/distribution/state.md:    TotalValAccum              sdk.Dec  // total valdator accum held by validators
docs/spec/distribution/state.md:    TotalDelAccumUpdateHeight  int64    // last height which the total delegator accum was updated
client/lcd/swagger-ui/swagger.yaml:      accum:
client/lcd/swagger-ui/swagger.yaml:      accum:
x/staking/keeper/val_state_change.go:			// This hook is extremely useful, otherwise lazy accum bugs will be difficult to solve.

@cwgoes can you shed some light on this please? Either something is missing here, or frontend ICS24 specs need update. Note that presently /distribution/pool returns the following:

{
  "community_pool": [
    {
      "denom": "stake",
      "amount": "1932.480000000000000000"
    }
  ]
}

alessio · 2019-02-01T06:31:18Z

All GET endpoints are now complete. Please test @cosmos/cosmos-ui.

@faboweb @fedekunze @sabau Note that specs are changed, @cwgoes reviewed and updated them. I'd suggest you regenerate swagger docs and validate endpoints compliance against the new interfaces.

Please keep me posted

alessio · 2019-02-02T03:47:46Z

Progress tracking:

Endpoint	GET	POST
/distribution/delegators/{delegatorAddr}/rewards
/distribution/delegators/{delegatorAddr}/rewards/{validatorAddr}
/distribution/delegators/{delegatorAddr}/withdraw_address
/distribution/validators/{validatorAddr}		N/A
/distribution/validators/{validatorAddr}/rewards
/distribution/parameters		N/A
/distribution/outstanding_rewards		N/A

alessio · 2019-02-02T04:10:05Z

How to test the PR

Get started

$ gaiad init --moniker=[YOUR_MONIKER_HERE]

open your genesis.json, then look up inflation in the mint params:

    "mint": {
      "minter": {
        "inflation": "10000.000000000000000000",

inflation needs to be very high. I tested with 10000.000000000000000000 and rewards popped up pretty quickly.

Now, modify inflation_rate_change and inflation_max according to your inflation:

      "params": {
        "mint_denom": "stake",
        "inflation_rate_change": "10000.000000000000000000",
        "inflation_max": "20000.000000000000000000",

create a key with gaiacli:

$ gaiacli keys add foo

Add foo to genesis.json and give it lots of coins:

$ gaiad add-genesis-account foo 10000000stake   # 10 mln

Generate a gentx. Still, delegate lots of coins:

$ gaiad gentx --name=foo --amount=500000stake   # 0.5 mln

Collect the gentx and start gaiad

$ gaiad collect-gentxs
$ gaiad start

Start the REST server:

$ gaiacli rest-server

sabau · 2019-02-02T17:59:20Z

just to go quicker I scripted the boot process if anyone is interested in playing on the parameters

kickoff.sh

sabau · 2019-02-02T19:15:51Z

I haven't finished all the rounds. Generally the errors and some return values are plain strings so not json parsable.

The behaviour seems to work but I need a bit more time

Regarding the errors (null string or plain text anyway) I think is something not related to this PR but generic.

jackzampolin · 2019-02-02T19:50:32Z

@sabau We are working on better error returns over in this PR: #3451

alessio · 2019-02-02T20:04:23Z

@jackzampolin I think Karoly is talking about JSON error responses. I'll open a PR about it

sabau · 2019-02-02T23:30:22Z

Exaclty, I imagine everything produced by an API to be consumable by a client that interpret only that format (let it be xml, json, ...). If an error occurs it shuold be interpretable too, otherwise every client will have to implement those special cases

sabau · 2019-02-04T09:23:14Z

x/distribution/client/common/common.go

@@ -0,0 +1,143 @@
+package common


nice 👍 common module

ref #3476

alexanderbez

Look great @alessio -- I left a minor comment, but overall it looks solid. The localnet CI task is failing. Also, correct me if I'm wrong, but there aren't any LCD tests, so have all the endpoints been tested?

x/distribution/client/common/common.go

Closes: #3490

sabau · 2019-02-04T19:39:16Z

Would be great if you can add unit tests (GET and POST) for the happy paths at the endpoint:
/distribution/delegators/{delegatorAddr}/rewards/{validatorAddr}

If you are running out of time I can ask @faboweb if I can borrow some Voyager hours to write unit tests to cover the others.

Manually so far so good!

@bez

cc'ing @bez

alessio requested review from faboweb, jackzampolin, cwgoes and fedekunze January 31, 2019 03:16

alessio requested review from ebuchman and rigelrozanski as code owners January 31, 2019 03:16

alessio requested a review from sabau January 31, 2019 04:18

alexanderbez reviewed Jan 31, 2019

View reviewed changes

x/distribution/client/cli/query.go Outdated Show resolved Hide resolved

alexanderbez reviewed Jan 31, 2019

View reviewed changes

x/distribution/client/cli/query.go Outdated Show resolved Hide resolved

alessio force-pushed the alessio/2358-fee-dist-rest branch 2 times, most recently from 51b2b11 to 7e5e0c2 Compare January 31, 2019 23:46

alessio assigned alexanderbez and unassigned alexanderbez Feb 1, 2019

alessio force-pushed the alessio/2358-fee-dist-rest branch from 2ac3c64 to 87c4a0d Compare February 2, 2019 01:10

alessio changed the title ~~WIP: Implement fee distribution RESTful endpoints~~ R4R: Implement fee distribution RESTful endpoints Feb 2, 2019

alessio added the ready-for-review label Feb 2, 2019

alessio force-pushed the alessio/2358-fee-dist-rest branch from d571e69 to 010540f Compare February 2, 2019 03:52

alessio force-pushed the alessio/2358-fee-dist-rest branch from 010540f to 258536b Compare February 2, 2019 22:29

sabau reviewed Feb 4, 2019

View reviewed changes

x/distribution/client/common/common.go

@@ -0,0 +1,143 @@

package common

Copy link

Contributor

sabau Feb 4, 2019

Choose a reason for hiding this comment

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

nice 👍 common module

Alessio Treglia and others added 19 commits February 4, 2019 09:00

Refactor, make new client common sub-package

50bf781

Implement GET /distribution/delegators/{delegatorAddr}/withdraw_address

71d2898

WIP, issue is blocked

effc2e9

Update distribution swagger.yaml section for F1

094aa52

Replace pool with outstanding_rewards

009e927

Finalize query endpoints

fd26e55

Implement POST /delegators/{delegatorAddr}/rewards/{validatorAddr}

be16e5c

Implement /validators/{validatorAddr}/rewards

9e9e665

Factour out logic to prepare multi-msg withdrawal

32811f9

WIP

1d5cf3f

all_delegation_rewards -> total_delegation_rewards

f12c772

rigel/distr-queriers

834dd56

Implement /delegators/{delegatorAddr}/rewards

bbc8f99

Implement cli withdraw-all-rewards command as multi-message tx

bb8767d

ref #3476

Implement /delegators/{delegatorAddr}/withdraw_address

ec34237

invert abort with ok

20826e8

Clean up

4e23cf3

Update PENDING.md

a285f10

Clean up rebase leftover, port to new rest package

0e9504c

alessio force-pushed the alessio/2358-fee-dist-rest branch from 258536b to 0e9504c Compare February 4, 2019 17:23

alexanderbez suggested changes Feb 4, 2019

View reviewed changes

x/distribution/client/common/common.go Outdated Show resolved Hide resolved

Alessio Treglia added 2 commits February 4, 2019 10:53

Don't write errors twice when ReadRESTReq() fails

d18d7eb

Closes: #3490

Update PENDING.md

4291cbe

Alessio Treglia added 2 commits February 4, 2019 12:34

Split up QueryRewards

045d9f4

cc'ing @bez

Add tests

09239a9

alexanderbez approved these changes Feb 5, 2019

View reviewed changes

jackzampolin approved these changes Feb 5, 2019

View reviewed changes

jackzampolin merged commit a2b73c8 into develop Feb 5, 2019

jackzampolin deleted the alessio/2358-fee-dist-rest branch February 5, 2019 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Implement fee distribution RESTful endpoints #3460

R4R: Implement fee distribution RESTful endpoints #3460

alessio commented Jan 31, 2019 •

edited

Loading

codecov bot commented Jan 31, 2019 •

edited

Loading

faboweb commented Jan 31, 2019 •

edited

Loading

alessio commented Jan 31, 2019

alessio commented Feb 1, 2019

alessio commented Feb 2, 2019

alessio commented Feb 2, 2019

sabau commented Feb 2, 2019

sabau commented Feb 2, 2019 •

edited

Loading

jackzampolin commented Feb 2, 2019

alessio commented Feb 2, 2019

sabau commented Feb 2, 2019 •

edited

Loading

sabau Feb 4, 2019

alexanderbez left a comment

sabau commented Feb 4, 2019 •

edited

Loading

R4R: Implement fee distribution RESTful endpoints #3460

R4R: Implement fee distribution RESTful endpoints #3460

Conversation

alessio commented Jan 31, 2019 • edited Loading

codecov bot commented Jan 31, 2019 • edited Loading

Codecov Report

faboweb commented Jan 31, 2019 • edited Loading

alessio commented Jan 31, 2019

alessio commented Feb 1, 2019

alessio commented Feb 2, 2019

alessio commented Feb 2, 2019

How to test the PR

sabau commented Feb 2, 2019

sabau commented Feb 2, 2019 • edited Loading

jackzampolin commented Feb 2, 2019

alessio commented Feb 2, 2019

sabau commented Feb 2, 2019 • edited Loading

sabau Feb 4, 2019

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

sabau commented Feb 4, 2019 • edited Loading

alessio commented Jan 31, 2019 •

edited

Loading

codecov bot commented Jan 31, 2019 •

edited

Loading

faboweb commented Jan 31, 2019 •

edited

Loading

sabau commented Feb 2, 2019 •

edited

Loading

sabau commented Feb 2, 2019 •

edited

Loading

sabau commented Feb 4, 2019 •

edited

Loading