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.

Sign up for GitHub

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

Jump to bottom

Implemented the Base Key Types #18

Closed
wants to merge 2 commits into from
Closed

Implemented the Base Key Types #18

wants to merge 2 commits into from

Conversation

nothingalike
Copy link
Member

PrivateKey, PublicKey, and Mnemonic types. Removed the BIP32 functions from the KeyService and converted to extension methods of the key types.

Issue #5

@nothingalike
implemented the base key types
436b46d 
PrivateKey, PublicKey, and Mnemonic types. Removed the BIP32 functions from the KeyService and converted to extension methods of the key types.
tweakch
tweakch approved these changes Jul 30, 2021
View reviewed changes
Copy link
Collaborator

@tweakch tweakch left a comment

Choose a reason for hiding this comment

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

Looking good. I'd appreciate an effort to create a collection of test vectors for these classes.

CardanoSharp.Wallet.Test/AddressTests.cs
@@ -125,11 +121,10 @@ public void VerifyRewardAddress(string mnemonic, string stakingAddr)
/// <param name="path"></param>
/// <param name="rootKey"></param>
/// <returns></returns>
private (byte[], byte[]) getKeyPairFromPath(string path, (byte[], byte[]) rootKey)
private (PrivateKey, PublicKey) getKeyPairFromPath(string path, PrivateKey rootKey)
Copy link
Collaborator

@tweakch tweakch Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Return KeyPair here (ie your suggested type in #5 )

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I wrote this and thought about the Key (better name: KeyPair) type. Alright I get this added.

@nothingalike
Copy link
Member Author

Looking good. I'd appreciate an effort to create a collection of test vectors for these classes.

Awesome, yeah I'll add tests next.

@nothingalike
Copy link
Member Author

Alright... I think I hit a bit of a wall.

  1. What are some good test vectors for the new PrivateKey and PublicKey? Ultimately all the logic isn't necessarily new. Its just moved or restructured. We have existing tests around the Key logic: Generating Mnemonics and Creating RootKeys. Once the WalletPath type is merged, I can update those tests to use the new types as well. @tweakch let me know your thoughts and any advice here

  2. I tried to incorporate SimpleExec but the nature of how we pass parameters to the cardano-address binary presented some serious issues. Since it uses a pipe input, the only solutions I came across was to execute cmd.exe and execute your command as an input. I'm ok with this, but cmd.exe isn't cross platform from my understanding. @safestak-keith got any ideas for this one?

ultimately, I want to watch on #17 before merging this PR since the types added here would be impacted by WalletPath

@nothingalike nothingalike deleted the base-key-types branch August 10, 2021 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tweakch tweakch tweakch approved these changes

@Daniel-SchaeferJ Daniel-SchaeferJ Awaiting requested review from Daniel-SchaeferJ

@safestak-keith safestak-keith Awaiting requested review from safestak-keith

Assignees
No one assigned
Labels
hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nothingalike @tweakch