feat(cardano): add support for catalyst voting registration

[gabrielKerekes]

Motivation: Update to support Catalyst voting registration.

metadata field has been deprecated and replaced with auxiliaryData - if a client sends in metadata an error is thrown telling the client to use auxiliaryData instead. The auxiliaryData field can only be used with updated FW.

I had to update the legacyResults in the tests to also support failing tests so that the above use-cases could be tested.

Depends on trezor/trezor-firmware#1564.

Testing:

• yarn test:integration -i cardanoSignTransaction -g -f 2.3.6 passes, also I tested it with the firmware built from the PR referenced above by manually updating the trezor-user-env docker container

[gabrielKerekes]

PR has been updated to correspond to updates made in trezor/trezor-firmware#1564.

@szymonlesisz I've added re-exports for Cardano enums from protobuf.d.ts to cardano.d.ts. However, when trying to import them from a client app the values (or the enums, not sure) are undefined. Do you maybe know if there is a way we could make this enums available for clients?

[szymonlesisz]

@gabrielKerekes protobuf types are intentionally not exposed to public because of ambiguous names (like Address)
but i agree that this should be allowed for enums/objects
we have 2 possibilities:

• export them explicitly from the "network file" like cardano.d.ts [pros: you can rename it, const: has to be done manually]
• export all protobuf enums from the index [const: cant be renamed]

and i dont know which way is better, any suggestions?

[gabrielKerekes]

I would prefer the first option - explicit export from "network file". This is also what I tried to do here, notice the exports at the bottom of src/ts/types/networks/cardano.d.ts. Or is this not what you mean?

However, when trying to import CardanoAddressType in another typescript project the enum is undefined - trying to do CardanoAddressType.BASE results in Cannot read property 'BASE' of undefined. Normal types (e.g. CardanoSignedTx) are imported and recognized without problems. I've tried various things, but nothing seems to work correctly.

While writing this comment I tried one more thing - making the enum const as per this SO post and this finally worked. I am not sure whether this a good approach.

[szymonlesisz]

export const enum looks legit, but in needs to be fixed here in the script which is generating types from protobuf. should i do it for you globally?

[szymonlesisz]

and i've noticed that test against 2-master FW (not released 2.3.7) are failing on signMetadata test, should i update the fixtures (create legacy fixtures)?

Edit: fixed here is that correct?

Edit 2: i missed that you are removing this param in this PR :) reverting my changes then

[gabrielKerekes]

export const enum looks legit, but in needs to be fixed here in the script which is generating types from protobuf. should i do it for you globally?

Good point, I can try to update it tomorrow and if it would be wrong, you could update it then.

and i've noticed that test against 2-master FW (not released 2.3.7) are failing on signMetadata test, should i update the fixtures (create legacy fixtures)?

So this is resolved then or are the tests still failing? I tried running yarn test:integration -i cardanoSignTransaction -g which should run against the current master (right?) and all the tests passed.

[szymonlesisz]

i've temporary narrow this test to fw 2.3.6 and marked it as related to this PR
08cfe71

now im going to prepare this enum export and then you can rebase it and use it

[szymonlesisz]

@gabrielKerekes i just found this article about exporting const enum and now i'm thinking about replacing enum with Object.freeze, same as in flowtype declarations

Edit:
nope, ts is failing at: A 'const' initializer in an ambient context must be a string or numeric literal or literal enum reference.

[gabrielKerekes]

So we still don't have a good solution, right? If not, I could raise the issue internally with our TS people and maybe we would be able to figure something out.

[szymonlesisz]

this needs to be done differently, no typescript magic is required here.

Explanation:
connect is written with flowtype typescript is only a copy/reflection of flowtype and it doesnt have influence on runtime. it's only a helper for typescript project above the connect.

enums/Object.freeze are the only "non-type" values in protobuf definitions (flowtype doesnt even have enums)
those values were never exposed to public (no use case until now)

import { SomeEnum } from './protobuf' is not importing from src/ts/types/*.d.ts files but from src/js/types/*.js files (flowtype)

i will do it today, dont worry :)

[szymonlesisz]

@gabrielKerekes pls check this PR: #803, especially the second commit.
i'm not 100% sure should i remove duplicated constants from src/constants/cardano or leave it (it's a breaking change)

How to test it locally:

• checkout on this branch
• build npm package (yarn build:npm)

javascript:
create foo.js in connect root

const TC = require('./npm');

console.log(TC.CardanoAddressType, TC.CardanoPoolRelayType);

run it:
node ./foo.js

typescript:
create foo.ts in connect root

import TrezorConnect, { CardanoAddressType, CardanoPoolRelayType } from './npm';
console.log(CardanoAddressType, CardanoPoolRelayType);

run it:
ts-node ./foo.ts *ts-node is not a part of connect, so either add it (just for this test) or use it from different project (like ../trezor-suite/node_modules/.bin/ts-node ./foo.ts)

[gabrielKerekes]

Thanks for that PR - I've added a comment regarding the constants and just one small thing, otherwise it looks good and it works.

However, sorry, I just need to clarify if I understand this correctly. So even in a TypeScript project importing connect the cardano.d.ts is just a helper for types, but the actual values are imported from cardano.js at runtime, is that right? And since they aren't being exported in this PR yet, they are obviously undefined. Right?

[szymonlesisz]

exactly, typescript types were added not so long ago only as a helper (also step forward to migration from flowtype to typescript) and they don't have influence on the runtime code, this is why you get undefined

[gabrielKerekes]

Awesome, thanks for the explanation and resolution 👍 Please let me know when #803 will be ready so I can rebase this.

[szymonlesisz]

merged, you can rebase now

[gabrielKerekes]

Rebased. Should I also squash it at this point?

[szymonlesisz]

@gabrielKerekes did you update protobuf messages using this readme? it's explained how to do it automatically

$Exact is a flowtype utility and it's used whenever given protobuf type has at least 1 required field

and yes, you can squash it, but i would like to ask you for split the commits to be more transparent to something like this:

• 1st commit: chore: update proto messages trezor-firmware@XXX, where XXX is a commit from trezor-firmware master (example: 59732a1)
• 2nd commit: test: allow legacy results success to be false
• 3rd commit: feat(cardano): add support for catalyst voting registration

[gabrielKerekes]

No, I didn't update the protobuf messages, I wasn't aware of that README, but I noticed that you would always do some additional work on top of our PRs - so I suppose this is it, right?

And sure, I will do that first thing tomorrow together with the splitting of commits.

[szymonlesisz]

but I noticed that you would always do some additional work on top of our PRs - so I suppose this is it, right?

exactly, i'm always bumping those messages to be sure that nothing is missing.

quick how-to (slightly different from the readme):

• edit Makefie, comment lines 52 and 53, uncomment 55 and 56 (and change the path to your local trezor-firmware repo if needed)
• run make protobuf, json + js/types + ts/types should be modified
• revert Makefile changes

and that's it :)

[gabrielKerekes]

I've updated the protobuf messages and squashed the commits.

If you have already reviewed the code, I've also made one more change during rebasing. I changed the type of CardanoCatalystRegistrationParametersType.nonce to string in flow/ts and to amount for validateParams since it is a uint64 and can actually contain large values.

[szymonlesisz]

great, now the only concern left is that payload.metadata new error, which is kinda breaking change. if we release it today or tomorrow older implementations may stop working. I guess we should synchronize the release with you then?

[gabrielKerekes]

This shouldn't be a problem since the metadata field probably wasn't used at all. Adalite and Daedalus don't use metadata and we're checking with Yoroi. I'll let you know.

However it's definitely worth mentioning this in the changelog along with the exported enums - I suppose that is managed by you? Or shall I update it?

[szymonlesisz]

i will update changelog before the release

[gabrielKerekes]

I just heard from Yoroi guys and they also don't use metadata so I think this should be safe enough. 👍

i will update changelog before the release

Great, thanks!