-
Notifications
You must be signed in to change notification settings - Fork 181
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
Rust: Add conversion between raw types and safe wrappers #248
base: master
Are you sure you want to change the base?
Conversation
bindings/rust/src/lib.rs
Outdated
} | ||
} | ||
|
||
impl std::convert::TryFrom<blst_scalar> for SecretKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be renamed to core::convert::TryFrom
. You can choose to do nothing and I'll change it on commit :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, of course. good catch!
/// passed [`blst_scalar`] must be ok as per [`blst_sk_check`] | ||
pub unsafe fn from_scalar_unchecked(value: blst_scalar) -> Self { | ||
SecretKey { value } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue against this (as well as TryFrom below). Secret key interfaces should discourage copying of the secret key material, not encourage it(*). Well, one can say that this is inconsistent with the fact that SecretKey has Clone (auto-derived) trait. I'd argue that it was a mistake, unfortunately removing it constitutes a breaking change :-( As alternative I'd suggest a reference transmute. This naturally entails adding #repr(transparent)
to the structure definition.
(*) Granted, Rust common practices don't appear to be really conducive to handling secret key material.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable. Would you rather change these methods to do the transmutation or leave it to the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn on this. I appreciate the fact that suggested from_scalar_unchecked is marked unsafe, because it makes the user pay attention. But it's impossible to mark impl<'a> From<&'a blst_scalar> for &'a SecretKey
unsafe and as result it will appear safe to the user. On the other hand it's a convenience method that doesn't make a copy of the secret key material, hence there is no strong reason for making the user pay as much attention... So I suppose it's appropriate to internalize the unsafe transmute...
However, as it turns out it's impossible to have both From<&'a blst_scalar>
and TryFrom<&'a blst_scalar>
for &'a SecretKey
. So I suppose one can add
impl<'a> core::convert::TryFrom<&'a blst_scalar> for &'a SecretKey {
type Error = BLST_ERROR;
fn try_from(value: &'a blst_scalar) -> Result<Self, Self::Error> {
unsafe {
if !blst_sk_check(value) {
return Err(BLST_ERROR::BLST_BAD_ENCODING);
}
Ok(transmute::<_, _>(value))
}
}
}
And leave the option to perform the raw unchecked transmute to the user.
As for referred PR to sigp/achor. Note that there are blst_sk_{add,sub,mul}_n_check that operate on scalars as opposed to fr_t... |
This enables use cases where users want to perform some computations using the
blst_*
functions, but also use theblst::{min_pk,min_sig}::*
types elsewhere.