Dustin-Ray / capyCRYPT

An experimental high-performance cryptosystem.
MIT License
12 stars 1 forks source link

fix: wrap all functions in ops with Result #27

Closed Dustin-Ray closed 11 months ago

Dustin-Ray commented 1 year ago

Use rust's built-in Result wrapper to return success or error depending on result of operation. This library relies heavily on assumptions that inputs are correct, forcing the consumer to do extra work to ensure their input is well-formed. Ultimately, Box wrappers will be converted to Rc wrappers which necessitate avoiding panics wherever possible. Thus, our library should make every attempt to ensure that publicly available apis do not allow malformed input. Examples to follow.

Dustin-Ray commented 1 year ago

Here's a great simple example of how to carry this out:

fn divide(a: f64, b: f64) -> f64 {
    a / b 
}

Should become:

fn divide(a: f64, b: f64) -> Result<f64, String> {
    if b == 0.0 {
        Err(“Division by zero”.to_string())
    } else {
        Ok(a / b)
    }
}
Dustin-Ray commented 1 year ago

Here is a really nice idiomatic way of using match statements in rust to handle all possible outcomes of the result wrapper:

fn test_divide() {
    // Test with valid input
    match divide(10.0, 2.0) {
        Ok(result) => println!("Test passed: 10 / 2 = {}", result),
        Err(err) => println!("Test failed with error: {}", err),
    }

    // Test with invalid input (division by zero)
    match divide(10.0, 0.0) {
        Ok(result) => println!("Test failed: Expected error, got result {}", result),
        Err(err) => println!("Test passed: Expected error, got error '{}'", err),
    }
}

When pursuing this ticket, the wrapper will cause bubble-up changes in the code that will require modifications at higher levels. This is a great exercise to become familiar with the repo, as well as to become familiar with the nice functional-style features that rust offers.

Dustin-Ray commented 1 year ago

Check out this example of wrapping cshake with the Result wrapper in src/ops.rs:

pub fn cshake(x: &mut Vec<u8>, l: u64, n: &str, s: &str, d: u64) -> Result<Vec<u8>, String> {
    if n.is_empty() && s.is_empty() {
        shake(x, l);
    }
    let mut encoded_n = encode_string(&mut n.as_bytes().to_vec());
    let encoded_s = encode_string(&mut s.as_bytes().to_vec());

    encoded_n.extend_from_slice(&encoded_s);

    let bytepad_w = match d {
        256 => Ok(168),
        512 => Ok(136),
        _ => Err("Value of 'd' must be either 256 or 512".to_string()),
    }?;

    let mut out = byte_pad(&mut encoded_n, bytepad_w);

    out.append(x);
    out.push(0x04);
    Ok(sponge_squeeze(&mut sponge_absorb(&mut out, d), l, 1600 - d))
}
Dustin-Ray commented 1 year ago

If you're new to rust, this syntax might look a bit unusual:

let bytepad_w = match d {
        256 => Ok(168),
        512 => Ok(136),
        _ => Err("Value of 'd' must be either 256 or 512".to_string()),
    }?;

The match statement is similar to switch/case and if/else. We only support values per NIST standard of 256 and 512 for example, as input to bytepad, so we match those values to 168 and 136 (sponge rates) respectively. Anything else is matched with the _ wildcard, which we break out with Err.

If you're coming from C or Java for example, the ? at the end is similar to the ternary operator, but with only two possible outcomes and they are not immediately configurable in this case. The way I like to read ? in rust is "Did the operation work? If so, Ok, if not, Err."

Dustin-Ray commented 1 year ago

Labeling this as good first issue because it should be relatively straightforward to carry out, but it will likely turn into a larger project to bubble up all of the result checks. It should be a really nice way to get a total view of the library and all of the functions it offers because you'll have get to jump to into all of them :)

Dustin-Ray commented 1 year ago

the following functions perform checks that panic:

Resolving this issue entails replacing all panic!s with Result wrappers so the library doesnt crash during runtime. We should avoid panic! during runtime wherever possible.

Dustin-Ray commented 12 months ago

working with crypto bigint crate lately and realized this is probably a better way to check any Result that needs to be unwrapped:

use crypto_bigint::{U448, NonZero};

let a = U448::from(8_u64);
let res = NonZero::new(U448::from(4_u64))
    .map(|b| a.div_rem(&b))
    .expect("Division by zero");
Dustin-Ray commented 11 months ago

46 resolves this as idiomatically as possible