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

[zk-keygen] create cli utility and add new command #31641

Merged
merged 17 commits into from May 21, 2023

Conversation

samkim-crypto
Copy link
Contributor

@samkim-crypto samkim-crypto commented May 15, 2023

Problem

There is currently no cli utility to create encryption keys (ElGamal and authenticated encryption) used for token-2022 and more generally zero-knowledge proofs.

Summary of Changes

Create a zk-keygen cli tool to generate encryption keys and add the new command that randomly generates encryption keys analogously to keygen.

Other commands like recover (recover key/keypair from passphrase) and pubkey (print out pubkey from an ElGamal keypair) will be added in a follow-up PR.

The solana-zk-keygen is marked not for publication for now.

Fixes #

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #31641 (e207a45) into master (531c5eb) will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #31641     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         736      736             
  Lines      207493   207493             
=========================================
- Hits       168755   168746      -9     
- Misses      38738    38747      +9     

.disable_version_flag(true)
.arg(
Arg::new("key_type")
.short('t')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.short('t')

(throughout)

we typically avoid declaring shortopts in the solana tools. a few have sneaked in over the years and they are all ambiguous in some contexts today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! I removed the shortopt for key_type. The rest of the shortops were taken from solana-keygen so we should probably leave it for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd prefer to not propagate any of the short args from solana-keygen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, all shortopts are now removed!

zk-keygen/src/main.rs Outdated Show resolved Hide resolved
zk-keygen/src/main.rs Outdated Show resolved Hide resolved
zk-keygen/src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

one more and i think i'm good

zk-keygen/src/main.rs Outdated Show resolved Hide resolved
samkim-crypto and others added 2 commits May 17, 2023 17:45
Co-authored-by: Trent Nelson <trent.a.b.nelson@gmail.com>
t-nelson
t-nelson previously approved these changes May 17, 2023
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm now, thanks! @CriesofCarrots mind taking a pass?

@CriesofCarrots
Copy link
Contributor

mind taking a pass?

Will do first thing tomorrow!

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Generally looks great! Just a nit plus a "short" whiplash

Comment on lines 53 to 57
Arg::new("outfile")
.long("outfile")
.value_name("FILEPATH")
.takes_value(true)
.help("Path to generated file"),
Copy link
Contributor

Choose a reason for hiding this comment

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

(Sorry in advance for this 😞) I know @t-nelson had you take out all the short definitions, but I think we should preserve this one. I use solana-keygen -o all the time; I think it's intuitive and much easier than typing the long flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also use the -o flag often and it is used quite a bit in the solana docs cli examples as well. I added it back in for now, but @t-nelson please let me know if you feel strongly against this. I'd be happy to go either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

my opinion here isn't strong. would be nice to add it back hidden, but i don't see any obvious what to only hide the short opt with clap v3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yeah I don't quite see an easy way to hide just the short opt. Since this is not marked for publication just yet, let's keep the short opt for now. There will be a chance to discuss it again before publication.

zk-keygen/src/main.rs Outdated Show resolved Hide resolved
zk-keygen/src/main.rs Show resolved Hide resolved
zk-keygen/src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Sorry, noticed one more nit

zk-keygen/Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Tyera <teulberg@gmail.com>
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Lgtm! Quick, merge it before @t-nelson notices the outfile thing ;)

@samkim-crypto samkim-crypto merged commit fd69ae7 into solana-labs:master May 21, 2023
24 checks passed
@samkim-crypto samkim-crypto deleted the zk-keygen branch May 21, 2023 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants