Remove async dependency #779

jochem-brouwer · 2020-06-16T15:19:53Z

This is a WIP.

Closes #734

evertonfraga · 2020-06-17T13:02:07Z

LMK if you need help here with testing, reviewing, rebasing :)

ryanio · 2020-06-24T16:40:17Z

@jochem-brouwer just a note I removed the async dep in testUtil.setupPreConditions in #788 so if you rebase onto master you'll get those changes 👍

Feel free to ask for any help as you keep working on this PR! Thanks for taking it on.

jochem-brouwer · 2020-06-24T16:59:05Z

@ryanio Thank you. That change seems to explicitly remove promisify. I am not sure that this is within the scope of this PR/issue: this is not part of the async package. Correct me if I'm wrong.

ryanio · 2020-06-24T17:27:09Z

@jochem-brouwer it does remove an async.series and async.eachSeries

jochem-brouwer · 2020-06-24T17:28:24Z

@ryanio missed this when skimming over the PR. Great! 😄

jochem-brouwer · 2020-06-29T12:13:52Z

Hey @evertonfraga, could you rebase master on top of this here? I am almost done with this PR, except I need one extra file in the VM tests where @ryanio has removed some async package calls already.

I did rebase before but it seems that this went wrong. The files changed also have files changed which are changed from rebasing master, I expected that those would not show up in "files changed". I instead expected that I would still only see the files I changed and not the files changed due to the rebase. But I think I did something wrong 😅

If this yields a lot of conflicts and the code is confusing: I intend to clean it up later, but if that makes rebasing easier let me know, then I'll do that now.

codecov · 2020-06-29T19:24:05Z

Codecov Report

Merging #779 into master will decrease coverage by 0.64%.
The diff coverage is 73.75%.

Flag	Coverage Δ
#account	`92.85% <ø> (ø)`
#block	`79.28% <79.20%> (-0.69%)`	⬇️
#blockchain	`83.09% <79.20%> (-1.42%)`	⬇️
#common	`94.14% <ø> (+0.15%)`	⬆️
#ethash	`81.37% <28.57%> (-4.93%)`	⬇️
#tx	`94.16% <ø> (ø)`
#vm	`92.91% <76.92%> (-0.22%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

ryanio · 2020-06-29T20:22:37Z

packages/blockchain/src/index.ts

@@ -20,6 +19,21 @@ const Stoplight = require('flow-stoplight')
 const level = require('level-mem')
 const semaphore = require('semaphore')

+// promisify a function: resolves this promise if callback is called


nice, you can actually just import { promisify } from 'util'

This is an external package right? I am not sure if this does what this intends to do; this is a much simpler variant I think.

Ah, no, this is a build-in Node.js functionality (see link from Ryan) and we are using this all over the place. Please use here as well, otherwise this bloats the code unnecessarily.

I was not aware of it, I thought it was an external package. Will change, thanks!

great :) if it has trouble erroring out in karma test runner then you might need to import from util-promisify for a browser-friendly version, as used elsewhere in the monorepo, but it's a very small package and we can hopefully remove the need for it soon.

ryanio · 2020-06-30T03:37:36Z

packages/vm/tests/api/state/stateManager.js

-    if (isRunningInKarma()) {
-      st.skip('skip slow test when running in karma')
-      return st.end()
-    }


it looks like the ci check failure is due to these lines being removed (it times out)

I have no idea why I deleted this 🤔

Could've been on my rebase. It was a nasty one.

ryanio · 2020-06-30T03:44:57Z

packages/vm/lib/runBlockchain.ts

-          .runBlock({
-            block: block,
-            root: parentState,
+        await new Promise((resolve, reject) =>


you can make your life a lot easier using node.js util promisify and callbackify along with async/await syntax to flatten all this code, but no worries i can do that in a follow-up pr (or here if you'd prefer)

after blockchain (+ ethash) are converted the entire monorepo will be promise-based (as i don't think there's any more internal usage of callbacks) so that will be great.

I'll clean up all these promises, it makes more sense to do it in this PR 😄

jochem-brouwer · 2020-07-13T13:08:05Z

Hey guys! This PR is open for review. However, for some reason the tests seem to fail - they /do/ work locally, after bootstrapping and re-building everything, so I'm not sure what's going wrong here (looks like some timeouts).

This PR looks huge but it seems like the changes which were rebased-in from master a while back are also in the diff. I'm not sure if it's possible to only show my changes on the diff? @evertonfraga

I'm not super proud of this code as I don't think it's that elegant. I have thus changed (mostly in the Blockchain package) every async call now to a native promise call. However, before we start to review, we might want to consider to actually remove all the callbacks from Blockchain and thus change the signature of each function where each function would normally return a <void> we could now return a Promise<T> and therefore make these things async (this will definitely improve the code a lot). But this might also be something to consider for a different PR?

CC @holgerd77 @ryanio

holgerd77 · 2020-07-15T08:30:37Z

Hi @jochem, can you do a rebase of this on top of the TangerineWhistle changes (I would recommend to do a local repo backup before)?

@ryanio can you take over the review here, you would be very suited here with all your rewrite experience on the topic! 😄

jochem · 2020-07-15T08:41:32Z

Hi @jochem, can you do a rebase of this on top of the TangerineWhistle changes (I would recommend to do a local repo backup before)?

Let me relay this one to @jochem-brouwer ;)

holgerd77 · 2020-07-15T08:54:48Z

@jochem That's reasonable. Lol. 😜

jochem-brouwer · 2020-07-15T14:50:50Z

Rebased! These tests will need some inspection though; looks like the API tests pass, but if they are done in browser they timeout (!?). Someone has an idea why this happens? This could of course be due to browser handling the async stuff different than in node?

jochem-brouwer · 2020-07-30T09:54:15Z

If it makes it easier to review, I might also build on top of this one the changes necessary to remove the callbacks from blockchain? Right now the blockchain package is the only one which still uses callbacks; we can asyncify these functions (which would be a PR built in the future anyways). It would also make sense to introduce this in our next V5 release?

evertonfraga · 2020-07-30T11:53:17Z

test:API and test:API:browser are executing different amounts of test cases. Then, there are some assertions with no apparent output.

I'm taking a deep look at this now @jochem-brouwer. Thanks!

ryanio · 2020-07-30T20:18:44Z

packages/ethash/src/index.ts

+          if (error) {
+            cb(false)
+          }
+          await new Promise((resolve, reject) => {


can this code be cleaned up/flattened further? we can consider removing the callback and just supporting promises since ethash is mostly an internal lib just used in the blockchain package.

Just took a look here: if we change this, this means we also have to "asyncify" the blockchain package. I agree it should be cleaned up and removed (we should be consistent and thus not have callback functions but instead promise functions in all packages of this monorepo; ethash and blockchain are exceptions currently). However I think this might be too much for this PR as this is likely to involves code changes for every call to the blockchain or ethash package (we thus have to convert every callback function call into a Promise function call).

If you are OK with that then I'll follow up with a new PR, if not then I'll build it on top of this one and we merge if that is ready.

thanks, ok that sounds good to me. i'm fine with either integrating into this PR or opening a new one 👍

Great! Could you approve this? Your other comment on ethash has the same remark and the nit one has been resolved 😄

ryanio · 2020-07-30T20:21:13Z

packages/ethash/test/block.spec.ts

+  await new Promise((resolve) => genesisBlock(resolve))
+    .then(function () {
+      return new Promise((resolve) => validBlockTest(resolve))
+    })
+    .then(function () {
+      return new Promise((resolve) => invalidblockTest(resolve))
+    })


same here, i think we can convert these functions to async

ryanio · 2020-07-30T20:24:17Z

packages/vm/tests/util.js

-        account: acnt,
-        testData: testData,
-      })
+      q.push(new Promise((resolve, reject) => {


nit: can use promisify here

ryanio · 2020-07-30T20:26:01Z

looking good! i still see quite a few new Promises that can be simplified further

holgerd77 · 2020-08-06T16:36:15Z

@jochem-brouwer can you address the latest requests from Ryan and rebase so that we can bring this over the finish line? 🙂

jochem-brouwer · 2020-08-06T20:43:33Z

@holgerd77 yes, will prioritize this after the weekend! 😄 (+squash commits)

remove async from ethash tests

restore skip test in state manager change promisify to util.promisify

[VM] fix browser test [VM] promisify util

jochem-brouwer · 2020-08-06T22:17:11Z

Never mind, should be done (if discussions are resolved) 😄

holgerd77

Approved after review of Ryan.

evertonfraga added package: blockchain type: refactor package: vm labels Jun 17, 2020

evertonfraga mentioned this pull request Jun 18, 2020

fix: Equalizing dependency versions #782

Merged

jochem-brouwer added the PR state: WIP label Jun 19, 2020

jochem-brouwer marked this pull request as draft June 26, 2020 19:57

evertonfraga force-pushed the remove-async-dependency branch from 6c85e79 to 573421a Compare June 29, 2020 19:23

ryanio reviewed Jun 29, 2020

View reviewed changes

ryanio reviewed Jun 30, 2020

View reviewed changes

evertonfraga added the package: ethash label Jun 30, 2020

evertonfraga changed the title ~~WIP: Remove async dependency~~ Remove async dependency Jun 30, 2020

jochem-brouwer added PR state: needs review and removed PR state: WIP labels Jul 13, 2020

jochem-brouwer marked this pull request as ready for review July 13, 2020 13:04

jochem-brouwer requested review from ryanio, holgerd77 and evertonfraga July 13, 2020 13:08

jochem-brouwer force-pushed the remove-async-dependency branch from 066c57b to 3f91f89 Compare July 15, 2020 14:49

ryanio reviewed Jul 30, 2020

View reviewed changes

jochem-brouwer force-pushed the remove-async-dependency branch from 3f91f89 to ed079c8 Compare August 6, 2020 21:30

jochem-brouwer added 5 commits August 6, 2020 23:33

remove async pkg from stateManager and blockchain

0db2526

remove async from ethash package

4277305

remove async from ethash tests

don't require the async package in runBlockchain

288572d

remove async from util

45e7b5d

restore skip test in state manager change promisify to util.promisify

add util.promisify as a dependency to blockchain

bc9d567

[VM] fix browser test [VM] promisify util

jochem-brouwer force-pushed the remove-async-dependency branch from ed079c8 to bc9d567 Compare August 6, 2020 21:34

update package-lock

9418d8e

holgerd77 approved these changes Aug 7, 2020

View reviewed changes

holgerd77 merged commit ca426da into master Aug 7, 2020

holgerd77 deleted the remove-async-dependency branch August 7, 2020 09:02

This was referenced Aug 14, 2020

Promisify blockchain and ethash packages #833

Merged

Remove async dependency / Test runner code cleanup #759

Closed

Remove async dependency #779

Remove async dependency #779

Conversation

jochem-brouwer commented Jun 16, 2020

evertonfraga commented Jun 17, 2020

ryanio commented Jun 24, 2020

jochem-brouwer commented Jun 24, 2020

ryanio commented Jun 24, 2020

jochem-brouwer commented Jun 24, 2020

jochem-brouwer commented Jun 29, 2020

codecov bot commented Jun 29, 2020 • edited Loading

Codecov Report

ryanio Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer commented Jul 13, 2020

holgerd77 commented Jul 15, 2020

jochem commented Jul 15, 2020

holgerd77 commented Jul 15, 2020

jochem-brouwer commented Jul 15, 2020

jochem-brouwer commented Jul 30, 2020

evertonfraga commented Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanio commented Jul 30, 2020

holgerd77 commented Aug 6, 2020

jochem-brouwer commented Aug 6, 2020 • edited Loading

jochem-brouwer commented Aug 6, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 29, 2020 •

edited

Loading

ryanio Jun 29, 2020 •

edited

Loading

evertonfraga commented Jul 30, 2020 •

edited

Loading

jochem-brouwer commented Aug 6, 2020 •

edited

Loading