Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Share MIR certificates code between era-based and legacy CLI parsers #107

Merged
merged 3 commits into from
Jul 26, 2023
Merged

Share MIR certificates code between era-based and legacy CLI parsers #107

merged 3 commits into from
Jul 26, 2023

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Jul 25, 2023 •
edited
Loading

Changelog

- description: |
    Share MIR certificates code between era-based and legacy CLI parsers
  compatibility: breaking
  type: feature

Context

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • The change log section in the PR description has been filled in
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • The changelog section in the PR is updated to describe the change
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@newhoggy newhoggy marked this pull request as ready for review July 25, 2023 14:30
@newhoggy newhoggy force-pushed the newhoggy/shared-mir-certificates-command-code branch 4 times, most recently from 23ec84e to 9ba52a8 Compare July 25, 2023 14:54
@newhoggy newhoggy changed the title Shared mir certificates command code Share MIR certificates code between era-based and legacy CLI parsers Jul 25, 2023
newhoggy
newhoggy commented Jul 25, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs
]

pAnyShelleyToBabbageEra :: EnvCli -> Parser AnyShelleyToBabbageEra
pAnyShelleyToBabbageEra envCli =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to accept shelley-to-babbage era flags in the legacy parser.

newhoggy
newhoggy commented Jul 25, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Options/Governance.hs
@@ -20,6 +25,17 @@ import qualified Options.Applicative as Opt
data EraBasedGovernanceCmd era
= EraBasedGovernancePreConwayCmd (ShelleyToBabbageEra era)
| EraBasedGovernancePostConwayCmd (ConwayEraOnwards era)
| EraBasedGovernanceMIRPayStakeAddressesCertificate
(ShelleyToBabbageEra era)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the ShelleyToBabbageEra witness in the era-based parsers.

newhoggy
newhoggy commented Jul 25, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Governance.hs
data GovernanceCmd
= GovernanceVoteCmd VoteCmd
| GovernanceActionCmd ActionCmd
| GovernanceMIRPayStakeAddressesCertificate
AnyShelleyBasedEra
AnyShelleyToBabbageEra
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use AnyShelleyToBabbageEra in the legacy parsers.

newhoggy
newhoggy commented Jul 25, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs
[ Opt.long "transfer"
, Opt.metavar "LOVELACE"
, Opt.help "The amount to transfer."
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These parsers are shared.

newhoggy
newhoggy commented Jul 25, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Certificate.hs Outdated

right $ Ledger.DelegStake $ obtainIsShelleyBasedEraConwayOnwards cOnwards kHash
TargetVotingDrep _ -> error "TODO: Conway era - Ledger.DelegVote"
TargetVotingDrepAndStakePool _ -> error "TODO: Conway era - Ledger.DelegStakeVote"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code has been moved into the era-based module structure.

newhoggy
newhoggy commented Jul 25, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/Run/Legacy/Governance.hs
ShelleyBasedEraBabbage -> do
return $ MirCertificateRequirements ShelleyToBabbageEraBabbage mirPot mirTarget
ShelleyBasedEraConway ->
Left ShelleyGovernanceCmdMIRCertNotSupportedInConway
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is gone because there is not longer a runtime error due to --conway-era flag being used. This flag does not exist for commands that don't work for Conway era.

Jimbo4350
Jimbo4350 approved these changes Jul 25, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Certificate.hs Outdated
@@ -4,7 +4,7 @@
{-# LANGUAGE TypeApplications #-}

module Cardano.CLI.EraBased.Certificate
( runAnyDelegationTarget
( runGovernanceDelegrationCertificate
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling: runGovernanceDelegationCertificate

cardano-cli/src/Cardano/CLI/Commands/EraBased.hs
@@ -21,7 +21,7 @@ import Options.Applicative (Parser)
import qualified Options.Applicative as Opt

data AnyEraCommand where
AnyEraCommandOf :: CardanoEra era -> EraBasedCommand era -> AnyEraCommand
AnyEraCommandOf :: ShelleyBasedEra era -> EraBasedCommand era -> AnyEraCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

We should rename to AnyShelleyBasedEraCommandOf or something like that

@Jimbo4350 Jimbo4350 force-pushed the newhoggy/shared-mir-certificates-command-code branch from 9ba52a8 to 7275232 Compare July 25, 2023 18:13
@Jimbo4350 Jimbo4350 requested review from a team as code owners July 25, 2023 18:13
@Jimbo4350 Jimbo4350 force-pushed the newhoggy/shared-mir-certificates-command-code branch from b47d513 to ca04fd5 Compare July 26, 2023 15:14
@Jimbo4350 Jimbo4350 force-pushed the newhoggy/shared-mir-certificates-command-code branch 3 times, most recently from ec6e257 to 07a2203 Compare July 26, 2023 17:05
@Jimbo4350 Jimbo4350 enabled auto-merge July 26, 2023 17:05
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Jul 26, 2023
Merged via the queue into main with commit 4c2961f Jul 26, 2023
@Jimbo4350 Jimbo4350 deleted the newhoggy/shared-mir-certificates-command-code branch July 26, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer approved these changes

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newhoggy @carbolymer @Jimbo4350