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

Make rand an optional dependency #16

Closed
naturallymitchell opened this issue Jan 31, 2019 · 6 comments
Closed

Make rand an optional dependency #16

naturallymitchell opened this issue Jan 31, 2019 · 6 comments

Comments

@naturallymitchell
Copy link

using randombytes from libsodium generating random data makes more sense to me than using rand crate. @dylanhart, what do you think?

@dylanhart
Copy link
Owner

This library supports alternate RNGs using the with_source constructor. This allows use of a crypto RNG. Is there an issue with this method that I'm not aware of?

@naturallymitchell
Copy link
Author

Is there an issue with this method that I'm not aware of?

I missed that constructor.

Could rand crate be a default feature that can be disabled, instead of required by default?

@dylanhart
Copy link
Owner

I can look into this when I have more time tonight, but the with_source constructor uses the RNG trait provided by rand.

@dylanhart
Copy link
Owner

dylanhart commented Feb 1, 2019

I did some research and analysis on this last night, but went to bed instead of writing it up; so here it is now.

I think that rand makes a very sane default. ThreadRng is a crypto rng by default and provides a way to implicitly pass the rng to ulid (rand::thread_rng()). It also is pretty quick (tested against modified versions with cargo bench) and is only ~8KiB in size (half the code size of exonum_sodiumoxide via cargo bloat --release --crates).

exonum_sodiumoxide is also way too slow to be the default: (edit: I thought this was 2.5ms not 2.5 us when I wrote this)

running 5 tests
test bench_from_string        ... bench:          41 ns/iter (+/- 16)
test bench_from_time          ... bench:       2,605 ns/iter (+/- 1,071)
test bench_generator_generate ... bench:          61 ns/iter (+/- 11)
test bench_new                ... bench:       2,710 ns/iter (+/- 385)
test bench_to_string          ... bench:          89 ns/iter (+/- 18)

test result: ok. 0 passed; 0 failed; 0 ignored; 5 measured

I'm still not against making rand an optional dependency. It can be done by creating a new RandomSource trait and providing and implementation for T where T: rand::Rng. This new trait can be used for the *with_source constructors and the other constructors can be disabled when the rand feature is not used.

Do you have a particular need for this feature? I want to know so I can prioritize.

@naturallymitchell
Copy link
Author

very helpful analysis, thank you

I don't need this feature for anything. it was just a thought

@dylanhart dylanhart changed the title replace rand with sodiumoxide Make rand an optional dependency Feb 2, 2019
@dylanhart
Copy link
Owner

Optional via std feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants