Add a cli option for creating a new transaction #225

akegalj · 2019-05-06T19:55:37Z

Issue Number

#96

Overview

I have add a CLI option for creating and sending a new transaction

Comments

rvl

Good start :-)

exe/wallet/Main.hs

rvl · 2019-05-07T04:13:51Z

exe/wallet/Main.hs

+        (wPwd, _) <- do
+            let prompt = "Please enter a passphrase: "
+            let parser = fromText @(Passphrase "encryption")
+            getSensitiveLine prompt Nothing parser


This applies the password validation rules (e.g. 10 chars). But if the password was set through an API call with curl, then it might not validate.

@rvl Not sure to understand what you mean here? What API call with cURL? To the wallet server API ?

I believe this is fixed by Matthias

The code is now asking for the encryption password twice when submitting a transaction.

@rvl I am not sure are you ok with this change?

This applies the password validation rules (e.g. 10 chars). But if the password was set through an API call with curl, then it might not validate.

password wouldn't validate with curl as well because validation rules are enforced in core (have a look at FromText and FromJSON). So this 10 char password isn't enforced only on cardano-wallet CLI client (as I think you might thought)

There are two things here:

1/ We do indeed perform validation at the API-level too. Whether we should allow empty passwords from the CLI or the API is another question, out-of-scope for this PR.

2/ Rodney's second remark is about the small refactoring you did, moving getPassphrase into the where section. This is actually wrong for sending transaction, as we would prompt the passphase twice in that case! We only need to prompt it once when creating transaction. This needs fix.

Ah true - didn't understand what Rodney meant to say

I did not realise the API enforces a 10 char minimum encryption password (weird idea), therefore I thought the CLI was incorrectly using different validation rules to the API.

That's what I thought! We completely share the same decoders in the CLI and API, so any rules applied to decoding values in the API should apply in the CLI.
In the future, we would even enforce some deeper validation rules for the passphrase (comparing against a list of mostly used and enforcing some good entropy in the passphrase) because that's the first step towards securing your wallet as a user.

Truth is, if we don't do that, people put an empty mnemonic and an empty passphrase and then, ask the support and their lawyers why their funds disappeared from their wallet 😞

exe/wallet/Main.hs

lib/cli/src/Cardano/CLI.hs

KtorZ

Some minor changes, after what, we're good to go I believe (haven't tested that in the terminal yet, we may likely have to do some fiddling with the API handler, but that's for later).

exe/wallet/Main.hs

lib/core/src/Cardano/Wallet/Api/Types.hs

lib/core/src/Data/Quantity.hs

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

lib/core/src/Cardano/Wallet/Api/Types.hs

exe/wallet/Main.hs

lib/cli/src/Cardano/CLI.hs

lib/core/src/Cardano/Wallet/Api/Types.hs

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

lib/text-class/src/Data/Text/Class.hs

specifications/api/swagger.yaml

exe/wallet/Main.hs

KtorZ · 2019-05-09T12:39:55Z

exe/wallet/Main.hs

+  --payment <PAYMENT>         address to send to and amount to send separated by @: '<amount>@<address>'
+
+Examples:
+  cardano-wallet transaction create --wallet-id 2512a00e9653fe49a44a5886202e24d77eeb998f --payment 22@2cWKMJemoBam7gg1y5K2aFDhAm5L8fVc96NfxgcGhdLMFTsToNAU9t5HVdBBQKy4iDswL # Create a transaction and send 22 lovelace from wallet-id to specified addres


What about:

Putting the --payment on a new line with a \

Using either a Yoroi address, or an arbitrary shorter string in base58, just to convey the idea, it doesn't have to be a real address

Make sure the last digit of the amount isn't the same digit as the address 😅 ... for enhanced readability

Make sure the last digit of the amount isn't the same digit as the address sweat_smile ... for enhanced readability

makes sense - didn't notice

Putting the --payment on a new line with a \

👍

Using either a Yoroi address, or an arbitrary shorter string in base58, just to convey the idea, it doesn't have to be a real address

👍

KtorZ · 2019-05-09T12:40:53Z

exe/wallet/Main.hs

+        wPwd <- getPassphrase
+        runClient @Transaction Aeson.encodePretty $ createTransaction (ApiT wId) $
+            PostTransactionData
+                -- NOTE: we are sure this will be non-empty


Side-note: for this kind of comment, the reason why we are sure is really what we're interested in 😉

This PR also adds NonEmpty to few functions where approriate.

comments were addressed

KtorZ

Changes look good 👍

akegalj requested a review from KtorZ May 6, 2019 19:55

KtorZ mentioned this pull request May 6, 2019

Extend cardano Wallet CLI with selected commands #96

Closed

10 tasks

rvl previously requested changes May 7, 2019

View reviewed changes

jonathanknowles reviewed May 7, 2019

View reviewed changes

exe/wallet/Main.hs Outdated Show resolved Hide resolved

jonathanknowles reviewed May 7, 2019

View reviewed changes

exe/wallet/Main.hs Outdated Show resolved Hide resolved

jonathanknowles reviewed May 7, 2019

View reviewed changes

lib/cli/src/Cardano/CLI.hs Outdated Show resolved Hide resolved

KtorZ assigned akegalj May 7, 2019

akegalj force-pushed the akegalj/96/transaction-create-cli branch from 301fc60 to d6a821d Compare May 8, 2019 08:55

akegalj requested review from rvl and jonathanknowles May 8, 2019 10:11

KtorZ approved these changes May 9, 2019

View reviewed changes

jonathanknowles previously requested changes May 9, 2019

View reviewed changes

Anviking reviewed May 9, 2019

View reviewed changes

exe/wallet/Main.hs Outdated Show resolved Hide resolved

akegalj added 8 commits May 9, 2019 14:06

Add a cli option for creating a new transaction

eabe431

Remove signed from FromText Natural instance

4a908a8

Rename targets to payments in API and CLI

a083eb4

Comply to 80chars line width

d685b35

Comply to hlint

863c2c3

Comply to 80chars line width

dace9ee

Fix small review points, mostly cosmetic

a14624e

Fix CLI example comment

e575e62

akegalj force-pushed the akegalj/96/transaction-create-cli branch from 19edc64 to e575e62 Compare May 9, 2019 12:06

KtorZ reviewed May 9, 2019

View reviewed changes

akegalj added 2 commits May 9, 2019 15:58

Don't confirm password when creating tx in CLI

60177c5

This PR also adds NonEmpty to few functions where approriate.

Reformat CLI example to max 80chars width

076bea2

akegalj requested a review from jonathanknowles May 9, 2019 17:12

KtorZ approved these changes May 9, 2019

View reviewed changes

KtorZ merged commit b343b3f into master May 9, 2019

KtorZ deleted the akegalj/96/transaction-create-cli branch May 9, 2019 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a cli option for creating a new transaction #225

Add a cli option for creating a new transaction #225

akegalj commented May 6, 2019 •

edited

Loading

rvl left a comment

rvl May 7, 2019

KtorZ May 7, 2019

akegalj May 8, 2019

rvl May 9, 2019

akegalj May 9, 2019

KtorZ May 9, 2019

akegalj May 9, 2019

rvl May 9, 2019

KtorZ May 10, 2019

KtorZ left a comment

KtorZ May 9, 2019

akegalj May 9, 2019

KtorZ May 9, 2019

KtorZ left a comment

Add a cli option for creating a new transaction #225

Add a cli option for creating a new transaction #225

Conversation

akegalj commented May 6, 2019 • edited Loading

Issue Number

Overview

Comments

rvl left a 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

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

akegalj commented May 6, 2019 •

edited

Loading