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

Crisis Module Manager Fixes #4854

Merged
merged 5 commits into from
Aug 6, 2019
Merged

Crisis Module Manager Fixes #4854

merged 5 commits into from
Aug 6, 2019

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Aug 6, 2019

  • Call the crisis module's end blocker in SimApp
  • Fix invariant registration bug by passing the crisis module by reference
  • Cleanup godocs and misc

closes: #4803


  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [-t <tag>] [-m <msg>]

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

x/crisis/module.go Outdated Show resolved Hide resolved
@alexanderbez alexanderbez marked this pull request as ready for review August 6, 2019 14:41
@alexanderbez alexanderbez added R4R and removed WIP labels Aug 6, 2019
@alexanderbez alexanderbez changed the title Crisis Module Fixes Crisis Module Manager Fixes Aug 6, 2019
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

x/crisis/internal/keeper/keeper.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

alexanderbez commented Aug 6, 2019

with regards to test_sim_import_export failing, the only difference this PR introduces is that we now actually run invariants during InitGenesis (before we weren't). So sim runs, state is exported, state is than imported, finally InitGenesis gets called and all the invariants execute upon which the same one always fails (from x/distributions InitGenesis):

panic: calculated final stake for delegator cosmos12h8cwerpnvwjfsgw5cehrpdghtg2at7ctkac50 greater than current stake
	final stake:	35411088.000000000000000000
	current stake:	34565140.560069448745397772

Interesting enough, regardless of the sim seed, final stake is always truncated to zero.

/cc @rigelrozanski

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.

ACK - great work on finding the solution @alexanderbez !

@codecov
Copy link

codecov bot commented Aug 6, 2019

Codecov Report

Merging #4854 into master will increase coverage by 0.04%.
The diff coverage is 57.89%.

@@            Coverage Diff            @@
##           master   #4854      +/-   ##
=========================================
+ Coverage   54.25%   54.3%   +0.04%     
=========================================
  Files         269     271       +2     
  Lines       17302   17344      +42     
=========================================
+ Hits         9388    9418      +30     
- Misses       7216    7228      +12     
  Partials      698     698

@alexanderbez alexanderbez merged commit c8ee82b into master Aug 6, 2019
@alexanderbez alexanderbez deleted the bez/4803-fix-invar-check branch August 6, 2019 19:12
alexanderbez added a commit that referenced this pull request Aug 6, 2019
alexanderbez added a commit that referenced this pull request Aug 6, 2019
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
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.

Invariants check not registered to module manager as expected
4 participants