-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat: add blake3 160, 192 & 256 bits #18
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.
Thank you! Looks good! I left a few comments inline.
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.
Looks good! Just one minor nit inline.
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.
Looks good, thanks! I left a few very minor comments inline related to docs/comments.
@bobbinth I started a PR in winterfell fixing that. Unfortunately, it will not be an isolated change since it will impact also Alternatively, we can wait for generic const expressions, so we don't leak the output requirement from the digest (hence, it can be defined during the trait implementation). Could be something like this in the end: #![feature(generic_const_exprs)]
pub trait Digest {
const OUTPUT: usize;
fn as_bytes(&self) -> &[u8; Self::OUTPUT];
} wdyt? If we go for the latter, then its an isolated change. The former, however, will require that all hasher defines their target output size (which kinda makes sense IMO, but it will change 3 traits instead of one) |
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.
Thanks! Looks good to me.
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.
Looks good to me, and I am in favor of the generic const solution.
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.
All looks good! Thank you!
Regarding Digest
updates - I'd also prefer the const generic solution. So, let's wait for that.
closes #6