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

Add support for CIP36 governance registration format #137

Merged
merged 21 commits into from
Feb 7, 2023

Conversation

davidmisiak
Copy link
Collaborator

@davidmisiak davidmisiak commented Oct 1, 2022

TODO:

  • add CIP36 support on Ledger (and remove the @ts-ignores)
  • manually test whether registrations are accepted by the voting sidechain
    • done on Trezor (used voting-tools to check registration validity)
  • remove local lib builds (@trezor/connect, @trezor/transport)
  • add correct HW app versions to catalyst-voting-registration-example.md

@davidmisiak davidmisiak requested a review from janmazak October 1, 2022 15:28
@gitmachtl
Copy link
Contributor

gitmachtl commented Oct 1, 2022

@davidmisiak

  • is it possible to add more vote-public-key input options? for example to read normal cli .vkey files (getting the key from the cborHex JSON entry), or directly using a gov_vk1xxxxxxxxxx bech key string? maybe doing a --vote-public-key-string for a direct input or an autodetection like i use in cardano-signer?
  • why is there a need to specify the network? as a check if the address is for the right chain?
  • is it possible to make an optional --vote-purpose parameter which default to zero (=Catalyst), but enabling other purpose numbers. its open in CIP-36 for future usage.

when i try to generate metadata with the current commit i always end up with Error: invalid Catalyst registration voting key

EDIT:
we are currenty talking multiple edge cases in the iog-governance slack channel. one of them is that it's also valid to generate an empty delegation array -> deregistration. in cardano-signer i added an extra flag --deregister to allow this, and not having deregistration by accident if people just forget to add a vote-public-key. in the case of a deregistration, i also don't read in a rewards-stake-address, because that information is more or less useless. its currently in discussion if map-entry 3 is needed in that case or if it can be a bytearray(length=0).
a de-registration was moved to use the key 61286, like described in the PR here. its up to the tools/wallets to implement this feature or not.

after a few discussions, the decision was made that multiple vote-key entries are valid. i currently check for the following conditions in cardano-signer:

  • total vote-weight is not zero
  • signing stake public key is different to all voting keys (because the new voting-keys are derived from another path, so there is no way that the stake public key is identical to a voting key

exporting the governance public key via the cardano-app version (https://github.com/vacuumlabs/ledger-app-cardano-shelley/tree/governance) for path 1694H/1815H/0H/0/0 is supported as it looks like. using your commit to derive it from the device ends with an error not implemented after the export.

generating the voting keys via the old method (https://github.com/vacuumlabs/cardano-hw-cli/blob/afcd46e15929c62aaca06a983de9500339daee2b/docs/catalyst-voting-registration-example.md#create-catalyst-voting-keys) is obsolete now, they must be derived from 1694H/1815H/0H/0/0 for cip-36. like:

cardano-hw-cli address key-gen \
      --path 1694H/1815H/0H/0/0 \
      --verification-key-file "gov.voting.vkey" \
      --hw-signing-file "gov.voting.hwsfile"

as discussed in the slack channel, it would be cool if the vkey file gets the description "Governance Hardware Verification Key" and "Governance Hardware Signing File". like:

Hardware-Voting-Signing-Key: gov.voting.hwsfile

{
  "type": "PaymentHWSigningFileShelley_ed25519",
  "description": "Governance Hardware Signing File",
  "path": "1694H/1815H/0H/0/0",
  "cborXPubKeyHex": "58400186abff41121414f856fd8cf613399f9a30e413bb1c75c2413f46121f020349d8426b745ca51de4f5bc02f3f403288a5457fc480c11b2af5df1703ce9170d8f"
}

Hardware-Voting-Public-Key: gov.voting.vkey

{
  "type": "PaymentVerificationKeyShelley_ed25519",
  "description": "Governance Hardware Verification Key",
  "cborHex": "58200186abff41121414f856fd8cf613399f9a30e413bb1c75c2413f46121f020349"
}

Not sure if the type should also be changed?
So having a file with the content of the public-key as plaintext like we had before will be a rare thing i guess. More likely to use it directly with such .vkey files or providing the bech-key directly as a parameter. i made a PR to get the prefix gov_vk and gov_sk into CIP-0005 for that purpose too. But it should work with all bech prefixes imo.

Also, vote-public-keys derived on the CLI via cardano-address for path 1694H/1815H/0H/0/0 might be extended ones like:

{
  "type": "PaymentExtendedVerificationKeyShelley_ed25519_bip32",
  "description": "Governance Verification Key",
  "cborHex": "5840c2cd50d8a231fbc1444d65abab4f6bf74178e6de64722558eeef0b73de293a8a3e852c83508c7d300593734ec84351c3d1916d00571a57f3c0f63a7c972eb299"
}

So reading in the vote-public-key from such files should only read the non-extended part of the public-key. Otherwise your internal length check would fail.

Sorry for the SPAM 😉

@gitmachtl
Copy link
Contributor

@davidmisiak any updates on that front?

README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/command-parser/parserConfig.ts Outdated Show resolved Hide resolved
src/commandExecutor.ts Outdated Show resolved Hide resolved
src/commandExecutor.ts Outdated Show resolved Hide resolved
src/commandExecutor.ts Outdated Show resolved Hide resolved
src/crypto-providers/trezorCryptoProvider.ts Outdated Show resolved Hide resolved
src/crypto-providers/trezorCryptoProvider.ts Outdated Show resolved Hide resolved
@gitmachtl
Copy link
Contributor

gitmachtl commented Nov 29, 2022

@janmazak as it looks like the common sense is that we allow any payment address to be used for the rewards address (was a stake address before), so the --reward-address-signing-key parameter should not be a required parameter anymore, but an optional one right?

Will the --vote-public-key parameters stay as the only input option (referencing a file), or will there also be an option like --vote-public-key-string to directly provide a bech vote public key gov_vk1... as an input parameter?

Should the whole function category moved from catalyst to a general governance naming?
cardano-hw-cli catalyst voting-key-registration-metadata ... -> cardano-hw-cli governance voting-key-registration-metadata ...

@gitmachtl
Copy link
Contributor

is there a way to add --vote-public-key-string as input options? including the hw-cli in automated env is a bit hard when you have to write out all keys into files first and than referencing those files on the hw-cli call. if not possible, there is a solution. just asking.

@janmazak janmazak force-pushed the governance-cip36 branch 2 times, most recently from 37c42a9 to 293ab5b Compare December 2, 2022 13:57
@janmazak janmazak force-pushed the governance-cip36 branch 2 times, most recently from 1787961 to 75122ae Compare December 4, 2022 18:17
@janmazak janmazak force-pushed the governance-cip36 branch 2 times, most recently from be05198 to 243b409 Compare December 4, 2022 18:22
@gitmachtl
Copy link
Contributor

Some bugs found -> #143 (comment)

@gitmachtl
Copy link
Contributor

gitmachtl commented Dec 10, 2022

I did some testing using the --vote-public-key-string parameters. Used 7 vote-keys (with doubles) with different weights, etc. I compared the generated cbor file with the one that i generated with my cardano-signer tool. For that i derived the ledger keys to normal cli keys first, so i had them as normal signing/verification keys. Result, the generated cbor files are absolutely identical. 🥳

image
image

janmazak and others added 11 commits December 13, 2022 00:48
* remove code related to internal hw-cli tx format

* Remove extra exclamation marks

* Remove old items from autocomplete.sh

* Rename signing validation to witnessing validation

* Add description to test/unit/commandParser/res/tx.raw

* Update interop-lib

* Change tx format in trezor tests

* Change tx format in cli tx files

* Add comments about manual test conversion

Co-authored-by: David Misiak <[email protected]>
@janmazak janmazak mentioned this pull request Feb 6, 2023
@janmazak janmazak marked this pull request as ready for review February 7, 2023 21:13
@janmazak janmazak merged commit fa5a5fa into develop Feb 7, 2023
@janmazak janmazak deleted the governance-cip36 branch February 7, 2023 21:29
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.

3 participants