Expose randomizer lifecycle views by token#66
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.
📝 WalkthroughWalkthroughThis PR adds token-level view functions to expose randomness request state scoped to individual token IDs, validates the complete lifecycle through a comprehensive test, and synchronizes documentation and work-tracking artifacts to reflect completion of randomness lifecycle hardening. ChangesToken-level randomness request views
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
@claude please review this PR. Focus areas:
Local validation is in the PR body; full |
|
@coderabbitai review Please review latest head |
|
@claude please review latest head Only the durable run-state file changed after the initial review request; the Solidity/test scope remains the token-level randomizer lifecycle view helpers and their empty/pending/fulfilled coverage. Local validation remains as summarized in the PR body: focused lifecycle suite 11/11, full |
|
@coderabbitai review Please review latest head |
|
@claude correction: please review latest head The previous latest-head comment expanded the short SHA incorrectly. Scope and validation remain unchanged. |
|
@coderabbitai review Correction: please review latest head |
|
@claude review Please review latest head |
There was a problem hiding this comment.
An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.
Once the cap resets or is raised, comment @claude review on this pull request to trigger a review.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR diff
gh pr view 66 --repo 6529-Collections/6529Stream --json title,body,files,commits,headRefName 2>/dev/null | jq .
echo "=== PR FILES ==="
gh pr diff 66 --repo 6529-Collections/6529Stream 2>/dev/nullLength of output: 25457
Review: Expose randomizer lifecycle views by tokenSolidity —
|
| Stage | Fields validated |
|---|---|
| Empty (pre-mint) | state == None, provider == address(0), randomnessRequestStateForToken == None |
| Pending (post-mint) | providerRequestId, collectionId, tokenId, state == Pending, randomnessRequestStateForToken == Pending |
| Fulfilled (post-callback) | state == Fulfilled, derivedSeed, fulfilledBlock > 0, fulfilledTimestamp > 0, randomnessRequestStateForToken == Fulfilled |
The test correctly wires expectedSeed through the same keccak256(abi.encode(...)) path the contract uses, so the derived-seed assertion is a genuine correctness check, not just a smoke test.
Coverage gap (intentional): Stale-state token-level lookup is not covered here. The PR notes correctly attribute that to #40/#41. No action needed in this PR.
API / indexing ambiguity
No ambiguity is introduced. These are additive, pure view helpers:
tokenToRequestalways holds the current active request ID for a token. Indexers readingretrieveRandomnessRequestForTokenget exactly the same record they would retrieve by first callingtokenToRequest(tokenId)and thenretrieveRandomnessRequest(requestId)— just with one fewer round-trip.- The failed/retry/stale-bulk-migration scenarios (
#40,#41) may eventually require a token-level history view, but that is a separate concern and is not blocked or prejudiced by these helpers.
Docs traceability
All touched doc files are consistent:
known-blockers.md— updated bullet mentions token-level views ✓ops/ROADMAP.md— P0-RAND-002 status updated; P0-RAND-003 closure noted; roadmap table row updated ✓test/README.md— prose updated to include token-level view coverage ✓ops/AUTONOMOUS_RUN.md— operational run-state and decision log updated ✓
Summary
The PR is correct, minimal, and closes #38 cleanly. The only item worth noting (not a blocker) is the mixed parameter-naming style (_requestId vs tokenId). Everything else — implementation, unknown-token parity, test coverage, and traceability — is solid.
[approve]
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the PR details and diff
gh pr view 66 --repo 6529-Collections/6529Stream --json number,title,body,headRefName,headRefOid,files
echo "=== PR FILES ==="
gh pr diff 66 --repo 6529-Collections/6529StreamLength of output: 24313
Review: Token-level Randomizer Lifecycle ViewsImplementation —
|
| Stage | Assertions |
|---|---|
| Pre-mint | state == None, provider == address(0) via both helpers |
| Post-mint | providerRequestId == 1, collectionId, tokenId, state == Pending |
| Post-fulfill | state == Fulfilled, derivedSeed matches inline derivation, fulfilledBlock > 0, fulfilledTimestamp > 0 |
The inline seed derivation in the test (keccak256(abi.encode(address(vrf), 1, COLLECTION_ID, TOKEN_ID, 2, words))) is self-documenting and consistent with what the contract stores.
One gap worth flagging: the Stale state is not covered through the token-level path. It is covered indirectly through existing request-ID tests, but a testTokenLevelViewForStaleRequest (or extending the stale-marking test with a retrieveRandomnessRequestForToken assertion) would make the token-level API complete against all currently-reachable states. This is low-priority since Stale is not new in this PR, but it's the one path not exercised via the new entry points.
API / indexing ambiguity
No new ambiguity is introduced. The helpers return the same RandomnessRequest struct already exposed by request ID; no new state is fabricated. Consumers that observe Fulfilled should be aware that failed post-processing states (#40) are not yet represented in the enum — but that is a pre-existing limitation of the state machine, not something introduced here, and the PR notes it correctly.
Docs traceability
known-blockers.md, ROADMAP.md (P0-RAND-002 status, P0-RAND-003 closure), and test/README.md are all updated consistently and accurately reflect what is now implemented vs. what remains open.
Summary
LGTM. The implementation is minimal, correct, and well-tested for the claimed scope. The one actionable item is covering Stale through the token-level path, which I'd suggest tracking in #40 or as a standalone follow-up rather than blocking this PR.
✅ 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 review Please review latest head |
|
@coderabbitai review Please review latest head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch the PR diff at the latest head
gh pr diff 66 --repo 6529-Collections/6529Stream 2>/dev/nullLength of output: 24086
This follow-up cleanly resolves the only gap I flagged.
|
| State | Test |
|---|---|
None |
testTokenLevelViewsExposeEmptyPendingAndFulfilledState |
Pending |
testTokenLevelViewsExposeEmptyPendingAndFulfilledState |
Fulfilled |
testTokenLevelViewsExposeEmptyPendingAndFulfilledState |
Stale |
testMarkedStaleRequestIsObservableAndCannotFulfill ← added here |
Docs
test/README.md, ops/ROADMAP.md, and ops/AUTONOMOUS_RUN.md are all consistent with the updated coverage. The roadmap test-matrix row now explicitly lists "token-level stale lookup."
LGTM. No further concerns.
[approve]
1 participant
Summary
Closes #38.
This PR completes the request/view portion of the randomizer lifecycle work by making token-level lookup first-class:
retrieveRandomnessRequestForToken(tokenId)to return the stored lifecycle request for a tokenrandomnessRequestStateForToken(tokenId)for direct state reads by tokenStreamRandomizerLifecycleTestThe unknown-token behavior intentionally mirrors unknown request-ID lookup: it returns the default empty request and
Nonestate.Validation
forge test --match-contract StreamRandomizerLifecycleTest -vvv-> 11 tests passed, 0 failedmake check-> 153 tests passed, 0 failedpowershell -ExecutionPolicy Bypass -File scripts/check.ps1-> 153 tests passed, 0 failedforge fmt --check smart-contracts/StreamRandomizerLifecycle.sol test/StreamRandomizerLifecycle.t.sol-> passedgit diff --check-> passedtotal=686,high=9,medium=29,weak-prng=2; no accepted-baseline expansion from these view helpersNotes
Follow-up randomness work remains tracked separately: failed post-processing/retry states (#40), stale provider migration/bulk pending handling (#41), and metadata exposure (#42).
Summary by CodeRabbit
New Features
Documentation
Tests
Chores