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: Add slashing genesis validation #3158

Merged
merged 4 commits into from
Dec 19, 2018

Conversation

jackzampolin
Copy link
Member


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)

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.

@jackzampolin you need to update the simulated random genesis parameters for this I presume (see the x/auth param store PR as an example).

@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #3158 into develop will decrease coverage by 0.12%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3158      +/-   ##
===========================================
- Coverage    55.05%   54.93%   -0.13%     
===========================================
  Files          133      133              
  Lines         9438     9459      +21     
===========================================
  Hits          5196     5196              
- Misses        3922     3943      +21     
  Partials       320      320

@jackzampolin
Copy link
Member Author

@alexanderbez You are right! Only saw these errors when I pushed to CI. Don't think I was running the right sims locally. I've fixed the issue.

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.

One minor change I think, but otherwise LGTM. Also warrants a review from @cwgoes and/or @rigelrozanski on the actual validation of params (are they stated in spec anywhere?).

@@ -167,6 +167,10 @@ func appStateFn(r *rand.Rand, accs []simulation.Account) json.RawMessage {
return appState
}

func between(r *rand.Rand, min, max int) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call this randIntRange.

Copy link
Contributor

Choose a reason for hiding this comment

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

or at least randIntBetween

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.

Changes LGTM, parameter ranges seem reasonable, agreed on renaming between.

@@ -167,6 +167,10 @@ func appStateFn(r *rand.Rand, accs []simulation.Account) json.RawMessage {
return appState
}

func between(r *rand.Rand, min, max int) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

or at least randIntBetween

@jackzampolin jackzampolin merged commit 4b4a2b8 into develop Dec 19, 2018
@jackzampolin jackzampolin deleted the jack/validate-slashing-genesis branch December 19, 2018 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants