Resolves issue 621 - Verify Address for exported Xpub Option #629

fedebuyito · 2024-11-13T01:28:10Z

Description

Because workflow of exportpubkey would must to be securely closed verifying addresses obtained on wallet software (Sparrow, for instance), issue reported #621 is conceptual consistent.

This pull request is categorized as a:

Checklist

I’ve run pytest and made sure all unit tests pass before sumbitting the PR. They was updated, too.

If you modified or added functionality/workflow, did you add new unit tests?

No, I’m a fool
Yes
N/A

I have tested this PR on the following platforms/os:

Raspberry Pi OS Manual Build
SeedSigner OS on a Pi0/Pi0W board
Other (enteropositivo emulator), new workflow was probed with sparrow wallet for segwit, nested and legacy scripts (taproot was not implemented)

Fixes #621
Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

Changing KEY_BACKSPACE size on ToolsDiceEntropy and ToolsCoinFlip screens

Missed import sentence for commands added on last commit

This reverts commit 7c26811.

This reverts commit e8b0e35.

On class SeedExportXpubQRDisplayView(View): Replaces "Destination" return call from MainMenuView to ScanAddressView. Rest of changes are for passing seed_num between class calling.

Changes for passing seed_num between class calling (for ScanAddressView class).

Test for seed views workflow affected.

fedebuyito · 2024-12-20T00:35:45Z

Hi. Here, local pytest runned on 3.10 / 3.12 and video capture of workflow on Seedsigner Emulator (with loop backed camera for Sparrow interaction) for signet/segwit addresses:

Tested with same results on my pi0-seedsigner device, too.

Best Regards. -

kdmukai · 2024-12-20T03:27:20Z

Very cool.

But I would suggest an intermediate screen. We have to explain what the second camera live preview is expecting to scan.

xpub QR export.
Intermediate screen prompt: "Verify New Wallet?"; brief 1 sentence explanation of why / how(?); buttons:
- [QR icon] Verify an address
- Done
Live camera preview to scan addr to verify.

fedebuyito · 2024-12-20T08:08:11Z

Thanks for your feedback, Keith. I will try, that makes sense.

Added SeedExportXpubQRAskVerifyAddView(View) class for intermediate screen asking for verifying address or done action after exporting Xpub

Added FlowStep(seed_views.SeedExportXpubQRAskVerifyAddView, screen_return_value=0) to seed flow tests.

fedebuyito · 2024-12-21T02:19:59Z

Do you think this way would be fit? (I updated tests too, because of the new screen)

Added AdviceScreen class

Resolves conflicts from previous merger

Added AdviceScreen class.

Resolves conflicts from previous merger

Added FlowStep(seed_views.SeedExportXpubQRAskVerifyAddView, screen_return_value=0) to seed flow tests.

Added new Screenshot for SeedExportXpubQRAskVerifyAddView

fedebuyito · 2024-12-29T21:10:14Z

Code adapted in order to resolve conflicts from last massive merge (PR Initial l10n / multilanguage support #620 ), including workflow test.
Revised view text and icon for better understanding.
Tested on local for 3.12 and 3.10, and pi0 manual build.
Added new view to screenshot generator (tests passed, too).
Did not try with multi language because is out of scope of this issue

jdlcdl · 2024-12-30T11:42:22Z

Reviewed and tried this pr at 2047506

This is nice functionality. I like that it helps teach users to verify their watch-only wallet setup.

Perhaps improved wording on the new Advice screen:
A better title might be "Wallet Verification". Text might be "Scan a wallet address to verify the exported xpub?". Button might be "Scan an address".
...once done, to make suggestions to translators/transifex for new strings in Spanish.
Since multisig cannot work with a single xpub (though this pr does check and succeed address verifications), it might only be relevant to, and therefore available, after exporting single-sig xpubs. It's not expected that the user would have access to the full descriptor until all bip32 xpubs have been exported... at that time, an advanced multisig user should know to verify their wallet setup with a complete wallet descriptor.

fedebuyito · 2024-12-30T16:32:04Z

Hi @jdlcdl . Thanks for reviewing and comments.

Sounds good that wording improvement. Note the headline with text "Check new wallet", below icon, is it useful? would you change it?

About this,

* Since multisig cannot work with a single xpub (though this pr does check and succeed address verifications), it might only be relevant to, and therefore available, after exporting single-sig xpubs.  It's not expected that the user would have access to the full descriptor until all bip32 xpubs have been exported... at that time, an advanced multisig user should know to verify their wallet setup with a complete wallet descriptor.

I agree, and think I'm almost there.

jdlcdl · 2024-12-31T19:57:04Z

Just noticing that this pr raises an error if an address is scanned before any seed has been loaded. What is expected instead is to land at the "load a seed" view so that user can load a seed in various ways.

Error looks like:

System Error
(!)
AttributeError
scan_views.py, 136, in run
'ScanView' object has no attribute 'seed_num'
(I Understand)

Will leave a marker in code comment if I see it.

Otherwise, things are a moving target lately, we'll catch all the bugs before merge/release.
Have a safe and wonderful end-of-year celebration @fedebuyito. All the best for 2025!

jdlcdl · 2024-12-31T20:03:10Z

src/seedsigner/views/scan_views.py

@@ -133,6 +133,8 @@ def run(self):
                from seedsigner.views.seed_views import AddressVerificationStartView
                address = self.decoder.get_address()
                (script_type, network) = self.decoder.get_address_type()
+                seed_num = self.seed_num


Cannot always be assumed that object has .seed_num, sometimes user will scan an address first, afterwards should be prompted to load a seed.

Fixed on 1e63527 commit.

only includes "seed_num" when this exists.

Only ask for verification xpub address option when SINGLE_SIG

Solves view wording

Fixes test flows regarding ask xpub address verification only when SINGLE_SIG

Resolves missing member declaration

fedebuyito · 2025-01-02T00:18:23Z

Just noticing that this pr raises an error if an address is scanned before any seed has been loaded. What is expected instead is to land at the "load a seed" view so that user can load a seed in various ways.

Error looks like:
System Error
(!)
AttributeError
scan_views.py, 136, in run
'ScanView' object has no attribute 'seed_num'
(I Understand)
Will leave a marker in code comment if I see it.

Otherwise, things are a moving target lately, we'll catch all the bugs before merge/release. Have a safe and wonderful end-of-year celebration @fedebuyito. All the best for 2025!

Thank you! I was not realized that there are two ways to scan an address before load a seed (Tools/Verify and SCAN directly, last I did catch it, curiously the most obvious). I logged the fix on marked commented code.

I have solved you suggested about not to ask xpub address verification when multi-signature on workflow, updating workflow test, too (all pytested and emulator tested). On screen wording I left without that yellow words but I read your opinion, of course, so you can tell me what you think is better.

@jdlcdl Nice to meet you on passed 2024 and thanks again for your kindly reviews and all the best for you too right now! 2025 will rock!

fedebuyito added 7 commits September 6, 2024 14:39

Update tools_screens.py

e8b0e35

Changing KEY_BACKSPACE size on ToolsDiceEntropy and ToolsCoinFlip screens

Update tools_screens.py

7c26811

Missed import sentence for commands added on last commit

Revert "Update tools_screens.py"

8bc985f

This reverts commit 7c26811.

Revert "Update tools_screens.py"

ef6e448

This reverts commit e8b0e35.

Update seed_views.py

3278ecd

On class SeedExportXpubQRDisplayView(View): Replaces "Destination" return call from MainMenuView to ScanAddressView. Rest of changes are for passing seed_num between class calling.

Update scan_views.py

22f2104

Changes for passing seed_num between class calling (for ScanAddressView class).

Update test_flows_seed.py

0798224

Test for seed views workflow affected.

fedebuyito changed the title ~~Resolves issue 621~~ Resolves issue 621- Verify Address for exported Xpub Option Dec 21, 2024

fedebuyito added 2 commits December 21, 2024 02:06

Update seed_views.py

b8b2674

Added SeedExportXpubQRAskVerifyAddView(View) class for intermediate screen asking for verifying address or done action after exporting Xpub

Update test_flows_seed.py

94fef88

Added FlowStep(seed_views.SeedExportXpubQRAskVerifyAddView, screen_return_value=0) to seed flow tests.

fedebuyito added 12 commits December 29, 2024 20:13

Update screen.py

f494b5e

Added AdviceScreen class

Update scan_views.py

2f3d2b2

Resolves conflicts from previous merger

Update seed_views.py

6731294

Resolves conflicts from previous merger

Update scan_views.py

fde013b

Resolves conflicts from previous merger

Update seed_views.py

3ae1bf0

Resolves conflicts from previous merger

Update screen.py

a4c786f

Resolves conflicts from previous merger

Update screen.py

3a38f1e

Added AdviceScreen class.

Merge branch 'dev' into resolves_issue_621

d75fd37

Update scan_views.py

3200dc5

Resolves conflicts from previous merger

Update seed_views.py

f728d7b

Resolves conflicts from previous merger

Update test_flows_seed.py

200b9a1

Added FlowStep(seed_views.SeedExportXpubQRAskVerifyAddView, screen_return_value=0) to seed flow tests.

Update generator.py

2047506

Added new Screenshot for SeedExportXpubQRAskVerifyAddView

jdlcdl reviewed Dec 31, 2024

View reviewed changes

fedebuyito added 5 commits January 1, 2025 22:53

Update scan_views.py

1e63527

only includes "seed_num" when this exists.

Update seed_views.py

c86c87d

Only ask for verification xpub address option when SINGLE_SIG

Update seed_views.py

e23a24f

Solves view wording

Update test_flows_seed.py

6266bd9

Fixes test flows regarding ask xpub address verification only when SINGLE_SIG

Update seed_views.py

37ea97d

Resolves missing member declaration

fedebuyito changed the title ~~Resolves issue 621- Verify Address for exported Xpub Option~~ Resolves issue 621 - Verify Address for exported Xpub Option Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolves issue 621 - Verify Address for exported Xpub Option #629

Resolves issue 621 - Verify Address for exported Xpub Option #629

fedebuyito commented Nov 13, 2024 •

edited

Loading

fedebuyito commented Dec 20, 2024

kdmukai commented Dec 20, 2024

fedebuyito commented Dec 20, 2024

fedebuyito commented Dec 21, 2024

fedebuyito commented Dec 29, 2024

jdlcdl commented Dec 30, 2024

fedebuyito commented Dec 30, 2024

jdlcdl commented Dec 31, 2024

jdlcdl Dec 31, 2024

fedebuyito Jan 2, 2025

fedebuyito commented Jan 2, 2025

Resolves issue 621 - Verify Address for exported Xpub Option #629

Are you sure you want to change the base?

Resolves issue 621 - Verify Address for exported Xpub Option #629

Conversation

fedebuyito commented Nov 13, 2024 • edited Loading

Description

Checklist

fedebuyito commented Dec 20, 2024

kdmukai commented Dec 20, 2024

fedebuyito commented Dec 20, 2024

fedebuyito commented Dec 21, 2024

fedebuyito commented Dec 29, 2024

jdlcdl commented Dec 30, 2024

fedebuyito commented Dec 30, 2024

jdlcdl commented Dec 31, 2024

jdlcdl Dec 31, 2024

Choose a reason for hiding this comment

fedebuyito Jan 2, 2025

Choose a reason for hiding this comment

fedebuyito commented Jan 2, 2025

fedebuyito commented Nov 13, 2024 •

edited

Loading