Add SimpleMerkleTree #36

ernestognw · 2024-02-23T08:32:04Z

Description

Alternative to #31 with less repeated code.
The SimpleMerkleTree also receives BytesLikes leaves so that the input is consistent with StandardMerkleTree and we can use inheritance to avoid code repetition.

I removed the T extends BytesLike[] for simplicity and because BytesLike[] is the only input supported.

Note that StandardMerkleTree overloaded functions are for backwards compatibility. We can get rid of them if we release a new non-backwards compatible version

src/bytes.ts

src/standard.ts

Amxx · 2024-02-23T10:09:08Z

We would need to find a way to support any[] values. Elements of the leafs are not all BytesLike.
For example, we must be able to do

    const leaves = [
      [0, []],
      [1, ['openzeppelin']],
      [2, ['hello', 'world']],
      [3, ['merkle', 'tree']],
    ];
    const types = [ 'uint256', 'string[]' ];
    const tree = StandardMerkleTree.of(leaves, types);

We should also be able to load dumps that have been produced with older version of the library

EDIT: solved in this comment

Amxx · 2024-02-23T11:32:21Z

There is also something that is bothering me with the way options are stored in the trees.

If possible I'd like to avoid that. I get that its necessary to set the options in SimpleMerkleTree so that hashleaf in the StandardMerkleTree works as part of the SimpleMerkleTree's construction (so before we can set a leafEncoding value in the standard tree ... but maybe there is another way

EDIT: solved in this comment

src/simple.ts

Amxx · 2024-02-23T13:00:45Z

Another think I just realized is broken in this PR is the interface of getMultiProof(leaves: (number | T)[]): MultiProof<string, T>

Amxx · 2024-02-23T19:34:06Z

I iterated on this PR in https://github.com/OpenZeppelin/merkle-tree/tree/support-raw-leaves-inheritance-amxx

Main differences:

Interface MerkleTree<T> and class MerkleTree<T> provide a generic base for all tree types (T is the leaf type)
SimpleMerkleTree extends MerkleTree<BytesLike>
StandardMerkleTree<T extends any[]> extends MerkleTree<T>
Re-introduce support for non BytesLike element in the leaf (with corresponding tests)
Reduce the number of variants to reduce code (this includes removing putting leafEncoding in the options)
Re-add the generic multiproof to keep support for Multiproof<T> in StandardMerkleTree

I think this is looking better than #31

Diff can be seen here

index.test.ts

src/simple.ts

…citelly HexString

src/standard.ts

ernestognw · 2024-02-25T21:49:19Z

src/standard.ts

-    this.hashLookup = Object.fromEntries(
-      values.map(({ value }, valueIndex) => [hex(standardLeafHash(value, leafEncoding)), valueIndex]),
-    );
+    super(tree, values, standardLeafHasher.bind(null, leafEncoding));


Is there any reason to use .bind? I think it's more confusing than:

Suggested change

super(tree, values, standardLeafHasher.bind(null, leafEncoding));

super(tree, values, v => standardLeafHasher(leafEncoding, v));

ernestognw · 2024-02-25T21:49:49Z

src/standard.ts

+    leafEncoding: string[],
+    options: MerkleTreeOptions = {},
+  ): StandardMerkleTree<T> {
+    const [tree, indexedValues] = MerkleTreeImpl.prepare(values, options, standardLeafHasher.bind(null, leafEncoding));


Same as the other

Suggested change

const [tree, indexedValues] = MerkleTreeImpl.prepare(values, options, standardLeafHasher.bind(null, leafEncoding));

const [tree, indexedValues] = MerkleTreeImpl.prepare(values, options, v => standardLeafHasher(leafEncoding, v));

src/merkletree.ts

src/simple.ts

src/standard.test.ts

src/simple.ts

src/merkletree.ts

src/simple.ts

ernestognw · 2024-02-26T00:21:07Z

I reviewed the updates and generally addressed comments because I think they're not too controversial. Added explanations accordingly.

The main differences are:

Each StandardMerkleTree and SimpleMerkleTree now define their format on their own data types. Also MerkleTree was marked as abstract so that dump() remains virtual until implemented.
Changed the leafHasher function in the StandardMerkleTree to an arrow function. I think the alternative bind is confusing because it specifies this as null.
Specialized the errors into InvalidArgumentError and InvariantError. I noticed there was a lot of new Error around when we already had a throwError helper.

At this point, the PR looks good to me. I left a couple of open comments that need comments. Also, appreciate feedback on point number 3 regarding errors. If that's too much to discuss here let's revert to ada28f1

Amxx · 2024-02-26T11:10:00Z

src/merkletree.ts

+  values: { value: T; treeIndex: number }[];
+};
+
+export interface MerkleTree<T> {


Ideally we would have a way to prevent T from being number, as this leads to confusion between leaf value and leaf index.

I tried doing:

export abstract class MerkleTreeImpl<T, _ = Exclude<T, number>> implements MerkleTree<T> {

But it doesn't work as I intended.

Couldn't make it work. I think the best we can do is restrict it at the MerkleTree constructor.

I added an update, it'd be the last thing imo, otherwise LGTM.

For your curiosity, a way in which it's possible to achieve something like Exclude<any, number> is to use conditional types.

function foo<T>(n : T extends number ? never : T): void {} foo(1) // error foo("a") // ok

But this is obscure and I would not recommend it. I'm not even sure I understand why it works. 😛

ernestognw · 2024-02-26T22:47:37Z

The new "snapshot" files don't convince me but I think it's not a big deal given we don't have too many variations.

I guess we can merge this but I guess is another strong argument in favor of Ava as a testing framework.

src/bytes.ts

frangio

Mostly minor comments but also I think one blocker. Feel free to ignore any nitpicks.

src/core.ts

src/simple.ts

src/merkletree.ts

src/bytes.ts

Simple MerkleTree alternative

302fa2d

Amxx changed the base branch from master to support-raw-leaves February 23, 2024 09:30

Amxx changed the base branch from support-raw-leaves to master February 23, 2024 09:37