-
Notifications
You must be signed in to change notification settings - Fork 15
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
stake-address registration-certificate: do not allow key-reg-deposit-amt before conway #509
stake-address registration-certificate: do not allow key-reg-deposit-amt before conway #509
Conversation
68bd79c
to
d0f4c75
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.
If I understand #504, and the function createRegistrationCertRequirements
correctly, we can (should!) go even further. The --key-reg-deposit-amt
flag should be present only on the Conway command line (and the legacy one, where we can't avoid it...), shouldn't it?
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.
Couple of code style comments.
(\babbageOnwards -> | ||
let babbageOrConway = babbageEraOnwardsToShelleyBasedEra babbageOnwards | ||
babbageBuilder si = StakeAddressRegistrationCertificateCmd babbageOrConway si Nothing | ||
conwayBuilder si lovelace = StakeAddressRegistrationCertificateCmd babbageOrConway si (Just lovelace) | ||
in make $ case babbageOnwards of | ||
BabbageEraOnwardsBabbage -> babbageBuilder <$> pStakeIdentifier <*> pOutputFile | ||
BabbageEraOnwardsConway -> conwayBuilder <$> pStakeIdentifier <*> pKeyRegistDeposit <*> pOutputFile | ||
) | ||
sbe | ||
where | ||
desc = Opt.progDesc "Create a stake address registration certificate" | ||
make parser = subParser "registration-certificate" (Opt.info parser desc) |
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.
This code is a bit hard to follow and reason about. How about something simpler, using forEraInEonMaybe
:
(\babbageOnwards -> | |
let babbageOrConway = babbageEraOnwardsToShelleyBasedEra babbageOnwards | |
babbageBuilder si = StakeAddressRegistrationCertificateCmd babbageOrConway si Nothing | |
conwayBuilder si lovelace = StakeAddressRegistrationCertificateCmd babbageOrConway si (Just lovelace) | |
in make $ case babbageOnwards of | |
BabbageEraOnwardsBabbage -> babbageBuilder <$> pStakeIdentifier <*> pOutputFile | |
BabbageEraOnwardsConway -> conwayBuilder <$> pStakeIdentifier <*> pKeyRegistDeposit <*> pOutputFile | |
) | |
sbe | |
where | |
desc = Opt.progDesc "Create a stake address registration certificate" | |
make parser = subParser "registration-certificate" (Opt.info parser desc) | |
(\_babbageOnwards -> do | |
let mpKeyRegistDeposit = | |
sequenceA $ forEraInEonMaybe | |
@ConwayEraOnwards | |
era | |
(const pKeyRegistDeposit) | |
make $ StakeAddressRegistrationCertificateCmd sbe | |
<$> pStakeIdentifier | |
<*> mpKeyRegistDeposit | |
<*> pOutputFile | |
) | |
sbe | |
where | |
desc = Opt.progDesc "Create a stake address registration certificate" | |
make parser = subParser "registration-certificate" (Opt.info parser desc) |
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 went for something simpler in the new version, can you have another look?
d0f4c75
to
ba5b5e6
Compare
That's correct. It's a conway only flag. |
ba5b5e6
to
8abc774
Compare
8abc774
to
d2b4c76
Compare
$ Opt.info | ||
( StakeAddressRegistrationCertificateCmd (shelleyToBabbageEraToShelleyBasedEra shelleyToBabbage) | ||
<$> pStakeIdentifier | ||
<*> optional pKeyRegistDeposit |
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 pure Nothing
for Shelley to 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.
Hum yeah really smart, I hadn't thought of lifting over the value type rather than the Parser
type 👍
Done the change.
d2b4c76
to
ae880fc
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! Just squash the commits.
ae880fc
to
f4b67ed
Compare
Changelog
Context
Fixes #504
Checklist