Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement "--seed NUMBER" flag to seed Math.random() and crypto.getRandomValues() #2483

Merged
merged 7 commits into from Jun 11, 2019

Conversation

3 participants
@mtharrison
Copy link
Contributor

commented Jun 9, 2019

Fixes #2322

@mtharrison mtharrison force-pushed the mtharrison:seed-flag branch from 50b8beb to 28c4611 Jun 9, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 9, 2019

Thanks!

Does this work with crypto.getRandomValues() ?

deno/cli/ops.rs

Lines 2213 to 2220 in a115340

fn op_get_random_values(
_state: &ThreadSafeState,
_base: &msg::Base<'_>,
data: Option<PinnedBuf>,
) -> Box<OpWithError> {
thread_rng().fill(&mut data.unwrap()[..]);
Box::new(ok_future(empty_buf()))
}

Can you add this to tests/seed_random.js

const arr = new UInt8Array(32);
crypto.getRandomValues(arr);
console.log(arr);
@@ -379,6 +392,19 @@ pub fn parse_flags(matches: &ArgMatches) -> DenoFlags {
v8_flags.insert(0, "deno".to_string());
flags.v8_flags = Some(v8_flags);
}
if matches.is_present("seed") {
let seed = matches.value_of("seed").unwrap();
let v8_seed_flag = format!("--random_seed={}", seed);

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 9, 2019

Contributor

should be --random-seed

);
assert_eq!(subcommand, DenoSubcommand::Run);
assert_eq!(argv, svec!["deno", "script.ts"])
}

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 9, 2019

Contributor

Can you add test case that includes --v8-flags as well?

.validator(|val: String| {
match val.parse::<i64>() {
Ok(_) => Ok(()),
Err(_) => Err("value should be a number".to_string())

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 9, 2019

Contributor

s/value/Seed/

This comment has been minimized.

Copy link
@mtharrison

mtharrison Jun 9, 2019

Author Contributor

Thanks for the comments, I've addressed them so far.

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@ry this PR doesn't currently affect the values generated by Crypto.getRandomValues(), only the values generated by V8 (which I believe the only interface to is Math.random()). Would we like to support seeding the Rust RNG too?

mtharrison added some commits Jun 9, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 9, 2019

Would we like to support seeding the Rust RNG too?

Yes - I'd like deno to be completely deterministic some day.

mtharrison added some commits Jun 11, 2019

@mtharrison mtharrison changed the title Implement "--seed NUMBER" flag to seed Math.random()'s RNG Implement "--seed NUMBER" flag to seed Math.random() and crypto.getRandomValues() Jun 11, 2019

@ry

ry approved these changes Jun 11, 2019

Copy link
Collaborator

left a comment

LGTM - thanks @mtharrison !

@ry ry merged commit d82c199 into denoland:master Jun 11, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@mtharrison mtharrison deleted the mtharrison:seed-flag branch Jun 11, 2019

@mtharrison

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Thanks 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.