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

Rename revoke(d) to jail(ed) #2120

Merged
merged 1 commit into from
Aug 22, 2018
Merged

Rename revoke(d) to jail(ed) #2120

merged 1 commit into from
Aug 22, 2018

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Aug 22, 2018

Closes #1305

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

@cwgoes cwgoes added ready-for-review T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Aug 22, 2018
@@ -44,7 +44,7 @@ func (v Validator) GetDelegatorShares() sdk.Dec {
}

// Implements sdk.Validator
func (v Validator) GetRevoked() bool {
func (v Validator) GetJailed() bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd change it to IsJailed()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the pattern is GetXYZ() with all the other functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't disagree with this particular one but it's probably better not to be inconsistent with the rest

@@ -37,7 +37,7 @@ func (b BondStatus) Equal(b2 BondStatus) bool {

// validator for a delegated proof of stake system
type Validator interface {
GetRevoked() bool // whether the validator is revoked
GetJailed() bool // whether the validator is jailed
Copy link
Collaborator

Choose a reason for hiding this comment

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

IsJailed()


// Validator must exist
validator := k.validatorSet.Validator(ctx, msg.ValidatorAddr)
if validator == nil {
return ErrNoValidatorForAddress(k.codespace).Result()
}

if !validator.GetRevoked() {
return ErrValidatorNotRevoked(k.codespace).Result()
if !validator.GetJailed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IsJailed()

@@ -113,16 +113,16 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p
minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx)
if height > minHeight && signInfo.SignedBlocksCounter < k.MinSignedPerWindow(ctx) {
validator := k.validatorSet.ValidatorByPubKey(ctx, pubkey)
if validator != nil && !validator.GetRevoked() {
// Downtime confirmed, slash, revoke, and jail the validator
if validator != nil && !validator.GetJailed() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IsJailed()

@cwgoes cwgoes mentioned this pull request Aug 22, 2018
5 tasks
// unrevoke to measure power
sk.Unrevoke(ctx, val)
// should be jailed
require.True(t, sk.Validator(ctx, addr).GetJailed())
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

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

@cwgoes cwgoes merged commit 17f234b into develop Aug 22, 2018
@cwgoes cwgoes deleted the cwgoes/revoke-to-jail branch August 22, 2018 16:09
@cwgoes cwgoes mentioned this pull request Aug 22, 2018
5 tasks
@jackzampolin jackzampolin mentioned this pull request Aug 22, 2018
51 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

2 participants