Conversation
Tomasvrba
force-pushed
the
eth-stream
branch
from
0dda6f5 to
2ec0fb1
Compare
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
Thank you!
Please bump Cargo.toml and NPM_VERSION versions and add a changelog entry to both CHANGELOG files.
| /// Signs an Ethereum transaction. It returns a 65 byte signature (R, S, and 1 byte recID). The | ||
| /// `tx` param can be constructed manually or parsed from a raw transaction using | ||
| /// `raw_tx_slice.try_into()` (`rlp` feature required). | ||
| pub async fn eth_sign_transaction( |
There was a problem hiding this comment.
The flow here is even nicer than in bitbox02.py, where the streaming was folded into the handle_antiklepto helper 😄
| @@ -0,0 +1,174 @@ | |||
| #![cfg(feature = "simulator")] | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
Should be the first line, followed by an empty line. I see the other files in this folder have the same mistake - would you mind fixing them all up?
tests/test_eth.rs
Outdated
| assert!(address.starts_with("0x")); | ||
| assert_eq!(address.len(), 42); |
There was a problem hiding this comment.
Just compare the whole address to a string literal - the simulator runs with a fixed seed.
tests/test_eth.rs
Outdated
| async fn test_eth_sign_transaction_nonstreaming() { | ||
| test_initialized_simulators(async |paired_bitbox| { | ||
| // Skip if firmware doesn't support ETH | ||
| if !paired_bitbox.eth_supported() { |
There was a problem hiding this comment.
I'd assert!(paired_bitbox.eth_supported()) instead, as the simulator has it and it's important to not accidentally skip the test (we do the same in the cardno simulator tests)
tests/test_eth.rs
Outdated
| assert!( | ||
| result.is_ok(), | ||
| "eth_sign_transaction failed: {:?}", | ||
| result.err() | ||
| ); |
There was a problem hiding this comment.
Redundant, the next line will make the test fail if it was not ok. Same for the tests below.
tests/test_eth.rs
Outdated
| "eth_sign_transaction streaming failed: {:?}", | ||
| result.err() | ||
| ); | ||
| assert_eq!(result.unwrap().len(), 65); |
There was a problem hiding this comment.
How can we tell that streaming worked properly?
Maybe we could verify the signature using bitcoin::secp256k1 against the sighash of the tx above? Could hardcode the expected sighash for this (maybe with a tiny Go/Python script in a comment that shows how it was computed, similar to the test gen tools in the PR you made in the fw repo).
There was a problem hiding this comment.
we don't need to generate hundreds of test cases here right, so just do it directly in rust in the test file without external scripts?
added it to the test file, don't have to rely on an external script this way that would have to run to regenerate the sighashes if something changed
There was a problem hiding this comment.
Yeah didn't realize doing it in Rust would be so simple, even better 👍
will do, wasn't done yet, was still going to do a full end to end test with Rabby before asking for a review but you beat me to it :D |
Tomasvrba
force-pushed
the
eth-stream
branch
from
2ec0fb1 to
258f381
Compare
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
utACK, nice. Please squash the streaming-related commits, and leave the webhid.js and license header fix in separate commits. Thanks
eth: update proto files to prepare for streaming eth: implement streaming handler for large transaction data eth: add simulator integration tests for eth tx data streaming tests: add signature verification to eth tests bump versions and add CHANGELOG entries
wasm-pack 0.13+ adds "type": "module" to the generated package.json, which requires fully-specified import paths. Add the .js extension to the raw_module attribute so the generated import works with strict ESM bundlers like webpack.
Tomasvrba
force-pushed
the
eth-stream
branch
from
258f381 to
1e4f59a
Compare
2 participants
No description provided.