Add thousands separators to amounts

core/.changelog.d/2394.added

@@ -0,0 +1 @@
+Show thousands separator when displaying large amounts.

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

@@ -190,7 +190,12 @@ async def confirm_total(
     fee_rate_str: str | None = None
 
     if fee_rate >= 0:
-        fee_rate_str = f"({fee_rate:.1f} sat/{'v' if coin.segwit else ''}B)"
+        # Use format_amount to get correct thousands separator -- micropython's built-in
+        # formatting doesn't add thousands sep to floating point numbers.
+        # We multiply by 10 to get a fixed-point integer with one decimal place,
+        # and add 0.5 to round to the nearest integer.
+        fee_rate_formatted = format_amount(int(fee_rate * 10 + 0.5), 1)
+        fee_rate_str = f"({fee_rate_formatted} sat/{'v' if coin.segwit else ''}B)"
 
     await layouts.confirm_total(
         ctx,

core/src/trezor/strings.py

@@ -10,8 +10,14 @@ def format_amount(amount: int, decimals: int) -> str:
         sign = "-"
     else:
         sign = ""
-    d = pow(10, decimals)
-    s = f"{sign}{amount // d}.{amount % d:0{decimals}}".rstrip("0").rstrip(".")
+    d = 10**decimals
+    integer = amount // d
+    decimal = amount % d
+
+    # TODO: bug in mpz: https://github.com/micropython/micropython/issues/8984
+    grouped_integer = f"{integer:,}".lstrip(",")
+
+    s = f"{sign}{grouped_integer}.{decimal:0{decimals}}".rstrip("0").rstrip(".")
     return s
 
 

core/tests/test_apps.bitcoin.signtx.omni.py

@@ -22,8 +22,8 @@ def test_parse(self):
         VECTORS = {
              "6f6d6e69000000000000001f000000002b752ee0": "Simple send of 7.291 USDT",
              "6f6d6e69000000000000001f0000000020c85580": "Simple send of 5.5 USDT",
-             "6f6d6e690000000000000003000000002b752ee0": "Simple send of 729100000 MAID",
-             "6f6d6e690000000000000000000000002b752ee0": "Simple send of 729100000 UNKN",
+             "6f6d6e690000000000000003000000002b752ee0": "Simple send of 729,100,000 MAID",
+             "6f6d6e690000000000000000000000002b752ee0": "Simple send of 729,100,000 UNKN",
              "6f6d6e6901000000": "Unknown transaction",
         }
         for k, v in VECTORS.items():

core/tests/test_apps.ethereum.layout.py

@@ -12,13 +12,13 @@ def test_format(self):
         text = format_ethereum_amount(1, None, 1)
         self.assertEqual(text, '1 Wei ETH')
         text = format_ethereum_amount(1000, None, 1)
-        self.assertEqual(text, '1000 Wei ETH')
+        self.assertEqual(text, '1,000 Wei ETH')
         text = format_ethereum_amount(1000000, None, 1)
-        self.assertEqual(text, '1000000 Wei ETH')
+        self.assertEqual(text, '1,000,000 Wei ETH')
         text = format_ethereum_amount(10000000, None, 1)
-        self.assertEqual(text, '10000000 Wei ETH')
+        self.assertEqual(text, '10,000,000 Wei ETH')
         text = format_ethereum_amount(100000000, None, 1)
-        self.assertEqual(text, '100000000 Wei ETH')
+        self.assertEqual(text, '100,000,000 Wei ETH')
         text = format_ethereum_amount(1000000000, None, 1)
         self.assertEqual(text, '0.000000001 ETH')
         text = format_ethereum_amount(10000000000, None, 1)
@@ -44,7 +44,7 @@ def test_format(self):
         text = format_ethereum_amount(100000000000000000000, None, 1)
         self.assertEqual(text, '100 ETH')
         text = format_ethereum_amount(1000000000000000000000, None, 1)
-        self.assertEqual(text, '1000 ETH')
+        self.assertEqual(text, '1,000 ETH')
 
         text = format_ethereum_amount(1000000000000000000, None, 61)
         self.assertEqual(text, '1 ETC')
@@ -97,7 +97,7 @@ def test_unknown_token(self):
         self.assertEqual(text, '0 Wei UNKN')
         # unknown token has 0 decimals so is always wei
         text = format_ethereum_amount(1000000000000000000, unknown_token, 1)
-        self.assertEqual(text, '1000000000000000000 Wei UNKN')
+        self.assertEqual(text, '1,000,000,000,000,000,000 Wei UNKN')
 
 
 if __name__ == '__main__':

core/tests/test_trezor.strings.py

@@ -4,13 +4,20 @@
 
 
 class TestStrings(unittest.TestCase):
-
     def test_format_amount(self):
         VECTORS = [
             (123456, 3, "123.456"),
             (4242, 7, "0.0004242"),
             (-123456, 3, "-123.456"),
             (-4242, 7, "-0.0004242"),
+            (123, 5, "0.00123"),
+            (100, 5, "0.001"),
+            (123456789, 0, "123,456,789"),
+            (100000000, 5, "1,000"),
+            (100000001, 5, "1,000.00001"),
+            (100001000, 5, "1,000.01"),
+            (-100001000, 5, "-1,000.01"),
+            (123_456_789_123_456_789_123_456_789, 18, "123,456,789.123456789123456789"),
         ]
         for v in VECTORS:
             self.assertEqual(strings.format_amount(v[0], v[1]), v[2])
@@ -151,5 +158,5 @@ def test_format_timestamp(self):
         strings.format_timestamp(1616057224)
 
 
-if __name__ == '__main__':
+if __name__ == "__main__":
     unittest.main()

crypto/address.c

@@ -73,7 +73,7 @@ void ethereum_address_checksum(const uint8_t *addr, char *address, bool rskip60,
   keccak_256_Init(&ctx);
   if (rskip60) {
     char prefix[16] = {0};
-    int prefix_size = bn_format_uint64(chain_id, NULL, "0x", 0, 0, false,
+    int prefix_size = bn_format_uint64(chain_id, NULL, "0x", 0, 0, false, 0,
                                        prefix, sizeof(prefix));
     keccak_Update(&ctx, (const uint8_t *)prefix, prefix_size);
   }

crypto/bignum.c

@@ -1670,27 +1670,31 @@ void bn_divmod10(bignum256 *x, uint32_t *r) { bn_long_division(x, 10, x, r); }
 // Assumes output is an array of length output_length
 // The function doesn't have neither constant control flow nor constant memory
 //   access flow with regard to any its argument
-size_t bn_format(const bignum256 *amount, const char *prefix, const char *suffix, unsigned int decimals, int exponent, bool trailing, char *output, size_t output_length) {
+size_t bn_format(const bignum256 *amount, const char *prefix, const char *suffix, unsigned int decimals, int exponent, bool trailing, char thousands, char *output, size_t output_length) {
 
 /*
   Python prototype of the function:
 
-  def format(amount, prefix, suffix, decimals, exponent, trailing):
+  def format(amount, prefix, suffix, decimals, exponent, trailing, thousands):
       if exponent >= 0:
-          amount *= 10 ** exponent
+          amount *= 10**exponent
       else:
           amount //= 10 ** (-exponent)
 
       d = pow(10, decimals)
 
+      integer_part = amount // d
+      integer_str = f"{integer_part:,}".replace(",", thousands or "")
+
       if decimals:
-          output = "%d.%0*d" % (amount // d, decimals, amount % d)
+          decimal_part = amount % d
+          decimal_str = f".{decimal_part:0{decimals}d}"
           if not trailing:
-              output = output.rstrip("0").rstrip(".")
+              decimal_str = decimal_str.rstrip("0").rstrip(".")
       else:
-          output = "%d" % (amount // d)
+          decimal_str = ""
 
-      return prefix + output + suffix
+      return prefix + integer_str + decimal_str + suffix
 */
 
 // Auxiliary macro for bn_format
@@ -1773,18 +1777,29 @@ size_t bn_format(const bignum256 *amount, const char *prefix, const char *suffix
 
   {  // Add integer-part digits of amount
     // Add trailing zeroes
+    int digits = 0;
     if (!bn_is_zero(&temp)) {
       for (; exponent > 0; --exponent) {
+        ++digits;
         BN_FORMAT_ADD_OUTPUT_CHAR('0')
+        if (thousands != 0 && digits % 3 == 0) {
+          BN_FORMAT_ADD_OUTPUT_CHAR(thousands)
+        }
       }
     }
     // decimals == 0 && exponent == 0
 
     // Add significant digits
+    bool is_zero = false;
     do {
+      ++digits;
       bn_divmod10(&temp, &digit);
+      is_zero = bn_is_zero(&temp);
       BN_FORMAT_ADD_OUTPUT_CHAR('0' + digit)
-    } while (!bn_is_zero(&temp));
+      if (thousands != 0 && !is_zero && digits % 3 == 0) {
+        BN_FORMAT_ADD_OUTPUT_CHAR(thousands)
+      }
+    } while (!is_zero);
   }
 
   // Add prefix

crypto/bignum.h

@@ -114,7 +114,8 @@ void bn_divmod1000(bignum256 *x, uint32_t *r);
 void bn_inverse(bignum256 *x, const bignum256 *prime);
 size_t bn_format(const bignum256 *amount, const char *prefix,
                  const char *suffix, unsigned int decimals, int exponent,
-                 bool trailing, char *output, size_t output_length);
+                 bool trailing, char thousands, char *output,
+                 size_t output_length);
 
 // Returns (uint32_t) in_number
 // Assumes in_number < 2**32
@@ -149,13 +150,21 @@ static inline int bn_is_odd(const bignum256 *x) { return (x->val[0] & 1) == 1; }
 
 static inline size_t bn_format_uint64(uint64_t amount, const char *prefix,
                                       const char *suffix, unsigned int decimals,
-                                      int exponent, bool trailing, char *output,
+                                      int exponent, bool trailing,
+                                      char thousands, char *output,
                                       size_t output_length) {
   bignum256 bn_amount;
   bn_read_uint64(amount, &bn_amount);
 
   return bn_format(&bn_amount, prefix, suffix, decimals, exponent, trailing,
-                   output, output_length);
+                   thousands, output, output_length);
+}
+
+static inline size_t bn_format_amount(uint64_t amount, const char *prefix,
+                                      const char *suffix, unsigned int decimals,
+                                      char *output, size_t output_length) {
+  return bn_format_uint64(amount, prefix, suffix, decimals, 0, false, ',',
+                          output, output_length);
 }
 
 #if USE_BN_PRINT

crypto/fuzzer/fuzzer.c

@@ -165,7 +165,7 @@ int fuzz_bn_format(void) {
   }
 
   ret = bn_format(&target_bignum, prefix, suffix, decimals, exponent, trailing,
-                  buf, FUZZ_BN_FORMAT_OUTPUT_BUFFER_SIZE);
+                  0, buf, FUZZ_BN_FORMAT_OUTPUT_BUFFER_SIZE);
 
   // basic sanity checks for r
   if (ret > FUZZ_BN_FORMAT_OUTPUT_BUFFER_SIZE) {

crypto/nem.c

@@ -118,7 +118,7 @@ static inline bool nem_write_mosaic_u64(nem_transaction_ctx *ctx,
                                         const char *name, uint64_t value) {
   char buffer[21] = {0};
 
-  if (bn_format_uint64(value, NULL, NULL, 0, 0, false, buffer,
+  if (bn_format_uint64(value, NULL, NULL, 0, 0, false, 0, buffer,
                        sizeof(buffer)) == 0) {
     return false;
   }

crypto/tests/test_bignum.py

@@ -5,6 +5,7 @@
 import random
 from ctypes import (
     c_bool,
+    c_char,
     c_int,
     c_size_t,
     c_uint,
@@ -607,23 +608,27 @@ def assert_bn_divmod10(x_old):
     assert r == x_old % 10
 
 
-def assert_bn_format(x, prefix, suffix, decimals, exponent, trailing):
-    def format(amount, prefix, suffix, decimals, exponent, trailing):
+def assert_bn_format(x, prefix, suffix, decimals, exponent, trailing, thousands):
+    def format(amount, prefix, suffix, decimals, exponent, trailing, thousands):
         if exponent >= 0:
             amount *= 10**exponent
         else:
             amount //= 10 ** (-exponent)
 
         d = pow(10, decimals)
 
+        integer_part = amount // d
+        integer_str = f"{integer_part:,}".replace(",", thousands or "")
+
         if decimals:
-            output = "%d.%0*d" % (amount // d, decimals, amount % d)
+            decimal_part = amount % d
+            decimal_str = f".{decimal_part:0{decimals}d}"
             if not trailing:
-                output = output.rstrip("0").rstrip(".")
+                decimal_str = decimal_str.rstrip("0").rstrip(".")
         else:
-            output = "%d" % (amount // d)
+            decimal_str = ""
 
-        return prefix + output + suffix
+        return prefix + integer_str + decimal_str + suffix
 
     def string_to_char_p(string):
         return ctypes.create_string_buffer(string.encode("ascii"))
@@ -641,11 +646,12 @@ def char_p_to_string(pointer):
         c_uint(decimals),
         c_int(exponent),
         c_bool(trailing),
+        c_char(0),
         output,
         c_size_t(output_length),
     )
 
-    correct_output = format(x, prefix, suffix, decimals, exponent, trailing)
+    correct_output = format(x, prefix, suffix, decimals, exponent, trailing, "")
     correct_return_value = len(correct_output)
     if len(correct_output) >= output_length:
         correct_output = ""
@@ -1018,15 +1024,16 @@ def test_bn_divmod10(r):
 
 
 @pytest.mark.parametrize(
-    "decimals,exponent,trailing,prefix,suffix,value",
+    "decimals,exponent,trailing,prefix,suffix,thousands,value",
     itertools.product(
         range(0, 5),
         range(-5, 5),
         [True, False],
         ["", "prefix"],
         ["", "suffix"],
-        [123, 120],
+        ["", ",", " "],
+        [123, 120, 123_456, 12_345, 100001, 10001000],
     ),
 )
-def test_bn_format(decimals, exponent, trailing, prefix, suffix, value):
-    assert_bn_format(value, prefix, suffix, decimals, exponent, trailing)
+def test_bn_format(decimals, exponent, trailing, prefix, suffix, thousands, value):
+    assert_bn_format(value, prefix, suffix, decimals, exponent, trailing, thousands)