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.

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

Create Jormungandr Config from command-line options #508

Merged
merged 8 commits into from
Jul 4, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Jul 4, 2019

Issue Number

#357

Overview

  • I have made the --state-dir option default to $HOME/.cardano-wallet

    • Rationale is that is was kinda awkward to have no idea where the files would go if not specified as it relied on the underlying node's default. Now at least, everything goes in a known default location, or, it is user-specified.
    • More importantly, this was needed in order to have a default folder where to put the generated node's configuration! Here again, we could have relied on the node's default itself, but it feels safer and cleaner to have everything in one place.
  • I have generated Jormungandr's configuration file from options passed to the CLI. Before this, we could pass --node-config and --node-port which could contain conflicting values. Indeed, the config file for Jörmungandr contains the url Jörmungandr rest component should listen too and nothing could prevent them to diverge. So, I saw two strategies:

    1. Either, removing --node-port and, parsing the node config yaml file to extract the data we needed. Which isn't a really great option since it would make the serve command a bit inconsistent and requires some ad-hoc parsing of the yaml file which, incidentally, doesn't even have to declare a rest API!

    2. Generate the configuration file on-the-fly ourselves! This way, we are in complete control of its content, can make sure that it declares a rest section and can also have the storage pointing to our --state-dir option! This helps maintaining good consistency and is much more user friendly.

  • I have renamed the --node-secret to --bft-leaders which I find less cryptic and more true about what should really be the content of that file. We aren't accepting any sort of secrets but are really expecting bft leaders keys!

Comments

@KtorZ KtorZ requested review from paweljakubas and piotr-iohk July 4, 2019 12:45
@KtorZ KtorZ self-assigned this Jul 4, 2019
@KtorZ KtorZ force-pushed the KtorZ/jormungandr-config-from-options branch from ed67ac3 to cd70814 Compare July 4, 2019 13:15
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

At first glance it looks good. A lot of details that has to be just put right. I agree fully that dedicated directory that has to be explicitely chosen is good direction. It is good to know where generated node's configuration is located, etc. I wonder how this PR will fit with PORT PR that just landed in master.

@@ -58,12 +65,18 @@ import Data.Proxy
( Proxy (..) )
import Data.Text.Class
( TextDecodingError (..) )
import Servant.Client.Core
Copy link
Contributor

Choose a reason for hiding this comment

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

servant-client should be removed from cabal I think (and also nix then)

Copy link
Member Author

Choose a reason for hiding this comment

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

(done)

@piotr-iohk
Copy link
Contributor

Generating own config is a sensible approach. I'd be nice however to add that info to launch command description so it's more explicit to the user where the config actually is.
I suspect that user's when setting up Jormungandr node will generate their own config, so might be surprised it is not used (i.e. when they try to change something in their config, restart the wallet and there's no effect etc.)

Also, maybe that's something to worry in future, but if the config is regenerated every time there is no way to add some additional stuff their... like trusted peers etc.

@KtorZ
Copy link
Member Author

KtorZ commented Jul 4, 2019

@piotr-iohk

so it's more explicit to the user where the config actually is.
We do already log where the state dir is (as the very first log line printed):

$ cardano-wallet launch --genesis-block block0.bin --bft-leaders secret.yaml 
[iohk.cardano-wallet.launch:Info:ThreadId 4] [2019-07-04 13:42:00.75 UTC] Using state directory: /home/ktorz/.cardano-wallet

Are you thinking of another log line that clearly indicates that we've generated a configuration for Jormungandr and put it under a given location?

I suspect that user's when setting up Jormungandr node will generate their own config, so might be surprised it is not used

Well, the idea for the launch command is really to be a out-of-the-box solution for users who don't really want to deal with the underlying node. For those who generate their own configuration, I'd say that the preferred way is to simply use the serve command, and start the underlying node on their own.

@KtorZ
Copy link
Member Author

KtorZ commented Jul 4, 2019

@paweljakubas

I wonder how this PR will fit with PORT PR that just landed in master.

It fits well because the PORT tests doesn't really do anything with the launch command. Though, I notice an issue with the http-bridge Launcher spec (we need to use temporary state directory now that we are providing a default one, otherwise, each test uses the state dir from the previous test!)

@piotr-iohk
Copy link
Contributor

piotr-iohk commented Jul 4, 2019

Are you thinking of another log line that clearly indicates that we've generated a configuration for Jormungandr and put it under a given location?

That would be nice actually as well, but I was rather thinking that the launch command descritpion could have this note (e.g. Please note that launch will generate config for jormungandr node in $HOME...) -> https://github.com/input-output-hk/cardano-wallet/blob/master/exe/wallet/jormungandr/Main.hs#L189

Well, the idea for the launch command is really to be a out-of-the-box solution for users who don't really want to deal with the underlying node.

Yes! But they still have to generate initial config (genesis block and secrets file) (?) All the sources I'm aware of also say about generating (like here) or generate (like the bootstrap script) the node.configexplicitly. I was thinking it might become confusing.

And therefore I thought this additional note on the CLI help, might be useful (in the log too, sure).

@KtorZ KtorZ force-pushed the KtorZ/jormungandr-config-from-options branch from cd70814 to 9a6d0e8 Compare July 4, 2019 15:50
@KtorZ KtorZ force-pushed the KtorZ/jormungandr-config-from-options branch 2 times, most recently from b04b47e to 0fece11 Compare July 4, 2019 16:13
@KtorZ KtorZ force-pushed the KtorZ/jormungandr-config-from-options branch from 0fece11 to 77ad611 Compare July 4, 2019 16:14
@KtorZ
Copy link
Member Author

KtorZ commented Jul 4, 2019

@piotr-iohk: I added a disclaimer in the footer of the launch command and, an extra log line to inform users about the location of the generated config file:

$ cardano-wallet launch --help
Usage: cardano-wallet launch ([--random-port] | [--port INT]) [--node-port INT]
                             [--state-dir DIR] ([--quiet] | [--verbose])
                             --genesis-block FILE --bft-leaders FILE
  Launch and monitor a wallet server and its chain producers.

Available options:
  -h,--help                Show this help text
  --random-port            serve wallet API on any available port (conflicts
                           with --port)
  --port INT               port used for serving the wallet API. (default: 8090)
  --node-port INT          port used for communicating with the target
                           node. (default: 8080)
  --state-dir DIR          write wallet state (blockchain and database) to this
                           directory (default: "$HOME/.cardano-wallet")
  --quiet                  suppress all log output apart from errors
  --verbose                display debugging information in the log output
  --genesis-block FILE     Path to the genesis block in binary format.
  --bft-leaders FILE       Path to BFT leaders' secrets (.yaml/.json).

Please note that launch will generate a configuration for Jörmungandr in a
folder specified by '--state-dir'.
$ cardano-wallet launch  --genesis-block block0.bin --bft-leaders secret.yaml 
[iohk.cardano-wallet.launch:Info:ThreadId 4] [2019-07-04 19:18:18.81 UTC] Using state directory: /home/ktorz/.cardano-wallet
[iohk.cardano-wallet.launch:Info:ThreadId 4] [2019-07-04 19:18:18.82 UTC] Generated Jörmungandr's configuration to: /home/ktorz/.cardano-wallet/jormungandr-config.json
[iohk.cardano-wallet.launch:Info:ThreadId 4] [2019-07-04 19:18:18.82 UTC] launch:
  - jormungandr
         --genesis-block block0.bin
         --config /home/ktorz/.cardano-wallet/jormungandr-config.json
         --secret secret.yaml
  - cardano-wallet
         serve
         --port 8090
         --node-port 8080
         --database /home/ktorz/.cardano-wallet/wallet.db
         --genesis-hash dba597bee5f0987efbf56f6bd7f44c38158a7770d0cb28a26b5eca40613a7ebd

@KtorZ KtorZ merged commit ab5e556 into master Jul 4, 2019
@KtorZ KtorZ deleted the KtorZ/jormungandr-config-from-options branch July 12, 2019 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants