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

Introduce SomeMnemonic as source of root keys (instead of entropy) #1343

Merged
merged 3 commits into from
Feb 15, 2020
Merged

Introduce SomeMnemonic as source of root keys (instead of entropy) #1343

merged 3 commits into from
Feb 15, 2020

Conversation

Anviking
Copy link
Collaborator

@Anviking Anviking commented Feb 13, 2020 •
edited
Loading

Issue Number

#1316, preliminary work to unblock #1321

Overview

  • Add SomeMnemonic as return type of fromMnemonic
  • Make e.g. unsafeGenerateKeyFromSeed take a SomeMnemonic instead
    entropy. This is more similar to Icarus.generateKeyFromHardwareLedger
  • Add genMnemonic helper in a shared location

Comments

@Anviking Anviking force-pushed the anviking/ADP-81/some-mnemonic branch from 039e19a to 8fd159b Compare February 13, 2020 14:48
Anviking
Anviking commented Feb 13, 2020
View reviewed changes
lib/core/test/unit/Cardano/WalletSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/TypesSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/AddressDiscovery/SequentialSpec.hs Outdated

dummyMnemonic :: SomeMnemonic
dummyMnemonic = either (error . show) id $
SomeMnemonic . entropyToMnemonic @12 <$> mkEntropy (BS.pack $ replicate 16 0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Originally seeds of different lengths were used.

Also, matching the 12 with the 16 bytes has to be done manually. We could maybe define a helper:

mkEntropyFromByteStringOfSize @16 $ replicate 16 0

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDiscovery/RandomSpec.hs
arbitraryMnemonic :: [Text]
arbitraryMnemonic =
arbitraryMnemonic :: SomeMnemonic
arbitraryMnemonic = either (error . show) id $ fromMnemonic @'[12]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having to explicitly tell (@'[12]) which mnemonic lengths are supported, when we are hard-coding the mnemonic, could be considered cumbersome.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, use: unsafeMkMnemonic from Cardano.Wallet.Unsafe

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivation/IcarusSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivation/ByronSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs Show resolved Hide resolved
@Anviking Anviking self-assigned this Feb 13, 2020
@Anviking Anviking force-pushed the anviking/ADP-81/some-mnemonic branch from 33e5c18 to fe71acd Compare February 14, 2020 09:38
@Anviking
Copy link
Collaborator Author

bors try

iohk-bors bot added a commit that referenced this pull request Feb 14, 2020
@iohk-bors
Try #1343:
d27b73a
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 14, 2020

try

Build succeeded

@Anviking Anviking requested a review from KtorZ February 14, 2020 10:53
@Anviking Anviking force-pushed the anviking/ADP-81/some-mnemonic branch from ba87902 to 922eed3 Compare February 14, 2020 10:53
KtorZ
KtorZ reviewed Feb 14, 2020
View reviewed changes
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

Looks good overall, but some minor tweaks to go through before merging 👍

@Anviking Anviking force-pushed the anviking/ADP-81/some-mnemonic branch 3 times, most recently from e644810 to 58820d4 Compare February 14, 2020 14:39
@Anviking Anviking requested review from KtorZ and removed request for KtorZ February 14, 2020 15:09
Anviking
Anviking commented Feb 14, 2020
View reviewed changes
lib/core/test/unit/Cardano/Wallet/Primitive/AddressDiscovery/SequentialSpec.hs Outdated

dummyMnemonic :: SomeMnemonic
dummyMnemonic =
unsafeMkSomeMnemonicFromEntropy (Proxy @12) (BS.pack $ replicate 16 0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replacing both 16- and 32- bytes long entropies in this module with 16, seems iffy.

Adding a dummyMnemonic :: (many constraints) => Proxy mw -> SomeMnemonic sounds tempting…

But in-doubt, I will not, I think.

Will remove dummyMnemonic.

@Anviking
Copy link
Collaborator Author

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 14, 2020
@iohk-bors @Anviking
Merge #1343
48caadf 
1343: Introduce SomeMnemonic as source of root keys (instead of entropy) r=Anviking a=Anviking

# Issue Number

#1316, preliminary work to unblock #1321 
<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

- Add SomeMnemonic as return type of fromMnemonic
- Make e.g. unsafeGenerateKeyFromSeed take a SomeMnemonic instead
entropy. This is more similar to `Icarus.generateKeyFromHardwareLedger`
- Add genMnemonic helper in a shared location

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@KtorZ
Copy link
Member

KtorZ commented Feb 14, 2020 •
edited
Loading

(IMO, this is a good case where the last commits don't bring much to the story the PR is telling. Because they're simply correcting stuff introduced earlier in the PR itself. This is a good case for squashing 😉)

@Anviking Anviking force-pushed the anviking/ADP-81/some-mnemonic branch from 9a8b250 to ff18a7f Compare February 14, 2020 15:54
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 14, 2020

Canceled

@Anviking
Copy link
Collaborator Author

Argh! 😄

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 14, 2020
@iohk-bors @Anviking
Merge #1343
1b7dcef 
1343: Introduce SomeMnemonic as source of root keys (instead of entropy) r=Anviking a=Anviking

# Issue Number

#1316, preliminary work to unblock #1321 
<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

- Add SomeMnemonic as return type of fromMnemonic
- Make e.g. unsafeGenerateKeyFromSeed take a SomeMnemonic instead
entropy. This is more similar to `Icarus.generateKeyFromHardwareLedger`
- Add genMnemonic helper in a shared location

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@Anviking
Copy link
Collaborator Author

Because they're simply correcting stuff introduced earlier in the PR itself.

But maybe the indirect path from A to B in the PR was interesting… which the author of the PR might be biased when judging 😉

@KtorZ
Copy link
Member

KtorZ commented Feb 14, 2020 •
edited
Loading

maybe the indirect path from A to B in the PR was interesting

Was it ? ( :trollface: )

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 14, 2020

Build failed

@Anviking Anviking force-pushed the anviking/ADP-81/some-mnemonic branch from c78d1ba to bc410a0 Compare February 15, 2020 21:50
Anviking and others added 3 commits February 15, 2020 22:55
@Anviking @KtorZ
Introduce SomeMnemonic as source for root keys
a2cc401 
- Add SomeMnemonic as return type of fromMnemonic
- Make e.g. unsafeGenerateKeyFromSeed take a SomeMnemonic instead
entropy. This is more similar to `Icarus.generateKeyFromHardwareLedger`

This is from a patch by Matthias without modifications.

Co-authored-by: KtorZ <matthias.benkort@gmail.com>
@Anviking
Fix tests, add unsafeMkSomeMnemonicFromEntropy, unsafeMkEntropy, genM…
b05a262 
…nemonic
@Anviking
Add someDummyMnemonic
984e7cc
@Anviking Anviking force-pushed the anviking/ADP-81/some-mnemonic branch from bc410a0 to 984e7cc Compare February 15, 2020 21:56
@Anviking
Copy link
Collaborator Author

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 15, 2020
@iohk-bors @Anviking
Merge #1343
345c57b 
1343: Introduce SomeMnemonic as source of root keys (instead of entropy) r=Anviking a=Anviking

# Issue Number

#1316, preliminary work to unblock #1321 
<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

- Add SomeMnemonic as return type of fromMnemonic
- Make e.g. unsafeGenerateKeyFromSeed take a SomeMnemonic instead
entropy. This is more similar to `Icarus.generateKeyFromHardwareLedger`
- Add genMnemonic helper in a shared location

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 15, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 984e7cc into master Feb 15, 2020
@Anviking Anviking mentioned this pull request Feb 17, 2020
Merged
3 tasks
@KtorZ KtorZ deleted the anviking/ADP-81/some-mnemonic branch February 24, 2020 08:06
@Anviking Anviking mentioned this pull request Feb 27, 2020
Closed
7 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
(ADP-81) Wallet: Command-line utils f...
Development

Successfully merging this pull request may close these issues.
2 participants
@Anviking @KtorZ