Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

step-by-step guide on how to use HWI and Bitcoin Core #413

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

8go
Copy link
Contributor

@8go 8go commented Jan 12, 2021

This page shows in details through screenshots and description on how to use a hardware wallet with Bitcoin Core through the help of HWI. I hope this one-pager helps people to see how it works, takes their fear and increments the use of hardware wallets with Bitcoin Core.

@prusnak prusnak added the docs label Feb 22, 2021
Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

It would be nice for the screenshots to have more descriptive names rather than just the date and time they were made.

Additionally, since we are moving to using Sphinx and RST for docs soon (see #464), it would be nice for this to be formatted with RST so that it will work without changes in Sphinx.

docs/walk-through/README.md Outdated Show resolved Hide resolved
docs/walk-through/README.md Outdated Show resolved Hide resolved
docs/walk-through/README.md Outdated Show resolved Hide resolved
# of unknown address on Trezor display
wallet=wallet.test
rec=$(hwi --testnet -f $TREZORFINGERPRINT_TESTNET getkeypool --wpkh --path m/84h/${DERIVATIONPATH_TESTNET}h/0h/0/* --keypool 0 1000)
chg=$(hwi --testnet -f $TREZORFINGERPRINT_TESTNET getkeypool --wpkh --path m/84h/${DERIVATIONPATH_TESTNET}h/0h/1/* --keypool --internal 0 1000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--wpkh was replaced with --addr-type wpkh.

Additionally, it is unnecessary to specify --path if you are planning on just using the default path. To get the testnet path, you can use the --testnet option.

Lastly, without --path, getkeypool will return both change and receiving descriptors, so it is unnecessary to have call it twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Andrew, I replaced the --wpkh with --addr-type wpkh so that this option is up-to-date.

I know and I understand that by removing --path that with a single command both receiving and change address will be handled at once and that there are fewer calls, i.e. the script is shorter. However, I copied this code out of my script that I used for testing many different scenarios. I find it definitely helpful, for users to see which address they are creating, so the next time around e.g. they can do address 1000 through 2000, etc. I personally also found it useful to see path so that the script can be adapted to different networks. In my humble opinion it helps the user understand better and through these examples the user learns more and can adapt this script to more situations.

In short, I was not going for the shortest code, or the simplest solution, but for the solution that teaches the most and demos the most info. As such, I have left the options --path and the additional --keypool arguments in place.

What do you think @achow101 ? Reasonable?

docs/walk-through/README.md Outdated Show resolved Hide resolved
8go pushed a commit to 8go/HWI that referenced this pull request Feb 25, 2021
- all/most of the recommended changes from @achow101 have been worked into this RST file
- see comments and discussion in bitcoin-core#413
8go pushed a commit to 8go/HWI that referenced this pull request Feb 25, 2021
@8go
Copy link
Contributor Author

8go commented Feb 25, 2021

Thanks Andrew @achow101 for your valuable feedback on PR #413.

I addressed nearly all of your issues and followed your suggestions.
The new version is now also in RST format instead of MD.

Your suggestions have been added via the 3 commits 067e0c0 , 58379f6 and 30b1bb4.

Please have a look at the improved version. Let me know if there is anything else that needs to be done.

Thanks again and cheers!

8go pushed a commit to 8go/HWI that referenced this pull request Feb 25, 2021
@8go 8go requested a review from achow101 February 25, 2021 19:59
@achow101
Copy link
Member

Please squash your commits. Also, please don't @ mention people in commits as they will receive Github notifications every time that commit is pushed.

I would still like for the screenshots to be given understandable names.

@8go
Copy link
Contributor Author

8go commented Feb 26, 2021

Hi Andrew,

All done.
The commits are squashed.
Screenshots have nice descriptive names now.

Please have a look at this revised version. Thanks!

@achow101
Copy link
Member

ACK 7de0237

@achow101 achow101 merged commit 1623f1c into bitcoin-core:master Feb 26, 2021
achow101 added a commit that referenced this pull request Feb 26, 2021
c603135 Move walkthrough into sphinx (Andrew Chow)

Pull request description:

  Followup to #413.

  Moves the walkthrough doc under the examples directory and adds the file to the sphinx generated documentation.

Top commit has no ACKs.

Tree-SHA512: 96af0fa093f3035da3842fc7601d587ccb09ac4a89f93844ccdd345802f351abae0005ccb3fe06a88cea7aac543674fb81d37a8ded18c65abd9bf8981f341170
@8go
Copy link
Contributor Author

8go commented Feb 26, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants