New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tezos integration #292

Merged
merged 7 commits into from Sep 13, 2018

Conversation

Projects
None yet
5 participants
@Adman
Contributor

Adman commented Jul 27, 2018

Updated fields of messages trezor/trezor-common#170

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak
Member

prusnak commented Jul 27, 2018

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Jul 27, 2018

Contributor

@prusnak fixed

Contributor

Adman commented Jul 27, 2018

@prusnak fixed

@prusnak prusnak added this to the v2.0.8 milestone Jul 30, 2018

@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Aug 24, 2018

Contributor

@Adman could you add python-trezor support and some device tests along the way please? We have two types of tests:

  • unit tests, directly included in trezor-core/trezor-mcu repositories, which you've added so that's great
  • device tests (aka integration tests), which are part of the python-trezor repository and are "trezor model independent". In other words those are tests that can be run against both Trezor One and Trezor T. They actually send protobuf messages to the device and expect some other protobuf messages with results

Have a look on the Cardano's or Ripple's PR for example for some inspiration.

Contributor

tsusanka commented Aug 24, 2018

@Adman could you add python-trezor support and some device tests along the way please? We have two types of tests:

  • unit tests, directly included in trezor-core/trezor-mcu repositories, which you've added so that's great
  • device tests (aka integration tests), which are part of the python-trezor repository and are "trezor model independent". In other words those are tests that can be run against both Trezor One and Trezor T. They actually send protobuf messages to the device and expect some other protobuf messages with results

Have a look on the Cardano's or Ripple's PR for example for some inspiration.

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Aug 27, 2018

Contributor

@tsusanka ok, I'll add python-trezor support and make a PR.

Contributor

Adman commented Aug 27, 2018

@tsusanka ok, I'll add python-trezor support and make a PR.

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Aug 28, 2018

Contributor

@tsusanka Can you give me a hint how to run device_tests in python-trezor?
When running python -m pytest -v from python-trezor/trezorlib/tests/device_tests folder it fails
StopIteration ERROR: could not load python-trezor/trezorlib/tests/device_tests/conftest.py.

unit_tests work properly.

Contributor

Adman commented Aug 28, 2018

@tsusanka Can you give me a hint how to run device_tests in python-trezor?
When running python -m pytest -v from python-trezor/trezorlib/tests/device_tests folder it fails
StopIteration ERROR: could not load python-trezor/trezorlib/tests/device_tests/conftest.py.

unit_tests work properly.

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Aug 28, 2018

Member
Member

prusnak commented Aug 28, 2018

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Aug 31, 2018

Contributor

@tsusanka @prusnak Here's the python-trezor implementation: trezor/python-trezor#302

Contributor

Adman commented Aug 31, 2018

@tsusanka @prusnak Here's the python-trezor implementation: trezor/python-trezor#302

@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 3, 2018

Contributor

@tsusanka @prusnak Here's the python-trezor implementation: trezor/python-trezor#302

Awesome! We are currently in the process of finalizing our next release. When that's done we'll review your PR completely. Thanks

Contributor

tsusanka commented Sep 3, 2018

@tsusanka @prusnak Here's the python-trezor implementation: trezor/python-trezor#302

Awesome! We are currently in the process of finalizing our next release. When that's done we'll review your PR completely. Thanks

@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 5, 2018

Contributor

@Adman What do you think about supporting just one curve instead of all three? Supporting all three brings unnecessary complexity. We can add the other later but I believe in the first version of Tezos support we should use just one. You can choose (maybe do a bit of research what is used the most?) but I suggest secp256k or ed25519. What do you think?

Contributor

tsusanka commented Sep 5, 2018

@Adman What do you think about supporting just one curve instead of all three? Supporting all three brings unnecessary complexity. We can add the other later but I believe in the first version of Tezos support we should use just one. You can choose (maybe do a bit of research what is used the most?) but I suggest secp256k or ed25519. What do you think?

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Sep 5, 2018

Member

As per agreement on Gitter, we'll support only Ed25519 curve = tz1. Relevant commit to trezor-common: trezor/trezor-common@b6f59d3

Please update this PR (and PR to python-trezor) to use only tz1/Ed25519 curve. Thank you!

Member

prusnak commented Sep 5, 2018

As per agreement on Gitter, we'll support only Ed25519 curve = tz1. Relevant commit to trezor-common: trezor/trezor-common@b6f59d3

Please update this PR (and PR to python-trezor) to use only tz1/Ed25519 curve. Thank you!

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 7, 2018

Contributor
Contributor

Adman commented Sep 7, 2018

@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 7, 2018

Contributor

@Adman please do a rebase (let me know if you need help), the tests should be passing then

Contributor

tsusanka commented Sep 7, 2018

@Adman please do a rebase (let me know if you need help), the tests should be passing then

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 7, 2018

Contributor

@tsusanka rebased, but master is failing because of cardano's get_public_key and address tests https://travis-ci.org/trezor/trezor-core/jobs/425716279#L1550

Contributor

Adman commented Sep 7, 2018

@tsusanka rebased, but master is failing because of cardano's get_public_key and address tests https://travis-ci.org/trezor/trezor-core/jobs/425716279#L1550

@prusnak

This comment has been minimized.

Show comment
Hide comment
@prusnak

prusnak Sep 7, 2018

Member

You did not rebase the commits - you did merge. Rebase is being done with git rebase master. https://git-scm.com/docs/git-rebase

Be careful, this can be quite dangerous, but if there are no conflicts, it should be pretty straightforward.

Member

prusnak commented Sep 7, 2018

You did not rebase the commits - you did merge. Rebase is being done with git rebase master. https://git-scm.com/docs/git-rebase

Be careful, this can be quite dangerous, but if there are no conflicts, it should be pretty straightforward.

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 7, 2018

Contributor

@prusnak There were some conflicts, but I hope everything's fine now.

Contributor

Adman commented Sep 7, 2018

@prusnak There were some conflicts, but I hope everything's fine now.

@szymonlesisz szymonlesisz referenced this pull request Sep 7, 2018

Merged

Tezos integration #208

@tsusanka tsusanka self-requested a review Sep 10, 2018

@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 10, 2018

Contributor

Now that python-trezor is merged, please do a rebase and set run_xfail = tezos in setup.cfg. That way we'll see the tests are passing. I'm working on a review, I'll publish it tonight/tomorrow.

Contributor

tsusanka commented Sep 10, 2018

Now that python-trezor is merged, please do a rebase and set run_xfail = tezos in setup.cfg. That way we'll see the tests are passing. I'm working on a review, I'll publish it tonight/tomorrow.

Adman added some commits Jul 27, 2018

tezos: added tezos cryptocurrency
Signed-off-by: Adrian Matejov <adrian.matejov@simplestaking.com>
tezos: display_address -> show
Signed-off-by: Adrian Matejov <adrian.matejov@simplestaking.com>
tezos: removed duplicated code
Signed-off-by: Adrian Matejov <adrian.matejov@simplestaking.com>
tezos: supporting only ed25519 curve
Signed-off-by: Adrian Matejov <adrian.matejov@simplestaking.com>
tezos: run_xfail set
Signed-off-by: Adrian Matejov <adrian.matejov@simplestaking.com>
@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 10, 2018

Contributor

@tsusanka Ok, I rebased and set run_xfail = tezos

Contributor

Adman commented Sep 10, 2018

@tsusanka Ok, I rebased and set run_xfail = tezos

@tsusanka

tsusanka requested changes Sep 11, 2018 edited

Thanks for you PR, consider this as a preliminary review, it's mostly nit-picks. I've requested some changes and have a general question:

Do you plan to use the GetPublicKey functions in particular on your front-end side? Or did you implement them just to fit the others? Since ed25519 does not support non-hardened derivation, therefore no xpub functionality, I am not sure what the use case is. If we don't have any in mind, let's remove it.

Show outdated Hide outdated src/apps/tezos/get_address.py Outdated
Show outdated Hide outdated src/apps/tezos/get_address.py Outdated
Show outdated Hide outdated src/apps/tezos/get_public_key.py Outdated
def split_address(address):
return chunks(address, 18)

This comment has been minimized.

@tsusanka

tsusanka Sep 11, 2018

Contributor

Let's use common.layout.split_address. It has size=17, but that doesn't matter that much or will be refactored together later

@tsusanka

tsusanka Sep 11, 2018

Contributor

Let's use common.layout.split_address. It has size=17, but that doesn't matter that much or will be refactored together later

This comment has been minimized.

@Adman

Adman Sep 11, 2018

Contributor

If I use 17, the address in transaction won't fit on display.

@Adman

Adman Sep 11, 2018

Contributor

If I use 17, the address in transaction won't fit on display.

This comment has been minimized.

@tsusanka

tsusanka Sep 11, 2018

Contributor

Ok, keep it.

@tsusanka

tsusanka Sep 11, 2018

Contributor

Ok, keep it.

Show outdated Hide outdated src/apps/tezos/sign_tx.py Outdated
Show outdated Hide outdated src/apps/tezos/sign_tx.py Outdated
Show outdated Hide outdated src/apps/tezos/sign_tx.py Outdated
Show outdated Hide outdated src/apps/tezos/sign_tx.py Outdated
@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 11, 2018

Contributor

@tsusanka Requested changes applied.

It would be cool if the common split_address function had optional parameter size, because using size=17 the address won't fit on display. I kept the function there for now.

GetPublicKey - @jurajselep will post a comment about it here.

Contributor

Adman commented Sep 11, 2018

@tsusanka Requested changes applied.

It would be cool if the common split_address function had optional parameter size, because using size=17 the address won't fit on display. I kept the function there for now.

GetPublicKey - @jurajselep will post a comment about it here.

@jurajselep

This comment has been minimized.

Show comment
Hide comment
@jurajselep

jurajselep Sep 11, 2018

When we inject transaction, origination or delegation into Tezos node for first time, we need to "reveal" public_key.
http://tezos.gitlab.io/betanet/api/p2p.html?highlight=reveal

We are using GetPublicKey on front-end side for operation preapply and forge .

http://tezos.gitlab.io/betanet/api/rpc.html?highlight=reveal#post-block-id-helpers-forge-operations
http://tezos.gitlab.io/betanet/api/rpc.html?highlight=reveal#post-block-id-helpers-preapply-operations

{ 
 "kind": "reveal",
 "source": $contract_id,
 "fee": $mutez,
 "counter": $positive_bignum,
 "gas_limit": $positive_bignum,
 "storage_limit": $positive_bignum,
 "public_key": $Signature.Public_key 
}

jurajselep commented Sep 11, 2018

When we inject transaction, origination or delegation into Tezos node for first time, we need to "reveal" public_key.
http://tezos.gitlab.io/betanet/api/p2p.html?highlight=reveal

We are using GetPublicKey on front-end side for operation preapply and forge .

http://tezos.gitlab.io/betanet/api/rpc.html?highlight=reveal#post-block-id-helpers-forge-operations
http://tezos.gitlab.io/betanet/api/rpc.html?highlight=reveal#post-block-id-helpers-preapply-operations

{ 
 "kind": "reveal",
 "source": $contract_id,
 "fee": $mutez,
 "counter": $positive_bignum,
 "gas_limit": $positive_bignum,
 "storage_limit": $positive_bignum,
 "public_key": $Signature.Public_key 
}
Show outdated Hide outdated src/apps/tezos/get_address.py Outdated
@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 11, 2018

Contributor

@jurajselep thanks for the explanation, that's fine by me.

@Adman please note, that the tests are failing currently becuase of style

Contributor

tsusanka commented Sep 11, 2018

@jurajselep thanks for the explanation, that's fine by me.

@Adman please note, that the tests are failing currently becuase of style

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 11, 2018

Contributor

@tsusanka Tests should be passing, but travis build failed with message The command "curl -LO "https://github.com/google/protobuf/releases/download/v${PROTOBUF_VERSION}/protoc-${PROTOBUF_VERSION}-linux-x86_64.zip"" failed and exited with 56.
I assume it's problem on travis side.

Contributor

Adman commented Sep 11, 2018

@tsusanka Tests should be passing, but travis build failed with message The command "curl -LO "https://github.com/google/protobuf/releases/download/v${PROTOBUF_VERSION}/protoc-${PROTOBUF_VERSION}-linux-x86_64.zip"" failed and exited with 56.
I assume it's problem on travis side.

tezos: fixed nitpicks
Signed-off-by: Adrian Matejov <adrian.matejov@simplestaking.com>
@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 11, 2018

Contributor

@tsusanka Ok, I restarted the build and it is passing. If you have further comments on this PR I will fix them.

Contributor

Adman commented Sep 11, 2018

@tsusanka Ok, I restarted the build and it is passing. If you have further comments on this PR I will fix them.

@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 11, 2018

Contributor

Great. Looks good to me. I'd like @jpochyla to have a final look later, but this is on a good way to be merged. Please be patient, he's busy with other things.

@jpochyla the only thing where I'm not sure is whether _encode_zarith could be simplified or moved to writers with a better name.

tACK dc6e405

Contributor

tsusanka commented Sep 11, 2018

Great. Looks good to me. I'd like @jpochyla to have a final look later, but this is on a good way to be merged. Please be patient, he's busy with other things.

@jpochyla the only thing where I'm not sure is whether _encode_zarith could be simplified or moved to writers with a better name.

tACK dc6e405

@tsusanka tsusanka closed this Sep 11, 2018

@tsusanka tsusanka reopened this Sep 11, 2018

@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 11, 2018

Contributor

(grr, the closing was of course unintentional)

Contributor

tsusanka commented Sep 11, 2018

(grr, the closing was of course unintentional)

@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 11, 2018

Contributor

Maybe one more thing. Could you add a brief README? Have a look on Ripple and feel free to omit the maintainer/reviewer fields. It doesn't have to be verbose, but some links to documentation/explorer etc. would be nice.

Contributor

tsusanka commented Sep 11, 2018

Maybe one more thing. Could you add a brief README? Have a look on Ripple and feel free to omit the maintainer/reviewer fields. It doesn't have to be verbose, but some links to documentation/explorer etc. would be nice.

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 11, 2018

Contributor

@tsusanka Readme added.

Contributor

Adman commented Sep 11, 2018

@tsusanka Readme added.

tezos: added Readme
Signed-off-by: Adrian Matejov <adrian.matejov@simplestaking.com>

@tsusanka tsusanka merged commit 7918e1f into trezor:master Sep 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tsusanka

This comment has been minimized.

Show comment
Hide comment
@tsusanka

tsusanka Sep 13, 2018

Contributor

Thank you!

Contributor

tsusanka commented Sep 13, 2018

Thank you!

@Adman

This comment has been minimized.

Show comment
Hide comment
@Adman

Adman Sep 13, 2018

Contributor

Thanks all for the collaboration!

Contributor

Adman commented Sep 13, 2018

Thanks all for the collaboration!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment