Disambiguate 2-n flags in governance new-committee action #302

smelc · 2023-09-21T13:58:17Z

Changelog

- description: |
    A number of flags appeared in the parameters of new-committee multiple times, with the same long flag, making the parser break. This PR disambiguates the repeated occurences.
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way <- Clément: I believe right?
  # - 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

Contributes to fixing #300

How to trust this PR

This PR is mostly boilerplate, but there is one thing to make sure of:

When a prefix argument is introduced in a parser, it is used in all branches of the parser. For example in this snippet:

asum [ AnyStakePoolKey <$> pStakePoolVerificationKeyOrHashOrFile prefix
       , AnyStakeKey <$> pStakeVerificationKeyOrHashOrFile prefix
       ]

It is important to pass prefix to every branch of the asum. Otherwise we could create inconsistent parsers. I have made sure this was correct.

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
NA 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

smelc · 2023-09-21T14:02:52Z

On the current state of this PR, the create-new-committee API is now as follows (cc @CarlosLopezDeLara):

Usage: cardano-cli conway governance action create-new-committee 
                                                                   ( --mainnet
                                                                   | --testnet
                                                                   )
                                                                   --governance-action-deposit NATURAL
                                                                   ( --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
                                                                   )
                                                                   --proposal-url TEXT
                                                                   ( --proposal-text TEXT
                                                                   | --proposal-file FILE
                                                                   | --proposal-hash HASH
                                                                   )
                                                                   [ --remove-cc-stake-pool-verification-key STRING
                                                                   | --remove-cc-cold-verification-key-file FILE
                                                                   | --remove-cc-stake-pool-id STAKE_POOL_ID
                                                                   | --remove-cc-stake-verification-key STRING
                                                                   | --remove-cc-stake-verification-key-file FILE
                                                                   | --remove-cc-stake-key-hash HASH
                                                                   ]
                                                                   [
                                                                     ( --add-cc-stake-pool-verification-key STRING
                                                                     | --add-cc-cold-verification-key-file FILE
                                                                     | --add-cc-stake-pool-id STAKE_POOL_ID
                                                                     | --add-cc-stake-verification-key STRING
                                                                     | --add-cc-stake-verification-key-file FILE
                                                                     | --add-cc-stake-key-hash HASH
                                                                     )
                                                                     --epoch NATURAL]
                                                                   --quorum RATIONAL
                                                                   [--governance-action-tx-id TXID
                                                                     --governance-action-index WORD32]
                                                                   --out-file FILE

smelc · 2023-09-21T14:09:38Z

@CarlosLopezDeLara> if I retry your earlier example from Slack, it now fails differently:

> cabal run cardano-cli -- conway governance action create-new-committee --testnet --governance-action-deposit 0 --stake-verification-key-file utxo-keys/stake1.vkey --proposal-url https://tinyurl.com/3wrwb2as --proposal-file proposal.txt --remove-cc-cold-verification-key-file cc/cc-cold.vkey --add-cc-cold-verification-key-file cc/cc-cold1.vkey --epoch 5 --quorum 
1/100 --out-file new-cc.action

cardano-cli: proposal.txt: withBinaryFile: does not exist (No such file or directory)

Whereas before this PR, I was reproducing your parsing failure. So I think we're good.

smelc · 2023-09-21T14:38:56Z

@CarlosLopezDeLara> while doing this PR, I realized we likely have the same kind of problem for create-treasury-withdrawal. This is visible here in this PR: https://github.com/input-output-hk/cardano-cli/blob/smelc/fix-new-committee-parser/cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs#L328 In this position, we should likely pass a prefix, because the pAnyStakeIdentifier parser is used twice, and so is likely ambiguous.

carbolymer · 2023-09-21T14:52:48Z

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

    <*> pProposalUrl
    <*> pProposalHashSource
-    <*> many pAnyStakeIdentifier
-    <*> many ((,) <$> pAnyStakeIdentifier <*> pEpochNo "Committee member expiry epoch")
+    <*> many (pAnyStakeIdentifier (Just "remove-cc"))


I think we could use IsString here:

instance IsString a => IsString (Maybe a) where fromString = Just . fromString

this way we could just write "remove-cc" instead without Just

I'm not sure, generally speaking, this instance looks surprising to me (in other words: I'd be surprised to have in my context 🙂).

Also the occurrences of Just "prefix" are actually quite rare, most of this PR is just passing the prefix in context around.

cardano-cli/src/Cardano/CLI/EraBased/Options/StakePool.hs

carbolymer · 2023-09-21T14:58:10Z

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

              <*> pProposalUrl
              <*> pProposalHashSource
-              <*> many ((,) <$> pAnyStakeIdentifier <*> pTransferAmt)
+              <*> many ((,) <$> pAnyStakeIdentifier Nothing <*> pTransferAmt) -- TODO we should likely pass a prefix here, becaus pAnyStakeIdentiefier is used twice


I agree, I think this should be included in this pr.

I would do a separate PR, because it's a different command. I can do this other PR soon after this one is merged.

carbolymer

LGTM, but get an approval from @CarlosLopezDeLara and @Jimbo4350 or @newhoggy too

CarlosLopezDeLara · 2023-09-21T16:45:41Z

I can confirm that this resolved the parsing issue.

CarlosLopezDeLara · 2023-09-21T16:50:09Z

@CarlosLopezDeLara> while doing this PR, I realized we likely have the same kind of problem for create-treasury-withdrawal. This is visible here in this PR: https://github.com/input-output-hk/cardano-cli/blob/smelc/fix-new-committee-parser/cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs#L328 In this position, we should likely pass a prefix, because the pAnyStakeIdentifier parser is used twice, and so is likely ambiguous.

Interestingly enough, it does not complain about this:

$CARDANO_CLI conway governance action create-treasury-withdrawal \
--testnet \
--governance-action-deposit 0 \
--stake-verification-key-file "${UTXO_DIR}/stake1.vkey" \
--proposal-url https://tinyurl.com/3wrwb2as \
--proposal-file  "${ROOT}/govActionJustification.txt" \
--stake-verification-key-file "${UTXO_DIR}/stake1.vkey" \
--transfer "${WITHDRAW_AMOUNT}" \
--out-file "${UTXO_DIR}/treasury.action"

smelc · 2023-09-21T18:24:58Z

Interestingly enough, it does not complain about this

Weird 😵‍💫

smelc force-pushed the smelc/fix-new-committee-parser branch from 52adb31 to c457776 Compare September 21, 2023 14:36

smelc marked this pull request as ready for review September 21, 2023 14:41

smelc requested review from Jimbo4350, newhoggy, carbolymer, kevinhammond and disassembler as code owners September 21, 2023 14:41

smelc force-pushed the smelc/fix-new-committee-parser branch from c457776 to 46fb3fa Compare September 21, 2023 14:47

carbolymer reviewed Sep 21, 2023

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Options/StakePool.hs Outdated Show resolved Hide resolved

carbolymer reviewed Sep 21, 2023

View reviewed changes

carbolymer approved these changes Sep 21, 2023

View reviewed changes

governance new-committee action: disambiguate parsing

783fb14

smelc force-pushed the smelc/fix-new-committee-parser branch from 46fb3fa to 783fb14 Compare September 21, 2023 16:12

CarlosLopezDeLara added this pull request to the merge queue Sep 21, 2023

Merged via the queue into main with commit 093655e Sep 21, 2023
19 checks passed

CarlosLopezDeLara deleted the smelc/fix-new-committee-parser branch September 21, 2023 17:04

CarlosLopezDeLara mentioned this pull request Sep 27, 2023

governance action create-new-committee NOT WORKING #300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disambiguate 2-n flags in governance new-committee action #302

Disambiguate 2-n flags in governance new-committee action #302

smelc commented Sep 21, 2023 •

edited

Loading

smelc commented Sep 21, 2023 •

edited

Loading

smelc commented Sep 21, 2023

smelc commented Sep 21, 2023

carbolymer Sep 21, 2023 •

edited

Loading

smelc Sep 21, 2023 •

edited

Loading

carbolymer Sep 21, 2023

smelc Sep 21, 2023 •

edited

Loading

carbolymer left a comment

CarlosLopezDeLara commented Sep 21, 2023

CarlosLopezDeLara commented Sep 21, 2023

smelc commented Sep 21, 2023

Disambiguate 2-n flags in governance new-committee action #302

Disambiguate 2-n flags in governance new-committee action #302

Conversation

smelc commented Sep 21, 2023 • edited Loading

Changelog

Context

How to trust this PR

Checklist

smelc commented Sep 21, 2023 • edited Loading

smelc commented Sep 21, 2023

smelc commented Sep 21, 2023

carbolymer Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

smelc Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

carbolymer Sep 21, 2023

Choose a reason for hiding this comment

smelc Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

carbolymer left a comment

Choose a reason for hiding this comment

CarlosLopezDeLara commented Sep 21, 2023

CarlosLopezDeLara commented Sep 21, 2023

smelc commented Sep 21, 2023

smelc commented Sep 21, 2023 •

edited

Loading

smelc commented Sep 21, 2023 •

edited

Loading

carbolymer Sep 21, 2023 •

edited

Loading

smelc Sep 21, 2023 •

edited

Loading

smelc Sep 21, 2023 •

edited

Loading