adding hardware signers - #135

Merged
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi
Mar 8, 2023
Merged

adding hardware signers#135
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi

Conversation

@ulrichard

@ulrichardulrichard commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

Description

Adding hardware signers, so that hardware wallets can be used to sign transactions.

Notes to the reviewers

It doesn't work 100% yet for me.
I am using the following wallet:
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets2descriptors.rs#L89
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets/multisig_hw_segwit
I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".
So, something must still be missing.

Initially I had some dependency problems, so I deleted Cargo.lock. That solved the dependency problems, but that's the reason for the large diff on that file.

Changelog notice

Added hardware signers through the use of HWI

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Concept ACK.. Thanks for adding this.. I was just thinking about this the other day.. Could be useful if anyone wanna use bdk-cli with mainnet (still not recommended)..

I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".

From an initial glance, it looks like all the wallets you are using are watch-only? It has Xpubs and addresses only.. Such wallets in bdk can't sign for transactions as they don't have the private key.. and is_finalised=false means the resulting psbt doesn't have signatures.

PS: We should throw an error in those cases if use asks to sign a bdk-cli with watch only descriptors.

Can you try testing with a single sig wallet, with keys generated from bdk-cli itself?? If the HWI part is working properly that should work..

I will also try to test this out in an emulator..

@rajarshimaitra

rajarshimaitra commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I think the elctrum error you are seeing is also part of our lack of error handling in unsigned psbts and should be fixed with #128.

But it only handles it while broadcast.. I will try to see how the watch-only case can be captured in signing also..

@ulrichard

Copy link
Copy Markdown
ContributorAuthor

I tested with single sig hw wallets now.
With the Trezor everything works nicely.
With the Ledger, I got the descriptors, but when I tried to sign, I got an IO error "failed to open device". Anyway, since it works with the Trezor, I'm sure it has nothing to do with the bdk-cli integration.

@ulrichard
ulrichard marked this pull request as ready for review January 10, 2023 14:32
@ulrichard
ulrichardforce-pushed the feature/hwi branch 2 times, most recently from 816b8cc to bd2b66eCompareJanuary 10, 2023 14:45
@notmandatorynotmandatory added the enhancement New feature or request label Jan 10, 2023

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review ACK bd2b66e

Few more changes and it would be good to go..

Also seems to have conflict with master..

I have successfully tried it out with a single sig coldcard..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recently changed our changelog system from this file to just include it in the PR description.

The new PR template is here.. https://github.com/bitcoindevkit/bdk-cli/blob/master/.github/pull_request_template.md

Can you just add a Change Log section with this line in the description instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

Comment threadsrc/handlers.rs
Comment threadsrc/utils.rs Outdated
Comment threadsrc/utils.rs Outdated

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re Ack 927b066 modulo one comment..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

@notmandatory

notmandatory commented Feb 14, 2023

Copy link
Copy Markdown
Member

@ulrichard we made some CI changes and bumped the bdk and bdk-reserves versions, please rebase after #143 is merged and then this should be ready to go. Thanks!

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utACK 100ea8d

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rajarshimaitra
rajarshimaitra merged commit e3d09a6 into bitcoindevkit:masterMar 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@ulrichard@rajarshimaitra@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

adding hardware signers - #135

Merged
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi
Mar 8, 2023
Merged

adding hardware signers#135
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi

Conversation

@ulrichard

@ulrichardulrichard commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

Description

Adding hardware signers, so that hardware wallets can be used to sign transactions.

Notes to the reviewers

It doesn't work 100% yet for me.
I am using the following wallet:
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets2descriptors.rs#L89
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets/multisig_hw_segwit
I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".
So, something must still be missing.

Initially I had some dependency problems, so I deleted Cargo.lock. That solved the dependency problems, but that's the reason for the large diff on that file.

Changelog notice

Added hardware signers through the use of HWI

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Concept ACK.. Thanks for adding this.. I was just thinking about this the other day.. Could be useful if anyone wanna use bdk-cli with mainnet (still not recommended)..

I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".

From an initial glance, it looks like all the wallets you are using are watch-only? It has Xpubs and addresses only.. Such wallets in bdk can't sign for transactions as they don't have the private key.. and is_finalised=false means the resulting psbt doesn't have signatures.

PS: We should throw an error in those cases if use asks to sign a bdk-cli with watch only descriptors.

Can you try testing with a single sig wallet, with keys generated from bdk-cli itself?? If the HWI part is working properly that should work..

I will also try to test this out in an emulator..

@rajarshimaitra

rajarshimaitra commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I think the elctrum error you are seeing is also part of our lack of error handling in unsigned psbts and should be fixed with #128.

But it only handles it while broadcast.. I will try to see how the watch-only case can be captured in signing also..

@ulrichard

Copy link
Copy Markdown
ContributorAuthor

I tested with single sig hw wallets now.
With the Trezor everything works nicely.
With the Ledger, I got the descriptors, but when I tried to sign, I got an IO error "failed to open device". Anyway, since it works with the Trezor, I'm sure it has nothing to do with the bdk-cli integration.

@ulrichard
ulrichard marked this pull request as ready for review January 10, 2023 14:32
@ulrichard
ulrichardforce-pushed the feature/hwi branch 2 times, most recently from 816b8cc to bd2b66eCompareJanuary 10, 2023 14:45
@notmandatorynotmandatory added the enhancement New feature or request label Jan 10, 2023

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review ACK bd2b66e

Few more changes and it would be good to go..

Also seems to have conflict with master..

I have successfully tried it out with a single sig coldcard..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recently changed our changelog system from this file to just include it in the PR description.

The new PR template is here.. https://github.com/bitcoindevkit/bdk-cli/blob/master/.github/pull_request_template.md

Can you just add a Change Log section with this line in the description instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

Comment threadsrc/handlers.rs
Comment threadsrc/utils.rs Outdated
Comment threadsrc/utils.rs Outdated

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re Ack 927b066 modulo one comment..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

@notmandatory

notmandatory commented Feb 14, 2023

Copy link
Copy Markdown
Member

@ulrichard we made some CI changes and bumped the bdk and bdk-reserves versions, please rebase after #143 is merged and then this should be ready to go. Thanks!

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utACK 100ea8d

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rajarshimaitra
rajarshimaitra merged commit e3d09a6 into bitcoindevkit:masterMar 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@ulrichard@rajarshimaitra@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

adding hardware signers - #135

Merged
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi
Mar 8, 2023
Merged

adding hardware signers#135
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi

Conversation

@ulrichard

@ulrichardulrichard commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

Description

Adding hardware signers, so that hardware wallets can be used to sign transactions.

Notes to the reviewers

It doesn't work 100% yet for me.
I am using the following wallet:
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets2descriptors.rs#L89
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets/multisig_hw_segwit
I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".
So, something must still be missing.

Initially I had some dependency problems, so I deleted Cargo.lock. That solved the dependency problems, but that's the reason for the large diff on that file.

Changelog notice

Added hardware signers through the use of HWI

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Concept ACK.. Thanks for adding this.. I was just thinking about this the other day.. Could be useful if anyone wanna use bdk-cli with mainnet (still not recommended)..

I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".

From an initial glance, it looks like all the wallets you are using are watch-only? It has Xpubs and addresses only.. Such wallets in bdk can't sign for transactions as they don't have the private key.. and is_finalised=false means the resulting psbt doesn't have signatures.

PS: We should throw an error in those cases if use asks to sign a bdk-cli with watch only descriptors.

Can you try testing with a single sig wallet, with keys generated from bdk-cli itself?? If the HWI part is working properly that should work..

I will also try to test this out in an emulator..

@rajarshimaitra

rajarshimaitra commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I think the elctrum error you are seeing is also part of our lack of error handling in unsigned psbts and should be fixed with #128.

But it only handles it while broadcast.. I will try to see how the watch-only case can be captured in signing also..

@ulrichard

Copy link
Copy Markdown
ContributorAuthor

I tested with single sig hw wallets now.
With the Trezor everything works nicely.
With the Ledger, I got the descriptors, but when I tried to sign, I got an IO error "failed to open device". Anyway, since it works with the Trezor, I'm sure it has nothing to do with the bdk-cli integration.

@ulrichard
ulrichard marked this pull request as ready for review January 10, 2023 14:32
@ulrichard
ulrichardforce-pushed the feature/hwi branch 2 times, most recently from 816b8cc to bd2b66eCompareJanuary 10, 2023 14:45
@notmandatorynotmandatory added the enhancement New feature or request label Jan 10, 2023

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review ACK bd2b66e

Few more changes and it would be good to go..

Also seems to have conflict with master..

I have successfully tried it out with a single sig coldcard..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recently changed our changelog system from this file to just include it in the PR description.

The new PR template is here.. https://github.com/bitcoindevkit/bdk-cli/blob/master/.github/pull_request_template.md

Can you just add a Change Log section with this line in the description instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

Comment threadsrc/handlers.rs
Comment threadsrc/utils.rs Outdated
Comment threadsrc/utils.rs Outdated

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re Ack 927b066 modulo one comment..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

@notmandatory

notmandatory commented Feb 14, 2023

Copy link
Copy Markdown
Member

@ulrichard we made some CI changes and bumped the bdk and bdk-reserves versions, please rebase after #143 is merged and then this should be ready to go. Thanks!

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utACK 100ea8d

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rajarshimaitra
rajarshimaitra merged commit e3d09a6 into bitcoindevkit:masterMar 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@ulrichard@rajarshimaitra@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

adding hardware signers - #135

Merged
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi
Mar 8, 2023
Merged

adding hardware signers#135
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi

Conversation

@ulrichard

@ulrichardulrichard commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

Description

Adding hardware signers, so that hardware wallets can be used to sign transactions.

Notes to the reviewers

It doesn't work 100% yet for me.
I am using the following wallet:
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets2descriptors.rs#L89
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets/multisig_hw_segwit
I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".
So, something must still be missing.

Initially I had some dependency problems, so I deleted Cargo.lock. That solved the dependency problems, but that's the reason for the large diff on that file.

Changelog notice

Added hardware signers through the use of HWI

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Concept ACK.. Thanks for adding this.. I was just thinking about this the other day.. Could be useful if anyone wanna use bdk-cli with mainnet (still not recommended)..

I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".

From an initial glance, it looks like all the wallets you are using are watch-only? It has Xpubs and addresses only.. Such wallets in bdk can't sign for transactions as they don't have the private key.. and is_finalised=false means the resulting psbt doesn't have signatures.

PS: We should throw an error in those cases if use asks to sign a bdk-cli with watch only descriptors.

Can you try testing with a single sig wallet, with keys generated from bdk-cli itself?? If the HWI part is working properly that should work..

I will also try to test this out in an emulator..

@rajarshimaitra

rajarshimaitra commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I think the elctrum error you are seeing is also part of our lack of error handling in unsigned psbts and should be fixed with #128.

But it only handles it while broadcast.. I will try to see how the watch-only case can be captured in signing also..

@ulrichard

Copy link
Copy Markdown
ContributorAuthor

I tested with single sig hw wallets now.
With the Trezor everything works nicely.
With the Ledger, I got the descriptors, but when I tried to sign, I got an IO error "failed to open device". Anyway, since it works with the Trezor, I'm sure it has nothing to do with the bdk-cli integration.

@ulrichard
ulrichard marked this pull request as ready for review January 10, 2023 14:32
@ulrichard
ulrichardforce-pushed the feature/hwi branch 2 times, most recently from 816b8cc to bd2b66eCompareJanuary 10, 2023 14:45
@notmandatorynotmandatory added the enhancement New feature or request label Jan 10, 2023

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review ACK bd2b66e

Few more changes and it would be good to go..

Also seems to have conflict with master..

I have successfully tried it out with a single sig coldcard..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recently changed our changelog system from this file to just include it in the PR description.

The new PR template is here.. https://github.com/bitcoindevkit/bdk-cli/blob/master/.github/pull_request_template.md

Can you just add a Change Log section with this line in the description instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

Comment threadsrc/handlers.rs
Comment threadsrc/utils.rs Outdated
Comment threadsrc/utils.rs Outdated

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re Ack 927b066 modulo one comment..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

@notmandatory

notmandatory commented Feb 14, 2023

Copy link
Copy Markdown
Member

@ulrichard we made some CI changes and bumped the bdk and bdk-reserves versions, please rebase after #143 is merged and then this should be ready to go. Thanks!

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utACK 100ea8d

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rajarshimaitra
rajarshimaitra merged commit e3d09a6 into bitcoindevkit:masterMar 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@ulrichard@rajarshimaitra@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

adding hardware signers - #135

Merged
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi
Mar 8, 2023
Merged

adding hardware signers#135
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi

Conversation

@ulrichard

@ulrichardulrichard commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

Description

Adding hardware signers, so that hardware wallets can be used to sign transactions.

Notes to the reviewers

It doesn't work 100% yet for me.
I am using the following wallet:
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets2descriptors.rs#L89
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets/multisig_hw_segwit
I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".
So, something must still be missing.

Initially I had some dependency problems, so I deleted Cargo.lock. That solved the dependency problems, but that's the reason for the large diff on that file.

Changelog notice

Added hardware signers through the use of HWI

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Concept ACK.. Thanks for adding this.. I was just thinking about this the other day.. Could be useful if anyone wanna use bdk-cli with mainnet (still not recommended)..

I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".

From an initial glance, it looks like all the wallets you are using are watch-only? It has Xpubs and addresses only.. Such wallets in bdk can't sign for transactions as they don't have the private key.. and is_finalised=false means the resulting psbt doesn't have signatures.

PS: We should throw an error in those cases if use asks to sign a bdk-cli with watch only descriptors.

Can you try testing with a single sig wallet, with keys generated from bdk-cli itself?? If the HWI part is working properly that should work..

I will also try to test this out in an emulator..

@rajarshimaitra

rajarshimaitra commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I think the elctrum error you are seeing is also part of our lack of error handling in unsigned psbts and should be fixed with #128.

But it only handles it while broadcast.. I will try to see how the watch-only case can be captured in signing also..

@ulrichard

Copy link
Copy Markdown
ContributorAuthor

I tested with single sig hw wallets now.
With the Trezor everything works nicely.
With the Ledger, I got the descriptors, but when I tried to sign, I got an IO error "failed to open device". Anyway, since it works with the Trezor, I'm sure it has nothing to do with the bdk-cli integration.

@ulrichard
ulrichard marked this pull request as ready for review January 10, 2023 14:32
@ulrichard
ulrichardforce-pushed the feature/hwi branch 2 times, most recently from 816b8cc to bd2b66eCompareJanuary 10, 2023 14:45
@notmandatorynotmandatory added the enhancement New feature or request label Jan 10, 2023

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review ACK bd2b66e

Few more changes and it would be good to go..

Also seems to have conflict with master..

I have successfully tried it out with a single sig coldcard..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recently changed our changelog system from this file to just include it in the PR description.

The new PR template is here.. https://github.com/bitcoindevkit/bdk-cli/blob/master/.github/pull_request_template.md

Can you just add a Change Log section with this line in the description instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

Comment threadsrc/handlers.rs
Comment threadsrc/utils.rs Outdated
Comment threadsrc/utils.rs Outdated

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re Ack 927b066 modulo one comment..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

@notmandatory

notmandatory commented Feb 14, 2023

Copy link
Copy Markdown
Member

@ulrichard we made some CI changes and bumped the bdk and bdk-reserves versions, please rebase after #143 is merged and then this should be ready to go. Thanks!

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utACK 100ea8d

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rajarshimaitra
rajarshimaitra merged commit e3d09a6 into bitcoindevkit:masterMar 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@ulrichard@rajarshimaitra@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

adding hardware signers - #135

Merged
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi
Mar 8, 2023
Merged

adding hardware signers#135
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi

Conversation

@ulrichard

@ulrichardulrichard commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

Description

Adding hardware signers, so that hardware wallets can be used to sign transactions.

Notes to the reviewers

It doesn't work 100% yet for me.
I am using the following wallet:
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets2descriptors.rs#L89
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets/multisig_hw_segwit
I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".
So, something must still be missing.

Initially I had some dependency problems, so I deleted Cargo.lock. That solved the dependency problems, but that's the reason for the large diff on that file.

Changelog notice

Added hardware signers through the use of HWI

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Concept ACK.. Thanks for adding this.. I was just thinking about this the other day.. Could be useful if anyone wanna use bdk-cli with mainnet (still not recommended)..

I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".

From an initial glance, it looks like all the wallets you are using are watch-only? It has Xpubs and addresses only.. Such wallets in bdk can't sign for transactions as they don't have the private key.. and is_finalised=false means the resulting psbt doesn't have signatures.

PS: We should throw an error in those cases if use asks to sign a bdk-cli with watch only descriptors.

Can you try testing with a single sig wallet, with keys generated from bdk-cli itself?? If the HWI part is working properly that should work..

I will also try to test this out in an emulator..

@rajarshimaitra

rajarshimaitra commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I think the elctrum error you are seeing is also part of our lack of error handling in unsigned psbts and should be fixed with #128.

But it only handles it while broadcast.. I will try to see how the watch-only case can be captured in signing also..

@ulrichard

Copy link
Copy Markdown
ContributorAuthor

I tested with single sig hw wallets now.
With the Trezor everything works nicely.
With the Ledger, I got the descriptors, but when I tried to sign, I got an IO error "failed to open device". Anyway, since it works with the Trezor, I'm sure it has nothing to do with the bdk-cli integration.

@ulrichard
ulrichard marked this pull request as ready for review January 10, 2023 14:32
@ulrichard
ulrichardforce-pushed the feature/hwi branch 2 times, most recently from 816b8cc to bd2b66eCompareJanuary 10, 2023 14:45
@notmandatorynotmandatory added the enhancement New feature or request label Jan 10, 2023

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review ACK bd2b66e

Few more changes and it would be good to go..

Also seems to have conflict with master..

I have successfully tried it out with a single sig coldcard..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recently changed our changelog system from this file to just include it in the PR description.

The new PR template is here.. https://github.com/bitcoindevkit/bdk-cli/blob/master/.github/pull_request_template.md

Can you just add a Change Log section with this line in the description instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

Comment threadsrc/handlers.rs
Comment threadsrc/utils.rs Outdated
Comment threadsrc/utils.rs Outdated

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re Ack 927b066 modulo one comment..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

@notmandatory

notmandatory commented Feb 14, 2023

Copy link
Copy Markdown
Member

@ulrichard we made some CI changes and bumped the bdk and bdk-reserves versions, please rebase after #143 is merged and then this should be ready to go. Thanks!

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utACK 100ea8d

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rajarshimaitra
rajarshimaitra merged commit e3d09a6 into bitcoindevkit:masterMar 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@ulrichard@rajarshimaitra@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

adding hardware signers - #135

Merged
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi
Mar 8, 2023
Merged

adding hardware signers#135
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi

Conversation

@ulrichard

@ulrichardulrichard commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

Description

Adding hardware signers, so that hardware wallets can be used to sign transactions.

Notes to the reviewers

It doesn't work 100% yet for me.
I am using the following wallet:
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets2descriptors.rs#L89
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets/multisig_hw_segwit
I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".
So, something must still be missing.

Initially I had some dependency problems, so I deleted Cargo.lock. That solved the dependency problems, but that's the reason for the large diff on that file.

Changelog notice

Added hardware signers through the use of HWI

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Concept ACK.. Thanks for adding this.. I was just thinking about this the other day.. Could be useful if anyone wanna use bdk-cli with mainnet (still not recommended)..

I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".

From an initial glance, it looks like all the wallets you are using are watch-only? It has Xpubs and addresses only.. Such wallets in bdk can't sign for transactions as they don't have the private key.. and is_finalised=false means the resulting psbt doesn't have signatures.

PS: We should throw an error in those cases if use asks to sign a bdk-cli with watch only descriptors.

Can you try testing with a single sig wallet, with keys generated from bdk-cli itself?? If the HWI part is working properly that should work..

I will also try to test this out in an emulator..

@rajarshimaitra

rajarshimaitra commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I think the elctrum error you are seeing is also part of our lack of error handling in unsigned psbts and should be fixed with #128.

But it only handles it while broadcast.. I will try to see how the watch-only case can be captured in signing also..

@ulrichard

Copy link
Copy Markdown
ContributorAuthor

I tested with single sig hw wallets now.
With the Trezor everything works nicely.
With the Ledger, I got the descriptors, but when I tried to sign, I got an IO error "failed to open device". Anyway, since it works with the Trezor, I'm sure it has nothing to do with the bdk-cli integration.

@ulrichard
ulrichard marked this pull request as ready for review January 10, 2023 14:32
@ulrichard
ulrichardforce-pushed the feature/hwi branch 2 times, most recently from 816b8cc to bd2b66eCompareJanuary 10, 2023 14:45
@notmandatorynotmandatory added the enhancement New feature or request label Jan 10, 2023

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review ACK bd2b66e

Few more changes and it would be good to go..

Also seems to have conflict with master..

I have successfully tried it out with a single sig coldcard..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recently changed our changelog system from this file to just include it in the PR description.

The new PR template is here.. https://github.com/bitcoindevkit/bdk-cli/blob/master/.github/pull_request_template.md

Can you just add a Change Log section with this line in the description instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

Comment threadsrc/handlers.rs
Comment threadsrc/utils.rs Outdated
Comment threadsrc/utils.rs Outdated

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re Ack 927b066 modulo one comment..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

@notmandatory

notmandatory commented Feb 14, 2023

Copy link
Copy Markdown
Member

@ulrichard we made some CI changes and bumped the bdk and bdk-reserves versions, please rebase after #143 is merged and then this should be ready to go. Thanks!

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utACK 100ea8d

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rajarshimaitra
rajarshimaitra merged commit e3d09a6 into bitcoindevkit:masterMar 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@ulrichard@rajarshimaitra@notmandatory
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

adding hardware signers - #135

Merged
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi
Mar 8, 2023
Merged

adding hardware signers#135
rajarshimaitra merged 1 commit into
bitcoindevkit:masterfrom
AminaBank:feature/hwi

Conversation

@ulrichard

@ulrichardulrichard commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

Description

Adding hardware signers, so that hardware wallets can be used to sign transactions.

Notes to the reviewers

It doesn't work 100% yet for me.
I am using the following wallet:
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets2descriptors.rs#L89
https://github.com/RCasatta/electrum2descriptors/blob/main/tests/wallets/multisig_hw_segwit
I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".
So, something must still be missing.

Initially I had some dependency problems, so I deleted Cargo.lock. That solved the dependency problems, but that's the reason for the large diff on that file.

Changelog notice

Added hardware signers through the use of HWI

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@rajarshimaitra

Copy link
Copy Markdown
Contributor

Concept ACK.. Thanks for adding this.. I was just thinking about this the other day.. Could be useful if anyone wanna use bdk-cli with mainnet (still not recommended)..

I can create and sign a transaction with this version of bdk-cli. It displays the details on the Trezor and everything looks good so far. But in the CLI output of bdk-cli it says "is_finalized= false" and when I open the supposedly signed tx in Electrum, it aslo says "Status: Unsigned".

From an initial glance, it looks like all the wallets you are using are watch-only? It has Xpubs and addresses only.. Such wallets in bdk can't sign for transactions as they don't have the private key.. and is_finalised=false means the resulting psbt doesn't have signatures.

PS: We should throw an error in those cases if use asks to sign a bdk-cli with watch only descriptors.

Can you try testing with a single sig wallet, with keys generated from bdk-cli itself?? If the HWI part is working properly that should work..

I will also try to test this out in an emulator..

@rajarshimaitra

rajarshimaitra commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

I think the elctrum error you are seeing is also part of our lack of error handling in unsigned psbts and should be fixed with #128.

But it only handles it while broadcast.. I will try to see how the watch-only case can be captured in signing also..

@ulrichard

Copy link
Copy Markdown
ContributorAuthor

I tested with single sig hw wallets now.
With the Trezor everything works nicely.
With the Ledger, I got the descriptors, but when I tried to sign, I got an IO error "failed to open device". Anyway, since it works with the Trezor, I'm sure it has nothing to do with the bdk-cli integration.

@ulrichard
ulrichard marked this pull request as ready for review January 10, 2023 14:32
@ulrichard
ulrichardforce-pushed the feature/hwi branch 2 times, most recently from 816b8cc to bd2b66eCompareJanuary 10, 2023 14:45
@notmandatorynotmandatory added the enhancement New feature or request label Jan 10, 2023

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review ACK bd2b66e

Few more changes and it would be good to go..

Also seems to have conflict with master..

I have successfully tried it out with a single sig coldcard..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recently changed our changelog system from this file to just include it in the PR description.

The new PR template is here.. https://github.com/bitcoindevkit/bdk-cli/blob/master/.github/pull_request_template.md

Can you just add a Change Log section with this line in the description instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

Comment threadsrc/handlers.rs
Comment threadsrc/utils.rs Outdated
Comment threadsrc/utils.rs Outdated

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re Ack 927b066 modulo one comment..

Comment threadCHANGELOG.md Outdated
Comment on lines +9 to +10
- Added hardware signers through the use of HWI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to clarify after adding the log in the description, its actually not needed anymore in this file..

@notmandatory

notmandatory commented Feb 14, 2023

Copy link
Copy Markdown
Member

@ulrichard we made some CI changes and bumped the bdk and bdk-reserves versions, please rebase after #143 is merged and then this should be ready to go. Thanks!

@notmandatorynotmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

utACK 100ea8d

@rajarshimaitrarajarshimaitra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rajarshimaitra
rajarshimaitra merged commit e3d09a6 into bitcoindevkit:masterMar 8, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants

@ulrichard@rajarshimaitra@notmandatory