-
Notifications
You must be signed in to change notification settings - Fork 122
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
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
77491c3
WIP
tessico 236613c
fix a comment on serialization
tessico dc83de3
Add trait bounds for `SignatureScheme`'s verification key and signature
tessico 87a980a
Fix rust doc formatting for CanoninicalBytes
tessico 257cfc7
Merge branch 'main' into sig-trait-bounds
tessico f14cca0
move constants from bls.rs to constants.rs
tessico 202148e
Add a Zeroize bound on `SigningKey`
tessico a00c769
map errors
tessico 7149b1a
Remove `Deref` impl for BLS structs
tessico 919a41a
Replace tags with updated definitions from espresso common
tessico cbc3bae
Add de/ser bounds on `SigningKey`
tessico 59f7ef9
add bounds on SignatureScheme associated types
tessico 7d6dcfa
Add Eq, PartialEq bounds on SignatureScheme types
tessico 0d29835
Revert "Remove `Deref` impl for BLS structs"
tessico 734a45e
Merge branch 'main' into sig-trait-bounds
tessico d15a511
pin espresso-common to 0.4
tessico File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
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 ofsig.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
(notDerefMut
) 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 keepingDeref
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
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. @tessicothis 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:
Deref
to simulate OOP-style inheritance (instead, we should wait for delegation to be available in rust, or useambassador
crate to achieve that)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 :)