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

Simulation setup improvements #2588

Merged
merged 7 commits into from
Sep 3, 2022
Merged

Simulation setup improvements #2588

merged 7 commits into from
Sep 3, 2022

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Sep 3, 2022

What is the purpose of the change

A long standing issue of the simulator is how complex / verbose its setup code is, for very little reason.
I was working on adding property checks, and got derailed in trying to simplify the setup code & be able to use the simulation manager within the executor.

One spin off is: #2587

This has some of the basic property check struct definitions in here, so that these aren't living in stale branches. Its not in use with this.

Overarching instantiation changes:

  • Split up Simulator configs
  • Make simulator take in a AppConstructor rather than the app itself
  • (Corollaries of that):
    • Make simulation.App interface have SimulationManager method
    • Make AppStateFn take in simulation manager
    • make simulator return LastCommitId
    • Change ModuleAccountAddrs to not be defined on the app struct (ts already stateless)

Brief Changelog

  • Change app.ModuleAccountAddrs -> ModuleAccountAddrs() (not driveby, needed for the
  • Split up simulator config into multiple internal configs
  • Change simulation setup args, to remove some of the complexity
  • Move app instantiation complexity to have simulator-unique components set by the simulator rather than instantiation
  • Move Action setup for gamm into the simulation package now that its more complex (driveby change)

Testing and Verifying

Simulator still works

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? go API breaks
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? not applicable

@ValarDragon ValarDragon requested review from a team and czarcas7ic September 3, 2022 17:48
@github-actions github-actions bot added C:app-wiring Changes to the app folder C:simulator Edits simulator or simulations C:x/gamm Changes, features and bugs related to the gamm module. labels Sep 3, 2022
// in case we have to end early, don't os.Exit so that we can run cleanup code.
// TODO: Understand exit pattern, this is so screwed up. Then delete ^

legacyInvariantPeriod := uint(10) // TODO: Make a better answer of what to do here, at minimum put into config
Copy link
Member Author

Choose a reason for hiding this comment

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

This todo should just get handled with more property check work, and being able to set this to 0

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Awesome! ACK everything except for the pubsub stuff, which I know is still getting fleshed out 🚀

@ValarDragon ValarDragon merged commit 6ec8a90 into main Sep 3, 2022
@ValarDragon ValarDragon deleted the dev/sim_init_cleanup branch September 3, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:simulator Edits simulator or simulations C:x/gamm Changes, features and bugs related to the gamm module.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants