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

Do we still need getrandom? #955

Open
cjpatton opened this issue Feb 23, 2024 · 6 comments
Open

Do we still need getrandom? #955

cjpatton opened this issue Feb 23, 2024 · 6 comments

Comments

@cjpatton
Copy link
Collaborator

I recall I initially pushed for using getrandom in lieu of rand in order to have compatibility with WASM. First, I'm not sure this was ever technically necessary. Second, we no longer need WASM for Daphne:
https://github.com/cloudflare/daphne/blob/b59507a0092e1267e7ec7c3f99c33b59bd1deb34/daphne_worker_test/Cargo.toml#L31-L34

What do you think about removing the dependency?
cc/ @mendess

@divergentdave
Copy link
Collaborator

rand and rand_core rely on the getrandom crate with default features enabled, and that backs the OsRng implementation, so I think it's six of one or half a dozen of the other.

@cjpatton
Copy link
Collaborator Author

Right, but can we avoid using it directly here? I.e., could we replace getrandom() with thread_rng().fill()?

@tgeoghegan
Copy link
Contributor

Dropping a direct dependency would be nice, but it sounds like this would have no other functional effect. On the other hand, we have a couple error enums with variants:

 /// Failure when calling getrandom().
 #[error("getrandom: {0}")]
 GetRandom(#[from] getrandom::Error)

That means that dropping getrandom is a public API change. Not one I'm opposed to per se, but we'd have to wait for prio 0.17.

@cjpatton
Copy link
Collaborator Author

I think it's worth doing, and yeah this could wait until 0.17.

@divergentdave
Copy link
Collaborator

Note that dropping our direct dependency on getrandom would now require removing our wasm-compat feature added in #1059. The README for the rand crate says that, for wasm32-unknown-unknown in a JavaScript environment, you have to turn on the getrandom/js feature separately from your rand dependency.

@cjpatton
Copy link
Collaborator Author

cjpatton commented Jun 5, 2024

Looks like we're good to drop the "wasm-compat" feature in daphne: cloudflare/daphne#613

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

No branches or pull requests

3 participants