search

dylanhart / ulid-rs

This is a Rust implementation of the ulid project
https://crates.io/crates/ulid
MIT License
389 stars 37 forks source link

Make rand an optional dependency #16

Closed naturallymitchell closed 3 years ago

naturallymitchell commented 5 years ago

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

dylanhart commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

dylanhart commented 5 years ago

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 commented 5 years ago

very helpful analysis, thank you

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

dylanhart commented 3 years ago

Optional via std feature