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

Relax Rng trait bounds to allow ?Sized Rngs #394

Merged

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented May 17, 2022

This allows the code to compile if you pass it &mut dyn RngType,
since trait objects are unsized.

See here for an example of caller code that is simplified by this
change:

mobilecoinfoundation/mobilecoin#1977 (comment)

This allows the code to compile if you pass it `&mut dyn RngType`,
since trait objects are unsized.

See here for an example of caller code that is simplified by this
change:

mobilecoinfoundation/mobilecoin#1977 (comment)
@tarcieri tarcieri added the do-for-4.0 This should be resolved before we do a 4.0 release label Nov 15, 2022
@@ -651,7 +651,7 @@ impl RistrettoPoint {
/// discrete log of the output point with respect to any other
/// point should be unknown. The map is applied twice and the
/// results are added, to ensure a uniform distribution.
pub fn random<R: RngCore + CryptoRng>(rng: &mut R) -> Self {
pub fn random<R: RngCore + CryptoRng + ?Sized>(rng: &mut R) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively this could just be:

    pub fn random(rng: &mut dyn CryptoRngCore) -> Self {

Though this requires virtual dispatch, it dramatically simplifies the method signature while allowing trait objects.

See related discussion for the @RustCrypto projects: RustCrypto/traits#1148

@rozbb
Copy link
Contributor

rozbb commented Nov 24, 2022

Of all the options, I think R: ... + ?Sized is the most Rustic, and less surprising than &mut dyn, as Artyom mentioned in the linked thread. Happy to merge. @tarcieri sounds good?

@tarcieri
Copy link
Contributor

Yeah that’s where we netted out too I think. The only difference being we switched to the new CryptoRngCore trait. But this is fine for now.

@rozbb rozbb changed the base branch from develop to release/4.0 November 27, 2022 01:55
@rozbb rozbb merged commit 840a9dc into dalek-cryptography:release/4.0 Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-for-4.0 This should be resolved before we do a 4.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants