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

Add create-new-committee command to era based cli #175

Merged
merged 2 commits into from
Aug 17, 2023
Merged

Add create-new-committee command to era based cli #175

merged 2 commits into from
Aug 17, 2023

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Aug 16, 2023 •
edited
Loading

Changelog

Resolves #173

- description: |
    Add create-new-committee command to era based cli
# 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

Additional context for the PR goes here.

If the PR fixes a particular issue please provide a
link
to the issue.

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
  • The changelog section in the PR is updated to describe the change
  • Self-reviewed the diff

@Jimbo4350 Jimbo4350 requested review from a team as code owners August 16, 2023 16:20
@Jimbo4350 Jimbo4350 force-pushed the jordan/propose-new-committee branch from 5e8bfcc to 242d8c1 Compare August 16, 2023 18:27
CarlosLopezDeLara
CarlosLopezDeLara reviewed Aug 16, 2023
View reviewed changes
.../test/cardano-cli-golden/files/golden/help/conway_governance_action_create-new-committee.cli
@@ -0,0 +1,79 @@
Usage: cardano-cli conway governance action create-new-committee --governance-action-deposit NATURAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are there so many of these:

                                                                   [ --stake-pool-verification-key STRING
                                                                   | --cold-verification-key-file FILE
                                                                   | --stake-pool-id STAKE_POOL_ID
                                                                   | --stake-verification-key STRING
                                                                   | --stake-verification-key-file FILE
                                                                   | --stake-key-hash HASH
                                                                   ]

I think it needs to take something like

--new-committee-member–cold-key-hash STRING

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users can submit their stake key in different formats, via the hash, the key string or the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jimbo4350

I know, but it shows the same thing 3 times

                                                               ( --stake-pool-verification-key STRING
                                                               | --cold-verification-key-file FILE
                                                               | --stake-pool-id STAKE_POOL_ID
                                                               | --stake-verification-key STRING
                                                               | --stake-verification-key-file FILE
                                                               | --stake-key-hash HASH
                                                               )
                                                               [ --stake-pool-verification-key STRING
                                                               | --cold-verification-key-file FILE
                                                               | --stake-pool-id STAKE_POOL_ID
                                                               | --stake-verification-key STRING
                                                               | --stake-verification-key-file FILE
                                                               | --stake-key-hash HASH
                                                               ]
                                                               [
                                                                 ( --stake-pool-verification-key STRING
                                                                 | --cold-verification-key-file FILE
                                                                 | --stake-pool-id STAKE_POOL_ID
                                                                 | --stake-verification-key STRING
                                                                 | --stake-verification-key-file FILE
                                                                 | --stake-key-hash HASH
                                                                 )
                                                                 --epoch NATURAL]
                                                               --quorum RATIONAL
                                                               --tx-in TX-IN
                                                               --out-file FILE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not the same thing:

deposit return stake address, old committee stake keys, new committee stake keys with expiration epoch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do need to be updated to reflect this however as it's not obvious.

@Jimbo4350 Jimbo4350 force-pushed the jordan/propose-new-committee branch from 6fe5291 to a55cea2 Compare August 17, 2023 12:12
carbolymer
carbolymer reviewed Aug 17, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs
Comment on lines +61 to +64
featureInEra Nothing (\cOn -> Just $
subParser "create-new-committee"
$ Opt.info (pCmd cOn)
$ Opt.progDesc "Create a new committee proposal.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be rewritten into:

Suggested change
featureInEra Nothing (\cOn -> Just $
subParser "create-new-committee"
$ Opt.info (pCmd cOn)
$ Opt.progDesc "Create a new committee proposal.")
cOn <- maybeFeatureInera era
pure
$ subParser "create-new-committee"
$ Opt.info (pCmd cOn)
$ Opt.progDesc "Create a new committee proposal."

which may be a bit more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely agree. Can you make a follow up PR that implements this pattern?

@Jimbo4350 Jimbo4350 enabled auto-merge August 17, 2023 13:49
@Jimbo4350 Jimbo4350 disabled auto-merge August 17, 2023 13:49
@Jimbo4350 Jimbo4350 merged commit f604139 into main Aug 17, 2023
@Jimbo4350 Jimbo4350 deleted the jordan/propose-new-committee branch August 17, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlosLopezDeLara CarlosLopezDeLara CarlosLopezDeLara left review comments

@carbolymer carbolymer carbolymer approved these changes

@newhoggy newhoggy Awaiting requested review from newhoggy

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

Successfully merging this pull request may close these issues.

Implement new committee creation governance action
3 participants
@Jimbo4350 @carbolymer @CarlosLopezDeLara