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: Simulation Refactor #3819

Merged
merged 21 commits into from
Mar 14, 2019
Merged

R4R: Simulation Refactor #3819

merged 21 commits into from
Mar 14, 2019

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Mar 7, 2019

replaces #3744

  • simulations saved in ~/.gaiad/simulations/ (with smaller name only the time, without spaces)
  • "lean" simulation output option to exclude No-ops and !ok functions (--SimulationLean flag)
  • getSimulateFromSeedInput helper function to improve setup in cmd/gaia/app/sim_test.go
  • standardize simulation output, begin building in json simulation output for simulation re-running
  • cleanup bank simulation messages / remove dup code in bank simulation
  • simulation moved to its own module (not a part of mock)
  • logger type instead of passing function variables everywhere
  • logger json output (for reloadable simulation running)

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

@rigelrozanski rigelrozanski mentioned this pull request Mar 7, 2019
5 tasks
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #3819 into develop will decrease coverage by <.01%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           develop    #3819      +/-   ##
===========================================
- Coverage    60.95%   60.95%   -0.01%     
===========================================
  Files          192      192              
  Lines        14360    14359       -1     
===========================================
- Hits          8753     8752       -1     
  Misses        5033     5033              
  Partials       574      574

@rigelrozanski rigelrozanski changed the title WIP: simulation refactor R4R: Simulation Refactor Mar 7, 2019
}
}

//// Builds a function to append logs
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this commented out code.

@cwgoes cwgoes self-assigned this Mar 11, 2019
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.

Generally the structural changes make sense to me, several comments.

Have you tried replicating a simulation from the log messages (is that the intent)?

PENDING.md Outdated Show resolved Hide resolved
x/bank/simulation/msgs.go Show resolved Hide resolved
x/bank/simulation/msgs.go Show resolved Hide resolved
x/gov/keeper.go Outdated Show resolved Hide resolved
x/simulation/simulate.go Outdated Show resolved Hide resolved
x/simulation/simulate.go Outdated Show resolved Hide resolved
x/simulation/simulate.go Outdated Show resolved Hide resolved
@rigelrozanski
Copy link
Contributor Author

@cwgoes

Have you tried replicating a simulation from the log messages (is that the intent)?

This is definitely the intent. The next step which is not included in this PR is the replication the simulation, I stopped short of following through with that to focus more on actual bug huntin'. Although a lot of the required high level structure is now here, there is a little more thinking/refactors required before that can be achieved surrounding the generation and storage of random parameters.

@rigelrozanski rigelrozanski requested a review from cwgoes March 14, 2019 14:47
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.

ACK

@cwgoes cwgoes merged commit f0d1efa into develop Mar 14, 2019
@cwgoes cwgoes deleted the rigel/sim-misc2 branch March 14, 2019 18:13
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