Add test for sign_tx() #36
Conversation
danielabrozzoni
left a comment
danielabrozzoni
left a comment
There was a problem hiding this comment.
Concept ACK, a few comments.
Once again, stop putting everything in a single commit! This PR should have been separated in 3 commits: one for deserialize, one for the automation.json, one for the test itself.
src/lib.rs
Outdated
| bip32_derivation: hd_keypaths, | ||
| ..Default::default() | ||
| }], | ||
| outputs: vec![Output::default(), Output::default()], |
There was a problem hiding this comment.
You should add one TxOut to the transaction, and here put only one Output::default()
|
|
||
| #[derive(Clone, Eq, PartialEq, Debug, Deserialize)] | ||
| pub struct HWIPartiallySignedTransaction { | ||
| #[serde(deserialize_with = "deserialize_psbt")] |
There was a problem hiding this comment.
Why is this needed? PSBT already has the deserialize method https://docs.rs/bitcoin/latest/bitcoin/util/psbt/struct.PartiallySignedTransaction.html#impl-Deserialize%3C%27de%3E
There was a problem hiding this comment.
I agree that it shouldn't be needed, but it doesn't work without it. Serde complains that expected struct PartiallySignedTransaction while deserializing.
There was a problem hiding this comment.
Mh, weird. Ok to leave it like that then
wszdexdrf
force-pushed
the
signtx
branch
2 times, most recently
from
93519b3 to
a347f10
Compare
I apologize for being lazy. I will keep it in mind. 🙇 |
|
|
||
| #[derive(Clone, Eq, PartialEq, Debug, Deserialize)] | ||
| pub struct HWIPartiallySignedTransaction { | ||
| #[serde(deserialize_with = "deserialize_psbt")] |
There was a problem hiding this comment.
Mh, weird. Ok to leave it like that then
src/lib.rs
Outdated
| version: 1, | ||
| lock_time: 0, | ||
| input: vec![txin], | ||
| output: vec![], |
There was a problem hiding this comment.
Here you should put a txout
src/lib.rs
Outdated
| }], | ||
| }; | ||
|
|
||
| let txin = TxIn { |
There was a problem hiding this comment.
I would call this "previous_txin", so it's easier to understand that it's not the current tx's txin
src/lib.rs
Outdated
| // Here device fingerprint is same as master xpub fingerprint | ||
| hd_keypaths.insert(pk.public_key, (device.fingerprint, derivation_path)); | ||
|
|
||
| let tx = Transaction { |
There was a problem hiding this comment.
Should be called previous_tx
src/lib.rs
Outdated
| xpub: Default::default(), | ||
| version: 0, | ||
| proprietary: BTreeMap::new(), | ||
| unknown: BTreeMap::new(), |
There was a problem hiding this comment.
You can probably use default on those?
|
utACK a1a1454 |
a1a1454df6f6bbcec252be289bd1231573452551 Add test for sign_tx (wszdexdrf)
43bd5797acfc9f829622ac7e01bbf25c411c77a5 Add deserialize function for PSBT (wszdexdrf)
8d3c29bad501170835161a1be0c5b0325a1b4ab1 Change `automation.json` for signing tx. (wszdexdrf)
Pull request description:
Also made necessary changes to automation.json (for ledger devices). Add
deserializing method for PSBT.
ACKs for top commit:
danielabrozzoni:
utACK a1a1454df6f6bbcec252be289bd1231573452551
Tree-SHA512: 2d222006918c420d3d1cc21b7dc09494445791c10d067ba412551d419a3217e682ee73dce4fbc215cf8f1e20c32e828e1a3d76968e07999dfc0a8374ad93dd86
2 participants
Also made necessary changes to automation.json (for ledger devices). Add
deserializing method for PSBT.