feat: remove ecOptions from secp256k1 signer (#150)

ExodusMovement · Sep 30, 2024 · c1dd7e1 · c1dd7e1
1 parent 971221c
commit c1dd7e1
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 38 deletions.
diff --git a/features/keychain/api/__tests__/index.test.js b/features/keychain/api/__tests__/index.test.js
@@ -1,3 +1,4 @@
+import { hash } from '@exodus/crypto/hash'
 import KeyIdentifier from '@exodus/key-identifier'
 import { mnemonicToSeed } from 'bip39'
 import apiDefinition from '../index.js'
@@ -205,11 +206,11 @@ describe('keychain api', () => {
     })
 
     test('signBuffer signs binary data', async () => {
-      const data = Buffer.from("Batman's identity was revealed as Harvey Dent")
+      const data = await hash('sha256', "Batman's identity was revealed as Harvey Dent")
       const signature = await api.secp256k1.signBuffer({ seedId, keyId, data })
 
       expect(signature.toString('hex')).toBe(
-        '3044022012305c1fbb450372c3e83217f97f86a5289c8f3e295f61ec65d0acec3393ba8102201714227763f27039e62fc66144db9c1684865574f538dead27d3f0f1e727f328'
+        '3045022100f3aab0f6b44f62ef387050c86fb79bacabb36b254d3017d83dc801b8e72ad58602202339bf7576cb07eadd82c9ce97d5c117ceeaa3d0b990695ae9f6e7659f535fac'
       )
     })
   })

diff --git a/features/keychain/api/index.d.ts b/features/keychain/api/index.d.ts
@@ -1,19 +1,8 @@
 import KeyIdentifier from '@exodus/key-identifier'
-import BN from 'bn.js'
 
 type SeedId = string
 type KeySource = { seedId: SeedId; keyId: KeyIdentifier }
 
-type EcOptions = {
-  canonical?: boolean
-}
-
-type EcSignature = {
-  r: BN
-  s: BN
-  recoveryParam?: number
-}
-
 type PublicKeys = {
   publicKey: Buffer
   xpub: string
@@ -42,13 +31,8 @@ export interface KeychainApi {
     signBuffer(params: { data: Buffer } & KeySource): Promise<Buffer>
   }
   secp256k1: {
-    signBuffer(params: { data: Buffer; ecOptions?: EcOptions } & KeySource): Promise<Buffer>
-    signBuffer(
-      params: { data: Buffer; ecOptions?: EcOptions; enc: 'der' } & KeySource
-    ): Promise<Buffer>
-    signBuffer(
-      params: { data: Buffer; ecOptions?: EcOptions; enc: 'raw' } & KeySource
-    ): Promise<EcSignature>
+    signBuffer(params: { data: Buffer } & KeySource): Promise<Buffer>
+    signBuffer(params: { data: Buffer; enc: 'der' } & KeySource): Promise<Buffer>
   }
 }
 

diff --git a/features/keychain/module/__tests__/ecdsa.test.js b/features/keychain/module/__tests__/ecdsa.test.js
@@ -2,6 +2,7 @@ import { mnemonicToSeed } from 'bip39'
 
 import createKeychain from './create-keychain.js'
 import { getSeedId } from '../crypto/seed-id.js'
+import { hash, hashSync } from '@exodus/crypto/hash'
 import KeyIdentifier from '@exodus/key-identifier'
 
 const seed = mnemonicToSeed(
@@ -25,9 +26,9 @@ describe('EcDSA Signer', () => {
 
     const signer = keychain.createSecp256k1Signer(keyId)
     const plaintext = Buffer.from('I really love keychains')
-    const signature = await signer.signBuffer({ seedId, data: plaintext })
+    const signature = await signer.signBuffer({ seedId, data: await hash('sha256', plaintext) })
     const expected =
-      '30460221009288b22525674d76b0d5b8b20f333d4de4f4f88340a7d0a4cadd54b719e6162d022100f63e7591ce6b3bc0bf66fa2948d220e74ea2a74b63fc9dcb20e0f53191550b67'
+      '30440220722491f3d490960c4fc16b56b8dacafa9d446e17d9321dbbe3b216da845adc9802203afd466c1450c60f7ef0fcdf55b1e3bb206d9f989530996059890a9d92ab1ef9'
     expect(signature.toString('hex')).toBe(expected)
   })
 
@@ -37,11 +38,11 @@ describe('EcDSA Signer', () => {
     const signature = await keychain.secp256k1.signBuffer({
       seedId,
       keyId,
-      data: plaintext,
+      data: await hash('sha256', plaintext),
     })
 
     const expected =
-      '30460221009288b22525674d76b0d5b8b20f333d4de4f4f88340a7d0a4cadd54b719e6162d022100f63e7591ce6b3bc0bf66fa2948d220e74ea2a74b63fc9dcb20e0f53191550b67'
+      '30440220722491f3d490960c4fc16b56b8dacafa9d446e17d9321dbbe3b216da845adc9802203afd466c1450c60f7ef0fcdf55b1e3bb206d9f989530996059890a9d92ab1ef9'
     expect(signature.toString('hex')).toBe(expected)
   })
 
@@ -58,7 +59,7 @@ describe('EcDSA Signer', () => {
       keychain.secp256k1.signBuffer({
         seedId,
         keyId,
-        data: plaintext,
+        data: await hash('sha256', plaintext),
       })
     ).rejects.toThrow('ECDSA signatures are not supported for nacl')
   })
@@ -85,23 +86,23 @@ describe.each([
     const signature = await keychain.secp256k1.signBuffer({
       seedId,
       keyId,
-      data: plaintext,
+      data: await hash('sha256', plaintext),
     })
 
     const expected =
-      '30460221009288b22525674d76b0d5b8b20f333d4de4f4f88340a7d0a4cadd54b719e6162d022100f63e7591ce6b3bc0bf66fa2948d220e74ea2a74b63fc9dcb20e0f53191550b67'
+      '30440220722491f3d490960c4fc16b56b8dacafa9d446e17d9321dbbe3b216da845adc9802203afd466c1450c60f7ef0fcdf55b1e3bb206d9f989530996059890a9d92ab1ef9'
     expect(signature.toString('hex')).toBe(expected)
   })
 })
 
 describe('EcDSA Signer Signature Encoding', () => {
   const keychain = createKeychain({ seed })
-  const data = Buffer.from('I really love keychains')
+  const data = hashSync('sha256', 'I really love keychains')
   const expected = {
     default:
-      '30460221009288b22525674d76b0d5b8b20f333d4de4f4f88340a7d0a4cadd54b719e6162d022100f63e7591ce6b3bc0bf66fa2948d220e74ea2a74b63fc9dcb20e0f53191550b67',
+      '30440220722491f3d490960c4fc16b56b8dacafa9d446e17d9321dbbe3b216da845adc9802203afd466c1450c60f7ef0fcdf55b1e3bb206d9f989530996059890a9d92ab1ef9',
     binary:
-      '9288b22525674d76b0d5b8b20f333d4de4f4f88340a7d0a4cadd54b719e6162df63e7591ce6b3bc0bf66fa2948d220e74ea2a74b63fc9dcb20e0f53191550b6700',
+      '722491f3d490960c4fc16b56b8dacafa9d446e17d9321dbbe3b216da845adc983afd466c1450c60f7ef0fcdf55b1e3bb206d9f989530996059890a9d92ab1ef900',
   }
 
   it('Default encoding', async () => {

diff --git a/features/keychain/module/__tests__/ethsign.test.js b/features/keychain/module/__tests__/ethsign.test.js
@@ -54,13 +54,12 @@ describe('ETH Signer', () => {
 
       const ethSigner = async (buffer) => {
         const sig = await secp256k1Signer.signBuffer({
-          data: buffer,
+          data: Buffer.from(buffer, 'hex'),
           keyId: new KeyIdentifier({
             keyType: 'secp256k1',
             derivationPath: 'm/0', // doesn't matter in this fixture as we don't use it
             derivationAlgorithm: 'BIP32',
           }),
-          ecOptions: { canonical: true },
           enc: 'raw',
         })
         const signature = new Uint8Array(64)

diff --git a/features/keychain/module/crypto/secp256k1.js b/features/keychain/module/crypto/secp256k1.js
@@ -5,9 +5,6 @@ import { mapValues } from '@exodus/basic-utils'
 
 import { tweakPrivateKey } from './tweak.js'
 
-const isValidEcOptions = (ecOptions) =>
-  !ecOptions || Object.keys(ecOptions).every((key) => ['canonical', 'pers'].includes(key))
-
 const encodeSignature = ({ signature, enc }) => {
   if (enc === 'der') return Buffer.from(signature.toDER())
 
@@ -23,15 +20,14 @@ export const create = ({ getPrivateHDKey }) => {
   const curve = new EC('secp256k1')
 
   const createInstance = () => ({
-    signBuffer: async ({ seedId, keyId, data, ecOptions, enc = 'der' }) => {
+    signBuffer: async ({ seedId, keyId, data, enc = 'der' }) => {
       assert(
         keyId.keyType === 'secp256k1',
         `ECDSA signatures are not supported for ${keyId.keyType}`
       )
       assert(['der', 'raw', 'binary'].includes(enc), 'signBuffer: invalid enc')
-      assert(isValidEcOptions(ecOptions), 'signBuffer: invalid EC option')
       const { privateKey } = getPrivateHDKey({ seedId, keyId })
-      const signature = curve.sign(data, privateKey, ecOptions)
+      const signature = curve.sign(data, privateKey, { canonical: true })
       return encodeSignature({ signature, enc })
     },
     signSchnorr: async ({ seedId, keyId, data, tweak, extraEntropy }) => {