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: Implement slashing period #2122

Merged
merged 34 commits into from
Sep 1, 2018
Merged

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Aug 22, 2018

Implements #2001.
Depends on #2120.
Replaces #2121.
Closes #1990.

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

@codecov
Copy link

codecov bot commented Aug 22, 2018

Codecov Report

Merging #2122 into develop will increase coverage by 0.09%.
The diff coverage is 77.57%.

@@            Coverage Diff             @@
##           develop   #2122      +/-   ##
==========================================
+ Coverage    63.51%   63.6%   +0.09%     
==========================================
  Files          136     139       +3     
  Lines         8370    8454      +84     
==========================================
+ Hits          5316    5377      +61     
- Misses        2692    2713      +21     
- Partials       362     364       +2

@cwgoes
Copy link
Contributor Author

cwgoes commented Aug 24, 2018

Thinking about the best way to implement the slashing period hooks - do we have way for two keepers to mutually reference each other? Ideally the slashing period logic would live in the slashing module, and the staking module would call into it as described in the spec.

Certainly we can do this with common types defined in types/ and something like:

a := newStakingKeeper(...)
b := newSlashingKeeper(a, ....)
a.setSlashingKeeper(b)

but that's a bit ugly, and I wonder if we have any concerns with this pattern.

cc @rigelrozanski

@cwgoes
Copy link
Contributor Author

cwgoes commented Aug 24, 2018

Also, I think this is probably about the right time to split slashing into subpackages as per #1779 - but I'm going to do it in a separate PR, after the functionality changes, to make review easier.

@rigelrozanski
Copy link
Contributor

I think the code pattern you've described makes sense - especially so if a takes a an interface version of b

Certainly we can do this with common types defined in types/ and something like

ideally the staking module actually defines what interface it needs for b even though the core implementation happens in a different module (staking module should probably implement a dummy slashing keeper for staking tests in that situation).

Yeah splitting up as a second PR makes sense

@cwgoes
Copy link
Contributor Author

cwgoes commented Aug 27, 2018

This is not being written for Tendermint NextValSet, that will happen in a separate PR. Note that slashing period heights will need to be offset.

@cwgoes cwgoes force-pushed the cwgoes/implement-slashing-period branch from a99363c to 3534a99 Compare August 27, 2018 17:46
@cwgoes cwgoes force-pushed the cwgoes/implement-slashing-period branch from f3b0070 to 6ed552a Compare August 27, 2018 18:21
@cwgoes cwgoes changed the title WIP: Implement slashing period R4R: Implement slashing period Aug 27, 2018
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.

hello!

types/stake.go Outdated Show resolved Hide resolved
x/slashing/hooks.go Show resolved Hide resolved
x/slashing/hooks.go Outdated Show resolved Hide resolved
x/slashing/slashing_period.go Outdated Show resolved Hide resolved
x/slashing/hooks.go Outdated Show resolved Hide resolved
x/slashing/keys.go Outdated Show resolved Hide resolved
x/slashing/keys.go Outdated Show resolved Hide resolved
x/slashing/keys.go Outdated Show resolved Hide resolved
x/slashing/slashing_period.go Outdated Show resolved Hide resolved
x/slashing/slashing_period.go Outdated Show resolved Hide resolved
EndHeight: 0,
SlashedSoFar: sdk.ZeroDec(),
}
k.setValidatorSlashingPeriod(ctx, slashingPeriod)
Copy link
Contributor

Choose a reason for hiding this comment

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

dummy question: is it only possible to have just one slashing period per keeper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's possible to have one per validator per start height - the key is partially read from the ValidatorSlashingPeriod struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks. It's just the method name IMHO implies that there's only one period. addValidatorSlashingPeriod would make more sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, changed to addOrUpdateValidatorSlashingPeriod

@rigelrozanski
Copy link
Contributor

@cwgoes responded to all your comments from my review above ^

I've marked everything which I'm satisfied with as "resolved" (new github feature) everything that is not marked as resolved (including outdated comments) is worth a further comment

@cwgoes
Copy link
Contributor Author

cwgoes commented Aug 31, 2018

Updated for #2040 which is an unfortunate confusion of concerns but there's no way around it.

@rigelrozanski rigelrozanski merged commit 1204857 into develop Sep 1, 2018
@rigelrozanski rigelrozanski deleted the cwgoes/implement-slashing-period branch September 1, 2018 00:01
@rigelrozanski
Copy link
Contributor

rigelrozanski commented Sep 1, 2018

gah! attack of the untested merge! This merge broke develop working on a fix right now. It looks like it didn't have the most recent recent develop merged into it :(

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