Change the CSPRNG API to use Seed#8578
Change the CSPRNG API to use Seed#8578andreacerulli wants to merge 3 commits intoandrea-crp-2107from
Conversation
| /// into `Randomness`. If the hash is less than 32 bytes, the remaining bytes | ||
| /// are padded with zeros. If the hash is greater than 32 bytes, the remaining | ||
| /// bytes are truncated. | ||
| pub fn crypto_hashable_to_randomness<T: CryptoHashable>(data: &T) -> Randomness { |
There was a problem hiding this comment.
nit: Or randomness_from_cryptohashable (or randomness_from_crypto_hashable), given that its also called seed_from_randomness and seed_from_random_beacon (and not x_to_y).
| let hash = crypto_hash(crypto_hashable); | ||
| Seed::from_bytes(&hash.get().0) | ||
| /// Creates a seed from the given randomness. | ||
| pub fn seed_from_randomness(randomness: &Randomness) -> Seed { |
There was a problem hiding this comment.
nit: Optionally, we could explain in a comment (or the docu) why the function lives here at this somewhat unexpected place:
// The function lives here because:
// * if it would live on Seed, ic-crypto-internal-seed would have to deped on ic-types (not ideal)
// * if it would live on Randomness, ic-types would have to depend on ic-crypto-internal-seed (not ideal)
Similar for RandomBeacon. Feel free to ignore.
| let hash = crypto_hash(crypto_hashable); | ||
| Seed::from_bytes(&hash.get().0) | ||
| /// Creates a seed from the given randomness. | ||
| pub fn seed_from_randomness(randomness: &Randomness) -> Seed { |
There was a problem hiding this comment.
nit: Both seed_from_randomness and seed_from_random_beacon could also just be public functions in the crate, i.e., they don't necessarily have to be implemented on Csprng. Then at the call site, it would look a bit nicer, e.g., ic_crypto_prng::seed_from_randomness or even shorter with use ic_crypto_prng::seed_from_randomness; (or in a utils module: ic_crypto_prng::utils::seed_from_randomness.
| /// Creates a CSPRNG from the given seed for the given purpose. | ||
| pub fn from_seed_and_purpose(seed: &Randomness, purpose: &RandomnessPurpose) -> Self { | ||
| let seed = Seed::from_bytes(&seed.get()); | ||
| pub fn from_seed_and_purpose(seed: Seed, purpose: &RandomnessPurpose) -> Self { |
There was a problem hiding this comment.
Is it intentional that the function takes ownership of seed? It would be sufficient to just take a reference. But maybe your intention was to consume the seed so that it is more likely that it is not used for something else. If the latter is the case, maybe add a respective comment.
There was a problem hiding this comment.
Yes, that indeed was the intention. However, thinking about it again I think we should pass a reference: since we domain separate things based on context, it is expected that the same seed may be reused.
2 participants
No description provided.