trezor / python-trezor

:snake: Don't use this repo, use the new monorepo instead:
https://github.com/trezor/trezor-firmware
GNU Lesser General Public License v3.0
201 stars 194 forks source link

add Ontology support #301

Closed backslash47 closed 6 years ago

backslash47 commented 6 years ago

Hello,

I prepared command line options for Ontology based on trezor/trezor-core#323 I also prepared tests.

Unfortunately I don't know how to automate the tests when using Scrollpage for transaction confirmation (that is used in ontology_sign_ont_id_register and ontology_sign_ont_id_add_attributes). When I run it against debug emulator I always need to swipe the screen manually.

matejcik commented 6 years ago

I'm pretty sure the code in trezorctl doesn't work - there is (currently) no way to take a protobuf message as a command line argument. You'll need some sort of conversion.

Also the choice of outputting a "payload" dict is strange. Are you trying to interoperate with some sort of tool? If yes, which?

If you're free to choose the format of your input, I'd suggest a JSON structured the same as the protobuf message; I have code mostly ready to merge that can perform the conversion. If the format is different, you'll have to do a conversion by hand.

backslash47 commented 6 years ago

Hi, sorry for the payload. I want to return two things: the signature and the transaction script (also called payload in ontology). I missed to add the script.

About the input format. Json will be fine. Could you point me to a similar function I could inspire from?

matejcik commented 6 years ago

look at ripple_sign_tx() in trezorctl, that's how your functions should look like. however, if you don't care about the input format, let's make it match the protobufs. Because the code is mostly ready and there's no point in writing it manually, just put a big TODO mark and raise a NotImplementedError instead of calling create_sign_tx_msg, and I'll replace it with the appropriate call when I merge it.

ripple echo's the output field manually; the way you do it with the dict is also fine

backslash47 commented 6 years ago

OK thanks. I will do it when I am at my computer.

matejcik commented 6 years ago

re swiping: look at test_msg_resetdevice_t2 I didn't look at your code on the core side - other currencies don't use swiping, I think? But if your code requires it, you'll probably have to handle the messages individually, as the resetdevice_t2 test does :/

backslash47 commented 6 years ago

Re:swiping : only two methods require it. Basic transfer is without it. But the two methods need to show more data than one page. I will look at the reset test then

backslash47 commented 6 years ago

I push the changes to trezorctl and add to do comment into the json -> protobuf converter methods. About the swipe that would be harder. Will try tommorow

backslash47 commented 6 years ago

Hi, I've resolved the problem with tests and fixed the method calls. So that's all. If you have anything else, please let me know.

matejcik commented 6 years ago

in all your tests you are importing TrezorTest twice, and also importing unused TREZOR_VERSION

backslash47 commented 6 years ago

Sorry for that.

I removed unused imports.

matejcik commented 6 years ago

i just merged a change I have been waiting for. You will need to do the following:

You should also be able to replace your create_* functions with calls to protobuf.dict_to_proto() (ideally remove the create_ functions completely, and use dict_to_proto in trezorctl)

matejcik commented 6 years ago

otherwise the code looks good now, so I'm ready to merge after you update the code as outlined above.

backslash47 commented 6 years ago

Thanks. Will look into it tommorow

backslash47 commented 6 years ago

All done.

matejcik commented 6 years ago

Wonderful. Merging, thank you!

tsusanka commented 6 years ago

@backslash47 just FYI I believe this swipe was redundant and made other tests (resetdevice_t2 in particular) fail, so I've removed it. https://github.com/trezor/python-trezor/commit/5fadcb6b0602c846cb0b05a36ea05cf2138212bb