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.

Sign up for GitHub

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

Jump to bottom

Support minting on Ledger signing device #1301

Merged
merged 4 commits into from
Jan 20, 2025
Merged

Support minting on Ledger signing device #1301

merged 4 commits into from
Jan 20, 2025

Conversation

guggero
Copy link
Member

@guggero guggero commented Jan 16, 2025 •
edited
Loading

Depends on #1290.
Depends on #1295.

Adds the necessary meta information to the group key VM transaction PSBT to allow signing of such a mint authorization transaction using a custom version of the HWI library and a Ledger signing device.

A new guide documents the required steps.

@guggero guggero requested review from Roasbeef and ffranr January 16, 2025 18:45
@coveralls
Copy link

coveralls commented Jan 16, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 12871300835

Details

  • 33 of 134 (24.63%) changed or added relevant lines in 1 file are covered.
  • 36 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-0.06%) to 40.772%
Changes Missing Coverage Covered Lines Changed/Added Lines %
tapgarden/planter.go 33 134 24.63%
Files with Coverage Reduction New Missed Lines %
tappsbt/create.go 2 53.22%
tapchannel/aux_invoice_manager.go 2 85.28%
asset/asset.go 2 77.13%
commitment/tap.go 3 84.32%
tapdb/universe.go 4 80.91%
tapgarden/planter.go 4 65.19%
tapgarden/caretaker.go 4 68.5%
tapchannel/aux_leaf_signer.go 5 43.08%
universe/interface.go 10 52.81%
Totals Coverage Status
Change from base Build 12871160944: -0.06%
Covered Lines: 26661
Relevant Lines: 65390
💛 - Coveralls

Base automatically changed from pederson-group-key to main January 17, 2025 08:10
@guggero guggero force-pushed the cold-group-key-pedersen branch 2 times, most recently from 826920d to bc4ee59 Compare January 17, 2025 16:58
@guggero guggero changed the base branch from main to sealbatch-signed-group-psbt January 17, 2025 16:58
@guggero
Copy link
Member Author

guggero commented Jan 17, 2025

Changed the base branch to sealbatch-signed-group-psbt (#1295) since that gives us the nicer user experience and simplifies the signing guide for the Ledger as well.

@guggero guggero mentioned this pull request Jan 17, 2025
Merged
@ffranr ffranr requested review from GeorgeTsagk and removed request for Roasbeef January 20, 2025 09:06
@gijswijs gijswijs self-requested a review January 20, 2025 09:14
@ffranr ffranr force-pushed the sealbatch-signed-group-psbt branch 3 times, most recently from 21f44ca to 65d1b52 Compare January 20, 2025 09:45
ffranr
ffranr reviewed Jan 20, 2025
View reviewed changes
docs/external-group-key-ledger.md Outdated

## Step 2: Install HWI and derive the xpub for the group key

We will be using the [HWI]() library to talk with the Ledger device from the
Copy link
Contributor

Choose a reason for hiding this comment

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

missing hyperlink

Copy link
Contributor

Choose a reason for hiding this comment

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

also maybe mention that we are using a custom branch of that lib in this guide

Copy link
Contributor

@ffranr ffranr Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should link to the branch by it's commit hash (permalink). So that we know the link matches something definite.

We might want to rename the branch https://github.com/lightninglabs/HWI/tree/tapd-cold-minting
to something that can stick around for a while so that the reference will live on. Maybe tapd-v0.6.0-cold-minting

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I kept the branch but pushed a tag tapd-v0.6.0-cold-minting for this that is referenced in the doc.

docs/external-group-key-ledger.md Outdated Show resolved Hide resolved
@ffranr
Copy link
Contributor

ffranr commented Jan 20, 2025

This needs a rebase. But might need to be rebased again if #1295 gets in first. And vice versa.

@guggero guggero force-pushed the cold-group-key-pedersen branch from bc4ee59 to 559cdbe Compare January 20, 2025 10:56
@guggero
Copy link
Member Author

guggero commented Jan 20, 2025

Addressed the comments and rebased.

ffranr
ffranr approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Nice!

(Depends on #1295)

gijswijs
gijswijs approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@gijswijs gijswijs left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉
I confirmed cold storage signing on my Ledger Nano S plus, and everyhting worked as expected.

I made some comments on the guide that should provide more clarity for people trying to do this as well.

docs/external-group-key-ledger.md
available firmware (`v1.3.1` at time of writing this).
3. Using the "Ledger Live" software, make sure you install the "Bitcoin" app
or update it to the latest version (`v2.3.0` at time of writing this).
4. If you plan to experiment on regtest or testnet first, you also need to
Copy link
Contributor

Choose a reason for hiding this comment

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

The Ledger needs to be in "Developer mode" to be able to install Testnet apps. Maybe mention that here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I forgot about that. Will definitely add! Thank you.

docs/external-group-key-ledger.md Outdated Show resolved Hide resolved
docs/external-group-key-ledger.md Outdated Show resolved Hide resolved
docs/external-group-key-ledger.md Show resolved Hide resolved
asset/asset.go Show resolved Hide resolved
@ffranr
Copy link
Contributor

ffranr commented Jan 20, 2025

Consider the base branch before merging please!

GeorgeTsagk
GeorgeTsagk approved these changes Jan 20, 2025
View reviewed changes
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM

@ffranr ffranr changed the base branch from sealbatch-signed-group-psbt to main January 20, 2025 15:25
@ffranr ffranr enabled auto-merge January 20, 2025 15:25
@ffranr ffranr added this pull request to the merge queue Jan 20, 2025
@guggero guggero removed this pull request from the merge queue due to a manual request Jan 20, 2025
@guggero guggero force-pushed the cold-group-key-pedersen branch from 559cdbe to f82549e Compare January 20, 2025 15:32
@guggero
Copy link
Member Author

guggero commented Jan 20, 2025

Tested on mainnet successfully.

@guggero guggero merged commit 78db2a6 into main Jan 20, 2025
18 checks passed
@guggero guggero deleted the cold-group-key-pedersen branch January 20, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgeTsagk GeorgeTsagk GeorgeTsagk approved these changes

@gijswijs gijswijs gijswijs approved these changes

@ffranr ffranr ffranr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@guggero @coveralls @ffranr @gijswijs @GeorgeTsagk