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

feat(cardano): add support for CIP36 governance registration format #82

Closed
wants to merge 6 commits into from

Conversation

davidmisiak
Copy link
Collaborator

@davidmisiak davidmisiak commented Sep 24, 2022

This PR adds support for CIP36 governance registration format. Changes:

  • New field: delegations array (each delegation consists of a voting key and its voting power proportion) as an alternative to delegating the entire voting power to a single voting key. The array is embedded in the protobuf message due to simplicity (max. 32 delegations).
  • New field: voting_purpose integer (currently 0 = Catalyst, 1 = other). For CIP36 registrations, this field is always serialized, (value 0 is used if not provided by the client).
  • All Catalyst occurences are replaced by governance, since the format should be usable for other governance purposes, not only Catalyst.
  • The ed25519_pk bech32 prefix is replaced by gov_vk for the purpose of displaying voting public keys.

The new 1694' derivation path and the vote cast call implementation is not included in this PR.

Copy link
Collaborator

@gabrielKerekes gabrielKerekes left a comment

Choose a reason for hiding this comment

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

Looks good in general. I left just a few small suggestions.

As mentioned offline - I wasn't able to compare the auxiliary data hash with cardano-cli so please investigate. The produced data look good to me though.

core/src/apps/cardano/auxiliary_data.py Outdated Show resolved Hide resolved
core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
@davidmisiak davidmisiak changed the title feat(cardano): add support for CIP36 Catalyst registration format feat(cardano): add support for CIP36 governance registration format Sep 28, 2022
@gabrielKerekes
Copy link
Collaborator

@davidmisiak One thing I noticed while reviewing the connect PR. Maybe we could display "Voting purpose: Catalyst" and "Voting purpose: Other" for voting purpose 0 and other respectively. What do you think?

@davidmisiak
Copy link
Collaborator Author

@davidmisiak One thing I noticed while reviewing the connect PR. Maybe we could display "Voting purpose: Catalyst" and "Voting purpose: Other" for voting purpose 0 and other respectively. What do you think?

What about Catalyst / 1 (other) / 2 (other) / etc.? @janmazak what do you think?

@janmazak
Copy link
Collaborator

janmazak commented Oct 5, 2022

@davidmisiak One thing I noticed while reviewing the connect PR. Maybe we could display "Voting purpose: Catalyst" and "Voting purpose: Other" for voting purpose 0 and other respectively. What do you think?

What about Catalyst / 1 (other) / 2 (other) / etc.? @janmazak what do you think?

I think the number should be displayed even in case there is some text. Esp. because "other" might expand in the future. If there is enough space on the screen, go for it (I won't do anything on Ledger, I guess it's not worth the additional code size --- you have to declare tmp buffers etc. to concatenate strings in C).

core/src/apps/cardano/auxiliary_data.py Show resolved Hide resolved
core/src/apps/cardano/auxiliary_data.py Show resolved Hide resolved
core/src/apps/cardano/layout.py Outdated Show resolved Hide resolved
@davidmisiak davidmisiak force-pushed the cardano-governance-cip36 branch 2 times, most recently from 564b49f to cd333a8 Compare October 7, 2022 12:47
@davidmisiak
Copy link
Collaborator Author

Merged in trezor#2561

@davidmisiak davidmisiak closed this Nov 1, 2022
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.

4 participants