Skip to content

Conversation

erikd
Copy link
Contributor

@erikd erikd commented Apr 28, 2020

> cabal run cardano-cli:cardano-cli -- shelley genesis create-genesis
Usage: cardano-cli shelley genesis create-genesis --genesis-dir DIR 
                                                  [--genesis-delegates INT] 
                                                  [--start-time UTC_TIME]
                                                  --supply LOVELACE
  Create a Shelley genesis file from a genesis template and
  genesis/delegation/spending keys.

Available options:
  --genesis-dir DIR        The genesis directory containing the genesis template
                           and required genesis/delegation/spending keys.
  --genesis-delegates INT  The number of genesis delegates [default is 7].
  --start-time UTC_TIME    The genesis start time in YYYY-MM-DDThh:mm:ssZ
                           format. If unspecified, will be the current time +30
                           seconds.
  --supply LOVELACE        The initial coin supply in Lovelace which will be
                           evenly distributed across initial stake holders.
  • Requires a genesis.spec.json file.
  • Genesis delegates are created.
  • Bootstrap Utxo addresses are created.
  • Initial distribution is split approximatelt equally between the addresses.

The tests for this code can be run using:

cabal build all && cardano-cli/test/cli/create-genesis/run

@erikd erikd requested review from dcoutts and Jimbo4350 April 28, 2020 06:21
@erikd erikd force-pushed the erikd/cli-next branch 2 times, most recently from 0ad9072 to 54b3925 Compare April 28, 2020 06:52
Copy link
Contributor Author

@erikd erikd left a comment

Choose a reason for hiding this comment

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

Fix that coerce.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Couple of comments & the CI gods need to be pleased but LGTM!

@erikd erikd force-pushed the erikd/cli-next branch 2 times, most recently from 56a8c81 to 604a788 Compare April 28, 2020 08:10
@erikd erikd force-pushed the erikd/cli-next branch 2 times, most recently from 799d1f3 to 80a6e87 Compare April 28, 2020 08:43
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looking great. A few comments below.

@@ -98,8 +99,7 @@ runGenesisCmd (GenesisKeyGenDelegate vk sk ctr) = runGenesisKeyGenDelegate vk sk
runGenesisCmd (GenesisKeyGenUTxO vk sk) = runGenesisKeyGenUTxO vk sk
runGenesisCmd (GenesisKeyHash vk) = runGenesisKeyHash vk
runGenesisCmd (GenesisVerKey vk sk) = runGenesisVerKey vk sk
runGenesisCmd cmd@GenesisCreate{} = liftIO $ putStrLn $ "runGenesisCmd: " ++ show cmd

runGenesisCmd (GenesisCreate gd ms am) = runGenesisCreate gd ms am
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, if you wanted to split this up into more modules, it'd be ok imho to move all the genesis-related commands and this runGenesisCmd dispatcher into the separate Cardano.CLI.Shelley.Run.Genesis module. A similar pattern would work for all the sub-commands of the shelley command.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with this but I could begin this process in a follow up PR.

Comment on lines 113 to 177
readIndexVerKey :: KeyRole -> FilePath -> ExceptT KeyError IO (Int, Ledger.VKey TPraosStandardCrypto)
readIndexVerKey role fpath =
(extractIndex fpath,) <$> readVerKey role fpath

extractIndex :: FilePath -> Int
extractIndex fpath =
case filter isDigit fpath of
[] -> 0
xs -> Prelude.read xs
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we're expecting all .vkey files in the genesis-keys/ and delegate-keys/ dirs to be of the expected key types, then perhaps we can just use the whole filename (base name) string as the key, rather than my original suggestion (as you've got get) of expecting some numeric component to the file name.

So it'd be ok to have genesis-keys/foo.vkey and delegate-keys/foo.vkey.

Comment on lines 91 to 148
-- The mapMaybe will silently drop any map elements for which the key does not exist
-- in both maps.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should complain instead. We can compare the difference in key sets both ways and report the missing ones.

Comment on lines 50 to 51
forM_ [ 1 .. count ] $
createRequiredKeys gendir
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so now this means the behaviour is it always automatically makes the keys (and then finds them again below).

Perhaps we can easily allow for both manually creating the keys and then automatically making the genesis, and also this do-it-all-in-one-go behaviour of making the keys and the genesis.

If we were to make the default for the --genesis-delegates be 0, then the forM_ [1 .. 0] will do nothing by default, and will create N keys when specified, and so we can get both semi and fully automatic for cheap.

<> "Encountered: " <> (T.pack $ Prelude.show invalid)
<> "Encountered: " <> (Text.pack $ Prelude.show invalid)

instance Crypto crypto => ToJSON (ShelleyGenesis crypto) where
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to move this here? Could we not keep it in a separate Shelley specific module? It's quite chunky, and it is genesis specific. No objections to having "orphan" in the module name, but just because something is an orphan doesn't mean we should chuck everything in together in one big sin bin.

@@ -0,0 +1,85 @@
{-# LANGUAGE OverloadedStrings #-}

module Cardano.Config.Shelley.Address
Copy link
Contributor

Choose a reason for hiding this comment

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

For addresses, as far as possible we should be trying to reuse things from our client API, since that handles addresses and (will) cover both Byron and Shelley addresses.

Showing we can reuse that module shows some sense of completeness & usefulness of that API. It's a good sanity check on that API to reuse it here.

Comment on lines 161 to 162
readAsGenesisVerKey :: KeyRole -> FilePath -> ExceptT KeyError IO GenesisVerKey
readAsGenesisVerKey role fp = do Ledger.VKey genVerKey <- readVerKey role fp
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
readAsGenesisVerKey :: KeyRole -> FilePath -> ExceptT KeyError IO GenesisVerKey
readAsGenesisVerKey role fp = do Ledger.VKey genVerKey <- readVerKey role fp
readGenesisVerKey :: KeyRole -> FilePath -> ExceptT KeyError IO GenesisVerKey
readGenesisVerKey role fp = do Ledger.VKey genVerKey <- readVerKey role fp

to match the naming convention

import Control.Monad.Fail (fail)

import Cardano.Crypto.Hash.Class as Crypto
import Cardano.Config.Orphanage ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just keep the genesis orphan instance here in this module and rename the module something like Cardano.Config.Shelley.GenesisOrphans? It'd keep the module structure much clearer. I don't like us mixing the Byron and Shelley things unnecessarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that makes sense, I'll make the change.

@@ -23,7 +23,7 @@ import qualified Data.Set as Set

import Cardano.TracingOrphanInstances.Common
import Cardano.TracingOrphanInstances.Consensus ()
import Cardano.Config.Shelley.Genesis ()
import Cardano.Config.Orphanage ()
Copy link
Contributor

Choose a reason for hiding this comment

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

😢

@erikd erikd force-pushed the erikd/cli-next branch 2 times, most recently from e700648 to 8d96a96 Compare April 28, 2020 23:46
@ArturWieczorek
Copy link
Contributor

Just did a quick check and I was able to generate genesis.json - example below.

One question - do we want to allow overwriting existing genesis.json without giving any info if file already exists in specified location or we don't really care about this case ?

Example:
cardano-cli shelley genesis create-genesis --genesis-dir genesis --genesis-delegates 3 --start-time 2020-04-29T02:08:59Z --supply 100

{
    "DecentralisationParam": 0.8,
    "ActiveSlotsCoeff": 6.259,
    "ProtocolMagicId": 838299499,
    "StartTime": "2020-04-29T02:08:59Z",
    "GenDelegs": {
        "d8de28340b176c637c237c2b093d8d8c32ccedd0069289ddf88f4a486c88a350": "fceaaf665bbbb2e85b9d5ff295dce57f2ea9b2434af071c625d5b634a0f4fb96",
        "db6cc85bdbce5f5c5f446c47c04554cb12db8ce7118e65bd247471dad39dcf83": "6a0d3865d391b1e909cf8931fd93811ae877c00ca2b1beeab2ee18efb24f445c",
        "482ff49580223df157e89e284f6e3bbd230d6213f4991187def17a569a5f9bf3": "1a0842e41ad97380308955f0baf344d4936f23ea10b5aa48cac51d65c9cbeb32"
    },
    "UpdateQuorum": 12,
    "MaxHeaderSize": 8192,
    "MaxMajorPV": 25446,
    "MaxBodySize": 2097152,
    "MaxLovelaceSupply": 100,
    "InitialFunds": {
        "82085820cab9f8e28f141811f65ed8ea4ce47420774680ece1b903e219dc7a3427bd846b": 33,
        "82085820c4c70e549c12967a7a31c952d9ac85d57d4b72fe410a56aa247cd90ec1ad2b76": 33,
        "82085820125f074c70b659b838262f8ca4e522958a57c50dccc6ead6abc991ebb0302297": 34
    },
    "NetworkMagic": 4036000900,
    "EpochLength": 21600,
    "SlotLength": 20000,
    "SlotsPerKESPeriod": 216000,
    "MaxKESEvolutions": 1080000,
    "SecurityParam": 1
}

@erikd
Copy link
Contributor Author

erikd commented Apr 29, 2020

do we want to allow overwriting existing genesis.json without giving any info if file already exists in specified location or we don't really care about this case ?

I really don't think we care in this case. The only people using this command are devs and very advanced users who want to run their own Shelley testnet.

@erikd erikd force-pushed the erikd/cli-next branch 3 times, most recently from 08f37f7 to 5e63dae Compare April 29, 2020 06:42
@erikd
Copy link
Contributor Author

erikd commented Apr 29, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Apr 29, 2020
852: First working draft of Shelley 'create-genesis' command r=erikd a=erikd

```
> cabal run cardano-cli:cardano-cli -- shelley genesis create-genesis
Usage: cardano-cli shelley genesis create-genesis --genesis-dir DIR 
                                                  [--genesis-delegates INT] 
                                                  [--start-time UTC_TIME]
                                                  --supply LOVELACE
  Create a Shelley genesis file from a genesis template and
  genesis/delegation/spending keys.

Available options:
  --genesis-dir DIR        The genesis directory containing the genesis template
                           and required genesis/delegation/spending keys.
  --genesis-delegates INT  The number of genesis delegates [default is 7].
  --start-time UTC_TIME    The genesis start time in YYYY-MM-DDThh:mm:ssZ
                           format. If unspecified, will be the current time +30
                           seconds.
  --supply LOVELACE        The initial coin supply in Lovelace which will be
                           evenly distributed across initial stake holders.
```

* Requires a `genesis.spec.json` file.
* Genesis delegates are created.
* Bootstrap Utxo addresses are created.
* Initial distribution is split approximatelt equally between the addresses.

The tests for this code can be run using:
```
cabal build all && cardano-cli/test/cli/create-genesis/run
```


Co-authored-by: Erik de Castro Lopo <erikd@mega-nerd.com>
Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 29, 2020

Canceled

erikd and others added 2 commits April 29, 2020 17:33
config: Moved orphan instances into the orphange.
config: Created a module reading, writing and generation of Addresses.
cli: Uses addresses for Utxo bootstrap distribution.
@ArturWieczorek
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 29, 2020

@iohk-bors iohk-bors bot merged commit d3e38fb into master Apr 29, 2020
@iohk-bors iohk-bors bot deleted the erikd/cli-next branch April 29, 2020 10:17
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.

6 participants