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

R4R: Staking Gaia-lite (ex LCD) refactor #1880

Merged
merged 53 commits into from
Aug 8, 2018
Merged

Conversation

fedekunze
Copy link
Collaborator

@fedekunze fedekunze commented Jul 30, 2018

These are breaking changes. For a complete list of refactored endpoints please check luniehq/lunie#1010

Left an issue (#1939) to finish the TODOs for a later Voyager sprint.

  • Linked to github-issue with discussion and accepted design
  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
  • Updated cmd/gaia and examples/

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)

@fedekunze fedekunze self-assigned this Jul 30, 2018
@fedekunze fedekunze added C:x/staking lcd T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Jul 30, 2018
Copy link
Contributor

@faboweb faboweb left a comment

Choose a reason for hiding this comment

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

There is a lot of duplicate code. Can we refactor here?

Which endpoint returns the the bond I have with any validator?

@faboweb
Copy link
Contributor

faboweb commented Jul 30, 2018

Idea: If it is still the case, let's resolve rational shares already in the LCD. It is harder to handle those on client side. (Maybe for another PR)

@fedekunze
Copy link
Collaborator Author

@faboweb you can get a delegation with GET /stake/delegators/{addr}/validators/{addr}/txs?type=bond

@faboweb
Copy link
Contributor

faboweb commented Jul 30, 2018

From the structure of the request I would expect it returns all bonding transactions I made to the validator, not the current bond.

@fedekunze fedekunze requested a review from NodeGuy July 30, 2018 09:42
@rigelrozanski
Copy link
Contributor

@ebuchman - want to sync on this one?

@fedekunze
Copy link
Collaborator Author

@rigelrozanski addressed your comments. Are we good to merge now ? 😄

@ebuchman ebuchman mentioned this pull request Aug 8, 2018
6 tasks
actions = append(actions, string(tags.ActionCompleteUnbonding))
actions = append(actions, string(tags.ActionBeginRedelegation))
actions = append(actions, string(tags.ActionCompleteRedelegation))
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a switch statement here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since it doesn't has an expression I thought it was better with if. But I can change it for better legibility

// r.HandleFunc(
// "/stake/delegators/{delegatorAddr}/validators/{validatorAddr}/txs",
// stakingTxsHandlerFn(cliCtx, cdc),
// ).Queries("type", "{type}").Methods("GET")
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete all commented out code in this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

// r.HandleFunc(
// "/stake/delegators/{delegatorAddr}/validators",
// delegatorValidatorHandlerFn(cliCtx, cdc),
// ).Methods("GET")
Copy link
Contributor

Choose a reason for hiding this comment

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

delete all commented out code in this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rigelrozanski This is a TODO. Should we delete it or leave it as commented ? We already have the functionality but I spoke with @faboweb and agreed to leave it for later...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll make an issue for that and delete the comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #1939


// Check if the delegator is bonded or redelegated to the validator
keyDel := stake.GetDelegationKey(delegatorAddr, validatorAccAddr)
// keyRed := stake.GetREDsByDelToValDstIndexKey(delegatorAddr, validatorAccAddr)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh it was an attempt to create another key in order to get incoming redelegations, but Fabo already discovered how to do so without creating it. Will take it out

@cwgoes cwgoes dismissed rigelrozanski’s stale review August 8, 2018 10:23

Comments addressed AFAIK

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Tested ACK

@cwgoes cwgoes merged commit 1da1115 into develop Aug 8, 2018
@cwgoes cwgoes deleted the fedekunze/lcd_refactor branch August 8, 2018 10:38
This was referenced Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/staking T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants