[FEATURE][LIVE-10220] make Qtum work with old and new nano app interf…

…aces (#5675)

* make qtum work with old and new nano app interfaces

update isPathNormal to allow qtum coin type (88)

* misc: code improvement

- use switch instead of if else cascade
- use semver for nano apps versions related tests

* add changeset

* add tests for isPathNormal

* improve documentation and readability of isPathNormal function

* update Qtum version using new btc implem to 3.0.0

* add regression tests regarding semver use

.changeset/nice-bobcats-drop.md

@@ -0,0 +1,5 @@
+---
+"@ledgerhq/hw-app-btc": minor
+---
+
+make Qtum work with old and new nano app interfaces

libs/ledgerjs/packages/hw-app-btc/src/Btc.ts

@@ -1,3 +1,4 @@
+import semver from "semver";
 import type Transport from "@ledgerhq/hw-transport";
 import BtcNew from "./BtcNew";
 import BtcOld from "./BtcOld";
@@ -49,12 +50,19 @@ export default class Btc {
       ],
       scrambleKey,
     );
-    // new APDU (nano app API) for bitcoin and old APDU for altcoin
-    if (currency === "bitcoin" || currency === "bitcoin_testnet") {
-      this._impl = new BtcNew(new AppClient(this._transport));
-    } else {
-      this._impl = new BtcOld(this._transport);
-    }
+
+    this._impl = (() => {
+      switch (currency) {
+        case "bitcoin":
+        case "bitcoin_testnet":
+        case "qtum":
+          // new APDU (nano app API) for currencies using app-bitcoin-new implementation
+          return new BtcNew(new AppClient(this._transport));
+        default:
+          // old APDU (legacy API) for currencies using legacy bitcoin app implementation
+          return new BtcOld(this._transport);
+      }
+    })();
   }
 
   /**
@@ -263,25 +271,31 @@ export default class Btc {
     // if BtcOld was instantiated, stick with it
     if (this._impl instanceof BtcOld) return this._impl;
 
-    const appAndVersion = await getAppAndVersion(this._transport);
-    let isBtcLegacy = true; // default for all altcoins
+    const { name, version } = await getAppAndVersion(this._transport);
 
-    if (appAndVersion.name === "Bitcoin" || appAndVersion.name === "Bitcoin Test") {
-      const [major, minor] = appAndVersion.version.split(".");
-      // we use the legacy protocol for versions below 2.1.0 of the Bitcoin app.
-      isBtcLegacy = parseInt(major) <= 1 || (parseInt(major) == 2 && parseInt(minor) == 0);
-    } else if (
-      appAndVersion.name === "Bitcoin Legacy" ||
-      appAndVersion.name === "Bitcoin Test Legacy"
-    ) {
-      // the "Bitcoin Legacy" and "Bitcoin Testnet Legacy" app use the legacy protocol, regardless of the version
-      isBtcLegacy = true;
-    } else if (appAndVersion.name === "Exchange") {
-      // We can't query the version of the Bitcoin app if we're coming from Exchange;
-      // therefore, we use a workaround to distinguish legacy and new versions.
-      // This can be removed once Ledger Live enforces minimum bitcoin version >= 2.1.0.
-      isBtcLegacy = await checkIsBtcLegacy(this._transport);
-    }
+    const isBtcLegacy = await (async () => {
+      switch (name) {
+        case "Bitcoin":
+        case "Bitcoin Test": {
+          // we use the legacy protocol for versions below 2.1.0 of the Bitcoin app.
+          return semver.lt(version, "2.1.0");
+        }
+        case "Bitcoin Legacy":
+        case "Bitcoin Test Legacy":
+          // the "Bitcoin Legacy" and "Bitcoin Testnet Legacy" app use the legacy protocol, regardless of the version
+          return true;
+        case "Exchange":
+          // We can't query the version of the Bitcoin app if we're coming from Exchange;
+          // therefore, we use a workaround to distinguish legacy and new versions.
+          // This can be removed once Ledger Live enforces minimum bitcoin version >= 2.1.0.
+          return await checkIsBtcLegacy(this._transport);
+        case "Qtum":
+          // we use the legacy protocol for versions below 3.0.0 of the Qtum app.
+          return semver.lt(version, "3.0.0");
+        default:
+          return true;
+      }
+    })();
 
     if (isBtcLegacy) {
       this._impl = new BtcOld(this._transport);

libs/ledgerjs/packages/hw-app-btc/src/BtcNew.ts

@@ -445,11 +445,11 @@ function accountTypeFromArg(
 /*
   The new protocol only allows standard path.
   Standard paths are (currently):
-  M/44'/(1|0)'/X'
-  M/49'/(1|0)'/X'
-  M/84'/(1|0)'/X'
-  M/86'/(1|0)'/X'
-  M/48'/(1|0)'/X'/Y'
+  M/44'/(1|0|88)'/X'
+  M/49'/(1|0|88)'/X'
+  M/84'/(1|0|88)'/X'
+  M/86'/(1|0|88)'/X'
+  M/48'/(1|0|88)'/X'/Y'
   followed by "", "(0|1)", or "(0|1)/b", where a and b are 
   non-hardened. For example, the following paths are standard
   M/48'/1'/99'/7'
@@ -460,32 +460,60 @@ function accountTypeFromArg(
   M/48'/0'/99'/7'/1/17/2 // Too many non-hardened derivation steps
   M/199'/0'/1'/0/88      // Not a known purpose 199
   M/86'/1'/99'/2         // Change path item must be 0 or 1
+
+  Useful resource on derivation paths: https://learnmeabitcoin.com/technical/derivation-paths
 */
-function isPathNormal(path: string): boolean {
-  //path is not deepest hardened node of a standard path or deeper, use BtcOld
-  const h = 0x80000000; //HARDENED from bip32
-  const pathElems = pathStringToArray(path);
 
-  const hard = (n: number) => n >= h;
-  const soft = (n: number | undefined) => n === undefined || n < h;
-  const change = (n: number | undefined) => n === undefined || n === 0 || n === 1;
+//path is not deepest hardened node of a standard path or deeper, use BtcOld
+const H = 0x80000000; //HARDENED from bip32
+
+const VALID_COIN_TYPES = [
+  0, // Bitcoin
+  1, // Bitcoin (Testnet)
+  88, // Qtum
+];
+
+const VALID_SINGLE_SIG_PURPOSES = [
+  44, // BIP44 - https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki
+  49, // BIP49 - https://github.com/bitcoin/bips/blob/master/bip-0049.mediawiki
+  84, // BIP84 - https://github.com/bitcoin/bips/blob/master/bip-0084.mediawiki
+  86, // BIP86 - https://github.com/bitcoin/bips/blob/master/bip-0086.mediawiki
+];
+
+const VALID_MULTISIG_PURPOSES = [
+  48, // BIP48 - https://github.com/bitcoin/bips/blob/master/bip-0048.mediawiki
+];
 
+const hard = (n: number) => n >= H;
+const soft = (n: number | undefined) => n === undefined || n < H;
+const change = (n: number | undefined) => n === undefined || n === 0 || n === 1;
+
+const validCoinPathPartsSet = new Set(VALID_COIN_TYPES.map(t => t + H));
+const validSingleSigPurposePathPartsSet = new Set(VALID_SINGLE_SIG_PURPOSES.map(t => t + H));
+const validMultiSigPurposePathPartsSet = new Set(VALID_MULTISIG_PURPOSES.map(t => t + H));
+
+export function isPathNormal(path: string): boolean {
+  const pathElems = pathStringToArray(path);
+
+  // Single sig
   if (
     pathElems.length >= 3 &&
     pathElems.length <= 5 &&
-    [44 + h, 49 + h, 84 + h, 86 + h].some(v => v == pathElems[0]) &&
-    [0 + h, 1 + h].some(v => v == pathElems[1]) &&
+    validSingleSigPurposePathPartsSet.has(pathElems[0]) &&
+    validCoinPathPartsSet.has(pathElems[1]) &&
     hard(pathElems[2]) &&
     change(pathElems[3]) &&
     soft(pathElems[4])
   ) {
     return true;
   }
+
+  // Multi sig
   if (
     pathElems.length >= 4 &&
     pathElems.length <= 6 &&
-    48 + h == pathElems[0] &&
-    [0 + h, 1 + h].some(v => v == pathElems[1]) &&
+    validMultiSigPurposePathPartsSet.has(pathElems[0]) &&
+    validCoinPathPartsSet.has(pathElems[1]) &&
     hard(pathElems[2]) &&
     hard(pathElems[3]) &&
     change(pathElems[4]) &&

libs/ledgerjs/packages/hw-app-btc/tests/newops/isPathNormal.test.ts

@@ -0,0 +1,30 @@
+import { isPathNormal } from "../../src/BtcNew";
+
+describe("isPathNormal", () => {
+  describe("should return true for normal and supported paths", () => {
+    test.each([
+      "44'/0'/0'",
+      "44'/0'/0'/0/0",
+      "84'/0'/0'",
+      "84'/0'/0'/0/0",
+      "86'/0'/0'",
+      "86'/0'/0'/0/0",
+      "49'/0'/0'",
+      "49'/0'/0'/0/0",
+      "48'/1'/99'/7'",
+      "86'/1'/99'/0",
+      "48'/0'/99'/7'/1/17",
+    ])("%s", path => {
+      expect(isPathNormal(path)).toEqual(true);
+    });
+  });
+
+  describe("should return false for non-normal or non supported paths", () => {
+    test.each(["48'/0'/99'", "48'/0'/99'/7'/1/17/2", "199'/0'/1'/0/88", "86'/1'/99'/2"])(
+      "%s",
+      path => {
+        expect(isPathNormal(path)).toEqual(false);
+      },
+    );
+  });
+});

libs/ledgerjs/packages/hw-app-btc/tests/semver.test.ts

@@ -0,0 +1,37 @@
+import semver from "semver";
+
+/**
+ * To detect any semver regression that could impact the behavior of the package
+ * implementation when working with versions having prefix.
+ * One might say this is not necessary since it should already be covered by the
+ * semver package own testsuite, but better be safe than sorry.
+ *
+ * Following this comment:
+ * Are you 100% sure this does the same as the current implem?
+ * Just being paranoid, because app versions can be different from a clean
+ * major.minor.patch
+ * e.g they can have -rc0 -next0 or such additional stuff at the end,
+ * and it happened in the past that semver doesn't behave as we think it does.
+ * cf. https://github.com/LedgerHQ/ledger-live/pull/5675/files#r1417174514
+ */
+
+describe("semver", () => {
+  describe("should be able to properly compare version with prefix", () => {
+    it("2.0.0-rc0 should be lower than 2.1.0", () => {
+      const version = "2.0.0-rc1";
+      expect(semver.lt(version, "2.1.0"));
+    });
+    it("2.0.0-next0 should be lower than 2.1.0", () => {
+      const version = "2.0.0-next0";
+      expect(semver.lt(version, "2.1.0"));
+    });
+    it("2.1.0-rc0 should be greater than 2.1.0", () => {
+      const version = "2.1.0-rc0";
+      expect(semver.gt(version, "2.1.0"));
+    });
+    it("2.1.0-next0 should be greater than 2.1.0", () => {
+      const version = "2.1.0-next0";
+      expect(semver.gt(version, "2.1.0"));
+    });
+  });
+});