Added a closure-based do_hazardous_operations()#40
Conversation
41a90f5 to
a237265
Compare
| /// | ||
| /// // In this example, we initialize a KeyMateriol512 (64 bytes) with only 32 bytes of input. | ||
| /// let mut key = KeyMaterial512::from_bytes_as_type( | ||
| /// &[1u8; 32], |
There was a problem hiding this comment.
Ok. Can you fix it, or suggest what the correct style should be?
There was a problem hiding this comment.
Sure, correct coding style according to rustfmt is
let mut key =
KeyMaterial512::from_bytes_as_type(&[1u8; 32], KeyType::BytesFullEntropy).unwrap();
assert_eq!(key.key_len(), 32);| let ret = f(key); | ||
|
|
||
| // to allow nested closures, if this key instance allowed | ||
| // before entering, then leave it. |
There was a problem hiding this comment.
Is there any case where nested hazard ops are allowed?
There was a problem hiding this comment.
I don't have one explicitly in mind, but I if a user wants to do this:
do_hazardous_operations(&mut seed, |seed| {
seed.set_security_strength(SecurityStrength::_256bit)?;
MLDSA87::keygen_from_seed(&seed)?;
Ok(())
});(syntax may not be exactly correct there).
the keygen_from_seed will itself likely use a do_hazardous_operations closure on the same seed object, so that would be a nested closure. Throwing an error about nested closures and forcing the user to figure out how to move that out of the closure just seems like bad ergonomics ... this should just work.
| where | ||
| KEY: KeyMaterialTrait + ?Sized, | ||
| F: FnOnce(&mut KEY) -> Result<(), KeyMaterialError>, | ||
| { |
There was a problem hiding this comment.
Let me check what can be done about dyn. I'm just playing around with do_hazardous_operations, and have realised that it always returns (), which isn't handy, and it should return T.
pub fn do_hazardous_operations<KEY, R, F>(key: &mut KEY, f: F) -> Result<R, KeyMaterialError>
where
KEY: KeyMaterialTrait + ?Sized,
F: FnOnce(&mut KEY) -> Result<R, KeyMaterialError>,
{There was a problem hiding this comment.
Hmm. I think I'll go ahead and merge this PR the way it is, and maybe you can prepare a new PR that explores this idea?
I see the potential benefit in being able to return a value from the closure, but also, won't this require the user to set a type for R every time they use the closure? Won't that be annoying to use?
There was a problem hiding this comment.
I'll fix it after you merge it.
| min(&output_security_strength, &SecurityStrength::from_bits(bytes_written * 8)).clone(), | ||
| ) | ||
| }) | ||
| .expect( |
There was a problem hiding this comment.
This part requires a bit of talk. The library should never abort on error, because that could cause a DoS of the app that is using it. It's better if we return an error here.
bc-rust should go the other way, and that's sweep change focusing on removal of excessive use of .unwrap() or .execpt()
There was a problem hiding this comment.
Good suggestion! I have opened #42 and assigned it to you. Are you willing to start this effort?
Although in this particular case, I disagree that this error should be forwarded to the caller since, as explained in the expect comment, if this ever triggered, it would be because of a programming state error inside this function, not due to bad input from the caller. In other words, this should be infallible, so I will argue that it is correct for the library to swallow this error.
Question: do you think you can construct a test that triggers this expect? That would be the only case in which the err should be returned.
This PR deletes the problematic
KeyMaterial.allow_hazardous_operations()andKeyMaterial.drop_hazardous_operations()in favour of a more elegant closure-based solution.I'm submitting this as a draft with only the core idea in it. If this is in the right direction, then my next steps will be:
allow_hazardous_operations()and.drop_hazardous_operations().In the first attempt, this sample code runs successfully (I'm pulling it out because github doesn't seem to highlight doctests nicely).