Prove vendored library provenance#76
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR documents vendored OpenZeppelin library provenance with SHA-256 manifests, adds focused regression tests for Base64 and Math behaviors, updates Slither high/medium rows to documented false positives, and records those changes in roadmap and autonomous-run documentation. ChangesVendored Library Provenance Documentation and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/StreamVendoredLibraries.t.sol (1)
77-80: ⚡ Quick winConsider validating zero-denominator revert data format.
The overflow test (lines 71-75) validates the exact revert message format using
keccak256, but the zero-denominator test only checks that revert data is non-empty. For consistency and stronger coverage, consider validating the zero-denominator revert format as well.💡 Suggested enhancement
(bool zeroDenominatorSuccess, bytes memory zeroDenominatorRevertData) = address(harness).staticcall(abi.encodeWithSelector(harness.mulDiv.selector, 1, 1, 0)); zeroDenominatorSuccess.assertFalse("zero denominator mulDiv succeeded"); -(zeroDenominatorRevertData.length > 0).assertTrue("zero denominator revert data"); +keccak256(zeroDenominatorRevertData) + .assertEq( + keccak256(abi.encodeWithSignature("Error(string)", "Math: mulDiv zero denominator")), + "zero denominator revert data" + );Note: You'll need to verify the actual revert message used by Solidity for division by zero in Math.mulDiv.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/StreamVendoredLibraries.t.sol` around lines 77 - 80, The zero-denominator test currently only checks that revert data is non-empty; make it assert the exact revert payload like the overflow test: after calling address(harness).staticcall(abi.encodeWithSelector(harness.mulDiv.selector, 1, 1, 0)) use zeroDenominatorRevertData to compare against the expected Solidity revert encoding (e.g. keccak256(abi.encodeWithSignature("Error(string)", "<actual division by zero message>")) wrapped via abi.encodeWithSelector or the appropriate custom error selector if Math.mulDiv uses one); replace the loose non-empty check with an assertion that keccak256(zeroDenominatorRevertData) == keccak256(expectedRevertBytes) so the test validates the exact revert format for harness.mulDiv.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/StreamVendoredLibraries.t.sol`:
- Around line 77-80: The zero-denominator test currently only checks that revert
data is non-empty; make it assert the exact revert payload like the overflow
test: after calling
address(harness).staticcall(abi.encodeWithSelector(harness.mulDiv.selector, 1,
1, 0)) use zeroDenominatorRevertData to compare against the expected Solidity
revert encoding (e.g. keccak256(abi.encodeWithSignature("Error(string)",
"<actual division by zero message>")) wrapped via abi.encodeWithSelector or the
appropriate custom error selector if Math.mulDiv uses one); replace the loose
non-empty check with an assertion that keccak256(zeroDenominatorRevertData) ==
keccak256(expectedRevertBytes) so the test validates the exact revert format for
harness.mulDiv.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81649ebd-612f-4bc5-9285-f42e47c4e8cb
📒 Files selected for processing (9)
docs/known-blockers.mddocs/status.mddocs/vendored-libraries.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdops/SLITHER_BASELINE.mdsmart-contracts/Strings.soltest/README.mdtest/StreamVendoredLibraries.t.sol
1 participant
Summary
docs/vendored-libraries.mdwith OpenZeppelin tag URLs, upstream/local SHA-256s, and local delta notes for retained utility librariesStreamVendoredLibraries.t.solcoverage for Base64 golden/padding vectors andMath.mulDivprecision, rounding, overflow, and zero-denominator behaviorincorrect-exp/divide-before-multiplySlither rows as documented false positives, refresh roadmap/status/test traceability, and correct theStrings.solprovenance header to v4.9.0Closes #11.
Slither note
4 High / 19 Medium.666to668due to lower-impact/informational/optimization churn from the new provenance test/header surface.Validation
forge fmt --check test\StreamVendoredLibraries.t.solforge test --match-path test\StreamVendoredLibraries.t.sol -vvv(5 tests, 0 failed)make check(187 tests, 0 failed)powershell -ExecutionPolicy Bypass -File scripts\check.ps1(187 tests, 0 failed)git diff --checkrg -n "^#|^##|^###" docs\vendored-libraries.md docs\known-blockers.md docs\status.md test\README.md ops\ROADMAP.md ops\SLITHER_BASELINE.md ops\AUTONOMOUS_RUN.mdP0-LIB-001,StreamVendoredLibraries,docs/vendored-libraries.md,False Positive,incorrect-exp,divide-before-multiply, OpenZeppelin v4.7.0/v4.8.0/v4.9.0 tags, and the668 total/4 High and 19 MediumSlither status{"slither_exit":-1,"total":668,"high":4,"medium":19,"low":63,"informational":575,"optimization":7,"incorrect_exp":1,"divide_before_multiply":9,"unused_return":1}Slither still exits nonzero because accepted test-only and documented false-positive rows remain visible, plus lower-impact findings are not yet a CI gate.
Summary by CodeRabbit
Tests
Documentation