Cardano Shelley Update 1/3

[gabrielKerekes]

As mentioned earlier in #611, we are updating Cardano to support the new Shelley era.

Since it requires quite a lot of changes we are splitting it to (at least) 3 PRs on Trezor FW. We will do the same with Trezor Connect so they would stay in sync.

Trezor FW Issue
Trezor FW PR 1

I've tried to find all the files that require changes, but I am not sure I found all the places that need to be updated.

I've updated types, methods and tests for both CardanoSignTransaction and CardanoGetAddress. Tests pass and I've also tried running it with an app running on localhost. I've also updated the READMEs, but I will check those once more when this gets approved. (In the READMEs I mention a mainnet protocol magic value of 0. We don't know what this value will be yet, so this will be updated when we find out.)

I do have some questions regarding what should be updated:

1. How do protobuf messages get updated? (I have been just copy and pasting messages-cardano.proto from Trezor FW repo)
2. Do both cardanoSignTransaction.spec.js and tests/__fixtures__/cardanoSignTransaction.js have to be updated? Or is the .spec.js osbolete now? (I'm going to be doing a lot more changes in the next PR and it would be simpler to edit just one - but it's not a big deal)
3. Are all the files edited manually or can they somehow be generated? (e.g. src/ts/types/networks/cardano.d.ts and src/js/types/networks/cardano.js are very similar and I was wondering if you edit them manually or not)

Please let me know if I have missed something major. Otherwise I'll be waiting for a normal review.

As also mentioned in the Trezor FW PR it is possible that some things will change in the following days, but it should only be minor changes.

[gabrielKerekes]

The checks are failing due to old protobuf messages not having the new properties. As I have mentioned, I am not sure how to update them correctly.

[mroz22]

Maybe rebase on current develop please. It should contain latest protobuf messages.

[gabrielKerekes]

The messages I'm missing are the ones I am updating in this Trezor FW PR which has not yet been merged, so I'm not sure rebasing would help.

[matejcik]

important point: can we safely drop fields from protobufs? won't that cause compatibility issues?
i'm assuming no, because Connect with the old definitions will not be able to talk to new FW anyway, and old FW (for new Connect) will not work because it can't sign valid transactions?

but wanted to check this

[gabrielKerekes]

Hi guys, do you think you could take a look at this draft of PR number 2 in our fork? I'm struggling with the nested types and I am not sure about the way I'm dealing with them in the code. I've only so far done CardanoGetAddress. The functional tests do pass, so it works (other tests are not yet complete).

However, I am not sure about the correctness of the code, since e.g CardanoAddressParameters.path is of type string | number[] although when returning the address (CardanoAddress) it should be strictly number[] as path was before. Could you please take a look at it and provide some guidance?

[gabrielKerekes]

Could you please let us know how we'll go about reviewing/merging this? Or what the next steps are? I've almost got the 2nd PR ready and the 3rd one will hopefully come shortly after. I can't create the next PRs without having a branch to PR against.

[gabrielKerekes]

I've got both the 2nd PR and the 3rd PR ready now. I can't move them to this repo, since they're based on this PR.

It would probably also make sense to merge at least this and the 2nd PR into one PR since the Trezor FW August release will contain changes, which aren't compatible with this PR, but require the 2nd PR. The 3rd PR will maybe also be included in the August release, but that is not yet confirmed.

I'll be waiting for instructions on what to do next.

[MichalPetro]

@matejcik @prusnak guys what about Trezor connect? PRs are still open. We already promised the community that we will deliver the integration with Trezor on AdaLite on the 5th August but we need to move this forward ASAP (all 3 PRs) if we want to have working companion app on the 5th.

[szymonlesisz]

i'm on it, i've just created a branch with new protobuf messages and now i'm trying to run and debug your PR.
I'll let you know in few hours

[szymonlesisz]

@gabrielKerekes
I've only so far done CardanoGetAddress. The functional tests do pass, so it works (other tests are not yet complete).

I'm pretty sure that its not possible :) It's failing here:
https://github.com/trezor/connect/blob/feat/cardano-fork/src/js/device/DeviceCommands.js#L405

you are sending address_n in Object root while according to protobuf messages it needs to be wrapped into address_parameters.address_n together with multiple others params (like address_type, address_n_staking etc)

So the first thing, those params needs to be passed to connect in some additional field (addressParams?)

EDIT:
i'm trying to fix it now

[gabrielKerekes]

Oh, I suppose you are testing this PR against the Trezor release branch? Because this PR is paired with the first Trezor FW PR which didn't have addressParameters yet. That's added in the 2nd Connect PR. The PR hasn't yet been made against this repo, since this one hasn't been merged yet.

[gabrielKerekes]

There have been 3 Trezor FW PRs. PR 1, PR2, PR3. And there are 3 Trezor Connect PRs corresponding (working) with each FW PR - this one, PR 2 and PR 3.

[szymonlesisz]

it couldn't be merged into develop before firmware release, otherwise it will break entire Cardano implementation (it will NOT work with older FW and there is no new FW released yet)

whats the point of merging this if it's not working? :)

so should i test part 2 instead?

[gabrielKerekes]

The point was to have both repos in sync - although that didn't really work out. However, as I mentioned in an earlier comment, since Trezor FW PR1 and PR2 have been merged and will definitely be released together, we can close this PR and deal only with the 2nd one.

[MichalPetro]

@szymonlesisz the old FW is not working anyways because Cardano hard-forked 2 days ago. We need this to be released together with FW release on 5th August or even better before so we can test the wallet integration properly.

[szymonlesisz]

ok, sorry was out of the pictrure with this cardano fork.
i've just spoke with one of the colleagues what was the motivation behind splitting this into parts and now i understand (i think)

so im skipping this and going straight to part 3. right?

[gabrielKerekes]

That depends on whether FW Part 3 will be squeezed into the release. But perhaps it might not be too much of a problem even if Connect has all 3 parts and FW has only the first two?

[szymonlesisz]

tbh for me it would be better to have all the code in one PR. im aware that it would be probably a lot of code, but switching between different FW build makes it's harder to test.

so could you pls create PR with 2 and 3 combined?

[szymonlesisz]

That depends on whether FW Part 3 will be squeezed into the release

I've just confirmed that it is present in new FW (freezed)

[gabrielKerekes]

I'll create it. I don't have much time though, so I will not combine the commits in any way.

[gabrielKerekes]

Combined PR - #638. I'm sorry, but I won't be available until monday, so hopefully you'll be able to do a review of it.

[szymonlesisz]

closing in favour of #639