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

Simplify multitest storage #335

Merged
merged 33 commits into from
Jul 21, 2021
Merged

Simplify multitest storage #335

merged 33 commits into from
Jul 21, 2021

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Jul 19, 2021

merge after #334

Major overhaul of how storage and caching works in multi-test.
This should allow for much more extensibility in multi-test in the future

Base automatically changed from contract-builders to main July 20, 2021 07:32
@ethanfrey ethanfrey marked this pull request as ready for review July 20, 2021 10:26
@ethanfrey ethanfrey requested a review from maurolacy July 20, 2021 10:26
@ethanfrey
Copy link
Member Author

Okay, this massive PR is ready for review.

It is a quasi-rewrite of the internals of multi-test, keeping a very similar API.

I think it makes everything much clearer and extensible, and lays the groundwork to easily add more "Keepers" to mock out staking, or handle custom messages

@maurolacy
Copy link
Contributor

Can you add some context / description of the changes? It is impossible for me to review this in any meaningful way. Too many changes, that are unrelated, or at least, difficult to relate to the previous code base.

packages/multi-test/src/wasm.rs Outdated Show resolved Hide resolved
packages/multi-test/src/wasm.rs Outdated Show resolved Hide resolved
packages/multi-test/src/wasm.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

Can you add some context / description of the changes? It is impossible for me to review this in any meaningful way. Too many changes, that are unrelated, or at least, difficult to relate to the previous code base.

Yeah, I would just review the new codebase, and not the diff.
I will write a docs.rs file explaining the new architecture, then ask for a re-review.

The comments above were helpful to identify that.

@ethanfrey
Copy link
Member Author

@maurolacy I added a DESIGN.md file that should help.

I would not review the diff, but rather review the new code by itself. You can add comments on the diff, but this is basically a re-write, so it makes more sense to look at it as a whole.

test_helpers, transactions, and contracts rust files are more or less unchanged.
bank, wasm, app and executor are > 50% modified

@maurolacy
Copy link
Contributor

@maurolacy I added a DESIGN.md file that should help.

Thanks. I'll take a look.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Reviewed the DESIGN doc.

packages/multi-test/DESIGN.md Outdated Show resolved Hide resolved
packages/multi-test/DESIGN.md Outdated Show resolved Hide resolved
packages/multi-test/DESIGN.md Show resolved Hide resolved
packages/multi-test/DESIGN.md Show resolved Hide resolved
packages/multi-test/DESIGN.md Show resolved Hide resolved
packages/multi-test/DESIGN.md Show resolved Hide resolved
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

lgtm, but this is still above my head. Need to get more exposure to both using and modifying / extending multi-test.

packages/multi-test/src/wasm.rs Show resolved Hide resolved
packages/multi-test/src/wasm.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

Thanks for the feedback. I will update and merge.
There are a number of smaller steps in the next few weeks to get this closer to the vision

@ethanfrey ethanfrey merged commit dbddd7d into main Jul 21, 2021
@ethanfrey ethanfrey deleted the simplify-multitest-storage branch July 21, 2021 17:54
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.

2 participants