From 492ff958a5800f9b060d7bd1867efb00f5ab93eb Mon Sep 17 00:00:00 2001 From: grdddj Date: Tue, 8 Aug 2023 12:35:28 +0200 Subject: [PATCH] feat(core): improve sign message designs [no changelog] --- .../rust/src/ui/model_tr/component/button.rs | 47 +++++++++++--- .../src/ui/model_tr/component/show_more.rs | 8 ++- core/embed/rust/src/ui/model_tr/layout.rs | 24 +++---- core/mocks/generated/trezorui2.pyi | 3 +- core/src/trezor/ui/layouts/tr/__init__.py | 62 +++++++++++-------- core/src/trezor/ui/layouts/tt_v2/__init__.py | 2 + tests/device_tests/test_cancel.py | 9 +++ tests/input_flows.py | 15 +++++ 8 files changed, 118 insertions(+), 52 deletions(-) diff --git a/core/embed/rust/src/ui/model_tr/component/button.rs b/core/embed/rust/src/ui/model_tr/component/button.rs index 25bace9108..f8c658566b 100644 --- a/core/embed/rust/src/ui/model_tr/component/button.rs +++ b/core/embed/rust/src/ui/model_tr/component/button.rs @@ -351,7 +351,10 @@ pub struct ButtonDetails { offset: Offset, } -impl ButtonDetails { +impl ButtonDetails +where + T: StringType, +{ /// Text button. pub fn text(text: T) -> Self { Self { @@ -376,6 +379,19 @@ impl ButtonDetails { } } + /// Resolves text and finds possible icon names. + pub fn from_text_possible_icon(text: T) -> Self { + if text.as_ref() == "" { + Self::cancel_icon() + } else if text.as_ref() == "left_arrow_icon" { + Self::left_arrow_icon() + } else if text.as_ref() == "up_arrow_icon" { + Self::up_arrow_icon() + } else { + Self::text(text) + } + } + /// Text with arms signalling double press. pub fn armed_text(text: T) -> Self { Self::text(text).with_arms() @@ -529,6 +545,15 @@ where ) } + /// Left text, armed text and right info icon/text. + pub fn text_armed_info(left: T, middle: T) -> Self { + Self::new( + Some(ButtonDetails::from_text_possible_icon(left)), + Some(ButtonDetails::armed_text(middle)), + Some(ButtonDetails::text("i".into()).with_fixed_width(theme::BUTTON_ICON_WIDTH)), + ) + } + /// Left cancel, armed text and right info icon/text. pub fn cancel_armed_info(middle: T) -> Self { Self::new( @@ -559,16 +584,16 @@ where /// Left and right texts. pub fn text_none_text(left: T, right: T) -> Self { Self::new( - Some(ButtonDetails::text(left)), + Some(ButtonDetails::from_text_possible_icon(left)), None, - Some(ButtonDetails::text(right)), + Some(ButtonDetails::from_text_possible_icon(right)), ) } /// Left text and right arrow. pub fn text_none_arrow(text: T) -> Self { Self::new( - Some(ButtonDetails::text(text)), + Some(ButtonDetails::from_text_possible_icon(text)), None, Some(ButtonDetails::right_arrow_icon()), ) @@ -577,7 +602,7 @@ where /// Left text and WIDE right arrow. pub fn text_none_arrow_wide(text: T) -> Self { Self::new( - Some(ButtonDetails::text(text)), + Some(ButtonDetails::from_text_possible_icon(text)), None, Some(ButtonDetails::down_arrow_icon_wide()), ) @@ -585,7 +610,11 @@ where /// Only right text. pub fn none_none_text(text: T) -> Self { - Self::new(None, None, Some(ButtonDetails::text(text))) + Self::new( + None, + None, + Some(ButtonDetails::from_text_possible_icon(text)), + ) } /// Left and right arrow icons for navigation. @@ -602,7 +631,7 @@ where Self::new( Some(ButtonDetails::left_arrow_icon()), None, - Some(ButtonDetails::text(text)), + Some(ButtonDetails::from_text_possible_icon(text)), ) } @@ -611,7 +640,7 @@ where Self::new( Some(ButtonDetails::up_arrow_icon()), None, - Some(ButtonDetails::text(text)), + Some(ButtonDetails::from_text_possible_icon(text)), ) } @@ -656,7 +685,7 @@ where Self::new( Some(ButtonDetails::cancel_icon()), None, - Some(ButtonDetails::text(text)), + Some(ButtonDetails::from_text_possible_icon(text)), ) } diff --git a/core/embed/rust/src/ui/model_tr/component/show_more.rs b/core/embed/rust/src/ui/model_tr/component/show_more.rs index cca56bf7ed..360f1d6903 100644 --- a/core/embed/rust/src/ui/model_tr/component/show_more.rs +++ b/core/embed/rust/src/ui/model_tr/component/show_more.rs @@ -27,8 +27,12 @@ where T: Component, U: StringType + Clone, { - pub fn new(content: T) -> Self { - let btn_layout = ButtonLayout::cancel_armed_info("CONFIRM".into()); + pub fn new(content: T, cancel_button: Option, button: U) -> Self { + let btn_layout = if let Some(cancel_text) = cancel_button { + ButtonLayout::text_armed_info(cancel_text, button) + } else { + ButtonLayout::cancel_armed_info(button) + }; Self { content: Child::new(content), buttons: Child::new(ButtonController::new(btn_layout)), diff --git a/core/embed/rust/src/ui/model_tr/layout.rs b/core/embed/rust/src/ui/model_tr/layout.rs index 9ec31f2ed1..09344d4731 100644 --- a/core/embed/rust/src/ui/model_tr/layout.rs +++ b/core/embed/rust/src/ui/model_tr/layout.rs @@ -254,19 +254,7 @@ fn content_in_button_page( hold: bool, ) -> Result { // Left button - icon, text or nothing. - let cancel_btn = if let Some(verb_cancel) = verb_cancel { - if !verb_cancel.is_empty() { - if verb_cancel.as_ref() == "left_arrow_icon" { - Some(ButtonDetails::left_arrow_icon()) - } else { - Some(ButtonDetails::text(verb_cancel)) - } - } else { - Some(ButtonDetails::cancel_icon()) - } - } else { - None - }; + let cancel_btn = verb_cancel.map(ButtonDetails::from_text_possible_icon); // Right button - text or nothing. // Optional HoldToConfirm @@ -1060,6 +1048,11 @@ extern "C" fn new_show_mismatch() -> Obj { extern "C" fn new_confirm_with_info(n_args: usize, args: *const Obj, kwargs: *mut Map) -> Obj { let block = move |_args: &[Obj], kwargs: &Map| { let title: StrBuffer = kwargs.get(Qstr::MP_QSTR_title)?.try_into()?; + let button: StrBuffer = kwargs.get(Qstr::MP_QSTR_button)?.try_into()?; + let verb_cancel: Option = kwargs + .get(Qstr::MP_QSTR_verb_cancel) + .unwrap_or_else(|_| Obj::const_none()) + .try_into_option()?; let items: Obj = kwargs.get(Qstr::MP_QSTR_items)?; let mut paragraphs = ParagraphVecShort::new(); @@ -1078,6 +1071,8 @@ extern "C" fn new_confirm_with_info(n_args: usize, args: *const Obj, kwargs: *mu title, ShowMore::>, StrBuffer>::new( paragraphs.into_paragraphs(), + verb_cancel, + button, ), ))?; Ok(obj.into()) @@ -1670,9 +1665,10 @@ pub static mp_module_trezorui2: Module = obj_module! { /// def confirm_with_info( /// *, /// title: str, - /// button: str, # unused on TR + /// button: str, /// info_button: str, # unused on TR /// items: Iterable[Tuple[int, str]], + /// verb_cancel: str | None = None, /// ) -> object: /// """Confirm given items but with third button. Always single page /// without scrolling.""" diff --git a/core/mocks/generated/trezorui2.pyi b/core/mocks/generated/trezorui2.pyi index c2ff8b7790..3622affb55 100644 --- a/core/mocks/generated/trezorui2.pyi +++ b/core/mocks/generated/trezorui2.pyi @@ -223,9 +223,10 @@ def show_mismatch() -> object: def confirm_with_info( *, title: str, - button: str, # unused on TR + button: str, info_button: str, # unused on TR items: Iterable[Tuple[int, str]], + verb_cancel: str | None = None, ) -> object: """Confirm given items but with third button. Always single page without scrolling.""" diff --git a/core/src/trezor/ui/layouts/tr/__init__.py b/core/src/trezor/ui/layouts/tr/__init__.py index f776fbede8..0f48486072 100644 --- a/core/src/trezor/ui/layouts/tr/__init__.py +++ b/core/src/trezor/ui/layouts/tr/__init__.py @@ -711,6 +711,7 @@ async def should_show_more( br_type: str = "should_show_more", br_code: ButtonRequestType = BR_TYPE_OTHER, confirm: str | bytes | None = None, + verb_cancel: str | None = None, ) -> bool: """Return True if the user wants to show more (they click a special button) and False when the user wants to continue without showing details. @@ -726,7 +727,8 @@ async def should_show_more( title=title.upper(), items=para, button=confirm.upper(), - info_button=button_text.upper(), + verb_cancel=verb_cancel, # type: ignore [No parameter named "verb_cancel"] + info_button=button_text.upper(), # unused on TR ) ), br_type, @@ -747,6 +749,8 @@ async def confirm_blob( title: str, data: bytes | str, description: str | None = None, + verb: str = "CONFIRM", + verb_cancel: str | None = "", # icon hold: bool = False, br_code: ButtonRequestType = BR_TYPE_OTHER, ask_pagination: bool = False, @@ -759,15 +763,17 @@ async def confirm_blob( description=description, data=data, extra=None, - verb_cancel="", # to show the cancel icon + verb=verb, + verb_cancel=verb_cancel, hold=hold, - verb_cancel="", # icon ) ) if ask_pagination and layout.page_count() > 1: assert not hold - await _confirm_ask_pagination(br_type, title, data, description, br_code) + await _confirm_ask_pagination( + br_type, title, data, description, verb_cancel, br_code + ) else: await raise_if_not_confirmed( @@ -784,6 +790,7 @@ async def _confirm_ask_pagination( title: str, data: bytes | str, description: str, + verb_cancel: str | None, br_code: ButtonRequestType, ) -> None: paginated: ui.Layout | None = None @@ -796,6 +803,7 @@ async def _confirm_ask_pagination( if not await should_show_more( title, para=[(ui.NORMAL, description), (ui.MONO, data)], + verb_cancel=verb_cancel, br_type=br_type, br_code=br_code, ): @@ -806,7 +814,7 @@ async def _confirm_ask_pagination( trezorui2.confirm_more( title=title, button="GO BACK", - items=[(ui.MONO, data)], + items=[(ui.BOLD, f"Size: {len(data)} bytes"), (ui.MONO, data)], ) ) else: @@ -1089,29 +1097,31 @@ async def confirm_sign_identity( async def confirm_signverify( coin: str, message: str, address: str, verify: bool ) -> None: - if verify: - header = f"Verify {coin} message" - br_type = "verify_message" - else: - header = f"Sign {coin} message" - br_type = "sign_message" + br_type = "verify_message" if verify else "sign_message" - await confirm_blob( - br_type, - header.upper(), - address, - "Confirm address:", - br_code=BR_TYPE_OTHER, - ) + # Allowing to go back from the second screen + while True: + await confirm_blob( + br_type, + "SIGNING ADDRESS", + address, + verb="CONTINUE", + br_code=BR_TYPE_OTHER, + ) - await confirm_value( - header.upper(), - message, - "Confirm message:", - br_type, - BR_TYPE_OTHER, - verb="CONFIRM", - ) + try: + await confirm_blob( + br_type, + "CONFIRM MESSAGE", + message, + verb_cancel="up_arrow_icon", + br_code=BR_TYPE_OTHER, + ask_pagination=True, + ) + except ActionCancelled: + continue + else: + break async def show_error_popup( diff --git a/core/src/trezor/ui/layouts/tt_v2/__init__.py b/core/src/trezor/ui/layouts/tt_v2/__init__.py index fbf515f396..1b33b105ca 100644 --- a/core/src/trezor/ui/layouts/tt_v2/__init__.py +++ b/core/src/trezor/ui/layouts/tt_v2/__init__.py @@ -725,6 +725,7 @@ async def confirm_blob( data: bytes | str, description: str | None = None, verb: str = "CONFIRM", + verb_cancel: str | None = None, hold: bool = False, br_code: ButtonRequestType = BR_TYPE_OTHER, ask_pagination: bool = False, @@ -739,6 +740,7 @@ async def confirm_blob( extra=None, hold=hold, verb=verb, + verb_cancel=verb_cancel, ) ) diff --git a/tests/device_tests/test_cancel.py b/tests/device_tests/test_cancel.py index be15f763e1..ebf73ccc1e 100644 --- a/tests/device_tests/test_cancel.py +++ b/tests/device_tests/test_cancel.py @@ -90,6 +90,15 @@ def test_cancel_on_paginated(client: Client): resp = client._raw_read() assert isinstance(resp, m.ButtonRequest) + + # In TR, confirm message is no longer paginated by default, + # user needs to click info button + if client.debug.model == "R": + client._raw_write(m.ButtonAck()) + client.debug.press_right() + resp = client._raw_read() + assert isinstance(resp, m.ButtonRequest) + assert resp.pages is not None client._raw_write(m.ButtonAck()) diff --git a/tests/input_flows.py b/tests/input_flows.py index bdab88e723..624055aa9e 100644 --- a/tests/input_flows.py +++ b/tests/input_flows.py @@ -235,6 +235,11 @@ class InputFlowSignMessagePagination(InputFlowBase): yield self.debug.press_yes() + # press info + yield + self.debug.press_right() + + # paginate through the whole message br = yield # TODO: try load the message_read the same way as in model T if br.pages is not None: @@ -243,6 +248,10 @@ class InputFlowSignMessagePagination(InputFlowBase): self.debug.swipe_up() self.debug.press_yes() + # confirm message + yield + self.debug.press_yes() + class InputFlowShowAddressQRCode(InputFlowBase): def __init__(self, client: Client): @@ -789,12 +798,18 @@ class InputFlowEthereumSignTxScrollDown(InputFlowBase): self.debug.wait_layout() self.debug.press_yes() + yield # confirm data + self.debug.press_info() + br = yield # paginated data assert br.pages is not None for _ in range(br.pages): self.debug.wait_layout() self.debug.swipe_up() + yield # confirm data + self.debug.press_yes() + yield # confirm amount self.debug.wait_layout() self.debug.press_yes()