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

Slashing bugfixes (start height, handler registration) #1200

Merged
merged 7 commits into from
Jun 12, 2018

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Jun 11, 2018

Closes #1199
Closes #1196

Can be cherry-picked for a testnet hotfix. This PR does change the state machine.

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated Basecoin / other examples
  • Squashed related commits and prefixed with PR number per coding standards

@cwgoes cwgoes requested a review from ebuchman as a code owner June 11, 2018 00:57
@codecov
Copy link

codecov bot commented Jun 11, 2018

Codecov Report

Merging #1200 into develop will increase coverage by 0.23%.
The diff coverage is 73.33%.

@@             Coverage Diff             @@
##           develop    #1200      +/-   ##
===========================================
+ Coverage    65.23%   65.46%   +0.23%     
===========================================
  Files           87       87              
  Lines         4415     4428      +13     
===========================================
+ Hits          2880     2899      +19     
+ Misses        1356     1350       -6     
  Partials       179      179

@cwgoes cwgoes changed the title WIP: Slashing bugfixes (start height, handler registration) Slashing bugfixes (start height, handler registration) Jun 11, 2018
@cwgoes cwgoes requested a review from rigelrozanski June 11, 2018 01:17
@kidinamoto01
Copy link
Contributor

so we found out the root cause of #1197?

@cwgoes
Copy link
Contributor Author

cwgoes commented Jun 11, 2018

so we found out the root cause of #1197?

Not yet, this PR fixes two issues which are (as far as I know) both unrelated to #1197.

@@ -129,3 +129,36 @@ func TestHandleAbsentValidator(t *testing.T) {
validator, _ = sk.GetValidatorByPubKey(ctx, val)
require.Equal(t, sdk.Unbonded, validator.GetStatus())
}

func TestHandleNewValidator(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a small description (couple sentences) explaining the intention of this test... looks cool, I'd like to better understand the scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a few-line description to all tests in this file.

if !found {
// If this validator has never been seen before, set the start height
signInfo.StartHeight = height
}
Copy link
Contributor

@rigelrozanski rigelrozanski Jun 11, 2018

Choose a reason for hiding this comment

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

I tend to think this isn't the best practice - it would be more clear to have a NewSignInfo function where you pass in the height and we explicitly define signInfo with this function if !found - this way we can remove that second comment there that rationalizes what is happening - it will just be implied by the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed - that's clearer - changed to NewValidatorSigningInfo.

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.

Looks good just a couple minor comments before merge

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.

Looks good just a couple minor comments before merge

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.

woot!

@rigelrozanski rigelrozanski merged commit dc62279 into develop Jun 12, 2018
@rigelrozanski rigelrozanski deleted the cwgoes/slashing-bugfixes branch June 12, 2018 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants