feat: add MerkleLib tests #15

mrice32 · 2022-02-01T06:33:11Z

This PR adds MerkleLib tests. To do this, it needed:

A library to construct merkle trees from custom leaf types.
A test contract to proxy calls to the lib and provide storage.

Note: the smart contract changes are only cosmetic, as there was a minor naming inconsistency.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 · 2022-02-01T06:34:41Z

hardhat.config.ts

 import "@typechain/hardhat";
 import "hardhat-gas-reporter";
 import "solidity-coverage";
+import "hardhat-deploy";


To use fixtures, hardhat deploy was needed.

welcome back my old friend

mrice32 · 2022-02-01T06:34:54Z

contracts/test/MerkleLibTest.sol

@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-3.0-only


Test contract

mrice32 · 2022-02-01T06:35:04Z

contracts/MerkleLib.sol

     * @notice Verifies that a repayment is contained within a merkle root.
     * @param root the merkle root.
-     * @param repayment the repayment struct.
+     * @param rebalance the rebalance struct.


Fixed a naming inconsistency

mrice32 · 2022-02-01T06:36:19Z

utils/MerkleTree.ts

@@ -0,0 +1,141 @@
+// This script provides some useful methods for building MerkleTrees. It is essentially the uniswap implementation
+// https://github.com/Uniswap/merkle-distributor/blob/master/src/merkle-tree.ts with some added convenience methods


See this comment for the differences between this file and the one in protocol.

mrice32 · 2022-02-01T06:37:36Z

test/utils.ts

 }

-export async function getContractFactory(name: string, signer: SignerWithAddress) {
+export async function getContractFactory(name: string, signer: Signer) {


I made these normal signers because it seemed like there was nothing in these methods that explicitly required them to be SignerWithAddress (and Signer is directly importable via ethers).

mrice32 · 2022-02-01T06:40:16Z

test/MerkleLib.Fixture.ts

+import { getContractFactory } from "./utils";
+import hre from "hardhat";
+
+export const merkleLibFixture = hre.deployments.createFixture(async ({ deployments }) => {


I wasn't sure if this was the type of fixtures we were wanting to use, but this is the type I'm familiar with (where hardhat keeps a snapshot of the fixture so it doesn't have to redeploy).

ye, so this is why you needed the hardhat-deploy package but @nicholaspai and my "fixtures" thus far have not. we simply created functions that did the deployment, which is not great. we should converge on one standard that does the same thing throughout the repo.

I think what you have here is better, though, as it will actually do the snapshotting and no re-deployment. I can refactor the other "fixtures" once this PR is in.

Sounds good. I think that's the right path to avoid this PR growing too large. I'm also happy to make this change as a follow-up.

mrice32 · 2022-02-01T06:42:27Z

contracts/MerkleLib.sol

+        PoolRebalance memory rebalance,
        bytes32[] memory proof
    ) public pure returns (bool) {
-        return MerkleProof.verify(proof, root, keccak256(abi.encode(repayment))) || true; // Run code but set to true.


Note: I temporarily removed the || true to prove that these tests were working. I can put it back before merging if helpful, but now that we have the MerkleTree ts library, we should be able to construct test data with relative ease. Let me know what y'all prefer!

whatever is easiest!

I'll keep it without the || true. Feel free to add back if needed!

ye, let's avoid this part to force us to use correct trees, generated from this lib. the only place I touch these methods has not yet had any meaningful implementation added so this won't impact anything in the hubPool.

chrismaree · 2022-02-01T22:10:08Z

test/MerkleLib.Claims.ts

+import { Contract, BigNumber } from "ethers";
+
+describe("MerkleLib Claims", async function () {
+  let merkleLibTest: Contract;


nit: in the other test files we've done the declaration of these types of variables outside of the describe block.

Okay, sounds good.

chrismaree · 2022-02-01T22:12:26Z

test/MerkleLib.Fixture.ts

+  const merkleLibTest = await (
+    await hre.ethers.getContractFactory("MerkleLibTest", {
+      signer,
+      libraries: {


can this be linted to fewer lines?

Are you suggesting changing the prettier settings?

just seems like it can be rolled into

... signer, libraries: { MerkleLib: merkleLib.address } ...

one one line.

chrismaree · 2022-02-01T22:17:08Z

test/MerkleLib.Fixture.ts

+  const [signer] = await hre.ethers.getSigners();
+  const merkleLib = await (await getContractFactory("MerkleLib", signer)).deploy();
+  const merkleLibTest = await (
+    await hre.ethers.getContractFactory("MerkleLibTest", {


there is an argument to put this linking deployment within the getContractFactory from utils so you dont need to create your own ethers factory with getContractFactory and then this would be hidden from this fixture. what I describe is more similar to what was done in the hub pool tests. downside with that is the utils needs branching logic based on the contract name to do the associated library linking. curious what pattern you think is best and if it warrants changing what's in the HubPool's fixture, here

Yeah, I think I might've left a comment on that PR (can't remember if I deleted it before sending). I don't love branching on individual contract names as it just gets to be really convoluted after a while. It also requires people writing tests to modify common libraries, which seems like an antipattern.

One compromise between the two approaches is have the second argument mirror the structure of the ethers method, Signer | Options where options could contain the signer and the libraries. Then the user can still use the function, but they can pick the library deployment they want to link.

So I made the change I mention above. PTAL and let me know what you think.

chrismaree · 2022-02-01T22:17:48Z

test/MerkleLib.Claims.ts

+    const claim1499 = BigNumber.from(2).pow(219);
+    const claim1500 = BigNumber.from(2).pow(220);
+    const claim1501 = BigNumber.from(2).pow(221);
+    expect(await merkleLibTest.claimedBitMap(5)).to.equal(claim1499.add(claim1500).add(claim1501));


chrismaree · 2022-02-01T22:19:46Z

test/MerkleLib.Proofs.ts

+function randomBigNumber() {
+  return ethers.BigNumber.from(ethers.utils.randomBytes(31));
+}
+
+function randomAddress() {
+  return ethers.utils.hexlify(ethers.utils.randomBytes(20));
+}


should we put these in the utils file? we've been doing something similar to this for other tests with common utils.

chrismaree · 2022-02-01T22:19:56Z

test/MerkleLib.Proofs.ts

+}
+
+describe("MerkleLib Proofs", async function () {
+  let merkleLibTest: Contract;


same nit about where this should be defined.

chrismaree · 2022-02-01T22:22:23Z

test/MerkleLib.Proofs.ts

+    const proof = merkleTree.getHexProof(poolRebalances[34]);
+    expect(await merkleLibTest.verifyPoolRebalance(root, poolRebalances[34], proof)).to.equal(true);


please test the negative case as well. the test you have here only tests that a valid proof passes but not that an invalid proof returns false.

Ah, good point. Can add.

chrismaree · 2022-02-01T22:23:44Z

test/utils.ts

  amountToSeedWith: number | BigNumber
 ) {
-  for (const token of tokens) await token.mint(walletToFund.address, amountToSeedWith);
+  for (const token of tokens) await token.mint(await walletToFund.getAddress(), amountToSeedWith);


why this change?

SignerWithAddress is more restrictive than Signer. It's pretty easy to get the address from the Signer and the Signer type is much easier to extract from the ethers package, so I think we should prefer using it over the more specific and cumbersome SignerWithAddress when we're building generic library functions.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree

LGTM! thanks for adding this

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

feat: add merkle lib tests

fb73310

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested review from chrismaree and nicholaspai February 1, 2022 06:33

WIP

8c3c9a2

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 commented Feb 1, 2022

View reviewed changes

Merge branch 'master' into add_merkle_lib

2ffe387

nicholaspai approved these changes Feb 1, 2022

View reviewed changes

chrismaree reviewed Feb 1, 2022

View reviewed changes

WIP

4979522

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested a review from chrismaree February 1, 2022 23:25

chrismaree approved these changes Feb 1, 2022

View reviewed changes

WIP

186c895

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 merged commit 7f00eeb into master Feb 2, 2022

mrice32 deleted the add_merkle_lib branch February 2, 2022 02:14

pxrl pushed a commit that referenced this pull request Feb 20, 2024

fix: M03 Permit2OrderLib Is Not Fully Compliant With EIP-712 (#15)

446aae6

		@@ -0,0 +1,141 @@
		// This script provides some useful methods for building MerkleTrees. It is essentially the uniswap implementation
		// https://github.com/Uniswap/merkle-distributor/blob/master/src/merkle-tree.ts with some added convenience methods

		const proof = merkleTree.getHexProof(poolRebalances[34]);
		expect(await merkleLibTest.verifyPoolRebalance(root, poolRebalances[34], proof)).to.equal(true);

feat: add MerkleLib tests #15

feat: add MerkleLib tests #15

Uh oh!

Conversation

mrice32 commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrismaree Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Feb 1, 2022 •

edited

Loading

mrice32 Feb 1, 2022 •

edited

Loading

chrismaree Feb 1, 2022 •

edited

Loading

mrice32 Feb 1, 2022 •

edited

Loading