Add cardano support to trezorctl and some tests #300

ddeath · 2018-08-18T08:48:49Z

Added device tests for get_address and sign_transaction calls.

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

tsusanka · 2018-08-20T09:39:49Z

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

matejcik · 2018-08-20T11:26:56Z

trezorlib/cardano.py

+
+
+@session
+def cardano_sign_transaction(client, transaction):


please rename to sign_tx ("tx" instead of "transaction", no "cardano" prefix)

also the signature is not great, see comments in trezorctl and on create_sign_transaction_message.

In general, these functions should consume either simple values, or filled out protobuf messages. In this case, best I could come up with is:

def sign_tx(client, inputs: List[CardanoTxInputType], outputs: List[CardanoTxOutputType], transactions: List[bytes])

matejcik · 2018-08-20T11:35:58Z

trezorctl

+@click.pass_obj
+def cardano_sign_transaction(connect, file):
+    client = connect()
+    signed_transaction = cardano.cardano_sign_transaction(client, transaction=json.load(file))


you should not put raw dict into the API call.
Instead, something like:

transaction = cardano.create_sign_tx(json.load(file)) signed_tx = cardano.sign_tx(client, transaction)

Also, how can the user create the JSON?
I expect there is a separate tool / script that generates it, but can it include the appropriate BIP32 paths?

fixed with you metioned suggestion bellow

matejcik · 2018-08-20T11:38:07Z

trezorlib/cardano.py

+
+@session
+def cardano_sign_transaction(client, transaction):
+    msg = create_sign_transaction_message(transaction)


this should not be part of this function, see comment in trezorctl

matejcik · 2018-08-20T11:39:00Z

trezorlib/cardano.py

+        ack_message = create_tx_ack_message(transaction.get('transactions'), tx_index)
+        response = client.call(ack_message)
+
+    return {


any reason to return a dict, as opposed to a tuple (hash, body) ?

It is so user will see what is hash and what is body without looking at code. if it is only tuple you will have hard times to identify body vs hash as both looks same

this is not user-facing function though. please return a tuple and do the conversion in trezorctl

matejcik · 2018-08-20T11:43:44Z

trezorlib/cardano.py

+
+
+def create_sign_transaction_message(transaction) -> messages.CardanoSignTransaction:
+    if not all(transaction.get(k) for k in ('inputs', 'outputs', 'transactions')):


please extract the lists of required fields to the top level:

REQUIRED_FIELDS_TRANSACTION = ("inputs", "outputs"... REQUIRED_FIELDS_INPUT = (... REQUIRED_FIELDS_OUTPUT = (...

Don't repeat the fields in the ValueError either.

Fixed except output fields as there is only amount required and one other but it does not matter which one - well there must be at least one address_n (at least in one of the outputs)

matejcik · 2018-08-20T11:47:06Z

trezorlib/cardano.py

+
+    outputs = []
+    for output in transaction.get('outputs'):
+        if not output.get('amount') or not(output.get('address') or output.get('path')):


please use the same all(...) check as above

alternately, a better approach could be ensure_required_fields(dict, REQUIRED_FIELDS) helper that raises the ValueError. In that case, you could even put the list of required fields into the error messages

Not sure how could I use all whene there are not || not ( || ) which is same as not || (not && not)

ok, my bad, didn't notice the nesting

trezorlib/cardano.py

matejcik · 2018-08-20T12:23:31Z

trezorlib/cardano.py

+    while response.tx_index is not None:
+        tx_index = response.tx_index
+
+        ack_message = create_tx_ack_message(transaction.get('transactions'), tx_index)


the create_tx_ack_message is sort of pointless, esp. if you get transactions directly as an argument. please inline it here.

(also maybe verify that the requested index makes sense? but AFAICT the index is pointless anyway, so maybe it doesn't matter)

fixed without index verification

matejcik · 2018-08-20T12:25:31Z

trezorlib/tests/device_tests/test_msg_cardano_get_address.py

+import pytest
+
+from .common import TrezorTest
+from .conftest import TREZOR_VERSION


you don't seem to be using TREZOR_VERSION

matejcik · 2018-08-20T12:27:51Z

trezorlib/tests/device_tests/test_msg_cardano_sign_transaction.py

+class TestMsgCardanoSignTransaction(TrezorTest):
+
+    def test_cardano_sign_transaction(self):
+        self.setup_mnemonic_nopin_nopassphrase()


ISTM this test could be vastly simplified if you used set_expected_responses

see e.g. https://github.com/trezor/python-trezor/blob/master/trezorlib/tests/device_tests/test_msg_signtx.py

I want to used that but I was unsure how to use it when I need to do swipes

Ah, the swipes thing again! Yes, it makes sense in that case. We need a better infrastructure for simulating swipes.

matejcik · 2018-08-20T12:34:42Z

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 · 2018-08-20T12:37:24Z

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

ddeath · 2018-08-21T16:51:42Z

@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 · 2018-08-21T17:05:13Z

trezorlib/cardano.py

+        ack_message = messages.CardanoTxAck(transaction=transaction_data)
+        response = client.call(ack_message)
+
+    return (


return response.tx_hash, response.tx_body

I just returned response because it is CardanoSignedTx message now. Is that ok?

trezorctl

matejcik · 2018-08-21T17:18:59Z

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 · 2018-08-21T18:21:50Z

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

ddeath · 2018-08-22T05:57:59Z

I fixed comments and also did the rebase.

tsusanka · 2018-08-30T11:57:45Z

@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.

Is that okay on your side?
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 · 2018-08-31T13:59:54Z

@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
I will regenerate the tests

matejcik · 2018-08-31T14:46:14Z

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

Two things:

please add at least one invalid sig to verify_message test :) this way it's not testing much.
do the tests pass in the current trezor-core master? if not, they should all be marked xfail

tsusanka · 2018-09-02T09:53:00Z

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 · 2018-09-04T09:47:06Z

@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 · 2018-09-04T10:17:27Z

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 · 2018-09-04T10:24:06Z

trezorlib/tests/device_tests/test_msg_cardano_sign_message.py

+
+@pytest.mark.cardano
+@pytest.mark.skip_t1  # T1 support is not planned
+class TestMsgCardanoGetPublicKey(TrezorTest):


please rename to match test contents

matejcik · 2018-09-04T10:25:00Z

trezorlib/tests/device_tests/test_msg_cardano_verify_message.py

+
+@pytest.mark.cardano
+@pytest.mark.skip_t1  # T1 support is not planned
+class TestMsgCardanoGetPublicKey(TrezorTest):


please rename test and method names, and consider using pytest.mark.parametrize

matejcik · 2018-09-04T10:29:41Z

trezorlib/tests/device_tests/test_msg_cardano_sign_message.py

+@pytest.mark.skip_t1  # T1 support is not planned
+class TestMsgCardanoGetPublicKey(TrezorTest):
+
+    def test_cardano_get_public_key(self):


also this

while you're at it, consider using @pytest.mark.parametrize to simplify test code here, see e.g. test_cancel

matejcik · 2018-09-04T10:30:56Z

trezorlib/tests/device_tests/test_msg_cardano_sign_message.py

+
+    def test_cardano_get_public_key(self):
+        self.client.load_device_by_mnemonic(
+            mnemonic='plastic that delay conduct police ticket swim gospel intact harsh obtain entire',


Any reason you're using this mnemonic? If not, please regenerate the sigs with the default all all all...

(a good reason would be, e.g., if these were officially documented test vectors)

I was choosing this mnemonic because the same is in trezor-core

In that case let's change it to the default all all all.... It should be changed in trezor-core too, but we'll probably do that later along with other currencies that use custom mnemonics for tests.

I can change it as I will be removing / changing all the tests because of change of derivation scheme in the next PR.

matejcik · 2018-09-04T10:33:44Z

trezorlib/tests/device_tests/test_msg_cardano_sign_message.py

+            signature = sign_message(self.client, parse_path(derivation_path), message)
+            assert expected_signature == hexlify(signature.signature).decode('utf8')
+
+        incorrect_messages = [


FWIW you don't need to test invalid sigs here, because you're already testing against an expected value; it's very unlikely that a sign_message would produce the particular signature that you're testing it's not producing.

(as opposed to the verify_message method, where you need a test that it can fail, that it doesn't return True for every input)

unless of course these are vectors for some known failure mode

matejcik · 2018-09-04T10:35:39Z

trezorlib/cardano.py

+REQUIRED_FIELDS_INPUT = ('path', 'prev_hash', 'prev_index', 'type')
+
+
+@expect(messages.CardanoAddress)


this should probably extract the @field("address") directly, right?

matejcik · 2018-09-04T13:16:50Z

trezorlib/tests/device_tests/test_msg_cardano_get_address.py

-        assert address == '2w1sdSJu3GVhMmEYeGYEPWahV1V17pFw59GfgqjSRqa6x1rKFxbyCZrQWLe78xdSx3zyed6DrrN5yMgoY7ST2vJeaMzUDB7W3WG'
-        address = get_address(self.client, parse_path("m/44'/1815'/0'/0/2")).address
-        assert address == '2w1sdSJu3GVeHCDfy3mjq8RkzkN3Vh7Di3cB8NRzkwkLQ2FAjxX1kvkNdP9hNBzyBVEJdeWwyb5GfFYXgKe7rPgvWj2QD8FE4W3'
+        address = get_address(self.client, parse_path(path)).address


ISTM this will fail now that get_address returns the address directly?

Yes it failed. Fixed. Also replaced custom mnemonic for all all...

matejcik · 2018-09-04T14:33:43Z

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)

CR fix

ddeath · 2018-09-05T08:25:31Z

@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

ddeath · 2018-09-05T08:26:03Z

@matejcik should be fixed now

tsusanka · 2018-09-05T10:26:40Z

@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 · 2018-09-05T12:22:18Z

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 · 2018-09-05T12:27:30Z

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

ddeath · 2018-09-05T12:43:02Z

I am ok with the current paths

Update submodule to trezor official one

ddeath · 2018-09-05T12:52:06Z

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

matejcik · 2018-09-05T12:56:08Z

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

ddeath mentioned this pull request Aug 18, 2018

Cardano support #295

Closed

ddeath force-pushed the cardano-tests branch from bcf7f16 to 61f7ba8 Compare August 18, 2018 09:06

ddeath mentioned this pull request Aug 18, 2018

Cardano device tests trezor/trezor-core#309

Closed

matejcik suggested changes Aug 20, 2018

View reviewed changes

ddeath force-pushed the cardano-tests branch from 61f7ba8 to d622669 Compare August 21, 2018 16:35

ddeath force-pushed the cardano-tests branch from d622669 to f4164cd Compare August 21, 2018 17:03

matejcik suggested changes Aug 21, 2018

View reviewed changes

matejcik force-pushed the master branch from d83d515 to b5ef996 Compare August 21, 2018 17:11

ddeath force-pushed the cardano-tests branch from f4164cd to 5355759 Compare August 22, 2018 05:54

tsusanka mentioned this pull request Aug 24, 2018

Tezos integration trezor/trezor-core#292

Merged

ddeath force-pushed the cardano-tests branch from 5355759 to 0d2575b Compare September 4, 2018 09:34

matejcik reviewed Sep 4, 2018

View reviewed changes

ddeath added 3 commits September 5, 2018 08:09

Add cardano support to trezorctl and some tests

2832504

CR fix

CR fix

932dc3b

Add field declaration

0bffe2b

ddeath added 4 commits September 5, 2018 08:09

Fix styles

00c25ba

Fix failing test

c016ae7

Replace custom mnemonic to all all...

1fd4bc8

Fix styling

46b4327

ddeath force-pushed the cardano-tests branch from 8d9f805 to 46b4327 Compare September 5, 2018 07:27

ddeath mentioned this pull request Sep 5, 2018

Cardano scheme v2 #305

Closed

Add support for cardano derivation scheme v2

44e3d73

Update submodule to trezor official one

ddeath force-pushed the cardano-tests branch from 1640385 to 44e3d73 Compare September 5, 2018 12:51

matejcik approved these changes Sep 5, 2018

View reviewed changes

matejcik merged commit fb22b89 into trezor:master Sep 5, 2018

matejcik added this to the v0.11 milestone Nov 26, 2018



		def create_sign_transaction_message(transaction) -> messages.CardanoSignTransaction:
		if not all(transaction.get(k) for k in ('inputs', 'outputs', 'transactions')):

		REQUIRED_FIELDS_INPUT = ('path', 'prev_hash', 'prev_index', 'type')


		@expect(messages.CardanoAddress)

Add cardano support to trezorctl and some tests #300

Add cardano support to trezorctl and some tests #300

Conversation

ddeath commented Aug 18, 2018

tsusanka commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Aug 20, 2018

matejcik commented Aug 20, 2018

ddeath commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Aug 21, 2018

tsusanka commented Aug 21, 2018 • edited Loading

ddeath commented Aug 22, 2018

tsusanka commented Aug 30, 2018

ddeath commented Aug 31, 2018

matejcik commented Aug 31, 2018

tsusanka commented Sep 2, 2018 • edited Loading

ddeath commented Sep 4, 2018

matejcik commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Sep 4, 2018

ddeath commented Sep 5, 2018

ddeath commented Sep 5, 2018

tsusanka commented Sep 5, 2018

matejcik commented Sep 5, 2018

tsusanka commented Sep 5, 2018

ddeath commented Sep 5, 2018

ddeath commented Sep 5, 2018

matejcik commented Sep 5, 2018

tsusanka commented Aug 21, 2018 •

edited

Loading

tsusanka commented Sep 2, 2018 •

edited

Loading