feat(contract-dev): edit random numbers guide to fit style guide and use Tolk code#2115
feat(contract-dev): edit random numbers guide to fit style guide and use Tolk code#2115
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates the randomness techniques document to use the new APIs ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Updates the “Random numbers” technique guide to match the documentation style guide and replaces FunC-centric examples/links with Tolk equivalents.
Changes:
- Rewrites the introduction and “why randomness is hard” sections for style/clarity.
- Replaces FunC APIs (
randomize_lt,rand,random) with Tolk APIs (random.initialize,random.range,random.uint256) and updates the example code block to Tolk. - Reorganizes content by adding a comparison table and tightening the security-model descriptions for each approach.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
|
/review |
|
/review |
This comment has been minimized.
This comment has been minimized.
![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
♻️ Duplicate comments (3)
contract-dev/techniques/random.mdx (3)
69-73:⚠️ Potential issue | 🟠 MajorClarify that
random.initialize()is required setup, not sufficient security.Line 72 says not to use
random.initialize()for high-value applications, while Line 134 says to always call it. The risk is relying on single-block randomness, not the initialization call itself.Suggested wording
- Validators can predict single-block randomness. Do not use `random.initialize()` for high-value applications such as financial lotteries or significant asset distribution. + Validators can predict single-block randomness. Do not rely on single-block randomness that only calls `random.initialize()` for high-value applications such as financial lotteries or significant asset distribution.-- Always call `random.initialize()` before using `random.uint256()` or `random.range()` to prevent users from predicting random values. -- Keep randomness out of external message receivers. External messages remain vulnerable even with `random.initialize()`. +- For single-block randomness, call `random.initialize()` before using `random.uint256()` or `random.range()`. This reduces regular-user prediction risk, but does not protect against validator control. +- Keep randomness out of external message receivers. External messages remain vulnerable even with `random.initialize()`.Also applies to: 134-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract-dev/techniques/random.mdx` around lines 69 - 73, Update the caution and the instruction that calls random.initialize() to make clear that random.initialize() is a required setup step but does not provide secure randomness by itself; specifically, modify the Aside warning and the guidance around random.initialize() to say “always call random.initialize() during setup, but do not rely on single-block randomness for high-value use cases” and mention recommended alternatives (e.g., VRF, commit-reveal, or an oracle like Chainlink) for secure randomness instead of relying solely on random.initialize().
25-25:⚠️ Potential issue | 🟡 MinorReplace vague speed intensifiers with concrete wording.
“Very slow”/“very slow” is subjective and repeats a previously flagged style issue. Prefer “Slow” in the table and reserve the explanatory timing detail for the Speed section.
Suggested wording
-| Speed | Fast | Slow | Very slow | +| Speed | Fast | Slow | Slow |-The commit-reveal scheme is very slow compared to other approaches, as it involves multiple phases and requires waiting for several blocks to complete the process. The time to finality can range from minutes to hours depending on the number of participants and block times. +The commit-reveal scheme is slower than the other approaches, as it involves multiple phases and requires waiting for several blocks to complete the process. The time to finality can range from minutes to hours depending on the number of participants and block times.Also applies to: 118-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract-dev/techniques/random.mdx` at line 25, Replace the subjective/redundant "Very slow" cell in the table row (the cell containing the Speed intensity labels: "Fast | Slow | Very slow") with "Slow" so the table uses a single concrete label, and update the corresponding explanatory text in the "Speed" section (the narrative paragraph that expands on timing) to include the timing detail previously implied by "Very slow"; also remove the duplicate "Very slow" occurrence noted elsewhere (the second occurrence flagged) so both table cells use consistent concrete wording ("Slow") and timing is documented only in the Speed section.
100-100:⚠️ Potential issue | 🟠 MajorAvoid unconditional security claims for commit-reveal.
“Cryptographically secure” and “the most secure” overstate the guarantee unless participant independence, binding commitments, penalties, and censorship assumptions are satisfied.
Suggested wording
-Multiple participants commit to secret values by publishing hashes, then reveal those values in a later phase. The final randomness is derived from combining all revealed values, ensuring no single party can determine the outcome alone. When properly implemented, this approach provides cryptographically secure randomness suitable for high-value applications. +Multiple participants commit to secret values by publishing hashes, then reveal those values in a later phase. The final randomness is derived from combining all valid revealed values, reducing the ability of any single party to determine the outcome alone. With binding commitments, independent participants, non-reveal penalties, and review, this approach can support high-value applications.-The commit-reveal scheme is the most secure on-chain randomness approach, as it relies on cryptographic commitments and multiple independent participants. It prevents any single participant from unilaterally determining the final value. +The commit-reveal scheme can provide stronger on-chain randomness guarantees than single-block or block-skipping approaches when it relies on cryptographic commitments and multiple independent participants. It prevents any single participant from unilaterally determining the final value under those assumptions.Also applies to: 110-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract-dev/techniques/random.mdx` at line 100, The statement about "commit-reveal" overclaims security; change the sentence asserting it is "cryptographically secure" to a conditional claim that depends on specific assumptions by updating the paragraph about commit-reveal to note that it can provide strong unpredictability only when participant independence, binding commitments, enforceable penalties or slashing for non-reveal, and censorship-resistance are ensured; explicitly remove absolute terms like "cryptographically secure" and "the most secure" and replace with phrasing such as "can provide strong randomness under the above conditions" and/or list the required assumptions so readers understand the limits of the commit-reveal scheme.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract-dev/techniques/random.mdx`:
- Around line 104-106: Update the "Commit phase"/"Reveal phase"/"Combination"
sections to require domain-separated, canonical commitments: instruct
participants to commit to H(domain || round_id || participant_id ||
random_nonce) instead of hashing the raw random number, and require the contract
to verify participant_id and round_id when accepting reveals; also replace or
augment the XOR/sum example with a canonical aggregation specification (e.g.,
sort participant IDs and aggregate in that deterministic order or use a vetted
merge function) to prevent replay/duplication attacks and cancellation issues.
---
Duplicate comments:
In `@contract-dev/techniques/random.mdx`:
- Around line 69-73: Update the caution and the instruction that calls
random.initialize() to make clear that random.initialize() is a required setup
step but does not provide secure randomness by itself; specifically, modify the
Aside warning and the guidance around random.initialize() to say “always call
random.initialize() during setup, but do not rely on single-block randomness for
high-value use cases” and mention recommended alternatives (e.g., VRF,
commit-reveal, or an oracle like Chainlink) for secure randomness instead of
relying solely on random.initialize().
- Line 25: Replace the subjective/redundant "Very slow" cell in the table row
(the cell containing the Speed intensity labels: "Fast | Slow | Very slow") with
"Slow" so the table uses a single concrete label, and update the corresponding
explanatory text in the "Speed" section (the narrative paragraph that expands on
timing) to include the timing detail previously implied by "Very slow"; also
remove the duplicate "Very slow" occurrence noted elsewhere (the second
occurrence flagged) so both table cells use consistent concrete wording ("Slow")
and timing is documented only in the Speed section.
- Line 100: The statement about "commit-reveal" overclaims security; change the
sentence asserting it is "cryptographically secure" to a conditional claim that
depends on specific assumptions by updating the paragraph about commit-reveal to
note that it can provide strong unpredictability only when participant
independence, binding commitments, enforceable penalties or slashing for
non-reveal, and censorship-resistance are ensured; explicitly remove absolute
terms like "cryptographically secure" and "the most secure" and replace with
phrasing such as "can provide strong randomness under the above conditions"
and/or list the required assumptions so readers understand the limits of the
commit-reveal scheme.
🪄 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: 131b0e2a-42a8-45d8-8a73-6d4d39279880
📒 Files selected for processing (1)
contract-dev/techniques/random.mdx
| 1. **Commit phase**: Each participant generates a random number off-chain and submits its hash to the contract. | ||
| 1. **Reveal phase**: After all commitments are received, participants disclose their original numbers. | ||
| 1. **Combination**: The contract combines the revealed numbers (e.g., by XOR or sum) to produce the final random value. |
There was a problem hiding this comment.
Bind commitments to the participant and round.
Hashing only the random number is copyable and replayable; with XOR aggregation, duplicated commitments can also cancel out. The guide should require domain-separated commitments and canonical aggregation.
Suggested wording
-1. **Commit phase**: Each participant generates a random number off-chain and submits its hash to the contract.
+1. **Commit phase**: Each participant generates a secret off-chain and submits a domain-separated hash that binds the secret to the participant address, contract address, and round identifier.
1. **Reveal phase**: After all commitments are received, participants disclose their original numbers.
-1. **Combination**: The contract combines the revealed numbers (e.g., by XOR or sum) to produce the final random value.
+1. **Combination**: The contract validates each reveal, rejects duplicates or mismatched commitments, orders accepted reveals canonically, and hashes the transcript to produce the final random value. - On-chain verification of commitments.
+- Domain separation and replay protection for commitments.
+- Duplicate commitment handling.
- Penalty mechanisms for non-reveals or invalid reveals.
- Timeout handling for missing participants.Also applies to: 120-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract-dev/techniques/random.mdx` around lines 104 - 106, Update the
"Commit phase"/"Reveal phase"/"Combination" sections to require
domain-separated, canonical commitments: instruct participants to commit to
H(domain || round_id || participant_id || random_nonce) instead of hashing the
raw random number, and require the contract to verify participant_id and
round_id when accepting reveals; also replace or augment the XOR/sum example
with a canonical aggregation specification (e.g., sort participant IDs and
aggregate in that deterministic order or use a vetted merge function) to prevent
replay/duplication attacks and cancellation issues.
There was a problem hiding this comment.
Hey @novusnota or @delovoyhomie, can one of you confirm this?
It sounds reasonable, but my crypto skills aren't strong enough to check this without substantial effort 🫣
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Fixes #2113.
Summary by CodeRabbit