Merge pull request #14 from LedgerHQ/fix-a-few-things

fix and improve the CI workflow

.github/workflows/ci-workflow.yml

@@ -3,85 +3,157 @@ name: Compilation & tests
 on: [push, pull_request]
 
 jobs:
+  scan-build:
+    name: Clang Static Analyzer
+    runs-on: ubuntu-latest
+
+    container:
+      image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder:latest
+
+    steps:
+      - uses: actions/checkout@v2
+
+      - name: Build with Clang Static Analyzer
+        run: |
+          make clean
+          scan-build --use-cc=clang -analyze-headers -enable-checker security -enable-checker unix -enable-checker valist -o scan-build --status-bugs make default
+
+      - uses: actions/upload-artifact@v2
+        if: failure()
+        with:
+          name: scan-build
+          path: scan-build
+
   job_build_debug:
     name: Build debug
     runs-on: ubuntu-latest
 
     container:
-      image: docker://ledgerhq/ledger-app-builder:1.6.1-2
+      image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder:latest
 
     steps:
       - name: Clone
         uses: actions/checkout@v2
 
-      - name: Build
+      - name: Build Nano S app
         run: |
           make DEBUG=1
 
       - name: Upload app binary
         uses: actions/upload-artifact@v2
         with:
-          name: xrp-app-debug
+          name: app-debug-nanos
+          path: bin
+
+      - name: Cleanup the repository
+        run: |
+          git clean -dxf
+
+      - name: Build Nano X app
+        run: |
+          make DEBUG=1 BOLOS_SDK=$NANOX_SDK
+
+      - name: Upload app binary
+        uses: actions/upload-artifact@v2
+        with:
+          name: app-debug-nanox
           path: bin
 
   job_unit_test:
     name: Unit tests
-    needs: job_build_debug
     runs-on: ubuntu-latest
 
     container:
-      image: docker://ledgerhq/ledger-app-builder:1.6.1-2
+      image: ghcr.io/ledgerhq/ledger-app-builder/ledger-app-builder:latest
 
     steps:
       - name: Clone
         uses: actions/checkout@v2
 
-      - name: Install dependancies
+      - name: Install dependencies
         run: |
-          apt-get install libssl-dev
+          apt-get update -q
+          apt-get install -qy libssl-dev
 
       - name: Build unit tests
         run: |
-          cmake -Btests/build -Htests/ && make -C tests/build/ && make -C tests/build test
+          cmake -Btests/build -Htests/ && make -C tests/build/
+
+      - name: Run unit tests
+        run: |
+          make -C tests/build test
 
   job_test:
     name: Functional tests
     needs: job_build_debug
     runs-on: ubuntu-latest
 
     container:
-      image: docker://ledgerhq/speculos:latest
-      ports:
-        - 1234:1234
-        - 9999:9999
-        - 40000:40000
-        - 41000:41000
-        - 42000:42000
-        - 43000:43000
+      image: ghcr.io/ledgerhq/speculos:latest
       options: --entrypoint /bin/bash
 
     steps:
       - name: Clone
         uses: actions/checkout@v2
 
-      - name: Install dependancies
+      - name: Install dependencies
         run: |
-          apt-get update && apt-get install build-essential -y libudev-dev libusb-1.0-0-dev libfox-1.6-dev
+          pip install -r tests/requirements.txt
+          apt-get update -q
+          apt-get install -qy netcat
 
-      - name: Download app binary
+      - name: Download Nano S app binary
         uses: actions/download-artifact@v2
         with:
-          name: xrp-app-debug
-          path: bin
+          name: app-debug-nanos
+          path: bin-nanos
+
+      - name: Download Nano X app binary
+        uses: actions/download-artifact@v2
+        with:
+          name: app-debug-nanox
+          path: bin-nanox
+
+      - name: Run speculos in the background
+        run: |
+          /speculos/speculos.py --display headless --automation file:tests/automation.json bin-nanos/app.elf 2>speculos.log &
+          echo $! >/tmp/speculos.pid
+          timeout 10 sh -c 'until nc -z 127.0.0.1 9999; do sleep 1; done'
+
+      - name: Run tests against the Nano S app
+        env:
+          LEDGER_PROXY_ADDRESS: 127.0.0.1
+          LEDGER_PROXY_PORT: 9999
+        run: |
+          pytest tests/
 
-      - name: Run test
+      - name: Upload speculos log
+        uses: actions/upload-artifact@v2
+        if: failure()
+        with:
+          name: speculos-nanos-log
+          path: speculos.log
+
+      - name: Kill speculos
+        run: |
+          kill -9 $(cat /tmp/speculos.pid)
+
+      - name: Run speculos in the background
+        run: |
+          /speculos/speculos.py --model nanox --display headless --automation file:tests/automation.json bin-nanox/app.elf 2>speculos.log &
+          echo $! >/tmp/speculos.pid
+          timeout 10 sh -c 'until nc -z 127.0.0.1 9999; do sleep 1; done'
+
+      - name: Run tests against the Nano X app
+        env:
+          LEDGER_PROXY_ADDRESS: 127.0.0.1
+          LEDGER_PROXY_PORT: 9999
         run: |
-          nohup bash -c "python /speculos/speculos.py bin/app.elf --sdk 1.6  --log-level automation:DEBUG --automation file:tests/automation.json --display headless" > speculos.log 2<&1 &
-          sleep 4
-          cd tests && pip install -r requirements.txt && LEDGER_PROXY_ADDRESS=127.0.0.1 LEDGER_PROXY_PORT=9999 pytest -v -s
+          pytest tests/
 
-      - name: Upload Speculos log
+      - name: Upload speculos log
         uses: actions/upload-artifact@v2
+        if: failure()
         with:
-          name: speculos-log
+          name: speculos-nanox-log
           path: speculos.log

src/apdu/messages/sign_transaction.c

@@ -243,6 +243,8 @@ void handle_packet_content(uint8_t p1,
                            uint8_t *work_buffer,
                            uint8_t data_length,
                            volatile unsigned int *flags) {
+    UNUSED(p2);
+
     uint16_t total_length = prefix_length + parse_context.length + data_length;
     if (total_length > MAX_RAW_TX) {
         // Abort if the user is trying to sign a too large transaction

src/ui/other/loading.c

@@ -96,7 +96,7 @@ void execute_async(action_t action_to_load, char* message) {
     pending_action = action_to_load;
 
     memset(loading_message, 0, sizeof(loading_message));
-    os_memmove(loading_message, message, MIN(sizeof(loading_message) - 1, strlen(message)));
+    memmove(loading_message, message, MIN(sizeof(loading_message) - 1, strlen(message)));
 
     UX_DISPLAY(loading_ui, loading_ui_button_prepro)
 }

src/xrp/flags.c

@@ -100,7 +100,7 @@ static void format_account_set_transaction_flags(uint32_t value, field_value_t *
     }
 
     if (HAS_FLAG(value, TF_ALLOW_XRP)) {
-        offset = append_item(dst, offset, "Allow XRP");
+        append_item(dst, offset, "Allow XRP");
     }
 }
 
@@ -175,7 +175,7 @@ static void format_offer_create_flags(uint32_t value, field_value_t *dst) {
     }
 
     if (HAS_FLAG(value, TF_SELL)) {
-        offset = append_item(dst, offset, "Sell");
+        append_item(dst, offset, "Sell");
     }
 }
 
@@ -195,7 +195,7 @@ static void format_payment_flags(uint32_t value, field_value_t *dst) {
     }
 
     if (HAS_FLAG(value, TF_LIMIT_QUALITY)) {
-        offset = append_item(dst, offset, "Limit Quality");
+        append_item(dst, offset, "Limit Quality");
     }
 }
 
@@ -225,7 +225,7 @@ static void format_trust_set_flags(uint32_t value, field_value_t *dst) {
     }
 
     if (HAS_FLAG(value, TF_CLEAR_FREEZE)) {
-        offset = append_item(dst, offset, "Clear Freeze");
+        append_item(dst, offset, "Clear Freeze");
     }
 }
 
@@ -240,7 +240,7 @@ static void format_payment_channel_claim_flags(uint32_t value, field_value_t *ds
     }
 
     if (HAS_FLAG(value, TF_CLOSE)) {
-        offset = append_item(dst, offset, "Close");
+        append_item(dst, offset, "Close");
     }
 }
 

src/xrp/time.c

@@ -67,7 +67,6 @@ static int ripple_epoch_to_tm(long long t, tm_mini_t *tm) {
     int remdays, remsecs, remyears;
     int qc_cycles, c_cycles, q_cycles;
     int years, months;
-    int wday, yday, leap;
     static const char days_in_month[] = {31, 30, 31, 30, 31, 31, 30, 31, 30, 31, 31, 29};
 
     /* Reject time_t values whose year would overflow int */
@@ -81,9 +80,6 @@ static int ripple_epoch_to_tm(long long t, tm_mini_t *tm) {
         days--;
     }
 
-    wday = (3 + days) % 7;
-    if (wday < 0) wday += 7;
-
     qc_cycles = days / DAYS_PER_400Y;
     remdays = days % DAYS_PER_400Y;
     if (remdays < 0) {
@@ -103,10 +99,6 @@ static int ripple_epoch_to_tm(long long t, tm_mini_t *tm) {
     if (remyears == 4) remyears--;
     remdays -= remyears * 365;
 
-    leap = !remyears && (q_cycles || !c_cycles);
-    yday = remdays + 31 + 28 + leap;
-    if (yday >= 365 + leap) yday -= 365 + leap;
-
     years = remyears + 4 * q_cycles + 100 * c_cycles + 400 * qc_cycles;
 
     for (months = 0; days_in_month[months] <= remdays; months++) remdays -= days_in_month[months];

src/xrp/xrp_helpers.c

@@ -109,7 +109,8 @@ size_t xrp_public_key_to_encoded_base58(xrp_pubkey_t *pubkey,
 
     if (pubkey != NULL) {
         xrp_public_key_hash160(pubkey, tmp.buf + version_size);
-    } else {
+    }
+    if (account != NULL) {
         memmove(tmp.buf + version_size, account->buf, sizeof(account->buf));
     }
 

tests/automation.json

@@ -2,7 +2,7 @@
     "version": 1,
     "rules": [
         {
-            "regexp": "Account$|Amount|Balance|Clear Flag|Currency.*|Deliver Min|Destination|Domain|Expiration|Flags|Fulfillment|Invoice|Issuer|Limit Amount|Memo Fmt|Quality In|Quality Out|Regular Key|Send Max|Signer Quorum|Transfer Rate|Tick Size|Set Flag|Settle Delay|Signer Weight.*|Source Tag|Taker Gets|Taker Pays|Transaction Type|Offer Sequence|Fee|Txn Sig|Owner|Memo Data|.*\\(\\d/\\d\\)",
+            "regexp": "Account$|Account\n|Amount|Authorize|Balance|Cancel After|Clear Flag|Currency.*|Deliver Min|Destination|Domain|Email Hash|Expiration|Flags|Finish After|Fulfillment|Invoice|[I|l]ssuer|Limit Amount|Memo Fmt|Quality [Il]n|Quality Out|Regular Key|Send Max|Signer Quorum|Transfer Rate|Tick Size|Unauthorize|Set Flag|Settle Delay|Signer Weight.*|Source Tag|Taker Gets|Taker Pays|Transaction Type|Offer Sequence|Fee|Txn Sig|Owner|Memo Data|.*\\(\\d/\\d\\)|.*\\[\\d\\]|.*\\[P\\d\\:\\s*S\\d\\]",
             "actions": [
                 [ "button", 2, true ],
                 [ "button", 2, false ]

tests/functional_test.py

@@ -7,38 +7,38 @@
 pytest-3 -v -s
 """
 
-import binascii
-import json
 import pytest
 import sys
-import time
 from enum import IntEnum
 
 from ledgerwallet.client import LedgerClient, CommException
-from ledgerwallet.crypto.ecc import PrivateKey
 from ledgerwallet.params import Bip32Path
 from ledgerwallet.transport import enumerate_devices
 
 DEFAULT_PATH = "44'/144'/0'/0'/0"
 CLA = 0xE0
 
+
 class Ins(IntEnum):
     GET_PUBLIC_KEY = 0x02
-    SIGN           = 0x04
+    SIGN = 0x04
+
 
 class P1(IntEnum):
     NON_CONFIRM = 0x00
-    CONFIRM     = 0x01
-    FIRST       = 0x00
-    NEXT        = 0x01
-    LAST        = 0x00
-    MORE        = 0x80
+    CONFIRM = 0x01
+    FIRST = 0x00
+    NEXT = 0x01
+    LAST = 0x00
+    MORE = 0x80
+
 
 class P2(IntEnum):
-    NO_CHAIN_CODE   = 0x00
-    CHAIN_CODE      = 0x01
+    NO_CHAIN_CODE = 0x00
+    CHAIN_CODE = 0x01
     CURVE_SECP256K1 = 0x40
-    CURVE_ED25519   = 0x80
+    CURVE_ED25519 = 0x80
+
 
 @pytest.fixture(scope="module")
 def client():
@@ -49,6 +49,7 @@ def client():
 
     return LedgerClient(devices[0], cla=CLA)
 
+
 class TestGetPublicKey:
     INS = Ins.GET_PUBLIC_KEY
 
@@ -62,6 +63,7 @@ def test_path_too_long(self, client):
             client.apdu_exchange(self.INS, path, P1.NON_CONFIRM, P2.CURVE_SECP256K1)
         assert e.value.sw == 0x6a80
 
+
 class TestSign:
     INS = Ins.SIGN
 
@@ -90,14 +92,14 @@ def test_sign_too_large(self, client):
         payload = path + b"a" * (max_size - 4)
         with pytest.raises(CommException) as e:
             self._send_payload(client, payload)
-        assert e.value.sw == 0x6700
+        assert e.value.sw in [0x6700, 0x6813]
 
     def test_sign_invalid_tx(self, client):
         path = Bip32Path.build(DEFAULT_PATH)
         payload = path + b"a" * (40)
         with pytest.raises(CommException) as e:
             self._send_payload(client, payload)
-        assert e.value.sw == 0x6803
+        assert e.value.sw in [0x6803, 0x6807]
 
     def test_sign_valid_tx(self, client, raw_tx_path):
         if raw_tx_path.endswith("19-really-stupid-tx.raw"):

tests/requirements.txt

@@ -1,2 +1,7 @@
 pytest>=6.1.1,<7.0.0
-ledgerwallet>=0.1.2
+ledgerwallet>=0.1.2
+
+# This is a temporary fix for this issue: https://github.com/LedgerHQ/ledgerctl/issues/17
+# (fixed in https://github.com/LedgerHQ/ledgerctl/commit/cd30a2e8cefa7bb9b417b7c5f370fb26e8505f01).
+# This line could be removed once a new release of ledgerwallet > 0.1.2 includes this fix.
+construct==2.10.61