Refactor TrieNode, add levelup types #98
Conversation
|
This pull request introduces 3 alerts when merging de8432e into 882799e - view on LGTM.com new alerts:
|
|
Will try to have a look at this during the next 2-3 days, can't promise though. |
|
(review from everyone else welcome!) |
There was a problem hiding this comment.
Changes in the PR are somewhat hard to review or directly comment on, since there is so much code logic shifted around and re-structured. Changes - especially this 3-Class breakdown - make a lot of sense and trie node code is a lot easier to read without all these conditional switches and the code logic directly attached to the corresponding node type.
I've now spent roughly an hour following various code structure parts and checked for equivalency, everything ok so far. One question on having a trie node base class - no blocker though - would give this a go.
| } | ||
|
|
||
| hash(): Buffer { | ||
| return keccak256(this.serialize()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldn't it make sense to keep TrieNode as the parent class (or eventually an abstract class) and keep generic functionality like serialize() or hash() (and eventually some more) there?
There was a problem hiding this comment.
I tried something like that but the only method that can be moved there would be hash(). serialize() seems possible on first glance, but it calls this.raw() which has a different return type based on node type (specifically BranchNode is different than the other two).
On a high-level this PR:
TrieNodeinto 3 classesBranchNode,ExtensionNodeandLeafNode. It also makes the necessary modifications inBaseTrieto make it work with this new structure.I'm still not fully sure about the details of the new types, specially around
EmbeddedNode. That might have to change.The way some of the internal methods modify nodes is very prone to errors (I had to debug a really nasty bug), due to arrays being mutable references.
I think after this I will try to promisify some parts and make the code a bit flatter so it's easier to work with.
Update: Oh and I tested it against ethereumjs-vm, tests pass.