Add cardano support to trezorctl and some tests

[ddeath]

Added device tests for get_address and sign_transaction calls.

To the trezorctl was also added get_public_key, sign and verify message

[tsusanka]

@matejcik I can leave this for you to review, right?

[matejcik]

I have reviewed the changes. Please see comments and make modifications where appropriate.

In general, my main question is, how do you expect this to be used, from the user side? Is there a tool/service that generates the appropriate JSON data? If not, then, I suppose we'll keep it like this for now. I'm working on some changes that would make it more useful; namely, converting a dict to protobuf directly.

In any case, unless there are good reasons against, please go with my suggestion for the sign_tx() signature. Then it depends on where the JSON comes from. If the format is arbitrary, I suggest parsing it in trezorctl:

inputs = [cardano.create_input(input) for input in transaction['inputs']]
outputs = [cardano.create_output(output) for outputs in transaction['outputs']]
transactions = transaction['transactions']

If the format comes from somewhere else, I'd keep the create_transaction function, but make it return a tuple inputs, outputs, transactions as opposed to a protobuf message (which doesn't include the "transactions" field)`

[matejcik]

Also please add the remaining tests (sign/verify message, get public key i think?)

[ddeath]

@matejcik I tried to fix all your comments and also added tests...

About that JSON tool - there is no such a tool. If somebody want to use it then they could look here: https://github.com/vacuumlabs/cardanolite/blob/78b03bbcf80a9b4d04444d0f954c7c8184ea636d/app/frontend/wallet/cardano-wallet.js#L82

There is our own code for creating tx or they can take a look to connect v5.

The CI is failing because of some other test fails to fetch some data from internet - I guess that we did not break it

[matejcik]

Tests look reasonable. Please fix my latest comments, I'll look over the whole thing again tomorrow.

Travis failure is on our side; if you rebase on top of current master, it should go away.

[tsusanka]

Please also modify the signing test where now CardanoSignedTx message is returned (trezor/trezor-core@e657397) (if needed).

[ddeath]

I fixed comments and also did the rebase.

[tsusanka]

@ddeath we would like to unify what derivation paths are sent to which coins. We came up with this doc. We would like Cardano wallets to send 44'/1815'/a' for get public key and 44'/1815'/a'/0/i for get_address / sign.

1) Is that okay on your side? 2) Do you think you could modify the tests accordingly? I know it's a pain to regenerate the data, but the tests serve as a point of reference sometimes and it would be unfortunate to have it incorrect.

[ddeath]

@tsusanka

1. I would suggest to change get_address and sign to 44'/1815'/a'/{0,1}/i as I have almost ready PR for adding support for cardano derivation scheme v2 which uses 0 or 1 in that place. In the v1 it must be 0 but in v2 it can be 0 or 1
2. I will regenerate the tests

[matejcik]

I finally got to review the current state. It looks mostly OK.

Two things:

1. please add at least one invalid sig to verify_message test :) this way it's not testing much.

2. do the tests pass in the current trezor-core master? if not, they should all be marked xfail

[tsusanka]

I would suggest to change get_address and sign to 44'/1815'/a'/{0,1}/i as I have almost ready PR for adding support for cardano derivation scheme v2 which uses 0 or 1 in that place. In the v1 it must be 0 but in v2 it can be 0 or 1

Ok, agreed.

[ddeath]

@tsusanka I changed test so it reflect the derivation paths. Please check if that is what you mean. Also I overlooked the get_address and sign_transaction derivation paths and would suggest to change it to 44'/1815'/a'/{0,1}/{i, i'} as the official cardano wallet Daedalus is working with addresses derived with i' it is also possible to use i but for example Binance has problems to accept these kind of addresses. Also in derivation scheme v2 is used i by default in cardano new wallet Icarus.

@matejcik added invalid signatures tests. The test should be passing on master, but they are not because there was introduced bugs couple days ago in this commit where the name of messages are incorrectly swaped and also in this commit where the .ui was not renamed to .layout

So not sure if I should mark it as xfail. I will create a PR for adding derivation scheme v2 in couple of hours where I fixed it already. But not sure about your workflow so please just point me out what is the way to do it in this situation.

Thanks

[matejcik]

Btw for further reference it would be great if you didn't squash the commit every time, so we could see changes from last time and not have to read through the whole thing.

If your code is in core, and the tests are supposed to pass, then no xfail is needed. xfail is for cases when the tests are submitted ahead of the device code.

[matejcik]

In the meantime, I merged a big change into master. This should affect you very little, although you will probably get merge conflicts in trezorctl.

IMHO the safest way to resolve them is simply copy-paste your code from pre-merge trezorctl to post-merge trezorctl.

Another change that affects you is that @expect and @field are now the same decorator, so please modify that one use of @field to @expect(..., field='...')

afterwards, run make style (note you need python 3.6 for that)

[ddeath]

@tsusanka please ignore last conversation about i vs i'. It should be ok as it is, because as stated here https://github.com/trezor/trezor-crypto/pull/179#issuecomment-418340268 the derivation scheme v1 is going to be dropped

[ddeath]

@matejcik should be fixed now

[tsusanka]

@tsusanka please ignore last conversation about i vs i'. It should be ok as it is, because as stated here trezor/trezor-crypto#179 (comment) the derivation scheme v1 is going to be dropped

Ok so 44'/1815'/a'/{0,1}/i it is.

[matejcik]

looks good to merge from my side.

@tsusanka @ddeath any planned changes regarding the paths? Or should I merge it now and path changes will be resolved in a new PR?

[tsusanka]

IMHO it is correct now and the tests seem to use correct paths.

[ddeath]

I am ok with the current paths

[ddeath]

@matejcik added changes for cadrano derivation path v2 and marked all test as xfail

[matejcik]

Neat, merging. Now let's see what happens with core :) Thanks!