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

feat!: expose secret key length as a constant #181

Merged
merged 2 commits into from Jun 6, 2023

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented Jun 5, 2023

Currently, the PublicKey trait exposes the encoded key byte length via the associated KEY_LEN constant, and also provides it via the key_length function. However, the SecretKey trait only exposes its encoded key byte length via the key_length function, which cannot be constant. There are use cases where it's handy to get the key size at compile time, particularly when you want to use arrays and avoid heap allocations.

This PR unifies the PublicKey and SecretKey traits by adding an associated KEY_LEN constant to SecretKey. It then updates the RistrettoSecretKey implementation accordingly.

BREAKING CHANGE: The SecretKey trait now requires a KEY_LEN constant.

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Thanks!

@sdbondi
Copy link
Member

sdbondi commented Jun 6, 2023

Technically, this change is breaking in the sense that if anyone implemented the PublicKey and/or SecretKey traits, their code would not compile after this update. Of course, the impact on anyone is likely zero.

@sdbondi sdbondi changed the title feat: expose secret key length as a constant feat!: expose secret key length as a constant Jun 6, 2023
@sdbondi sdbondi merged commit 90ad63d into tari-project:main Jun 6, 2023
6 checks passed
@AaronFeickert AaronFeickert deleted the secret-key-length branch June 6, 2023 13:12
SWvheerden pushed a commit to tari-project/tari that referenced this pull request Jun 15, 2023
Description
---
Cleans up and improves `EncryptedData` for simpler operations.
Supersedes [PR 5433](#5433).

Motivation and Context
---
A pending PR updates `EncryptedData` to zeroize the plaintext commitment
mask and expose the data as a tuple of byte arrays. However, there are
more improvements and simplifications we can make.

This PR does several things:
- Uses the `const-generics` feature in `borsh` to allow for the use of a
single byte array to represent the entire `EncryptedData` struct
- Performs zeroizing of the plaintext mask during encryption and
decryption
- Uses the work in [another
PR](tari-project/tari-crypto#181) to get the
byte-encoded length of the mask at compile time (instead of relying on a
brittle use of `size_of` that isn't guaranteed to hold)
- Removes heap allocations during encryption and decryption
- Provides `as_bytes` to expose the `EncryptedData` as a byte slice
- Provides `to_bytes` to expose the `EncryptedData` as a byte array
- Cleans up comments and naming for clarity
- Updates faucet output JSON encoding to account for the changes (this
does not affect genesis block data otherwise)

How Has This Been Tested?
---
Existing tests pass.

What process can a PR reviewer use to test or verify this change?
---
Confirm that existing tests pass, and that the listed features are
implemented correctly.

Breaking Changes
---
There are no breaking changes. The existing `EncryptedData` API is
unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants