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

Add AES cipher module #151

Merged
merged 5 commits into from
Apr 18, 2024
Merged

Add AES cipher module #151

merged 5 commits into from
Apr 18, 2024

Conversation

xevisalle
Copy link
Member

@xevisalle xevisalle commented Apr 5, 2024

Resolves: #152

Summary

  • We create the cipher module, which uses AES-GCM for encryption.
  • We use the cipher module throughout the code, removing PoseidonCipher.
  • We remove the Message module, only meant to be used along with the send to / withdraw from contract obfuscated, whose feature will be removed too.

@xevisalle xevisalle self-assigned this Apr 5, 2024
@xevisalle xevisalle linked an issue Apr 5, 2024 that may be closed by this pull request
@xevisalle xevisalle marked this pull request as ready for review April 9, 2024 15:48
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

Some questions:

  • Are those unwrap/expect inside encrypt/decrypt intentional? Wouldn't a Result fit better?
  • Any reason to remove the "deterministic" creation of the note?

src/note.rs Show resolved Hide resolved
src/note.rs Outdated Show resolved Hide resolved
src/cipher.rs Outdated Show resolved Hide resolved
@xevisalle
Copy link
Member Author

Some questions:

  • Are those unwrap/expect inside encrypt/decrypt intentional? Wouldn't a Result fit better?
  • Any reason to remove the "deterministic" creation of the note?

I will think about the errors... probably you are right.

About the deterministic: the cipher module needs randomness to create a nonce. I know that before this was a parameter for the encryption, but as its generation is now very linked to the AES crate we use, I think is better to compute everything into the module. So, this function is no longer "deterministic". However, now the randomness r has to be provided when creating a new note, so I think that the same functionality we had before is still there.

Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

Great work!
A few little comments and generally please try to get rid of the unwrap. Better return a Result and convert the aes-gcm error type into an phoenix-core error type so you can use the ? operator. (deleted all my previous separate comments on this to declutter the review)
And another question, is it possible to name the cipher module encryption? not sure what is the convention here but to me encryption makes more sense

src/crossover.rs Show resolved Hide resolved
src/cipher.rs Outdated Show resolved Hide resolved
src/note.rs Show resolved Hide resolved
src/note.rs Outdated Show resolved Hide resolved
src/note.rs Outdated Show resolved Hide resolved
@xevisalle
Copy link
Member Author

Great work!

A few little comments and generally please try to get rid of the unwrap. Better return a Result and convert the aes-gcm error type into an phoenix-core error type so you can use the ? operator. (deleted all my previous separate comments on this to declutter the review)

And another question, is it possible to name the cipher module encryption? not sure what is the convention here but to me encryption makes more sense

Good, I'll create the errors and use the ?.

Cipher is the algorithm. Encryption is commonly used for the same purpose but AFAIK is a bit more generic. I used it in our context to refer to the pair nonce + ciphertext. I don't have a strong opinion but encryption = encryption::encrypt() doesn't look that nice 😔. In any case, as you prefer!

@moCello
Copy link
Member

moCello commented Apr 10, 2024

Personally I have a preference for encryption::encrypt and in lib.rs have a pub use encryption::{encrypt, decrypt}; re-export.

@xevisalle xevisalle requested a review from moCello April 18, 2024 10:22
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

Nice!
A general remark:
I would really try to avoid try_into. It makes you need to unwrap where it's not really needed.
If you generate an array that has the correct size and then copy the bytes you need, you have more control on what's happening and avoid unwrap.

src/error.rs Outdated Show resolved Hide resolved
src/encryption.rs Outdated Show resolved Hide resolved
src/encryption.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/note.rs Show resolved Hide resolved
src/crossover.rs Outdated Show resolved Hide resolved
src/note.rs Outdated Show resolved Hide resolved
src/note.rs Outdated Show resolved Hide resolved
src/convert.rs Outdated Show resolved Hide resolved
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

Very nice!
one last left-over from the last changes but otherwise approved!

src/error.rs Outdated
use core::fmt;
use dusk_bytes::{BadLength, Error as DuskBytesError, InvalidChar};

extern crate alloc;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this anymore.

src/note.rs Outdated
@@ -93,14 +93,14 @@ impl Eq for Note {}
impl Note {
/// Creates a new phoenix output note
pub fn new<R: RngCore + CryptoRng>(
rng: &mut R,
mut rng: &mut R,
Copy link
Member

Choose a reason for hiding this comment

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

remove mut

src/note.rs Outdated
psk: &PublicKey,
value: u64,
blinding_factor: JubJubScalar,
) -> Self {
let stealth_address = psk.gen_stealth_address(r);
let r = JubJubScalar::random(&mut rng);
Copy link
Member

Choose a reason for hiding this comment

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

the trick is to do &mut *rng

@xevisalle xevisalle merged commit 0daaba6 into master Apr 18, 2024
5 checks passed
@xevisalle xevisalle deleted the aes branch April 18, 2024 15:17
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.

Implement and use the AES cipher instead of PoseidonCipher
3 participants