RustCrypto / traits

Collection of cryptography-related traits
593 stars 193 forks source link

digest: expose `AssociatedAlgorithmIdentifier` through `CoreWrapper` #1575

Closed baloo closed 3 months ago

baloo commented 6 months ago

This allows implementation in consumer crates like: https://github.com/RustCrypto/hashes/pull/586

tarcieri commented 6 months ago

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.

newpavlov commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

tarcieri commented 6 months ago

@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 commented 6 months ago

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

newpavlov commented 6 months ago

Yes, I meant Blake2b<U64>.