-
Notifications
You must be signed in to change notification settings - Fork 94
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
SignatureScheme
trait bounds
#148
Conversation
@EspressoSystems/jellyfish |
impl core::ops::Deref for BLSSignature { | ||
type Target = Signature; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
&self.0 | ||
} | ||
} |
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 don't see manual impl of Deref
often, especially not for newtype, may I ask why do we need this?
(some say that implementing Deref for newtype is a bad idea)
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.
It's a convenient way of allowing calls to the underlying value directly, e.g. sig.verify(...)
instead of sig.0.verify(...)
.
When you say you don't see it done manually, do you mean it's mostly done via a derive, like derive-more crate? Or generally not done via Deref
?
Here's a some points against using it: https://rust-unofficial.github.io/patterns/anti_patterns/deref.html
Though also in the discussion you linked there are some strong arguments for using Deref
(not DerefMut
) specifically for the purpose of newtypes.
In this specific case I don't see an immediate danger of implementing Deref
though. If you do, I'm not gonna fight for keeping Deref
and happy to use another solution.
Let's also hear other opinions?
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.
yeah I also agree there's no intermediate harm, but the anti-pattern does introduce mental load as
Most importantly this is a surprising idiom - future programmers reading this in code will not expect this to happen.
it's abusing the Deref
which is supposed to be custom pointer type, not newtype accessing its inner value.
(another testimony of the same problem)
I don't mind too much about sig.0.verify()
, it's internal details, won't get exposed to consumer anyway.
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've removed Deref
as a trial in 7149b1a. We can still revert later :)
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.
So as I dig deeper, there are some subtleties involved.
TL;DR: yes, in our case, we can use Deref
as you originally proposed. @tessico
this comment rust-lang/api-guidelines#249 (comment) convinced me that using Deref
for transparent newtype is not only okay but also practically seen in std lib and some high-profile crates.
So is suggested in the Rust book itself.
What I'm also more sure of is:
- we should NOT use
Deref
to simulate OOP-style inheritance (instead, we should wait for delegation to be available in rust, or useambassador
crate to achieve that) - we should be careful about the potential violation of intended visibility when implementing
DerefMut
(to accessfn method(&mut self)
of the underlying types). see my example here.
Jon Gjengset had updated a good summary here.
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.
@alxiong Nice research! I knew I saw it in the Rust book somewhere, I must have missed it last time around.
It's encouraging to see it used in std :)
instead use the inner reference with `<data>.0`
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.
LGTM!
primitives/src/signatures/mod.rs
Outdated
type SigningKey: Clone + Send + Sync + Zeroize; | ||
|
||
/// Verification key | ||
type VerificationKey; | ||
type VerificationKey: Clone + Send + Sync + for<'a> Deserialize<'a> + Serialize; | ||
|
||
/// Public Parameter | ||
type PublicParameter; | ||
|
||
/// Signature | ||
type Signature; | ||
type Signature: Clone + for<'a> Deserialize<'a> + Serialize; | ||
|
||
/// A message is &\[MessageUnit\] | ||
type MessageUnit; |
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.
follow-up comment:
- all should have
Debug + Sync + Send + Clone
(so that they can be printed, and shared across threads), includingMessageUnit
should also have those traits. - all except
MessageUnit
should haveParitalEq + Eq
for comparison.
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.
It seems that blst::min_sig::SecretKey
doesn't implement PartialEq
for some reason (while PublicKey
and Signature
do), see an issue here. For now I've implemented it manually by comparing the output of serialization, which seems the only way currently.
Depending on what ppl say in the blst issue, we might either add the implementation to their crate and contribute upstream, or if there was a good reason for not including it, remove the bounds from our SignatureScheme
trait.
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.
oh we have overlapping work. I also implemented a (possibly constant-time) comparison for PartialEq
in #156
bounds for `Debug + Clone + Send + Sync`
This reverts commit 7149b1a.
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.
LGTM!
(I will try to resolve the conflict with my PR later, somehow some of the overlap between our work slip my mind when I worked on my PR)
Description
Add trait bounds on
SignatureScheme
trait.Bounds not yet added for
SigningKey
, pending discussion in espresso-common.closes: #145, #155
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
Files changed
in the GitHub PR explorer