BinaryTree: extract proof methods #3889
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. |
| const root = putStack[0][0] | ||
| proofTrie.root(root) | ||
| return proofTrie | ||
| return binaryTreeFromProof(_proof) |
There was a problem hiding this comment.
Do we even need these class methods anymore? I'd be inclined to just remove them from the class entirely. @gabrocheleau what do you think?
There was a problem hiding this comment.
They don't make much sense to use. But maybe they satisfy the expectations of a trie interface?
| * @returns The value from the key, or null if valid proof of non-existence. | ||
| */ | ||
| export async function verifyBinaryProof( | ||
| _rootHash: Uint8Array, |
There was a problem hiding this comment.
Why are there _ before these variable names? It looks like they are all used in the code so assuming we don't need this for linting, right?
There was a problem hiding this comment.
Probably put there to satisfy the linter when the method was not yet implemented. can delete the _ now
| const expectedValue = valueNode.values[_key[31]] | ||
| if (!expectedValue) { | ||
| if (value) { | ||
| throw new Error('Proof is invalid') |
There was a problem hiding this comment.
Would it make sense to give more detailed error messaging since we have different possible outcomes (i.e. proof witness does not reach key and proof witness leads to different value or something to that effect)?
| _rootHash: Uint8Array, | ||
| _key: Uint8Array, | ||
| _proof: Uint8Array[], | ||
| ): Promise<Uint8Array | null> { |
There was a problem hiding this comment.
I know we've had similar discussions in the past and can't remember, don't we have a stylistic preference for using undefined instead of null within the monorepo? I have no opinion in this case but just want to make sure we're consistent.
There was a problem hiding this comment.
We should clarify what a null return means -- is it possible the key exists with null as the value? or does returning null mean there is no entry at that key?
BinaryTree methods
.fromProofandverifyBinaryProofcan exist as independent functions and do not need to be called from aBinaryTreeinstance.This PR creates
proof.tsto export two functions:binaryTreeFromProofandverifyBinaryProofand modifies the class methods to call these functions.