Add collection metadata URI policy guard#114
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 (17)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughContracts replace string reverts with typed custom errors, add collection/library/script URI safety checks via StreamMetadataRenderer helpers, update tests to assert error selectors, refresh docs/ops to record enforcement and size headroom, and regenerate deployment/release verification artifacts to reflect ABI/bytecode changes. ChangesCollection URI Validation and Custom Error Enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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 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.
Actionable comments posted: 1
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)
473-493:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPersist
_newCollectionBaseURIin the full-update branch.
_requireCollectionInfoLimits(...)validates_newCollectionBaseURI, but the_FULL_COLLECTION_UPDATE_INDEXpath never writes it back to storage. That makes the "full" update silently keep the old base URI unless the caller makes a second_BASE_URI_UPDATE_INDEXcall.Suggested fix
collectionInfo[_collectionID].collectionDescription = _newCollectionDescription; collectionInfo[_collectionID].collectionWebsite = _newCollectionWebsite; collectionInfo[_collectionID].collectionLicense = _newCollectionLicense; + collectionInfo[_collectionID].collectionBaseURI = _newCollectionBaseURI; collectionInfo[_collectionID].collectionLibrary = _newCollectionLibrary; collectionInfo[_collectionID].collectionDependencyScript = _newCollectionDependencyScript;🤖 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 473 - 493, The full-update branch guarded by _FULL_COLLECTION_UPDATE_INDEX calls _requireCollectionInfoLimits(...) including _newCollectionBaseURI but does not persist it; update the branch to assign collectionInfo[_collectionID].collectionBaseURI = _newCollectionBaseURI (next to the other collectionInfo assignments) so the base URI is stored during a full update, and ensure any related pinning or events used for base URI updates are invoked if your contract expects them.
🤖 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 359-361: Your interface probes currently call
IRandomizer(_randomizerContract).isRandomizerContract() and
IStreamAdmins(_newContract).isAdminContract() /
IStreamMinter(_newContract).isMinterContract() directly, which can revert during
ABI decoding for EOAs, address(0) or non-conforming contracts; change each check
to first gate with _addr.code.length == 0 and then perform a low-level
staticcall to the interface selector (IYourInterface.yourCheck.selector), verify
success and that returndata.length >= 32 before abi.decode, and revert with the
existing typed errors (InvalidRandomizerContract, InvalidAdminContract,
InvalidMinterContract) when the code length is zero, the staticcall fails,
returndata is too short, or the decoded bool is false; update the checks around
IRandomizer.isRandomizerContract, IStreamAdmins.isAdminContract and
IStreamMinter.isMinterContract accordingly.
---
Outside diff comments:
In `@smart-contracts/StreamCore.sol`:
- Around line 473-493: The full-update branch guarded by
_FULL_COLLECTION_UPDATE_INDEX calls _requireCollectionInfoLimits(...) including
_newCollectionBaseURI but does not persist it; update the branch to assign
collectionInfo[_collectionID].collectionBaseURI = _newCollectionBaseURI (next to
the other collectionInfo assignments) so the base URI is stored during a full
update, and ensure any related pinning or events used for base URI updates are
invoked if your contract expects them.
🪄 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: b648b2f0-2a34-4ede-b974-8b4e48815b59
📒 Files selected for processing (23)
CHANGELOG.mddeployments/address-books/anvil-6529stream-v0.1.0-001-broadcast.jsondeployments/address-books/anvil-6529stream-v0.1.0-001.jsondeployments/examples/anvil-6529stream-v0.1.0-001-broadcast.jsondeployments/examples/anvil-6529stream-v0.1.0-001.jsondocs/adr/0006-metadata-freeze.mddocs/known-blockers.mddocs/metadata.mddocs/status.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/abi-checksums.jsonrelease-artifacts/latest/release-artifact-manifest.jsonrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/release-manifest.jsonrelease-artifacts/latest/source-verification-inputs.jsonsmart-contracts/StreamCore.solsmart-contracts/StreamMetadataRenderer.soltest/README.mdtest/StreamMetadataFreeze.t.soltest/StreamMetadataGolden.t.soltest/StreamMetadataUriPolicy.t.sol
|
Review response for CodeRabbit on latest head
Validation on the final local tree:
@coderabbitai review |
|
One thing worth keeping an eye on: EIP-170 headroom has tightened from ~228 bytes (PR open) to ~61 bytes after this round of fixes (24,515 / 24,576 byte limit). That's still within bounds, but any future custom error, function, or non-trivial logic addition will need careful size budgeting or an offsetting optimization. You may want to track this as a near-term blocker in the roadmap. Triggering a full incremental review of head ✅ Action performedReview finished.
|
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 (2)
smart-contracts/StreamCore.sol (2)
325-339:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake zero-supply handling explicit.
_collectionTotalSupply == 0bypasses the new guard and then underflows at Line 339, so this path reverts withPanic(0x11)instead of a contract-defined error. Reject zero up front, or mirror_finalizeCollectionSupply()if zero-supply collections are meant to be representable.🤖 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 325 - 339, The code allows _collectionTotalSupply == 0 which bypasses the > _COLLECTION_TOKEN_RANGE guard and later causes an underflow when computing reservedMaxTokensIndex; either explicitly reject zero or mirror _finalizeCollectionSupply() behavior. Add an explicit check for _collectionTotalSupply == 0 and revert with a clear error (e.g., CollectionSupplyZero) before writing into collectionAdditionalData, or if zero-supply collections are intended, apply the same initialization logic used in _finalizeCollectionSupply() (adjust reservedMaxTokensIndex calculation and setFinalSupplyTimeAfterMint handling) to avoid the underflow; locate the check near where _collectionTotalSupply, _COLLECTION_TOKEN_RANGE, collectionAdditionalData[_collectionID], reservedMinTokensIndex and reservedMaxTokensIndex are set.
676-680:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate dependency-registry swaps too.
_opt == 3is now the only contract-update branch that still accepts any address. Pointing it ataddress(0)or an EOA will make_pinCollectionDependency()fail later at Lines 687-699, which can block future collection create/update flows until an admin repairs the config. Add at least acode.lengthguard here, and preferably a typed registry probe if the interface exposes one.🤖 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 676 - 680, The branch handling _opt == 3 assigns dependencyRegistry = IDependencyRegistry(_newContract) without validating the address; add a check that _newContract.code.length > 0 and (if available) call a lightweight registry probe (e.g., a view function on IDependencyRegistry such as supportsInterface/registryVersion or a no-op getter) before casting to IDependencyRegistry to ensure it's a contract that implements the expected interface; keep the existing frozenCollectionCount guard and revert with a clear error if validation fails so _pinCollectionDependency() cannot later be broken by an EOA or address(0).
🤖 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 325-339: The code allows _collectionTotalSupply == 0 which
bypasses the > _COLLECTION_TOKEN_RANGE guard and later causes an underflow when
computing reservedMaxTokensIndex; either explicitly reject zero or mirror
_finalizeCollectionSupply() behavior. Add an explicit check for
_collectionTotalSupply == 0 and revert with a clear error (e.g.,
CollectionSupplyZero) before writing into collectionAdditionalData, or if
zero-supply collections are intended, apply the same initialization logic used
in _finalizeCollectionSupply() (adjust reservedMaxTokensIndex calculation and
setFinalSupplyTimeAfterMint handling) to avoid the underflow; locate the check
near where _collectionTotalSupply, _COLLECTION_TOKEN_RANGE,
collectionAdditionalData[_collectionID], reservedMinTokensIndex and
reservedMaxTokensIndex are set.
- Around line 676-680: The branch handling _opt == 3 assigns dependencyRegistry
= IDependencyRegistry(_newContract) without validating the address; add a check
that _newContract.code.length > 0 and (if available) call a lightweight registry
probe (e.g., a view function on IDependencyRegistry such as
supportsInterface/registryVersion or a no-op getter) before casting to
IDependencyRegistry to ensure it's a contract that implements the expected
interface; keep the existing frozenCollectionCount guard and revert with a clear
error if validation fails so _pinCollectionDependency() cannot later be broken
by an EOA or address(0).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd45a52c-923d-4961-8510-cc5d3a3b70b5
📒 Files selected for processing (15)
CHANGELOG.mddeployments/address-books/anvil-6529stream-v0.1.0-001-broadcast.jsondeployments/address-books/anvil-6529stream-v0.1.0-001.jsondeployments/examples/anvil-6529stream-v0.1.0-001-broadcast.jsondeployments/examples/anvil-6529stream-v0.1.0-001.jsonops/AUTONOMOUS_RUN.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/abi-checksums.jsonrelease-artifacts/latest/release-artifact-manifest.jsonrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/release-manifest.jsonrelease-artifacts/latest/source-verification-inputs.jsonsmart-contracts/StreamCore.solsmart-contracts/StreamMetadataRenderer.soltest/StreamMetadataUriPolicy.t.sol
✅ Files skipped from review due to trivial changes (6)
- release-artifacts/latest/release-artifact-manifest.json
- deployments/address-books/anvil-6529stream-v0.1.0-001.json
- deployments/examples/anvil-6529stream-v0.1.0-001-broadcast.json
- release-artifacts/latest/abi-checksums.json
- release-artifacts/latest/source-verification-inputs.json
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (7)
- deployments/address-books/anvil-6529stream-v0.1.0-001-broadcast.json
- release-artifacts/latest/SHA256SUMS
- deployments/examples/anvil-6529stream-v0.1.0-001.json
- release-artifacts/latest/release-manifest.json
- smart-contracts/StreamMetadataRenderer.sol
- test/StreamMetadataUriPolicy.t.sol
- ops/AUTONOMOUS_RUN.md
|
Follow-up on CodeRabbit's bytecode-headroom note, latest head
Validation on the documentation-only follow-up:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh issue view 115 --repo 6529-Collections/6529Stream --json number,title,body,state,urlLength of output: 2126
The three changes on this head are documentation-only and correctly scoped: roadmap gets the P1-SIZE-001 item and the 61-byte figure, run-state records the decision rationale, and no release artifact drift is expected. All looks good. Triggering the incremental review of (ᐢ·ᴗ·ᐢ) ✅ Action performedReview finished.
|
|
Follow-up on CodeRabbit outside-diff findings, latest head
Validation:
@coderabbitai review |
|
One thing to note: EIP-170 headroom has tightened further — from 61 bytes (head Triggering the incremental review of ✅ Action performedReview finished.
|
1 participant
Summary
UnsafeMetadataURI().StreamCorestring reverts with custom errors to keep the added enforcement deployable under EIP-170 and improve selector-based assertions.InvalidDependencyRegistryContract().StreamCorebytecode margin in issue Recover StreamCore bytecode headroom before further feature work #115,docs/status.md, andops/ROADMAP.md.Validation
forge build --sizes --via-ir --skip test --skip script --forceforge test --match-path test\StreamMetadataUriPolicy.t.sol -vvvforge test --match-path test\StreamMetadataFreeze.t.sol -vvvforge test --match-path test\StreamMetadataGolden.t.sol -vvvforge test --match-path test\StreamRandomizerLifecycle.t.sol -vvvforge test --match-path test\StreamInitialization.t.sol -vvvforge test --match-path test\StreamDependencyRegistry.t.sol -vvvmake release-checksumsmake checkpowershell -ExecutionPolicy Bypass -File scripts\check.ps1forge fmt --checkfor touched Solidity/test filesgit diff --checkAfter the latest review follow-up,
StreamCoreis 24,545 runtime bytes, leaving 31 bytes of EIP-170 headroom. Follow-up issue #115 tracks recovering sustainable Core bytecode margin before further non-trivial Core feature work.Refs #51
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores