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.

Sign up for GitHub

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

Jump to bottom

verification-key: support DRep keys as well as committee keys, extended or not #652

Merged
merged 5 commits into from
Mar 19, 2024
Merged

verification-key: support DRep keys as well as committee keys, extended or not #652

merged 5 commits into from
Mar 19, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Mar 18, 2024

Changelog

- description: |
    verification-key: support DRep keys as well as committee keys, extended or not
# 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

Fixes #651

How to trust this PR

  • Run the new tests before the PR, witness the tests don't pass
  • Run the new tests on the PR's head, witness the tests pass

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/extend-key-verification-key branch 5 times, most recently from 255a3c9 to a0d287e Compare March 18, 2024 15:07
@smelc smelc marked this pull request as ready for review March 18, 2024 15:08
carbolymer
carbolymer reviewed Mar 18, 2024
View reviewed changes
cardano-cli/src/Cardano/CLI/Read.hs Outdated
@@ -721,6 +721,8 @@ readWitnessSigningData (KeyWitnessSigningData skFile mbByronAddr) = do
-- A Byron address should only be specified along with a Byron signing key.
Left ReadWitnessSigningDataSigningKeyAndAddressMismatch
where
-- If you update these variables, consider updating the ones with the same
-- names in Cardano.CLI.Read
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is confusing - it's Cardano.CLI.Read module. Copy & paste error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, copy/paste error. good catch! Corrected 👍

carbolymer
carbolymer reviewed Mar 18, 2024
View reviewed changes
cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/Committee.hs
H.noteM_ $ bracketSem semaphore $ \skeyFile -> execCardanoCLI
[ "conway", "key", "verification-key"
, "--signing-key-file", skeyFile
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need a semaphore for the signing key. It's used as an input here (only read), so protecting it seems redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 👍

carbolymer
carbolymer reviewed Mar 18, 2024
View reviewed changes
cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/Committee.hs Outdated
txBody <- noteInputFile "test/cardano-cli-golden/files/input/governance/drep/extended-key-signing/tx.body"

outGold <- H.note "test/cardano-cli-golden/files/golden/governance/committee/tx.cold.extended.signed"
outFile <- H.noteTempFile tempDir "outFile"

H.noteM_ $ execCardanoCLI
H.noteM_ $ bracketSem extendedColdCCSkeySem $ \skeyFile -> execCardanoCLI
Copy link
Contributor

Choose a reason for hiding this comment

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

Semaphore for input file (which is only read) seems redundant here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 👍

carbolymer
carbolymer approved these changes Mar 18, 2024
View reviewed changes
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.

Nice work.

cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/DRep.hs Outdated
txBody <- noteInputFile "test/cardano-cli-golden/files/input/governance/drep/extended-key-signing/tx.body"

outFile <- H.noteTempFile tempDir "outFile"
outGold <- H.note "test/cardano-cli-golden/files/golden/governance/drep/extended-key-signing/tx.signed"

void $ execCardanoCLI
H.noteShowM_ $ bracketSem extendedDRepSkeySem $ \skeyFile -> execCardanoCLI
Copy link
Contributor

Choose a reason for hiding this comment

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

Semaphore for an input key (which is only read here) seems redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, semaphore is only needed for concurrent writes, so not for input files, only golden ones!

cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/DRep.hs Outdated
vkeyFileOut <- noteTempFile tempDir "drep.extended.vkey"
goldenFile <- H.note "cardano-cli/test/cardano-cli-golden/files/golden/governance/drep/drep-extended.vkey.out"

H.noteShowM_ $ bracketSem extendedDRepSkeySem $ \skeyFile -> execCardanoCLI
Copy link
Contributor

Choose a reason for hiding this comment

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

The same case - semaphore for input file is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 👍

cardano-cli/test/cardano-cli-test-lib/Test/Cardano/CLI/Aeson.hs
Comment on lines +152 to +163
case v of
Object inner ->
return $ Object $ Aeson.KeyMap.delete (Aeson.fromText "description") inner
Array _ -> failWrongType "array"
Number _ -> failWrongType "number"
Bool _ -> failWrongType "bool"
String _ -> failWrongType "string"
Null -> failWrongType "null"
where
failWrongType got = do
H.note_ $ "Expected object but got: " <> got
H.failure
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about using lens-aeson here?

Suggested change
case v of
Object inner ->
return $ Object $ Aeson.KeyMap.delete (Aeson.fromText "description") inner
Array _ -> failWrongType "array"
Number _ -> failWrongType "number"
Bool _ -> failWrongType "bool"
String _ -> failWrongType "string"
Null -> failWrongType "null"
where
failWrongType got = do
H.note_ $ "Expected object but got: " <> got
H.failure
v & L.atKey "description" .~ Nothing

Matching on Value constructors seems a bit excessive here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I assume right that the lens will not fail if the Value is not an Object? (it will simply do nothing)

That's just for me to know, behavior is fine for me in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so. That's how I understand the signature. Something similar to alter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carbolymer> this would be the first time we use lens and lens-aeson in this repo, which I'm not comfortable to add myself. It's quite something we are pulling here, so I rather keep my lowtech initial version.

cardano-cli/test/cardano-cli-test/Test/Cli/VerificationKey.hs Outdated Show resolved Hide resolved
@smelc smelc force-pushed the smelc/extend-key-verification-key branch 3 times, most recently from 5b890c5 to 21df4d3 Compare March 19, 2024 09:27
@smelc smelc force-pushed the smelc/extend-key-verification-key branch from 21df4d3 to 111f71e Compare March 19, 2024 09:42
@smelc smelc enabled auto-merge March 19, 2024 09:43
@smelc smelc force-pushed the smelc/extend-key-verification-key branch from 111f71e to 2b374d0 Compare March 19, 2024 09:50
@smelc smelc added this pull request to the merge queue Mar 19, 2024
Merged via the queue into main with commit 149940e Mar 19, 2024
17 checks passed
@smelc smelc deleted the smelc/extend-key-verification-key branch March 19, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer approved these changes

@Jimbo4350 Jimbo4350 Awaiting requested review from Jimbo4350 Jimbo4350 is a code owner

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cardano-cli conway key verification-key command does not support DRep, CC-Cold & CC-Hot signing keys
2 participants
@smelc @carbolymer