refactor: double hash

[andreivladbrg]

Closes #204 and #205

[PaulRBerg]

I have stopped my review when I saw the binarySearch and sort functions implemented in our code base.

Both OpenZeppelin and Solady offer implementations for this:

• findUpperBound
• searchSorted

There's no point in maintaining mirror implementations of the same code, so we should just import and use one of these two libraries

[andreivladbrg]

Firstly, there is no function for sort implemented by either OpenZeppelin or solady, please review carefully what that function does. Secondly, both versions you mentioned use a uint256 parameter, not bytes32, tho this isn't ultimately the big problem because conversion is possible. The main issue with these functions is that they require a storage parameter, not a memory or calldata parameter, which is what we need.

Was getting this error:

[PaulRBerg]

There are mupltiple sort functions available in Solady - see LibSort.

Then, it is true that the searchSorted function require a storage variable, but why is that a problem?

You can just define a storage array in the Defaults contract, like this:

bytes32[] public leaves = new bytes32[](RECIPIENTS_COUNT);

constructor() {
    leaves[0] = MerkleBuilder.computeLeaf(INDEX1, users.recipient1.addr, CLAIM_AMOUNT);
    leaves[1] = MerkleBuilder.computeLeaf(INDEX2, users.recipient2.addr, CLAIM_AMOUNT);
    leaves[2] = MerkleBuilder.computeLeaf(INDEX3, users.recipient3.addr, CLAIM_AMOUNT);
    leaves[3] = MerkleBuilder.computeLeaf(INDEX4, users.recipient4.addr, CLAIM_AMOUNT);
}

In general, I would like us to prefer to import code rather than copy-paste it, so that we don't have to (i) understand it and (ii) maintain it.

[andreivladbrg]

There are mupltiple sort functions available in Solady - see LibSort.

Did not know about LibSort

(i) understand it and (ii) maintain it.

makes sense, going to remove them

[PaulRBerg]

Perfect, thanks!

[PaulRBerg]

Generally LGTM, let's just add the explanatory comment mentioned below

[andreivladbrg]

I see that you want to remove the _initMerkleTree function, I believe it would be better to keep it because we would have all the logic inside the MERKLE STREAMER header "scope" so that when you read those constants/variables you don't have to switch back and forth to constructor

[PaulRBerg]

I insist on keeping those declarations in the constructor, because they pertain to initializing immutable variables. See my latest commit.

I've also noticed that the constructor was defined in the middle of the contract, and so I've moved it to the top.

I think it's looking good now!

[andreivladbrg]

I insist on keeping those declarations in the constructor, because they pertain to initializing immutable variables

well, in case of LEAVES arrays cannot be constant/immutable

[PaulRBerg]

in case of LEAVES arrays cannot be constant/immutable

Yes, but they are treated as immutables. Let's keep the code like this

[PaulRBerg]

looks good