cardano-foundation / cardano-wallet

HTTP server & command-line for managing UTxOs and HD wallets in Cardano.
Apache License 2.0
766 stars 214 forks source link

Extend cardano Wallet CLI with selected commands #96

Closed KtorZ closed 5 years ago

KtorZ commented 5 years ago

Context

We'll provide a CLI to interact with the wallet layer from the terminal. The CLI acts as a proxy to the wallet backend server (and therefore, requires the wallet server to be up-and-running) such that every endpoint from the API has an equivalent command in the CLI (which delegates the logic to the API via an HTTP call).

e.g. POST /api/wallets has a corresponding command cardano-wallet wallets create --param1 --param2 --param3

Decision

This first iteration only concerns the following endpoints for which, at least mock API handlers should exists.

Acceptance Criteria

  1. Commands for endpoints listed above must be available in the CLI
  2. There should be an extra command to start the underlying web server (e.g. server start)
  3. We may rename the cardano-wallet-server to simply cardano-wallet for the CLI has a more general purpose than just being a web-server.
  4. Each command should output data in a JSON format, matching the one from the API
  5. Sensitive information (like passphrase or mnemonics) must be prompted and not passed as options or arguments

Development Plan

PR

QA

https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-command-line-interface

akegalj commented 5 years ago

Add options for loading passphrases from files.

I don't believe this is necessary. My experience with CLI tools is that sometimes there is an option to read passphrase from stdin - and then user can redirect file to stdin or similar.

But adding option to store passphrase to the file might not be the wise thing - as it might lead to errors from the user:

We can protect user from the later but there are other ways user can mess up so I would rather not add this option or if we really have to have this feature then reading from stdin should be enough

piotr-iohk commented 5 years ago

I have added some integration tests for CLI #264 (however don't know if it's gonna work, in particular still not sure how to test cardano-wallet wallet create since it is a responsive cli) Also planning actually to create some manual regression test suite and make it part of the Release Checklist. (especially for anything that cannot be covered by integration tests)

Anyway still it would be good perhaps to improve on the code coverage here -> https://coveralls.io/builds/23377405/source?filename=lib/cli/src/Cardano/CLI.hs

KtorZ commented 5 years ago

@piotr-iohk I just realized that we won't get any code-coverage with the CLISpec integration tests. Because stack only collects code-coverage for code that is executed through the test-suites (and here, it's executed through a separate executable).

There seems to be a way to instrument stack to generate coverage reports with extra inputs from other executables. I'll have a look and see whether I can get this to work with shc/coveralls as well!

piotr-iohk commented 5 years ago

Added more integration tests. Still plan to add more, but we can close this one.