Merge pull request #1652 from input-output-hk/feat/fix-trezor-signing

Feat/fix trezor signing

Author: rhyslbw

packages/hardware-ledger/src/LedgerKeyAgent.ts

@@ -53,6 +53,7 @@ import _LedgerConnection, {
   Transaction,
   TransactionSigningMode,
   TxOutputDestinationType,
+  TxRequiredSignerType,
   VoterType,
   VoterVotes
 } from '@cardano-foundation/ledgerjs-hw-app-cardano';
@@ -207,10 +208,12 @@ const isMultiSig = (tx: Transaction): boolean => {
   const result = false;
 
   const allThirdPartyInputs = !tx.inputs.some((input) => input.path !== null);
+  const allThirdPartyRequiredSigner = !tx.requiredSigners?.some((signer) => signer.type === TxRequiredSignerType.PATH);
   // Ledger doesn't allow change outputs to address controlled by your keys and instead you have to use script address for change out
   const allThirdPartyOutputs = !tx.outputs.some((out) => out.destination.type !== TxOutputDestinationType.THIRD_PARTY);
 
   if (
+    allThirdPartyRequiredSigner &&
     allThirdPartyInputs &&
     allThirdPartyOutputs &&
     !tx.collateralInputs &&

packages/hardware-ledger/test/LedgerKeyAgent.test.ts

@@ -359,6 +359,56 @@ describe('LedgerKeyAgent', () => {
 
       expect(LedgerKeyAgent.getSigningMode(tx)).toEqual(Ledger.TransactionSigningMode.MULTISIG_TRANSACTION);
     });
+
+    it('can detect ordinary transaction signing mode when we own a required signer', async () => {
+      const tx: Ledger.Transaction = {
+        certificates: [
+          {
+            params: {
+              stakeCredential: {
+                scriptHashHex: 'cb0ec2692497b458e46812c8a5bfa2931d1a2d965a99893828ec810f',
+                type: Ledger.CredentialParamsType.SCRIPT_HASH
+              }
+            },
+            type: Ledger.CertificateType.STAKE_DEREGISTRATION
+          }
+        ],
+        fee: 10n,
+        includeNetworkId: false,
+        inputs: [
+          {
+            outputIndex: 0,
+            path: null,
+            txHashHex: '0f3abbc8fc19c2e61bab6059bf8a466e6e754833a08a62a6c56fe0e78f190000'
+          }
+        ],
+        network: {
+          networkId: Ledger.Networks.Testnet.networkId,
+          protocolMagic: 999
+        },
+        outputs: [
+          {
+            amount: 10n,
+            datumHashHex: '0f3abbc8fc19c2e61bab6059bf8a466e6e754833a08a62a6c56fe0e78f19d9d5',
+            destination: {
+              params: {
+                addressHex:
+                  '009493315cd92eb5d8c4304e67b7e16ae36d61d34502694657811a2c8e32c728d3861e164cab28cb8f006448139c8f1740ffb8e7aa9e5232dc'
+              },
+              type: Ledger.TxOutputDestinationType.THIRD_PARTY
+            },
+            format: Ledger.TxOutputFormat.ARRAY_LEGACY
+          }
+        ],
+        requiredSigners: [
+          { path: [util.harden(1852), util.harden(1815), util.harden(0), 2, 0], type: Ledger.TxRequiredSignerType.PATH }
+        ],
+        ttl: 1000,
+        validityIntervalStart: 100
+      };
+
+      expect(LedgerKeyAgent.getSigningMode(tx)).toEqual(Ledger.TransactionSigningMode.ORDINARY_TRANSACTION);
+    });
   });
 
   describe('Unsupported Transaction Errors', () => {

packages/hardware-trezor/src/TrezorKeyAgent.ts

@@ -92,12 +92,14 @@ const multiSigWitnessPaths: BIP32Path[] = [
 
 const isMultiSig = (tx: Omit<Trezor.CardanoSignTransaction, 'signingMode'>): boolean => {
   const allThirdPartyInputs = !tx.inputs.some((input) => input.path);
+  const allThirdRequiredSigner = !tx.requiredSigners?.some((signer) => signer.keyPath);
   // Trezor doesn't allow change outputs to address controlled by your keys and instead you have to use script address for change out
   const allThirdPartyOutputs = !tx.outputs.some((out) => 'addressParameters' in out);
 
   return (
     allThirdPartyInputs &&
     allThirdPartyOutputs &&
+    allThirdRequiredSigner &&
     !tx.collateralInputs &&
     !tx.collateralReturn &&
     !tx.totalCollateral &&
@@ -325,7 +327,9 @@ export class TrezorKeyAgent extends KeyAgentBase {
       const signedData = result.payload;
 
       if (!areStringsEqualInConstantTime(signedData.hash, hash)) {
-        throw new errors.HwMappingError('Trezor computed a different transaction id');
+        throw new errors.HwMappingError(
+          `Trezor computed a different transaction id: ${signedData.hash} than expected: ${hash}`
+        );
       }
 
       return new Map<Crypto.Ed25519PublicKeyHex, Crypto.Ed25519SignatureHex>(
@@ -340,7 +344,7 @@ export class TrezorKeyAgent extends KeyAgentBase {
         )
       );
     } catch (error: any) {
-      if (error.innerError.code === 'Failure_ActionCancelled') {
+      if (error.innerError?.code === 'Failure_ActionCancelled') {
         throw new errors.AuthenticationError('Transaction signing aborted', error);
       }
       throw transportTypedError(error);

packages/hardware-trezor/src/transformers/tx.ts

@@ -11,6 +11,8 @@ export const trezorTxTransformer: Transformer<
   Omit<Trezor.CardanoSignTransaction, 'signingMode' | 'derivationType' | 'includeNetworkId' | 'chunkify' | 'ttl'> & {
     /* eslint-disable @typescript-eslint/no-explicit-any */
     ttl: any; // TODO: the Transformer util cant handle ttl as TOptional<string | number>
+    /* eslint-disable @typescript-eslint/no-explicit-any */
+    includeNetworkId: any; // TODO: the Transformer util cant handle TOptional<string | boolean>
   },
   TrezorTxTransformerContext
 > = {
@@ -21,6 +23,7 @@ export const trezorTxTransformer: Transformer<
   collateralReturn: ({ collateralReturn }, context) =>
     collateralReturn ? toTxOut({ index: 0, isCollateral: true, txOut: collateralReturn }, context!) : undefined,
   fee: ({ fee }) => fee.toString(),
+  includeNetworkId: ({ networkId }) => !!networkId,
   inputs: ({ inputs }, context) => mapTxIns(inputs, context!),
   mint: ({ mint }) => mapTokenMap(mint, true),
   networkId: (_, context) => context!.chainId.networkId,

packages/hardware-trezor/test/TrezorKeyAgent.test.ts

@@ -125,6 +125,14 @@ describe('TrezorKeyAgent', () => {
       expect(signingMode).toEqual(Trezor.PROTO.CardanoTxSigningMode.ORDINARY_TRANSACTION);
     });
 
+    it('can detect ordinary transaction signing mode when we own a required signer', async () => {
+      const signingMode = TrezorKeyAgent.matchSigningMode({
+        ...validMultisigTx,
+        requiredSigners: [{ keyPath: knownAddressKeyPath }]
+      });
+      expect(signingMode).toEqual(Trezor.PROTO.CardanoTxSigningMode.ORDINARY_TRANSACTION);
+    });
+
     it('can detect pool registrations signing mode', async () => {
       const signingMode = TrezorKeyAgent.matchSigningMode({
         ...simpleTx,

packages/hardware-trezor/test/transformers/tx.test.ts

@@ -1,4 +1,5 @@
 import * as Trezor from '@trezor/connect';
+import { Cardano } from '@cardano-sdk/core';
 import { CardanoKeyConst, TxInId, util } from '@cardano-sdk/key-management';
 import {
   babbageTxBodyWithScripts,
@@ -22,6 +23,34 @@ describe('tx', () => {
       expect(await txToTrezor(minValidTxBody, contextWithoutKnownAddresses)).toEqual({
         additionalWitnessRequests: [],
         fee: '10',
+        includeNetworkId: false,
+        inputs: [
+          {
+            prev_hash: txIn.txId,
+            prev_index: txIn.index
+          }
+        ],
+        networkId: 0,
+        outputs: [
+          {
+            address:
+              'addr_test1qz2fxv2umyhttkxyxp8x0dlpdt3k6cwng5pxj3jhsydzer3jcu5d8ps7zex2k2xt3uqxgjqnnj83ws8lhrn648jjxtwq2ytjqp',
+            amount: '10',
+            format: Trezor.PROTO.CardanoTxOutputSerializationFormat.ARRAY_LEGACY
+          }
+        ],
+        protocolMagic: 999,
+        tagCborSets: false
+      });
+    });
+
+    test('can set includeNetworkId if network field is set', async () => {
+      expect(
+        await txToTrezor({ ...minValidTxBody, networkId: Cardano.NetworkId.Mainnet }, contextWithoutKnownAddresses)
+      ).toEqual({
+        additionalWitnessRequests: [],
+        fee: '10',
+        includeNetworkId: true,
         inputs: [
           {
             prev_hash: txIn.txId,
@@ -69,6 +98,7 @@ describe('tx', () => {
           }
         ],
         fee: '10',
+        includeNetworkId: false,
         inputs: [
           {
             path: knownAddressKeyPath,
@@ -232,6 +262,7 @@ describe('tx', () => {
           }
         ],
         fee: '10',
+        includeNetworkId: false,
         inputs: [
           {
             path: knownAddressKeyPath,
@@ -315,6 +346,7 @@ describe('tx', () => {
           format: Trezor.PROTO.CardanoTxOutputSerializationFormat.ARRAY_LEGACY
         },
         fee: '10',
+        includeNetworkId: false,
         inputs: [
           {
             prev_hash: txIn.txId,
@@ -385,6 +417,7 @@ describe('tx', () => {
           format: Trezor.PROTO.CardanoTxOutputSerializationFormat.MAP_BABBAGE
         },
         fee: '10',
+        includeNetworkId: false,
         inputs: [
           {
             path: knownAddressKeyPath,