-
Notifications
You must be signed in to change notification settings - Fork 999
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
refactor(identity): call Into::into instead of copying the slice #5788
base: master
Are you sure you want to change the base?
Conversation
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. Left a small minor comment.
Can you update the changelog, and, if applicable, Cargo.toml
?
array.copy_from_slice(generic_array.as_slice()); | ||
|
||
let message = Message::parse(&array); | ||
let message = Message::parse(&Sha256::digest(msg).into()); |
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 wonder if we should have it in separate variables before passing it to Message::parse
eg
let message = Message::parse(&Sha256::digest(msg).into()); | |
let digest = Sha256::digest(msg); | |
let message = Message::parse(&digest.into()); |
Thoughts? Not that it should impact anything from a glance over, but just thinking about readability.
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 believe the information is sufficient since the method is called digest
. Calling into
introduces some ambiguity but I don't think it matters.
I am not sure, but I think the FIXME note was added because we don't want to use If so, then I think that reasoning still applies, because we still indirectly depend on |
Oops I forgot to check the actual dependency tree. But it compiles for some reason. |
I believe we were already able to use |
I think so.
I have no idea because that's not even our direct dependency. |
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.
Description
Fixes:
rust-libp2p/identity/src/secp256k1.rs
Lines 136 to 138 in 02040ff
Related: #3850
Notes & open questions
Internal change.
Change checklist
I have made corresponding changes to the documentationI have added tests that prove my fix is effective or that my feature worksA changelog entry has been made in the appropriate crates