Add foundation & initial implementation for a full-blown wallet CLI #133

jonathanknowles · 2019-03-29T07:26:31Z

Experimental!

Issue Number

Overview

Add beginnings of a docopt-based CLI for the Wallet.

KtorZ · 2019-03-29T07:36:40Z

app/wallet/USAGE.txt

+
+Usage:
+  cardano-wallet address list --wallet-id=UUID
+  cardano-wallet wallet create --name=STRING --mnemonic-sentence=STRING --passphrase=STRING [--address-pool-gap=INT] [--mnemonic-second-factor=STRING]


The mnemonics will have to be prompted 😶 These are sensitive pieces of info we do not want in the bash history 😅

The mnemonics will have to be prompted no_mouth These are sensitive pieces of info we do not want in the bash history sweat_smile

That's a really good point. I'm happy to add this. Is there any particular approach you would prefer for reading the mnemonics in?

Just a plain shell prompt, doesn't necessarily have to be hidden (because it may become very tricky to type-in 24 words if you can see shit 😂 )
But clearing the prompt once done.

$ Please enter your 12-to-24-word mnemonics: ...

|
|
v

$ Please enter your 12-to-24-word mnemonics: patate patate patate patate

|
|
v

hits enter

|
|
v

Please enter your 12-to-24 word mnemonics: **** Thank you, now we've got all your ADA in a safe place

The mnemonics will have to be prompted 😶
These are sensitive pieces of info we do not want in the bash history 😅
But clearing the prompt once done.

@KtorZ I've now implemented a basic version of the above. It's still a bit rough at the edges (in particular, with the reporting of errors), but clearing the prompt does work.

One issue that I ran into when clearing data from a terminal screen is that it's not always possible to determine the original position of the cursor. If we're not careful, we run the risk of not clearing all the sensitive data from the display.

Therefore, if the system on which the CLI is running is not able to provide this info, I've opted to clear the entire screen after the user has finished entering their data.

KtorZ · 2019-03-29T07:37:27Z

app/wallet/Main.hs

+{-# LANGUAGE DuplicateRecordFields #-}
+{-# LANGUAGE QuasiQuotes #-}
+
+module Main where


Side-note but this cli and cardano-wallet-server are one and one thing. We do want to have them merged together in the end, so that there's one CLI to spawn the server, and the same one to interact with it 👍

Side-note but this cli and cardano-wallet-server are one and one thing. We do want to have them merged together in the end, so that there's one CLI to spawn the server, and the same one to interact with it +1

Understood! In that case, are you expecting something like this:

cardano-wallet server start ...
cardano-wallet wallet create --name wibble ...
cardano-wallet wallet list

Sounds reasonable to me :)

KtorZ · 2019-04-02T18:11:27Z

src/Cardano/Wallet/Api/Types.hs

+
+-- | Indicates an error that occurred while decoding from text.
+newtype TextDecodingError = TextDecodingError
+    { getTextDecodingError :: String }


Not sure what's the value of the newtype here. I'd simply go for a string to be honest. It adds value when we have multiple ones, one per actual type (albeit having a raw string isn't really the best option). But since it has now become a common abstraction, a String has pretty much the same value as the newtype here.

I think there is some value in having a newtype here.

The descriptive name of the type TextDecodingError is self-commenting. When I see this type in the wild somewhere, I don't need to think very hard to know that it's an error relating to the decoding of text, and not some random string that happens to have been inserted into the left-hand-side of an Either.

If I see TextDecodingError in the wild, in a place outside of the Api.Types module, then I have a direct way to look up its definition, at which point I can see the extended comment.

If I were to try refactor TextDecodingError in some way, then I might get better compiler feedback than if it were just a naked String.

What do you think? Of course, if you think the arguments I've presented have no basis, then we can remove it.

(Note that I've now created a separate PR for the textual encoding work: #142.)

KtorZ · 2019-04-02T18:12:18Z

src/Cardano/Wallet/Api/Types.hs

-    , encodeApiWalletName
+    , ToText (..)
+    , FromText (..)
+    , TextDecodingError (..)


The overall looks clean and re-usable. Nice work!
(one reserve about the TextDecodingError)

Thanks! I've moved the textual encoding work into a separate PR: #142.

I'll keep this PR for the docopt CLI-specific work.

This function reads a line containing sensitive data from the terminal. The terminal lines containing the data are cleared once the user has finished entering their data.

KtorZ

Reviewed & Wrapped up.
I'll add a few more tests and merge it.

akegalj · 2019-04-26T15:37:30Z

app/Cardano/CLI.hs

+-- The terminal lines containing the data are also cleared if the application
+-- exits abnormally, before the user has finished entering data.
+--
+getLineWithSensitiveData :: IO Text


alternitavelly we could even hide user input from the screen (similar like sudo does).

The cons of hiding the user input is that user is unable to check/verify/easily-correct entered mnemonic or password.

Some CLI application approach the problem of requesting the same input data to be done twice in order to confirm that user didn't make a mistake:

Passphrase: ******** Retype passphrase: ********

user experience is lower with this model but users are now able to enter their their mnemonics or passphrases in public spaces.

Mnemonics are pretty long (15+ words) and passphrase as well (10+ chars) so there is a high chance of mistake. That's also the case not to implement what I am proposing.

I guess CLI will most likely be used by someone already more technically advanced - so there is a higher chance that person already is aware that mnemonic and passphrase is something he should hide and not reveal (ie, type in public place).

Conclusion is that I think this is ok solution but maybe we should keep in mind and research alternatives.

KtorZ reviewed Mar 29, 2019

View reviewed changes

KtorZ mentioned this pull request Mar 29, 2019

Extend cardano Wallet CLI with selected commands #96

Closed

10 tasks

KtorZ assigned jonathanknowles Mar 29, 2019

KtorZ mentioned this pull request Mar 29, 2019

[wip] Add the beginnings of a wallet CLI. #129

Closed

jonathanknowles force-pushed the jonathanknowles/cli-docopt branch 4 times, most recently from 642050b to b47368d Compare April 2, 2019 09:09

KtorZ reviewed Apr 2, 2019

View reviewed changes

jonathanknowles force-pushed the jonathanknowles/cli-docopt branch from b47368d to 88de8b4 Compare April 3, 2019 02:39

jonathanknowles mentioned this pull request Apr 3, 2019

Add FromText and ToText classes to Api.Types #142

Merged

jonathanknowles force-pushed the jonathanknowles/cli-docopt branch 4 times, most recently from d82c76c to 21f644e Compare April 3, 2019 08:06

jonathanknowles requested a review from KtorZ April 3, 2019 09:25

jonathanknowles and others added 6 commits April 3, 2019 14:10

Add the beginnings of a docopt-based CLI for the Wallet.

28420c9

Add function getLineWithSensitiveData.

1351384

This function reads a line containing sensitive data from the terminal. The terminal lines containing the data are cleared once the user has finished entering their data.

Add structure to the docopt-based CLI module.

f3ba92a

Add functionality to gather sensitive data for wallet creation.

8e278f8

unify CLIs approaches and CLI themselves into on 'cardano-wallet' CLI

c0a52e2

move 'FromText' and 'ToText' into their own modules

09c17d5

KtorZ approved these changes Apr 3, 2019

View reviewed changes

KtorZ marked this pull request as ready for review April 3, 2019 16:49

move 'FromText' & 'ToText' instance closer to their corresponding types

8b8dbfe

KtorZ force-pushed the jonathanknowles/cli-docopt branch from 8114af7 to 8b8dbfe Compare April 3, 2019 16:59

KtorZ changed the title ~~[wip] Add beginnings of docopt-based CLI for the Wallet.~~ Add foundation & initial implementation for a full-blown wallet CLI Apr 3, 2019

KtorZ merged commit efde04f into master Apr 3, 2019

KtorZ deleted the jonathanknowles/cli-docopt branch April 3, 2019 17:23

akegalj reviewed Apr 26, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add foundation & initial implementation for a full-blown wallet CLI #133

Add foundation & initial implementation for a full-blown wallet CLI #133

jonathanknowles commented Mar 29, 2019

KtorZ Mar 29, 2019

jonathanknowles Mar 29, 2019

KtorZ Mar 29, 2019 •

edited

Loading

jonathanknowles Apr 3, 2019 •

edited

Loading

KtorZ Mar 29, 2019

jonathanknowles Mar 29, 2019

KtorZ Mar 29, 2019

KtorZ Apr 2, 2019

jonathanknowles Apr 3, 2019

KtorZ Apr 2, 2019

jonathanknowles Apr 3, 2019

KtorZ left a comment

akegalj Apr 26, 2019

Add foundation & initial implementation for a full-blown wallet CLI #133

Add foundation & initial implementation for a full-blown wallet CLI #133

Conversation

jonathanknowles commented Mar 29, 2019

Issue Number

Overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

jonathanknowles Apr 3, 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

KtorZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Mar 29, 2019 •

edited

Loading

jonathanknowles Apr 3, 2019 •

edited

Loading