Harden randomizer request lifecycle#65
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
|
@claude please review this PR. Focus areas:
Local validation is in the PR body; full |
|
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 (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughCentralizes randomness request lifecycle (StreamRandomizerLifecycle), exposes per-collection epochs in StreamCore, refactors VRF/RNG adapters to use lifecycle validation (and add reentrancy guard for ARRNG), marks NXT non-production, adds mocks and tests, and updates docs/ops baselines. ChangesRandomizer Lifecycle and Request Validation
Sequence DiagramsequenceDiagram
participant VrfCoord as MockVrfCoordinator
participant RandomizerVRF as NextGenRandomizerVRF
participant Lifecycle as StreamRandomizerLifecycle
participant StreamCore as StreamCore
VrfCoord->>RandomizerVRF: requestRandomWords(tokenId) -> requestId
RandomizerVRF->>Lifecycle: _recordRandomnessRequest(requestId, collectionId, tokenId, epoch)
VrfCoord->>RandomizerVRF: fulfillRandomWords(requestId, randomWords[])
RandomizerVRF->>Lifecycle: _fulfillRandomnessRequest(requestId, randomWords[])
Lifecycle->>StreamCore: viewRandomizerEpoch(collectionId) (staleness check)
Lifecycle->>StreamCore: (on success) emit RandomnessFulfilled
RandomizerVRF->>StreamCore: setTokenHash(tokenId, derivedSeed)
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 Please review the latest head for randomizer request lifecycle/callback hardening, especially the scoped Slither suppression around |
|
@coderabbitai review Please review latest head |
|
@coderabbitai review Please start a fresh review for latest head |
|
✅ Action performedReview finished.
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and file list
gh pr view 65 --repo 6529-Collections/6529Stream --json title,body,files,commits,headRefName 2>/dev/null | jq '{title,headRefName,files: [.files[].path], commits: [.commits[] | {oid:.oid, message:.messageHeadline}]}'Length of output: 1299 🏁 Script executed: #!/bin/bash
# Get the diff for the PR
gh pr diff 65 --repo 6529-Collections/6529Stream 2>/dev/null | head -3000Length of output: 80946 🏁 Script executed: #!/bin/bash
# Get remaining diff
gh pr diff 65 --repo 6529-Collections/6529Stream 2>/dev/null | tail -n +3001 | head -3000Length of output: 365 🏁 Script executed: #!/bin/bash
cat smart-contracts/RandomizerRNG.solLength of output: 5154 🏁 Script executed: #!/bin/bash
cat smart-contracts/RandomizerVRF.solLength of output: 4950 🏁 Script executed: #!/bin/bash
cat smart-contracts/StreamRandomizerLifecycle.solLength of output: 7123 🏁 Script executed: #!/bin/bash
cat test/StreamRandomizerLifecycle.t.solLength of output: 11838
OverallThe implementation is solid. The shared 1. Randomizer Lifecycle Validation (
|
| Path | Covered |
|---|---|
| VRF pending → fulfilled → duplicate revert | ✅ testVrfRequestRecordsLifecycleAndFulfillmentSetsHashOnce |
| Unknown request ID | ✅ testVrfUnknownAndEmptyFulfillmentsFailClosed |
| Empty random words | ✅ same |
| Stale epoch / replaced provider | ✅ testVrfStaleEpochOrProviderFulfillmentFails |
| Admin-marked stale request | ✅ testMarkedStaleRequestIsObservableAndCannotFulfill |
| Pause does not block fulfillment | ✅ testRandomnessRequestPauseDoesNotBlockVrfFulfillment |
| arRNG full lifecycle | ✅ testArrngRequestAndFulfillmentUseSameLifecycle |
| Reentrant arRNG controller | ✅ testArrngControllerCannotReenterFulfillmentDuringRequest |
| NXT production disablement | ✅ testNxtRandomizerCannotBeConfiguredForProductionCollections |
| Wrong core token-to-collection binding | ✅ testVrfWrongCollectionBindingFailsClosed |
requestId == 0 from arRNG controller |
⬜ Not explicitly tested (malicious controller returning 0 causes _recordRandomnessRequest to revert with UnknownRandomnessRequest(0); the ETH is already sent. This is the inherent write-after-call tradeoff, acceptable for the scoped suppression). |
The requestId == 0 path is the only untested edge; it is a low-priority addition since the guard reverts before writing any state.
Summary
No blocking issues. The lifecycle validation is complete for VRF and arRNG adapters, the reentrancy guard rationale is correct, NXT disablement is sufficient for this PR slice, and the IStreamCore additions carry no regression risk within this codebase. The items above are low-priority hardening and documentation suggestions for follow-on work.
✅ Action performed
Review finished.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
|
@claude please review latest head This follow-up addresses CodeRabbit comment |
|
@coderabbitai review Please review latest head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the diff for the latest head commit specifically
gh pr diff 65 --repo 6529-Collections/6529Stream 2>/dev/null | head -2000
# Also get the specific commit details
gh api repos/6529-Collections/6529Stream/commits/d6b3788bbe820a1dcb4cd8b091110eb067d9ba24 2>/dev/null | jq '{sha, message: .commit.message, files: [.files[].filename]}'Length of output: 84281 🏁 Script executed: #!/bin/bash
# Get the specific files changed in the follow-up commit
gh api repos/6529-Collections/6529Stream/commits/d6b3788bbe820a1dcb4cd8b091110eb067d9ba24 2>/dev/null | jq '.files[] | {filename, additions, deletions, patch}'Length of output: 17521
Verification summary:
The Slither suppression entry, roadmap test matrix, and [approve] |
1 participant
Summary
Closes #37. Refs #14, #38, #39, #41, #43.
This PR implements the first ADR 0005 randomizer callback hardening slice for the current async adapters:
StreamRandomizerLifecyclerequest records, states, views, custom errors, and request/fulfillment/stale eventsStreamCoreand exposes the live randomizer provider/epoch throughIStreamCoresetTokenHashRandomizerNXTas a production-configurable randomizer under ADR 0005Review Follow-Up
CodeRabbit comment
4671138033reported no blockers and suggested low-priority hardening. Commitd6b3788addresses that follow-up by:tokenIdToCollectiontestArrngZeroRequestIdFailsBeforeRecordingLifecycleRemaining Randomness Work
This PR intentionally does not finish every ADR 0005 child ticket. Remaining work stays tracked for deterministic post-processing retry, callback-after-burn behavior, richer metadata state integration, raw-output hash storage policy, provider config/runbooks, request-level reserve accounting, and final
XRandomsscoping/removal.Local Validation
forge test --match-contract "StreamRandomizerLifecycleTest" -vvv-> 10 passedmake check-> 152 passedpowershell -ExecutionPolicy Bypass -File scripts\check.ps1-> 152 passedforge fmt --checkon touched Solidity/test files -> passedgit diff --check-> passedweak-prngrows, noNextGenRandomizerRNG.requestRandomWordsreentrancy-ethrow, and no lifecycle-mocklocked-ether/arbitrary-send-ethrowsNotes For Reviewers
NextGenRandomizerRNG.requestRandomWordshas a scoped Slither suppression forreentrancy-ethandwrite-after-write. The arRNG controller returns the provider request ID from the external payable call, so request state must be recorded after the call. The adapter now uses a local in-progress guard, fulfillment rejects during that window, andtestArrngControllerCannotReenterFulfillmentDuringRequestplustestArrngZeroRequestIdFailsBeforeRecordingLifecyclecover the critical request-submission edges.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Updates