Skip to content
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 serde feature & PartialOrd & Ord #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dignifiedquire
Copy link
Contributor

No description provided.

@rklaehn
Copy link
Collaborator

rklaehn commented Oct 25, 2024

Somebody has added serde support to the original blake3 crate. Maybe we should try to get in sync with "upstream" before doing this ourselves...

BLAKE3-team/BLAKE3@5e3eb94

constant time implementation based on `crypto-bigint`
@dignifiedquire
Copy link
Contributor Author

dignifiedquire commented Oct 25, 2024

Somebody has added serde support to the original blake3 crate. Maybe we should try to get in sync with "upstream" before doing this ourselves...

BLAKE3-team/BLAKE3@5e3eb94

This is worse than what we have, because we do the human readable differentation though

@dignifiedquire dignifiedquire changed the title feat: add serde feature feat: add serde feature & PartialOrd & Ord Oct 25, 2024
Copy link

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit of an "oof" for reviewing the const-time Ord implementation from me.

Would you be up for writing a small proptest that checks the const-time Ord for Hash against the non-const-time Ord for [u8; 32]?

Something like this:

#[proptest]
fn prop_hash_ord_matches_slice_ord(a: [u8; 32], b: [u8; 32]) {
    prop_assert_eq!(
        Hash::from_bytes(a).cmp(&Hash::from_bytes(b)),
        a.cmp(&b)
    );
}

Needs test-strategy for the nice #[proptest] annotation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants