-
Notifications
You must be signed in to change notification settings - Fork 753
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
Integrate promises into StateManager #719
Conversation
This comment has been minimized.
This comment has been minimized.
Context: |
eb7d255
to
d061157
Compare
d061157
to
8a8bd4a
Compare
This comment has been minimized.
This comment has been minimized.
8a8bd4a
to
d137095
Compare
Codecov Report
@@ Coverage Diff @@
## master #719 +/- ##
==========================================
+ Coverage 91.01% 91.66% +0.65%
==========================================
Files 48 46 -2
Lines 3059 2998 -61
Branches 498 469 -29
==========================================
- Hits 2784 2748 -36
+ Misses 157 151 -6
+ Partials 118 99 -19
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
d137095
to
c85fc11
Compare
This pull request fixes 1 alert when merging c85fc11 into 9cbf19b - view on LGTM.com fixed alerts:
|
Thanks @ryanio, that's really great work! 😄 The "VM Test / test-api" suite is still failing, this would need some fix. Some note: I think it would make sense (likely separate PR if we decide) to prefix the job names in the workflows with the name of the package, e.g. test-api for the VM. These are shown in the branch protection rule settings page like this: So it's not possible to differentiate or at least associate. I also not sure how this will behave on identically named jobs like I've now activated the three VM test jobs ( |
c85fc11
to
437053d
Compare
This pull request fixes 1 alert when merging 437053d into 8310617 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 3a3b69c into 8310617 - view on LGTM.com fixed alerts:
|
@holgerd77 great thanks I have resolved the failing build, so this PR is now ready for review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some short starter, one change request, one question.
This pull request fixes 1 alert when merging 26a9585 into 8310617 - view on LGTM.com fixed alerts:
|
26a9585
to
073c358
Compare
This pull request fixes 1 alert when merging 073c358 into 8310617 - view on LGTM.com fixed alerts:
|
@ryanio: just for info, @PhilippLgh will continue the review here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commits are not so easy to review, given the amount of linting changes. For the future, I propose to pack them in groups such as:
- Linting code
- VM docs
- Fix A
- Integrate B
This pull request fixes 1 alert when merging 6b700bf into 36fde16 - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have continued here a bit. Short progress update: now at 34 / 51 files viewed. 😄
@@ -73,7 +71,7 @@ async function runTx(vm: VM, rawTx: any, privateKey: Buffer) { | |||
|
|||
main() | |||
.then(() => process.exit(0)) | |||
.catch(error => { | |||
.catch((error) => { | |||
console.error(error) | |||
process.exit(1) | |||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, all example changes look good.
return { current, original } | ||
} else { | ||
return current | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
@@ -186,7 +184,7 @@ async function _runTx(this: VM, opts: RunTxOpts): Promise<RunTxResult> { | |||
*/ | |||
if (results.execResult.selfdestruct) { | |||
const keys = Object.keys(results.execResult.selfdestruct) | |||
for (const k of keys) { | |||
for await (const k of keys) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for understanding: what is this double-await changing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holgerd77 for await
is for async iterators, but Object.keys does not return an async iterator, so this is a mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryanio nit ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@holgerd77 @kumavis noted, thanks, I will address in a follow-up PR.
edit: here it is #728
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kumavis thanks for having another look here! 🙂
const account = await this._lookupAccount(address) | ||
this._update(address, account, false, false) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew. Nice. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, just was on it and directly finished going through, thanks @ryanio, fantastic work done here 😄 🎉, looks good, will merge.
this._cache.del(address) | ||
} | ||
} | ||
this._touched.clear() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. 😄 👍
This PR:
StateManager
, resolving the need forPStateManager
.8.x
to12.x
.Diff views: stateManager.ts, state/cache.ts