Move some db ops to DBManager #91

s1na · 2019-02-13T15:52:35Z

I started refactoring this by moving some of the (easier) db-related logic to a new class DBManager (which works with promises), and used util.callbackify in the same methods in Blockchain. I'm not sure if util.callbackify works in browser? (I think I remember browserify supports some libraries like util). But honestly, the bigger functions like _putBlockOrHeader are a piece of work, and I think rewriting it from scratch in typescript might be easier!

src/dbManager.js

vpulim · 2019-02-14T13:55:37Z

src/dbManager.js

+    this._opts = {
+      keyEncoding: 'binary',
+      valueEncoding: 'binary'
+    }


this._opts can be removed (see suggested changes below for this.get())

vpulim · 2019-02-14T14:19:15Z

I made a few minor DRY suggestions above, but otherwise looks great!

Regarding browser support for util.callbackify(), it's not currently available in webpack or browserify, but it's almost there: browserify/browserify#1869

In the meantime, we can just directly use the util.callbackify() shim from node-util@0.11.1

Also, during the re-factor, one thing to think about is that the current Blockchain implementation uses a Block as the atomic structure for chains. However, this makes it difficult to create chains of just headers, which is useful in the context of the EthereumJS client. Geth and other clients treat the blockchain as a chain of headers that can optionally have bodies (transactions and uncles) attached to them. One of the reasons for the ugly _putBlockOrHeader method is that the legacy Blockchain code forced you to create chains of empty, header-only Blocks even if all you wanted to do was create a chain of just Headers. I added the new put/getHeader() methods to the API in preparation for a future, improved implementation of Blockchain that more efficiently stores Header chains, but for now I had to shoehorn Headers into empty Blocks.

s1na · 2019-02-18T12:56:36Z

@vpulim Thanks for the suggestions, they made the code much cleaner!

It seems that node 6 imports its own util which has no method callbackify and ci is failing because of that. Tried const util = require('../node_modules/util') locally with node6 and it works but it's hacky. Do you have any ideas how to overcome this?

Injected this manually to ethereumjs-vm/node_modules and the tests pass. However I noticed that @babel/runtime should be added as a dependency for libraries importing ethereumjs-blockchain to work.

vpulim · 2019-02-18T14:21:00Z

@s1na Honestly, I think you should just modify .travis.yml to drop Node 6 support like most of the other ethereumjs libraries already did. In fact, Node 6 no longer works properly with Xcode 10 (see: nodejs/node#24648) so I'm not even able to do an npm install on my development machine. We should probably remove Node 6 support from all EthereumJS libraries as major version update releases (@holgerd77, what do you think?).

I believe @babel/runtime is supposed to be added as a dependency when you have @babel/plugin-transform-runtime installed as a devDependency, so what you have is correct.

EDIT: Actually, I forgot we are building the dist/index.js file before publishing the package, so @babel/runtime should be listed as a devDependency, not a regular dependency.

holgerd77 · 2019-02-19T09:36:50Z

I would be cautiously supportive for dropping Node 6 (@vpulim: though this hasn't been dropped yet on other libraries, would be a "first" here, maybe you are confusing with Node 4?). Just had a short look, Node 6 reaches EOL in April 2019, so this should be justifiable. I will open a PR on this on organization and also point to it from Gitter to make this a bit more transparent and give people the chance to intervene if necessary.

From a look a the release table linked above, we should probably currently test against Node 8, Node 10 and Node 11, this makes sense, doesn't it?

@s1na: can you directly upgrade Travis config here accordingly?

s1na · 2019-02-19T10:08:28Z

Updated the travis config, tests run against node 8, 10 and 11, lint and coverage against node 8. There's a problem with coveralls currently (other projects are also failing).

I'll rebase after the review.

holgerd77 · 2019-02-19T10:37:16Z

For clarification: main reason for rebase is to get a more comprise commit history, right?

s1na · 2019-02-19T10:41:24Z

Yeah exactly, I've noticed that rebasing makes reviewing harder. I'm now trying to keep the entire commit history, and only rebase at the end to get a more concise commit history for merging.

holgerd77 · 2019-02-19T10:48:40Z

👍

vpulim · 2019-02-19T13:14:49Z

@vpulim: though this hasn't been dropped yet on other libraries, would be a "first" here, maybe you are confusing with Node 4?

Unless I'm misreading the config files, it looks like ethereum-vm (https://github.com/ethereumjs/ethereumjs-vm/blob/master/.circleci/config.yml#L6_ and ethereum-block (https://github.com/ethereumjs/ethereumjs-block/blob/master/.travis.yml#L2) have already dropped testing on Node 6. In any case, I'm glad you're in agreement about dropping Node 6 going forward since it's going to be EOL'd soon and it makes development very difficult for mac users.

package.json

holgerd77 · 2019-02-21T08:37:36Z

src/dbManager.js

+      return value
+    }
+
+    return this._db.get(key, dbOpts)


This is also still operating on this._db?

Well, its the get method, so... I didn't know how to avoid using this._db.get 😅 Open to ideas.

Ah, sorry, put this somewhere else due to all this comments interrupting the code flow. ARgh. 🤣

holgerd77 · 2019-02-21T08:41:28Z

src/dbManager.js

+
+    return new Block([header].concat(body), {common: this._common})
+  }
+}


Can you add JSDoc comments to the publicly exposed functions?

(hmm, second thought: or maybe we'll leave it until integrating auto-generated documentation, up to you)

Will try to add a bit at least for now. This made me think, should all these methods be public or private? On Blockchain it makes sense to have them as private, but they're the core functionality of DBManager 🤔 What do you think?

Ah, I am also getting unsure on various fronts. Wasn't getting aware that DBManager isn't really prepared to go into the public space (right?). Are all publicly documented API methods from the README still kept in the main Blockchain class?

It doesn't go public by default, when users do const Blockchain = require('ethereumjs-blockchain') they'd get the Blockchain class, and they'd have to explicitly import DBManager to get access to these methods. Should be fine.
Yeah, the API hasn't changed and the methods in Blockchain only act as a wrapper for methods in DBManager.

holgerd77 · 2019-02-21T08:45:52Z

Just went through the index.js file, this makes the whole thing already a lot more readable, great! 👍 😄

holgerd77 · 2019-02-21T08:46:47Z

@s1na Once you have addressed these latest changes you can probably already do the rebase to ease the process (?).

Mv _getBlock to DBManager Use get in other DBManager methods Import consts and util fns instead of re-defining Callbackify getHeads in _init Add jsdocs, make dbmanager methods public, re-order them

s1na · 2019-02-21T09:41:32Z

Added some documentation, removed the _ from DBManager methods, re-ordered them in order of importance, and rebased :)

coveralls · 2019-02-21T09:50:52Z

Coverage decreased (-1.5%) to 96.288% when pulling 2caf828 on refactor into 29a337f on master.

holgerd77 · 2019-02-21T09:59:01Z

Thanks! 😄 I have a tendency that we remove the Common dependency from the DBManager class and just return the raw values in _getHeader and _getBlock (and probably also rename the functions) - have the impression that would be cleaner and the interpretation logic belongs more to the Blockchain class - what do you think?

Would it also make sense to expose DBManager (and should we rename to db) similar like in the patricia tree library? Or does this expose too much functionality not totally clear if needed from the outside and puts too much burden on API maintenance?

holgerd77 · 2019-02-21T10:08:12Z

Updated thinking: maybe for now we should stick to the currently exposed API and not prematurely expose whatever stuff and wait till more things have settled (but feel free to express otherwise).

s1na · 2019-02-21T10:14:20Z

To me DBManager is an abstraction above db. DB only concerns itself with raw keys and values, but DBManager is aware of types. One could think about it in terms of model in the MVC paradigm. But probably the name is terrible and doesn't really reflect this. Maybe BlockManager or BlockModel? any other ideas?

Re exposing: Yeah, I'd agree that maybe at least for now keep it un-exposed, until it solidifies a bit more.

holgerd77 · 2019-02-21T10:16:18Z

Thanks, that's a good explanation. Let's stick to this PR "as is" for now, I think it's a solid basis for further work, naming should also be fine.

Will approve and merge.

holgerd77

Looks good.