refactor: fixing log related naming#16882
Conversation
f4d3736 to
e3c1e34
Compare
| const std::unordered_map<CanonicalAddress, DerivedAddress> derived_addresses = { | ||
| { CANONICAL_AUTH_REGISTRY_ADDRESS, AztecAddress("0x1c2474a77af3c756895867fa7622e5e9295121b35849e965f4614d51f00c62e2") }, | ||
| { CONTRACT_INSTANCE_REGISTRY_CONTRACT_ADDRESS, AztecAddress("0x001e627e78063a4944f1331b4c59544f8ad6d6d478f2fc79b025e8240b94b816") }, | ||
| { CONTRACT_INSTANCE_REGISTRY_CONTRACT_ADDRESS, AztecAddress("0x1f4284dea3e475d9d40283c7fe4c1aef5cdea59c012a8fa84c0c9ebe0752327e") }, |
There was a problem hiding this comment.
e3c1e34 to
bd87602
Compare
| @@ -161,7 +160,7 @@ impl LogEncryption for AES128 { | |||
| fn encrypt_log<let PlaintextLen: u32>( | |||
There was a problem hiding this comment.
This will be renamed in a followup PR to MessageEncryption::encrypt.
| // TODO(#12750): The global variables below should not be here as they are AES128 specific. | ||
| // ciphertext_length (2) + 14 bytes pkcs#7 AES padding. | ||
| pub(crate) global HEADER_CIPHERTEXT_SIZE_IN_BYTES: u32 = 16; | ||
|
|
||
| pub global EPH_PK_X_SIZE_IN_FIELDS: u32 = 1; | ||
| pub global EPH_PK_SIGN_BYTE_SIZE_IN_BYTES: u32 = 1; | ||
|
|
||
| // (17 - 1) * 31 - 16 - 1 = 479 | ||
| global MESSAGE_PLAINTEXT_SIZE_IN_BYTES: u32 = (MESSAGE_CIPHERTEXT_LEN - EPH_PK_X_SIZE_IN_FIELDS) | ||
| * 31 | ||
| - HEADER_CIPHERTEXT_SIZE_IN_BYTES | ||
| - EPH_PK_SIGN_BYTE_SIZE_IN_BYTES; | ||
| // Each field of the original note log was serialized to 32 bytes. Below we convert the bytes back to fields. | ||
| // 479 / 32 = 15 | ||
| pub global MESSAGE_PLAINTEXT_SIZE_IN_FIELDS: u32 = MESSAGE_PLAINTEXT_SIZE_IN_BYTES / 32; |
There was a problem hiding this comment.
These values were moved in here from log_encryption.nr and renamed to reflect the message naming. This allowed me to hide the MESSAGE_PLAINTEXT_SIZE_IN_BYTES which I think is a good sign (meaning it was only used in this file and previously imported from log_encryption.nr).
Think this file being called encoding.nr makes this quite a natural place for these constants but if the reviewer have a better idea where to place these I am fine with reworking this.
The HEADER_CIPHERTEXT_SIZE_IN_BYTES being here is ugly but having that in log_encryption.nr was ugly as well so I think I didn't worsen it.
| /// - `PRIVATE_LOG_CIPHERTEXT_LEN`: Fixed length of encrypted logs (defined globally) | ||
| /// - `PRIVATE_LOG_PLAINTEXT_SIZE_IN_FIELDS`: Maximum size of decrypted plaintext (defined globally) | ||
| /// - `MESSAGE_CIPHERTEXT_LEN`: Fixed length of encrypted message (defined globally) | ||
| /// - `MESSAGE_PLAINTEXT_SIZE_IN_FIELDS`: Maximum size of decrypted plaintext (defined globally) |
There was a problem hiding this comment.
| /// - `MESSAGE_PLAINTEXT_SIZE_IN_FIELDS`: Maximum size of decrypted plaintext (defined globally) | |
| /// - `MAX_MESSAGE_PLAINTEXT_LEN`: Maximum size of decrypted plaintext (defined globally) |
This creates a nice simmetry with MESSAGE_PLAINTEXT_LEN no? Without looking at this explanation it's not obvious to me what the difference between MESSAGE_CIPHERTEXT_LEN and MESSAGE_PLAINTEXT_SIZE_IN_FIELDS is - I'd guess that it has to do with fields vs bytes, but that's not it.
There was a problem hiding this comment.
Addressed in e1f6ab9
There is a bit of an inconsistency in naming. For length denominated in fields we use "_LEN" and for length denominated in bytes we use "_SIZE_IN_BYTES". The change would explode the scope of the PR so decided to not do it here and plan on just learning to live with it.
4603717 to
e1f6ab9
Compare
Fixes #15012 The naming became strange with the introduction of the concept of a message that can be emitted either as a private log or as an offchain message - via an offchain effect (also used to be called out-of-band). Before we just used to have a private log. ### Changes - Introduces "alias" for `PRIVATE_LOG_CIPHERTEXT_LEN` called `MESSAGE_CIPHERTEXT_LEN` in `Aztec.nr` - Renames `PRIVATE_LOG_PLAINTEXT_SIZE_IN_{BYTES,FIELDS}` → `MESSAGE_PLAINTEXT_SIZE_IN_{BYTES,FIELDS}`
e1f6ab9 to
05d892d
Compare
Pull Request is not mergeable
Fixes #16885 As promised in [here](#16882 (comment)) in this PR I rename `LogEncryption` trait as `MessageEncryption` and I drop "_log" from it's method names. Didn't replace it with "_message" as it seemed unncessary. I also do some related minor renamings.
Fixes #16885 As promised in [here](#16882 (comment)) in this PR I rename `LogEncryption` trait as `MessageEncryption` and I drop "_log" from it's method names. Didn't replace it with "_message" as it seemed unncessary. I also do some related minor renamings.
Fixes #16885 As promised in [here](#16882 (comment)) in this PR I rename `LogEncryption` trait as `MessageEncryption` and I drop "_log" from it's method names. Didn't replace it with "_message" as it seemed unncessary. I also do some related minor renamings.
Fixes #16885 As promised in [here](#16882 (comment)) in this PR I rename `LogEncryption` trait as `MessageEncryption` and I drop "_log" from it's method names. Didn't replace it with "_message" as it seemed unncessary. I also do some related minor renamings.

Fixes #15012
The naming became strange with the introduction of the concept of a message that can be emitted either as a private log or as an offchain message - via an offchain effect (also used to be called out-of-band). Before we just used to have a private log.
Changes
PRIVATE_LOG_CIPHERTEXT_LENcalledMESSAGE_CIPHERTEXT_LENinAztec.nrPRIVATE_LOG_PLAINTEXT_SIZE_IN_{BYTES,FIELDS}→MESSAGE_PLAINTEXT_SIZE_IN_{BYTES,FIELDS}