Removal of .unwrap() and .expect() in library code#46
Closed
npajkovsky wants to merge 4 commits into
Closed
Conversation
…ory defaults Replace the three pub DEFAULT_*_HASH_NAME constants with direct enum variant construction in Default, default_128_bit(), and default_256_bit(). The constants were only used internally to drive Self::new().unwrap() calls — removing them makes the defaults infallible at compile time and reduces the public surface area of the factory. Signed-off-by: Nikola Pajkovsky <nikolap@openssl.org>
…y defaults Same pattern as the HashFactory cleanup: replace the three pub DEFAULT_*_KDF_NAME constants with direct enum variant construction in Default, default_128_bit(), and default_256_bit(). The constants were only used internally to drive unwrap() calls on Self::new() — removing them makes the defaults infallible at compile time and shrinks the public API surface. Signed-off-by: Nikola Pajkovsky <nikolap@openssl.org>
…y defaults Replace pub DEFAULT_KDF_NAME, DEFAULT_128BIT_KDF_NAME, and DEFAULT_256BIT_KDF_NAME with direct enum variant construction in Default, default_128_bit(), and default_256_bit(). The constants existed solely to feed Self::new().unwrap() — constructing the variants directly makes the defaults infallible at compile time and removes three unnecessary items from the public API. Signed-off-by: Nikola Pajkovsky <nikolap@openssl.org>
Replace manual length checks followed by unwrap() with try_into().map_err() to coerce the signature slice to a fixed-size array in one step. Replace assert!() + unwrap() on the streaming verifier's optional public key with ok_or(), returning a proper SignatureError::GenericError instead of panicking. Use .then_some().ok_or() to convert the boolean verify result into a Result without an if/else branch. Signed-off-by: Nikola Pajkovsky <nikolap@openssl.org>
ounsworth
approved these changes
Jun 26, 2026
| pub const DEFAULT_HASH_NAME: &str = SHA3_256_NAME; | ||
| pub const DEFAULT_128BIT_HASH_NAME: &str = SHA3_256_NAME; | ||
| pub const DEFAULT_256BIT_HASH_NAME: &str = SHA3_512_NAME; | ||
|
|
Contributor
There was a problem hiding this comment.
Hmm. This change will make it hard in the future if we want to make the library defaults driven from a config file or env var, but I guess for now this saves us 3 unwraps per factory. Ok.
(also, I think the whole factory crate needs a big re-visit, see #23)
Contributor
|
Merged manually. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Most of the .unwrap(), .expect(), panic or unreachable can be tolerated, and this PR fixes
a couple of unsoundness in the library.
Closes #42