Merge pull request #270 from LedgerHQ/revamp

Refactoring and Stax UX revamp

CHANGELOG.md

@@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 Dates are in `dd-mm-yyyy` format.
 
+## [2.2.4] - 09-07-2024
+
+### Changed
+
+- Major revamp of the UI for transaction signing and wallet policy registration on Stax. Changed "wallet policy" with the simpler wording "account".
+- Slight performance improvements in the signing flow.
+- Added a technical limit of at most 10 distinct cosigners in a wallet policy.
+
+### Fixed
+
+- OP_RETURN outputs with a `0x00` data push were incorrectly rejected.
+
 ## [2.2.3] - 06-05-2024
 
 ### Added

Makefile

@@ -47,7 +47,7 @@ PATH_SLIP21_APP_LOAD_PARAMS = "LEDGER-Wallet policy"
 # Application version
 APPVERSION_M = 2
 APPVERSION_N = 2
-APPVERSION_P = 3
+APPVERSION_P = 4
 APPVERSION_SUFFIX = # if not empty, appended at the end. Do not add a dash.
 
 ifeq ($(APPVERSION_SUFFIX),)

bitcoin_client_js/src/__tests__/automations/register_wallet_accept.json

@@ -2,7 +2,7 @@
   "version": 1,
   "rules": [
     {
-      "regexp": "Register wallet|Wallet name|Wallet policy|Key",
+      "regexp": "Register account|Account name|Wallet policy|Key",
       "actions": [
         ["button", 2, true],
         ["button", 2, false]

bitcoin_client_js/src/__tests__/automations/sign_with_wallet_accept.json

@@ -2,7 +2,7 @@
   "version": 1,
   "rules": [
     {
-      "regexp": "Spend from|Wallet name|Review|Amount|Address|Fees",
+      "regexp": "Spend from|Account name|Review|Amount|Address|Fees",
       "actions": [
         ["button", 2, true],
         ["button", 2, false]

ragger_bitcoin/ragger_instructions.py

@@ -38,16 +38,22 @@ def nano_skip_screen(self, text, save_screenshot=True):
         self.new_request(text, NavInsID.RIGHT_CLICK, NavInsID.RIGHT_CLICK,
                          save_screenshot=save_screenshot)
 
-    def navigate_end_of_flow(self, save_screenshot=True):
-        self.new_request("Processing", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_REVIEW_TAP,
-                         save_screenshot=save_screenshot)
-
-    def review_start(self, output_count: int = 1, save_screenshot=True):
+    def review_start(self, output_count: int = 1, save_screenshot=True, has_warning=False):
         self.new_request("Review", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_REVIEW_TAP,
-                         save_screenshot=save_screenshot)
-        for _ in range(0, output_count):
-            self.new_request("Amount", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_REVIEW_TAP,
-                         save_screenshot=save_screenshot)
+                        save_screenshot=save_screenshot)
+
+        if has_warning:
+            self.same_request("Warning", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_CHOICE_CONFIRM,
+                            save_screenshot=save_screenshot)
+
+        for output_index in range(0, output_count):
+            # the initial 2 outputs are cached; that depends on the N_CACHED_EXTERNAL_OUTPUTS constant
+            if output_index < 2:
+                self.same_request("Amount", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_REVIEW_TAP,
+                            save_screenshot=save_screenshot)
+            else:
+                self.new_request("Amount", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_REVIEW_TAP,
+                            save_screenshot=save_screenshot)
     def review_fees(self, fees_on_same_request: bool = True, save_screenshot=True):
         if fees_on_same_request:
             self.same_request("Fees", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_REVIEW_TAP,
@@ -94,10 +100,6 @@ def reject_message(self, save_screenshot=True):
         self.new_request("Message", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_STATUS_DISMISS,
                          save_screenshot=save_screenshot)
 
-    def warning_accept(self, save_screenshot=True):
-        self.new_request("Warning", NavInsID.USE_CASE_REVIEW_TAP, NavInsID.USE_CASE_CHOICE_CONFIRM,
-                         save_screenshot=save_screenshot)
-
     def address_confirm(self, save_screenshot=True):
         self.new_request("Confirm", NavInsID.USE_CASE_REVIEW_TAP,
                          NavInsID.USE_CASE_ADDRESS_CONFIRMATION_CONFIRM,

src/boilerplate/io.c

@@ -85,6 +85,20 @@ void io_reset_timeouts() {
     G_was_processing_screen_shown = false;
 }
 
+void io_show_processing_screen() {
+    if (!G_was_processing_screen_shown) {
+        G_was_processing_screen_shown = true;
+        if (!G_swap_state.called_from_swap) {
+#ifdef HAVE_BAGL
+            ux_flow_init(0, ux_processing_flow, NULL);
+#endif  // HAVE_BAGL
+#ifdef HAVE_NBGL
+            nbgl_useCaseSpinner("Processing");
+#endif  // HAVE_NBGL
+        }
+    }
+}
+
 uint8_t io_event(uint8_t channel) {
     (void) channel;
 
@@ -121,18 +135,7 @@ uint8_t io_event(uint8_t channel) {
                 G_ticks - G_processing_timeout_start_tick >= PROCESSING_TIMEOUT_TICKS) {
                 io_clear_processing_timeout();
 
-                if (!G_was_processing_screen_shown) {
-                    G_was_processing_screen_shown = true;
-#ifdef HAVE_BAGL
-                    ux_flow_init(0, ux_processing_flow, NULL);
-#endif  // HAVE_BAGL
-#ifdef HAVE_NBGL
-
-                    if (!G_swap_state.called_from_swap) {
-                        nbgl_useCaseSpinner("Processing");
-                    }
-#endif  // HAVE_NBGL
-                }
+                io_show_processing_screen();
             }
 
             if (G_is_timeout_active.interruption &&

src/boilerplate/io.h

@@ -54,6 +54,11 @@ void io_clear_processing_timeout();
  */
 void io_reset_timeouts();
 
+/**
+ * Shows the "Processing..." screen.
+ */
+void io_show_processing_screen();
+
 /**
  * TODO: docs
  */

src/common/base58.c

@@ -43,14 +43,8 @@ char const BASE58_ALPHABET[] = {
 };
 
 int base58_decode(const char *in, size_t in_len, uint8_t *out, size_t out_len) {
-#ifdef USE_CXRAM_SECTION
-    // allocate buffers inside the cxram section; safe as there are no syscalls here
-    uint8_t *tmp = get_cxram_buffer();                          // MAX_DEC_INPUT_SIZE bytes buffer
-    uint8_t *buffer = get_cxram_buffer() + MAX_DEC_INPUT_SIZE;  // MAX_DEC_INPUT_SIZE bytes buffer
-#else
     uint8_t tmp[MAX_DEC_INPUT_SIZE] = {0};
     uint8_t buffer[MAX_DEC_INPUT_SIZE] = {0};
-#endif
 
     memset(tmp, 0, MAX_DEC_INPUT_SIZE);
     memset(buffer, 0, MAX_DEC_INPUT_SIZE);

src/common/script.c

@@ -189,8 +189,8 @@ int format_opscript_script(const uint8_t script[],
         }
 
         if (hex_length == 1) {
-            if (script[offset] == 0x81 || script[offset] <= 16) {
-                // non-standard, it should use OP_1NEGATE, or one of OP_0, ..., OP_16
+            if (script[offset] == 0x81 || (1 <= script[offset] && script[offset] <= 16)) {
+                // non-standard, it should use OP_1NEGATE, or one of OP_1, ..., OP_16
                 return -1;
             }
         }
@@ -219,3 +219,24 @@ int format_opscript_script(const uint8_t script[],
     out[out_ctr - 1] = '\0';
     return out_ctr;
 }
+
+#ifndef SKIP_FOR_CMOCKA
+
+bool format_script(const uint8_t script[],
+                   size_t script_len,
+                   char out[static MAX_OUTPUT_SCRIPT_DESC_SIZE]) {
+    int address_len = get_script_address(script, script_len, out, MAX_OUTPUT_SCRIPT_DESC_SIZE);
+    if (address_len < 0) {
+        // script does not have an address; check if OP_RETURN
+        if (is_opreturn(script, script_len)) {
+            if (0 > format_opscript_script(script, script_len, out)) {
+                return false;
+            }
+        } else {
+            return false;
+        }
+    }
+    return true;
+}
+
+#endif

src/common/script.h

@@ -1,5 +1,9 @@
 #pragma once
 
+#include "os.h"
+
+#include "../constants.h"
+
 /** Script opcodes */
 // from bitcoin-core
 enum opcodetype {
@@ -232,4 +236,23 @@ int get_script_address(const uint8_t script[], size_t script_len, char *out, siz
  */
 int format_opscript_script(const uint8_t script[],
                            size_t script_len,
-                           char out[static MAX_OPRETURN_OUTPUT_DESC_SIZE]);
+                           char out[static MAX_OPRETURN_OUTPUT_DESC_SIZE]);
+
+// the maximum length of the description of an output that we can display (address or OP_RETURN),
+// including the terminating null character
+#define MAX_OUTPUT_SCRIPT_DESC_SIZE MAX(MAX_ADDRESS_LENGTH_STR + 1, MAX_OPRETURN_OUTPUT_DESC_SIZE)
+
+/**
+ * Formats a bitcoin Script in the format that is displayed to the user. Only scripts with an
+ * address are supported, or OP_RETURN scripts as documented in format_opscript_script.
+ *
+ * The string is written onto `out` and is 0-terminated.
+ *
+ * @param[in] script the script to parse and format.
+ * @param[in] script_len the length of the script.
+ * @param[out] out the output array, that must be at least MAX_OPRETURN_OUTPUT_DESC_SIZE bytes long.
+ * @return `true` the script is a supported one that can be shown to the user, `false` otherwise.
+ */
+bool format_script(const uint8_t script[],
+                   size_t script_len,
+                   char out[static MAX_OUTPUT_SCRIPT_DESC_SIZE]);

src/common/wallet.c

@@ -1969,8 +1969,6 @@ static int16_t maxcheck(int16_t a, int16_t b) {
 // Maximum supported value for n in a thresh miniscript operator (technical limitation)
 #define MAX_N_IN_THRESH 128
 
-// Separated from the main function as it is stack-intensive, therefore we allocate large buffers
-// into the CXRAM section. There is some repeated work ()
 static int compute_thresh_ops(const policy_node_thresh_t *node,
                               miniscript_ops_t *out,
                               MiniscriptContext ctx) {
@@ -2010,8 +2008,6 @@ static int compute_thresh_ops(const policy_node_thresh_t *node,
     return 0;
 }
 
-// Separated from the main function as it is stack-intensive, therefore we allocate large buffers
-// into the CXRAM section. There is some repeated work ()
 static int compute_thresh_stacksize(const policy_node_thresh_t *node,
                                     miniscript_stacksize_t *out,
                                     MiniscriptContext ctx) {

src/common/wallet.h

@@ -37,6 +37,10 @@
 // longest supported policy in V1 is "sh(wsh(sortedmulti(5,@0,@1,@2,@3,@4)))", 38 bytes
 #define MAX_DESCRIPTOR_TEMPLATE_LENGTH_V1 40
 
+// Maximum number of keys supported for a wallet policy. It is a technical limit to
+// bound the total memory occupation of a wallet policy, and could be increased if necessary.
+#define MAX_N_KEYS_IN_WALLET_POLICY 10
+
 // This amount should be enough for many useful policies
 // We do not expect these limits to be reached in practice any time soon, but they can
 // be further increased if necessary.

src/constants.h

@@ -46,6 +46,11 @@
  */
 #define MAX_N_INPUTS_CAN_SIGN 512
 
+/**
+ * Maximum number of outputs supported while signing a transaction.
+ */
+#define MAX_N_OUTPUTS_CAN_SIGN 512
+
 // SIGHASH flags
 #define SIGHASH_DEFAULT      0x00000000
 #define SIGHASH_ALL          0x00000001