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

crypto-common: rename serializable_state module to hazmat #1487

Merged

Conversation

tarcieri
Copy link
Member

Also removes the toplevel re-exports for the traits, forcing users to go through the now pub hazmat module.

There's a lot of misuse potential with these traits, which are intended to make it possible to serialize/deserialize the internal state of hash functions. However previously they were presented side-by-side with other traits, which made that unclear.

This commit deliberately doesn't make other changes to the file so git will preserve its history:

diff --git a/crypto-common/src/serializable_state.rs b/crypto-common/src/hazmat.rs
similarity index 100%
rename from crypto-common/src/serializable_state.rs
rename to crypto-common/src/hazmat.rs

It would be very good for a followup commit to massively expand the documentation around both the module and the traits, especially to spell out the various ways they can be misused.

@tarcieri tarcieri force-pushed the crypto-common/move-serialize-traits-into-hazmat-module branch from ac09483 to 9b41783 Compare January 26, 2024 19:40
@@ -12,6 +12,9 @@
#[cfg(feature = "std")]
extern crate std;

/// Hazardous materials.
Copy link
Member Author

Choose a reason for hiding this comment

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

I put this here for now to avoid modifying the now hazmat.rs but it would be good to move the comment into there in a followup commit

@tarcieri
Copy link
Member Author

tarcieri commented Jan 26, 2024

Hmm, perhaps we should move the documentation checks into the "Workspace" workflow.

Running cargo doc on the toplevel of the repo addresses the error. It would also ensure we're always checking the rustdoc of every crate in the repo, rather than having to configure it on a crate-by-crate basis.

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

I wonder if it may be worth to move the serialization traits into a separate crate and make serialization support feature-gated and disabled by default.

@tarcieri
Copy link
Member Author

I could potentially add a hazmat feature and gate on that. A separate crate is also something to consider.

tarcieri added a commit that referenced this pull request Jan 27, 2024
This should fix the error we're encountering in #1487, and at the same
time also check all crates have valid documentation, rather than just
`elliptic-curve` and `signature`.
tarcieri added a commit that referenced this pull request Jan 27, 2024
This should fix the error we're encountering in #1487, and at the same
time also check all crates have valid documentation, rather than just
`elliptic-curve` and `signature`.
tarcieri added a commit that referenced this pull request Jan 27, 2024
This should fix the error we're encountering in #1487, and at the same
time also check all crates have valid documentation, rather than just
`elliptic-curve` and `signature`.
Also removes the toplevel re-exports for the traits, forcing users to go
through the now `pub hazmat` module.

There's a lot of misuse potential with these traits, which are intended
to make it possible to serialize/deserialize the internal state of hash
functions. However previously they were presented side-by-side with
other traits, which made that unclear.

This commit deliberately doesn't make other changes to the file so git
will preserve its history:

    diff --git a/crypto-common/src/serializable_state.rs b/crypto-common/src/hazmat.rs
    similarity index 100%
    rename from crypto-common/src/serializable_state.rs
    rename to crypto-common/src/hazmat.rs

It would be very good for a followup commit to massively expand the
documentation around both the module and the traits, especially to spell
out the various ways they can be misused.
@tarcieri tarcieri force-pushed the crypto-common/move-serialize-traits-into-hazmat-module branch from 9b41783 to d0b8719 Compare January 27, 2024 01:53
@tarcieri tarcieri merged commit d622f97 into master Jan 27, 2024
80 checks passed
@tarcieri tarcieri deleted the crypto-common/move-serialize-traits-into-hazmat-module branch January 27, 2024 02:04
baloo added a commit to baloo/traits that referenced this pull request Mar 17, 2024
tarcieri pushed a commit that referenced this pull request Mar 17, 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.

2 participants