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

governance: add hash command and remove text/file hash arguments of existing commands #442

Merged
merged 17 commits into from
Nov 15, 2023

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Nov 7, 2023

Changelog

- description: |
    Add command `governance hash (--file-binary|--file-text|--text)`.
    
    Remove flags: `--proposal-anchor-metadata-file`, `--proposal-anchor-metadata`,
    `--vote-anchor-metadata`, and `--vote-anchor-metadata-file`.
    
    To handle the removing of flags, call `cardano-cli conway governance hash` and pass the file or text to hash,
    and then pass the result of this command to the governance action you used to call directly
    (`create-constitution`, `update-committee`, `create-info`, `create-no-confidence`, `create-protocol-parameters-update`, `create-treasury-widthdrawal`, `vote create`).
  
    Rename `--proposal-anchor-url` to `--anchor-url`
    Rename `--proposal-anchor-metadata-hash` to `--anchor-data-hash`
    Rename `--vote-anchor-metadata-hash` to `--anchor-data-hash`
# 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...

Roadmap to finish this PR

  • Add new governance hash command
  • Adapt create-info
  • Adapt update-committee
  • Integrate action update-committee: add golden test #383 in this PR, to see how multi platform tests go.
  • Adapt create-constitution
  • Adapt create-no-confidence
  • Adapt create-protocol-parameters-update
  • Adapt create-treasury-withdrawal
  • Adapt conway governance vote create

Context

Follow-up issues (to make this PR smaller):

How to review this PR

  • Look at the diffs in help files
  • Look at the diffs in changed tests
  • You can look at the entire diff at once. I have the PR structured in multiple commits for my own tracking, but that doesn't really help review here.

Sorry, no smarter way to trust this change 😆

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@smelc smelc force-pushed the smelc/governance-rework-hash-arguments branch 7 times, most recently from cc0b1a7 to 9fe6ea3 Compare November 9, 2023 10:36
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.

I still disagree with this naming because it is ambiguous.

hashAnchorData :: forall c. Crypto c => AnchorData -> SafeHash c AnchorData
hashAnchorData = hashWithCrypto (Proxy @c)

data Anchor c = Anchor
  { anchorUrl :: !Url
  , anchorDataHash :: !(SafeHash c AnchorData)
  }
  deriving (Eq, Ord, Show, Generic)

This is not the hash of the Anchor. It is the hash of the Anchor's metadata. I think people will wrongly assume the URL is also hashed. We can reduce metadata to data or come up with something else.

@smelc smelc force-pushed the smelc/governance-rework-hash-arguments branch from 9fe6ea3 to 0cb397b Compare November 9, 2023 14:50
@smelc smelc force-pushed the smelc/governance-rework-hash-arguments branch 3 times, most recently from da9eda4 to 1ab7d09 Compare November 10, 2023 10:52
@smelc smelc requested a review from Jimbo4350 November 10, 2023 10:55
@smelc smelc marked this pull request as ready for review November 10, 2023 11:00
@smelc smelc force-pushed the smelc/governance-rework-hash-arguments branch from 91a2b10 to 77b7edb Compare November 13, 2023 19:47
@smelc smelc dismissed Jimbo4350’s stale review November 14, 2023 11:05

Feedback has been applied

@smelc smelc requested a review from carlhammann November 14, 2023 11:05
@smelc smelc added this pull request to the merge queue Nov 15, 2023
Merged via the queue into main with commit 0a7af1c Nov 15, 2023
19 checks passed
@smelc smelc deleted the smelc/governance-rework-hash-arguments branch November 15, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants