Added error codes for register_wallet; adapted tests

src/error_codes.h

@@ -1,5 +1,18 @@
 #pragma once
 
+/**
+ * REGISTER_WALLET
+ */
+
+// The name of the policy is not acceptable
+#define EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME 0x0000
+
+// The wallet policy does not respect the requirement of BIP-388, or the sanity rules of miniscript
+#define EC_REGISTER_WALLET_POLICY_NOT_SANE 0x0001
+
+// No key in the wallet policy was recognized as internal.
+#define EC_REGISTER_WALLET_POLICY_HAS_NO_INTERNAL_KEY 0x0002
+
 /**
  * SIGN_PSBT
  */

src/handler/register_wallet.c

@@ -32,6 +32,7 @@
 #include "../commands.h"
 #include "../constants.h"
 #include "../crypto.h"
+#include "../error_codes.h"
 #include "../ui/display.h"
 #include "../ui/menu.h"
 
@@ -106,7 +107,7 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version)
     // Verify that the name is acceptable
     if (!is_policy_name_acceptable(wallet_header.name, wallet_header.name_len)) {
         PRINTF("Policy name is not acceptable\n");
-        SEND_SW(dc, SW_INCORRECT_DATA);
+        SEND_SW_EC(dc, SW_INCORRECT_DATA, EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME);
         return;
     }
 
@@ -126,7 +127,7 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version)
                            wallet_header.n_keys)) {
         PRINTF("Policy is not sane\n");
 
-        SEND_SW(dc, SW_NOT_SUPPORTED);
+        SEND_SW_EC(dc, SW_NOT_SUPPORTED, EC_REGISTER_WALLET_POLICY_NOT_SANE);
         return;
     }
 
@@ -214,7 +215,7 @@ void handler_register_wallet(dispatcher_context_t *dc, uint8_t protocol_version)
         // Unclear if there is any use case for registering policies with no internal keys.
         // We disallow that, might reconsider in future versions if needed.
         PRINTF("Wallet policy with no internal keys\n");
-        SEND_SW(dc, SW_INCORRECT_DATA);
+        SEND_SW_EC(dc, SW_INCORRECT_DATA, EC_REGISTER_WALLET_POLICY_HAS_NO_INTERNAL_KEY);
         return;
     } else if (n_internal_keys != 1 && wallet_header.version == WALLET_POLICY_VERSION_V1) {
         // for legacy policies, we keep the restriction to exactly 1 internal key

tests/test_register_wallet.py

@@ -159,11 +159,20 @@ def test_register_wallet_invalid_names(navigator: Navigator, firmware: Firmware,
         )
 
         with pytest.raises(ExceptionRAPDU) as e:
-            client.register_wallet(wallet, navigator,
+            client.register_wallet(wallet, None,
                                    testname=test_name)
 
         assert DeviceException.exc.get(e.value.status) == IncorrectDataError
-        assert len(e.value.data) == 0
+        # defined in error_codes.h
+        EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME = 0x0000
+
+        if invalid_name == too_long_name:
+            # We don't return an error code for name too long
+            assert len(e.value.data) == 0
+        else:
+            assert len(e.value.data) == 2
+            error_code = int.from_bytes(e.value.data, 'big')
+            assert error_code == EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME
 
 
 def test_register_wallet_missing_key(client: RaggerClient):
@@ -222,7 +231,7 @@ def test_register_miniscript_long_policy(navigator: Navigator, firmware: Firmwar
 
     assert hmac.compare_digest(
         hmac.new(speculos_globals.wallet_registration_key,
-                    wallet_id, sha256).digest(),
+                 wallet_id, sha256).digest(),
         wallet_hmac,
     )
 
@@ -246,7 +255,14 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar
         )
 
     assert DeviceException.exc.get(e.value.status) == NotSupportedError
-    assert len(e.value.data) == 0
+    assert len(e.value.data) == 2
+
+    # defined in error_codes.h
+    EC_REGISTER_WALLET_POLICY_NOT_SANE = 0x0001
+
+    assert len(e.value.data) == 2
+    error_code = int.from_bytes(e.value.data, 'big')
+    assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE
 
     # Key placeholders referring to the same key must have distinct derivations
     with pytest.raises(ExceptionRAPDU) as e:
@@ -261,7 +277,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar
             testname=test_name
         )
     assert DeviceException.exc.get(e.value.status) == NotSupportedError
-    assert len(e.value.data) == 0
+    assert len(e.value.data) == 2
+    error_code = int.from_bytes(e.value.data, 'big')
+    assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE
 
     with pytest.raises(ExceptionRAPDU) as e:
         client.register_wallet(WalletPolicy(
@@ -276,7 +294,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar
             testname=test_name
         )
     assert DeviceException.exc.get(e.value.status) == NotSupportedError
-    assert len(e.value.data) == 0
+    assert len(e.value.data) == 2
+    error_code = int.from_bytes(e.value.data, 'big')
+    assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE
 
     # Miniscript policy with timelock mixing
     with pytest.raises(ExceptionRAPDU) as e:
@@ -291,7 +311,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar
             testname=test_name
         )
     assert DeviceException.exc.get(e.value.status) == NotSupportedError
-    assert len(e.value.data) == 0
+    assert len(e.value.data) == 2
+    error_code = int.from_bytes(e.value.data, 'big')
+    assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE
 
     # Miniscript policy that does not always require a signature
     with pytest.raises(ExceptionRAPDU) as e:
@@ -308,7 +330,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar
             testname=test_name
         )
     assert DeviceException.exc.get(e.value.status) == NotSupportedError
-    assert len(e.value.data) == 0
+    assert len(e.value.data) == 2
+    error_code = int.from_bytes(e.value.data, 'big')
+    assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE
 
     # Malleable policy, even if it requires a signature
     with pytest.raises(ExceptionRAPDU) as e:
@@ -323,7 +347,9 @@ def test_register_wallet_not_sane_policy(navigator: Navigator, firmware: Firmwar
             testname=test_name
         )
     assert DeviceException.exc.get(e.value.status) == NotSupportedError
-    assert len(e.value.data) == 0
+    assert len(e.value.data) == 2
+    error_code = int.from_bytes(e.value.data, 'big')
+    assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE
 
     # TODO: we can probably not trigger stack and ops limits with the current limits we have on the
     # miniscript policy size; otherwise it would be worth to add tests for them, too.

tests/test_register_wallet_v1.py

@@ -32,13 +32,15 @@ def test_register_wallet_accept_legacy_v1(navigator: Navigator, firmware: Firmwa
     )
 
     wallet_id, wallet_hmac = client.register_wallet(wallet, navigator,
-                                                    instructions=register_wallet_instruction_approve(firmware),
+                                                    instructions=register_wallet_instruction_approve(
+                                                        firmware),
                                                     testname=test_name)
 
     assert wallet_id == wallet.id
 
     assert hmac.compare_digest(
-        hmac.new(speculos_globals.wallet_registration_key, wallet_id, sha256).digest(),
+        hmac.new(speculos_globals.wallet_registration_key,
+                 wallet_id, sha256).digest(),
         wallet_hmac,
     )
 
@@ -57,13 +59,15 @@ def test_register_wallet_accept_sh_wit_v1(navigator: Navigator, firmware: Firmwa
     )
 
     wallet_id, wallet_hmac = client.register_wallet(wallet, navigator,
-                                                    instructions=register_wallet_instruction_approve(firmware),
+                                                    instructions=register_wallet_instruction_approve(
+                                                        firmware),
                                                     testname=test_name)
 
     assert wallet_id == wallet.id
 
     assert hmac.compare_digest(
-        hmac.new(speculos_globals.wallet_registration_key, wallet_id, sha256).digest(),
+        hmac.new(speculos_globals.wallet_registration_key,
+                 wallet_id, sha256).digest(),
         wallet_hmac,
     )
 
@@ -82,13 +86,15 @@ def test_register_wallet_accept_wit_v1(navigator: Navigator, firmware: Firmware,
     )
 
     wallet_id, wallet_hmac = client.register_wallet(wallet, navigator,
-                                                    instructions=register_wallet_instruction_approve(firmware),
+                                                    instructions=register_wallet_instruction_approve(
+                                                        firmware),
                                                     testname=test_name)
 
     assert wallet_id == wallet.id
 
     assert hmac.compare_digest(
-        hmac.new(speculos_globals.wallet_registration_key, wallet_id, sha256).digest(),
+        hmac.new(speculos_globals.wallet_registration_key,
+                 wallet_id, sha256).digest(),
         wallet_hmac,
     )
 
@@ -111,7 +117,8 @@ def test_register_wallet_reject_header_v1(navigator: Navigator, firmware: Firmwa
 
     with pytest.raises(ExceptionRAPDU) as e:
         client.register_wallet(wallet, navigator,
-                               instructions=register_wallet_instruction_reject(firmware),
+                               instructions=register_wallet_instruction_reject(
+                                   firmware),
                                testname=test_name)
 
     assert DeviceException.exc.get(e.value.status) == DenyError
@@ -120,12 +127,16 @@ def test_register_wallet_reject_header_v1(navigator: Navigator, firmware: Firmwa
 
 def test_register_wallet_invalid_names_v1(navigator: Navigator, firmware: Firmware, client:
                                           RaggerClient, test_name: str):
+    too_long_name = "This wallet name is much too long since it requires 65 characters"
+    assert len(too_long_name) == 65
+
     for invalid_name in [
         "",  # empty name not allowed
-        "Very long walletz",  # 17 characters is too long
-        " Test", "Test ",  # can't start with spaces
-        "Tæst",  # characters out of allowed range
+        too_long_name,
+        # " Test", "Test ",  # can't start or end with spaces
+        # "Tæst",  # characters out of allowed range
     ]:
+        print("Testing with:", invalid_name)  # TODO: remove
         wallet = MultisigWallet(
             name=invalid_name,
             address_type=AddressType.WIT,
@@ -137,11 +148,20 @@ def test_register_wallet_invalid_names_v1(navigator: Navigator, firmware: Firmwa
             version=WalletType.WALLET_POLICY_V1
         )
 
-    with pytest.raises(ExceptionRAPDU) as e:
-        client.register_wallet(wallet, navigator)
+        with pytest.raises(ExceptionRAPDU) as e:
+            client.register_wallet(wallet, None)
 
-    assert DeviceException.exc.get(e.value.status) == IncorrectDataError
-    assert len(e.value.data) == 0
+        assert DeviceException.exc.get(e.value.status) == IncorrectDataError
+        # defined in error_codes.h
+        EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME = 0x0000
+
+        if invalid_name == too_long_name:
+            # We don't return an error code for name too long
+            assert len(e.value.data) == 0
+        else:
+            assert len(e.value.data) == 2
+            error_code = int.from_bytes(e.value.data, 'big')
+            assert error_code == EC_REGISTER_WALLET_UNACCEPTABLE_POLICY_NAME
 
 
 def test_register_wallet_unsupported_policy_v1(navigator: Navigator, firmware: Firmware, client:
@@ -181,4 +201,9 @@ def test_register_wallet_unsupported_policy_v1(navigator: Navigator, firmware: F
 
     # NotSupportedError
     assert DeviceException.exc.get(e.value.status) == NotSupportedError
-    assert len(e.value.data) == 0
+    # defined in error_codes.h
+    EC_REGISTER_WALLET_POLICY_NOT_SANE = 0x0001
+
+    assert len(e.value.data) == 2
+    error_code = int.from_bytes(e.value.data, 'big')
+    assert error_code == EC_REGISTER_WALLET_POLICY_NOT_SANE