-
Notifications
You must be signed in to change notification settings - Fork 143
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 ECDSA key support for yahcli accounts create #17293
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Matt Hess <[email protected]>
Signed-off-by: Matt Hess <[email protected]>
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
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.
Thanks for the quick turn around.
One note on the naming confusion were we do ecdsa logic under an ed25519 class.
Also a request for an additional test.
hedera-node/hapi-utils/src/main/java/com/hedera/node/app/hapi/utils/keys/Ed25519Utils.java
Outdated
Show resolved
Hide resolved
@@ -115,7 +115,7 @@ protected boolean submitOp(HapiSpec spec) throws Throwable { | |||
if (immediateExportLoc.isPresent() && immediateExportPass.isPresent()) { | |||
final var exportLoc = immediateExportLoc.get(); | |||
final var exportPass = finalPassphrase.orElse(immediateExportPass.get()); | |||
exportWithPass(spec, name, exportLoc, exportPass); | |||
exportEd25519WithPass(spec, name, exportLoc, exportPass); |
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.
Is there a missing test in this PR for the new EC case?
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.
Yahcli doesn't have test cases, at least not in the traditional sense. Is there something specific you expected to see?
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.
I guess I was expecting a test that check the command work for the ECDSA case.
There would be a check for a valid account with a valid ECDSA key
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.
I guess I'm not sure I know what you're referring to. Yahcli doesn't have any automated tests. There are some very rudimentary shell scripts, but they look more like basic smoke tests; there are no automated tests set up to run in CI, and no suite of tests to run locally. Yahcli is written for exactly one customer (devops), and is essentially a hack that uses the existing hapi test framework; it's never been deemed worth having any automated testing. We did manually verify the new behavior in testnet, and this feature will presumably be used very soon by devops; I'm not sure how much better we can do with only one development day left.
I suppose we could write a new testing framework specifically for yahcli operations, but we definitely wouldn't be able to write it and get it set up quickly.
Are there other ideas for how we might write and run a quicker test?
Signed-off-by: Matt Hess <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17293 +/- ##
============================================
+ Coverage 67.24% 67.35% +0.11%
- Complexity 21991 22043 +52
============================================
Files 2583 2586 +3
Lines 96307 96384 +77
Branches 10054 10069 +15
============================================
+ Hits 64763 64924 +161
+ Misses 27839 27746 -93
- Partials 3705 3714 +9
|
This PR adds a command line param to Yahcli's
accounts create
command,--keyType
, that allows for creating accounts with either an ED25519 key (default) or an SECP256K1 ECDSA key. Its behavior should match the ED25519 case as closely as possible.Further ECDSA support will be added in #17288.
Closes #17236