-
Notifications
You must be signed in to change notification settings - Fork 30
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
impl standard IO traits for hashing #228
Conversation
In deciding what should be implemented, maybe we should just restrict it for now to the types that actually show up in the high-level interface. If people are reaching into |
@brycx Do you think there should be a new |
@vlmutolo I think it'd be best to keep it behind ATM, I think |
Is there something that blocks progression on this @vlmutolo? Should we discuss it more, maybe play around with more examples? |
So I don't think anything is "blocking" this exactly, but there are some open design questions I have that would be good to discuss. I'll write a bit about that on the relevant issue. |
I answered in the wrong place regarding some things. Apologies. Re-posting here:
I don't think another io feature is necessary. If we just gate it behind safe-api that should be fine, just like we do with the |
Edited your issue to having completed |
For when the implementation is more stable, I think it would be beneficial to try and integrate the |
Maybe it would be a good idea to split this PR into two: one for hashing, one for AEAD. |
Good idea. I'd probably do so, especially to work on the tests and not grow the PRs too large with that alone. |
I tried to use something like |
Did you build the docs with the parameters listed in the README? The command you just posted in the comment won't enable the rendering IIRC. |
Yep, that worked. Everything shows up as it should, including the feature bubble for |
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.
Great start on getting io
integration for the library! I have two general things in addition to code comments:
[1] Please rebase upon master
so that we're sure the doctests run with --no-default-features
(#263). I believe this should be the case already though, since the function itself is gated behind safe-api
.
[2] Since the title of the PR is "IO traits for hashing" do you plan to add Write
impls for SHA2 primitives in other PRs?
👍
That's a good question. I can definitely do the same write impl for SHA as I did for Blake. Wasn't thinking about that, but I should probably add it to this PR. |
This commit adds support for Rust's standard Write trait to the Blake2b type. It also adds a convenience function digest_from_reader that will consume all data from a reader and output the digest.
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.
Sorry about the delay - LGTM, great work!
See #227.
TODOs
EDIT: The scope of this PR has changed. We were going to try to fit the Read/Write impls for all of Orion's types into this PR, but then decided to split it up. #227 will keep track of all the impls.
Now this PR implements
std::io::Write
for Blake2b and adds a convenience functionorion::hash::digest_from_reader
.