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.

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

Move "conway governance hash" commands to "hash" #787

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Jun 13, 2024

Changelog

- description: |
    Move "conway governance hash" commands to "hash". Users should adapt their calls as follows:
    
    `cardano-cli conway governance hash anchor-data ...` becomes `cardano-cli hash anchor-data ...`
    `cardano-cli conway governance hash script ...` becomes `cardano-cli hash script ...`
# 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

Fixes #782

@smelc smelc force-pushed the smelc/remove-governance-in-conway-governance-hash branch from de8b437 to 3abbac0 Compare June 13, 2024 09:45
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I'm wondering if it would be a good idea to move other hashing commands into this subcommand (e.g. key hash).

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

So the hint that hashing is not era based is in the type signature of hashAnchorData:

hashAnchorData :: forall c. Crypto c => AnchorData -> SafeHash c AnchorData

Same for hashKey etc...

And then if you take a look at ledger there is only one instance of Crypto:


instance Crypto StandardCrypto where
  type DSIGN StandardCrypto = Ed25519DSIGN
  type KES StandardCrypto = Sum6KES Ed25519DSIGN Blake2b_256
  type VRF StandardCrypto = PraosVRF
  type HASH StandardCrypto = Blake2b_256
  type ADDRHASH StandardCrypto = Blake2b_224

So what we really want is a top level command that can hash all the hashable things. Or at least not give the impression that hashing depends on the era.

@smelc
Copy link
Contributor Author

smelc commented Jun 13, 2024

So what we really want is a top level command that can hash all the hashable things. Or at least not to give the impression that hashing depends on the era.

cc @CarlosLopezDeLara since the current design is your ask from #782

I can make hash a top-level command if you approve that like @Jimbo4350 does

@CarlosLopezDeLara
Copy link
Contributor

@smelc I like the idea! sounds great.

@smelc smelc force-pushed the smelc/remove-governance-in-conway-governance-hash branch from 3abbac0 to 2d0a701 Compare June 28, 2024 13:26
@smelc smelc force-pushed the smelc/remove-governance-in-conway-governance-hash branch 2 times, most recently from e8972dc to 601808c Compare June 28, 2024 13:35
@smelc
Copy link
Contributor Author

smelc commented Jun 28, 2024

@Jimbo4350> Made the command top-level and removed the era.

I will consider in a follow-up PR if other hashing commands can be moved there too (hashing keys).

@smelc smelc changed the title Move "conway governance hash" commands to "conway hash" Move "conway governance hash" commands to "hash" Jun 28, 2024
@smelc smelc force-pushed the smelc/remove-governance-in-conway-governance-hash branch from 085cd2e to 72e643f Compare July 1, 2024 08:10
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! However I would be wary of breaking the existing hashing functionality. Talk to @CarlosLopezDeLara about this as it may be an annoying for users who have existing scripts etc.

{-# LANGUAGE LambdaCase #-}

module Cardano.CLI.Commands.Hash
(
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@@ -48,8 +47,6 @@ data GovernanceCmds era
(GovernanceCommitteeCmds era)
| GovernanceDRepCmds
(GovernanceDRepCmds era)
| GovernanceHashCmds
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I would break this immediately. Better to deprecate it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to go through a deprecation period, as per @CarlosLopezDeLara's comment below 👇

{-# LANGUAGE GADTs #-}

module Cardano.CLI.Options.Hash
(
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

cardano-cli/src/Cardano/CLI/Commands/Hash.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Commands/Hash.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Run/Hash.hs Outdated Show resolved Hide resolved
@CarlosLopezDeLara
Copy link
Contributor

LGTM! However I would be wary of breaking the existing hashing functionality. Talk to @CarlosLopezDeLara about this as it may be an annoying for users who have existing scripts etc.

I think we can afford the breaking change here since 9.0 is still in the makings and 8.12 will not cross the hardfork. I'll update docs after this is on a cardano-cli release.

@smelc smelc force-pushed the smelc/remove-governance-in-conway-governance-hash branch 2 times, most recently from b13fb62 to 68048ca Compare July 1, 2024 15:50
@smelc smelc force-pushed the smelc/remove-governance-in-conway-governance-hash branch from 68048ca to fe71860 Compare July 2, 2024 11:58
@smelc smelc enabled auto-merge July 2, 2024 11:58
@smelc smelc added this pull request to the merge queue Jul 2, 2024
Merged via the queue into main with commit 2006e3b Jul 2, 2024
21 checks passed
@smelc smelc deleted the smelc/remove-governance-in-conway-governance-hash branch July 2, 2024 12:16
@gitmachtl
Copy link
Contributor

gitmachtl commented Jul 5, 2024

@smelc there are a lot of leftovers in the help texts like:

...
  --anchor-data-hash HASH  Proposal anchor data hash (obtain it with
                           "cardano-cli conway governance hash anchor-data ...")
  --constitution-url TEXT  Constitution URL.
  --constitution-hash HASH Hash of the constitution data (obtain it with
                           "cardano-cli conway governance hash anchor-data
                           ...").
...

pointing to the old location of the hash function conway governance hash

@smelc
Copy link
Contributor Author

smelc commented Jul 5, 2024

@gitmachtl> yes, it's being fixed by #821. I think that's because I I lost some of my changes while polishing the PR ☹️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move conway governance hash to conway hash
5 participants