-
Notifications
You must be signed in to change notification settings - Fork 445
test: improve Merkle
coverage
#1603
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
base: main
Are you sure you want to change the base?
Conversation
f3e7330
to
ed3e6dd
Compare
src/test/unit/libraries/Merkle.t.sol
Outdated
function test_verifyInclusion_ValidProof() public { | ||
assertValidProofs(); | ||
} | ||
|
||
function test_verifyInclusion_EmptyProofs() public { | ||
proofs = new bytes[](proofs.length); | ||
assertInvalidProofs(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: Adding tests to MerkleBaseTest
will automatically test both sha256 and keccak tree variants.
89a8343
to
e28b08a
Compare
# If enabled, allows internal expectRevert calls to be used in tests. | ||
allow_internal_expect_revert = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows us to test library reverts without needing a harness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good -- just wanna get more intermediate coverage as well
fdc800e
to
ca784b6
Compare
Motivation:
Recent audits identified a lack of direct testing on the
Merkle
library, and we want to fix that.Modifications:
Murky
(the most commonly used test implementation) for root and inclusion proofs.OZ
for inclusion proofs.Result:
More confidence, more coverage.