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

Ripple support #286

Closed tsusanka closed 6 years ago

tsusanka commented 6 years ago

This introduces support for Ripple, which is coming to trezor-core.

I wasn't sure about the API, so I've done it in the similar way as Stellar's. So the signing command takes a dict as an argument consisting of the fields. I'm not sure it is ideal though, so let me know what you think.

matejcik commented 6 years ago

quick notes:

matejcik commented 6 years ago

re API: where do you expect to get transaction data? Stellar parses them out of a XDR blob and passes the protobuf objects directly, iirc. That's a reasonable thing to do IMHO. Here, I'd probably prefer that you called create_sign_tx outside the signing function, i.e., use create_sign_tx to "decode" whatever the actual input data and only then call ripple.sign_tx.

I'm probably going to modify Lisk and NEM (which take dicts) to work similarly.

tsusanka commented 6 years ago

re API: where do you expect to get transaction data?

Yeah, this is something I've wanted to discuss with you. I guess there are three options how the input data can be provided:

1) Because we support only a simple payment scenario, I was considering an option where the user provides the actual arguments directly in a similar way it is for Ethereum. So the command line api would look like this:

ripple_sign_transaction(self, address_n, destination, amount, fee, sequence, flags=None, last_sequence=None)

This has one serious disadvantage that when the other transaction types are introduced, we'll have to change this API. So maybe it's not a good idea.

2) Provide the input data as a serialized unsigned transaction. This makes sense, but we'd have to include yet another serialization format - the Ripple's binary format, because of course they're not using something common.

3) Provide the input data as a JSON-like dictionary. This will resemble the official Ripple API, see here for an example of a Payment transaction JSON. So my idea is, the user would provide this struct. Because we do not want to parse JSON, it would be a simple dict. That's basically how it is done now. But of course, JSON != python's dict, so I'm not sure if we shouldn't address that somehow as well.

tsusanka commented 6 years ago

please move ripple methods from client to ripple.py; take a look at #276, that's how it's supposed to look in the future (except you don't need the MoveTo adapters because you're creating new functions)

Ok, I'll move the methods to ripple.py. How do I register it in the client? Should I do in the client.py something like

def ripple_sign_tx(self, n, transaction):
    return ripple.sign_tx(n, transaction)

or ...?

matejcik commented 6 years ago

re "register in client" - don't do that at all .) users are expected to from trezorlib import ripple and then ripple.sign_tx(client, ...)

matejcik commented 6 years ago

re API: why not "provide data as protobuf objects"? then we can have ripple.binary_to_proto to decode the binary format, and/or riple.dict_to_proto to decode the JSON-like input

tsusanka commented 6 years ago

Discussed in person. We'll load the transaction data from a json file and then parse into protobuf. The sign_tx function will accept the protobuf messages

tsusanka commented 6 years ago

@matejcik I think this is done from my side. Let me know if I forgot something.

matejcik commented 6 years ago

last thing (function names), otherwise LGTM

tsusanka commented 6 years ago

Good idea. Done.

matejcik commented 6 years ago

and done. thank you, please delete the branch when you're done here :)