Bump CHaP to cardano-ledger-conway-1.7.0.0 #179

carbolymer · 2023-08-09T13:58:56Z

Changelog

- description: |
    Updating the ledger dependency to cardano-ledger-conway-1.7.0.0:
      Many superficial renamings
      TxVotes carries a map now
      ResolvablePointers now has a different representation than does the ledger
      ProposalNewCommitee requires the old committee's credentials
      The ProposalNewConstitution case of toGovernanceAction was hashing the argument'ByteString, but it was already a hash.
      See temporarilyOptOutOfPrevGovAction
      makeGovernanceActionId was reusing the transaction id as the governance action id, but the types no longer allow that.
      Semigroup oprhan was missing for ConwayPParams
      QueryConstitutionHash phantom type is now more specific
      Cardano.Ledger.Api no longer export EraCrypto
      Introduced (internal) pattern synonyms for scripts to coverup a change in the corresponding ledger types.

# 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

Updating the ledger dependency to the work-in-progress cardano-ledger-conway-1.7.0.0 (via a source-repository-package for now) required the following.

Many superficial renamings
TxVotes carries a map now
ResolvablePointers now has a different representation than does the ledger
ProposalNewCommitee requires the old committee's credentials
The ProposalNewConstitution case of toGovernanceAction was hashing the argument'ByteString, but it was already a hash.
See temporarilyOptOutOfPrevGovAction
Many more TODOs throughout the code due to new partiality/missing arguments/etc
makeGovernanceActionId was reusing the transaction id as the governance action id, but the types no longer allow that.
Semigroup oprhan was missing for ConwayPParams
QueryConstitutionHash phantom type is now more specific
Cardano.Ledger.Api no longer export EraCrypto
Introduced (internal) pattern synonyms for scripts to coverup a change in the corresponding ledger types.

Checklist

cardano-api/gen/Test/Gen/Cardano/Api/Typed.hs

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

carbolymer · 2023-08-09T14:04:12Z

cardano-api/internal/Cardano/Api/Certificate.hs

+    $ Ledger.ConwayRegDRep
+        vcred
+        (toShelleyLovelace deposit)
+        (error "TODO" `asTypeOf` Ledger.SNothing)


Anchor can be added later to DRepRegistrationRequirements constructor.

I added an argument of type (Ledger.Url, ByteString), the same precursor of the to-be-defined cardano-api:Anchor data type that I've used elsewhere.

cardano-api/internal/Cardano/Api/TxBody.hs

carbolymer · 2023-08-09T14:51:32Z

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

@@ -79,18 +85,14 @@ data AnyGovernanceAction = forall era. AnyGovernanceAction (Gov.GovernanceAction
 -- TODO: Conway - fill in remaining actions
 data GovernanceAction
  = MotionOfNoConfidence
-  | ProposeNewConstitution ByteString
-  | ProposeNewCommittee [Hash StakeKey] Rational -- NB: This also includes stake pool keys
+  | ProposeNewConstitution ByteString   -- NB: This is the bytes of the hash, not the bytes that were hashed


from @nfrisby :

I anticipate that this argument should become an Anchor soon enough (eg this PR IntersectMBO/cardano-ledger#3625).

nfrisby · 2023-08-09T16:50:22Z

I don't ~~care about~~ have a preference regarding the code style/lint errors. My instinct is to dismiss them.

However, it's your repo! :D Should I default to accepting all of the suggestions? (.,. Even the one about shelleyBasedEraConstraints sbe? Its suggestion looks somewhat offensive to my eye.)

cardano-api/internal/Cardano/Api/Certificate.hs

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

cardano-api/internal/Cardano/Api/Governance/Actions/VotingProcedure.hs

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

nfrisby · 2023-08-14T16:00:07Z

I just force pushed a commit.

(Major) It rebases onto 3c2c27069 - (origin/main, origin/HEAD) Merge pull request #189 from input-output-hk/mgalazyn/release (5 hours ago) <Mateusz Gałażyn>.
(Minor) It updates the source-repository-package stanza for Consensus to a commit in which all the tests pass.

nfrisby · 2023-08-14T17:52:34Z

I just pushed another; it backs out the changes that I thought were required, but eg didn't understand the planned work's scope well enough to realize were premature.

In their place, I've left many calls to error or SNothing adorned with TODO comments so that a Node Team member will consider how to improve them.

cardano-api/internal/Cardano/Api/Fees.hs

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

Jimbo4350 · 2023-08-14T18:47:57Z

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

-      , Gov.pProcAnchor = SNothing -- TODO: Conway
+      , Gov.pProcReturnAddr = L.mkRwdAcnt (error "TODO which network?") (L.KeyHashObj retAddrh)
+      , Gov.pProcGovAction = toGovernanceAction sbe govAct
+      , Gov.pProcAnchor = error "TODO which anchor?"


SNothing would be better here with a -- TODO: Conway era. We want to still be able to submit Proposals in the cli.

It is no longer optional upstream: https://github.com/input-output-hk/cardano-ledger/blob/master/eras/conway/impl/src/Cardano/Ledger/Conway/Governance/Procedures.hs#L288

I used SNothing elsewhere, wherever it was permitted, along the lines of your suggestion here. But it's not permitted here any longer :/

Jimbo4350 · 2023-08-14T18:56:22Z

Looks good to me. Minor fixes:

newhoggy · 2023-08-15T07:19:58Z

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

+    { Gov.constitutionAnchor = Gov.Anchor
+      { Gov.anchorUrl = case textToUrl "TODO constitution anchorUrl" of
+          Nothing -> error "impossible! How could 27 ASCII chars be more than 64 bytes?"
+          Just url -> url


Is this TODO to add CLI parsing for the anchor url?

Throughout this ~~code~~ PR, "TODO" means "The most recent ledger changes will ultimately require additional data here, which the arguments of the local function do not yet provide. Frisby doesn't know the right way to handle that here right now, so here's a stub deserving attention of the Node Team."

newhoggy · 2023-08-15T07:20:56Z

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

+      { Gov.anchorUrl = case textToUrl "TODO constitution anchorUrl" of
+          Nothing -> error "impossible! How could 27 ASCII chars be more than 64 bytes?"
+          Just url -> url
+      , Gov.anchorDataHash = unsafeBytesToSafeHash bs   -- TODO "safe*" alternative?


What might the safe alternative involve?

My guess:

introduce a type for the hash of the constitution data (or perhaps the hash of any anchor data)

use that in the argument to ProposeNewConstitution instead of a bare ByteString

somewhere between the command-line and this logic needs to properly handle the case where the bytes given by the user cannot be interpreted as such a hash

Would it be unreasonable to ask ledger to provide this function?

@lehins As far as I know, unsafeMakeSafeHash :: Hash.Hash (HASH c) index -> SafeHash c index after Crypto.hashFromBytes :: ByteString -> Maybe (Hash a) is the only way to construct a SafeHash from the bytes of the hash. Is that right? And unsafeMakeSafeHash has a very strongly worded Haddock comment saying it shouldn't be used as it's currently being used in this PR.

This makes me wonder if the anchorDataHash really needs to be a SafeHash. The ledger rules themselves never deal with the actual constitution data, so I don't know that the Safe* part of SafeHash is actually useful here (or more generally to any Anchor for that matter).

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs

the relevant functions

QueryConstitution now returns the Constitution

…ProtocolParams StandardCrypto Implement Semigroup (Ledger.ConwayPParams StrictMaybe era) Update ScriptErrorMissingScript

…e-action Add goverance info action creation to era based cli

carbolymer requested a review from nfrisby August 9, 2023 13:59

github-advanced-security bot found potential problems Aug 9, 2023

View reviewed changes

cardano-api/gen/Test/Gen/Cardano/Api/Typed.hs Fixed Show fixed Hide fixed

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs Fixed Show fixed Hide fixed

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs Fixed Show fixed Hide fixed

carbolymer commented Aug 9, 2023

View reviewed changes

nfrisby changed the title ~~Nfrisby/preparing 8.3.0 pre~~ DO NOT MERGE preparing 8.3.0 pre Aug 9, 2023

Jimbo4350 reviewed Aug 9, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Certificate.hs Outdated Show resolved Hide resolved

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs Outdated Show resolved Hide resolved

nfrisby force-pushed the nfrisby/preparing-8.3.0-pre branch from ce768a4 to 654bacb Compare August 11, 2023 15:36

newhoggy reviewed Aug 13, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Governance/Actions/VotingProcedure.hs Outdated Show resolved Hide resolved

nfrisby force-pushed the nfrisby/preparing-8.3.0-pre branch from 654bacb to 0939a88 Compare August 14, 2023 15:58

github-advanced-security bot found potential problems Aug 14, 2023

View reviewed changes

nfrisby force-pushed the nfrisby/preparing-8.3.0-pre branch from 0939a88 to fdd45ff Compare August 14, 2023 17:48

nfrisby reviewed Aug 14, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Fees.hs Show resolved Hide resolved

nfrisby force-pushed the nfrisby/preparing-8.3.0-pre branch from fdd45ff to b517214 Compare August 14, 2023 18:07

nfrisby reviewed Aug 14, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs Show resolved Hide resolved

Jimbo4350 approved these changes Aug 14, 2023

View reviewed changes

newhoggy reviewed Aug 15, 2023

View reviewed changes

nfrisby force-pushed the nfrisby/preparing-8.3.0-pre branch from fa68b41 to ef35b6c Compare August 16, 2023 15:45

Jimbo4350 reviewed Aug 16, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Governance/Actions/ProposalProcedure.hs Outdated Show resolved Hide resolved

Jimbo4350 force-pushed the nfrisby/preparing-8.3.0-pre branch 2 times, most recently from a06068a to 4556af2 Compare August 17, 2023 17:30

carbolymer mentioned this pull request Aug 18, 2023

Add consensus governance queries #196

Merged

10 tasks

Jimbo4350 force-pushed the nfrisby/preparing-8.3.0-pre branch from 12bc50c to d67ab5b Compare August 18, 2023 16:52

disassembler force-pushed the nfrisby/preparing-8.3.0-pre branch from b4a5569 to 513de10 Compare August 18, 2023 23:30

Jimbo4350 force-pushed the nfrisby/preparing-8.3.0-pre branch from 64738a5 to 0519ee1 Compare August 21, 2023 15:58

Jimbo4350 changed the title ~~DO NOT MERGE preparing 8.3.0 pre~~ Bump CHaP to cardano-ledger-conway-1.7.0.0 Aug 21, 2023

Jimbo4350 force-pushed the nfrisby/preparing-8.3.0-pre branch 3 times, most recently from 1e0e393 to f5fae2c Compare August 21, 2023 18:21

Jimbo4350 marked this pull request as ready for review August 21, 2023 18:22

Jimbo4350 requested review from a team, dcoutts, erikd and disassembler as code owners August 21, 2023 18:22

Jimbo4350 force-pushed the nfrisby/preparing-8.3.0-pre branch from d8b91e0 to f5fae2c Compare August 21, 2023 18:32

disassembler force-pushed the nfrisby/preparing-8.3.0-pre branch from f5fae2c to fef23f2 Compare August 21, 2023 18:35

Jimbo4350 added 11 commits August 21, 2023 15:20

Bump CHaP to cardano-ledger-conway-1.7.0.0

4a09200

Update ledger certificate types

dc4b10a

Update GovernanceAction with ledger types and propagate these changes to

71fadbf

the relevant functions

Update TxVotes to reflect changes in ledger types

ca0a83c

Change query ConstitutionHash constructor to QueryConstitution

374b586

QueryConstitution now returns the Constitution

Simplify ledger script types with pattern synonyms

f26f670

Update getVotes with TxVotes

1d2cafd

Update key roles

ed2262a

Update Cardano.Api.LedgerState with consensus types

ab26ba6

Update Protocol m (CardanoBlock StandardCrypto) instance with Cardano…

fb27786

…ProtocolParams StandardCrypto Implement Semigroup (Ledger.ConwayPParams StrictMaybe era) Update ScriptErrorMissingScript

Expose various types

6b03aa4

disassembler force-pushed the nfrisby/preparing-8.3.0-pre branch from fef23f2 to 6b03aa4 Compare August 21, 2023 19:21

disassembler enabled auto-merge August 21, 2023 19:28

disassembler approved these changes Aug 21, 2023

View reviewed changes

disassembler added this pull request to the merge queue Aug 21, 2023

Merged via the queue into main with commit d235e12 Aug 21, 2023

disassembler deleted the nfrisby/preparing-8.3.0-pre branch August 21, 2023 20:56

carbolymer mentioned this pull request Aug 22, 2023

Add governance query commands IntersectMBO/cardano-cli#189

Merged

10 tasks

newhoggy pushed a commit that referenced this pull request Mar 11, 2024

Merge pull request #179 from input-output-hk/jordan/propose-governanc…

f1783d0

…e-action Add goverance info action creation to era based cli

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump CHaP to cardano-ledger-conway-1.7.0.0 #179

Bump CHaP to cardano-ledger-conway-1.7.0.0 #179

carbolymer commented Aug 9, 2023 •

edited by disassembler

Loading

carbolymer Aug 9, 2023

nfrisby Aug 9, 2023

carbolymer Aug 9, 2023

nfrisby commented Aug 9, 2023 •

edited

Loading

nfrisby commented Aug 14, 2023 •

edited

Loading

nfrisby commented Aug 14, 2023

Jimbo4350 Aug 14, 2023

nfrisby Aug 14, 2023

nfrisby Aug 14, 2023

Jimbo4350 commented Aug 14, 2023

newhoggy Aug 15, 2023 •

edited

Loading

nfrisby Aug 15, 2023 •

edited

Loading

newhoggy Aug 15, 2023

nfrisby Aug 15, 2023

Jimbo4350 Aug 16, 2023

nfrisby Aug 16, 2023 •

edited

Loading

Bump CHaP to cardano-ledger-conway-1.7.0.0 #179

Bump CHaP to cardano-ledger-conway-1.7.0.0 #179

Conversation

carbolymer commented Aug 9, 2023 • edited by disassembler Loading

Changelog

Context

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfrisby commented Aug 9, 2023 • edited Loading

nfrisby commented Aug 14, 2023 • edited Loading

nfrisby commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 commented Aug 14, 2023

newhoggy Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

nfrisby Aug 15, 2023 • edited Loading

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

nfrisby Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

carbolymer commented Aug 9, 2023 •

edited by disassembler

Loading

nfrisby commented Aug 9, 2023 •

edited

Loading

nfrisby commented Aug 14, 2023 •

edited

Loading

newhoggy Aug 15, 2023 •

edited

Loading

nfrisby Aug 15, 2023 •

edited

Loading

nfrisby Aug 16, 2023 •

edited

Loading