Share code in parsers of committee commands #816

smelc · 2024-07-03T12:17:19Z

Changelog

- description: |
    Share code in parsers of committee commands. This doesn't share the CLI's behavior.
# 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
  - refactoring    # QoL changes
  # - 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

While doing PRs recently in this part of the codebase, I realized that some places had code duplicated, without a good reason. Sharing will also fixes to affect multiple commands at once in the future, and also helps keeping different commands consistent.
Fixes Use pHexHash more often #811

More sharing is possible (e.g. hot VS cold), but I don't want to stack too much in one PR.

How to trust this PR

Golden files are not changed
The tight types means that if the code still compiles, and the types didn't change; then probably the runtime behavior didn't change either.
Each commit is standalone and makes sense on his own, to ease review. So it's advised to review commit per commit.
It's twice more deletions than additions

Checklist

Commit sequence broadly makes sense and commits have useful messages
Self-reviewed the diff

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

      . deserialiseFromRawBytesHex (AsHash a)
      . BSC.pack
+  where
+    errPrefix = maybe "" ((<>) ": ") mErrPrefix


palas

Nice refactoring. Just some ideas to bring it even further

palas · 2024-07-04T01:03:03Z

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

-    ]
+deserialiseHotCCKeyHashFromHex :: ReadM (Hash CommitteeHotKey)
+deserialiseHotCCKeyHashFromHex =
+  pHexHash AsCommitteeHotKey (Just "Invalid Consitutional Committee hot key hash")


Suggested change

pHexHash AsCommitteeHotKey (Just "Invalid Consitutional Committee hot key hash")

pHexHash AsCommitteeHotKey (Just "Invalid Constituional Committee hot key hash")

Consitutional -> Constitutional

The typo was already there before the PR, but it would be good to do a search and replace since you are at it

palas · 2024-07-04T01:18:27Z

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

-  :: SerialiseAsRawBytes (Hash a) => AsType a -> ReadM (Hash a)
-pHexHash a =
+  :: SerialiseAsRawBytes (Hash a) => AsType a -> Maybe String -> ReadM (Hash a)
+pHexHash a mErrPrefix =


Nice, you could also use this function in:
pAddCommitteeColdVerificationKeyHash, pGenesisVerificationKeyHash, pGenesisDelegateVerificationKeyHash, pVrfVerificationKeyHash, and pStakePoolMetadataHash, I think.

palas · 2024-07-04T01:22:25Z

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

-        . BSC.pack
+
+deserialiseColdCCKeyFromHex :: String -> Either String (VerificationKey CommitteeColdKey)
+deserialiseColdCCKeyFromHex =


Would also be nice to have also something like pHexHash for verification keys (or even generalize it further, not sure if it is possble), and that could also be used in several places in this file: pGenesisDelegateVerificationKey, pAddCommitteeColdVerificationKey, deserialiseColdCCKeyFromHex, deserialiseHotCCKeyFromHex, and pGenesisVerificationKey.

smelc · 2024-07-04T09:29:31Z

Thanks @palas, I'll look at your suggestions in another PR, I don't like big PRs 😉

carbolymer · 2024-07-04T09:53:48Z

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

 pHexHash
-  :: SerialiseAsRawBytes (Hash a) => AsType a -> ReadM (Hash a)
-pHexHash a =
+  :: SerialiseAsRawBytes (Hash a) => AsType a -> Maybe String -> ReadM (Hash a)


can you add a haddock for Maybe String. It takes a moment to understand its purpose.

It was a top-level haddock, changed to be attached to the parameter 👍

carbolymer · 2024-07-04T10:28:05Z

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

@@ -8,6 +8,7 @@
 {- HLINT ignore "Move brackets to avoid $" -}
 {- HLINT ignore "Use <$>" -}
 {- HLINT ignore "Move brackets to avoid $" -}
+{- HLINT ignore "Use section" -}


what's wrong with sections?

It yields this suggestion:

cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs:538:26-36: Suggestion: Use section Found: ((<>) ": ") Perhaps: (": " <>)

which I personally don't find a really good change. But no strong opinion. I can leave it back if you prefer.

They're fine imo. My problem is with per-module level hlint rules. I think we should agree together on a set of them, put them into the top level config and enforce those rules everywhere.

smelc requested review from Jimbo4350, newhoggy, carbolymer, palas, kevinhammond and disassembler as code owners July 3, 2024 12:17

github-advanced-security bot found potential problems Jul 3, 2024

View reviewed changes

smelc force-pushed the smelc/share-code-in-comittee-parsers branch from 36bb1a8 to e8e94d3 Compare July 3, 2024 12:26

palas approved these changes Jul 4, 2024

View reviewed changes

smelc added 4 commits July 4, 2024 11:29

Share code 1

82e6423

Share code 2

d722a0d

Share code 3

a6be275

Share code 4

e83e3bf

smelc force-pushed the smelc/share-code-in-comittee-parsers branch from e8e94d3 to a11c529 Compare July 4, 2024 09:29

smelc enabled auto-merge July 4, 2024 09:30

smelc mentioned this pull request Jul 4, 2024

Fix typo in help/error messages #820

Merged

2 tasks

carbolymer approved these changes Jul 4, 2024

View reviewed changes

smelc added this pull request to the merge queue Jul 4, 2024

smelc removed this pull request from the merge queue due to a manual request Jul 4, 2024

smelc added 2 commits July 4, 2024 13:54

Generalize pHexHash, preparing for reuse

54f027f

Reduce duplication by using pHexHash more

daddf0d

smelc force-pushed the smelc/share-code-in-comittee-parsers branch from a11c529 to daddf0d Compare July 4, 2024 11:55

smelc enabled auto-merge July 4, 2024 11:56

smelc disabled auto-merge July 4, 2024 12:34

smelc merged commit a1dc1a7 into main Jul 4, 2024
14 of 15 checks passed

smelc deleted the smelc/share-code-in-comittee-parsers branch July 4, 2024 12:34

smelc mentioned this pull request Jul 4, 2024

Share code in parsers (continued) #823

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share code in parsers of committee commands #816

Share code in parsers of committee commands #816

smelc commented Jul 3, 2024

palas left a comment

palas Jul 4, 2024

palas Jul 4, 2024

palas Jul 4, 2024

smelc commented Jul 4, 2024

carbolymer Jul 4, 2024

smelc Jul 4, 2024

carbolymer Jul 4, 2024

smelc Jul 4, 2024

carbolymer Jul 4, 2024

	pHexHash AsCommitteeHotKey (Just "Invalid Consitutional Committee hot key hash")
	pHexHash AsCommitteeHotKey (Just "Invalid Constituional Committee hot key hash")

Share code in parsers of committee commands #816

Share code in parsers of committee commands #816

Conversation

smelc commented Jul 3, 2024

Changelog

Context

How to trust this PR

Checklist

palas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smelc commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment