Ripple support #286

tsusanka · 2018-07-11T13:03:13Z

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 · 2018-07-11T13:11:06Z

quick notes:

please don't use import messages as proto anymore, i'm getting rid of that weirdness
please move ripple methods from client to ripple.py; take a look at preliminary: TrezorClient split #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)

matejcik · 2018-07-11T13:14:51Z

trezorlib/ripple.py

+
+
+def validate(transaction):
+    if False in (k in transaction for k in ("Fee", "Sequence", "TransactionType", "Amount", "Destination")):


if not all(k in transaction for k in (...)):

also maybe:

if not all(transaction.get(k) for k in (...)):

to check truthiness of the actual values?

You're right, so your second option it is

matejcik · 2018-07-11T13:19:39Z

trezorlib/ripple.py

+    if "Flags" in transaction:
+        msg.flags = transaction["Flags"]
+    if "LastLedgerSequence" in transaction:
+        msg.last_ledger_sequence = transaction["LastLedgerSequence"]


instead:

return messages.RippleSignTx( fee=transaction["Fee"], sequence=transaction["Sequence"], flags=transaction.get("Flags"), last_ledger_sequence=transaction.get("LastLedgerSequence"), payment=create_payment(transaction), )

(perhaps use get() for the first two as well, for consistency)

the dict.get method will return None if key is missing, so you don't need the if check.

also maybe validate(transaction) within this function?

Great, fixed

matejcik · 2018-07-11T13:21:09Z

trezorlib/ripple.py

+    msg = proto.RipplePayment()
+    msg.amount = transaction["Amount"]
+    msg.destination = transaction["Destination"]
+    return msg


i'd also put the properties in constructor (same as above), if they are just items from the dict

matejcik · 2018-07-11T13:23:26Z

trezorlib/client.py

+    @expect(proto.RippleSignedTx)
+    def ripple_sign_tx(self, n, transaction):
+        ripple.validate(transaction)
+        n = self._convert_prime(n)


no more _convert_prime please

I should use tools.parse_path(n) right?

yes, but not here. the n should already be the output of parse_path, that part is correct

matejcik · 2018-07-11T13:23:49Z

trezorlib/ripple.py

+
+import base64
+import struct
+import xdrlib


you don't seem to use xdrlib?

matejcik · 2018-07-11T13:31:33Z

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 · 2018-07-12T09:54:01Z

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:

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.

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.
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 · 2018-07-12T10:02:51Z

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 · 2018-07-12T10:41:42Z

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 · 2018-07-12T10:51:15Z

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 · 2018-07-12T11:44:31Z

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 · 2018-07-12T13:14:14Z

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

matejcik · 2018-07-12T13:53:50Z

trezorlib/ripple.py

+
+@field('address')
+@expect(messages.RippleAddress)
+def ripple_get_address(client, address_n, show_display=False):


please rename to get_address

matejcik · 2018-07-12T13:55:09Z

trezorlib/ripple.py

+
+
+@expect(messages.RippleSignedTx)
+def ripple_sign_tx(client, address_n, msg: messages.RippleSignTx):


please rename to sign_tx

matejcik · 2018-07-12T13:55:29Z

last thing (function names), otherwise LGTM

tsusanka · 2018-07-12T14:28:16Z

Good idea. Done.

matejcik · 2018-07-12T14:36:50Z

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

tsusanka requested a review from matejcik July 11, 2018 13:03

tsusanka force-pushed the tsusanka/ripple branch from 9f2ed75 to 0c39962 Compare July 11, 2018 13:04

matejcik reviewed Jul 11, 2018

View reviewed changes

tsusanka force-pushed the tsusanka/ripple branch from e55551b to 7debbea Compare July 12, 2018 10:05

matejcik reviewed Jul 12, 2018

View reviewed changes

tsusanka added 7 commits July 12, 2018 16:24

ripple: get address command including tests

20fe3b9

ripple: sign tx command including tests

8485455

vendor: trezor-common update

50439ab

ripple: code review fixes

2043b84

ripple: client functions moved to ripple.py

daf0c74

ripple: proper sign_tx command

c74ede4

ripple: ripple prefix dropped in functions

8111734

tsusanka force-pushed the tsusanka/ripple branch from 5ec537d to 8111734 Compare July 12, 2018 14:27

matejcik added the altcoin label Jul 12, 2018

matejcik added this to the next milestone Jul 12, 2018

matejcik merged commit 61e63c6 into master Jul 12, 2018

tsusanka deleted the tsusanka/ripple branch July 12, 2018 14:37

tsusanka mentioned this pull request Aug 13, 2018

Cardano support #295

Closed

tsusanka mentioned this pull request Aug 24, 2018

Tezos integration trezor/trezor-core#292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ripple support #286

Ripple support #286

tsusanka commented Jul 11, 2018

matejcik commented Jul 11, 2018

matejcik Jul 11, 2018

tsusanka Jul 12, 2018

matejcik Jul 11, 2018 •

edited

Loading

tsusanka Jul 12, 2018

matejcik Jul 11, 2018 •

edited

Loading

tsusanka Jul 12, 2018

matejcik Jul 11, 2018

tsusanka Jul 12, 2018

matejcik Jul 12, 2018

matejcik Jul 11, 2018

tsusanka Jul 12, 2018

matejcik commented Jul 11, 2018

tsusanka commented Jul 12, 2018 •

edited

Loading

tsusanka commented Jul 12, 2018 •

edited

Loading

matejcik commented Jul 12, 2018

matejcik commented Jul 12, 2018

tsusanka commented Jul 12, 2018

tsusanka commented Jul 12, 2018

matejcik Jul 12, 2018

matejcik Jul 12, 2018

matejcik commented Jul 12, 2018

tsusanka commented Jul 12, 2018

matejcik commented Jul 12, 2018



		def validate(transaction):
		if False in (k in transaction for k in ("Fee", "Sequence", "TransactionType", "Amount", "Destination")):



		@expect(messages.RippleSignedTx)
		def ripple_sign_tx(client, address_n, msg: messages.RippleSignTx):

Ripple support #286

Ripple support #286

Conversation

tsusanka commented Jul 11, 2018

matejcik commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik Jul 11, 2018 • edited Loading

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 Jul 11, 2018

tsusanka commented Jul 12, 2018 • edited Loading

tsusanka commented Jul 12, 2018 • edited Loading

matejcik commented Jul 12, 2018

matejcik commented Jul 12, 2018

tsusanka commented Jul 12, 2018

tsusanka commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Jul 12, 2018

tsusanka commented Jul 12, 2018

matejcik commented Jul 12, 2018

matejcik Jul 11, 2018 •

edited

Loading

matejcik Jul 11, 2018 •

edited

Loading

tsusanka commented Jul 12, 2018 •

edited

Loading

tsusanka commented Jul 12, 2018 •

edited

Loading