feat(core/ui): T3T1 success screens between shares #3992

obrusvit · 2024-07-02T20:08:35Z

Changes the instruction/success screens between shares during multi-share (shamir) recovery. It adds the information and also the menu button with an option to "Cancel recovery" or "Cancel backup check".

In case this flow is called between shares, the "Cancel recovery/backup check" button takes to a "long" cancel with explanation and "Tap to confirm" screen. This is not true for the first call of this flow (before any progress is made) where "Cance" button immediately cancels the flow.

In addition, this PR adds ability to rust components to send ButtonRequest repeatedly on every Event::Attach (7be9ed6).

Refactoring was needed in core/src/trezor/ui/layouts/mercury/recovery.py::homescreen_dialog. Namely, the "flow" logic was moved deeper into the layouts of individual models. This results in basically duplication of continue_recovery function in model_t and model_r, but for model_mercury, the function only awaits the new flow_continue_recovery.
The homescreen_dialog is a persistent workflow which endures device restart, that's why the repeated ButtonRequest is necessary.

TODO:

UI test cancel between shares
consider styling it as "Success" screen or using a success screen before the flow
-> kept as is for now

github-actions · 2024-07-02T20:38:20Z

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

obrusvit · 2024-07-03T07:39:46Z

On the 2nd thought, the new screens do not have the ability to cancel the flow, as discussed on Slack.
WDYT @Hannsek

Hannsek · 2024-07-03T10:49:23Z

Ah, yes, forgot about this. Let's wait with the merge, we do not rush anywhere now.

Hannsek · 2024-07-13T14:44:39Z

Maybe should be rebased on #4023 ?

The contribution will be reworked.

mmilata · 2024-07-30T00:36:57Z

This is necessary in recovery homescreen flow where we want to send ButtonRequest also after restarting the device.

Restarting device sounds like something where bootloader is involved but this is not the case right? Perhaps change "restarting the device" to "restarting python main loop"?

UI tests would be great but even without them this is big improvement. I still need to carefully review the python control flow changes but preliminary LGTM.

Hannsek · 2024-07-30T06:46:03Z

@obrusvit Please show the whole flow all the changes to @lapohoda so he can change it in Figma.

mmilata

Code-wise LGTM, did not test.

mmilata · 2024-07-30T14:08:45Z

core/embed/rust/src/ui/model_mercury/flow/continue_recovery.rs

+        match (self, msg) {
+            (Self::Main, FlowMsg::Info) => Self::Menu.transit(),
+            (Self::Menu, FlowMsg::Choice(0)) => Self::CancelIntro.swipe_left(),
+            (Self::Menu, FlowMsg::Cancelled) => Self::Main.swipe_right(),


This is gonna send ButtonRequest too right? Is Suite OK with it?

Yes, it will send BR too.

Yes, I think Suite should be fine with it because sending BRs behaves in the same way as in the old implementation of homescreen_dialog, i.e. the success/instruction screen sends BR and the confirmation of flow cancellation also sends BR. These two screens are called in a loop (until you confirm the success/instruction screen) so they possibly send the BRs repeatedly. Anyway, I'll ask QA for thorough testing of these scenarios.

mmilata · 2024-07-30T14:50:25Z

core/src/trezor/ui/layouts/mercury/recovery.py

+    # TODO: info_func should be changed to return data to be shown (and not show
+    # them) so that individual models can implement showing logic on their own.
+    # T3T1 should move the data to `flow_continue_recovery` and hide them
+    # in the context menu


So on T3T1 there's currently no way to show the remaining shares, also trezorui2.show_remaining_shares is never called, right?

I'd turn this comment into an issue since it sounds like a regression to me (minor as it only affects slip39 advanced).

Yes, currently we never use show_remaining_shares. I'd like to use it also for regular shamir. But not for this release.

I added it here: #3748

Btw. it's not used even on TS3... space-wise, it's more challening

obrusvit · 2024-07-30T22:13:30Z

@lapohoda @Hannsek This new test shows the following scenario:

recovery of multi-share backup
you enter first share (20 words), then you see this instruction screen which is shown also if you turn the device off and then on again
you decide to cancel the recovery process

See the whole flow here: https://data.trezor.io/dev/firmware/ui_report/10164723283/T3T1-en-core_device_test/new/T3T1_en-device_tests-reset_recovery-test_recovery_slip39_basic.py%3A%3Atest_abort_between_shares.html

Changes the content and visual appearance of the screens between shares during multi-share (shamir) recovery. Context menu with the option to cancel is added to the screen.

This commit allows creating a rust flow in which a component sends the configured ButtonRequest repeatedly on every Event::Attach. This is necessary in recovery homescreen flow where we want to send ButtonRequest also after restarting the device. [no changelog]

[no changelog]

Update as a part of T3T1 recovery share success screens and adding a new test of aborting the recovery process between shares for all models. [no changelog]

github-actions · 2024-07-31T08:40:21Z

legacy UI changes	device test(screens) main(screens)

obrusvit · 2024-07-31T08:42:19Z

Rebased on main.

I expect conflicts in fr lang fixtures.json tests because of the conflicts with #4071
Edit: seems like conflicts were resolved correctly.

bosomt · 2024-08-01T13:10:43Z

Did this change made it to freeze ?
This is what i see when recovering 2 of 2 old shamir seed

Device: Trezor T3T1 2.8.1 regular (revision 55473a0)

bosomt · 2024-08-02T05:35:26Z

QA OK

firmware-T3T1-2.8.1-5bba2e0aa.bin

bosomt · 2024-08-02T12:41:23Z

QA OK

T3T1

firmware-T3T1-2.8.1-5bba2e0aa.bin

first round

recovered one part of super shamir

reconnected

reconnected
cancelled recovery cancellation
cancelled recovery

second round

standard shamir
started recovery and cancelled recovery
web suite reported correctly that it was cancelled
cancelled cancellation of recovery after entered one share
reconnected device
finished recovery by entering 2of3 share
suite reported recovery as success

third round

1 of 5 entered
reconnect
cancelled recovery
suite reported that recovery was cancelled
2 of 5 entered
cancelled cancellation multiple times
3 of 5 entered /enough to recover/
disconnected device during success screen
suite reports recovery success after reconnect

T2T1

firmware-T2T1-2.8.1-5bba2e0aa.bin

first round

start recovery and disconnect device
device and suite is tarted in recovery mode
1 of 2 shares entered
reported on device: 1 of shares entereed + 1 more share needed
reconnect device
disconnected during entering second share
cancelled cancellation of recovery
cancelled recovery
recovered 2of2
disconnected during success screen
reovery went as expected

obrusvit · 2024-08-06T08:37:36Z

@bosomt thank you

obrusvit added the T3T1 Trezor Safe 5 label Jul 2, 2024

obrusvit self-assigned this Jul 2, 2024

obrusvit requested review from matejcik and prusnak as code owners July 2, 2024 20:08

obrusvit requested review from mmilata and removed request for prusnak July 2, 2024 20:08

mmilata added the translations Put this label on a PR to run tests in all languages label Jul 2, 2024

mmilata previously approved these changes Jul 2, 2024

View reviewed changes

obrusvit added the blocked Blocked by external force. Third party inputs required. label Jul 3, 2024

obrusvit marked this pull request as draft July 26, 2024 14:59

obrusvit added this to the T3T1 milestone Jul 26, 2024

obrusvit force-pushed the obrusvit/ui-t3t1-recovery-success-screens branch 3 times, most recently from 4f30ef8 to 63602b6 Compare July 28, 2024 15:15

obrusvit removed blocked Blocked by external force. Third party inputs required. translations Put this label on a PR to run tests in all languages labels Jul 28, 2024

obrusvit force-pushed the obrusvit/ui-t3t1-recovery-success-screens branch 3 times, most recently from c5b236e to c491ea4 Compare July 29, 2024 12:41

obrusvit marked this pull request as ready for review July 29, 2024 15:44

mmilata approved these changes Jul 30, 2024

View reviewed changes

obrusvit added the translations Put this label on a PR to run tests in all languages label Jul 30, 2024

obrusvit added 6 commits July 31, 2024 10:37

feat(core/ui): T3T1 instruction screens between shares

cacf44d

Changes the content and visual appearance of the screens between shares during multi-share (shamir) recovery. Context menu with the option to cancel is added to the screen.

feat(core/ui): send BR from rust during recovery

afa843b

[no changelog]

tests: fix input_flow of recovery abort

13bd3a2

[no changelog]

tests: UI test of cancel recovery between shares

c5f3a19

[no changelog]

chore(core): update fixtures.json

f94d6be

Update as a part of T3T1 recovery share success screens and adding a new test of aborting the recovery process between shares for all models. [no changelog]

obrusvit force-pushed the obrusvit/ui-t3t1-recovery-success-screens branch from fc2b0f4 to f94d6be Compare July 31, 2024 08:39

obrusvit merged commit ca46978 into main Jul 31, 2024
120 checks passed

obrusvit deleted the obrusvit/ui-t3t1-recovery-success-screens branch July 31, 2024 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core/ui): T3T1 success screens between shares #3992

feat(core/ui): T3T1 success screens between shares #3992

obrusvit commented Jul 2, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

obrusvit commented Jul 3, 2024

Hannsek commented Jul 3, 2024

Hannsek commented Jul 13, 2024

mmilata commented Jul 30, 2024

Hannsek commented Jul 30, 2024

mmilata left a comment

mmilata Jul 30, 2024

obrusvit Jul 30, 2024

mmilata Jul 30, 2024

obrusvit Jul 30, 2024

obrusvit Jul 30, 2024

obrusvit commented Jul 30, 2024

github-actions bot commented Jul 31, 2024

obrusvit commented Jul 31, 2024 •

edited

Loading

bosomt commented Aug 1, 2024

bosomt commented Aug 2, 2024

bosomt commented Aug 2, 2024 •

edited

Loading

obrusvit commented Aug 6, 2024

feat(core/ui): T3T1 success screens between shares #3992

feat(core/ui): T3T1 success screens between shares #3992

Conversation

obrusvit commented Jul 2, 2024 • edited Loading

github-actions bot commented Jul 2, 2024 • edited Loading

obrusvit commented Jul 3, 2024

Hannsek commented Jul 3, 2024

Hannsek commented Jul 13, 2024

mmilata commented Jul 30, 2024

Hannsek commented Jul 30, 2024

mmilata left a comment

Choose a reason for hiding this comment

mmilata Jul 30, 2024

Choose a reason for hiding this comment

obrusvit Jul 30, 2024

Choose a reason for hiding this comment

mmilata Jul 30, 2024

Choose a reason for hiding this comment

obrusvit Jul 30, 2024

Choose a reason for hiding this comment

obrusvit Jul 30, 2024

Choose a reason for hiding this comment

obrusvit commented Jul 30, 2024

github-actions bot commented Jul 31, 2024

obrusvit commented Jul 31, 2024 • edited Loading

bosomt commented Aug 1, 2024

bosomt commented Aug 2, 2024

bosomt commented Aug 2, 2024 • edited Loading

T3T1

first round

second round

third round

T2T1

first round

obrusvit commented Aug 6, 2024

obrusvit commented Jul 2, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

obrusvit commented Jul 31, 2024 •

edited

Loading

bosomt commented Aug 2, 2024 •

edited

Loading