Add thousands separators to amounts

[matejcik]

fixes #2394

on trezor-core side, the thousands separators are only added to amounts, or more generally, anything formatted with format_amount. There are several more places where large integers are displayed which are not amounts. These are mostly out of scope however.

On legacy, there is no special "format_amount", so the general bn_format function is modified. I considered adding a parameter to specify whether the caller wants thousands separators or not, but decided against it in the end, as it would require changes in a lot of places and using thousands separators everywhere seems desirable anyway.

[sime]

All amounts, or all amounts that are represented as sats?

[matejcik]

all amounts.

is there a requirement to not do this for non-sats?

[grdddj]

Looking at the big negative diff - looks like all the TTui2... test-cases from fixtures.json got deleted. Was this intentional?

[matejcik]

the general bn_format function is modified

this is a problem because it seems to be used internally in NEM code. it is weird that the NEM device tests did not crash on that though.

[sime]

all amounts.

is there a requirement to not do this for non-sats?

Originally yes. Clarified with @hynek-jina that Suite will thousand separate all cryptocurrencies.

So please disregard my previous comment.

[matejcik]

looks like all the TTui2... test-cases from fixtures.json got deleted

fixed in 3d7ea3f

[matejcik]

because of the NEM thing I had to change bn_format to accept an argument after all. There is now a separate commit for crypto, and legacy makes use of it through the new bn_format_amount function that hides a bunch of parameters.
When rebasing, I will need to move the "legacy" commit after the "crypto" commit.

[andrewkozlik]

I am wondering whether output_length is always sufficiently large to accommodate the addition of the the thousands separators. In case of bn_format_uint64() we typically allocate 32 bytes, which should always be sufficient. The most extreme example with UINT64_MAX that I can think of would be "-1,844,674,407,370,955,161.5 uDASH", which is 35 bytes including the terminating NULL, but I think it is unlikely that we could run into that considering that the maximum possible amounts in sats are not quite near UINT64_MAX.

I didn't check the size of the allocated buffers thoroughly, because I assume that the device tests would be likely to uncover any problems especially if we check the UI test screens thoroughly.

[marnova]

LGTM after fixes, you just missed one fixture https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2797235219/artifacts/test_ui_report/failed/TT_zcash-test_sign_tx.py::test_spend_v5_input.html

[grdddj]

Approve for the python part

[marnova]

Don't forget to change the fixture for https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2797235219/artifacts/test_ui_report/failed/TT_zcash-test_sign_tx.py::test_spend_v5_input.html , otherwise approve

[andrewkozlik]

Adding thousands separators in these cases looks a bit weird:

• #1,337
• Memo (ID) 12,345,678,901,234

https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2803415060/artifacts/master_diff/diff/T1_stellar-test_stellar.py::test_sign_tx[StellarManageBuyOfferOp].html
https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2803415060/artifacts/master_diff/diff/T1_stellar-test_stellar.py::test_sign_tx[StellarManageSellOfferOp].html
https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2803415060/artifacts/master_diff/diff/T1_stellar-test_stellar.py::test_sign_tx[memo_id].html

[matejcik]

Adding thousands separators in these cases looks a bit weird:

it does, but OTOH it's a correct rendering of numbers so 🤷‍♀️