Make `governance action create-protocol-parameters-update` Conway onwards only #366

newhoggy · 2023-10-11T13:26:48Z

Changelog

- description: |
    Make `governance action create-protocol-parameters-update` Conway onwards only
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

This also converts to use command argument types for governance actions commands.

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
- round trip tests
- integration tests
  See Running 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
Self-reviewed the diff

…up to babbage

newhoggy · 2023-10-11T13:28:34Z

cardano-cli/test/cardano-cli-golden/files/golden/help/latest_governance.cli

@@ -14,7 +13,6 @@ Available options:
 Available commands:
  create-mir-certificate   Create an MIR (Move Instantaneous Rewards)
                           certificate
-  action                   Governance action commands.


This change shows the benefit of allowing commands and command groups to decide for themselves if they appear in an era.

All I did was make governance action create-protocol-parameters-update Conway onwards only.

Then the action command figured out that it was empty for all the pre-Conway eras and decided to not appear in those eras.

cardano-cli/src/Cardano/CLI/EraBased/Run/Governance/Actions.hs

carlhammann · 2023-10-11T14:14:54Z

cardano-cli/src/Cardano/CLI/EraBased/Commands/Governance/Actions.hs

@@ -24,89 +27,88 @@ import           Data.Text (Text)
 import           Data.Word

 data GovernanceActionCmds era


I take it this is (part of) the new conventions you allude to here?

carlhammann · 2023-10-11T14:16:34Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs

@@ -10,7 +10,7 @@ import           Cardano.Api
 import           Cardano.Api.Ledger
 import           Cardano.Api.Shelley

-import           Cardano.CLI.EraBased.Commands.Governance.Actions
+import qualified Cardano.CLI.EraBased.Commands.Governance.Actions as Cmd


And this qualified import as well, part of the new convention?

The Cmd qualification allows us to avoid naming conflicts. For example in the records we have an eon field which if imported unqualified will name conflict with a lot of code.

smelc · 2023-10-11T15:04:58Z

cardano-cli/src/Cardano/CLI/EraBased/Commands/Governance/Actions.hs

-    , ucPreviousGovActionId :: Maybe (TxId, Word32)
-    , ucFilePath :: File () Out
-    } deriving Show
+      { networkId               :: !Ledger.Network


So I take that having multiple fields with the same name in a module is allowed by our coding style now?

I think this is okay for command argument types because the qualifications and record field matching allows us to avoid naming conflicts.

smelc

LGTM 🍾 Really nice that you have well defined commits, makes it possible to review iteratively.

newhoggy added 3 commits October 12, 2023 00:17

Consistent naming for command argument types

447382e

Add era type arguments to command argument types

ffc8afe

Remove governance action create-protocol-parameters-update from eras …

47240e0

…up to babbage

newhoggy commented Oct 11, 2023

View reviewed changes

newhoggy added 3 commits October 12, 2023 00:38

New GovernanceActionProtocolParametersUpdateCmdArgs type

900d75f

Standard command argument type field names

7cd67d9

Move eons into command argument types

c27087a

newhoggy marked this pull request as ready for review October 11, 2023 14:06

newhoggy requested review from Jimbo4350, carbolymer, kevinhammond and disassembler as code owners October 11, 2023 14:06

newhoggy changed the title ~~Command argument types for governance actions commands~~ Make governance action create-protocol-parameters-update Conway onwards only Oct 11, 2023

newhoggy changed the title ~~Make governance action create-protocol-parameters-update Conway onwards only~~ Make governance action create-protocol-parameters-update Conway onwards only Oct 11, 2023

Jimbo4350 approved these changes Oct 11, 2023

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Run/Governance/Actions.hs Outdated Show resolved Hide resolved

carlhammann approved these changes Oct 11, 2023

View reviewed changes

smelc reviewed Oct 11, 2023

View reviewed changes

smelc approved these changes Oct 11, 2023

View reviewed changes

Record field matching

42326f9

newhoggy force-pushed the newhoggy/command-arg-types-for-governance-actions-commands branch from d6b3bdf to 42326f9 Compare October 12, 2023 00:35

newhoggy added this pull request to the merge queue Oct 12, 2023

Merged via the queue into main with commit 935d5e8 Oct 12, 2023
19 checks passed

newhoggy deleted the newhoggy/command-arg-types-for-governance-actions-commands branch October 12, 2023 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `governance action create-protocol-parameters-update` Conway onwards only #366

Make `governance action create-protocol-parameters-update` Conway onwards only #366

newhoggy commented Oct 11, 2023 •

edited

newhoggy Oct 11, 2023

carlhammann Oct 11, 2023

newhoggy Oct 12, 2023

carlhammann Oct 11, 2023

newhoggy Oct 12, 2023

smelc Oct 11, 2023 •

edited

newhoggy Oct 12, 2023

smelc left a comment

		@@ -24,89 +27,88 @@ import Data.Text (Text)
		import Data.Word

		data GovernanceActionCmds era

Make governance action create-protocol-parameters-update Conway onwards only #366

Make governance action create-protocol-parameters-update Conway onwards only #366

Conversation

newhoggy commented Oct 11, 2023 • edited

Changelog

Context

Checklist

newhoggy Oct 11, 2023

Choose a reason for hiding this comment

carlhammann Oct 11, 2023

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

carlhammann Oct 11, 2023

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

smelc Oct 11, 2023 • edited

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

smelc left a comment

Choose a reason for hiding this comment

Make `governance action create-protocol-parameters-update` Conway onwards only #366

Make `governance action create-protocol-parameters-update` Conway onwards only #366

newhoggy commented Oct 11, 2023 •

edited

smelc Oct 11, 2023 •

edited