Promisify rest of library by ryanio · Pull Request #107 · ethereumjs/merkle-patricia-tree

ryanio · 2020-04-03T00:30:21Z

This PR:

Removes callbacks in favor of promises
Removes use of async lib (The async library organization#55)
Updates README examples to new syntax
Removes deprecated methods (getRaw, putRaw, delRaw)

Closes #66

Pending TODOs:

Investigate why _formatNode does not use the remove param inside the method (lost in a prior refactor?)

src/db.ts

github-actions · 2020-04-08T00:54:31Z

Coverage increased (+0.9%) to 94.992% when pulling 3a42145 on promisifyLib into ce10588 on master.

holgerd77 · 2020-04-08T20:33:19Z

Great to see the tests pass here, very cool! 😄

holgerd77

To be continued...

Can also be picked up (also partially).

holgerd77 · 2020-04-08T20:37:58Z

README.md

+  - [Trie and Patricia Trie Overview](https://www.youtube.com/watch?v=jXAHLqQthKw&t=26s)

 # EthereumJS



Did an overall README check, everything covered, examples look good as well.

holgerd77 · 2020-04-08T20:44:12Z

benchmarks/random.ts

+  }
 }
+
+go()


Tested both manually locally, updated benchmark suites at least run through. This seems to need more work to be usable - the checkpointing benchmark file e.g. is not taking the samples into account at all e.g., think due to a bug with a reused/side-used i counter variable as far as I see it - nothing for this PR though but just as a note.

ah thanks for pointing out, I will give it a closer look.

holgerd77 · 2020-04-08T20:51:47Z

package.json

-    "browserify": "^16.5.0",
    "husky": "^4.2.3",
    "karma": "^4.4.1",
    "karma-chrome-launcher": "^3.1.0",


package.json changes look good, types from removed libraries (async) removed as well.

holgerd77 · 2020-04-08T20:54:59Z

package.json

+    "readable-stream": "^3.6.0",
    "rlp": "^2.2.3",
-    "semaphore": ">=1.0.1"
+    "semaphore-async-await": "^1.5.1"


Context: https://github.com/jsoendermann/semaphore-async-await

holgerd77 · 2020-04-08T21:06:49Z

src/baseTrie.ts

- * @param {Buffer|String} [root] A hex `String` or `Buffer` for the root of a previously stored trie
- * @prop {Buffer} root The current root of the `trie`
- * @prop {Buffer} EMPTY_TRIE_ROOT the Root for an empty trie
+ * @param {Buffer} [root] - A `Buffer` for the root of a previously stored trie


Note: significant API change here, input limited to Buffer.

holgerd77 · 2020-04-08T21:09:53Z

src/baseTrie.ts

+    const p = stack.map((stackElem) => {
+      return stackElem.serialize()
    })
+    return p


Nice, elegantifying. 😄

holgerd77 · 2020-04-08T21:17:21Z

src/baseTrie.ts

+      // then update
+      await this._updateNode(key, value, remaining, stack)
    }
+    this.lock.signal()


This new async/await compatible locking is really great! Generally functions like the above (put) are so much more readable now, phew! 👍

Also thanks for all the small improvements like changing execution order on conditionals, rethinking the appliance of asserts, code cleanups,... 🎊

holgerd77 · 2020-04-08T21:25:09Z

src/baseTrie.ts

+      foundNode = decodeNode(value)
    }
+
+    return foundNode


del, _lookupNode ok.

holgerd77

To be continued... (again: can also be (partly) taken over).

holgerd77 · 2020-04-09T12:42:16Z

src/baseTrie.ts

-   *  - node - the last node found
-   *  - keyRemainder - the remaining key nibbles not accounted for
-   *  - stack - an array of nodes that forms the path to node we are searching for
+   * @param {Buffer} key - the search key


For the record (and the release notes): significant API change here (limitation to Buffer).

holgerd77 · 2020-04-09T12:47:05Z

src/baseTrie.ts

+      let targetKey = bufferToNibbles(key)
+
+      // walk trie and process nodes
+      await this._walkTrie(this.root, async (nodeRef, node, keyProgress, walkController) => {


Inlined processNode function, ok.

holgerd77 · 2020-04-09T12:49:59Z

src/baseTrie.ts

+
+      // Resolve if _walkTrie finishes without finding any nodes
+      resolve({ node: null, remaining: [], stack })
+    })


Rest of findPath more or less same code in a somewhat changed structural form, ok.

holgerd77 · 2020-04-09T12:55:06Z

src/baseTrie.ts

+   * @returns {Promise}
   */
-  _updateNode(
+  async _updateNode(


For the record: significant API change here (limit value input to Buffer).

holgerd77 · 2020-04-09T13:04:55Z

src/baseTrie.ts

+      } else {
+        resolve()
+      }
+    })


Whew, this is a biggy. I have to say that I just can't follow the changes here in _walkTrie, eventually someone else can give this another look (//cc @s1na @evertonfraga) For the moment I am not getting the whole picture.

Ok, I think I finally got the replacement constructs, can give this a go now too. 😀

holgerd77 · 2020-04-09T13:09:07Z

src/baseTrie.ts

-    })
+  async checkRoot(root: Buffer): Promise<boolean> {
+    const value = await this._lookupNode(root)
+    return !!value


Phew, nice. 😀

Apart from walkTrie needing a second look the rest from baseTrie.js looks good.

holgerd77 · 2020-04-09T13:10:59Z

src/checkpointTrie.ts

+      await this._exitCpMode(true)
+    }
+
+    this.lock.signal()


Nice restructure. 👍

holgerd77 · 2020-04-09T13:13:17Z

src/checkpointTrie.ts

+    let scratch = scratchDb || this._scratch
+    if (!scratch) {
+      throw new Error('No scratch found to use')
+    }


For the record: new throw on "No scratch found to use".

holgerd77 · 2020-04-09T13:13:37Z

src/checkpointTrie.ts

+    let scratch = scratchDb || this._scratch
+    if (!scratch) {
+      throw new Error('No scratch found to use')
+    }


checkpointTrie.ts looks good.

holgerd77

Two files/functions not reviewed yet (baseTrie->walkTrie() and scratch.ts), otherwise looking good. Really great work @ryanio Ryan! 🤗 🎸 🌴 🎉

holgerd77 · 2020-04-09T20:11:32Z

src/db.ts

-
-    this._leveldb.batch(opStack, ENCODING_OPTS, cb)
+  async batch(opStack: BatchDBOp[]): Promise<void> {
+    await this._leveldb.batch(opStack, ENCODING_OPTS)


db.ts ok.

src/prioritizedTaskExecutor.ts

holgerd77 · 2020-04-09T20:16:07Z

src/readStream.ts

+      })
+      await walkController.next()
+    })
+    this.push(null)


readStream.ts changes ok.

holgerd77 · 2020-04-09T20:19:41Z

src/scratchReadStream.ts

+      })
+      await walkController.next()
+    })
+    this.push(null)


scratchReadStream.ts ok.

holgerd77 · 2020-04-09T20:21:28Z

src/secure.ts

+  async del(key: Buffer): Promise<void> {
    const hash = keccak256(key)
-    super.del(hash, cb)
+    await super.del(hash)


secure.ts ok.

holgerd77 · 2020-04-09T20:22:16Z

src/trieNode.ts

    return BranchNode.fromArray(raw)
  } else if (raw.length === 2) {
-    const nibbles = stringToNibbles(raw[0])
+    const nibbles = bufferToNibbles(raw[0])


trieNode.ts ok.

holgerd77 · 2020-04-09T20:28:32Z

test/checkpoint.spec.ts

+    await trie.commit()
+    t.equal(trie.isCheckpoint, false)
+    t.equal(trie.root.toString('hex'), root.toString('hex'))
+    t.end()


checkpoint.spec.ts ok.

holgerd77 · 2020-04-09T20:34:05Z

test/db.spec.ts

+    await db.batch(ops)
+    const res = await db.get(k2)
+    st.ok(v2.equals(res!))
+    st.end()


db.spec.ts ok.

General (file unspecific) remark: the error cases in this library seem to be not very well tested, might be a field for some future improvements.

holgerd77 · 2020-04-09T20:41:15Z

test/stream.spec.ts

-      t.equal(Object.keys(expectedNodes).length, 0)
+    stream.on('end', () => {
+      let keys = Object.keys(expectedNodes)
+      t.equal(keys.length, 0)


Rest of the tests look good. 😄

holgerd77 · 2020-04-09T20:44:54Z

src/scratch.ts

    }

-    asyncFirstSeries(getDBs, dbGet, cb)
+    return value


Leave scratch.ts also open since not seeing through. Maybe will have a look with some fresh eyes in the following days myself, too tired right now, or otherwise open for a look from someone else as well (//cc @s1na @evertonfraga).

Ok, finally got this whole construct decomposition. Phew. 😛 Extremely valuable to have this simplified on such a level, thanks Ryan for working yourself through stuff like this. This was really over-the-top complex and extremely hard to grasp before.

holgerd77

LGTM. 😄

Will approve and directly merge.

holgerd77 · 2020-04-13T21:12:31Z

src/scratch.ts

    }

-    asyncFirstSeries(getDBs, dbGet, cb)
+    return value


Ok, finally got this whole construct decomposition. Phew. 😛 Extremely valuable to have this simplified on such a level, thanks Ryan for working yourself through stuff like this. This was really over-the-top complex and extremely hard to grasp before.

holgerd77 · 2020-04-13T21:33:01Z

src/baseTrie.ts

+      } else {
+        resolve()
+      }
+    })


Ok, I think I finally got the replacement constructs, can give this a go now too. 😀

holgerd77 · 2020-04-13T21:33:44Z

src/baseTrie.ts

+            const childRef = node.getBranch(childIndex)
+            if (!childRef) {
+              throw new Error('Could not get branch of childIndex')
+            }


For the record: new "Could not get branch of childIndex" error thrown here.

Upgrade from callbacks to promises

2e4c40e

This comment has been minimized.

Sign in to view

holgerd77 reviewed Apr 3, 2020

View reviewed changes

src/db.ts Outdated Show resolved Hide resolved

ryanio force-pushed the promisifyLib branch from 3a42145 to 1e4671a Compare April 8, 2020 01:07

ryanio marked this pull request as ready for review April 8, 2020 01:11

ryanio added the PR state: needs review label Apr 8, 2020

ryanio force-pushed the promisifyLib branch from 1e4671a to 860cdf8 Compare April 8, 2020 02:42

Upgrade from callbacks to promises

056152c

ryanio force-pushed the promisifyLib branch from 860cdf8 to 056152c Compare April 8, 2020 02:46

holgerd77 reviewed Apr 8, 2020

View reviewed changes

holgerd77 reviewed Apr 9, 2020

View reviewed changes

ryanio added 2 commits April 10, 2020 12:00

Improve benchmarks code

dfaa274

Fix typedoc (method execute not async)

9390ab1

holgerd77 approved these changes Apr 13, 2020

View reviewed changes

holgerd77 merged commit 41ef2d8 into master Apr 13, 2020

holgerd77 deleted the promisifyLib branch April 13, 2020 21:34

This was referenced Apr 17, 2020

Release - v4.0.0 #111

Merged

Re-add raw methods, accept leveldb in constructor, minor fixes #92

Merged

ryanio mentioned this pull request May 7, 2020

_lookupNode is not err-first callback style #17

Closed

		- [Trie and Patricia Trie Overview](https://www.youtube.com/watch?v=jXAHLqQthKw&t=26s)

		# EthereumJS

+                }
               }
+              go()

Conversation

ryanio commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

github-actions bot commented Apr 8, 2020

Uh oh!

holgerd77 commented Apr 8, 2020

Uh oh!

holgerd77 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holgerd77 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holgerd77 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holgerd77 left a comment

Choose a reason for hiding this comment

ryanio commented Apr 3, 2020 •

edited

Loading