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

digest: expose AssociatedAlgorithmIdentifier through CoreWrapper #1575

Closed
wants to merge 2 commits into from

Conversation

baloo
Copy link
Member

@baloo baloo commented May 22, 2024

This allows implementation in consumer crates like:
RustCrypto/hashes#586

@@ -20,6 +20,7 @@ block-buffer = { version = "=0.11.0-pre.5", optional = true }
subtle = { version = "2.4", default-features = false, optional = true }
blobby = { version = "0.3", optional = true }
const-oid = { version = "=0.10.0-pre.2", optional = true }
spki = { version = "=0.8.0-pre.0", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, this has a circular dependency: spki has an optional dependency on sha2 for computing SPKI fingerprints.

Copy link
Member Author

@baloo baloo May 23, 2024

Choose a reason for hiding this comment

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

Ha, I missed this one.

Newtype then ? :D
And we get the AssociatedOid and AssociatedAlgorithmIdentifier attached to those ? :D
(I agree with the usability comment you made over #1069)

Copy link
Member

Choose a reason for hiding this comment

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

If we had newtypes, it would at least get rid of any coupling between spki and digest, though the problem would remain for sha2.

We could potentially split off an e.g. spki-fingerprint crate so spki doesn't depend on sha2. It is something of a niche feature.

@tarcieri
Copy link
Member

As a meta comment: I would once again suggest making newtypes, rather than CoreWrapper type aliases, for each of the digest algorithms we support, possibly using a macro to write them.

This seems like a lot of unnecessary coupling through digest simply because the digest crates don't control their own types.

The type aliases are not semantically meaningful. It feels like internals leaking out.

It's also very, very confusing from a diagnostics perspective. See #1069.

@baloo baloo marked this pull request as draft May 23, 2024 15:41
@newpavlov
Copy link
Member

It may be worth to experiment with newtype macros. I think we should still expose block-level types and provide ways to do conversion in both directions, but for most users plain newtypes should be indeed easier to understand. We also need to decide what should be done with variable size hashes. For example, I don't think we should introduce Blak2b512 newtype, instead it should be an alias for Blake2b<U512>.

@tarcieri
Copy link
Member

tarcieri commented May 31, 2024

Would suggest closing this for now.

instead it should be an alias for Blake2b<U512>.

@newpavlov that seems fine to me. Curious with hybrid-array if we could even make that Blake2b<512>

@newpavlov
Copy link
Member

With hybrid-array we still have to use Blake2b<U512>.

@tarcieri
Copy link
Member

@newpavlov you could potentially use the AssocArraySize to get U512 from a const generic usize: https://docs.rs/hybrid-array/0.2.0-rc.8/hybrid_array/trait.AssocArraySize.html

@baloo
Copy link
Member Author

baloo commented May 31, 2024

That would be Blake2b<U64> though (as in pub type Blake2b512 = Blake2b<U64> like it is today)? Or am I missing something?

@newpavlov
Copy link
Member

Yes, I meant Blake2b<U64>.

@baloo baloo closed this Aug 20, 2024
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