♻️ (signer-zcash): Address PR review comments

may01 · may01 · commit 34c7368e7a40 · 2026-05-21T13:04:43.000+01:00
diff --git a/packages/signer/signer-zcash/package.json b/packages/signer/signer-zcash/package.json
@@ -39,6 +39,7 @@
   },
   "dependencies": {
     "@ledgerhq/signer-utils": "workspace:^",
+    "@noble/hashes": "^1.8.0",
     "inversify": "catalog:",
     "purify-ts": "catalog:",
     "reflect-metadata": "catalog:",
diff --git a/packages/signer/signer-zcash/src/api/model/CreateTransactionArg.ts b/packages/signer/signer-zcash/src/api/model/CreateTransactionArg.ts
@@ -15,7 +15,6 @@ export type LegacyTransaction = {
   inputs: LegacyTransactionInput[];
   outputs?: LegacyTransactionOutput[];
   locktime?: Uint8Array;
-  //witness?: Uint8Array;
   timestamp?: Uint8Array;
   nVersionGroupId?: Uint8Array;
   nExpiryHeight?: Uint8Array;
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/ZcashAppBinder.test.ts b/packages/signer/signer-zcash/src/internal/app-binder/ZcashAppBinder.test.ts
@@ -52,7 +52,7 @@ type ExecuteDeviceActionTaskCallArgs = {
   deviceAction: CallTaskInAppDeviceAction<
     unknown,
     DmkError,
-    UserInteractionRequired.None | UserInteractionRequired.None
+    UserInteractionRequired.None | UserInteractionRequired.SignTransaction
   >;
 };
 
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/GetTrustedInputCommand.test.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/GetTrustedInputCommand.test.ts
@@ -33,7 +33,7 @@ describe("GetTrustedInputCommand", () => {
       expect(apdu.getRawApdu()).toEqual(
         new Uint8Array([
           0xe0, // CLA
-          0x42, // INS.GET_TRUSTED_INPUT
+          0x42, // GET_TRUSTED_INPUT
           0x80, // P1.NEXT
           0x00, // P2.DEFAULT
           0x03, // data length
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/GetTrustedInputCommand.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/GetTrustedInputCommand.ts
@@ -33,7 +33,7 @@ export type GetTrustedInputCommandResponse = ApduResponse;
  * The length of the prefix for the index lookup in bytes.
  */
 export const INDEX_LOOKUP_PREFIX_LENGTH = 4;
-const GET_TRUSTED_INPUT_INS = 0x42;
+const GET_TRUSTED_INPUT = 0x42;
 
 export class GetTrustedInputCommand
   implements
@@ -77,7 +77,7 @@ export class GetTrustedInputCommand
 
     const getTrustedInputCommandArgs: ApduBuilderArgs = {
       cla: ZCASH_CLA,
-      ins: GET_TRUSTED_INPUT_INS,
+      ins: GET_TRUSTED_INPUT,
       p1: firstRound ? P1.FIRST : P1.NEXT,
       p2: P2.DEFAULT,
     };
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/HashOutputFullCommand.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/HashOutputFullCommand.ts
@@ -11,7 +11,6 @@ import { CommandErrorHelper } from "@ledgerhq/signer-utils";
 import { Maybe } from "purify-ts";
 
 import {
-  INS,
   P1,
   P2,
   ZCASH_CLA,
@@ -23,6 +22,8 @@ import {
   type ZcashErrorCodes,
 } from "./utils/zcashApplicationErrors";
 
+const FINALIZE_INPUT = 0x4a;
+
 export type HashOutputFullCommandArgs = {
   outputChunk: Uint8Array;
   isLastChunk: boolean;
@@ -50,7 +51,7 @@ export class HashOutputFullCommand
   getApdu(): Apdu {
     const apduArgs: ApduBuilderArgs = {
       cla: ZCASH_CLA,
-      ins: INS.FINALIZE_INPUT,
+      ins: FINALIZE_INPUT,
       p1: this.args.isLastChunk ? P1.NEXT : P1.FIRST,
       p2: P2.DEFAULT,
     };
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/ProvideOutputFullChangePathCommand.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/ProvideOutputFullChangePathCommand.ts
@@ -14,7 +14,6 @@ import {
 import { Maybe } from "purify-ts";
 
 import {
-  INS,
   P1,
   P2,
   ZCASH_CLA,
@@ -26,6 +25,8 @@ import {
   type ZcashErrorCodes,
 } from "./utils/zcashApplicationErrors";
 
+const FINALIZE_INPUT = 0x4a;
+
 export type ProvideOutputFullChangePathCommandArgs = {
   derivationPath: string;
 };
@@ -52,7 +53,7 @@ export class ProvideOutputFullChangePathCommand
   getApdu(): Apdu {
     const apduArgs: ApduBuilderArgs = {
       cla: ZCASH_CLA,
-      ins: INS.FINALIZE_INPUT,
+      ins: FINALIZE_INPUT,
       p1: P1.CHANGE_PATH,
       p2: P2.DEFAULT,
     };
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/SignTransactionCommand.test.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/SignTransactionCommand.test.ts
@@ -12,8 +12,7 @@ describe("SignTransactionCommand", () => {
       derivationPath: "44'/133'/0'/0/0",
       lockTime: 1,
       sigHashType: 0x01,
-      expiryHeight: new Uint8Array([0x01, 0x02, 0x03, 0x04]),
-      additionals: ["zcash"],
+      expiryHeight: Buffer.from([0x01, 0x02, 0x03, 0x04]),
     });
 
     const apdu = command.getApdu().getRawApdu();
@@ -27,6 +26,7 @@ describe("SignTransactionCommand", () => {
       derivationPath: "44'/133'/0'/0/0",
       lockTime: 0,
       sigHashType: 0x01,
+      expiryHeight: Buffer.alloc(4, 0),
     });
     const response = command.parseResponse(
       new ApduResponse({
@@ -44,11 +44,12 @@ describe("SignTransactionCommand", () => {
     );
   });
 
-  it("appends 4-byte zero expiry when expiryHeight omitted (no additionals required)", () => {
+  it("appends 4-byte expiry height from task-normalized buffer", () => {
     const command = new SignTransactionCommand({
       derivationPath: "44'/133'/0'/1/2",
       lockTime: 0,
       sigHashType: 0x01,
+      expiryHeight: Buffer.alloc(4, 0),
     });
 
     expect(Buffer.from(command.getApdu().getRawApdu()).toString("hex")).toBe(
@@ -61,6 +62,7 @@ describe("SignTransactionCommand", () => {
       derivationPath: "44'/133'/0'/0/0",
       lockTime: 0,
       sigHashType: 0x01,
+      expiryHeight: Buffer.alloc(4, 0),
     });
     const result = command.parseResponse(
       new ApduResponse({
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/SignTransactionCommand.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/SignTransactionCommand.ts
@@ -28,8 +28,7 @@ export type SignTransactionCommandArgs = {
   derivationPath: string;
   lockTime: number;
   sigHashType: number;
-  expiryHeight?: Uint8Array;
-  additionals?: string[];
+  expiryHeight: Buffer;
 };
 
 export type SignTransactionCommandResponse = {
@@ -59,11 +58,6 @@ export class SignTransactionCommand
     const lockTimeBuffer = Buffer.alloc(4);
     lockTimeBuffer.writeUInt32BE(lockTime, 0);
 
-    const expiryBytes =
-      expiryHeight !== undefined && expiryHeight.byteLength > 0
-        ? Buffer.from(expiryHeight)
-        : null;
-
     const apduArgs: ApduBuilderArgs = {
       cla: ZCASH_CLA,
       ins: 0x48,
@@ -81,13 +75,7 @@ export class SignTransactionCommand
       .addBufferToData(lockTimeBuffer)
       .add8BitUIntToData(sigHashType);
 
-    if (expiryBytes) {
-      builder.addBufferToData(expiryBytes);
-    } else {
-      // Zcash app SIGN (`0x48`) always expects 4-byte expiry height after sighash type;
-      // use zero when omitted (6700 WrongLength if callers omit `additionals` / expiry).
-      builder.addBufferToData(Buffer.alloc(4, 0));
-    }
+    builder.addBufferToData(expiryHeight);
     return builder.build();
   }
 
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/StartUntrustedHashTransactionInputCommand.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/StartUntrustedHashTransactionInputCommand.ts
@@ -11,7 +11,6 @@ import { CommandErrorHelper } from "@ledgerhq/signer-utils";
 import { Maybe } from "purify-ts";
 
 import {
-  INS,
   P1,
   P2,
   ZCASH_CLA,
@@ -23,6 +22,8 @@ import {
   type ZcashErrorCodes,
 } from "./utils/zcashApplicationErrors";
 
+const START_UNTRUSTED_HASH_TRANSACTION_INPUT = 0x44;
+
 export type StartUntrustedHashTransactionInputCommandArgs = {
   newTransaction: boolean;
   firstRound: boolean;
@@ -53,7 +54,7 @@ export class StartUntrustedHashTransactionInputCommand
   getApdu(): Apdu {
     const apduArgs: ApduBuilderArgs = {
       cla: ZCASH_CLA,
-      ins: INS.START_UNTRUSTED_HASH_TRANSACTION_INPUT,
+      ins: START_UNTRUSTED_HASH_TRANSACTION_INPUT,
       p1: this.args.firstRound ? P1.FIRST : P1.NEXT,
       p2: this.args.newTransaction ? P2.SAPLING : P2.HASH_INPUT_CONTINUE,
     };
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/ZcashSaplingOutputCommitCommand.test.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/ZcashSaplingOutputCommitCommand.test.ts
@@ -5,6 +5,7 @@ describe("ZcashSaplingOutputCommitCommand", () => {
     const command = new ZcashSaplingOutputCommitCommand({
       lockTime: 0,
       sigHashType: 1,
+      expiryHeight: Buffer.alloc(4, 0),
     });
 
     expect(Buffer.from(command.getApdu().getRawApdu()).toString("hex")).toBe(
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/ZcashSaplingOutputCommitCommand.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/ZcashSaplingOutputCommitCommand.ts
@@ -11,7 +11,6 @@ import { CommandErrorHelper } from "@ledgerhq/signer-utils";
 import { Maybe } from "purify-ts";
 
 import {
-  INS,
   P2,
   ZCASH_CLA,
 } from "@internal/app-binder/command/utils/apduHeaderUtils";
@@ -22,10 +21,12 @@ import {
   type ZcashErrorCodes,
 } from "./utils/zcashApplicationErrors";
 
+const SIGN_TRANSACTION = 0x48;
+
 export type ZcashSaplingOutputCommitCommandArgs = {
   lockTime: number;
   sigHashType: number;
-  expiryHeight?: Uint8Array;
+  expiryHeight: Buffer;
 };
 
 export type ZcashSaplingOutputCommitCommandResponse = {
@@ -57,23 +58,19 @@ export class ZcashSaplingOutputCommitCommand
     const { lockTime, sigHashType, expiryHeight } = this.args;
     const lockTimeLe = Buffer.alloc(4);
     lockTimeLe.writeUInt32LE(lockTime, 0);
-    const expiryLe =
-      expiryHeight !== undefined && expiryHeight.byteLength > 0
-        ? Buffer.from(expiryHeight)
-        : Buffer.alloc(4, 0);
     const payload = Buffer.concat([
       lockTimeLe,
       Buffer.from([0x00, 0x00]),
       Buffer.from([sigHashType & 0xff]),
-      expiryLe.subarray(0, 4),
+      expiryHeight,
     ]);
     if (payload.length !== 11) {
       throw new Error("ZcashSaplingOutputCommit: internal payload length");
     }
 
     const apduArgs: ApduBuilderArgs = {
       cla: ZCASH_CLA,
-      ins: INS.SIGN_TRANSACTION,
+      ins: SIGN_TRANSACTION,
       p1: 0x00,
       p2: P2.DEFAULT,
     };
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/command/utils/apduHeaderUtils.ts b/packages/signer/signer-zcash/src/internal/app-binder/command/utils/apduHeaderUtils.ts
@@ -1,13 +1,5 @@
 export const ZCASH_CLA = 0xe0 as const;
 
-export const INS = {
-  GET_WALLET_PUBLIC_KEY: 0x40,
-  GET_TRUSTED_INPUT: 0x42,
-  START_UNTRUSTED_HASH_TRANSACTION_INPUT: 0x44,
-  SIGN_TRANSACTION: 0x48,
-  FINALIZE_INPUT: 0x4a,
-} as const;
-
 export const P1 = {
   FIRST: 0x00,
   NEXT: 0x80,
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/task/GetTrustedInputTask.ts b/packages/signer/signer-zcash/src/internal/app-binder/task/GetTrustedInputTask.ts
@@ -223,9 +223,7 @@ const splitTransactionToTrustedInputChunks = (
 
     const scriptLength = readCompactSize(transaction, offset);
     offset = scriptLength.nextOffset;
-    //chunks.push(transaction.slice(valueStart, offset));
 
-    //const scriptStart = offset;
     ensureRange(transaction, offset, offset + scriptLength.value);
     offset += scriptLength.value;
     chunks.push(transaction.slice(valueStart, offset));
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/task/SignTransactionTask.test.ts b/packages/signer/signer-zcash/src/internal/app-binder/task/SignTransactionTask.test.ts
@@ -192,6 +192,27 @@ describe("SignTransactionTask", () => {
     } as unknown as InternalApi;
   });
 
+  it("returns validation error when expiryHeight length is not 4 bytes", async () => {
+    const result = await new SignTransactionTask(apiMock, {
+      transactionArg: {
+        inputs: [[PREVIOUS_TRANSACTION, 0, undefined, undefined]],
+        associatedKeysets: ["44'/133'/0'/0/0"],
+        outputScriptHex: "01",
+        additionals: ["zcash"],
+        expiryHeight: new Uint8Array([0x01, 0x02]),
+      },
+    }).run();
+
+    expect(isSuccessDmkResult(result)).toBe(false);
+    if (!isSuccessDmkResult(result)) {
+      expect(result.error).toBeInstanceOf(InvalidStatusWordError);
+      expect(
+        (result.error as InvalidStatusWordError).originalError?.message,
+      ).toMatch(/expiryHeight must be 4 bytes/);
+    }
+    expect(apiMock.sendCommand).not.toHaveBeenCalled();
+  });
+
   it("returns validation error when additionals does not include zcash", async () => {
     const result = await new SignTransactionTask(apiMock, {
       transactionArg: {
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/task/SignTransactionTask.ts b/packages/signer/signer-zcash/src/internal/app-binder/task/SignTransactionTask.ts
@@ -30,6 +30,7 @@ import {
   type InternalTransactionOutput,
   MAX_SCRIPT_BLOCK,
   parseOutputScriptsFromPaymentOutputBlob,
+  resolveExpiryHeightBytes,
   serializeTransaction,
   SIGHASH_ALL,
   toInternalTransaction,
@@ -90,10 +91,16 @@ export class SignTransactionTask {
     lockTimeBuffer.writeUInt32LE(lockTime, 0);
     const defaultVersion = getZcashDefaultTransactionVersion();
     // Ledger Wallet always passes expiry for v5 txs (often zero); required for BIP143 flow.
-    const expiryHeightBuffer =
-      expiryHeight !== undefined
-        ? Buffer.from(expiryHeight)
-        : Buffer.alloc(4, 0);
+    let expiryHeightBuffer: Buffer;
+    try {
+      expiryHeightBuffer = resolveExpiryHeightBytes(expiryHeight);
+    } catch (error) {
+      return DmkResultFactory({
+        error: new InvalidStatusWordError(
+          error instanceof Error ? error.message : "Invalid expiryHeight",
+        ),
+      });
+    }
 
     const trustedInputs: Array<{
       trustedInput: boolean;
@@ -300,7 +307,6 @@ export class SignTransactionTask {
           lockTime,
           sigHashType,
           expiryHeight: expiryHeightBuffer,
-          additionals,
         }),
       );
       if (!isSuccessCommandResult(signatureResult)) {
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/task/utils/legacyTransactionUtils.test.ts b/packages/signer/signer-zcash/src/internal/app-binder/task/utils/legacyTransactionUtils.test.ts
@@ -5,6 +5,7 @@ import {
   getZcashDefaultTransactionVersion,
   type InternalTransaction,
   parseOutputScriptsFromPaymentOutputBlob,
+  resolveExpiryHeightBytes,
   serializeTransactionOutputs,
 } from "./legacyTransactionUtils";
 
@@ -68,3 +69,27 @@ describe("parseOutputScriptsFromPaymentOutputBlob", () => {
     );
   });
 });
+
+describe("resolveExpiryHeightBytes", () => {
+  it("returns 4 zero bytes when expiryHeight is omitted", () => {
+    expect(resolveExpiryHeightBytes()).toEqual(Buffer.alloc(4, 0));
+  });
+
+  it("returns 4 zero bytes when expiryHeight is empty", () => {
+    expect(resolveExpiryHeightBytes(new Uint8Array())).toEqual(
+      Buffer.alloc(4, 0),
+    );
+  });
+
+  it("copies a 4-byte expiryHeight", () => {
+    expect(
+      resolveExpiryHeightBytes(new Uint8Array([0x01, 0x02, 0x03, 0x04])),
+    ).toEqual(Buffer.from([0x01, 0x02, 0x03, 0x04]));
+  });
+
+  it("throws when expiryHeight length is not 4 bytes", () => {
+    expect(() =>
+      resolveExpiryHeightBytes(new Uint8Array([0x01, 0x02])),
+    ).toThrow("expiryHeight must be 4 bytes (got 2)");
+  });
+});
diff --git a/packages/signer/signer-zcash/src/internal/app-binder/task/utils/legacyTransactionUtils.ts b/packages/signer/signer-zcash/src/internal/app-binder/task/utils/legacyTransactionUtils.ts
