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

Feature: Optimized uncle jim pdf for the Debasafebags. #1706

Merged
merged 9 commits into from
May 17, 2022
Merged

Feature: Optimized uncle jim pdf for the Debasafebags. #1706

merged 9 commits into from
May 17, 2022

Conversation

f9ert
Copy link
Contributor

@f9ert f9ert commented May 6, 2022 •
edited
Loading

image

@netlify
Copy link

netlify bot commented May 6, 2022 •
edited
Loading

✅ Deploy Preview for specter-desktop-docs ready!

Name Link
🔨 Latest commit 1fc111a
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/62837209eda2470009bc4679
😎 Deploy Preview https://deploy-preview-1706--specter-desktop-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@moneymanolis
Copy link
Collaborator

The displayed character length of 42 in the pdf for the addresses is fine: P2WPKH addresses are always 42 characters long (for more details see here: https://bitcoin.stackexchange.com/questions/100508/can-you-break-down-what-data-is-encoded-into-a-bech32-address). With regtest addresses we are adding two characters ("rt"), thus two characters are missing then, just keep this in mind. Perhaps it makes sense to add this non-trivial background info to the tool-tip belonging to the pdf, @k9ert?

@k9ert
Copy link
Collaborator

k9ert commented May 6, 2022

Perhaps it makes sense to add this non-trivial background info to the tool-tip belonging to the pdf, @k9ert?

Great finding! Maybe it also make sense to print it directly on the pdf but just if the network is regtest. Do we also have char-loss for testnet?

@moneymanolis
Copy link
Collaborator

No, testnet is fine, too, it's just tb:

image

@moneymanolis
Copy link
Collaborator

I think I would only enable the Uncle Jim PDF feature if the node is set to mainnet, it only poses risks to send to testnet addresses (if the wallet used is not validating correctly ..., see: https://bitcoin.stackexchange.com/questions/18028/what-happens-if-you-send-btc-to-a-testnet-address)

@k9ert
Copy link
Collaborator

k9ert commented May 10, 2022

I think I would only enable the Uncle Jim PDF feature if the node is set to mainnet,

Makes development and testing out stuff a lot more complicated. Don't like the idea so much. I'll try to print it at the bottom of the page...

k9ert
k9ert reviewed May 10, 2022
View reviewed changes
src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja Outdated
Comment on lines 245 to 247
doc.setLineDash([2, 2], 0);
doc.line(181,0,181,214)
doc.line(0,214,181,214)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're drawing those lines for every key in the wallet. Please move it out of the loop!

Kim Neunert and others added 6 commits May 10, 2022 10:12
@k9ert k9ert merged commit 7472e16 into cryptoadvance:master May 17, 2022
ankur12-1610 pushed a commit to ankur12-1610/specter-desktop that referenced this pull request May 18, 2022
@f9ert @moneymanolis @ankur12-1610
Feature: Optimized uncle jim pdf for the Debasafebags. (cryptoadvance…
ef70aba 
…#1706)

* Optimized for The Debasafetueten.

* adding regtest hint

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* lines moved out of the loop

Co-authored-by: Kim Neunert <k9ert@gmx.de>
Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>
ankur12-1610 pushed a commit to ankur12-1610/specter-desktop that referenced this pull request May 18, 2022
@f9ert @moneymanolis @ankur12-1610
Feature: Optimized uncle jim pdf for the Debasafebags. (cryptoadvance…
fb252da 
…#1706)

* Optimized for The Debasafetueten.

* adding regtest hint

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* lines moved out of the loop

Co-authored-by: Kim Neunert <k9ert@gmx.de>
Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>
ankur12-1610 pushed a commit to ankur12-1610/specter-desktop that referenced this pull request May 18, 2022
@f9ert @moneymanolis @ankur12-1610
Feature: Optimized uncle jim pdf for the Debasafebags. (cryptoadvance…
7444b4c 
…#1706)

* Optimized for The Debasafetueten.

* adding regtest hint

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* lines moved out of the loop

Co-authored-by: Kim Neunert <k9ert@gmx.de>
Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>
ankur12-1610 pushed a commit to ankur12-1610/specter-desktop that referenced this pull request May 18, 2022
@f9ert @moneymanolis @ankur12-1610
Feature: Optimized uncle jim pdf for the Debasafebags. (cryptoadvance…
852e334 
…#1706)

* Optimized for The Debasafetueten.

* adding regtest hint

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* lines moved out of the loop

Co-authored-by: Kim Neunert <k9ert@gmx.de>
Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>
ankur12-1610 pushed a commit to ankur12-1610/specter-desktop that referenced this pull request Jun 2, 2022
@f9ert @moneymanolis @ankur12-1610
Feature: Optimized uncle jim pdf for the Debasafebags. (cryptoadvance…
2d2d205 
…#1706)

* Optimized for The Debasafetueten.

* adding regtest hint

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* Update src/cryptoadvance/specter/templates/wallet/components/wallet_pdf.jinja

Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>

* lines moved out of the loop

Co-authored-by: Kim Neunert <k9ert@gmx.de>
Co-authored-by: Manolis <70536101+moneymanolis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moneymanolis moneymanolis moneymanolis left review comments

@k9ert k9ert k9ert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@f9ert @moneymanolis @k9ert