Store raw randomness output hashes#70
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.
|
@coderabbitai review Please review the raw-output hash storage implementation, especially the event ABI changes, |
|
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)
📝 WalkthroughWalkthroughPR ChangesRandomness Raw Output Hash Storage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 Head moved with a state-only |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find all relevant files
fd -e sol -e md | sort
echo "=== Directory structure ==="
tree -L 4 2>/dev/null || find . -maxdepth 4 -type f | sortLength of output: 6506 🏁 Script executed: #!/bin/bash
# Read the main randomizer contract
fd -e sol | xargs grep -l "RANDOMNESS_SEED_TYPEHASH\|rawOutputHash\|RandomnessFulfilled" 2>/dev/null
echo "=== Find sol files ==="
fd -e solLength of output: 2879 🏁 Script executed: #!/bin/bash
cat smart-contracts/StreamRandomizerLifecycle.solLength of output: 17357 🏁 Script executed: #!/bin/bash
cat smart-contracts/IRandomizerLifecycle.sol
cat smart-contracts/IRandomizer.solLength of output: 866 🏁 Script executed: #!/bin/bash
cat docs/adr/0005-randomness.mdLength of output: 24502 🏁 Script executed: #!/bin/bash
cat test/StreamRandomizerLifecycle.t.solLength of output: 35141 🏁 Script executed: #!/bin/bash
cat test/StreamRandomizerRetry.t.solLength of output: 27111 🏁 Script executed: #!/bin/bash
cat smart-contracts/RandomizerVRF.sol
echo "==="
cat smart-contracts/RandomizerRNG.solLength of output: 13488
Overall AssessmentThe core design is sound and faithfully implements the ADR 0005 seed-and-storage policy. 1.
|
| # | Severity | Area | Finding |
|---|---|---|---|
| 1 | Informational | Contract | ZeroDerivedSeed is unreachable; document as defense-in-depth |
| 2 | Minor | VRF vs RNG | Raw-words event emitted by VRF but not RNG; asymmetric auditability |
| 3 | Design doc | Events | Dual-event on retry success needs indexer documentation |
| 4 | Minor | Adapters | tokenIdToCollection storage round-trip in calculateTokenHash → requestRandomWords |
| 5 | Minor / ADR gap | Interface | IRandomizerLifecycle missing rawOutputHash and request-record views required by ADR 0005 |
| 6 | Test robustness | Tests | Log-scan helpers can overwrite found=true with found=false |
| 7 | Coverage | Tests | No assertion for rawOutputHash == 0 on stale request |
The rawOutputHash storage, TYPEHASH domain separation, and retry/failure preservation are all correctly implemented. The most actionable items are findings #5 (interface gap vs ADR) and #6 (test helper fragility).
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR info and list changed files
gh pr view 70 --repo 6529-Collections/6529Stream --json title,body,headRefName,headRefOid,baseRefName,files | jq '{title,headRefName,headRefOid,baseRefName,files:.files[].path}'Length of output: 2341 🏁 Script executed: #!/bin/bash
# Get the full diff of the PR
gh pr diff 70 --repo 6529-Collections/6529Stream 2>/dev/null | head -3000Length of output: 66795 🏁 Script executed: #!/bin/bash
# List all changed files
gh pr view 70 --repo 6529-Collections/6529Stream --json files | jq -r '.files[].path'Length of output: 401 🏁 Script executed: #!/bin/bash
cat smart-contracts/StreamRandomizerLifecycle.solLength of output: 17357 🏁 Script executed: #!/bin/bash
# Check for rawOutputHash in views
rg -n "rawOutputHash\|retrieveRandomnessRequest\|retrieveRandomnessRequestForToken" smart-contracts/StreamRandomizerLifecycle.solLength of output: 419 🏁 Script executed: #!/bin/bash
# Check TYPEHASH duplication and any import of it from tests
rg -n "RANDOMNESS_SEED_TYPEHASH" --type=solLength of output: 387 🏁 Script executed: #!/bin/bash
# Search for TYPEHASH across all .sol files
rg -n "RANDOMNESS_SEED_TYPEHASH"Length of output: 2277 🏁 Script executed: #!/bin/bash
# Verify _assertRandomnessFulfilled duplication between test files
grep -n "_assertRandomnessFulfilled\|_rawOutputHash\|_expectedSeed" test/StreamRandomizerLifecycle.t.sol test/StreamRandomizerRetry.t.solLength of output: 3658
Review: Store raw randomness output hashes (PR
|
| Event | Indexed params | Non-indexed params |
|---|---|---|
RandomnessFulfilled |
requestId, collectionId, tokenId |
provider, randomizerEpoch, derivedSeed, rawOutputHash |
RandomnessPostProcessingFailed |
requestId, collectionId, tokenId |
provider, randomizerEpoch, derivedSeed, rawOutputHash, failureDataHash |
RandomnessPostProcessingRetried |
requestId, collectionId, tokenId |
provider, randomizerEpoch, retryCount, derivedSeed, rawOutputHash |
RandomnessPostProcessingRetryFailed |
requestId, collectionId, tokenId |
provider, randomizerEpoch, retryCount, derivedSeed, rawOutputHash, failureDataHash |
Cross-checked against keccak256 topic strings in both test files — all match. ✅
RANDOMNESS_SEED_TYPEHASH domain separation — the constant is public on the contract:
bytes32 public constant RANDOMNESS_SEED_TYPEHASH = keccak256(
"6529StreamRandomnessSeed(address provider,uint256 requestId,uint256 collectionId,uint256 tokenId,uint256 randomizerEpoch,bytes32 rawOutputHash)"
);_deriveRandomnessSeed includes it as the first element of abi.encode, which is standard EIP-712-style domain separation. The six request-bound fields (provider, requestId, collectionId, tokenId, randomizerEpoch, rawOutputHash) match the ADR 0005 spec exactly. ✅
request.provider vs address(this) — provider is stored as address(this) at request-record time. _fulfillRandomnessRequest gates on request.provider != address(this) before computing the seed, so the value passed into _deriveRandomnessSeed is always the address that was recorded — correct and consistent. ✅
No-full-raw-words storage — _hashRawRandomWords is private pure; the uint256[] array never touches storage. ✅
rawOutputHash zero-init and preservation — initialized to bytes32(0) in _recordRandomnessRequest, set during _fulfillRandomnessRequest, and never cleared by _setRandomnessPostProcessingFailedState (which only updates state and failureDataHash). Retried and retry-failed paths emit the stored request.rawOutputHash. ✅
RandomnessStale not updated — intentionally omitted because stale requests never received a fulfillment, so there is no rawOutputHash to emit. Correct. ✅
test/StreamRandomizerLifecycle.t.sol / test/StreamRandomizerRetry.t.sol
Coverage — new tests cover: VRF and arRNG rawOutputHash storage on fulfillment, preservation through failed post-processing, preservation through retry success and retry failure, and the post-request changeTokenData mutation test (testVrfSeedIgnoresMutableTokenDataChangedAfterRequest). The mutation test correctly changes token data after the request is recorded and before fulfillment, proving seed derivation is independent of mutable metadata. ✅
Two minor maintainability observations:
-
RANDOMNESS_SEED_TYPEHASHduplicated in test files. The contract already declares this aspublic constant. Both test files redeclare it asprivate constantwith the full string literal. If the string changes in the contract, tests won't catch the mismatch automatically.// Instead of redeclaring, tests could reference the contract constant: bytes32 private constant RANDOMNESS_SEED_TYPEHASH = StreamRandomizerLifecycle.RANDOMNESS_SEED_TYPEHASH; // (requires casting from abstract, or accessing via a deployed adapter instance)
The practical risk is low since the constant is test-visible and the full test suite would break on a hash mismatch, but direct reference would make the dependency explicit.
-
_assertRandomnessFulfilled,_rawOutputHash, and_expectedSeedduplicated between the two test files. These helpers are independently defined. Moving them toCharacterizationTestBaseor a sharedStreamRandomizerTestBasewould eliminate the duplication. Not a correctness issue.
Docs (docs/adr/0005-randomness.md, docs/known-blockers.md, docs/status.md, ops/ROADMAP.md, test/README.md)
All updated sections accurately reflect the implementation: rawOutputHash storage policy, RANDOMNESS_SEED_TYPEHASH seed derivation, event shape changes, and P0-RAND-007 closure. The P0-RAND-007 open follow-up entry is correctly removed from the ADR. ✅
Summary
The core implementation is correct: rawOutputHash is stored, seeds are properly domain-separated, no raw word arrays hit storage, and all events expose both values consistently. The two test-side observations (TYPEHASH duplication, helper duplication) are maintainability nits and don't affect correctness. The PR is clean.
|
@coderabbitai review Please re-review current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the diff from the PR at current head
gh pr diff 70 --repo 6529-Collections/6529Stream 2>/dev/null | head -5000Length of output: 77542 🏁 Script executed: #!/bin/bash
# Read updated contract files
cat smart-contracts/StreamRandomizerLifecycle.sol
echo "=== IRandomizerLifecycle ==="
cat smart-contracts/IRandomizerLifecycle.sol
echo "=== RandomizerRNG ==="
cat smart-contracts/RandomizerRNG.solLength of output: 26449 🏁 Script executed: #!/bin/bash
# Read test files for updated helpers and stale coverage
grep -n "rawOutputHash\|found\|stale\|ZeroDerivedSeed\|defense\|monoton\|_assertRandomness" test/StreamRandomizerLifecycle.t.sol | head -80
echo "==="
grep -n "rawOutputHash\|found\|stale\|ZeroDerivedSeed\|defense\|monoton\|_assertRandomness" test/StreamRandomizerRetry.t.sol | head -80Length of output: 6026
Re-review: Store raw randomness output hashes (PR
|
| Finding | Status |
|---|---|
#1 ZeroDerivedSeed comment |
✅ Resolved |
#2 arRNG raw-word event |
✅ Resolved |
#3 Retry dual-event doc |
✅ Resolved |
#4 Storage round-trip |
Not addressed (minor, not blocking) |
#5 Interface views |
✅ Resolved |
#6 Monotonic log helpers |
✅ Resolved |
#7 Stale zero-hash coverage |
✅ Resolved |
Implementation is correct and complete for the PR's stated objectives. PR is clean.
[approve]
|
@coderabbitai review Please re-check latest head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check what files changed at latest head vs the previously reviewed head
gh pr view 70 --repo 6529-Collections/6529Stream --json files | jq -r '.files[].path'
echo "=== Recent commits ==="
gh pr view 70 --repo 6529-Collections/6529Stream --json commits | jq -r '.commits[-5:] | .[] | "\(.oid[0:8]) \(.messageHeadline)"'
echo "=== Diff stat ==="
gh pr diff 70 --repo 6529-Collections/6529Stream 2>/dev/null | grep "^diff --git" | sortLength of output: 1803 🏁 Script executed: #!/bin/bash
# Check what files changed in f52cd8f specifically (the incremental commit)
git diff --name-only f8d0470b f52cd8f3
echo "=== Full diff of f52cd8f ==="
git diff f8d0470b f52cd8f3Length of output: 5004
No Solidity, tests, or product docs touched. The implementation reviewed and approved at [approve] |
1 participant
Summary
rawOutputHash = keccak256(abi.encode(randomWords))in each randomness request alongside the derived seed, without storing full provider word arrays.RANDOMNESS_SEED_TYPEHASHand request-bound fields: provider, request ID, collection, token, randomizer epoch, andrawOutputHash.RandomnessFulfilled, post-processing failure, retry success, and retry failure events to expose the raw-output hash; fulfillment events now also include provider and epoch context for indexers.IRandomizerLifecyclerequest/state view signatures so monitoring and audit tooling can read request records and raw-output hashes through the lifecycle interface.RequestFulfilledevents from both VRF and arRNG adapters while keeping contract state hash-only.Validation
forge test --match-contract StreamRandomizerLifecycleTest -vvvpassed: 19 tests, 0 failed.forge test --match-contract StreamRandomizerRetryTest -vvvpassed: 10 tests, 0 failed.forge fmt --check smart-contracts\IRandomizerLifecycle.sol smart-contracts\RandomizerRNG.sol smart-contracts\StreamRandomizerLifecycle.sol test\StreamRandomizerLifecycle.t.sol test\StreamRandomizerRetry.t.solpassed.git diff --checkpassed.P0-RAND-007,rawOutputHash, raw-output hash,RANDOMNESS_SEED_TYPEHASH,RequestFulfilled(uint256,uint256[]),IRandomizerLifecycle, retry confirmation, and stale raw hash passed.make checkpassed: 171 tests, 0 failed.powershell -ExecutionPolicy Bypass -File scripts\check.ps1passed: 171 tests, 0 failed.slither_exit=-1,total=687,high=9,medium=29,weak-prng=2,arbitrary-send-eth=0,reentrancy-eth=0,reentrancy-no-eth=0,reentrancy-events=22.Closes #43.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores