-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
forc-crypto: vanity address generation #6661
Conversation
cf8dbc0
to
afd326b
Compare
afd326b
to
9b26daa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @zees-dev, Nice job on your first PR!
I will be leaving a second review that actually checks the code but to start with can you:
- run
cargo clippy
- format the
forc-crypto
Cargo.toml
file, basically the order of dependencies needs to be alphabetical - Ensure that root Cargo.lock file is free of conflicts.
Also if you use closes #6684 in PR description. Github automatically marks the relevant issue to be closed after this PR is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of suggestions. Great to see the benchmarking and tests added for this.
Nice! Quick question - is there a big difference in timings between random vs mnemonic? I'm thinking it would always be a good idea to provide the mnemonic by default and if they're close in timing we should only provide that pathway... but if the performance of random is way faster, then yeah let's keep both. |
There is a difference in timing; quite significant actually; here is benchmark:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
I think only thing left in CI is to fix the tests generated by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Description
Support for Bech32 case-insensitive vanity address generation via
forc-crypto
CLI tool.Usage:
CLI options:
Example outputs
Relevant issues