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.

Sign up for GitHub

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

Jump to bottom

R4R: Staking Querier pt1 #2249

Merged
merged 64 commits into from
Sep 13, 2018
Merged

R4R: Staking Querier pt1 #2249

merged 64 commits into from
Sep 13, 2018

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented Sep 6, 2018 •
edited
Loading

Split staking querier logic from #2139
Closes #2009 and luniehq/lunie#1133

  • 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)

Federico Kunze and others added 30 commits August 24, 2018 17:22
Cherry picked commits from prev branch
2ca7e61
Added new keepers for querier functionalities
5e285e1
Renaming
ec13fbe
Fixed gov errors and messages
3eacdfa
Added Querier to stake and app
682008f
Update delegation keepers
ceae374
REST Queriers not working
9ba98ab
Fix marshalling error
8bf6b2c
Querier tests working
cd0ca86
Pool and params working
01304c9
sdk.NewCoin for test handler
7889374
Refactor and renaming
d3b6c3e
Update LCD queries and added more tests for queriers
5fb1547
use sdk.NewCoin
8cfc507
Delegator summary query and tests
044b851
Added more tests for keeper
685f05a
Update PENDING.md
6d9c8b7
@fedekunze
Merge branch 'develop' into fedekunze/2009-queriers-staking
3f5ad5f
Update stake rest query
f6aed4e
Format and replaced panics for sdk.Error
b9b04a0
Refactor and addressed comments from Sunny and Aleks
2852b4a
@fedekunze
Merge branch 'develop' into fedekunze/2009-queriers-staking
128deb5
Fixed some of the errors produced by addr type change
e93a937
Fixed remaining errors
80cd207
Updated and fixed lite tests
c83172d
JSON Header and consistency on errors
7f3a42c
Increased cov for genesis
87c1a0f
Added comment for maxRetrieve param in keepers
5ecedad
Comment on DelegationWithoutDec
af757dd
Bech32Validator Keepers
80db853
rigelrozanski
rigelrozanski reviewed Sep 10, 2018
View reviewed changes
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Good stuff, code much cleaner than previous PR. Biggest comment here is we shouldn't allow for retrieve all (besides through the use of special functions for state dumps).

@fedekunze fedekunze dismissed alexanderbez’s stale review September 11, 2018 14:06

Addressed comments

@fedekunze fedekunze mentioned this pull request Sep 11, 2018
Closed
@rigelrozanski
Copy link
Contributor

failing lint

@fedekunze
Copy link
Collaborator Author

@rigelrozanski @cwgoes Fixed linter errors, but I also pulled from develop and test_sim_gaia_fast is failing now...

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Sep 13, 2018 •
edited
Loading

@fedekunze yeah test_sim_gaia_fast is failing on develop not due to this PR

rigelrozanski
rigelrozanski previously requested changes Sep 13, 2018
View reviewed changes
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

My old comments are resolved - export bug was introduced here though (see the comment)

x/stake/keeper/validator.go Outdated Show resolved Hide resolved
x/stake/genesis.go Outdated Show resolved Hide resolved
x/stake/keeper/query_utils.go Show resolved Hide resolved
@fedekunze
Copy link
Collaborator Author

@rigelrozanski @cwgoes addressed comments

alexanderbez
alexanderbez approved these changes Sep 13, 2018
View reviewed changes
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.

Left some general comments, but otherwise utACK 👍

client/lcd/lcd_test.go Show resolved Hide resolved
x/auth/client/rest/sign.go
@@ -22,6 +22,7 @@ type SignBody struct {
AppendSig bool `json:"append_sig"`
}

// nolint: unparam
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do _ context.CLIContext do we still get a lint error?

x/stake/keeper/query_utils.go Show resolved Hide resolved
rigelrozanski
rigelrozanski approved these changes Sep 13, 2018
View reviewed changes
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work!

@rigelrozanski rigelrozanski merged commit d195cc1 into develop Sep 13, 2018
@rigelrozanski rigelrozanski deleted the fedekunze/2009-querier-pt1 branch September 13, 2018 21:23
@fedekunze fedekunze mentioned this pull request Sep 17, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessio alessio alessio left review comments

@rigelrozanski rigelrozanski rigelrozanski approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

Assignees

@fedekunze fedekunze

Labels
C:x/staking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fedekunze @rigelrozanski @alessio @alexanderbez