binarytree: create binarytree package #3857
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
| this.DEBUG && this.debug(`Deleting stem node for stem: ${bytesToHex(stem)}`, ['put']) | ||
| putStack.push([this.merkelize(stemNode), null]) | ||
| } else { | ||
| return // nothing to delete |
There was a problem hiding this comment.
We should check if it's a deletion first and only then handle creating or updating the stem node.
f6b0b73 to
e0b215f
Compare
ba8bd14 to
a7d9f0d
Compare
| protected _root: Uint8Array | ||
|
|
||
| protected DEBUG: boolean | ||
| protected _debug: Debugger = debug('binarytree:#') |
There was a problem hiding this comment.
why the "#" on the end here?
There was a problem hiding this comment.
This is the new logging convention we use for the base topic for any package (so mpt:#, evm:#, etc). There was some screwiness with doing DEBUG=ethjs,evm* as opposed to DEBUG=ethjs,evm:* where subtopics weren't getting logged or something.
| return value !== null | ||
| } catch (error: any) { | ||
| if (error.message === 'Missing node in DB') { | ||
| return equalsBytes(root, this.EMPTY_TREE_ROOT) |
There was a problem hiding this comment.
Is the idea that checkRoot(this.EMPTY_TREE_ROOT) should return true? as written it will not, because db.get won't throw an error looking for it, but also wont find anything there, so this will return false
There was a problem hiding this comment.
I'm actually not sure here. I think this might have been copied over from another trie class since I don't know if we use this anywhere. We can definitely fix though.
There was a problem hiding this comment.
I am not sure and this is untested behavior, but I think we have a similar setup here than in the MPT, where we are putting and retrieving null from the DB even though it's not typed as such.
There was a problem hiding this comment.
Anyhow let's keep that change, we can revisit this whole handling once we have more clarity on how to handle deleted values etc.
There was a problem hiding this comment.
Added a few tests to boost the coverage.
This looks great! Giving my approval 👍
I gather that there are unanswered questions around how tree.del should work?
And, of course, the proof methods and whatnot need to be filled in. Are those defined yet in the spec, or is that still being worked on?
This PR creates a new binarytree package for implementing binary trees as described in https://eips.ethereum.org/EIPS/eip-7864