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 CIP-36 Catalyst registration format #6735

Merged
merged 6 commits into from
Nov 14, 2022

Conversation

davidmisiak
Copy link
Contributor

This PR adds:

  • Support for CIP-36 governance registration format. The corresponding firmware PR is here, please see cardanoSignTransaction.md changes for details.
  • Replacing all catalyst occurences by governance (so that it is clear that they serve also for other governance purposes, not only Catalyst).
  • A minor version check fix for required signers in non-Plutus signing modes.
  • Support for new testnets.

I believe that the only breaking change is renaming these two items in the public API:

  • catalystRegistrationParameters to governanceRegistrationParameters (in CardanoAuxiliaryData)
  • catalystSignature to governanceSignature (in CardanoAuxiliaryDataSupplement)

@mroz22
Copy link
Contributor

mroz22 commented Nov 1, 2022

I believe that the only breaking change is renaming these two items in the public API:

Would it be too painful to support old names as a fallback? so that we do not introduce any breaking changes?

cc @marekrjpolak

@davidmisiak
Copy link
Contributor Author

Would it be too painful to support old names as a fallback? so that we do not introduce any breaking changes?

I'm afraid that apart from making the code a bit more complex, it might end up being even more confusing for clients (similar item names that seem related, when in fact representing the same data) than the breaking change, which requires only a trivial rename on the client side (easily pointed out by a typechecker). Also, the change is only related to Catalyst registrations, so ordinary Cardano txs are not even affected. Or do you consider maintaining and explaining the duplicated items preferrable anyway? (Btw, LedgerJS is doing the same rename.)

@mroz22
Copy link
Contributor

mroz22 commented Nov 8, 2022

which requires only a trivial rename on the client side (easily pointed out by a typechecker)

still, it would break already existing implementations due to the nature of @trezor/connect which is updated "on the fly" for clients.

We are basically facing one decision - are there any clients using catalystRegistrationParameters ? If yes and we proceed with this, their code will stop working unfortunately. So this is a bit risky. I would still be in favour adding one statement, something like this?

// @ts-expect-error
if (params.oldname) {
   console.warn('please use newname') 
   params.newname = params.oldname
}

@davidmisiak
Copy link
Contributor Author

We are basically facing one decision - are there any clients using catalystRegistrationParameters ?

There are, but the next Catalyst round shouldn't happen sooner than in late December or January as far as I know, so there should be enough time for clients to update their Connect version.

I would still be in favour adding one statement, something like this?

Ah, so you mean keeping only the new type definitions? That would be much easier. (Though we should also do signed_data.oldname = signed_data.newname when returning the signed data in that case, right?)

@mroz22
Copy link
Contributor

mroz22 commented Nov 8, 2022

so there should be enough time for clients to update their Connect version.

99% users receive update of connect automatically when we release it. There is a way how to "pin" your connect version, using

TrezorConnect.init({ connectSrc: 'https://connect.trezor.io/9.0.0/`,... }) 

but since it is not very documented people are not using it.

Ah, so you mean keeping only the new type definitions

Yes, keeping only the new definitions would be ok. Their code will still work after we roll out update and once they update their connect version, they will also receive the new types so they will update their code too.

(Though we should also do signed_data.oldname = signed_data.newname when returning the signed data in that case, right?)

Yes runtime should remain completely identical please 🙏 sorry for that. We already shot ourselves in leg once doing a breaking change update when we believed it would not affect anybody but it did.

@davidmisiak
Copy link
Contributor Author

Alright, so a6c884e should fix this (sorry for the delay). I also tried signing a Catalyst registration with the legacy param (and expecting the legacy result) using a testing JS app, it should work as expected.

@mroz22
Copy link
Contributor

mroz22 commented Nov 10, 2022

Thanks, it looks great. We should be able to release together with firmware.

@marekrjpolak marekrjpolak self-requested a review November 14, 2022 14:11
Copy link
Contributor

@marekrjpolak marekrjpolak 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 and the tests are passing. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants