[Stellar] [WIP] Add protobuf messages to support Stellar #63

zulucrypto · 2017-12-06T18:02:03Z

Adds protobuf messages used by related trezor-mcu and python-trezor PRs.

See the trezor-mcu PR for more details: trezor/trezor-mcu#259

saleemrashid · 2017-12-06T19:38:11Z

protob/messages.proto

+//////////////////////
+
+/**
+ * In: request for the public key at the specified index


Use Request: and @next

saleemrashid · 2017-12-06T19:38:27Z

protob/messages.proto

+}
+
+/**
+ * Out: public key for the given index


Use Response: and @prev

saleemrashid · 2017-12-06T19:39:39Z

protob/messages.proto

+ * In: request for the public key at the specified index
+ */
+message StellarGetPublicKey {
+	required uint32 index = 1;			// Account index (derived as m/44'/148'/index')


Use optional instead of required https://developers.google.com/protocol-buffers/docs/proto

Can you clarify why this one should be optional? I'd like to ensure that an index is always passed with this message.

saleemrashid · 2017-12-06T19:40:22Z

protob/messages.proto

+ * In: request to sign the given transaction
+ */
+message StellarSignTx {
+	required uint32 index = 1;					// Account index (derived as m/44'/148'/index')


Is there any specific reason to not use repeated uint32 address_n?

I worked with the Stellar team and the developer working on Ledger support to standardize on a specific derivation path where the only thing the user can choose is an index. To keep things consistent, I'd prefer to prevent the user from overriding this.

More details here: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0005.md

prusnak · 2017-12-07T21:08:21Z

High-level remark: I don't like all the parsing that happens inside of the TREZOR. If you look at the implementation of other coins, what we do is that the client wallet does the parsing and passes already parsed structures to the device. The device can then build the binary stream (which is much safer than parsing), signs and (optionally returns) the constructed data together with the signature.

I would be really happy if we changed the high-level logic in this fashion.

zulucrypto · 2017-12-08T00:14:55Z

@prusnak I see where you're coming from on this, and I was originally hoping to go that way, but I think you'd end up with a more complicated situation.

Re-assembling the protobuf messages into XDR would require code with the same amount of complexity, but going the other direction. For example, I'd need stellar_xdr_hash_string, stellar_xdr_hash_adddress, etc. to take the protobuf message and convert it to the bytes to be hashed.

In addition, I'd need to add XDR parsing to the client applications to build the protobuf messages. This wouldn't be too bad for javascript (since there's an official library) but I don't believe there's an official one for python so it would mean adding a third-party dependency or a lot of custom code to python-trezor.

Is there some way I could add test cases to trezor-mcu or something else I could do to make you more comfortable with all the parsing?

saleemrashid · 2017-12-08T06:45:34Z

protob/messages.proto

+}
+
+/**
+ * Request: Next operation in the transaction


This should be Response:

saleemrashid · 2017-12-08T06:45:49Z

protob/messages.proto

+}
+
+/**
+ * Response: next operation to sign


This should be Request:

saleemrashid · 2017-12-08T06:47:56Z

protob/messages.proto

+ * @next StellarPublicKey
+ */
+message StellarGetPublicKey {
+	required uint32 index = 1;			// Account index (derived as m/44'/148'/index')


Use optional instead of required .

The rationale is explained in https://developers.google.com/protocol-buffers/docs/proto#specifying-field-rules under "Required Is Forever". Furthermore, Proto3 has removed required.

saleemrashid · 2017-12-08T06:48:10Z

protob/messages.proto

+ * @next StellarSignedTx
+ */
+message StellarTxOpAck {
+  required bytes xdr = 1;           // XDR-encoded bytes starting at the offset requested in StellarTxOpRequest


Indentation

saleemrashid · 2017-12-08T06:48:19Z

protob/messages.proto

+ * @prev StellarTxOpAck
+ */
+message StellarSignedTx {
+  required bytes public_key = 1;  // public key for the private key used to sign data


Indentation

prusnak · 2017-12-08T08:17:42Z

@prusnak I see where you're coming from on this, and I was originally hoping to go that way, but I think you'd end up with a more complicated situation.

It will not be a more complicated situation for firmware, it will be much more simple in the firmware and this is more important as this is the critical code.

Re-assembling the protobuf messages into XDR would require code with the same amount of complexity, but going the other direction.

Agreed.

In addition, I'd need to add XDR parsing to the client applications to build the protobuf messages. This wouldn't be too bad for javascript (since there's an official library) but I don't believe there's an official one for python so it would mean adding a third-party dependency or a lot of custom code to python-trezor.

The logic is - if you have a wallet code that wants to interact with TREZOR, you already have all parsing primitives in the codebase. So you don't need to write it. Python-trezor is a specific example, because it is not a wallet, but I am still more OK with making python-trezor codebase more complex while making trezor-mcu codebase more simple.

Is there some way I could add test cases to trezor-mcu or something else I could do to make you more comfortable with all the parsing?

It's not about making someone comfortable, it's about keeping the high-level idea of the device consistent. Imagine if every coin wanted to add their parsing routines in the mcu-code ...

prusnak · 2017-12-08T08:19:17Z

Also, it is far easier to understand the protocol if you look into the protobuf definition rather than looking into parsing code, which creates another added value.

Would you be willing to rewrite the this PR and the other PR to use the parsing logic outside of the mcu-code?

zulucrypto · 2017-12-08T08:31:45Z

... this is more important as this is the critical code.

Definitely agreed there!

Would you be willing to rewrite the this PR and the other PR to use the parsing logic outside of the mcu-code?

Yes, I'll rewrite the code that parses the transaction header to use protobuf messages and update python-trezor. Once I have that working (hopefully within a few days) I'll push it to this branch so you can verify it's going in the direction you expect. If that all looks good, I'll continue on and create messages for the rest of the operations.

@saleemrashid - Thanks for your updated comments, I'll fix those problems as well!

prusnak · 2017-12-08T08:45:55Z

Love it!

saleemrashid · 2017-12-08T17:24:45Z

@zulucrypto No problem!

Also your @next and @prev for StellarTxOpRequest and StellarTxOpAck are a bit confused, I think.

zulucrypto · 2017-12-13T18:41:32Z

@prusnak - I've just pushed changes to all three PRs with the refactor to parse the XDR in python and send protobuf messages to the Trezor.

I think it ended up being simpler overall, so thank you for sending me in this direction. I also discovered that there's a built-in XDR parsing library in Python, so that side was easier than I expected. Since it was easier, I was able to re-implement the transaction header parsing, the payment operation parsing, and also add the "create account" operation.

Something I'd like your opinion on: right now, both the "payment" and "create account" operations share the StellarTxOpAck message since there's a lot of overlap between the fields they use. If they were two separate protobuf messages then they'd also require their own fsm_* function and I think there would be a lot of extra code that was mostly duplicated.

However, from a design perspective it seems to make more sense to map each operation to its own protobuf message instead of taking the "union" approach. Any thoughts?

prusnak · 2017-12-13T18:58:26Z

Can you elaborate more on the overlap? I'd like to see which fields are shared and which not.

prusnak · 2017-12-13T18:58:32Z

Can you elaborate more on the overlap? I'd like to see which fields are shared and which not to make a decision.

zulucrypto · 2017-12-13T19:09:40Z

Current message:

message StellarTxOpAck {
	optional uint32 type = 1;				// Operation type (see: https://github.com/stellar/stellar-core/blob/master/src/xdr/Stellar-transaction.x#L16 )

	optional bytes source_account = 2;		// (optional) 32-byte source account
	optional bytes destination_account = 3;	// 32-byte destination account
	optional uint64 amount = 4;				// amount to use for create account, payment, etc.
	optional StellarAssetType asset = 5;	// asset involved in the operation
}

Fields used for each operation:

Common to all operations

type
source_account

Create Account

destination_account
amount

Payment

destination_account
amount
asset

Path Payment (not implemented yet)

destination_account
amount
asset
send_asset (new)
send_amount (new)
path (new)

Manage Offer (not implemented yet)

asset
amount
counter_asset (new)
price (new)
offer_id (new)

Passive Offer (not implemented yet)

Same fields as above

Set Options (not implemented yet)

(9 new fields)

Change Trust (not implemented yet)

asset
amount

Allow Trust (not implemented yet)

destination_account
asset
is_authorized (new)

Account Merge (not implemented yet)

destination_account

Manage Data (not implemented yet)

name (new)
value (new)

saleemrashid · 2017-12-13T19:36:29Z

@zulucrypto You might want to take a look at how I implemented NEM support. If you look at NEMSignTx, there are some common fields (transaction, multisig and cosigning) and then some fields of which only one is permitted (transfer, provision_namespace, mosaic_creation, supply_change, aggregate_modification and important_transfer). That might be useful for removing the overlap.

zulucrypto · 2018-01-14T21:40:20Z

@prusnak I decided to implement each Stellar operation as its own message. I've just pushed updates to this branch, the trezor-mcu repo, and the python-trezor repo.

All operations are now implemented and I don't anticipate adding any more major functionality. I'll be doing some more testing here, but it should be ready for review again.

prusnak · 2018-01-14T21:44:27Z

Thank you! I have something to do this week, but hopefully I will have time to review this the week after!

saleemrashid · 2018-01-14T21:42:50Z

protob/messages.proto

+ * @next StellarPublicKey
+ */
+message StellarGetPublicKey {
+	optional uint32 index = 1;		// Account index (derived as m/44'/148'/index')


I understand why you want to enforce the BIP-32 path, but it should use address_n to be consistent with all other supported coins.

it should use address_n...

Will do, are you OK with me using the address_n convention but only having one element in the array? Or would you still like it to be an n-element array and have the client be in charge of passing in the 44'/144' parts as well as the index?

I'll work on the rest of the changes as well!

Edit: I was just talking with the developer who did the Ledger implementation and he supports paths like m/44'/144'/1'/2'/3' so I'll take this route as well. The m/44'/144' part is hardcoded by Ledger, so would you be OK with me hardcoding this part of the path on the Trezor?

We never hardcode paths in the device as we try to avoid application specific code in the device codebase. We might revisit that in the future, but please always include full path.

saleemrashid · 2018-01-14T21:44:01Z

protob/messages.proto

+ * Request: ask device to sign the given string
+ * @next StellarSignedData
+ */
+message StellarSignString {


For consistency, this should be StellarSignMessage

saleemrashid · 2018-01-14T21:44:44Z

protob/messages.proto

+ * Response: device has signed data
+ * @prev StellarSignString
+ */
+message StellarSignedData {


For consistency, this should be StellarMessageSignature

saleemrashid · 2018-01-14T21:47:20Z

protob/messages.proto

+ * Response: device has checked signature and either verified or denied it
+ * @prev StellarVerifyMessage
+ */
+message StellarMessageVerification {


For consistency, this should be removed and Success or Failure used instead.

zulucrypto · 2018-01-23T06:17:09Z

I've refactored the code to use a BIP32 path array instead of the single index.

@saleemrashid I've also addressed the protobuf message names in the trezor-common PR.

As usual, thank you both for your quick comments!

zulucrypto · 2018-02-12T16:30:28Z

@prusnak I'm working on a web frontend and having trouble finding documentation on how to use Trezor Connect with a custom firmware. Is there anything you could point me at or is there a good time I could bug you on Gitter? :)

tsusanka · 2018-03-05T13:01:57Z

protob/messages.proto

+
+	optional bytes destination_account = 2;	// 32-byte destination account
+	optional StellarAssetType asset = 3;	// asset involved in the operation
+	optional int64 amount = 4;				// amount of the given asset to pay


Should be uint64 (also everywhere else)

tsusanka · 2018-03-05T13:03:52Z

@zulucrypto hey there, I should be the one looking more into this these days. So far I've noticed just one detail, some of the proto messages contain int64 instead of uint64, there is no reason for them to be signed imho. I've commented in the code as well.

Also, in case you find the strength, do you think you could rebase all the PRs on top of current master?

zulucrypto · 2018-03-06T04:45:46Z

@tsusanka - Thanks for your review! Using int64 is intentional as that's how the Stellar protocol stores all of the currency amounts. For example: https://github.com/stellar/stellar-core/blob/3c4e356803175f6c2645e4437881cf07522df94d/src/xdr/Stellar-transaction.x#L58

I will work on rebasing the PRs on master and push them soon.

tsusanka · 2018-03-06T10:08:23Z

@zulucrypto oh, I see. So amounts can be negative?

prusnak · 2018-03-06T12:44:02Z

That's really weird they use signed ints for amounts, unless they indeed need to use negative amounts.

zulucrypto · 2018-03-06T18:14:33Z

@tsusanka - Negative amounts are not allowed (this is enforced through logic in the servers that accept transactions from clients).

The explanation I was given when I asked why the values were signed was that it makes detecting overflows easier.

tsusanka · 2018-03-07T08:50:54Z

Hm, interesting. I think we could still use uint64 just to make it consistent with other currencies

zulucrypto · 2018-03-23T00:12:22Z

@tsusanka - I've got the branches rebased here, but haven't pushed them yet. Stellar is in the process of adding a new operation that I'd like to include support for, so I plan to add it and then submit this PR for review again.

In the meantime, some questions:

I agree that using uint64 would be consistent with other currencies but I think there would be less of a chance for errors/bugs if the data type matched what the Stellar protocol uses. Are you OK if I leave them as int64?
I noticed that split_message in layout2.c is now marked as static. I was using this function in stellar.c. Which of these options would you prefer:
a. Create a new static split_message in stellar.c
b. Remove the static keyword and use the split_message defined in layout2.c
c. Other

Thanks again for looking this over!

tsusanka · 2018-03-23T11:38:48Z

@zulucrypto awesome!

Okay, leave int64 for the moment, we can get back to it when we'll do the final review
How about creating a new function in layout2.c specific for Stellar which calls the split_message function? It is done in a similar way for NEM. See layoutNEMTransferPayload for example

One extra point:

Do you think you could also add some tests? We need those before merging. All device tests are in python-trezor, you may use the ethereum signing test for example. Let me know if you need help there

prusnak · 2018-04-10T12:12:22Z

LGTM, Thanks!

matejcik · 2018-04-11T08:05:12Z

@zulucrypto re the signed ints again

is it important that the value is signed, or that the value is "protobuf int64"? If the former, it will be better to use sint64 which is a more efficient encoding on the wire.
(It will also allow us to not support int64 at all :) )

zulucrypto · 2018-04-11T23:10:25Z

Hi @matejcik - I'm mostly concerned that the data structures that are being passed to the Trezor (and hashed) match what the Stellar protocol specifies.

From what I can tell, using sint64 in the protobuf definitions won't impact the data, so I'm OK with moving to that.

I'll update things here to use sint64 and if all goes well I'll submit a trezor-common PR that changes all the int64 references to sint64.

saleemrashid suggested changes Dec 6, 2017

View reviewed changes

prusnak mentioned this pull request Dec 7, 2017

Stellar: Proposal for Stellar Lumen support #53

Closed

saleemrashid suggested changes Dec 8, 2017

View reviewed changes

saleemrashid suggested changes Jan 14, 2018

View reviewed changes

tsusanka reviewed Mar 5, 2018

View reviewed changes

zulucrypto mentioned this pull request Mar 6, 2018

[Stellar] [WIP] Add support for Stellar trezor/trezor-mcu#259

Merged

Stellar - add protobuf messages and types

5e03ae7

zulucrypto force-pushed the stellar branch from f779c53 to 5e03ae7 Compare April 6, 2018 18:36

prusnak merged commit 8ac9068 into trezor:master Apr 10, 2018

alepop mentioned this pull request Apr 14, 2018

Change stellar fields from int64 -> sint64 #96

Merged

[Stellar] [WIP] Add protobuf messages to support Stellar #63

[Stellar] [WIP] Add protobuf messages to support Stellar #63

Conversation

zulucrypto commented Dec 6, 2017

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

prusnak commented Dec 7, 2017 • edited Loading

zulucrypto commented Dec 8, 2017

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

prusnak commented Dec 8, 2017

prusnak commented Dec 8, 2017

zulucrypto commented Dec 8, 2017

prusnak commented Dec 8, 2017

saleemrashid commented Dec 8, 2017

zulucrypto commented Dec 13, 2017

prusnak commented Dec 13, 2017

prusnak commented Dec 13, 2017

zulucrypto commented Dec 13, 2017

saleemrashid commented Dec 13, 2017

zulucrypto commented Jan 14, 2018

prusnak commented Jan 14, 2018 via email

Choose a reason for hiding this comment

zulucrypto Jan 22, 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

zulucrypto commented Jan 23, 2018

zulucrypto commented Feb 12, 2018

Choose a reason for hiding this comment

tsusanka commented Mar 5, 2018

zulucrypto commented Mar 6, 2018

tsusanka commented Mar 6, 2018 • edited Loading

prusnak commented Mar 6, 2018

zulucrypto commented Mar 6, 2018

tsusanka commented Mar 7, 2018

zulucrypto commented Mar 23, 2018

tsusanka commented Mar 23, 2018 • edited Loading

prusnak commented Apr 10, 2018

matejcik commented Apr 11, 2018

zulucrypto commented Apr 11, 2018

prusnak commented Dec 7, 2017 •

edited

Loading

zulucrypto Jan 22, 2018 •

edited

Loading

tsusanka commented Mar 6, 2018 •

edited

Loading

tsusanka commented Mar 23, 2018 •

edited

Loading