-
Notifications
You must be signed in to change notification settings - Fork 16
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-testnet-data: use experimental API and make arguments era specific #968
create-testnet-data: use experimental API and make arguments era specific #968
Conversation
a53325e
to
cf64c8b
Compare
b9c394f
to
9c3bac2
Compare
cf64c8b
to
e700d78
Compare
a41b90c
to
50bb15c
Compare
50bb15c
to
1075901
Compare
1075901
to
8560a66
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.
Looks good. I left a couple of comments on things that caught my eye, just minor things. Feel free to address them if you think they make sense, but I leave it to your judgment
deriving Show | ||
|
||
instance Show GenesisCreateTestNetDataCmdArgs where | ||
show _ = "GenesisCreateTestNetDataCmdArgs" |
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.
It doesn't derive anymore? It is odd because Era
does have a Show
instance (here), maybe after moving the parameter it will?
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.
@palas> it doesn't derive anymore because there's an existential type in GenesisCreateTestnetDataCmdArgs
now:
<*> pNumCommittee | ||
<*> pNumDReps | ||
<*> (case era of Exp.BabbageEra -> pure 0; Exp.ConwayEra -> pNumCommittee) -- Committee doesn't exist in babbage | ||
<*> (case era of Exp.BabbageEra -> pure $ DRepCredentials OnDisk 0; Exp.ConwayEra -> pNumDReps) -- DReps don't exist in babbage |
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.
Making it unrepresentable on the type would be safer. But this is probably much simpler, so it seems a reasonable trade-off
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.
Those inlined cases are not very readable, can you move them to a values in where
, and just reference them from here?
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'd also use Ord (Some Era)
instead of pattern matching on each era. I mean:
if someEra < Some Exp.ConwayEra
then pure 0
else pNumCommittee
and
if someEra < Some Exp.ConwayEra
then pure $ DRepCredentials OnDisk 0
else pNumDReps
Rationale: less maintenance burden when introducing new eras - you won't have to change each usage site of case
.
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'd also use Ord (Some Era) instead of pattern matching on each era. I mean:
@carbolymer> I'm not sure this is really better, because there are at most two experimental eras at any point in time and when we delete one era, the index of the other era (the one being kept) shrinks from 1 to 0. So I wouldn't rely on the ordering here. I think pattern matching makes the intent more explicit.
Ah, one more thing, probably say what got broken in the change log. (That some eras no longer have the command.) |
…at they are era specific
8560a66
to
7db283b
Compare
-- TODO This existential type parameter should become a regular type parameter | ||
-- when we parameterize the parent type by the experimental era API. | ||
data GenesisCreateTestNetDataCmdArgs = forall era. GenesisCreateTestNetDataCmdArgs | ||
{ eon :: !(Exp.Era era) |
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.
Why not Some Exp.Era
instead?
Changelog
Context
Fixes #962
How to trust this PR
Look at the changes in golden files
Checklist