fix: increase refinement test coverage (#2431)

Adds extensive test coverage to refinement of proposal/confirmation and API transactions/messages. It also includes small fixes that we found due to adding test coverage: - Increase test coverage of `TransactionVerifier` and `MessageVerifier` - Propagate similar test coverage to respective controller tests for transactions/messages
safe-global · Mar 7, 2025 · dd5ee84 · dd5ee84
1 parent 299b390
commit dd5ee84
Show file tree

Hide file tree

Showing 26 changed files with 5,869 additions and 4,105 deletions.
diff --git a/src/domain/common/entities/safe-signature.spec.ts b/src/domain/common/entities/safe-signature.spec.ts
@@ -3,10 +3,7 @@ import * as viem from 'viem';
 import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts';
 import { SafeSignature } from '@/domain/common/entities/safe-signature';
 import { SignatureType } from '@/domain/common/entities/signature-type.entity';
-import {
-  getContractSignature,
-  getApprovedHashSignature,
-} from '@/domain/common/utils/__tests__/signatures.builder';
+import { getSignature } from '@/domain/common/utils/__tests__/signatures.builder';
 
 describe('SafeSignature', () => {
   it('should create an instance', () => {
@@ -118,76 +115,61 @@ describe('SafeSignature', () => {
   });
 
   describe('owner', () => {
-    it('should recover the address of contract signatures', () => {
-      const owner = viem.getAddress(faker.finance.ethereumAddress());
-      const signature = getContractSignature(owner);
-      const hash = faker.string.hexadecimal({ length: 66 }) as `0x${string}`;
-
-      const safeSignature = new SafeSignature({ signature, hash });
-
-      expect(safeSignature.owner).toBe(owner);
-    });
-
-    it('should recover the address of an approved hash', () => {
-      const owner = viem.getAddress(faker.finance.ethereumAddress());
-      const signature = getApprovedHashSignature(owner);
-      const hash = faker.string.hexadecimal({ length: 66 }) as `0x${string}`;
-
-      const safeSignature = new SafeSignature({ signature, hash });
-
-      expect(safeSignature.owner).toBe(owner);
-    });
-
-    it('should recover the address of an eth_sign signature', async () => {
-      const privateKey = generatePrivateKey();
-      const signer = privateKeyToAccount(privateKey);
-      const hash = faker.string.hexadecimal({ length: 66 }) as `0x${string}`;
-      const signature = await signer.signMessage({ message: { raw: hash } });
-      const v = parseInt(signature.slice(-2), 16) + 4;
-
-      const safeSignature = new SafeSignature({
-        signature:
-          `${signature.slice(0, 130)}${v.toString(16)}` as `0x${string}`,
-        hash,
-      });
+    it.each(Object.values(SignatureType))(
+      'should recover the address of a %s signature',
+      async (signatureType) => {
+        const privateKey = generatePrivateKey();
+        const signer = privateKeyToAccount(privateKey);
+        const hash = faker.string.hexadecimal({ length: 66 }) as `0x${string}`;
+        const signature = await getSignature({
+          signer,
+          hash,
+          signatureType,
+        });
+
+        const safeSignature = new SafeSignature({ signature, hash });
+
+        expect(safeSignature.owner).toBe(signer.address);
+      },
+    );
 
-      expect(safeSignature.owner).toBe(signer.address);
-    });
+    it('should memoize the owner', async () => {
+      const encodeApiParametersSpy = jest.spyOn(viem, 'encodeAbiParameters');
 
-    it('should recover the address of an EOA signature', async () => {
       const privateKey = generatePrivateKey();
       const signer = privateKeyToAccount(privateKey);
       const hash = faker.string.hexadecimal({ length: 66 }) as `0x${string}`;
-      const signature = await signer.sign({ hash });
-
-      const safeSignature = new SafeSignature({
-        signature,
+      // Recovered via encodeAbiParameters
+      const signatureType = faker.helpers.arrayElement([
+        SignatureType.ApprovedHash,
+        SignatureType.ContractSignature,
+      ]);
+      const signature = await getSignature({
+        signer,
         hash,
+        signatureType,
       });
 
-      expect(safeSignature.owner).toBe(signer.address);
-    });
-
-    it('should memoize the owner', () => {
-      const encodeApiParametersSpy = jest.spyOn(viem, 'encodeAbiParameters');
-
-      const owner = viem.getAddress(faker.finance.ethereumAddress());
-      const signature = getContractSignature(owner);
-      const hash = faker.string.hexadecimal({ length: 66 }) as `0x${string}`;
-
       const safeSignature = new SafeSignature({ signature, hash });
 
-      expect(safeSignature.owner).toBe(owner);
+      expect(safeSignature.owner).toBe(signer.address);
       expect(encodeApiParametersSpy).toHaveBeenCalledTimes(1);
-      expect(safeSignature.owner).toBe(owner);
+
+      expect(safeSignature.owner).toBe(signer.address);
       expect(encodeApiParametersSpy).toHaveBeenCalledTimes(1);
 
-      const newOwner = viem.getAddress(faker.finance.ethereumAddress());
-      safeSignature.signature = getContractSignature(newOwner);
+      const newPrivateKey = generatePrivateKey();
+      const newSigner = privateKeyToAccount(newPrivateKey);
+      safeSignature.signature = await getSignature({
+        signer: newSigner,
+        hash,
+        signatureType,
+      });
 
-      expect(safeSignature.owner).toBe(newOwner);
+      expect(safeSignature.owner).toBe(newSigner.address);
       expect(encodeApiParametersSpy).toHaveBeenCalledTimes(2);
-      expect(safeSignature.owner).toBe(newOwner);
+
+      expect(safeSignature.owner).toBe(newSigner.address);
       expect(encodeApiParametersSpy).toHaveBeenCalledTimes(2);
     });
   });

diff --git a/src/domain/common/utils/__tests__/signatures.builder.ts b/src/domain/common/utils/__tests__/signatures.builder.ts
@@ -1,20 +1,64 @@
-export function getApprovedHashSignature(owner: `0x${string}`): `0x${string}` {
+import { SignatureType } from '@/domain/common/entities/signature-type.entity';
+import type { PrivateKeyAccount } from 'viem';
+
+export async function getSignature(args: {
+  signer: PrivateKeyAccount;
+  hash: `0x${string}`;
+  signatureType: SignatureType;
+}): Promise<`0x${string}`> {
+  switch (args.signatureType) {
+    case SignatureType.ContractSignature: {
+      return getContractSignature(args.signer.address);
+    }
+    case SignatureType.ApprovedHash: {
+      return getApprovedHashSignature(args.signer.address);
+    }
+    case SignatureType.Eoa: {
+      return await getEoaSignature({ signer: args.signer, hash: args.hash });
+    }
+    case SignatureType.EthSign: {
+      return await getEthSignSignature({
+        signer: args.signer,
+        hash: args.hash,
+      });
+    }
+    default: {
+      throw new Error(`Unknown signature type: ${args.signatureType}`);
+    }
+  }
+}
+
+function getApprovedHashSignature(owner: `0x${string}`): `0x${string}` {
   return ('0x000000000000000000000000' +
     owner.slice(2) +
     '0000000000000000000000000000000000000000000000000000000000000000' +
     '01') as `0x${string}`;
 }
 
-export function getContractSignature(owner: `0x${string}`): `0x${string}` {
+function getContractSignature(owner: `0x${string}`): `0x${string}` {
   return ('0x000000000000000000000000' +
     owner.slice(2) +
     '0000000000000000000000000000000000000000000000000000000000000000' +
     '00') as `0x${string}`;
 }
 
-export function adjustEthSignSignature(
-  signature: `0x${string}`,
-): `0x${string}` {
+async function getEoaSignature(args: {
+  signer: PrivateKeyAccount;
+  hash: `0x${string}`;
+}): Promise<`0x${string}`> {
+  return await args.signer.sign({ hash: args.hash });
+}
+
+async function getEthSignSignature(args: {
+  signer: PrivateKeyAccount;
+  hash: `0x${string}`;
+}): Promise<`0x${string}`> {
+  const signature = await args.signer.signMessage({
+    message: { raw: args.hash },
+  });
+
+  // To differentiate signature types, eth_sign signatures have v value increased by 4
+  // @see https://docs.safe.global/advanced/smart-account-signatures#eth_sign-signature
   const v = parseInt(signature.slice(-2), 16);
   return (signature.slice(0, 130) + (v + 4).toString(16)) as `0x${string}`;
 }
diff --git a/src/domain/messages/entities/__tests__/message.builder.ts b/src/domain/messages/entities/__tests__/message.builder.ts
@@ -9,14 +9,11 @@ import { getAddress, type PrivateKeyAccount } from 'viem';
 import { fakeJson } from '@/__tests__/faker';
 import { SignatureType } from '@/domain/common/entities/signature-type.entity';
 import { getSafeMessageMessageHash } from '@/domain/common/utils/safe';
-import {
-  getContractSignature,
-  getApprovedHashSignature,
-  adjustEthSignSignature,
-} from '@/domain/common/utils/__tests__/signatures.builder';
+import { getSignature } from '@/domain/common/utils/__tests__/signatures.builder';
 import type { Safe } from '@/domain/safe/entities/safe.entity';
 import type { MessageConfirmation } from '@/domain/messages/entities/message-confirmation.entity';
 
+// TODO: Refactor with multisig BuilderWithConfirmations
 class BuilderWithConfirmations<T extends Message> extends Builder<T> {
   public async buildWithConfirmations(args: {
     chainId: string;
@@ -45,25 +42,13 @@ class BuilderWithConfirmations<T extends Message> extends Builder<T> {
 
     message.confirmations = await Promise.all(
       args.signers.map(async (signer): Promise<MessageConfirmation> => {
-        const signatureType: SignatureType =
+        const signatureType =
           args.signatureType ?? faker.helpers.enumValue(SignatureType);
-
-        let signature: `0x${string}`;
-
-        // TODO: Refactor with multisig BuilderWithConfirmations
-        if (signatureType === SignatureType.ContractSignature) {
-          signature = getContractSignature(signer.address);
-        } else if (signatureType === SignatureType.ApprovedHash) {
-          signature = getApprovedHashSignature(signer.address);
-        } else if (signatureType === SignatureType.Eoa) {
-          signature = await signer.sign({ hash: message.messageHash });
-        } else if (SignatureType.EthSign) {
-          signature = await signer
-            .signMessage({ message: { raw: message.messageHash } })
-            .then(adjustEthSignSignature);
-        } else {
-          throw new Error(`Unknown signature type: ${signatureType}`);
-        }
+        const signature = await getSignature({
+          signer,
+          hash: message.messageHash,
+          signatureType,
+        });
 
         return {
           owner: signer.address,