This repository has been archived by the owner on Apr 4, 2024. It is now read-only.
use stack of contexts to implement snapshot revert #399
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
the integration tests are verified manually, not sure if it's get picked up in CI. |
fedekunze
reviewed
Aug 5, 2021
There was a problem hiding this comment.
I like this approach and PR. I think we should move the cached context struct into a new file and implement it as a stack. We should also add more comprehensive comments (especially on RevertToSnapshot and Snapshot) so that people know how this actually works and the motivation behind this approach.
thomas-nguy
reviewed
Aug 5, 2021
Codecov Report
@@ Coverage Diff @@
## main #399 +/- ##
==========================================
- Coverage 50.74% 50.69% -0.05%
==========================================
Files 48 49 +1
Lines 4846 4872 +26
==========================================
+ Hits 2459 2470 +11
- Misses 2288 2298 +10
- Partials 99 104 +5
|
thomas-nguy
reviewed
Aug 6, 2021
thomas-nguy
reviewed
Aug 6, 2021
thomas-nguy
reviewed
Aug 6, 2021
|
beside some naming convention, lgtm |
fedekunze
reviewed
Aug 6, 2021
fedekunze
reviewed
Aug 6, 2021
fedekunze
reviewed
Aug 6, 2021
yihuang
force-pushed
the
snapshot
branch
2 times, most recently
from
13352da
to
92fabd1
Compare
fedekunze
approved these changes
Aug 9, 2021
changelog added. |
fedekunze
approved these changes
Aug 9, 2021
Closes #338 add exception revert test case verify partial revert mutate state after the reverted subcall polish update comments name the module after the type name remove the unnecessary Snapshot in outer layer and add snapshot unit test assert context stack is clean after tx processing cleanups fix context revert fix comments update comments it's ok to commit in failed case too Update x/evm/keeper/context_stack.go Co-authored-by: Federico Kunze Küllmer <[email protected]> Update x/evm/keeper/context_stack.go Co-authored-by: Federico Kunze Küllmer <[email protected]> Update x/evm/keeper/context_stack.go Co-authored-by: Federico Kunze Küllmer <[email protected]> update comment and error message add comment to cacheContext k -> cs Update x/evm/keeper/context_stack.go Co-authored-by: Federico Kunze Küllmer <[email protected]> evm can handle state revert renames and unit tests
fedekunze
reviewed
Aug 10, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #338
Description
For contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)