Fix dependency script encoding hashes#71
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 (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds typed per-chunk and aggregated dependency-script hashes, extends the registry interface with chunk/content hash views, refactors StreamCore script retrieval for segment-safe concatenation, adds regression tests verifying hash distinctness and determinism, and updates docs/ops/Slither baselines to reflect fixes. ChangesP0-META-001: Dependency Script Collision-Safe Encoding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 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 Please review the latest head for dependency-script encoding safety and Slither traceability. Areas to focus on:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== DependencyRegistry.sol ==="
cat smart-contracts/DependencyRegistry.sol
echo ""
echo "=== IDependencyRegistry.sol ==="
cat smart-contracts/IDependencyRegistry.sol
echo ""
echo "=== StreamCore.sol (relevant sections) ==="
cat smart-contracts/StreamCore.sol
echo ""
echo "=== StreamMetadataEncoding.t.sol ==="
cat test/StreamMetadataEncoding.t.sol
echo ""
echo "=== Slither baseline ==="
fd -e json slither 2>/dev/null | head -5
fd slither_baseline 2>/dev/null | head -5
fd -e json . docs/ 2>/dev/null | head -10
echo ""
echo "=== ADR 0006 ==="
fd "0006" docs/ 2>/dev/null
fd "ADR" docs/ 2>/dev/null | head -10
fd -e md . docs/adr/ 2>/dev/null 2>&1 || fd -e md . docs/ 2>/dev/null | head -20
echo ""
echo "=== Roadmap ==="
fd -e md ROADMAP 2>/dev/null
fd -e md roadmap 2>/dev/nullLength of output: 41663 🏁 Script executed: #!/bin/bash
echo "=== SLITHER_BASELINE.md ==="
cat ops/SLITHER_BASELINE.md
echo ""
echo "=== ADR 0006 ==="
cat docs/adr/0006-metadata-freeze.md
echo ""
echo "=== ROADMAP.md ==="
cat ops/ROADMAP.md
echo ""
echo "=== StreamFixture.sol ==="
fd StreamFixture 2>/dev/null | head -5
cat test/helpers/StreamFixture.sol 2>/dev/null || fd -e sol StreamFixture 2>/dev/null | xargs cat
echo ""
echo "=== CharacterizationTestBase.sol ==="
cat test/helpers/CharacterizationTestBase.sol 2>/dev/null
echo ""
echo "=== Assertions.sol ==="
cat test/helpers/Assertions.sol 2>/dev/nullLength of output: 156312
SummaryThis PR correctly eliminates the 1. Typed Dependency Chunk/Content Hash SemanticsTypehash strings match usage exactly:
The rolling accumulator in chunksHash = keccak256(abi.encode(chunksHash, getDependencyScriptChunkHash(dependencyNameAndVersion, i)));correctly makes chunk order significant (a reversed chunk array produces a different One note on the zero-chunk case: when 2.
|
| Metric | Before | After |
|---|---|---|
| High | 9 | 8 |
| Medium | 29 | 28 |
encode-packed-collision |
1 | 0 |
uninitialized-local |
11 | 10 |
The P0-META-001 row resolution text is precise: it identifies abi.encode, dependency key, chunk count, chunk index, chunk byte length, and per-chunk content hash as the components, and references test/StreamMetadataEncoding.t.sol as the required regression test.
The roadmap test matrix row for "Dependency script packed encoding" correctly shows Passing with the scope note distinguishing P0 (segment-safe encoding) from P1-META-003 (dependency versioning, provenance, registry identity pinning).
6. ADR 0006 Scope Boundary
The ADR 0006 "Current Behavior" section was updated with this precise language:
retrieveDependencyScriptContentHash(tokenId)exposes a typed dependency content hash for the referenced dependency key. The hash is segment-safe for the current registry content, but it is not a full freeze manifest by itself because it does not pin registry identity, provenance, or immutable version lifecycle.
This is the correct framing. The P0/P1 boundary is clean: this PR closes the encoding ambiguity, and P1-META-003 retains ownership of immutable versions, deprecation semantics, and freeze-manifest pinning. The ADR "Open Follow-Ups" section correctly preserves the P0-META-001 typed hash regression suite as the foundation P1 work will build on.
Minor Observations (Non-blocking)
-
NatSpec absent on new public functions —
getDependencyScriptChunkHash,getDependencyScriptContentHash, andretrieveDependencyScriptContentHashhave no@notice/@param/@returndocs. This will matter before Gate F. -
Zero-chunk dependency hash untested — A test asserting
getDependencyScriptContentHashfor an empty dependency key (0 chunks) returns the deterministic expected value would be a useful completeness check forP1-META-003. -
addDependencyallows post-freeze registry mutation — Acknowledged and correctly deferred toP1-META-003. The content hash correctly reflects the current registry state at query time; callers needing stability must use the immutable versioning planned in P1.
Overall the PR is correct and well-scoped. The typed hash construction is collision-resistant, the rendered output is preserved, the Slither delta is accurate, and the ADR scope boundary is clean.
|
@coderabbitai review Latest head addresses the non-blocking observations from comment
Please re-check the latest head. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== DependencyRegistry.sol ==="
cat smart-contracts/DependencyRegistry.sol
echo ""
echo "=== IDependencyRegistry.sol ==="
cat smart-contracts/IDependencyRegistry.sol
echo ""
echo "=== StreamCore.sol (retrieveDependencyScriptContentHash) ==="
grep -n -A 15 "retrieveDependencyScriptContentHash" smart-contracts/StreamCore.sol
echo ""
echo "=== StreamMetadataEncoding.t.sol ==="
cat test/StreamMetadataEncoding.t.sol
echo ""
echo "=== SLITHER_BASELINE.md ==="
cat ops/SLITHER_BASELINE.mdLength of output: 30322
Observation 1 — NatSpec on new public views ✅
One minor note: the grep for Observation 2 — Zero-chunk dependency content hash coverage ✅
This confirms the degenerate Observation 3 — Slither baseline counts ✅
Unchanged from the prior capture, consistent with a documentation-only + NatSpec + test delta. The PR is correct and complete for the P0-META-001 scope. No new issues identified. [approve] |
1 participant
Summary
DependencyRegistryand exposes the referenced content hash fromStreamCorefor minted tokens.string.concatwhile preserving the rendered generative script output.Scope boundary
This closes the P0 packed/dynamic encoding blocker. Full immutable dependency versioning, registry identity pinning, provenance, deprecation semantics, and freeze-manifest pinning remain in the later P1 metadata/freeze workstream.
CodeRabbit follow-up
CodeRabbit comment
4673171581found the PR correct and well-scoped, with non-blocking observations only. The latest head adds NatSpec for the new public hash views and a zero-chunk dependency hash regression.Validation
forge fmt --check smart-contracts\DependencyRegistry.sol smart-contracts\StreamCore.sol test\StreamMetadataEncoding.t.solforge test --match-contract StreamMetadataEncodingTest -vvvpassed: 3 tests, 0 failedmake checkpassed: 174 tests, 0 failedpowershell -ExecutionPolicy Bypass -File scripts\check.ps1passed: 174 tests, 0 failedgit diff --checkP0-META-001,StreamMetadataEncoding, dependency hash views/typehashes, Slither detector rows, and PR Store raw randomness output hashes #70 merge stateslither_exit=-1,total=685,high=8,medium=28,low=63,informational=580,optimization=6,encode_packed_collision=0,uninitialized_local=10,calls_loop=8Closes #9
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation
Tests
Chores