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

feat: add simulations part 1 #314

Merged
merged 34 commits into from
Mar 3, 2023
Merged

feat: add simulations part 1 #314

merged 34 commits into from
Mar 3, 2023

Conversation

aljo242
Copy link

@aljo242 aljo242 commented Feb 21, 2023

1. Summary

Part of #294
Fixes QCK-112
Fixes QCK-105
Fixes QCK-106
Fixes QCK-104

Add simulation testing back into the app and create a basic implementation of x/mint simulation. x/epochs simulation already existed, so it is just added back in.

The remaining module simulations will be added piecemeal for each module and tracked in separate issues.

2.Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

3. Implementation details

  • Add simulation manager to application in /simulation
  • Add set of helper utilities for future simulation tests in /simulation/simtypes (adapted from https://github.com/osmosis-labs/osmosis/tree/main/simulation/simtypes)
  • Set up simulation helper functions in app/simulation
  • add x/{moduleName}/simulation for each of our custom modules
  • add simulation.yml workflow which runs a lightweight version of TestSimNonDeterminism

4. How to test/use

make test-sim-* targets

5. Checklist

  • Does the Readme need to be updated?

6. Limitations (optional)

Existing simulation "scenarios" need to be thoroughly checked.

7. Future Work (optional)

Add more sim functionality as needed

@aljo242 aljo242 marked this pull request as ready for review February 22, 2023 15:05
@aljo242 aljo242 changed the title feat: add simulations feat: add simulations part 1 Feb 22, 2023
@aljo242 aljo242 mentioned this pull request Feb 23, 2023
2 tasks
app/app.go Outdated Show resolved Hide resolved
joe-bowman
joe-bowman previously approved these changes Feb 24, 2023
Copy link
Contributor

@joe-bowman joe-bowman left a comment

Choose a reason for hiding this comment

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

untested, but on the whole looks good. just the question around moving of the mint module, but think I answered that one myself!

app/app.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
@ajansari95 ajansari95 added the run-sim runs simulations label Mar 3, 2023
@ajansari95
Copy link
Contributor

@aljo242 merging app.go changes created conflicts as they are same. If we can resolve please

@aljo242
Copy link
Author

aljo242 commented Mar 3, 2023

@ajansari95 looks like the sim did not run.

Merge conflicts resolved

ajansari95
ajansari95 previously approved these changes Mar 3, 2023
Copy link
Contributor

@ajansari95 ajansari95 left a comment

Choose a reason for hiding this comment

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

LGTM

@ajansari95
Copy link
Contributor

@joe-bowman it dismiised your review as stale.

@joe-bowman
Copy link
Contributor

sim didn't run? @ajansari95 @aljo242

@aljo242
Copy link
Author

aljo242 commented Mar 3, 2023

@ajansari95 @joe-bowman - action should be fixed now

@aljo242 aljo242 requested review from joe-bowman and ajansari95 March 3, 2023 15:52
@ajansari95
Copy link
Contributor

@ajansari95 @joe-bowman - action should be fixed now

yup looking in if you add labels on already existing PR will it work or not.

@aljo242
Copy link
Author

aljo242 commented Mar 3, 2023

Yeah I think this method should allow us to add the label at a later time - which is better

Copy link
Contributor

@joe-bowman joe-bowman left a comment

Choose a reason for hiding this comment

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

lgtm

@joe-bowman
Copy link
Contributor

@ajansari95 - your review was dismissed here ;)

Copy link
Contributor

@ajansari95 ajansari95 left a comment

Choose a reason for hiding this comment

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

LGTM

@aljo242 aljo242 merged commit 8c206d9 into main Mar 3, 2023
@aljo242 aljo242 deleted the feat/add-sim branch March 3, 2023 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-sim runs simulations testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants