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.

Sign up for GitHub

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

Jump to bottom

ETH send flow - unify button requests #3393

Merged
merged 6 commits into from
Dec 4, 2023
Merged

ETH send flow - unify button requests #3393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43e9d95
fix(core): unify sent button requests in ETH send flow
grdddj Nov 7, 2023
7a933cc
chore(core): decrease code-size a little bit
grdddj Nov 7, 2023
c8138a3
fix(core): unify button requests in modify amount flow
grdddj Nov 7, 2023
8c4773d
chore(tools): add new Ethereum layout name into exceptions for altcoi…
grdddj Nov 7, 2023
d24a8a7
fix(core): completely repaint layouts before showing them in a loop
grdddj Nov 22, 2023
05df37c
chore(tests): regenerate fixtures
grdddj Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/embed/rust/librust_qstr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ static void _librust_qstrs(void) {
MP_QSTR_confirm_blob;
MP_QSTR_confirm_coinjoin;
MP_QSTR_confirm_emphasized;
MP_QSTR_confirm_ethereum_tx;
MP_QSTR_confirm_fido;
MP_QSTR_confirm_firmware_update;
MP_QSTR_confirm_homescreen;
Expand All @@ -70,6 +69,7 @@ static void _librust_qstrs(void) {
MP_QSTR_dry_run;
MP_QSTR_encode;
MP_QSTR_encoded_length;
MP_QSTR_ethereum_tx_summary;
MP_QSTR_extra;
MP_QSTR_fee_amount;
MP_QSTR_fee_label;
Expand Down Expand Up @@ -102,7 +102,6 @@ static void _librust_qstrs(void) {
MP_QSTR_progress_event;
MP_QSTR_prompt;
MP_QSTR_qr_title;
MP_QSTR_recipient;
MP_QSTR_request_bip39;
MP_QSTR_request_complete_repaint;
MP_QSTR_request_number;
Expand Down
37 changes: 6 additions & 31 deletions core/embed/rust/src/ui/model_tr/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,6 @@ extern "C" fn new_confirm_joint_total(n_args: usize, args: *const Obj, kwargs: *

extern "C" fn new_confirm_modify_output(n_args: usize, args: *const Obj, kwargs: *mut Map) -> Obj {
let block = move |_args: &[Obj], kwargs: &Map| {
let address: StrBuffer = kwargs.get(Qstr::MP_QSTR_address)?.try_into()?;
let sign: i32 = kwargs.get(Qstr::MP_QSTR_sign)?.try_into()?;
let amount_change: StrBuffer = kwargs.get(Qstr::MP_QSTR_amount_change)?.try_into()?;
let amount_new: StrBuffer = kwargs.get(Qstr::MP_QSTR_amount_new)?.try_into()?;
Expand All @@ -590,8 +589,6 @@ extern "C" fn new_confirm_modify_output(n_args: usize, args: *const Obj, kwargs:
};

let paragraphs = Paragraphs::new([
Paragraph::new(&theme::TEXT_BOLD, "Address:".into()),
Paragraph::new(&theme::TEXT_MONO, address).break_after(),
Paragraph::new(&theme::TEXT_NORMAL, description.into()),
Paragraph::new(&theme::TEXT_MONO, amount_change).break_after(),
Paragraph::new(&theme::TEXT_BOLD, "New amount:".into()),
Expand Down Expand Up @@ -754,34 +751,15 @@ extern "C" fn new_confirm_total(n_args: usize, args: *const Obj, kwargs: *mut Ma
unsafe { util::try_with_args_and_kwargs(n_args, args, kwargs, block) }
}

extern "C" fn new_confirm_ethereum_tx(n_args: usize, args: *const Obj, kwargs: *mut Map) -> Obj {
extern "C" fn new_ethereum_tx_summary(n_args: usize, args: *const Obj, kwargs: *mut Map) -> Obj {
let block = |_args: &[Obj], kwargs: &Map| {
let recipient: StrBuffer = kwargs.get(Qstr::MP_QSTR_recipient)?.try_into()?;
let total_amount: StrBuffer = kwargs.get(Qstr::MP_QSTR_total_amount)?.try_into()?;
let maximum_fee: StrBuffer = kwargs.get(Qstr::MP_QSTR_maximum_fee)?.try_into()?;
let items: Obj = kwargs.get(Qstr::MP_QSTR_items)?;
let chunkify: bool = kwargs.get_or(Qstr::MP_QSTR_chunkify, false)?;

let get_page = move |page_index| {
match page_index {
0 => {
// RECIPIENT
let btn_layout = ButtonLayout::cancel_none_text("CONTINUE".into());
let btn_actions = ButtonActions::cancel_none_next();

let style = if chunkify {
// Chunkifying the address into smaller pieces when requested
theme::TEXT_MONO_ADDRESS_CHUNKS
} else {
theme::TEXT_MONO_DATA
};

let ops = OpTextLayout::new(style).text_mono(recipient.clone());

let formatted = FormattedText::new(ops).vertically_centered();
Page::new(btn_layout, btn_actions, formatted).with_title("RECIPIENT".into())
}
1 => {
// Total amount + fee
let btn_layout = ButtonLayout::up_arrow_armed_info("CONFIRM".into());
let btn_actions = ButtonActions::prev_confirm_next();
Expand All @@ -797,7 +775,7 @@ extern "C" fn new_confirm_ethereum_tx(n_args: usize, args: *const Obj, kwargs: *
let formatted = FormattedText::new(ops);
Page::new(btn_layout, btn_actions, formatted).with_title("Amount:".into())
}
2 => {
1 => {
// Fee information
let btn_layout = ButtonLayout::arrow_none_none();
let btn_actions = ButtonActions::prev_none_none();
Expand All @@ -824,7 +802,7 @@ extern "C" fn new_confirm_ethereum_tx(n_args: usize, args: *const Obj, kwargs: *
_ => unreachable!(),
}
};
let pages = FlowPages::new(get_page, 3);
let pages = FlowPages::new(get_page, 2);

let obj = LayoutObj::new(Flow::new(pages).with_scrollbar(false))?;
Ok(obj.into())
Expand Down Expand Up @@ -1778,12 +1756,11 @@ pub static mp_module_trezorui2: Module = obj_module! {

/// def confirm_modify_output(
/// *,
/// address: str,
/// sign: int,
/// amount_change: str,
/// amount_new: str,
/// ) -> object:
/// """Decrease or increase amount for given address."""
/// """Decrease or increase output amount."""
Qstr::MP_QSTR_confirm_modify_output => obj_fn_kw!(0, new_confirm_modify_output).as_obj(),

/// def confirm_output_address(
Expand Down Expand Up @@ -1816,16 +1793,14 @@ pub static mp_module_trezorui2: Module = obj_module! {
/// """Confirm summary of a transaction."""
Qstr::MP_QSTR_confirm_total => obj_fn_kw!(0, new_confirm_total).as_obj(),

/// def confirm_ethereum_tx(
/// def ethereum_tx_summary(
/// *,
/// recipient: str,
/// total_amount: str,
/// maximum_fee: str,
/// items: Iterable[Tuple[str, str]],
/// chunkify: bool = False,
/// ) -> object:
/// """Confirm details about Ethereum transaction."""
Qstr::MP_QSTR_confirm_ethereum_tx => obj_fn_kw!(0, new_confirm_ethereum_tx).as_obj(),
Qstr::MP_QSTR_ethereum_tx_summary => obj_fn_kw!(0, new_ethereum_tx_summary).as_obj(),

/// def tutorial() -> object:
/// """Show user how to interact with the device."""
Expand Down
3 changes: 1 addition & 2 deletions core/embed/rust/src/ui/model_tt/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1766,12 +1766,11 @@ pub static mp_module_trezorui2: Module = obj_module! {

/// def confirm_modify_output(
/// *,
/// address: str, # ignored
/// sign: int,
/// amount_change: str,
/// amount_new: str,
/// ) -> object:
/// """Decrease or increase amount for given address."""
/// """Decrease or increase output amount."""
Qstr::MP_QSTR_confirm_modify_output => obj_fn_kw!(0, new_confirm_modify_output).as_obj(),

/// def confirm_modify_fee(
Expand Down
10 changes: 3 additions & 7 deletions core/mocks/generated/trezorui2.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,11 @@ def confirm_joint_total(
# rust/src/ui/model_tr/layout.rs
def confirm_modify_output(
*,
address: str,
sign: int,
amount_change: str,
amount_new: str,
) -> object:
"""Decrease or increase amount for given address."""
"""Decrease or increase output amount."""


# rust/src/ui/model_tr/layout.rs
Expand Down Expand Up @@ -170,13 +169,11 @@ def confirm_total(


# rust/src/ui/model_tr/layout.rs
def confirm_ethereum_tx(
def ethereum_tx_summary(
*,
recipient: str,
total_amount: str,
maximum_fee: str,
items: Iterable[Tuple[str, str]],
chunkify: bool = False,
) -> object:
"""Confirm details about Ethereum transaction."""

Expand Down Expand Up @@ -599,12 +596,11 @@ def confirm_total(
# rust/src/ui/model_tt/layout.rs
def confirm_modify_output(
*,
address: str, # ignored
sign: int,
amount_change: str,
amount_new: str,
) -> object:
"""Decrease or increase amount for given address."""
"""Decrease or increase output amount."""


# rust/src/ui/model_tt/layout.rs
Expand Down
2 changes: 1 addition & 1 deletion core/src/apps/misc/get_ecdh_session_key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ async def get_ecdh_session_key(msg: GetECDHSessionKey) -> ECDHSessionKey:
await confirm_address(
f"Decrypt {proto}",
serialize_identity_without_proto(msg_identity),
None,
"",
)
# END require_confirm_ecdh_session_key

Expand Down
98 changes: 70 additions & 28 deletions core/src/trezor/ui/layouts/tr/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ async def show_address(
br_code,
pages=layout.page_count(),
)
layout.request_complete_repaint()
result = await ctx_wait(layout)

# User confirmed with middle button.
Expand Down Expand Up @@ -765,7 +766,7 @@ async def confirm_blob(
br_type: str,
title: str,
data: bytes | str,
description: str | None = None,
description: str = "",
verb: str = "CONFIRM",
verb_cancel: str | None = "", # icon
hold: bool = False,
Expand All @@ -774,7 +775,6 @@ async def confirm_blob(
chunkify: bool = False,
) -> None:
title = title.upper()
description = description or ""
layout = RustLayout(
trezorui2.confirm_blob(
title=title,
Expand Down Expand Up @@ -848,7 +848,7 @@ async def _confirm_ask_pagination(
def confirm_address(
title: str,
address: str,
description: str | None = "Address:",
description: str = "Address:",
br_type: str = "confirm_address",
br_code: ButtonRequestType = BR_TYPE_OTHER,
) -> Awaitable[None]:
Expand Down Expand Up @@ -1000,22 +1000,37 @@ async def confirm_ethereum_tx(
br_code: ButtonRequestType = ButtonRequestType.SignTx,
chunkify: bool = False,
) -> None:
await raise_if_not_confirmed(
interact(
RustLayout(
trezorui2.confirm_ethereum_tx(
recipient=recipient,
total_amount=total_amount,
maximum_fee=maximum_fee,
items=items,
chunkify=chunkify,
)
),
br_type,
br_code,
summary_layout = RustLayout(
trezorui2.ethereum_tx_summary(
total_amount=total_amount,
maximum_fee=maximum_fee,
items=items,
)
)

while True:
# Allowing going back and forth between recipient and summary/details
await confirm_blob(
br_type,
"RECIPIENT",
recipient,
verb="CONTINUE",
chunkify=chunkify,
)

try:
summary_layout.request_complete_repaint()
await raise_if_not_confirmed(
interact(
summary_layout,
br_type,
br_code,
)
)
break
except ActionCancelled:
continue


async def confirm_joint_total(spending_amount: str, total_amount: str) -> None:
await raise_if_not_confirmed(
Expand Down Expand Up @@ -1066,21 +1081,48 @@ async def confirm_modify_output(
amount_change: str,
amount_new: str,
) -> None:
await raise_if_not_confirmed(
interact(
RustLayout(
trezorui2.confirm_modify_output(
address=address,
sign=sign,
amount_change=amount_change,
amount_new=amount_new,
)
),
"modify_output",
ButtonRequestType.ConfirmOutput,
address_layout = RustLayout(
trezorui2.confirm_blob(
title="MODIFY AMOUNT",
data=address,
verb="CONTINUE",
verb_cancel=None,
description="Address:",
extra=None,
)
)
modify_layout = RustLayout(
trezorui2.confirm_modify_output(
sign=sign,
amount_change=amount_change,
amount_new=amount_new,
)
)

send_button_request = True
while True:
if send_button_request:
await button_request(
"modify_output",
ButtonRequestType.ConfirmOutput,
address_layout.page_count(),
)
address_layout.request_complete_repaint()
await raise_if_not_confirmed(ctx_wait(address_layout))

if send_button_request:
send_button_request = False
await button_request(
"modify_output",
ButtonRequestType.ConfirmOutput,
modify_layout.page_count(),
)
modify_layout.request_complete_repaint()
result = await ctx_wait(modify_layout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid you'll have to call .request_complete_repaint() on the two layouts otherwise they'll paint over each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this situation different in TR than in TT? This is the same exact code as in confirm_modify_output's code in core/src/trezor/ui/layouts/tt/__init__.py

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be the same & in both cases there is garbage on screen when you go back from the MODIFY AMOUNT screen in test_p2wpkh_in_p2sh_fee_bump_from_external.

TBH this should be handled automagically somewhere in class Layout, not sure if it's worth spending time on fixing it or rewriting to rust ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I did the quick fix in 4970a6b --- requesting repaint in all while True: cases where we define the layout in front of the loop


if result is CONFIRMED:
break


async def confirm_modify_fee(
title: str,
Expand Down
Loading
Loading