Add failed randomizer post-processing state#68
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.
|
@claude review |
|
@coderabbitai review |
|
@claude review |
|
@coderabbitai review |
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.
✅ Action performedReview finished.
|
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a FailedPostProcessing lifecycle state: adapters guard GenCore token-hash writes with try/catch, transition requests to FailedPostProcessing on revert while recording derived seed and failureDataHash, clear pending counts, emit RandomnessPostProcessingFailed, and add tests, mocks, ADR/docs, and ops updates reflecting the behavior. ChangesFailed Post-Processing State Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/mocks/MockRandomizerCore.sol (1)
26-31: ⚡ Quick winAlign mock
setTokenHashsemantics withStreamCoreto avoid false-positive tests.
setTokenHashcurrently allows any caller and silent overwrites. Since this mock already tracks randomizer and token collection mappings, mirroring core guards will keep lifecycle tests realistic and prevent masking authorization/overwrite regressions.Suggested patch
function setTokenHash(uint256, uint256 tokenId, bytes32 tokenHash) external { if (rejectTokenHash) { revert MockTokenHashRejected(); } + uint256 collectionId = tokenCollections[tokenId]; + require(msg.sender == randomizerContracts[collectionId], "unauthorized randomizer"); + require(tokenHashes[tokenId] == bytes32(0), "token hash already set"); tokenHashes[tokenId] = tokenHash; }🤖 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 `@test/mocks/MockRandomizerCore.sol` around lines 26 - 31, The mock's setTokenHash presently allows any caller and silently overwrites tokenHashes, which diverges from StreamCore; update setTokenHash to enforce the same authorization and overwrite guards as StreamCore by (1) requiring msg.sender is the registered randomizer for the token's collection (use the mock's collection->randomizer mapping or randomizerForCollection lookup) and (2) preventing silent overwrites by reverting if tokenHashes[tokenId] is already set (unless a dedicated overwrite flag exists), while preserving the existing rejectTokenHash revert path; update references to tokenHashes, rejectTokenHash and the mock's collection/randomizer mappings in setTokenHash accordingly.
🤖 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/StreamRandomizerLifecycle.sol`:
- Around line 79-85: The RandomnessPostProcessingFailed event is missing
provider and randomizerEpoch context; update the RandomnessPostProcessingFailed
event declaration to include address provider and uint256 randomizerEpoch (in
the same indexed/not-indexed pattern as other lifecycle events) and then update
every emission site that fires RandomnessPostProcessingFailed (and the related
duplicate declarations at the other occurrence of the event) to pass the current
provider and randomizerEpoch values so indexers can correlate failures without
extra storage lookups; search for RandomnessPostProcessingFailed and add the two
arguments to the event signature and all emit calls.
---
Nitpick comments:
In `@test/mocks/MockRandomizerCore.sol`:
- Around line 26-31: The mock's setTokenHash presently allows any caller and
silently overwrites tokenHashes, which diverges from StreamCore; update
setTokenHash to enforce the same authorization and overwrite guards as
StreamCore by (1) requiring msg.sender is the registered randomizer for the
token's collection (use the mock's collection->randomizer mapping or
randomizerForCollection lookup) and (2) preventing silent overwrites by
reverting if tokenHashes[tokenId] is already set (unless a dedicated overwrite
flag exists), while preserving the existing rejectTokenHash revert path; update
references to tokenHashes, rejectTokenHash and the mock's collection/randomizer
mappings in setTokenHash accordingly.
🪄 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: b33a945c-d53d-4efa-babe-e08caaa91f23
📒 Files selected for processing (11)
docs/adr/0005-randomness.mddocs/known-blockers.mddocs/status.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdsmart-contracts/RandomizerRNG.solsmart-contracts/RandomizerVRF.solsmart-contracts/StreamRandomizerLifecycle.soltest/README.mdtest/StreamRandomizerLifecycle.t.soltest/mocks/MockRandomizerCore.sol
| event RandomnessPostProcessingFailed( | ||
| uint256 indexed requestId, | ||
| uint256 indexed collectionId, | ||
| uint256 indexed tokenId, | ||
| bytes32 derivedSeed, | ||
| bytes32 failureDataHash | ||
| ); |
There was a problem hiding this comment.
Include provider and epoch in RandomnessPostProcessingFailed.
This is a new lifecycle transition event, but it omits provider and randomizerEpoch. The linked issue requires state-transition events to carry that context, and without it indexers have to do an extra storage lookup to correlate failures across adapter/epoch changes.
Proposed diff
event RandomnessPostProcessingFailed(
uint256 indexed requestId,
uint256 indexed collectionId,
uint256 indexed tokenId,
+ address provider,
+ uint256 randomizerEpoch,
bytes32 derivedSeed,
bytes32 failureDataHash
);
@@
emit RandomnessPostProcessingFailed(
- _requestId, request.collectionId, request.tokenId, request.derivedSeed, failureDataHash
+ _requestId,
+ request.collectionId,
+ request.tokenId,
+ request.provider,
+ request.randomizerEpoch,
+ request.derivedSeed,
+ failureDataHash
);Also applies to: 262-264
🤖 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/StreamRandomizerLifecycle.sol` around lines 79 - 85, The
RandomnessPostProcessingFailed event is missing provider and randomizerEpoch
context; update the RandomnessPostProcessingFailed event declaration to include
address provider and uint256 randomizerEpoch (in the same indexed/not-indexed
pattern as other lifecycle events) and then update every emission site that
fires RandomnessPostProcessingFailed (and the related duplicate declarations at
the other occurrence of the event) to pass the current provider and
randomizerEpoch values so indexers can correlate failures without extra storage
lookups; search for RandomnessPostProcessingFailed and add the two arguments to
the event signature and all emit calls.
There was a problem hiding this comment.
Fixed in 47754b2: RandomnessPostProcessingFailed now includes provider and randomizerEpoch, all emit sites pass the stored request context, tests decode/assert both fields, and ADR/roadmap/test/run-state docs were updated.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
FailedPostProcessinglifecycle state when deterministic core hash-writing fails after provider output has been acceptedCloses #40
Validation
forge test --match-contract StreamRandomizerLifecycleTest -vvv(18 tests)make check(160 tests)powershell -ExecutionPolicy Bypass -File scripts\check.ps1(160 tests)forge fmt --check smart-contracts\RandomizerVRF.sol smart-contracts\RandomizerRNG.sol smart-contracts\StreamRandomizerLifecycle.sol test\StreamRandomizerLifecycle.t.sol test\mocks\MockRandomizerCore.solgit diff --checkP0-RAND-004,FailedPostProcessing,failureDataHash,RandomnessPostProcessingFailed, and the active work itemStatic Analysis
.venv-tools\Scripts\slither.exe . --config-file slither.config.json --foundry-compile-all --json <temp-file>slither_exit=-1 total=685 high=9 medium=29 weak-prng=2 arbitrary-send-eth=0 reentrancy-eth=0 reentrancy-no-eth=0 reentrancy-events=22Notes
P0-RAND-006/ [P0-RAND-006] Add bounded manual retry for deterministic randomness post-processing failures #42.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation