Closed rithvikvibhu closed 1 year ago
Thanks @Falci! Addressed all comments.
Lots to review here. I read through all the wallet / background stuff and the new signing flow looks really good. I wrote a few review comments that I eventually deleted after reading more code ;-) I'm going to build locally next and try to break it, but great job so far on the wallet flow!
I think this display may be a bit too verbose for Bob. Maybe the goodies can be hidden behind a "show advanced" arrow or something. IMO, all we need here is the main output / action / value (dont even bother showing destination address if its not a NONE send ?) and "who needs to sign".
actually the change output might be confusing to ledger users as well. hm....
also I dont think we should allow users to export TX before they sign it themselves? I think thats going to lead to confusion
Ok this is fun: create 2of2 ... I signed a tx, but then altered the exported file (changed the sighash flag). Importing that into the second wallet, it showed the first signer as having not signed (because you cleverly actually check the sig with secp256k1) BUT when I signed with the second wallet, the logic still detected what it thinks is two valid signatures. I'm not sure there's any great way to check for this (palm reader doesnt either I dont think) BUT we could:
https://user-images.githubusercontent.com/2084648/196471195-16fc0b55-dc92-452a-85b2-c9d1fd1314b7.mp4
Here I tried to import the same xpub a second time. Instead of throwing an error, what it did was RENAME the original signer and then reset the second input field
while were talking about "advanced" features... maybe you can offer a "copy hex to clipboard" or something to be compatible with palm reader multisig UI 😬 . Its a good addition to export as file anyway
while were talking about "advanced" features... maybe you can offer a "copy hex to clipboard" or something to be compatible with palm reader multisig UI grimacing . Its a good addition to export as file anyway
added a tertiary button so that it's visible, but it doesn't attract attention from regular bob users won't need it @pinheadmz:
Adds multisig wallets to Bob! Closes #493, #549.
The wallet creation and import flows have one new section for multisig. This also works with Ledger devices. The general flow to use multisig wallets is:
Pics below may make more sense.
Screenshots
Click items to expand screenshots.
Creating / Importing multisig wallets
![image](https://user-images.githubusercontent.com/5113343/194629196-88b402b2-c92a-48b5-ab57-d0a5a9ee5495.png)Wallet setup
![image](https://user-images.githubusercontent.com/5113343/194630732-4348caff-f370-4442-ab55-450794a94190.png)Ready to use
![image](https://user-images.githubusercontent.com/5113343/194631072-5628a2ca-2aba-42a1-bbeb-793ec523cee3.png)Transaction Viewer
![image](https://user-images.githubusercontent.com/5113343/194632567-3ec22786-3d67-47e2-9313-e22c3610f1db.png)Multisig + Ledger
![image](https://user-images.githubusercontent.com/5113343/194633304-472ad060-8292-4e97-9d83-39f29b0c195a.png)Gotchas / Notes
What works and what doesn't
(bid, reveal, redeem, etc.)
(register, update, transfer, etc.)
*
**
*
/**
(send and receive)
&
&
&
*
**
&
For reviewers
Tips to review
Transaction File
This is conceptually similar to Bitcoin's PSBT, but in JSON format so easier to handle. It includes a transaction as a hex string, and metadata for inputs and outputs.
app/background/wallet/service.js
has JSDoc typedefs at the end of the file and helps with autocomplete if your editor supports it.where:
version
is always 1 (for now)metadata.{inputs, outputs}
are arrays with index matching input/output numbers (from 0)sighashType
(optional, defaults to ALL): refer to hsd's Script.hashTypename
- required if a covenant is present (non-NONE)bid
- required if covenant is BID, in dollarydoosTo Do / Discuss
master
and multisig requires https://github.com/handshake-org/hsd/pull/767. If we considermaster
as unstable, it should be fine to update package.json with hsd master