Create defaultYamlValue for cardano-testnet #5128
Conversation
Jimbo4350
force-pushed
the
jordan/create-default-yaml-value-cardano-testnet
branch
2 times, most recently
from
e6bb300 to
c1d1d16
Compare
Jimbo4350
requested review from
dcoutts,
erikd,
newhoggy,
JaredCorduan and
CarlosLopezDeLara
as code owners
| . HM.insert "TestAllegraHardForkAtEpoch" (J.toJSON @Int 0) | ||
| . HM.insert "TestMaryHardForkAtEpoch" (J.toJSON @Int 0) | ||
| . HM.insert "TestAlonzoHardForkAtEpoch" (J.toJSON @Int 0) | ||
| . HM.insert "LastKnownBlockVersion-Major" (J.toJSON @Int 6) |
There was a problem hiding this comment.
This code allowed us to choose which eras to hard fork to. Are we dropping this support?
There was a problem hiding this comment.
We aren't. I was going to do a follow up PR implementing a function that will take an era and produce the "correct" yaml configuration value. I can do it here as well however.
There was a problem hiding this comment.
Either that or add a TODO so we don't forget.
| . HM.insert "TestAlonzoHardForkAtEpoch" (J.toJSON @Int 0) | ||
| . HM.insert "LastKnownBlockVersion-Major" (J.toJSON @Int 6) | ||
| H.createDirectoryIfMissing_ logDir | ||
|
|
||
|
|
||
| -- We're going to use really quick epochs (300 seconds), by using short slots 0.2s | ||
| -- and K=10, but we'll keep long KES periods so we don't have to bother | ||
| -- cycling KES keys |
There was a problem hiding this comment.
There's a stray comment here. Looks like intended to comment code that does something necessary so needs to be paired up with code that does the thing it says.
There was a problem hiding this comment.
This comment exists by the shelley genesis creation below (which is where it should be). I'll delete this comment.
| @@ -700,15 +654,17 @@ cardanoTestnet testnetOptions H.Conf {..} = do | |||
| H.assertIO . IO.doesFileExist $ tempAbsPath </> "tx1.tx" | |||
|
|
|||
| -- Add Byron, Shelley and Alonzo genesis hashes to node configuration | |||
| -- TODO: These genesis filepaths should not be hardcoded. Using the cli as a library | |||
| -- rather as an executable will allow us to get the genesis files paths in a more | |||
| -- direct fashion. | |||
There was a problem hiding this comment.
If we test using the library rather than the executable, it would mean that we aren't testing the CLI parsing code. Is this definitely the direction we want to go?
If we do head down this way, there is something we need to watch out for. Some CLI does error handling with error which doesn't give us a clear interface with which to catch errors and handle them. We should probably be more rigorous with how we error in the command implementation and not use error if we go down this way so tests can handle those cases.
There was a problem hiding this comment.
If we test using the library rather than the executable, it would mean that we aren't testing the CLI parsing code. Is this definitely the direction we want to go?
I think so. We can separately test individual commands to see if they parse correctly but its not needed to launch a testnet and should simplify things.
There was a problem hiding this comment.
Some CLI does error handling with error which doesn't give us a clear interface with which to catch errors and handle them. We should probably be more rigorous with how we error in the command implementation and not use error if we go down this way so tests can handle those cases.
Good point 👍
| , ("defaultScribes", defaultScribes) | ||
| , ("setupBackends", Aeson.Array $ Vector.fromList ["KatipBK"]) | ||
| , ("defaultBackends", Aeson.Array $ Vector.fromList ["KatipBK"]) | ||
| , ("options", Aeson.object mempty) |
There was a problem hiding this comment.
It might be useful to have comments here about where these values come from or why these particular values were chosen if that information is available.
| -- The node process can fail on startup, e.g while | ||
| -- parsing the configuration yaml file. If we don't fail | ||
| -- when stderr is populated, we end up having to wait for the | ||
| -- timeout to expire before we can see the error. |
There was a problem hiding this comment.
Maybe if we had a command or function that only parses the configuration file and we can call that before starting the node?
There was a problem hiding this comment.
It seems wrong/not ideal to parse the configuration file twice. We should propagate the exceptions thrown by node.
Jimbo4350
force-pushed
the
jordan/create-default-yaml-value-cardano-testnet
branch
from
c1d1d16 to
3f51575
Compare
Jimbo4350
force-pushed
the
jordan/create-default-yaml-value-cardano-testnet
branch
2 times, most recently
from
dfdc11b to
ef381b9
Compare
Jimbo4350
force-pushed
the
jordan/create-default-yaml-value-cardano-testnet
branch
2 times, most recently
from
5986d40 to
b2a0445
Compare
Jimbo4350
force-pushed
the
jordan/create-default-yaml-value-cardano-testnet
branch
from
b2a0445 to
8142d40
Compare
Previously we were reading configuration files from disk and then modifying the configuration value. This is not ideal as one could not reasonably expect the on disk configuration to be equivalent to what the node's actual configuration is.