Add Binance Chain - Githubissues

notatestuser commented 5 years ago

This adds Binance Chain to defs/misc/misc.json.

Binance Chain is a work-in-progress and access is limited to select partners for the time being. We will forward more information to the Trezor team privately to support this request.

What is the approximate scale? Have a look on the other apps in trezor-core and try to compare your new app with one of those present in the terms of scale.

We will need json parsing, sorting, validation and display. bech32 address generation a nice to have. Transaction signatures are created from deterministically generated json strings.

Do we need any new cryptographic primitives to support this coin?

We use ECDSA (secp256k1) key pairs and bech32 encoding of addresses.

Is there (or will you develop) a frontend wallet that will support Trezor with this coin?

Yes.

For reference, here are examples of the canonical json blobs we expect to be produced and signed by the device:

{"account_number":"1","chain_id":"Binance-Chain-Test","data":null,"memo":"MEMO","msgs":[{"inputs":[{"address":"tbnb1hlly02l6ahjsgxw9wlcswnlwdhg4xhx3f309d9","coins":[{"amount":10000000000,"denom":"BNB"}]}],"outputs":[{"address":"tbnb1hlly02l6ahjsgxw9wlcswnlwdhg4xhx3f309d9","coins":[{"amount":10000000000,"denom":"BNB"}]}]}],"sequence":"2","source":"1"}

{"account_number":"1","chain_id":"Binance-Chain-Test","data":null,"memo":"MEMO","msgs":[{"id":"B71E119324558ABA3AE3F5BC854F1225132465A0-0","ordertype":2,"price":100000000,"quantity":100000000,"sender":"tbnb1hlly02l6ahjsgxw9wlcswnlwdhg4xhx3f309d9","side":1,"symbol":"BTC-0AB_BNB","timeinforce":1}],"sequence":"2","source":"1"}

{"account_number":"1","chain_id":"Binance-Chain-Test","data":null,"memo":"MEMO","msgs":[{"refid":"B71E119324558ABA3AE3F5BC854F1225132465A0-0","sender":"tbnb1hlly02l6ahjsgxw9wlcswnlwdhg4xhx3f309d9","symbol":"BTC-0AB_BNB"}],"sequence":"2","source":"1"}

notatestuser commented 5 years ago

The build seems to be failing because BNB is already an ERC20 token. How can we resolve this?

prusnak commented 5 years ago

We can remove the BNB token from Trezor if you want.

notatestuser commented 5 years ago

Probably not viable as there are users with balances, and they will need access to the ERC20 tokens to swap. We could give the Binance Chain token a symbol like BNBC if this would not cause a problem in the implementation (the native token symbol is BNB on the new chain).

matejcik commented 5 years ago

We will need json parsing, sorting, validation and display. bech32 address generation a nice to have. Transaction signatures are created from deterministically generated json strings.

Do you actually need all the JSON handling? ISTM it would be preferable to convert JSON to protobuf on the host and only reconstruct it when generating the signature, given that you say that the resulting string is deterministic.

matejcik commented 5 years ago

Probably not as there are users with balances, and they will need access to the ERC20 tokens to swap. We could give the Binance Chain token a symbol like BNBC if this would not cause a problem in the implementation (the native token symbol is BNB on the new chain).

Sure, renaming the token is not a problem. Please make a PR to https://github.com/ethereum-lists/tokens with your data and reference it here

matejcik commented 5 years ago

What sort of operations does your network have, besides the basic show address / sign transaction? Depending on the scale, we might only want to support a subset.

notatestuser commented 5 years ago

Do you actually need all the JSON handling? ISTM it would be preferable to convert JSON to protobuf on the host and only reconstruct it when generating the signature, given that you say that the resulting string is deterministic.

Sure, but I would guess that might be the same or more effort to do that interim conversion to/from protobuf, unless there is a technical limitation preventing the transfer of a long json string. What if we just have the json in a variable length buffer?

Sure, renaming the token is not a problem. Please make a PR to https://github.com/ethereum-lists/tokens with your data and reference it here

Ah, I meant renaming the new chain token might be an option for Trezor

What sort of operations does your network have, besides the basic show address / sign transaction? Depending on the scale, we might only want to support a subset.

For the device, that is enough. For wallets there's multi-token support and token issuance as well as DEX features like placing and cancelling orders, among some others. I think we'd only want to support multi-token transfers on the Trezor wallet though. Your thoughts are welcome on this

matejcik commented 5 years ago

Do you actually need all the JSON handling? ISTM it would be preferable to convert JSON to protobuf on the host and only reconstruct it when generating the signature, given that you say that the resulting string is deterministic.

Sure, but I would guess that might be the same or more effort to do that interim conversion to/from protobuf, unless there is a technical limitation preventing the transfer of a long json string. What if we just have the json in a variable length string?

The limitation is not technical but conceptual. There is no JSON handling in Trezor currently. Adding it would make the code bigger and add a huge attack surface for basically no benefit. Using protobuf you can forgo a lot of data validation because the format is fixed, it allows you to "pre-chew" the data for display, etc. Rebuilding the JSON for signing, OTOH, is little more than string interpolation, and even if we make a mistake, the worst that can happen is an invalid signature.

Also there's existing code for converting JSON to a matching protobuf structure. So it mostly depends on the complexity of your transactions.

Ah, I meant renaming the new chain token might be an option for Trezor

I probably misunderstood your comment about the token symbols. Will there be any "BNBC currency"? Is that something like a gas-currency? Your main token on the chain will be marked as "BNB", right? If so, you'll have to rename the Ethereum ERC20 token symbol anyway.

For the device, that is enough. For wallets there's multi-token support and token issuance as well as DEX features like placing and cancelling orders, among some others. I think we'd only want to support multi-token transfers on the Trezor wallet though. Your thoughts are welcome on this

My question was more about the complexity of transactions to be signed. The basic operation is "send coins to address ". I'm not sure what a "multi-token transfer" is, but if it is something like "send multiple different amounts of different tokens to an address", then that would be another operation. We would probably avoid the DEX features, but that depends on details.

Also it might be worth pointing out: it's extremely unlikely that we would support signing "generic operations". Trezor device must be able to decode, understand and properly display (for user confirmation) every operation that it signs. This is why using JSON doesn't make much sense. The device won't allow any flexibility beyond what's explicitly coded for, using a loose format like JSON only complicates enforcing this.

notatestuser commented 5 years ago

The limitation is not technical but conceptual. There is no JSON handling in Trezor currently. Adding it would make the code bigger and add a huge attack surface for basically no benefit. Using protobuf you can forgo a lot of data validation because the format is fixed, it allows you to "pre-chew" the data for display, etc.

Rebuilding the JSON for signing, OTOH, is little more than string interpolation, and even if we make a mistake, the worst that can happen is an invalid signature.

Also there's existing code for converting JSON to a matching protobuf structure. So it mostly depends on the complexity of your transactions.

Right, I can see the benefit in not having to introduce any new parsing logic on your side. There would be some extra cost in maintaining this solution for us (keeping the protobuf structs up to date with any changes) but I think that it is something we could handle.

Could you point me to the tool to convert the json to protobuf structures?

I probably misunderstood your comment about the token symbols. Will there be any "BNBC currency"? Is that something like a gas-currency? Your main token on the chain will be marked as "BNB", right? If so, you'll have to rename the Ethereum ERC20 token symbol anyway.

We do not plan to rename the ERC20 token for the sake of being compatible with Trezor. Could we look for an alternative solution to allow for a BNB ERC20 token and a BNB native token on Binance Chain?

My question was more about the complexity of transactions to be signed. The basic operation is "send coins to address ". I'm not sure what a "multi-token transfer" is, but if it is something like "send multiple different amounts of different tokens to an address", then that would be another operation.

Yes, that's right

We would probably avoid the DEX features, but that depends on details.

We have an unlock step for hardware wallets on our DEX UI. Ideally we would support all "transaction types" (of which there are ~8, with more coming).

darren-liu commented 5 years ago

We would probably avoid the DEX features, but that depends on details.

"DEX features" are just transaction types. for the normal use case, there are just 2 more types of transactions, NewOrder, and CancelOrder, in addition to Send. We can leave Multi-Send for now, which is more likely used by centralized exchange hot wallets. How we interact with other hardware wallets is we pass different transactions in JSON and the wallet only needs to show it and sign it. If we cannot use JSON, we can pass you in the different protobuf structures.

matejcik commented 5 years ago

Could you point me to the tool to convert the json to protobuf structures?

in Python, it's trezorlib.protobuf.dict_to_proto and trezorlib.protobuf.to_dict.

in Javascript, protobuf.js does this natively.

but you would be using Trezor Connect / trezor-link anyway though, no?

We do not plan to rename the ERC20 token for the sake of being compatible with Trezor. Could we look for an alternative solution to allow for a BNB ERC20 token and a BNB native token on Binance Chain?

It is currently not allowed for two different currencies to use the same shortcut, because the shortcut is the only thing that lets the user know what they're signing. (This is not great and we're looking into other ways to confirm the currency, but it's what it is now.) The usual solution is to add something to the shortcut, i.e. BNB (ERC20) or BNB (old).

"DEX features" are just transaction types. for the normal use case, there are just 2 more types of transactions, NewOrder, and CancelOrder, in addition to Send. We can leave Multi-Send for now, which is more likely used by centralized exchange hot wallets. How we interact with other hardware wallets is we pass different transactions in JSON and the wallet only needs to show it and sign it. If we cannot use JSON, we can pass you in the different protobuf structures.

4 transaction types sound reasonable for a first pass

notatestuser commented 5 years ago

The usual solution is to add something to the shortcut, i.e. BNB (ERC20) or BNB (old).

This seems reasonable. Could we make that change ASAP with no loss of existing functionality?

matejcik commented 5 years ago

The usual solution is to add something to the shortcut, i.e. BNB (ERC20) or BNB (old).

This seems reasonable. Could we make that change ASAP with no loss of existing functionality?

yes.

notatestuser commented 5 years ago

To clarify, we would have to append (ERC20) to the token's symbol in ethereum-lists/tokens?

matejcik commented 5 years ago

that is correct, yes

notatestuser commented 5 years ago

@ligi has suggested adding the suffix in a post-processing step in Trezor's toolchain scripts instead of merging a manual edit to the token definition (ethereum-lists/tokens#182). What are your thoughts on this @matejcik?

notatestuser commented 5 years ago

Protobufs have been added to this branch. @prusnak

prusnak commented 5 years ago

@notatestuser Please don't use int32/int64. If these values are never supposed to be less then zero, use uint32/uint64. If they can become less than zero, use sint32/sint64.

notatestuser commented 5 years ago

We can go with sint64/uint64. This just reflects what we use in our go structs.

Aside: We use int64 not uint64 as a design choice: checking for/erroring on <0 is much easier than underflowing and getting a huge value that might mess things up big time. So, tl;dr, we use int64 but the values cannot be negative.

pbuf int64 seemed appropriate here because ranges are shared and we are not "likely" to ever have negative values

prusnak commented 5 years ago

1) What about the other int32 values you haven't changed? Are these supposed to be sint32 or uint32? 2) Why did you change string json to bytes json? Do you expect to have binary data encoded in this field? I don't think that JSON should contain non-UTF8 characters.

matejcik commented 5 years ago

our code does not support int32/64 types by design, so you need to use sint if negative values are allowed or uint if they aren't. Encoding-wise sint is basically identical to int on positive values anyway.

(re over/underflow checking, you could as well check that unsigned values have the high bit set (or if they're smaller than 2**63), which is identical to checking if a signed value is less than zero, if i'm not mistaken)

JSON should definitely be a string

what is the data field?

why is the sequence field a string?

I'd recommend having a BinanceTxCommon struct for account, chain_id, data, etc., and share this struct between the different kinds of messages

notatestuser commented 5 years ago

Thanks for the suggestions.

what is the data field?

It's for exchanges/gateways to attach implementation-specific transaction tracking data to a tx. Since it is binary data, we do not want it being rendered as a "memo". It probably won't be used with trezor, so should always be nil.

prusnak commented 5 years ago

It probably won't be used with trezor

Remove it from the protobuf if it is not going to be used by Trezor.

notatestuser commented 5 years ago

For reference, I have added examples of the canonical json blobs we expect to be produced and signed by the device in the OP.

matejcik commented 5 years ago

One more thing occurs to me. It seems that your messages can contain arbitrary number of sub-operations, so the size of a request packet is potentially unlimited, which is not great. BinanceTransferTx is especially unpleasant in this regard, as you can have (1) multiple transfers, each of which has (2) multiple inouts, each of which in turn can have (3) multiple coins.

Trezor can't stream this message directly from USB. So if we want to support unlimited number of operations, we would have to convert this to a model where Trezor asks the host to send the next chunk. With BinanceOrderTx, we could just send the number of orders in the intro message, and then exchange something like BinanceTxRequest/BinanceTxResponse with one order per exchange. But multiple levels of nesting make this rather more complicated.

(With this, we would also want the BinanceTxRequest to send out a chunk of JSON with every message, so that we don't need to hold the whole thing in memory.)

An alternative is to limit the nesting, e.g., allow only one coin per address and only one transfer per BinanceTransferTx -- speaking of which, is there any value in allowing multiple transfers, each with multiple inouts, as opposed to just stuff all the inputs and outputs into a single transfer?

We could leave this be and assume that the transactions will never be too big. But that's going to lead to a nasty surprise when one day you want to send a big tx and you get a MemoryError :/

Another consideration is UI/UX though. Confirmation dialogs with multiple assets per multiple inputs and multiple outputs are going to be seriously confusing. This leads me to think that a better solution would be some explicit limits, for example, support multiple assets but just one input and one output.

What are your thoughts?

notatestuser commented 5 years ago

For these types there’s just one msg per transaction. So should not be an issue.

Get Outlook for iOShttps://aka.ms/o0ukef

From: matejcik notifications@github.com Sent: Wednesday, February 6, 2019 7:19 pm To: trezor/trezor-common Cc: Luke Plaster; Mention Subject: Re: [trezor/trezor-common] Add Binance Chain (#264)

One more thing occurs to me. It seems that your messages can contain arbitrary number of sub-operations, so the size of a request packet is potentially unlimited, which is not great. BinanceTransferTx is especially unpleasant in this regard, as you can have (1) multiple transfers, each of which has (2) multiple inouts, each of which in turn can have (3) multiple coins.

Trezor can't stream this message directly from USB. So if we want to support unlimited number of operations, we would have to convert this to a model where Trezor asks the host to send the next chunk. With BinanceOrderTx, we could just send the number of orders in the intro message, and then exchange something like BinanceTxRequest/BinanceTxResponse with one order per exchange. But multiple levels of nesting make this rather more complicated.

(With this, we would also want the BinanceTxRequest to send out a chunk of JSON with every message, so that we don't need to hold the whole thing in memory.)

An alternative is to limit the nesting, e.g., allow only one coin per address and only one transfer per BinanceTransferTx -- speaking of which, is there any value in allowing multiple transfers, each with multiple inouts, as opposed to just stuff all the inputs and outputs into a single transfer?

We could leave this be and assume that the transactions will never be too big. But that's going to lead to a nasty surprise when one day you want to send a big tx and you get a MemoryError :/

Another consideration is UI/UX though. Confirmation dialogs with multiple assets per multiple inputs and multiple outputs are going to be seriously confusing. This leads me to think that a better solution would be some explicit limits, for example, support multiple assets but just one input and one output.

What are your thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/trezor/trezor-common/pull/264#issuecomment-460988338, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABMp9tbbT6Jp9MOBftmeNpqRWf8Kdqgtks5vKrpPgaJpZM4aHNXW.

matejcik commented 5 years ago

For these types there’s just one msg per transaction. So should not be an issue.

In that case please remove the repeated <Something> fields and inline their contents.

matejcik commented 5 years ago

I also just noticed that there are no fields for a BIP32 path for signing.

Is there supposed to be one path per account? One path per address, with multiple addresses per account? In particular, what do you use for signing transfers with multiple input addresses?

notatestuser commented 5 years ago

We’re getting further away from the actual structure of the tx, and the 1 msg limit is subject to change. Is it that much of a problem to keep it as it is?

Get Outlook for iOShttps://aka.ms/o0ukef

From: matejcik notifications@github.com Sent: Wednesday, February 6, 2019 7:24 pm To: trezor/trezor-common Cc: Luke Plaster; Mention Subject: Re: [trezor/trezor-common] Add Binance Chain (#264)

For these types there’s just one msg per transaction. So should not be an issue.

In that case please remove the repeated fields and inline their contents.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/trezor/trezor-common/pull/264#issuecomment-460989482, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABMp9sY2li9SovtZ2uHIvzBvBr6IP6wSks5vKrtcgaJpZM4aHNXW.

notatestuser commented 5 years ago

Good point. We should send a bip32 path for signing. We plan to have an address select function with UX flow similar to how using a hardware wallet with MEW works.

Get Outlook for iOShttps://aka.ms/o0ukef

From: matejcik notifications@github.com Sent: Wednesday, February 6, 2019 7:26 pm To: trezor/trezor-common Cc: Luke Plaster; Mention Subject: Re: [trezor/trezor-common] Add Binance Chain (#264)

I also just noticed that there are no fields for a BIP32 path for signing.

Is there supposed to be one path per account? One path per address, with multiple addresses per account? In particular, what do you use for signing transfers with multiple input addresses?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/trezor/trezor-common/pull/264#issuecomment-460990076, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABMp9s8q7GZk9HMsLlts4rCmZpNKhaChks5vKrvegaJpZM4aHNXW.

notatestuser commented 5 years ago

To be clear, there is a way to use multiple input addresses and attach the relevant signatures, but not from any of the wallet code or UIs that we have right now. The signatures are supplied in an array, and each of them contain a public key derived with a bip32 path. An "account" is generally considered to be one private key seed, where each distinct HD path becomes an address in that account.

If you would like to see a visual demonstration of this, I could provide examples.

Get Outlook for iOShttps://aka.ms/o0ukef

From: matejcik notifications@github.com Sent: Wednesday, February 6, 2019 7:26 pm To: trezor/trezor-common Cc: Luke Plaster; Mention Subject: Re: [trezor/trezor-common] Add Binance Chain (#264)

I also just noticed that there are no fields for a BIP32 path for signing.

Is there supposed to be one path per account? One path per address, with multiple addresses per account? In particular, what do you use for signing transfers with multiple input addresses?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/trezor/trezor-common/pull/264#issuecomment-460990076, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABMp9s8q7GZk9HMsLlts4rCmZpNKhaChks5vKrvegaJpZM4aHNXW.

matejcik commented 5 years ago

looks like it means "kind of like bitcoin"? I.e., your paths will be m/44'/714'/a'/c/n, for account, change and n-th address respectively? Do you use the UTXO model and/or change addresses?

Will the address field be superseded by the BIP32 path? Are there other fields that are actually from-addresses (sender maybe)?

Looking at the example JSONs, iiuc the basic structure is like so:

{
   /* common fields go here */
   msgs: [
     { /* message object 1 */ },
     /*...*/,
     { /* message object n */ },
   ]
}

Presumably, the whole blob will be signed by a single HD path - for now anyway? (nothing stopping you from resubmitting the transaction with a different HD path to get another signature)

Do I understand correctly that the envelope does not encode operation type, and it is determinded by looking at each message object?

If that's the case, I'd recommend the following changes:

add address_n as a field to the common properties
flip the structure around, similar to what Stellar has:
1. rename BinanceTxCommon to BinanceSignTx
2. add a field uint32 msg_count
3. add a message BinanceTxRequest (possibly with uint32 index), for which each of Binance<Something>Tx is a possible response
4. drop the common fields and inline the other fields, i.e., each Binance<Something>Tx will 1-to-1 correspond to one element of the msgs array. (and perhaps rename Binance<Something>Tx to Binance<Something>Msg to better express this?

This allows you to sign a transaction with arbitrary number of operations, and AFAICT is a better match for your transaction format.

There can still be problems with the Transfer message, with arbitrary number of inputs / outputs, but these can be solved by adding details to the BinanceTxRequest if necessary.

notatestuser commented 5 years ago

looks like it means "kind of like bitcoin"? I.e., your paths will be m/44'/714'/a'/c/n, for account, change and n-th address respectively? Do you use the UTXO model and/or change addresses?

Just inputs and outputs. Using a change address is possible but not mandatory.

Will the address field be superseded by the BIP32 path? Are there other fields that are actually from-addresses (sender maybe)?

I am not quite sure what you meant by "superseded by the BIP32 path".

You are right that sender represents the from address.

If you were referring to BinanceGetAddress in your first question, the address_n field was actually supposed to be for the bip32 path (repeated uint32's).

On that note, it occurs to me that the bip32 path should probably also be included in something like BinanceTxCommon.

Presumably, the whole blob will be signed by a single HD path - for now anyway? (nothing stopping you from resubmitting the transaction with a different HD path to get another signature)

That's correct.

Do I understand correctly that the envelope does not encode operation type, and it is determinded by looking at each message object?

Yes.

This allows you to sign a transaction with arbitrary number of operations, and AFAICT is a better match for your transaction format.

Seems good to me. I'll make this change and update this PR. Thank you!

notatestuser commented 5 years ago

I have applied your suggestions with some minor adjustments:

BinanceSignTx is just sent once in the initial BinanceTxRequest.
BinanceTxRequest contains msg_count and first_msg_index to start the tx build. This splits out the common standard tx fields from our codebase from the Trezor-specific fields.
Each msg contains a next_msg_index to allow an indication of which comes next. When the msg_count sent in BinanceTxRequest is reached, the device will not follow this pointer and instead build the signature.

Let me know of your thoughts.

prusnak commented 5 years ago

@notatestuser Thanks! The protocol looks sane now. You can start implementing the changes into the trezor-core firmware using this protocol we just iterated.

tsbd commented 5 years ago

@prusnak Hello, this is Tim from the commercial side of Binance working on coordinating integration of Binance Chain, DEX connectivity with Trezor. I work along with @notatestuser and his team. We are aiming for February 20 to complete the integration and want to ensure you have the proper support to meet this tight deadline.

Can you clarify on which side will be implementing the changes into the firmware? What are your remaining requirements from us?

prusnak commented 5 years ago

@tsbd My understanding was that you will be working on the firmware. Is that not the case? There was no indication in the thread you were expecting us to do the work.

tsbd commented 5 years ago

@prusnak I see. I have sent a message to your e-mail at pavol@rusnak.io. Please check when you have the chance. Thanks.

matejcik commented 5 years ago

I have applied your suggestions with some minor adjustments:

looks good overall. but details are somewhat murky

BinanceSignTx is just sent once in the initial BinanceTxRequest.

BinanceTxRequest contains msg_count and first_msg_index to start the tx build. This splits out the common standard tx fields from our codebase from the Trezor-specific fields.

Unfortunately, this is not a protocol flow? :) It's unclear which side sends which messages.

My idea was the following conversation:

PC -> Trezor: BinanceSignTx (common fields, number of messages)
Trezor -> PC: BinanceTxRequest (index of requested message)
PC -> Trezor: Binance<Any>Msg
Trezor -> PC: BinanceTxRequest (index of next message, optionally chunk of JSON)
etc.
Trezor -> PC: BinanceSignedTx (json, signature)

By convention, <any>Request are from Trezor to PC, and <any>SignTx is a starting message from PC to Trezor.

The "index of requested message" is not strictly necessary - if you know that you will always send the operations in the same order, feel free to leave that out. The "JSON chunk" is also not strictly necessary. It would allow us to stream out the JSON response as opposed to keeping it in memory. But that's something that can be easily added in the future. All in all, BinanceTxRequest can be an empty message, indicating "please continue" the appropriate number of times.

IIUC you want a separate struct for your standard fields, aside from Trezor-specific ones. That is OK, but then please nest that struct within the BinanceSignTx, which is going to be the protocol starting message.

Each msg contains a next_msg_index to allow an indication of which comes next. When the msg_count sent in BinanceTxRequest is reached, the device will not follow this pointer and instead build the signature.

What is the function of this field? Is this supposed to be the "index of the requested message" as described above? Or is there a different feature that I don't understand?

ALSO, i'm assuming that it is possible to stream-sign. I.e., calculate a hash of the JSON result progressively and sign that hash at the end; whenever Trezor receives a Binance<Any>Msg, it can generate that part of the JSON separately, extend the hash, and then forget about it. Is this assumption correct?

matejcik commented 5 years ago

looks like it means "kind of like bitcoin"? I.e., your paths will be m/44'/714'/a'/c/n, for account, change and n-th address respectively? Do you use the UTXO model and/or change addresses?

Just inputs and outputs. Using a change address is possible but not mandatory.

my point was, do you need to spend the whole input (and thus need change outputs), or can you do a partial spend (which makes change addresses unnecessary)?

Will the address field be superseded by the BIP32 path? Are there other fields that are actually from-addresses (sender maybe)?

I am not quite sure what you meant by "superseded by the BIP32 path".

You are right that sender represents the from address.

From a security standpoint it is better if you omit, or at least ignore, those address strings that can be calculated from the given BIP32 path. My question was, which fields are these, if any?

E.g., I would expect all address fields in inputs be empty. Or, to support multisig, if a field is non-empty, it's explicitly listed on the Trezor screen.

notatestuser commented 5 years ago

Got it. Now the conventions are clearer to me since the wiki is quite sparse on details. The intention here was for BinanceTxRequest to be the first message from PC->Trezor.

Could you tell me why an empty BinanceTxRequest is even needed, then? Is it to detect if the device is ready to accept the messages or something like that? Surely another more generic type of rejection (via something like Ledger’s CLA/INS check) might be appropriate here.

Regardless of the need for an empty message, I’ll get these changes you’ve suggested into this branch soon.

matejcik commented 5 years ago

Could you tell me why an empty BinanceTxRequest is even needed, then? Is it to detect if the device is ready to accept the messages or something like that? Surely another more generic type of rejection (via something like Ledger’s CLA and INS check) might be appropriate here.

"Something like that", yes :) Basically, there must be a reply for every message sent from the PC, so that we can cleanly interleave Trezor's interaction requests. So after you send BinanceSignTx, there needs to be a valid reply for that. We have a generic Failure message, but no generic "Continue" message because we didn't need it so far (and because we usually want to send data in the reply).

Regardless of the need for an empty message, I’ll get these changes you’ve suggested into this branch soon.

thanks

matejcik commented 5 years ago

note that before your start implementation, you'll have to add your messages to protob/messages.proto

notatestuser commented 5 years ago

How is messages-binance.proto looking now?

In response to your questions:-

my point was, do you need to spend the whole input (and thus need change outputs), or can you do a partial spend (which makes change addresses unnecessary)?

Partial spends are allowed.

What is the function of this field? Is this supposed to be the "index of the requested message" as described above? Or is there a different feature that I don't understand?

Yes, that was the intention. I was not sure why in your example flow Trezor is sending these indexes to PC. Wouldn't it be the PC telling Trezor which messages to include along with their contents?

i'm assuming that it is possible to stream-sign. I.e., calculate a hash of the JSON result progressively and sign that hash at the end; whenever Trezor receives a Binance<Any>Msg, it can generate that part of the JSON separately, extend the hash, and then forget about it. Is this assumption correct?

Yes, this would be possible, as Trezor builds out the json it can stream into the hash function.

E.g., I would expect all address fields in inputs be empty. Or, to support multisig, if a field is non-empty, it's explicitly listed on the Trezor screen.

I understand what you are suggesting, but this part of the underlying implementation probably will not change at this point.

matejcik commented 5 years ago

What is the function of this field? Is this supposed to be the "index of the requested message" as described above? Or is there a different feature that I don't understand?

Yes, that was the intention. I was not sure why in your example flow Trezor is sending these indexes to PC. Wouldn't it be the PC telling Trezor which messages to include along with their contents?

I'm sorry that I was not sufficiently clear about this.

A lot of the quirks of Trezor's communication protocols are due to the device having very small memory. So, e.g., with large transactions on Bitcoin, it's necessary for the Trezor device to have random access to data that it doesn't store. This is the origin of the "Request" convention - the device will request from the PC the next chunk of data that it needs.

The index would be used similarly here: if, for some reason, it turned out that Trezor needs to, e.g., process messages in arbitrary order, or look at some of them repeatedly, it would send a BinanceTxRequest with the specified index.

(Right now it seems that this feature is not needed, so we are leaving out the index. It's easy enough to add later, if it's needed for whatever reason.)

With that in mind, what would be the point of the PC telling Trezor which messages to include? If the PC wants some messages excluded, it can simply not send them, right?

notatestuser commented 5 years ago

Got it. Now the flow is starting to make sense. I appreciate the explanation.

The idea behind the message indexes being sent from the PC was to preemptively signal to the Trezor that a particular message is incoming, so that it could allocate memory or otherwise prepare the json structure for the new data. This might be entirely unnecessary - I just wanted the protocol to be as explicit as possible having little knowledge of what the firmware code will look like at this point.

matejcik commented 5 years ago

Oh, ok. That won't be necessary indeed, the incoming packet handling will take care of any such concerns. So please remove these fields.

I've left one additional comment on BinanceVerifyMessage, and I believe that's the last of it.

trezor / trezor-common

Add Binance Chain #264