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

fix(legacy): show XPUB using a QR code #4480

Merged
merged 2 commits into from
Jan 9, 2025
Merged

fix(legacy): show XPUB using a QR code #4480

merged 2 commits into from
Jan 9, 2025

Conversation

romanz
Copy link
Contributor

@romanz romanz commented Jan 8, 2025

Similar layout to fsm_layoutAddress is used.

Fixes #3043.

@romanz romanz added T1B1 legacy Trezor One bug Something isn't working as expected low hanging fruit Simple, quick task. labels Jan 8, 2025
@romanz romanz self-assigned this Jan 8, 2025
Copy link

github-actions bot commented Jan 8, 2025

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@prusnak
Copy link
Member

prusnak commented Jan 8, 2025

Do all possible XPUBs (bip-44, 49, 84, 86) fit on the device screen?

@romanz romanz force-pushed the romanz/xpub-qr-legacy branch from b080e89 to cadcda3 Compare January 8, 2025 18:18
@romanz
Copy link
Contributor Author

romanz commented Jan 8, 2025

The screenshot below is for
xpub6BiVtCpG9fQPxnPmHXG8PhtzQdWC2Su4qWu6XW9tpWFYhxydCLJGrWBJZ5H6qTAHdPQ7pQhtpjiYZVZARo14qHiay2fvrX996oEP42u8wZy BIP-44 XPUB and due to its size, it requires two pages of text.

The QR code is rendered with a higher resolution, and looks like it fits on the screen:

image

@prusnak I will make sure that we have tests covering all XPUB types.

@prusnak
Copy link
Member

prusnak commented Jan 8, 2025

The QR code is rendered with a higher resolution, and looks like it fits on the screen:

Yes, it fits. But I doubt this can be read by any camera. Even QR codes with doubled pixels are quite problematic to be read by most of the cameras from T1 display.

I am not saying we should not merge this PR, I am just saying that doing so probably does not add much value unfortunately. :-/

@romanz
Copy link
Contributor Author

romanz commented Jan 8, 2025

I understand, thanks @prusnak!

If preferable, I think that we can fix the original issue (XPUB layout being non-cancellable) using 405a1cb, WDYT?

CC: @Hannsek

@prusnak
Copy link
Member

prusnak commented Jan 8, 2025

If preferable,

No strong opinion. I am fine with both approaches. Maybe we can show the QR for consistency with GetAddress, but like I said, scanning it might be very problematic.

@Hannsek
Copy link
Contributor

Hannsek commented Jan 8, 2025

It can be read by iPhone (13) camera. Older phones might have a problem. I don't think that lot of users do that anyway. Therefore I would proceed with the pr and leave the qr there for consistency.

@prusnak
Copy link
Member

prusnak commented Jan 8, 2025

Therefore I would proceed with the pr and leave the qr there for consistency.

ACK

romanz added 2 commits January 9, 2025 11:41
Similar layout to `fsm_layoutAddress` is used.

Fixes #3043.
@romanz romanz force-pushed the romanz/xpub-qr-legacy branch from ef094a2 to 423ec24 Compare January 9, 2025 10:45
@romanz romanz merged commit 367ba7c into main Jan 9, 2025
95 checks passed
@romanz romanz deleted the romanz/xpub-qr-legacy branch January 9, 2025 11:49
@bosomt
Copy link

bosomt commented Jan 23, 2025

tested on my Nothing phone 1 with Android 15
i needed to zoom but QR code was successfully scanned 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected low hanging fruit Simple, quick task. T1B1 legacy Trezor One
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

Cannot cancel displaying public key on T1
5 participants