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

⚡️ MerkleProof Verify Calldata Version #243

Merged
merged 4 commits into from
May 19, 2022

Conversation

Vectorized
Copy link
Contributor

@Vectorized Vectorized commented May 18, 2022

Description

This is for #242.

After some marinating, I've decided to change verify to take in a calldata array instead of a memory array.

Since MerkleProof is used for saving on-chain storage afterall,
the data is almost certainly likely to be from an external call.

Since Solmate is an opinionated minimalist repo, I'd recommend just including the version that is most likely to be relevant.

Also, sorry for missing the empty trailing space in the previous PR.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Ran forge snapshot?
  • Ran npm run lint?
  • Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

Gas Benchmarks

The gas snapshots will have higher gas because of the call overhead required to test calldata.

Once you factor in the call overhead, these are the numbers:

[PASS] testValidProofSupplied() (gas: 2464)
[PASS] testValidProofSuppliedCalldata() (gas: 2280)
[PASS] testVerifyEmptyMerkleProofSuppliedLeafAndRootDifferent() (gas: 1733)
[PASS] testVerifyEmptyMerkleProofSuppliedLeafAndRootDifferentCalldata() (gas: 1565)
[PASS] testVerifyEmptyMerkleProofSuppliedLeafAndRootSame() (gas: 1727)
[PASS] testVerifyEmptyMerkleProofSuppliedLeafAndRootSameCalldata() (gas: 1537)
[PASS] testVerifyInvalidProofSupplied() (gas: 2460)
[PASS] testVerifyInvalidProofSuppliedCalldata() (gas: 2322)

@Vectorized
Copy link
Contributor Author

Vectorized commented May 18, 2022

Ok, removed the memory version.

There may be a very niche use case for a memory version as seen in

https://github.com/fx-portal/contracts/blob/baed24d22178201bca33140c303e0925661ec0ac/contracts/tunnel/FxBaseRootTunnel.sol

But for their case, I think it's better to just make a custom Merkle library, just like they did.

My take is that the rare few who really need a memory version should be able to modify this implementation to fit their use case.

If anyone still thinks it's better to add in a memory version under verifyMemory, ping me.

@transmissions11
Copy link
Owner

amazing work, lgtm!

@transmissions11 transmissions11 merged commit 34e2edf into transmissions11:v7 May 19, 2022
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

Successfully merging this pull request may close these issues.

2 participants