Chunkifying the receiving address #3237

grdddj · 2023-08-25T11:59:08Z

allowing for showing the address in chunks of 4 characters

grdddj · 2023-08-25T13:54:58Z

Interesting edge-case in TT and Cardano - when there are more pages, the text area is smaller because of the scrollbar, and the last character does not fit. Might be possible to decrease the x_offset in case there are more pages

prusnak · 2023-08-25T19:04:57Z

Interesting edge-case in TT

Do we still want to show the dots representing the pages even after adding the new hints? Especially since you can't go back anyway ...

common/protob/messages-bitcoin.proto

Hannsek · 2023-08-27T12:26:23Z

Interesting edge-case in TT

Do we still want to show the dots representing the pages even after adding the new hints? Especially since you can't go back anyway ...

The thing you cannot go back is a bug. Should be solved by #2888 (#2902).
We are thinking about removing the pagination when there are only 2 pages. But don't have a resolution now.

@grdddj We should display max 16 chunks per page, not more. See the image below.

What is the vertical space between rows/chunks? Is it currently variable?

Also, can we align the "next page" arrow to fit the last chunk (or block of text)?

grdddj · 2023-08-28T07:42:26Z

@grdddj We should display max 16 chunks per page, not more. See the image below.

Is it worth doing this complexity, when the only addresses longer than 16 chunks are Cardano? Others are fine even without this limit

What is the vertical space between rows/chunks? Is it currently variable?

Vertical space between coherent text (inside one paragraph) is constant, as in all other text

Also, can we align the "next page" arrow to fit the last chunk (or block of text)?

Yes, we can somehow, but same as above, it is only valid for Cardano, others do not need it

Hannsek · 2023-09-04T11:31:51Z

Is it worth doing this complexity, when the only addresses longer than 16 chunks are Cardano? Others are fine even without this limit?

Hmm, probably not. And we can set the cardano address to appear as a standard string.
However, we should do it for (multisig) xpubs as well in the future.

Vertical space between coherent text (inside one paragraph) is constant, as in all other text

We should make it larger in both, TT and TR.

grdddj · 2023-09-05T07:37:00Z

Vertical space between coherent text (inside one paragraph) is constant, as in all other text

We should make it larger in both, TT and TR.

Do you want to increase the vertical margin in this special case of chunked address, or in all the screens in general?

Hannsek · 2023-09-05T08:03:10Z

Only in this special case of chunked address.

Hannsek · 2023-09-05T08:58:31Z

Also please increase the horizontal space between chunks on TT by 1 px.

grdddj · 2023-09-05T09:22:41Z

Force-pushed after rebase on master.

It includes increase in vertical spacing, as well as increase in TT's horizontal space

grdddj · 2023-09-11T13:22:04Z

Last commit introduces chunkifying also for the address in sign flow. In TR, there is one edge-case where the word CoinJoin (as an address_label) is also shown at that screen, and it is chunkified as well. It would be quite complicated to render the label without chunks and the address with chunks.

@Hannsek Please consider if this looks good enough - and please look at all the other changed screens in signing flow.

Hannsek · 2023-09-11T13:55:49Z

We should render the label not chunked.

grdddj · 2023-09-11T14:52:51Z

Did also the account addition, and some of the labels do not fit properly:

mmilata · 2023-09-11T18:32:39Z

Not sure if it's an improvement on model T:

mmilata · 2023-09-11T19:09:20Z

Is the font really fixed width? Sometimes the alignment looks very wrong:

grdddj · 2023-09-12T07:16:00Z

Is the font really fixed width? Sometimes the alignment looks very wrong:

Right, the font is an almost-mono. Some letters like m, M, w or W are one pixel wider than others (8 vs 7 pixels - including margin) - can be seen in https://github.com/trezor/trezor-firmware/blob/master/core/embed/lib/fonts/font_pixeloperatormono_regular_8.c

Seems there are no easy fixes, as the font is so small (narrow) that these four letters just cannot be made as narrow as other characters.

Possible measures:

manually increasing the width of all the letters from 7 to 8 pixels ... at the cost of making the majority of the text wastefully wider
accounting for it in the rendering, remembering the number or "extra pixels" in the chunk and decreasing the x-offset - this way at least the chunks would be starting at the same vertical line (however, not applicable when there are no chunks)

Do you see some other possibilities?

Hannsek · 2023-09-12T07:47:41Z

Please delete "account" in receive flow.
We need to figure out the wording in send on TT. So probably keep the "account" only for send flow on T2B1.

accounting for it in the rendering, remembering the number or "extra pixels" in the chunk and decreasing the x-offset - this way at least the chunks would be starting at the same vertical line (however, not applicable when there are no chunks)

Please try it

Hannsek · 2023-09-12T15:12:02Z

Now it looks good, but there is some trouble regarding long addresses (cardano) but I believe it is ok to leave it like this.

It should look like this:

Hannsek · 2023-09-12T15:13:22Z

The bigger problem is this, wrong positioning of the left arrow.

…ignTx protobuf messages

[no changelog]

…es from CI [no changelog]

[no changelog]

…and CLI

[no changelog]

core/src/apps/tezos/sign_tx.py

matejcik

non-Rust part LGTM, just some superfluous default arguments

matejcik · 2023-09-14T13:47:16Z

core/src/apps/nem/transfer/__init__.py

@@ -12,11 +12,12 @@ async def transfer(
    common: NEMTransactionCommon,
    transfer: NEMTransfer,
    node: bip32.HDNode,
+    chunkify: bool = False,


argument here and in layout.ask_transfer does not need a default value

Done in 7e03759

matejcik · 2023-09-14T13:47:51Z

core/src/apps/monero/layout.py

@@ -143,6 +145,7 @@ async def _require_confirm_output(
    dst: MoneroTransactionDestinationEntry,
    network_type: MoneroNetworkType,
    payment_id: bytes | None,
+    chunkify: bool = False,


remove default value

Done in 7e03759

matejcik · 2023-09-14T13:48:21Z

core/src/apps/cardano/layout.py

@@ -210,6 +210,7 @@ async def confirm_sending(
    to: str,
    output_type: Literal["address", "change", "collateral-return"],
    network_id: int,
+    chunkify: bool = False,


remove default value

Done in 7e03759

matejcik · 2023-09-14T13:49:11Z

core/src/apps/bitcoin/sign_tx/layout.py

@@ -62,6 +62,7 @@ async def confirm_output(
    coin: CoinInfo,
    amount_unit: AmountUnit,
    output_index: int,
+    chunkify: bool = False,


remove default value

Done in 7e03759

matejcik · 2023-09-14T13:49:41Z

core/src/apps/binance/layout.py



-async def _confirm_transfer(inputs_outputs: Sequence[tuple[str, str, str]]) -> None:
+async def _confirm_transfer(
+    inputs_outputs: Sequence[tuple[str, str, str]], chunkify: bool = False


remove default value

Done in 7e03759

mmilata

Screens look good now. Commented some nits, also not super happy about the renderer changes but don't really see how we can come up with something better in short time.

core/src/apps/bitcoin/keychain.py

mmilata · 2023-09-14T13:24:22Z

common/protob/messages-bitcoin.proto

@@ -110,6 +110,7 @@ message GetAddress {
    optional MultisigRedeemScriptType multisig = 4;                     // filled if we are showing a multisig address
    optional InputScriptType script_type = 5 [default=SPENDADDRESS];    // used to distinguish between various address formats (non-segwit, segwit, etc.)
    optional bool ignore_xpub_magic = 6;                                // ignore SLIP-0132 XPUB magic, use xpub/tpub prefix for all account types
+    optional bool chunkify = 7;                                         // display the address in chunks of 4 characters


Nit: thinking if we can come up with a more descriptive name, perhaps indicate that it is related to show_display. show_chunks? show_segmented?

Can change it to anything, if we agree. What is your preference? @matejcik

show_chunks sounds good to me

core/embed/rust/src/ui/component/text/layout.rs

core/embed/rust/src/ui/component/text/op.rs

mmilata · 2023-09-14T13:50:47Z

core/embed/rust/src/ui/component/text/layout.rs

+    /// How big will be the space between chunks (in pixels).
+    pub x_offset: i16,
+    /// Optional characters that are wider and should be accounted for
+    pub wider_chars: Option<&'static str>,


Kinda hate this tbh (as well as the whole idea of trying to render variable-width font as a fixed-width font). Trying to think how to make this more general. Can we instead remember the width of a normal (or smallest) character and keep track of the actual chunk size when processing chunk?

Nor am I happy with this, but this "almost-mono-font" can hardly be fixed to be "completely-mono", because of its small width, as I am describing here

Not sure I follow your suggestion - can you prototype some code for it?

Merging this does not need to be final, we can improve it in the future

mmilata · 2023-09-14T13:56:05Z

core/embed/rust/src/ui/component/text/layout.rs

+                    line.advance.y = 0;
+                    // Decreasing the offset for each wider character in the chunk
+                    line.advance.x += chunkify_config.x_offset - chunks_wider_chars;
+                    return line;


I wonder if this can't break line-counting somewhere?

The tests seem to be alright, it works even with longer addresses that paginate (Cardano ones)

matejcik · 2023-09-14T13:58:02Z

core/embed/rust/src/ui/component/text/op.rs

+                Op::Chunkify(chunk) => {
+                    if let Some((chunk_config, extra_y_offset)) = chunk {
+                        self.layout.style.chunks = Some(chunk_config);
+                        self.layout.style.extra_y_line_advance = Some(extra_y_offset);
+                    } else {
+                        self.layout.style.chunks = None;
+                        self.layout.style.extra_y_line_advance = None;
+                    }
+                }


Suggested change

Op::Chunkify(chunk) => {

if let Some((chunk_config, extra_y_offset)) = chunk {

self.layout.style.chunks = Some(chunk_config);

self.layout.style.extra_y_line_advance = Some(extra_y_offset);

} else {

self.layout.style.chunks = None;

self.layout.style.extra_y_line_advance = None;

}

}

Op::Chunkify(Some((chunk_config, extra_y_offset))) => {

self.layout.style.chunks = Some(chunk_config);

self.layout.style.extra_y_line_advance = Some(extra_y_offset);

}

Op::Chunkify(None) => {

self.layout.style.chunks = None;

self.layout.style.extra_y_line_advance = None;

}

I have simplified it in eb3cab9 by separating to two Op enums

…nd send flows

[no changelog]

mmilata

I'm OK with merging this, rename the parameter to show_chunks if you want.

…can send a button request in between [no changelog]

[no changelog]

…ests to start with "is_" [no changelog]

…ce tests [no changelog]

[no changelog]

grdddj · 2023-09-14T18:07:00Z

@prusnak please review the change request, it is blocking the merge.

prusnak

I reviewed only protobuf def changes

grdddj self-assigned this Aug 25, 2023

grdddj marked this pull request as ready for review August 25, 2023 14:47

grdddj requested review from matejcik and prusnak as code owners August 25, 2023 14:47

grdddj requested review from Hannsek and removed request for prusnak August 25, 2023 14:47

prusnak requested changes Aug 26, 2023

View reviewed changes

common/protob/messages-bitcoin.proto Outdated Show resolved Hide resolved

grdddj force-pushed the grdddj/chunked_address branch from 1d8d08f to 798a2d2 Compare September 5, 2023 09:22

grdddj requested a review from onvej-sl as a code owner September 6, 2023 10:41

grdddj force-pushed the grdddj/chunked_address branch from 048d592 to 141024f Compare September 11, 2023 13:15

grdddj force-pushed the grdddj/chunked_address branch from d8baa5b to fa24f7f Compare September 12, 2023 09:04

grdddj requested a review from mmilata September 13, 2023 09:36

This was referenced Sep 14, 2023

Improvements on chunkifying addresses - spaces #3271

Closed

Improvements on chunkifying addresses - alignment #3272

Closed

grdddj force-pushed the grdddj/chunked_address branch from 361682b to c2d03d1 Compare September 14, 2023 10:33

grdddj added 7 commits September 14, 2023 13:54

feat(all): add optional chunkify parameter for all getAddress and s…

aa35726

…ignTx protobuf messages

feat(tests): show newly created tests in master-diff report

05db28e

[no changelog]

feat(tests): allow for removing missing tests when updating UI fixtur…

0d04c16

…es from CI [no changelog]

feat(tests): add device tests for chunkifying addresses

c777ec5

[no changelog]

feat(python): allow for chunkifying addresses in trezorlib functions …

a9111f8

…and CLI

chore(makefile): remove missing tests when recording local UI fixtures

3bbcdb8

[no changelog]

fix(tests): add chunkify argument to unittests

5057912

[no changelog]

grdddj force-pushed the grdddj/chunked_address branch from c2d03d1 to dd38f2a Compare September 14, 2023 12:15

matejcik reviewed Sep 14, 2023

View reviewed changes

core/src/apps/tezos/sign_tx.py Show resolved Hide resolved

matejcik reviewed Sep 14, 2023

View reviewed changes

mmilata reviewed Sep 14, 2023

View reviewed changes

matejcik reviewed Sep 14, 2023

View reviewed changes

grdddj added 3 commits September 14, 2023 17:58

feat(core): support optional chunkification of addresses in receive a…

e7a224a

…nd send flows

chore(all): add changelog entries for address chunkification

2e8f83a

chore(tests): regenerate fixtures

7019b1a

[no changelog]

mmilata approved these changes Sep 14, 2023

View reviewed changes

grdddj force-pushed the grdddj/chunked_address branch from ddef752 to 7019b1a Compare September 14, 2023 18:03

grdddj added 5 commits September 14, 2023 20:05

chore(core): separate confirm_output for T2B1 into two layouts so we …

efa28a4

…can send a button request in between [no changelog]

chore(tests): accounting for the extra T2B1 buton request in input flows

25f54e0

[no changelog]

chore(tests): change all the model-specific variables for button_requ…

e27e4eb

…ests to start with "is_" [no changelog]

chore(tests): account for T2B1 sending extra ConfirmOutput BR in devi…

9e05928

…ce tests [no changelog]

chore(tests): regenerate fixtures

b34cedd

[no changelog]

grdddj requested a review from prusnak September 14, 2023 18:08

prusnak approved these changes Sep 15, 2023

View reviewed changes

grdddj merged commit 8c98015 into master Sep 15, 2023

grdddj deleted the grdddj/chunked_address branch September 15, 2023 07:50

Chunkifying the receiving address #3237

Chunkifying the receiving address #3237

Conversation

grdddj commented Aug 25, 2023

grdddj commented Aug 25, 2023

prusnak commented Aug 25, 2023 • edited Loading

Hannsek commented Aug 27, 2023 • edited Loading

grdddj commented Aug 28, 2023

Hannsek commented Sep 4, 2023

grdddj commented Sep 5, 2023

Hannsek commented Sep 5, 2023

Hannsek commented Sep 5, 2023

grdddj commented Sep 5, 2023

grdddj commented Sep 11, 2023

Hannsek commented Sep 11, 2023

grdddj commented Sep 11, 2023

mmilata commented Sep 11, 2023

mmilata commented Sep 11, 2023

grdddj commented Sep 12, 2023

Hannsek commented Sep 12, 2023

Hannsek commented Sep 12, 2023 • edited Loading

Hannsek commented Sep 12, 2023

matejcik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilata left a comment

Choose a reason for hiding this comment

grdddj commented Sep 14, 2023

prusnak left a comment

Choose a reason for hiding this comment

prusnak commented Aug 25, 2023 •

edited

Loading

Hannsek commented Aug 27, 2023 •

edited

Loading

Hannsek commented Sep 12, 2023 •

edited

Loading