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

Vertaisarvionti/peer review #3

Open
wathenro opened this issue Jun 17, 2023 · 0 comments
Open

Vertaisarvionti/peer review #3

wathenro opened this issue Jun 17, 2023 · 0 comments

Comments

@wathenro
Copy link

Project downloaded 16.6.2023 at 8:58.

Hi,

Thank you for the nice work. This was my first introduction to Rust so I had to take a crash course, but that was no problem as everything unfolded quite nicely from the link you provided.

As said, I was a complete rustling starting with your code so I am not an expert commenting. My first thought was that I wish it was a little but more commented, but I think your functions are well named and so short and simple that it is pretty easy to follow what is going on.

I installed Rust and was able to run your program as instructed. Some of my feedback is on the functionality rather than the algorithms, sorry for that. Your program collapses if one gives a letter in the menu. Of course one should not as the instructions are quite clear. ;-) Also, if you choose 1 you seem to be getting into infinite loop. I mean encrypting works and all, but you need to feed a message after another.

There is no way out of here is there?

        loop {
            println!("Enter a message to encrypt: ");
            io::stdin()
                .read_line(&mut message)
                .expect("Failed to read line");
            let message_uint: num_bigint::BigUint = Encrypt::convert_text_to_int(&message);
            let message_uint_encrypted: num_bigint::BigUint =
                Encrypt::encrypt(message_uint.clone(), n.clone(), e.clone());
            println!("Encrypted message: {}", message_uint_encrypted);
        }

If you choose very small prime sizes like 1-5, you seem to get into somekind of loop. There may be an obvious reason for this, I did not dive deeper into that.

It would be great if the program printed the keys it uses in encryption/decryption. Kind of missing the point of keeping things secret, I know, but would make following and understanding easier.

I am not sure if I got it right, I spent a quite a bit of time on it, but the function demo_with_user_keys() is not yet doing what it is supposed to be doing. If so, it would be great if this was mentioned in the code. This with the assumption of course that someone else will read the code during the development. I found reference to this in your weekly report, which was kinda relief. :-)

If I choose number 2 from the menu, the program panics and crashes, which I think is funny. But I think the reason is that you have no reading of user input? Probably has been accidentaly deleted if I had to guess. The version on github looks like this:

    fn decrypt_and_print() {
        let n_string = String::from("Enter n");
        let d_string = String::from("Enter d");
        let n: BigUint = BigUint::parse_bytes(&n_string.into_bytes(), 10).unwrap();
        let d: BigUint = BigUint::parse_bytes(&d_string.into_bytes(), 10).unwrap();
        let message_string = String::from("Enter message");
        let message: BigUint = Encrypt::convert_text_to_int(&message_string);
        println!("{}", Decrypt::decrypt(message, d, n));
    }

Then I wondered if this small part could be written more simply:

        let x_copy = x.clone();
        //if x is negative, add phi to it
        if x < BigInt::zero() {
            x = x + phi_copy;
            return Self::convert_bigint_to_biguint(x);
        }
        //return x as biguint
        return Self::convert_bigint_to_biguint(x_copy);

as

       
        //if x is negative, add phi to it
        if x < BigInt::zero() {
            x = x + phi_copy;
        }
        return Self::convert_bigint_to_biguint(x);

It probably has to do with mutability or something that you have done as you have, but would seem logical to me that if x is mutable and can be sent to convert_bigint_to_biguint(x) why make a copy of that. But then again, I may be completely lost here and talk about stuff I do not know all/anything about.

I checked your tests with typing ‘cargo test’ hoping that it is all needed. All tests passed fine and seem to me relevant.

It seems to me that the important stuff, the algorithms are implemented correctly and the project looks very good, I wish mine was at that stage. It was a pleasure reading this. Out of the scope I was very happy being introduced to Rust.

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

No branches or pull requests

1 participant