Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to latest merkletreejs version will break a unit test in MerkleProof.test.js #3813

Closed
pcaversaccio opened this issue Nov 14, 2022 · 9 comments

Comments

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Nov 14, 2022

I just wanted to let you know that if you update to the latest merkletreejs version (0.3.2), the following line will break (at least it did for me :-D):

const badProof = badMerkleTree.getHexProof(badElements[0]);

I realised it since I used the same test setup for my Vyper Snekmake foundry tests. You can fix it by adding the index of 2. See my commit here: pcaversaccio/snekmate@0f66c3e:

const badProof = badMerkleTree.getHexProof(badElements[0], 2);
// or the following will produce the same result as well
const badProof = badMerkleTree.getHexProof(badElements[2], 2);
@pcaversaccio pcaversaccio changed the title Update to latest merkletreejs will break a unit test Update to latest merkletreejs version will break a unit test in MerkleProof.test.js Nov 14, 2022
@frangio
Copy link
Contributor

frangio commented Nov 14, 2022

Do you know what changed? We should change our tests to use our own merkle-tree library now.

@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Nov 14, 2022

Tbh I didn't do a deep-dive since there are no release notes which pisses me somehow off. I think that PR introduced the change: merkletreejs/merkletreejs@8af3c3d, but might be wrong. Tagging @miguelmota for help.

I actually assumed you wanted to switch, therefore I didn't open a PR :).

@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Nov 14, 2022

For the error logs, you can see a failed action here:

2022-11-14T02:10:50.649702Z ERROR foundry_evm::executor::inspector::cheatcodes::ext: stderr err="/home/runner/work/snekmate/snekmate/node_modules/@ethersproject/logger/lib/index.js:247\n        throw
this.makeError(message, code, params);\n        ^\n\nError: types/values length mismatch (count={\"types\":1,\"values\":2},
value={\"types\":[\"bytes32\"],\"values\":[\"0x\",\"0x\"]}, code=INVALID_ARGUMENT, version=abi/5.7.0)\n    at Logger.makeError
(/home/runner/work/snekmate/snekmate/node_modules/@ethersproject/logger/lib/index.js:238:21)\n    at Logger.throwError
(/home/runner/work/snekmate/snekmate/node_modules/@ethersproject/logger/lib/index.js:247:20)\n    at AbiCoder.encode
(/home/runner/work/snekmate/snekmate/node_modules/@ethersproject/abi/lib/abi-coder.js:83:20)\n    at Object
<anonymous> (/home/runner/work/snekmate/snekmate/test/utils/scripts/generate-bad-proof.js:11:32)\n    at
Module._compile (node:internal/modules/cjs/loader:1155:14)\n    at Object.Module._extensions..js
(node:internal/modules/cjs/loader:1209:10)\n    at Module.load (node:internal/modules/cjs/loader:1033:32)\n    at
Function.Module._load (node:internal/modules/cjs/loader:868:12)\n    at Function.executeUserEntryPoint [as runMain
(node:internal/modules/run_main:81:12)\n    at node:internal/main/run_main_module:22:47 {\n  reason: 'types/values length
mismatch',\n  code: 'INVALID_ARGUMENT',\n  count: { types: 1, values: 2 },\n  value: { types: [ 'bytes32' ], values: [ '0x', '0x' ]
}\n}\n"

@pcaversaccio
Copy link
Contributor Author

I actually just realised that the latest version 0.3.2 is not even committed to the repo as the latest version bump is to 0.3.1. merkletreejs/merkletreejs@68566c5.

@miguelmota
Copy link

miguelmota commented Nov 14, 2022

Can you provide a reproducible snippet? there hasn't been any functional changes; only addition of new methods and updated return type as you pointed out in commit, and all the lib tests still pass, so I wouldn't expect there to be any breaking changes but I could be missing something

@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Nov 14, 2022

sure - the following snippet will run with version 0.3.0 but not with 0.3.2:

const { MerkleTree } = require("merkletreejs");
const ethers = require("ethers");

const badElements = ["d", "e", "f"];
const badMerkleTree = new MerkleTree(badElements);

const badProof = badMerkleTree.getHexProof(badElements[0]);
// to make it work with version `0.3.2` I had to add an index value of 2
// const badProof = badMerkleTree.getHexProof(badElements[0], 2);

// eslint-disable-next-line no-undef
process.stdout.write(
  ethers.utils.defaultAbiCoder.encode(["bytes32"], badProof)
);

@miguelmota
Copy link

miguelmota commented Nov 14, 2022

Thanks @pcaversaccio. The latest version should be fixed now. There were some optimization updates recently that I had forgotten went into a release but the tests didn't catch that there was a breaking change so it's been updated now to undo the line that caused the breaking change and additional test has been added.

@miguelmota
Copy link

As frangio pointed out, the oz lib @openzeppelin/merkle-tree will supersede merkletreejs since it'll be more well maintained in the longer term

@pcaversaccio
Copy link
Contributor Author

@miguelmota awesome - just upgraded and can confirm it runs smoothly. Ok, I will upgrade my Merkle proof verification tests to @openzeppelin/merkle-tree once my time permits. Closing the issue now as it's resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants