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: implement display/debug traits for private keys #30

Merged
merged 8 commits into from Sep 15, 2022

Conversation

erwanor
Copy link
Contributor

@erwanor erwanor commented Sep 5, 2022

Fixes #14, at least partially! For the public keys, base64 will have to do until #1 is resolved.

For consideration: do you think a Secret derive macro that:

  1. derives Zeroize
  2. implements safe Display and Debug
  3. implements unsafe_write_secret: &self -> String (for debugging)

makes sense? I could slot it into this PR. This would make the code nicer:

#[derive(SecretMaterial)]
 pub struct BLS12381PrivateKey {
     pub privkey: blst::SecretKey,
     pub bytes: OnceCell<[u8; BLS_PRIVATE_KEY_LENGTH]>,
     ...
}

@erwanor
Copy link
Contributor Author

erwanor commented Sep 5, 2022

Reviewers: the first commit fixes cargo test

@punwai
Copy link
Contributor

punwai commented Sep 5, 2022

This is perfect @erwanor. It would be super nice to have a test or two. Otherwise, the PR looks good to me!

@erwanor
Copy link
Contributor Author

erwanor commented Sep 5, 2022

Ah yeah good point, added a test for each.

src/ed25519.rs Outdated Show resolved Hide resolved
src/bls12381.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Thanks @erwanor ! This is good, but a tad tedious to implement for each scheme, would it be worth implementing those as a proc-macro à la Diem?

@erwanor
Copy link
Contributor Author

erwanor commented Sep 6, 2022

Neat thanks, I'll refactor with those macros

huitseeker
huitseeker previously approved these changes Sep 11, 2022
kchalkias
kchalkias previously approved these changes Sep 13, 2022
Cargo.toml Outdated
@@ -36,6 +36,7 @@ blst = "0.3.10"
digest = "0.10.3"
once_cell = "1.13.1"
readonly = "0.2.2"
diem-crypto-derive = "0.0.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd personally avoid a diem dependency, especially because this code part is not complex to be ported (copy pasted with a comment reference) without maintainability issues. However, we can do in an upcoming PR. If we do in upcoming PR, please merge and add a github issue.

Copy link
Contributor Author

@erwanor erwanor Sep 13, 2022

Choose a reason for hiding this comment

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

I can do it now, but macros need their own special crate type, so we will have a nested crate fastcrypto::crypto_derive.

I'll push the changes shortly.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

😍

// Imported from diem-crypto-derive@0.0.3
// https://github.com/diem/diem/blob/release-1.4.3/crypto/crypto-derive/src/lib.rs#L113

#[proc_macro_derive(SilentDisplay)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! thanks

@kchalkias kchalkias merged commit 244e9bf into MystenLabs:main Sep 15, 2022
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.

[crypto] Implement display for our keys
4 participants