refactor confirm_summary #4412

obrusvit · 2024-11-30T13:01:41Z

This is a refactor of a transaction summary confirmation dialog: confirm_summary. The reason is to unify the rust function signature across models. This paves the way for easier work on #3658. uPy functions were refactored accordingly. The rust functions of all three models now have these parameters:

amount
amount_label
fee
fee_label
title (Optional)
account_items (Optional) - KV pairs of "send from" info (e.g. account name, derivation paths, etc.)
extra_items (Optional) - KV pairs of whatever other info, typically used for other fee info (e.g. fee rate, gas price of ETH, etc.) or some other coin-specific info (e.g. TTL for Cardano)
extra_title (Optional) - title for extra_items
verb_cancel (Optional) - cancel button verb, can be used on TR and TT to signify whether the button is arrow up (^), meaning a user can go back one step in uPy flow

Changes:

[TT]
- confirm_total replaced by confirm_summary
- drawback: info _items are passed to rust but are actually not used there as they are used within uPy flow of with_info
[TR]
- confirm_total and altcoin_tx summary were merged to confirm_summary. The function now handles all Optional info placed one after another.
[Mercury]
- flow_confirm_summary replaced by confirm_summary
- old confirm_total removed - it wasn't used

UI diff:

[TR]
- info screens now use vertically_centered() instead of manual .newline(), lots of negligible diffs
- ℹ️ not accessible when not provided
- in some cases, titles in ℹ️ are fixed
- in some cases we have less pagination, and in a few cases we have more pagination.

github-actions · 2024-11-30T13:01:57Z

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

github-actions · 2024-11-30T13:02:01Z

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)

ibz

Beautiful refactor!

Some comments added.

ibz · 2024-12-02T11:40:21Z

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

-        theme::ICON_CANCEL,
-        cancel_text.unwrap_or(TR::send__cancel_sign.into()),
-    );
+    if content_extra.is_some() && menu_items.len() < 2 {


I don't really like the hardcoded "2" here. Ideally it would be somehow computed based on the screen size, but a big step forward (probably good enough) would be a constant in vertical_menu.rs - something like MAX_ITEMS_ON_SCREEN.

ibz · 2024-12-02T11:49:18Z

core/embed/rust/src/ui/model_tr/layout.rs

+            .get(Qstr::MP_QSTR_extra_items)
+            .unwrap_or_else(|_| Obj::const_none())
+            .try_into_option()?;
+        let cancel_arrow: bool = kwargs.get_or(Qstr::MP_QSTR_cancel_arrow, false)?;


I think that on model R it should be verb_cancel = "^" versus verb_cancel = "" to mark the "up arrow" versus the "x" icon. I've seen that used in other places. Introducing the concept of "cancel_arrow" seems wrong, not just because it's unclear what it means, but also because it is not portable across models.

Both TT and TR either uses "x" or "^" as the cancel button so that you can use "^" if the summary is a part of uPy flow.

TT uses cancel_arrow now and it used it before.
TR used cancel_cross before in altcoin_tx_summary which was basically the opposite of what cancel_arrow does. So I unified it.
Lincoln will probably need it as well given current designs.
Mercury does not need it but maybe it will if the designs change in the future based on Lincoln.

Having verb_cancel there might make more sense. Let me try.

I think you were right. Now, the param is even usable on mercury (but unused from the uPy layer the moment). See:

TT e7c5bc2

TR f9c897d

Mercury b1850b9

made a mistake for TR, fixed here: 61448aa

matejcik · 2024-12-02T14:21:10Z

why make fee_items and extra_items separate?

obrusvit · 2024-12-02T14:29:16Z

why make fee_items and extra_items separate?

@matejcik fee_items is used for fee specific information, whereas extra_items serves for whatever other info which do not suit in the other 2. Some altcoins might hide some info here (e.g. "Cardano" stuff like TTL etc.)
We could make "fee informations" be a part of "extra information" but separating them makes for easy setting the titles of the info dialogs ("Fee info" vs. "More info" in TR) and the buttons (VerticalMenu of mercury).

EDIT: we agreed to change to just account_info and extra_info

- model_t confirm_total refactored to confirm_summary - parameter set changed to pave the way for unification across models [no changelog]

matejcik · 2024-12-04T11:45:57Z

code ACK, waiting for device tests before approving

- altcoin_tx_summary removed and replaced with confirm_summary [no changelog]

- old confirm_total removed - flow_confirm_summary refactored to confirm_summary [no changelog]

obrusvit added core Trezor Core firmware. Runs on Trezor Model T and T2B1. code Code improvements labels Nov 30, 2024

obrusvit self-assigned this Nov 30, 2024

obrusvit force-pushed the obrusvit/refactor-confirm-summary branch from 41daa08 to ff7f6ea Compare November 30, 2024 22:51

obrusvit marked this pull request as ready for review November 30, 2024 22:51

obrusvit requested review from matejcik and prusnak as code owners November 30, 2024 22:51

obrusvit requested a review from ibz November 30, 2024 22:51

obrusvit changed the title ~~[draft] refactor confirm_summary~~ refactor confirm_summary Nov 30, 2024

ibz reviewed Dec 2, 2024

View reviewed changes

ibz approved these changes Dec 3, 2024

View reviewed changes

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

refactor(core): model_t confirm_summary

d0f11ea

- model_t confirm_total refactored to confirm_summary - parameter set changed to pave the way for unification across models [no changelog]

obrusvit force-pushed the obrusvit/refactor-confirm-summary branch from 6a25ea7 to f57d045 Compare December 4, 2024 10:38

matejcik approved these changes Dec 4, 2024

View reviewed changes

obrusvit added 2 commits December 4, 2024 13:50

refactor(core): model_r confirm_summary

c819339

- altcoin_tx_summary removed and replaced with confirm_summary [no changelog]

refactor(core): mercury confirm_summary

69eab66

- old confirm_total removed - flow_confirm_summary refactored to confirm_summary [no changelog]

obrusvit force-pushed the obrusvit/refactor-confirm-summary branch from f57d045 to 69eab66 Compare December 4, 2024 12:51

obrusvit merged commit 61ebb19 into main Dec 4, 2024
139 checks passed

obrusvit deleted the obrusvit/refactor-confirm-summary branch December 4, 2024 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor confirm_summary #4412

refactor confirm_summary #4412

obrusvit commented Nov 30, 2024 •

edited

Loading

github-actions bot commented Nov 30, 2024 •

edited

Loading

github-actions bot commented Nov 30, 2024 •

edited

Loading

ibz left a comment

ibz Dec 2, 2024

obrusvit Dec 2, 2024

ibz Dec 2, 2024

obrusvit Dec 2, 2024

obrusvit Dec 2, 2024

obrusvit Dec 2, 2024

matejcik commented Dec 2, 2024

obrusvit commented Dec 2, 2024 •

edited

Loading

matejcik commented Dec 4, 2024

refactor confirm_summary #4412

refactor confirm_summary #4412

Conversation

obrusvit commented Nov 30, 2024 • edited Loading

github-actions bot commented Nov 30, 2024 • edited Loading

github-actions bot commented Nov 30, 2024 • edited Loading

ibz left a comment

Choose a reason for hiding this comment

ibz Dec 2, 2024

Choose a reason for hiding this comment

obrusvit Dec 2, 2024

Choose a reason for hiding this comment

ibz Dec 2, 2024

Choose a reason for hiding this comment

obrusvit Dec 2, 2024

Choose a reason for hiding this comment

obrusvit Dec 2, 2024

Choose a reason for hiding this comment

obrusvit Dec 2, 2024

Choose a reason for hiding this comment

matejcik commented Dec 2, 2024

obrusvit commented Dec 2, 2024 • edited Loading

matejcik commented Dec 4, 2024

obrusvit commented Nov 30, 2024 •

edited

Loading

github-actions bot commented Nov 30, 2024 •

edited

Loading

github-actions bot commented Nov 30, 2024 •

edited

Loading

obrusvit commented Dec 2, 2024 •

edited

Loading