Add dependency version immutability#85
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.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughConverts DependencyRegistry to immutable per-version dependency records with provenance and typed content hashes; extends registry and core interfaces; StreamCore pins collections to dependency (key, version, contentHash, registry) at create/update and uses pinned values for rendering and freeze-manifest hashing; tests, fixtures, docs, roadmap, and ops updated. ChangesDependency Version Immutability
🎯 4 (Complex) | ⏱️ ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Deployer
participant StreamCore
participant DependencyRegistry
participant FreezeManifest
Deployer->>StreamCore: createCollection(..., dependencyKey)
StreamCore->>DependencyRegistry: latestDependencyVersion(dependencyKey)
DependencyRegistry-->>StreamCore: (version)
StreamCore->>DependencyRegistry: getDependencyScriptContentHashAtVersion(dependencyKey, version)
DependencyRegistry-->>StreamCore: (contentHash)
StreamCore-->>Deployer: emit DependencyVersionPinned(collectionID, dependencyKey, version, contentHash, registry)
Deployer->>StreamCore: updateCollectionInfo(..., dependencyKey) (repin)
StreamCore->>DependencyRegistry: getDependencyScriptAtVersion(dependencyKey, pinnedVersion, index)
StreamCore->>FreezeManifest: include pinned version/contentHash/registry in freeze manifest hash
Possibly related issues
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/StreamMetadataEncoding.t.sol (1)
121-140: ⚡ Quick winExtract duplicated collection-pinning helper into a shared test utility.
_pinCollectionDependencyhere duplicates the same update payload used intest/StreamDependencyRegistry.t.sol. Moving this intotest/helpers/StreamFixture.solwill reduce drift when collection-update fields/constants change.🤖 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/StreamMetadataEncoding.t.sol` around lines 121 - 140, The helper _pinCollectionDependency duplicates the collection update payload used elsewhere; extract its logic into a shared test utility (e.g., add a public/internal helper function in StreamFixture with the same signature and payload constants) and replace the local _pinCollectionDependency implementation with calls to that shared helper; ensure the new helper exposes the same parameters (DeployedStream memory deployed, bytes32 dependencyKey) and uses the same updateCollectionInfo call and script payload so tests in StreamMetadataEncoding.t.sol and StreamDependencyRegistry.t.sol both call the single shared implementation.
🤖 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.
Inline comments:
In `@smart-contracts/StreamCore.sol`:
- Around line 545-555: The _pinCollectionDependency function currently accepts
bytes32(0) or unknown keys because
dependencyRegistry.latestDependencyVersion(...) can return 0 and
getDependencyScriptContentHashAtVersion(...,0) returns the empty-script hash;
add an explicit validation in _pinCollectionDependency to reject zero/unknown
dependency keys by checking that dependencyNameAndVersion != bytes32(0) and that
dependencyRegistry.latestDependencyVersion(dependencyNameAndVersion) != 0 (or
use a registry-provided exists check) and revert with a clear error (e.g.,
"UnknownDependency" or similar) before reading the content hash; reference the
functions dependencyRegistry.latestDependencyVersion and
dependencyRegistry.getDependencyScriptContentHashAtVersion as the locations to
validate against.
---
Nitpick comments:
In `@test/StreamMetadataEncoding.t.sol`:
- Around line 121-140: The helper _pinCollectionDependency duplicates the
collection update payload used elsewhere; extract its logic into a shared test
utility (e.g., add a public/internal helper function in StreamFixture with the
same signature and payload constants) and replace the local
_pinCollectionDependency implementation with calls to that shared helper; ensure
the new helper exposes the same parameters (DeployedStream memory deployed,
bytes32 dependencyKey) and uses the same updateCollectionInfo call and script
payload so tests in StreamMetadataEncoding.t.sol and
StreamDependencyRegistry.t.sol both call the single shared implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98a5fdf6-709e-4ba4-a092-997f704125be
📒 Files selected for processing (12)
docs/known-blockers.mddocs/metadata.mddocs/status.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdsmart-contracts/DependencyRegistry.solsmart-contracts/IDependencyRegistry.solsmart-contracts/IStreamCore.solsmart-contracts/StreamCore.soltest/README.mdtest/StreamDependencyRegistry.t.soltest/StreamMetadataEncoding.t.sol
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
smart-contracts/StreamCore.sol (1)
546-552:⚠️ Potential issue | 🟠 MajorFix
bytes32(0)sentinel drift: reserve it inDependencyRegistrywrite paths (and/or forceStreamCoreto ignore 0-key versions).
DependencyRegistry._createDependencyVersionacceptsbytes32 _collectionDependencyNameand creates versions by doinglatestDependencyVersions[_collectionDependencyName] + 1with norequire(_collectionDependencyName != bytes32(0)), so real dependency versions can be created forbytes32(0).StreamCore._pinCollectionDependencyonly reverts ondependencyNameAndVersion != bytes32(0) && version == 0; whendependencyNameAndVersion == bytes32(0), it will still pinlatestDependencyVersion(bytes32(0))if non-zero—so the intended “no dependency” sentinel can change behavior if a 0-key entry is ever written.- Add an explicit rejection for
bytes32(0)inDependencyRegistry(e.g., in_createDependencyVersion/ public add* entrypoints), and/or hardenStreamCore._pinCollectionDependencyto forceversion = 0(and thus empty-script hash) wheneverdependencyNameAndVersion == bytes32(0).🤖 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 `@smart-contracts/StreamCore.sol` around lines 546 - 552, Reject the bytes32(0) sentinel on write in DependencyRegistry by adding a require(_collectionDependencyName != bytes32(0)) in the creation/add entrypoints (e.g., in _createDependencyVersion and any public add* functions that call it) so no real versions can be recorded under the zero key, and also harden StreamCore._pinCollectionDependency to treat dependencyNameAndVersion == bytes32(0) as “no dependency” by skipping the registry lookup and forcing version = 0 (or returning early) instead of using latestDependencyVersion(bytes32(0)); reference DependencyRegistry._createDependencyVersion, latestDependencyVersions, and StreamCore._pinCollectionDependency.
🤖 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.
Outside diff comments:
In `@smart-contracts/StreamCore.sol`:
- Around line 546-552: Reject the bytes32(0) sentinel on write in
DependencyRegistry by adding a require(_collectionDependencyName != bytes32(0))
in the creation/add entrypoints (e.g., in _createDependencyVersion and any
public add* functions that call it) so no real versions can be recorded under
the zero key, and also harden StreamCore._pinCollectionDependency to treat
dependencyNameAndVersion == bytes32(0) as “no dependency” by skipping the
registry lookup and forcing version = 0 (or returning early) instead of using
latestDependencyVersion(bytes32(0)); reference
DependencyRegistry._createDependencyVersion, latestDependencyVersions, and
StreamCore._pinCollectionDependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9b07f50-c9ba-472b-88da-d4e5fa66e2f6
📒 Files selected for processing (7)
docs/metadata.mdops/AUTONOMOUS_RUN.mdsmart-contracts/StreamCore.soltest/README.mdtest/StreamDependencyRegistry.t.soltest/StreamMetadataEncoding.t.soltest/helpers/StreamFixture.sol
💤 Files with no reviewable changes (1)
- test/StreamMetadataEncoding.t.sol
✅ Files skipped from review due to trivial changes (1)
- test/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/metadata.md
- ops/AUTONOMOUS_RUN.md
|
@coderabbitai review Latest head is |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@smart-contracts/StreamCore.sol`:
- Around line 546-559: _pinCollectionDependency currently pins only version and
content hash but not the registry address, so later reads may resolve against a
mutated dependencyRegistry and break immutability; modify
_pinCollectionDependency to also store the current dependencyRegistry address
into a new or existing mapping (e.g.,
collectionDependencyRegistries[_collectionID]) and emit it if appropriate, then
update all readers (collectionDependencyVersionState(),
retrieveDependencyScript(), retrieveDependencyScriptContentHash() and the code
blocks around the other affected regions noted) to read from that pinned
collectionDependencyRegistries[_collectionID] instead of the global
dependencyRegistry so all lookups use the exact same registry that produced the
pinned version/content-hash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccbdf53b-8921-4502-bf76-966790e6747c
📒 Files selected for processing (13)
docs/known-blockers.mddocs/metadata.mddocs/status.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdsmart-contracts/DependencyRegistry.solsmart-contracts/IDependencyRegistry.solsmart-contracts/IStreamCore.solsmart-contracts/StreamCore.soltest/README.mdtest/StreamDependencyRegistry.t.soltest/StreamMetadataEncoding.t.soltest/helpers/StreamFixture.sol
|
@coderabbitai review Latest head is Local validation evidence is in the PR body: dependency suite 10/10, adjacent metadata suites pass, full |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/StreamDependencyRegistry.t.sol (1)
540-563: 💤 Low valueConsider extracting duplicated hash helpers to a shared test utility.
The
_contentHash,_chunkHash, and_expectedGenerativeScripthelper functions are duplicated betweenStreamDependencyRegistry.t.solandStreamMetadataEncoding.t.sol. While acceptable for test independence, extracting these to a shared test utility contract would reduce duplication and ensure consistency if the hashing logic needs updates.🤖 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/StreamDependencyRegistry.t.sol` around lines 540 - 563, The helper functions _contentHash, _chunkHash, and _expectedGenerativeScript are duplicated across tests; extract them into a shared test utility contract (e.g., TestHashingUtils) and have both StreamDependencyRegistry.t.sol and StreamMetadataEncoding.t.sol inherit or import that utility instead of redefining the functions. Move the implementations of _contentHash and _chunkHash (and _expectedGenerativeScript) into the new utility, keep the same function signatures and DEPENDENCY_* TYPEHASH constants accessible (export or replicate only in the utility if needed), remove the duplicated definitions from both test files, and update the tests to call the shared functions (or inherit the utility contract) so hashing logic stays centralized and consistent.
🤖 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.
Inline comments:
In `@ops/AUTONOMOUS_RUN.md`:
- Line 40: The "Last updated" header entry is stale; update the timestamp string
in the AUTONOMOUS_RUN.md header line labeled "Last updated" to match the most
recent Decision Log entry (e.g., change `2026-06-11 02:17 UTC` to `2026-06-11
02:45 UTC`) so the file header is consistent with the timeline; ensure the exact
timestamp format and backticks are preserved.
---
Nitpick comments:
In `@test/StreamDependencyRegistry.t.sol`:
- Around line 540-563: The helper functions _contentHash, _chunkHash, and
_expectedGenerativeScript are duplicated across tests; extract them into a
shared test utility contract (e.g., TestHashingUtils) and have both
StreamDependencyRegistry.t.sol and StreamMetadataEncoding.t.sol inherit or
import that utility instead of redefining the functions. Move the
implementations of _contentHash and _chunkHash (and _expectedGenerativeScript)
into the new utility, keep the same function signatures and DEPENDENCY_*
TYPEHASH constants accessible (export or replicate only in the utility if
needed), remove the duplicated definitions from both test files, and update the
tests to call the shared functions (or inherit the utility contract) so hashing
logic stays centralized and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdf92927-ee23-45f4-b611-4265a1559e42
📒 Files selected for processing (13)
docs/known-blockers.mddocs/metadata.mddocs/status.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdsmart-contracts/DependencyRegistry.solsmart-contracts/IDependencyRegistry.solsmart-contracts/IStreamCore.solsmart-contracts/StreamCore.soltest/README.mdtest/StreamDependencyRegistry.t.soltest/StreamMetadataEncoding.t.soltest/helpers/StreamFixture.sol
|
@coderabbitai review Latest head is Validation passed locally after the follow-up:
|
|
✅ Action performedFull review finished. |
|
@coderabbitai resume Latest head remains |
|
Merging with CodeRabbit aggregate status treated as stale. Current evidence before merge:
Local validation for the final head is captured in the PR body, including full |
|
✅ Action performedReviews resumed. Review finished.
|
|
✅ Action performedReviews resumed. |
1 participant
Summary
Implements the first P1-META-003 dependency-versioning slice for immutable dependency records and collection dependency pins.
bytes32(0)as the explicit no-dependency sentinel and reserving that key from real dependency writes.collectionDependencyVersionStateplusDependencyVersionPinnedfor indexers/releases.test/helpers/TestHashingUtils.soland updating the autonomous run timestamp.Refs #48
Validation
forge buildforge test --match-contract StreamDependencyRegistryTest -vvv(10 passed)forge test --match-contract "StreamMetadata(Encoding|Freeze)Test" -vvv(10 passed)make checkpowershell -ExecutionPolicy Bypass -File scripts\check.ps1forge fmt --check smart-contracts\DependencyRegistry.sol smart-contracts\IDependencyRegistry.sol smart-contracts\StreamCore.sol smart-contracts\IStreamCore.sol test\StreamDependencyRegistry.t.sol test\StreamMetadataEncoding.t.solforge fmt --check test\helpers\TestHashingUtils.sol test\StreamDependencyRegistry.t.sol test\StreamMetadataEncoding.t.solgit diff --checkrg -n "^#|^##|^###" ops\ROADMAP.md ops\AUTONOMOUS_RUN.md docs\metadata.md test\README.mdtest/helpers/TestHashingUtils.solslither . --config-file slither.config.json --foundry-compile-all --json <temp-file>returned accepted nonzero exit with718total findings, high/medium unchanged at4/19, and selected critical detectors still zero forarbitrary-send-eth,reentrancy-eth,weak-prng,encode-packed-collision, anduninitialized-state.Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests
Ops