feat: deterministic Random utility for Collections#266
feat: deterministic Random utility for Collections#266
Conversation
…_same_seed_produces_same_sequence
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA new Move package Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #266 +/- ##
==========================================
- Coverage 89.88% 88.69% -1.19%
==========================================
Files 19 20 +1
Lines 1790 1814 +24
Branches 484 502 +18
==========================================
Hits 1609 1609
- Misses 168 192 +24
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
collections/sources/random.move (1)
40-46: Potential short period due to bit shifting in the LCG.The
>> 1shift discards the LSB on each iteration, which deviates from a standard Linear Congruential Generator and may significantly reduce the generator's period (potentially to ~2^63 or less depending on the multiplier's properties). For a utility intended for "reproducible internal flows," this might be acceptable, but if longer periods are needed, consider removing the shift or using a different approach.Also, the mask
0x0000000000000000ffffffffffffffffhas redundant leading zeros—0xffffffffffffffffis equivalent and cleaner.♻️ Minor cleanup: remove redundant zeros
public(package) fun rand(r: &mut Random): u64 { r.seed = ( ( - ((9223372036854775783u128 * ((r.seed as u128)) + 999983) >> 1) & 0x0000000000000000ffffffffffffffff, + ((9223372036854775783u128 * ((r.seed as u128)) + 999983) >> 1) & 0xffffffffffffffff, ) as u64, ); r.seed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/sources/random.move` around lines 40 - 46, In rand(&mut Random) the right-shift ">> 1" truncates the LSB each iteration (reducing LCG period) and the mask has redundant leading zeros; to fix, remove the ">> 1" shift so the LCG update uses the full 128-bit product+increment before truncating to 64 bits, and replace the mask literal 0x0000000000000000ffffffffffffffff with the canonical 0xffffffffffffffff; update the assignment to r.seed in function rand to compute (multiplier * (r.seed as u128) + increment) then mask/cast to u64 without shifting.collections/tests/random_tests.move (1)
37-42: Consider strengthening the property test to check more values.The property test only asserts that the first
rand()output matches for identical seeds. Checking a few more values would provide stronger confidence in sequence determinism.♻️ Suggested enhancement
#[random_test] fun test_same_seed_produces_same_sequence(seed: u64) { let mut random1 = new(seed); let mut random2 = new(seed); assert_eq!(random1.rand(), random2.rand()); + assert_eq!(random1.rand(), random2.rand()); + assert_eq!(random1.rand(), random2.rand()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/tests/random_tests.move` around lines 37 - 42, The test test_same_seed_produces_same_sequence currently only checks the first rand() value; extend it to verify multiple outputs to ensure deterministic sequences for identical seeds by calling new(seed) to create random1 and random2 and then comparing several subsequent rand() calls (e.g., in a for loop for N iterations or by collecting N values into vectors and asserting equality). Use the existing symbols new and rand and keep the seed parameter; ensure you assert equality for each iteration (or for the collected sequences) so the test fails if any later value diverges.collections/README.md (1)
1-1: Consider expanding README documentation.The README currently contains only a title. Consider adding:
- Brief package description
- Available modules (e.g.,
random)- Basic usage examples
- Any important caveats (e.g., "not cryptographically secure")
The PR checklist indicates documentation is pending, so this may already be planned.
Would you like me to draft expanded README content for this package?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/README.md` at line 1, The README currently only has the title "# openzeppelin_collections"; expand it to include a brief package description, a short list of available modules (e.g., random) with one-line summaries, basic usage examples for core API calls, and any important caveats (for example "not cryptographically secure"); update the top-level README content (replace/augment the existing title) and ensure you add a "Usage" and "Caveats" section so the documentation satisfies the PR checklist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@collections/README.md`:
- Line 1: The README currently only has the title "# openzeppelin_collections";
expand it to include a brief package description, a short list of available
modules (e.g., random) with one-line summaries, basic usage examples for core
API calls, and any important caveats (for example "not cryptographically
secure"); update the top-level README content (replace/augment the existing
title) and ensure you add a "Usage" and "Caveats" section so the documentation
satisfies the PR checklist.
In `@collections/sources/random.move`:
- Around line 40-46: In rand(&mut Random) the right-shift ">> 1" truncates the
LSB each iteration (reducing LCG period) and the mask has redundant leading
zeros; to fix, remove the ">> 1" shift so the LCG update uses the full 128-bit
product+increment before truncating to 64 bits, and replace the mask literal
0x0000000000000000ffffffffffffffff with the canonical 0xffffffffffffffff; update
the assignment to r.seed in function rand to compute (multiplier * (r.seed as
u128) + increment) then mask/cast to u64 without shifting.
In `@collections/tests/random_tests.move`:
- Around line 37-42: The test test_same_seed_produces_same_sequence currently
only checks the first rand() value; extend it to verify multiple outputs to
ensure deterministic sequences for identical seeds by calling new(seed) to
create random1 and random2 and then comparing several subsequent rand() calls
(e.g., in a for loop for N iterations or by collecting N values into vectors and
asserting equality). Use the existing symbols new and rand and keep the seed
parameter; ensure you assert equality for each iteration (or for the collected
sequences) so the test fails if any later value diverges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85e15975-7eb2-48d4-b53f-00da88694ba4
⛔ Files ignored due to path filters (1)
collections/Move.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
collections/Move.tomlcollections/README.mdcollections/sources/random.movecollections/tests/random_tests.move
2 participants
Resolves #260
PR Checklist
Summary by CodeRabbit
New Features
Tests