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: Validator unbonding state #2163

Merged
merged 18 commits into from
Sep 1, 2018
Merged

R4R: Validator unbonding state #2163

merged 18 commits into from
Sep 1, 2018

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Aug 27, 2018

Closes #1676
Closes #1877

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

@rigelrozanski rigelrozanski changed the title Validator unbonding state WIP: Validator unbonding state Aug 27, 2018
@rigelrozanski rigelrozanski changed the title WIP: Validator unbonding state R4R: Validator unbonding state Aug 29, 2018
@codecov
Copy link

codecov bot commented Aug 30, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@1204857). Click here to learn what that means.
The diff coverage is 68.62%.

@@            Coverage Diff             @@
##             develop    #2163   +/-   ##
==========================================
  Coverage           ?   64.18%           
==========================================
  Files              ?      140           
  Lines              ?     8567           
  Branches           ?        0           
==========================================
  Hits               ?     5499           
  Misses             ?     2692           
  Partials           ?      376

@@ -423,6 +418,19 @@ func (v Validator) BondedTokens() sdk.Dec {
return sdk.ZeroDec()
}

// Returns if the validator should be considered unbonded
func (v Validator) IsUnbonded(ctx sdk.Context) bool {
if v.Status == sdk.Unbonded {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not switch here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

++

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh ic switch on the status. Okay yup ++

@@ -134,12 +143,12 @@ func (k Keeper) Unjail(ctx sdk.Context, pubkey crypto.PubKey) {
}

// set the jailed flag on a validator
func (k Keeper) setJailed(ctx sdk.Context, pubkey crypto.PubKey, jailed bool) {
func (k Keeper) setJailed(ctx sdk.Context, pubkey crypto.PubKey, isJailed bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(out of curiosity) why did you decide to rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I first read this it confused me a bit, had to read it twice before it clicked - so I figured it'd probably confuse others at some point too. I think the word jailed might be overloaded just a tiny bit - I figured isJailed is just a little bit more explicit. Very minor though.

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.

On the surface, looks good. I'll want to play around with this a bit too.

@@ -423,6 +418,19 @@ func (v Validator) BondedTokens() sdk.Dec {
return sdk.ZeroDec()
}

// Returns if the validator should be considered unbonded
func (v Validator) IsUnbonded(ctx sdk.Context) bool {
if v.Status == sdk.Unbonded {
Copy link
Contributor

Choose a reason for hiding this comment

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

++

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.

utACK -- minor question, does a validator go from unbonding to unbonded after UnbondingMinTime automatically? Or is IsUnbonded meant to be the only method used for determining that status.

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.

utACK pending merge conflict resolution - and we should merge develop first, ensure all changes from #2040 are reflected

@@ -45,6 +45,7 @@ func SupplyInvariants(ck bank.Keeper, k stake.Keeper, am auth.AccountMapper) sim
case sdk.Bonded:
bonded = bonded.Add(validator.GetPower())
case sdk.Unbonding:
loose = loose.Add(validator.GetTokens().RoundInt())
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 for updating the invariants!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:)

@rigelrozanski
Copy link
Contributor Author

Making required updates now

@rigelrozanski
Copy link
Contributor Author

@cwgoes corrections for the new develop have been made

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.

utACK

@cwgoes cwgoes merged commit d214952 into develop Sep 1, 2018
@cwgoes cwgoes deleted the rigel/validator-unbonding branch September 1, 2018 21:15
chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants