-
Notifications
You must be signed in to change notification settings - Fork 720
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
cardano-testnet: allow to pass genesis files #5645
Conversation
2a2abde
to
f75e77d
Compare
f75e77d
to
d3f8830
Compare
let testnetMagic = cardanoTestnetMagic testnetOptions | ||
slotLength = cardanoSlotLength testnetOptions | ||
epochLength = cardanoEpochLength testnetOptions | ||
maxLovelaceLovelaceSupply = cardanoMaxSupply testnetOptions | ||
in Api.shelleyGenesisDefaults | ||
in (fromMaybe Api.shelleyGenesisDefaults mBase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the problem here is if a user did provide a shelley genesis we would be overriding several fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this is mitigated by the additional documentation I have added in all the call chain about this.
Also, discussing a similar API with @andreabedini, this overriding is the default expectation of callers (not doing it would be the surprise!).
So I'd keep it like this.
cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Cli/Alonzo/LeadershipSchedule.hs
Outdated
Show resolved
Hide resolved
A viable option is exposing a |
731e6d9
to
84f047e
Compare
=> NumPools | ||
-> AnyCardanoEra -- ^ The era to use | ||
-> ShelleyGenesis StandardCrypto -- ^ The shelley genesis to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this change to avoid mixing CardanoTestnetOptions
and genesis files in the same signature. We should avoid having them together in a signature, because they contain duplicate data, e,g, the network magic is both in CardanoTestnetOptions
and the genesis files.
So if we have them together, we are imposing on the caller to be careful to pass coherent data. This change avoids that, by using smaller types instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the ConwayGenesis
and AlonzoGenesis
parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not expose the data constructor for NumPools
and expose smart constructors instead: e.g numPools :: CardanoTestnetOptions -> NumPoolNodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the ConwayGenesis and AlonzoGenesis parameters?
@Jimbo4350> since create-testnet-data
doesn't yet take alonzo and conway genesis as CLI parameters, they do not make sense here yet. Adding them is tracked by IntersectMBO/cardano-cli#548
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not expose the data constructor for NumPools and expose smart constructors instead: e.g numPools :: CardanoTestnetOptions -> NumPoolNodes
Done 👍
cardanoTestnetDefault :: () | ||
=> CardanoTestnetOptions | ||
-> Conf | ||
-> H.Integration TestnetRuntime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we're not mixing CardanoTestnetOptions
and genesis files, which is good (as I explained elsewhere). This is nice for the vast majority of callers, that don't need to be exposed to a complex API.
=> CardanoTestnetOptions -- ^ The options to use. Must be consistent with the genesis files. | ||
-> Conf | ||
-> UTCTime -- ^ The starting time. Must be the same as the one in the shelley genesis. | ||
-> ShelleyGenesis StandardCrypto -- ^ The shelley genesis to use, for example 'Defaults.defaultShelleyGenesis'. | ||
-- Some fields are overridden by the accompanying 'CardanoTestnetOptions'. | ||
-> AlonzoGenesis -- ^ The alonzo genesis to use, for example 'Defaults.defaultAlonzoGenesis'. | ||
-> ConwayGenesis StandardCrypto -- ^ The conway genesis to use, for example 'Defaults.defaultConwayGenesis'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we mix CardanoTestnetOptions
and genesis files. So callers must be careful to provide coherent data. If they don't, they will get caught by the new checks below. Ideally, in the future, we'll want to remove fields from CardanoTestnetOptions
, so that the intersection with the data of genesis files becomes empty. This a bigger change though, so I chose this version, which is an improvement already, and minimizes disturbing users.
18a2054
to
38c52b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! However you need to make the change regarding my comments about the smart constructors.
=> NumPools | ||
-> AnyCardanoEra -- ^ The era to use | ||
-> ShelleyGenesis StandardCrypto -- ^ The shelley genesis to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the ConwayGenesis
and AlonzoGenesis
parameters?
-> TmpAbsolutePath | ||
-> m FilePath -- ^ Shelley genesis directory | ||
createSPOGenesisAndFiles testnetOptions startTime (TmpAbsolutePath tempAbsPath') = do | ||
createSPOGenesisAndFiles NumPools {unNumPools = numPoolNodes} era shelleyGenesis (TmpAbsolutePath tempAbsPath') = do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(NumPools numPoolNodes)
should be sufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
=> NumPools | ||
-> AnyCardanoEra -- ^ The era to use | ||
-> ShelleyGenesis StandardCrypto -- ^ The shelley genesis to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not expose the data constructor for NumPools
and expose smart constructors instead: e.g numPools :: CardanoTestnetOptions -> NumPoolNodes
38c52b8
to
e49b5c6
Compare
e49b5c6
to
73c883d
Compare
Description
We want to be able to tweak genesis files in
cardano-testnet
. This PR makes it possible to pass custom alonzo, conway, and shelley genesis files tocreateTestnet
. A new functioncreateTestnetDefault
is introduced, that uses default genesis files for all 3.Context
This PR replaces #5643, whose design was deemed unsatisfying.
Checklist