-
Notifications
You must be signed in to change notification settings - Fork 89
Conversation
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>
Do you already have got some idea for a release strategy for the latest and short-term future refactoring (+ eventually Should we do an in-between release at some point? Or wait and release everything en-block? If we do two releases would likely be two breaking releases nevertheless, on the other hand things might get more manageable and predictable from the end-user perspective. 🤔 |
I was actually going to comment here and ask for your opinion. I wasn't sure if it's better to go with this first, then migrate to typescript and do the release or go in some other order. What I had in mind was, since the changes are rather major, one option would be to bundle them all together and do a beta release to get it tested a bit before a stable release. And since users would have to migrate, they could do it all at once. But on the other hand, you're right, splitting it in two releases would make it more manageable. What would be better from your point of view? |
I like the idea with the Does this make sense? Or are there any meaningful reasons to release parts of the functionality (as) early (as possible)? |
Agree! |
Just seeing that this actually passes the tests except from some linting errors, is this ready for review or does the @zmitton if you would find some time and could give an evaluation on the changes here that would actually pretty helpful! 😄 |
} | ||
|
||
hash () { | ||
return ethUtil.keccak256(this.serialize()) |
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.
This might be a dumb question, but isnt it weird that serialize
is only defined in the outer "inheriting" objects? If im following correctly, probably best to move serialize in here and then, if necessary, override it in the NullNode class
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.
Yeah, I think what you say makes sense. Will make sure to add at least a bare-bones serialieze
to Node
.
There are probably quite a few more of these points that need to be improved, the design is still not stable.
Seems like a pretty straightforward refactor. I'm no expert, as I found following this codebase pretty tricky for me (with all the callbacks and fancy semaphore stuff) |
The reason the tests are passing is that they're still on |
Hi @s1na, can you maybe have another look here and develop some strategy on how we proceed? This is such a large PR, it would be a pity if it gets forgotten. Can't judge how much work is left here, is it possible to finish? Or can we split up? Thanks! |
Most of the code is re-written. Some test cases also already pass, but not all. So some focused effort could bring this to pass the tests. But now I have a different opinion. I think it was a mistake to re-write everything in one go. But this PR doesn't have to go to waste, we can gradually adopt parts of this in multiple PRs. |
Ok, what you think makes sense. |
@s1na i think we can close this PR since most of the changes have already been implemented except for a |
No we can close it |
This PR rewrites
BaseTrie
andTrieNode
. Currently the new implementation is largely inspired by py-trie and to some extent geth. It's still a work in progress (but its passingtrietests
official tests), and the structure is subject to further change.It defines a sublass of
Node
for all possible types (NullNode
,BranchNode
,ExtensionNode
andLeafNode
) and uses their instances inBaseTrie
.The core
BaseTrie
methods are recursive (as in py-trie), and the goal is to (hopefully) improve readability and maintainability, and to add further complexity only when needed.The internal async methods use
async/await
syntax and the public methods return aPromise
if no callback was provided, so we can slowly migrate toPromise
-based async functions.