Skip to content
This repository has been archived by the owner on Jun 5, 2019. It is now read-only.

added Tezos support #302

Merged
merged 12 commits into from
Sep 10, 2018
Merged

added Tezos support #302

merged 12 commits into from
Sep 10, 2018

Conversation

* also added device tests for tezos

Signed-off-by: Adrian Matejov <[email protected]>
trezorctl Outdated
#
@cli.command(help='Get Tezos address for specified path.')
@click.option('-n', '--address', required=True, help="BIP-32 path, e.g. m/44'/1729'/0'/0'/0'")
@click.option('-c', '--curve', default=0, type=click.IntRange(0, 2), help='Curve to use (0 = ed25519, 1 = secp256k1, 2 = p256)')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use a ChoiceType for curve, see Bitcoin's get_address and CHOICE_INPUT_SCRIPT_TYPE

@pytest.mark.skip_t1
class TestMsgTezosSignTx(TrezorTest):

def setup_method(self, method):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of this method, make a constant TEZOS_PATH = parse_path(...) at top level and use it in tests


path = parse_path("m/44'/1729'/0'/0'/0'")

address = get_address(self.client, path, 0, show_display=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use your CurveType enums



@field('address')
@expect(messages.TezosAddress)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will need to update this to @expect(messages.TezosAddress, field="address")



@expect(messages.TezosSignedTx)
def sign_tx(client, msg: messages.TezosSignTx):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function should take address_n as argument

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and probably curve too, if that's a property of the derived address)

return client.call(msg)


def create_sign_tx_msg(address_n, curve, transaction) -> messages.TezosSignTx:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might be able to replace most of this function with protobuf.dict_to_proto


path = parse_path("m/44'/1729'/0'/0'/0'")

pk = get_public_key(self.client, path, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use your CurveType enums instead of constants like 0, 1, 2

@matejcik
Copy link
Contributor

matejcik commented Sep 4, 2018

I reviewed your code, please see and make changes where appropriate.

General notes:

  • you should use CurveType constants in most places - maybe not in the dicts that are inputs to create_sign_tx_msg, because these might come from outside? But definitely in arguments to calls.
  • please mark all your tests as @pytest.mark.xfail, so that we can merge this before trezor-core code is ready

I also did a big change to master, so now you get merge conflicts. Resolution instructions are as follows:

  • fix trezorctl. that should be as simple as copy-pasting your code pre-merge to the appropriate place post-merge
  • change @field-@expect decorators to @expect(..., field=...)
  • run make style. this requires Python 3.6

You should also be able to leverage protobuf.dict_to_proto instead of the explicit create_sign_tx_msg function.

Adrian Matejov added 2 commits September 5, 2018 09:12
@Adman
Copy link
Contributor Author

Adman commented Sep 5, 2018

@matejcik Everything should be fixed

def sign_tx(client, address_n, curve, operations):
operations["address_n"] = address_n
operations["curve"] = curve
msg = dict_to_proto(messages.TezosSignTx, operations)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead:

def sign_tx(client, address_n, curve, transaction: messages.TezosSignTx):
	transaction.address_n = address_n
    transaction.curve = curve
    return client.call(transaction)

trezorctl Outdated
def tezos_sign_tx(connect, address, curve, file):
client = connect()
address_n = tools.parse_path(address)
return tezos.sign_tx(client, address_n, curve, json.load(file))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not pass raw dicts to sign_tx, instead move the dict_to_proto call here.


import pytest

from trezorlib import messages as proto
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use messages directly instead of messages as proto

@matejcik
Copy link
Contributor

matejcik commented Sep 5, 2018

thank you. Looks mostly good now, a couple minor points. I'll also wait for changes in the used paths (or not), as discussed on Gitter.

Signed-off-by: Adrian Matejov <[email protected]>
@Adman
Copy link
Contributor Author

Adman commented Sep 5, 2018

@matejcik I will rework this to support only one curve ed25519.

Adrian Matejov added 4 commits September 6, 2018 14:01
Adrian Matejov added 3 commits September 7, 2018 14:21
@matejcik
Copy link
Contributor

Looks good.
Please add @pytest.mark.skip_t1 to all tests, we won't be doing Tezos on T1 codebase.
I'm ready to merge after that change.

Signed-off-by: Adrian Matejov <[email protected]>
@Adman
Copy link
Contributor Author

Adman commented Sep 10, 2018

Added

@matejcik matejcik merged commit 15df848 into trezor:master Sep 10, 2018
@matejcik
Copy link
Contributor

Thanks!

@matejcik matejcik added this to the v0.11 milestone Nov 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants