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(connect-cardano): allow external reward addresses in governance registrations #7768

Merged
merged 4 commits into from
Mar 17, 2023

Conversation

davidmisiak
Copy link
Contributor

The corresponding firmware PR is here. Changes in Cardano CIP-36 vote key registrations:

  • The address to receive voting rewards can be provided as an address string (paymentAddress) instead of address parameters.

  • Renamed identifiers (initiated by clarifications in CIP-36, context e.g. here and here):

    • governance replaced by cVote (all occurences)
    • rewardAddressParameters replaced by paymentAddressParameters
    • votingPublicKey replaced by votePublicKey

    The changes are also present in the public API, but I kept silent support for the old names, like we did in feat(cardano): add support for CIP-36 Catalyst registration format #6735 when we replaced catalyst by governance (I know that the renaming frequency is a bit unfortunate, but the Cardano terminology seems to be still evolving). Is this alright or would you prefer a different solution?

@davidmisiak davidmisiak changed the title feat(cardano): allow external reward addresses in governance registrations feat(connect-cardano): allow external reward addresses in governance registrations Mar 4, 2023
@sime sime added the connect-popup Connect popup used by 3rd parties label Mar 9, 2023
@mroz22 mroz22 added the connect Connect API related (ie. fee calculation) label Mar 9, 2023
@mroz22 mroz22 added this to the connect 9.0.8 milestone Mar 9, 2023
@mroz22
Copy link
Contributor

mroz22 commented Mar 9, 2023

Hi, thanks for contribution, we are looking into this PR now.

I'd stick to keeping old namings along with the new ones. In general, we should avoid breaking changes here.

Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

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

Hi @davidmisiak I couldn't spot any issues here so it looks like it is ready to merge

Just let me ask, do you have any testing environment where @trezor/qa could test this prior to release/

@mroz22 mroz22 merged commit 2e373e6 into trezor:develop Mar 17, 2023
@davidmisiak
Copy link
Contributor Author

Thanks for processing the PR!

Just let me ask, do you have any testing environment where @trezor/qa could test this prior to release/

I'm not sure what kind of environment do you mean. We usually test the changes in Trezor Firmware and Connect in cardano-hw-cli. We will update hw-cli to use a build of Connect with the changes from this PR in the following days/weeks (though the changes are rather minor and mostly in naming). Is this something that could be useful to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connect Connect API related (ie. fee calculation) connect-popup Connect popup used by 3rd parties
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants