Port mnemonic module #47

paweljakubas · 2019-03-11T19:42:08Z

Issue Number

Overview

I have removed mnemonicToSeed, mnemonicToAesKey, blake2b from main module and adapted tests accordingly
I have stopped support of Mnemonic to Buildable and ToSchema
I have added executable in Cardano/Wallet/Mnemonic
I have used Mnemonic 15 rather than Mnemonic 12 as default in executable and specMnemonic used in forbidden

Now it works like that:

$ stack build
[pawel@arch cardano-wallet]$ stack exec cardano-generate-mnemonic
["love","dutch","usual","uphold","whale","certain","merry","horror","bulb","injury","school","crane","grass","police","beauty"]

Comments

The module depends on cardano-crypto
I added (AllowAmbiguousTypes needed):

126     :: forall n . (KnownNat n)                                                                                                                                                            
127     => Integer                                                                                                                                                                            
128 ambiguousNatVal = natVal @n Proxy

Now eitherToParse looks like below (relying on Show rather than Buildable):

328 eitherToParser                                                                                                                                                                            
329     :: Show a                                                                                                                                                                             
330     => Either a b                                                                                                                                                                         
331     -> Parser b                                                                                                                                                                           
332 eitherToParser =                                                                                                                                                                          
333     either (fail . show) pure

Prelude is used.

jonathanknowles · 2019-03-12T01:29:08Z

src/Cardano/Wallet/Mnemonic.hs

+    deriving (Show)
+
+
+


Lots of extra blank lines here! Perhaps just have one?

jonathanknowles · 2019-03-12T01:33:20Z

src/Cardano/Wallet/Mnemonic.hs

+    :: Basement.String
+    -> Text
+fromUtf8String =
+    pack . Basement.toList


Writing functions this way is going to use a lot of vertical space. While it's definitely good to wrap once the 80 character limit is exceeded, I think it's not required here.

Perhaps write the above pair of functions like this:

toUtf8String :: Text -> Basement.String toUtf8String = Basement.fromString . unpack fromUtf8String :: Basement.String -> Text fromUtf8String = pack . Basement.toList

I sticked to the types convention , but readjusted definition as requested

jonathanknowles · 2019-03-12T01:37:28Z

src/Cardano/Wallet/Mnemonic.hs

+    Basement.toBytes Basement.UTF8 . mnemonicSentenceToString Dictionary.english
+
+
+


We don't need to leave so much vertical space between functions. One blank line is really enough! :)

jonathanknowles · 2019-03-12T01:42:39Z

src/Cardano/Wallet/Mnemonic.hs

+
+module Cardano.Wallet.Mnemonic
+       (
+       -- * Types


We should avoid variable length indentation here, and use a fixed-width indentation here (4 characters).

https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#avoid-variable-length-indentation

Agreed, let's indent module exports by 4 chars only 👍

jonathanknowles · 2019-03-12T01:44:37Z

src/Cardano/Wallet/Mnemonic.hs

+
+import Prelude
+
+


No blank line required here. From the coding guidelines.

in order to maximise readability, imports should be grouped into two groups, separated by a blank newline.

Explicit imports

Qualified imports

That's the point. import Prelude is the only implicit import why it is standing separate. Then, explicit and quantified related groups go

I see in every src file this convention

jonathanknowles · 2019-03-12T01:47:12Z

src/Cardano/Wallet/Mnemonic.hs

+
+-- | The initial seed has to be vector or length multiple of 4 bytes and shorter
+-- than 64 bytes. Not that this is good for testing or examples, but probably
+-- not for generating truly random Mnemonic words.


Small typo? Perhaps this should be:

"Note that this is good for testing or examples, but probably not for generating truly random Mnemonic words."

jonathanknowles · 2019-03-12T01:50:57Z

src/Cardano/Wallet/Mnemonic.hs

+
+
+newtype MnemonicException csz =
+    UnexpectedEntropyError (EntropyError csz)


What does it mean to have an UnexpectedEntropyError? Perhaps add a comment here with an explanation?

added the explanation

jonathanknowles · 2019-03-12T01:52:21Z

src/Cardano/Wallet/Mnemonic.hs

+    = ErrMnemonicWords MnemonicWordsError
+    | ErrEntropy (EntropyError csz)
+    | ErrDictionary DictionaryError
+    | ErrForbidden


What do these errors mean? I recommend adding some comments here! :)

added the explanation

KtorZ · 2019-03-12T08:13:38Z

cardano-wallet.cabal

    , cborg
    , containers
    , cryptonite
+    , data-default


Let's not, we don't need this. A plain defaultWhatever function is fine.

removed data-default

KtorZ · 2019-03-12T08:14:07Z

cardano-wallet.cabal

@@ -47,6 +53,7 @@ library
    , text
    , time-units
    , transformers
+    , QuickCheck


No QuickCheck / Arbitrary instances in the source please 🙏
Those only makes sense during testing.

KtorZ · 2019-03-12T08:14:54Z

cardano-wallet.cabal

+       , cardano-wallet
+       , aeson
+  hs-source-dirs:
+      src/Cardano/Wallet/Mnemonic


All executables should be located in app/<executable-sub-folder>. Let's try to stick with that 👍

added the executable to app/mnemonic

KtorZ

Thanks for porting this 👍
A few minor adjustments to make sure it fits with our current style and that we don't introduce some dependencies.

KtorZ · 2019-03-12T08:15:36Z

src/Cardano/Wallet/Mnemonic.hs

+
+module Cardano.Wallet.Mnemonic
+       (
+       -- * Types


Agreed, let's indent module exports by 4 chars only 👍

KtorZ · 2019-03-12T08:17:54Z

src/Cardano/Wallet/Mnemonic.hs

+            [ mnemonicToSentence (def @(Mnemonic 12))
+            ]
+    in
+        any (constEq bytes) forbiddenMnemonics


Let's also get rid of this one and make sure (as it's now in the spec) that the mnemonics from the documentation represent invalid mnemonics (so that we don't have to have an extra hard-coded mnemonic here..

in the api spec we have (Mnemonic 15) - you mean - put the exact example from the spec and correct the affected tests, right? I assume so (please correct me if I am wrong)

I mean, not check for forbidden mnemonics. They won't be needed if the mnemonic we put in the documentation is invalid (so by essence, it can't be used) such that we can remove all this logic from our way.

done, added respective test to make sure mnemonic example from API is invalid:

114 it "Mnemonic from Api is invalid" $ do 115 let mnemonicFromApi = 116 "[squirrel,material,silly,twice,direct,slush,pistol,razor,become,junk,kingdom,flee,squirrel,silly,twice]" 117 (mkMnemonic @15 . extractWords) mnemonicFromApi `shouldSatisfy` isLeft

In detail it gives:

Left (ErrEntropy (ErrInvalidEntropyChecksum (Checksum 25) (Checksum 26)))

KtorZ · 2019-03-12T08:18:47Z

src/Cardano/Wallet/Mnemonic.hs

+import Data.Proxy
+    ( Proxy (..) )
+import Data.Text
+    ( Text, pack, unpack )


pack and unpack are ambiguous (same name in all ByteString packages), so let's use qualified imports for them 👍

KtorZ · 2019-03-12T08:19:00Z

src/Cardano/Wallet/Mnemonic.hs

+    ( when, (>=>) )
+import Control.Monad.Catch
+    ( throwM )
+import Crypto.Encoding.BIP39


Let's use explicit imports here 👍

added explicit imports

KtorZ · 2019-03-12T08:20:23Z

src/Cardano/Wallet/Mnemonic/GenerateMnemonic.hs

+main = do
+    backupPhrase <- generateBackupPhrase
+    let backupPhraseString = backupPhraseToString backupPhrase
+    putStrLn backupPhraseString


Minor note: would be better to actually output a Text or a ByteString here. In practice, we probably never wants to output strings. They are inefficient (list of chars) and don't play well in multi-thread contexts.

used Text

KtorZ · 2019-03-12T08:25:15Z

src/Cardano/Wallet/Mnemonic.hs

+        . map (fromUtf8String . dictionaryIndexToWord Dictionary.english)
+        . unListN
+        . mnemonicSentenceToListN
+        . mnemonicToSentence


I'd suggest not to bring any JSON dependencies at this stage and provide instead, serialization combinators that may be use to define JSON instances where it's relevant.

mnemonicToText :: Mnemonic mw -> [Text] mnemonicToText = ... -- or unMnemonic :: Mnemonic mw -> [Text] unMnemonic = ...

added mnemonicToText because used in executable. But left aeson as they are used in the tests

Nope, the Aeson tests should also be removed 👍
They will probably be part of the API tests, but that's the responsibility of the API layer, not this module.

aeson tests and ToJSON, FromJSON instances removed

KtorZ · 2019-03-12T08:25:44Z

test/unit/Cardano/Wallet/MnemonicSpec.hs

+    ( length )
+
+
+import Cardano.Wallet.Mnemonic


Explicit imports here 🙏

KtorZ · 2019-03-12T08:26:18Z

test/unit/Cardano/Wallet/MnemonicSpec.hs

+
+
+import Prelude hiding
+    ( length )


Let's not hide length, but rather use a qualified import on Data.ByteString for that 👍

Anviking · 2019-03-12T01:06:15Z

src/Cardano/Wallet/Mnemonic/GenerateMnemonic.hs

+    putStrLn backupPhraseString
+
+backupPhraseToString :: Mnemonic 12 -> String
+backupPhraseToString backupPhrase = show $ encode backupPhrase


The show, instead of unpack, is a change in behaviour.

stack exec cardano-generate-mnemonic "[\"gaze\",\"envelope\",\"submit\",\"umbrella\",\"diesel\",\"spirit\",\"twelve\",\"tonight\",\"arena\",\"salute\",\"gallery\",\"seminar\"]"

vs

cardano-generate-mnemonic ["moral","glass","grain","sure","weather","awful","scorpion","electric","vanish","injury","few","shell"]

now it is ok:

[pawel@arch cardano-wallet]$ stack exec cardano-generate-mnemonic ["grant","fan","matter","receive","shy","flash","buyer","worth","crater","interest","noble","pink","nurse","budget","pave"]

paweljakubas · 2019-03-12T15:50:08Z

Complied with all remarks (hope so), rebased and squashed

[16] mnemonic module and unit tests working properly [16] add mnemonic executable [16] weeder and hlint fixes [16] one more hlint suggestion [16] move exec to app directory [16] Review changes [16] Removing aeson and resigning from forbiddenMnemonics

paweljakubas · 2019-03-13T05:22:02Z

Removed Aeson instances and forbidenMnemonics from Cardano.Wallet.Mnemonic. Refatored tests and added new one checking if mnemonic from API is really invalid.

Code rebased and squashed.

paweljakubas requested review from Anviking and KtorZ March 11, 2019 19:42

paweljakubas force-pushed the paweljakubas/16/port-mnemonic-module branch from be8da3e to 4661019 Compare March 11, 2019 20:20

KtorZ mentioned this pull request Mar 11, 2019

Porting and cleaning up Cardano.Mnemonic module (incl. corresponding tests) #16

Closed

paweljakubas force-pushed the paweljakubas/16/port-mnemonic-module branch from 4661019 to 2ec79d3 Compare March 11, 2019 20:36

jonathanknowles reviewed Mar 12, 2019

View reviewed changes

KtorZ assigned paweljakubas Mar 12, 2019

KtorZ reviewed Mar 12, 2019

View reviewed changes

KtorZ suggested changes Mar 12, 2019

View reviewed changes

Anviking reviewed Mar 12, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/16/port-mnemonic-module branch from 2ec79d3 to 0f88116 Compare March 12, 2019 15:48

paweljakubas force-pushed the paweljakubas/16/port-mnemonic-module branch from 0f88116 to dee07aa Compare March 13, 2019 05:20

[16] first commit

dee07aa

[16] mnemonic module and unit tests working properly [16] add mnemonic executable [16] weeder and hlint fixes [16] one more hlint suggestion [16] move exec to app directory [16] Review changes [16] Removing aeson and resigning from forbiddenMnemonics

paweljakubas requested a review from KtorZ March 13, 2019 05:43

KtorZ approved these changes Mar 13, 2019

View reviewed changes

KtorZ merged commit 5757ccc into master Mar 13, 2019

KtorZ deleted the paweljakubas/16/port-mnemonic-module branch March 13, 2019 06:50

		Basement.toBytes Basement.UTF8 . mnemonicSentenceToString Dictionary.english



		newtype MnemonicException csz =
		UnexpectedEntropyError (EntropyError csz)

Port mnemonic module #47

Port mnemonic module #47

Conversation

paweljakubas commented Mar 11, 2019 • edited Loading

Issue Number

Overview

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Mar 12, 2019

paweljakubas commented Mar 13, 2019

paweljakubas commented Mar 11, 2019 •

edited

Loading

paweljakubas Mar 12, 2019 •

edited

Loading