Add bounded randomizer post-processing retry#69
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.
|
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 (4)
📝 WalkthroughWalkthroughImplements bounded admin-gated retry for deterministic randomness post-processing failures (P0-RAND-006): adds retry counters, a max-attempt constant, adapter entrypoints, success/failure events with retry metadata, lifecycle retry handlers, tests covering VRF/ARRNG cases, and documentation/ops updates reflecting the new behavior. ChangesPost-processing Retry Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 bounded deterministic randomness post-processing retry implementation, especially the lifecycle state transitions around retry success/failure, provider/epoch validation, and the Slither reentrancy suppressions around the deterministic core write. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get PR overview and files changed
gh pr view 69 --repo 6529-Collections/6529Stream --json files,additions,deletions,title,body 2>/dev/null | jq '{title, additions, deletions, files: [.files[].path]}'Length of output: 730 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Fetch the full diff for the PR
gh pr diff 69 --repo 6529-Collections/6529Stream 2>/dev/null | head -3000Length of output: 56232 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Full lifecycle contract
cat smart-contracts/StreamRandomizerLifecycle.solLength of output: 15470 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Full retry test for completeness
cat test/StreamRandomizerRetry.t.solLength of output: 18623
Review:
|
| Area | Verdict |
|---|---|
| Reentrancy suppressions | ✅ Correct and tightly scoped |
| Lifecycle state transitions | ✅ Correct; pending counts updated exactly once |
| Provider/epoch/binding validation | ✅ All checks precede state mutation |
| Retry count persistence | ✅ Incremented before external call |
| Dual-event emission on retry failure | RandomnessPostProcessingFailed fires on each retry failure |
fulfilledBlock/fulfilledTimestamp staleness |
🔵 Low — not refreshed on retry success |
Redundant state re-check in _confirm |
🔵 Low — safe but costs extra SLOAD |
| arRNG binding/limit test parity | 🔵 Informational |
The critical safety properties hold: no new randomness is accepted, stored seed is reused, provider/epoch bindings are live-validated before any state change, and reentrancy is properly closed by the pre-external-call Fulfilled state. The dual-event emission is the only item worth actively resolving before audit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ops/ROADMAP.md (1)
19-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale “deterministic randomness retry” from remaining P0 blockers.
Line 21 still lists deterministic randomness retry as a remaining blocker, but this same file now marks P0-RAND-006 as implemented/passing (for example in Appendix B). Please update this top-level blocker sentence to avoid contradictory status reporting.
🤖 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 `@ops/ROADMAP.md` around lines 19 - 23, Update the top-level blockers sentence in ROADMAP.md by removing the phrase "deterministic randomness retry" so it no longer contradicts the implemented/passing status of P0-RAND-006 (see Appendix B); locate the sentence containing "Known remaining P0 blockers" and edit it to omit that specific blocker while keeping the rest of the list intact and grammatically correct.
🤖 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.
Outside diff comments:
In `@ops/ROADMAP.md`:
- Around line 19-23: Update the top-level blockers sentence in ROADMAP.md by
removing the phrase "deterministic randomness retry" so it no longer contradicts
the implemented/passing status of P0-RAND-006 (see Appendix B); locate the
sentence containing "Known remaining P0 blockers" and edit it to omit that
specific blocker while keeping the rest of the list intact and grammatically
correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68ede4eb-bcbb-44f7-a0c2-c1ecbc5a50f0
📒 Files selected for processing (10)
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/StreamRandomizerRetry.t.sol
|
@coderabbitai review Addressed the review on the latest push:
Validation: |
|
@coderabbitai review Addressed the remaining roadmap-status comment on the latest push: the top-level |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Check lifecycle contract for split event/state mutation
grep -n "RandomnessPostProcessingFailed\|RandomnessPostProcessingRetryFailed\|_setFailedPostProcessingState\|_markRandomness\|failureDataHash\|fulfilledBlock\|fulfilledTimestamp" smart-contracts/StreamRandomizerLifecycle.solLength of output: 1977 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Check the retry test for arRNG parity and dual-event assertions
grep -n "testArrng\|testRetryLimit\|testRetryRejectsChanged\|RandomnessPostProcessingFailed\|fulfilledBlock\|fulfilledTimestamp" test/StreamRandomizerRetry.t.sol | head -80Length of output: 1261 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Show the full _markRandomnessPostProcessingFailed and _markRandomnessPostProcessingRetryFailed implementations
grep -n -A 20 "_markRandomnessPostProcessingFailed\|_markRandomnessPostProcessingRetryFailed\|_setFailedPostProcessingState" smart-contracts/StreamRandomizerLifecycle.sol | head -120Length of output: 2098
The refactoring is clean: the private Approving. (੭˃ᴗ˂)੭ ✦ [approve] |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Check the latest diff for this PR to see the docs-only changes
gh pr diff 69 --repo 6529-Collections/6529Stream 2>/dev/null | tail -200Length of output: 7887 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the latest commits on the PR to understand the latest push
gh pr view 69 --repo 6529-Collections/6529Stream --json commits --jq '.commits[-3:]'Length of output: 1206 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Check the diff for docs/status.md and ops/ROADMAP.md and docs/known-blockers.md on the full PR diff
gh pr diff 69 --repo 6529-Collections/6529Stream 2>/dev/null | grep -A 60 'diff --git a/docs/status.md'
gh pr diff 69 --repo 6529-Collections/6529Stream 2>/dev/null | grep -A 60 'diff --git a/ops/ROADMAP.md'
gh pr diff 69 --repo 6529-Collections/6529Stream 2>/dev/null | grep -A 60 'diff --git a/docs/known-blockers.md'Length of output: 12149
No stray "retry as remaining" references remain across any of the affected files. All four items from the prior review are fully addressed — the implementation, tests, and documentation are consistent and audit-ready for this PR. [approve] |
1 participant
Summary
retryRandomnessPostProcessing(uint256)to both VRF and arRNG adapters for deterministicFailedPostProcessingrequests.Safety Model
MAX_RANDOMNESS_POST_PROCESSING_RETRIES; failed retries preserve failed state, update the failure-data hash, and emit only the retry-specific failure event.Validation
forge buildforge test --match-contract StreamRandomizerRetryTest -vvv(10 tests, 0 failed)forge test --match-contract StreamRandomizerLifecycleTest -vvv(18 tests, 0 failed)make check(170 tests, 0 failed)powershell -ExecutionPolicy Bypass -File scripts\check.ps1(170 tests, 0 failed)forge fmt --check smart-contracts\StreamRandomizerLifecycle.sol smart-contracts\RandomizerVRF.sol smart-contracts\RandomizerRNG.sol test\StreamRandomizerRetry.t.sol test\helpers\CharacterizationTestBase.solgit diff --checkretryRandomnessPostProcessingslither_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; high/medium and production reentrancy counts unchangedCloses #42.
Summary by CodeRabbit
New Features
Tests
Documentation