Block randomizer migration with pending requests#67
Merged
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 failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR implements the ADR-0005 migration-guard policy by introducing lifecycle-aware pending-request tracking and blocking ordinary randomizer provider migration when pending requests exist. The changes span a new ChangesRandomizer migration guard with pending-request blocking
Sequence DiagramsequenceDiagram
participant Admin
participant StreamCore
participant OldRandomizer as OldRandomizer<br/>(IRandomizerLifecycle)
participant StreamRandomizer as StreamRandomizer<br/>(New)
Admin->>StreamCore: addRandomizer(collection, newRandomizer)
StreamCore->>OldRandomizer: try supportsRandomizerLifecycle()
activate OldRandomizer
OldRandomizer-->>StreamCore: true
deactivate OldRandomizer
alt Old Randomizer has pending requests
StreamCore->>OldRandomizer: pendingRandomnessRequests(collection)
OldRandomizer-->>StreamCore: count > 0
StreamCore-->>Admin: revert PendingRandomnessRequests
else No pending requests (or fulfilled/stale)
StreamCore->>OldRandomizer: pendingRandomnessRequests(collection)
OldRandomizer-->>StreamCore: 0
StreamCore->>StreamCore: epoch++, update randomizer
StreamCore-->>StreamRandomizer: new provider active
StreamCore-->>Admin: CollectionRandomizerUpdated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
Contributor
Author
|
@claude review Please review latest head Focus areas:
Local validation passed:
|
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.
Contributor
Author
|
@coderabbitai review Please review latest head Local validation passed:
|
Contributor
Author
|
@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 PR details and file list
gh pr view 67 --repo 6529-Collections/6529Stream --json files,headRefOid,baseRefOid | jq .
# Get the diff
gh pr diff 67 --repo 6529-Collections/6529StreamLength of output: 46414
Review: PR
|
| Test | Covers |
|---|---|
testRandomizerMigrationWithNoPendingRequestsSucceedsAndEmitsEpoch |
Happy path + event evidence |
testRandomizerMigrationWithPendingRequestIsBlocked |
VRF blocks migration |
testFulfilledRequestUnblocksMigrationAndOldDuplicateCannotOverwrite |
Fulfill → migrate → duplicate rejected |
testMarkedStaleRequestUnblocksMigrationAndNewProviderFulfillment |
Stale → migrate → new provider works end-to-end |
testArrngPendingRequestBlocksMigration |
arRNG variant |
testVrfStaleEpochOrProviderFulfillmentFails refactor is correct — using MockRandomizerCore.setRandomizer to bypass the pending-request guard isolates the stale-epoch rejection test from the migration guard. The epoch values shifting from (2, 3) to (1, 2) follow naturally from the mock starting at epoch 1.
_assertCollectionRandomizerUpdated: address(uint160(uint256(log.topics[i]))) is the correct way to extract an indexed address from a raw log topic. Good log verification.
Legacy compatibility ✅
NoopRandomizer, RandomizerNXT, and any other non-lifecycle adapter silently pass the guard — either via the catch block or the !supported early return. Existing synchronous/legacy provider behavior is fully preserved.
Documentation ✅
ADR 0005, known-blockers.md, status.md, ROADMAP.md, and test/README.md are consistently updated. ops/AUTONOMOUS_RUN.md records Queue Item 26 state accurately. Traceability for P0-RAND-005 / PendingRandomnessRequests / pendingRandomnessRequests is thorough.
Summary
Implementation correctly realises ADR 0005's default conservative policy. The only item worth carrying forward is documenting the pendingRandomnessRequests-without-try/catch assumption as a known invariant in the canonical lifecycle ownership follow-up (P0-RAND-005 / future core/coordinator-owned storage work). No blocking concerns.
[approve]
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This was referenced Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #41.
This implements ADR 0005's default randomizer migration policy for lifecycle-aware providers: ordinary
StreamCore.addRandomizermigration is blocked while the current randomizer reports pending randomness requests for the collection.IRandomizerLifecyclefor lifecycle-aware pending request viewsStreamRandomizerLifecycleStreamCore.addRandomizerrevert withPendingRandomnessRequestsbefore advancing the randomizer epoch when the current provider still has pending workCompatibility note
The guard is lifecycle-aware and fails open for old randomizers that do not implement
supportsRandomizerLifecycle(). That preserves existing synchronous/legacy provider compatibility while enforcing the pending-request guard for the VRF/arRNG lifecycle adapters added by the randomness hardening work.Validation
scripts/bootstrap-windows.ps1installed/confirmed Foundryv1.7.1because the app shell did not initially haveforgeon PATHforge test --match-contract StreamRandomizerLifecycleTest -vvvpassed: 16 tests, 0 failedmake checkpassed: build plus 158 tests, 0 failedpowershell -ExecutionPolicy Bypass -File scripts\check.ps1passed: 158 tests, 0 failedforge fmt --check smart-contracts\IRandomizerLifecycle.sol smart-contracts\StreamRandomizerLifecycle.sol smart-contracts\StreamCore.sol test\helpers\CharacterizationTestBase.sol test\StreamRandomizerLifecycle.t.solpassedgit diff --checkpassedP0-RAND-005,PendingRandomnessRequests,pendingRandomnessRequests, branch name, and Queue Item 26slither_exit=-1 total=686 high=9 medium=29 weak-prng=2 arbitrary-send-eth=0 reentrancy-eth=0Follow-up left intentionally out of scope
Summary by CodeRabbit
New Features
Tests
Documentation