Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return per-validator rewards when querying delegator all delegations rewards #4101

Merged
merged 13 commits into from
Apr 16, 2019

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Apr 12, 2019

Closes: #3715

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]
  • 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)

@alessio alessio changed the title Implement AC1 and update relevant tests Return per-validator reward when querying delegator total rewards Apr 12, 2019
@alessio alessio changed the title Return per-validator reward when querying delegator total rewards Return per-validator rewards when querying delegator all delegations rewards Apr 12, 2019
@codecov
Copy link

codecov bot commented Apr 12, 2019

Codecov Report

Merging #4101 into develop will increase coverage by 0.08%.
The diff coverage is 40.9%.

@@            Coverage Diff             @@
##           develop   #4101      +/-   ##
==========================================
+ Coverage    60.12%   60.2%   +0.08%     
==========================================
  Files          211     212       +1     
  Lines        15138   15155      +17     
==========================================
+ Hits          9101    9124      +23     
+ Misses        5415    5407       -8     
- Partials       622     624       +2

@alessio alessio force-pushed the 3715-batch-delegator-rewards branch from 1bcc00b to 2319880 Compare April 12, 2019 16:50
@alessio alessio marked this pull request as ready for review April 12, 2019 17:54
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

A few things:

  1. The REST client seems to perform what is defined in the issue, but the CLI is broken:
gaiacli q distr rewards cosmos16vj039cqcqu5er4229yctg2af24nu6es9uemel --chain-id=chain-EVziqe --home ./build/node0/gaiacli

panic: json: cannot unmarshal object into Go value of type []json.RawMessage

goroutine 1 [running]:
github.com/tendermint/go-amino.(*Codec).MustUnmarshalJSON(...)
	/Users/aleksbez/go/pkg/mod/github.com/tendermint/[email protected]/amino.go:445
github.com/cosmos/cosmos-sdk/x/distribution/client/cli.GetCmdQueryDelegatorRewards.func1(0xc000b76280, 0xc000bc8440, 0x1, 0x4, 0x0, 0x0)
	/Users/aleksbez/go/src/github.com/cosmos/cosmos-sdk/x/distribution/client/cli/query.go:161 +0x372
github.com/spf13/cobra.(*Command).execute(0xc000b76280, 0xc000bc8380, 0x4, 0x4, 0xc000b76280, 0xc000bc8380)
	/Users/aleksbez/go/pkg/mod/github.com/spf13/[email protected]/command.go:762 +0x465
github.com/spf13/cobra.(*Command).ExecuteC(0xc0001c8f00, 0x47ba47f, 0xc0001c8f00, 0x4b290c3)
	/Users/aleksbez/go/pkg/mod/github.com/spf13/[email protected]/command.go:852 +0x2ec
github.com/spf13/cobra.(*Command).Execute(...)
	/Users/aleksbez/go/pkg/mod/github.com/spf13/[email protected]/command.go:800
github.com/tendermint/tendermint/libs/cli.Executor.Execute(0xc0001c8f00, 0x4ef28c8, 0x2, 0xc000b0fe00)
	/Users/aleksbez/go/pkg/mod/github.com/tendermint/[email protected]/libs/cli/setup.go:89 +0x3c
main.main()
	/Users/aleksbez/go/src/github.com/cosmos/cosmos-sdk/cmd/gaia/cmd/gaiacli/main.go:110 +0x818
  1. The REST client response should not include the type or value. This is because this response type is registered with Amino and probably shouldn't be.

Result:

{
    "type": "cosmos-sdk/QueryDelegatorTotalRewardsResponse",
    "value": {
        "rewards": [
            {
                "reward": [
                    {
                        "amount": "671.477500000000000000",
                        "denom": "stake"
                    }
                ],
                "validator_address": "cosmosvaloper16vj039cqcqu5er4229yctg2af24nu6esqgdw4v"
            }
        ],
        "total": [
            {
                "amount": "671.477500000000000000",
                "denom": "stake"
            }
        ]
    }
}

Expected:

{
        "rewards": [
            {
                "reward": [
                    {
                        "amount": "671.477500000000000000",
                        "denom": "stake"
                    }
                ],
                "validator_address": "cosmosvaloper16vj039cqcqu5er4229yctg2af24nu6esqgdw4v"
            }
        ],
        "total": [
            {
                "amount": "671.477500000000000000",
                "denom": "stake"
            }
        ]
    }

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

@alessio almost there! The REST response looks good and the CLI (text) response looks good, but the CLI JSON response still has the type and value keys.

eg.

$ gaiacli q distr rewards cosmos17pfgtmlfrvjk4mty6mdw9jf0hjdu355uwwp25t --chain-id=chain-MKV85i --home ./build/node0/gaiacli -o json | pj

{
    "type": "cosmos-sdk/QueryDelegatorTotalRewardsResponse",
    "value": {
        "rewards": [
            {
                "validator_address": "cosmosvaloper17pfgtmlfrvjk4mty6mdw9jf0hjdu355ut64lcc",
                "reward": [
                    {
                        "denom": "stake",
                        "amount": "99.425000000000000000"
                    }
                ]
            }
        ],
        "total": [
            {
                "denom": "stake",
                "amount": "99.425000000000000000"
            }
        ]
    }
}

Why do we need to register the types? Can't they just be simply JSON encoded and decoded?

x/distribution/types/codec.go Outdated Show resolved Hide resolved
x/distribution/client/rest/query.go Outdated Show resolved Hide resolved
@alessio alessio force-pushed the 3715-batch-delegator-rewards branch from 6280548 to 9521da4 Compare April 16, 2019 13:11
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

TestedACK.

@alexanderbez alexanderbez merged commit 7693708 into develop Apr 16, 2019
@alexanderbez alexanderbez deleted the 3715-batch-delegator-rewards branch April 16, 2019 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:x/distribution distribution module related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch delegator rewards
3 participants