fix(rust-client): replace RandomCoin with ChaCha20Rng - #2414
Conversation
…ects a non-CryptoRng
ricomateo
changed the title
RandomCoinRandomCoin with ChaCha20Rng
juan518munoz
left a comment
juan518munoz
left a comment
There was a problem hiding this comment.
Looks good overall. What I still don't understand is why we are keeping rng and secure_rng separately, for what I see we could just get away with the new secure implementation in all places.
If I'm not mistaken, keeping a single rng would also make the trybuild test redundant.
| /// the field modulus, which keeps the result uniform over the field. The rejection | ||
| /// probability is about 2^-32. | ||
| pub fn draw_felt(rng: &mut impl rand::Rng) -> crate::Felt { | ||
| use rand::RngExt; |
There was a problem hiding this comment.
we should move the import outside this function
| /// Uses rejection sampling: [`Felt::new`](crate::Felt::new) rejects any `u64` at or beyond | ||
| /// the field modulus, which keeps the result uniform over the field. The rejection | ||
| /// probability is about 2^-32. | ||
| pub fn draw_felt(rng: &mut impl rand::Rng) -> crate::Felt { |
There was a problem hiding this comment.
Maybe this functions are better suited in a separate file, similar to how we do with utils.rs, we could add an rng.rs file.
| /// | ||
| /// Use this for note serial numbers when building notes from a plain [`rand`] generator, which | ||
| /// does not implement [`FeltRng`]. | ||
| pub fn draw_word(rng: &mut impl rand::Rng) -> crate::Word { |
|
@juan518munoz thanks! all comments were addressed in this commit e3b4748.
We keep both |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| /// Checks that [`ClientBuilder::rng`] rejects a generator that is not a `CryptoRng`. |
There was a problem hiding this comment.
this is already enforced by the compiler in the type system, we can remove this test IMO
There was a problem hiding this comment.
If we remove the test, then we should remove this file
juan518munoz
left a comment
juan518munoz
left a comment
There was a problem hiding this comment.
LGTM. Before merging this, it may be a good idea to create a branch on web-sdk that points to this branch, so we can assure there won't be any setbacks.
|
@juan518munoz I created a draft PR in the |
3 participants
Closes #2204
RandomCoinwithChaCha20Rng.ClientBuilder::rngnow requires the RNG to implementCryptoRng(i.e.RandomCoinis no longer accepted, since it is not aCryptoRng).Client::secure_rng, a second RNG intended to be used to generate randomness for secret values.trybuildtest asserting thatClientBuilder::rngrejects aFeltRngwhich is not aCryptoRng.