fix: default to eth_signTypedData_v4 (#9)

* feat: add walletconnect getPeerMeta

* fix: default to eth_signTypedData_v4 with special casing

* refactor: pull out wallets const

package.json

@@ -48,6 +48,7 @@
     "@typescript-eslint/eslint-plugin": "^5.42.0",
     "@typescript-eslint/parser": "^5.42.0",
     "@uniswap/sdk-core": "^3.1.0",
+    "@walletconnect/ethereum-provider": "^1.8.0",
     "babel-jest": "^29.3.0",
     "eslint": "^8.27.0",
     "eslint-config-prettier": "^8.5.0",
@@ -64,6 +65,7 @@
   },
   "peerDependencies": {
     "@uniswap/sdk-core": ">=3",
+    "@walletconnect/ethereum-provider": "^1.8.0",
     "ethers": "^5.6.1"
   }
-}
+}

src/provider.test.ts

@@ -1,6 +1,6 @@
-import { JsonRpcProvider } from '@ethersproject/providers'
+import { JsonRpcProvider, JsonRpcSigner } from '@ethersproject/providers'
 
-import { INVALID_PARAMS_CODE, signTypedData } from './provider'
+import { signTypedData } from './provider'
 
 describe('provider', () => {
   describe('signTypedData', () => {
@@ -36,77 +36,92 @@ describe('provider', () => {
       contents: 'Hello, Bob!',
     }
 
-    let signer
+    let signer: JsonRpcSigner
     beforeEach(() => {
       signer = new JsonRpcProvider().getSigner()
-      jest.spyOn(signer, 'getAddress').mockReturnValue(wallet)
+      jest.spyOn(signer, 'getAddress').mockResolvedValue(wallet)
     })
 
-    it('signs using eth_signTypedData', async () => {
+    function itFallsBackToEthSignIfUnimplemented(signingMethod: string) {
+      it.each(['not found', 'not implemented'])(`falls back to eth_sign if ${signingMethod} is %s`, async (message) => {
+        const send = jest
+          .spyOn(signer.provider, 'send')
+          .mockImplementationOnce((method) => {
+            if (method === signingMethod) return Promise.reject({ message: `method ${message}` })
+            throw new Error('Unimplemented')
+          })
+          .mockImplementationOnce((method, params) => {
+            if (method === 'eth_sign') return Promise.resolve()
+            throw new Error('Unimplemented')
+          })
+        jest.spyOn(console, 'warn').mockImplementation(() => undefined)
+
+        await signTypedData(signer, domain, types, value)
+        expect(console.warn).toHaveBeenCalledWith(
+          expect.stringContaining('signTypedData: wallet does not implement EIP-712, falling back to eth_sign'),
+          expect.anything()
+        )
+        expect(send).toHaveBeenCalledTimes(2)
+        expect(send).toHaveBeenCalledWith(signingMethod, [wallet, expect.anything()])
+        expect(send).toHaveBeenCalledWith('eth_sign', [wallet, expect.anything()])
+        const hash = send.mock.lastCall[1]?.[1]
+        expect(hash).toBe('0xbe609aee343fb3c4b28e1df9e632fca64fcfaede20f02e86244efddf30957bd2')
+      })
+    }
+
+    function itFailsIfRejected(signingMethod: string) {
+      it('fails if rejected', async () => {
+        const send = jest.spyOn(signer.provider, 'send').mockImplementationOnce((method) => {
+          if (method === signingMethod) return Promise.reject(new Error('User rejected'))
+          throw new Error('Unimplemented')
+        })
+
+        await expect(async () => await signTypedData(signer, domain, types, value)).rejects.toThrow('User rejected')
+        expect(send).toHaveBeenCalledTimes(1)
+        expect(send).toHaveBeenCalledWith(signingMethod, [wallet, expect.anything()])
+        const data = send.mock.lastCall[1]?.[1]
+        expect(JSON.parse(data)).toEqual(expect.objectContaining({ domain, message: value }))
+      })
+    }
+
+    it('signs using eth_signTypedData_v4', async () => {
       const send = jest.spyOn(signer.provider, 'send').mockImplementationOnce((method, params) => {
-        if (method === 'eth_signTypedData') return Promise.resolve()
+        if (method === 'eth_signTypedData_v4') return Promise.resolve()
+        throw new Error('Unimplemented')
       })
 
       await signTypedData(signer, domain, types, value)
       expect(send).toHaveBeenCalledTimes(1)
-      expect(send).toHaveBeenCalledWith('eth_signTypedData', [wallet, expect.anything()])
+      expect(send).toHaveBeenCalledWith('eth_signTypedData_v4', [wallet, expect.anything()])
       const data = send.mock.lastCall[1]?.[1]
       expect(JSON.parse(data)).toEqual(expect.objectContaining({ domain, message: value }))
     })
 
-    it('falls back to eth_signTypedData_v4 if the request has invalid params', async () => {
-      const send = jest
-        .spyOn(signer.provider, 'send')
-        .mockImplementationOnce((method) => {
-          if (method === 'eth_signTypedData') return Promise.reject({ code: INVALID_PARAMS_CODE })
-        })
-        .mockImplementationOnce((method, params) => {
-          if (method === 'eth_signTypedData_v4') return Promise.resolve()
+    itFallsBackToEthSignIfUnimplemented('eth_signTypedData_v4')
+    itFailsIfRejected('eth_signTypedData_v4')
+
+    describe('wallets which do not support eth_signTypedData_v4', () => {
+      describe.each(['SafePal Wallet'])('%s', (name) => {
+        beforeEach(() => {
+          signer.provider['provider'] = { isWalletConnect: true, connector: { peerMeta: { name } } }
         })
-      jest.spyOn(console, 'warn').mockImplementation(() => undefined)
 
-      await signTypedData(signer, domain, types, value)
-      expect(console.warn).toHaveBeenCalledWith(expect.stringContaining('eth_signTypedData failed'), expect.anything())
-      expect(send).toHaveBeenCalledTimes(2)
-      expect(send).toHaveBeenCalledWith('eth_signTypedData', [wallet, expect.anything()])
-      expect(send).toHaveBeenCalledWith('eth_signTypedData_v4', [wallet, expect.anything()])
-      const data = send.mock.lastCall[1]?.[1]
-      expect(JSON.parse(data)).toEqual(expect.objectContaining({ domain, message: value }))
-    })
+        it('signs using eth_signTypedData', async () => {
+          const send = jest.spyOn(signer.provider, 'send').mockImplementationOnce((method, params) => {
+            if (method === 'eth_signTypedData') return Promise.resolve()
+            throw new Error('Unimplemented')
+          })
 
-    it.each(['not found', 'not implemented'])('falls back to eth_sign if eth_signTypedData is %s', async (message) => {
-      const send = jest
-        .spyOn(signer.provider, 'send')
-        .mockImplementationOnce((method) => {
-          if (method === 'eth_signTypedData') return Promise.reject({ message: `method ${message}` })
-        })
-        .mockImplementationOnce((method, params) => {
-          if (method === 'eth_sign') return Promise.resolve()
+          await signTypedData(signer, domain, types, value)
+          expect(send).toHaveBeenCalledTimes(1)
+          expect(send).toHaveBeenCalledWith('eth_signTypedData', [wallet, expect.anything()])
+          const data = send.mock.lastCall[1]?.[1]
+          expect(JSON.parse(data)).toEqual(expect.objectContaining({ domain, message: value }))
         })
-      jest.spyOn(console, 'warn').mockImplementation(() => undefined)
-
-      await signTypedData(signer, domain, types, value)
-      expect(console.warn).toHaveBeenCalledWith(
-        expect.stringContaining('eth_signTypedData_* failed'),
-        expect.anything()
-      )
-      expect(send).toHaveBeenCalledTimes(2)
-      expect(send).toHaveBeenCalledWith('eth_signTypedData', [wallet, expect.anything()])
-      expect(send).toHaveBeenCalledWith('eth_sign', [wallet, expect.anything()])
-      const hash = send.mock.lastCall[1]?.[1]
-      expect(hash).toBe('0xbe609aee343fb3c4b28e1df9e632fca64fcfaede20f02e86244efddf30957bd2')
-    })
 
-    it('fails if rejected', async () => {
-      const send = jest.spyOn(signer.provider, 'send').mockImplementationOnce((method) => {
-        if (method === 'eth_signTypedData') return Promise.reject(new Error('User rejected'))
+        itFallsBackToEthSignIfUnimplemented('eth_signTypedData')
+        itFailsIfRejected('eth_signTypedData')
       })
-
-      await expect(async () => await signTypedData(signer, domain, types, value)).rejects.toThrow('User rejected')
-      expect(send).toHaveBeenCalledTimes(1)
-      expect(send).toHaveBeenCalledWith('eth_signTypedData', [wallet, expect.anything()])
-      const data = send.mock.lastCall[1]?.[1]
-      expect(JSON.parse(data)).toEqual(expect.objectContaining({ domain, message: value }))
     })
   })
 })

src/provider.ts

@@ -1,16 +1,29 @@
 import type { TypedDataDomain, TypedDataField } from '@ethersproject/abstract-signer'
 import { _TypedDataEncoder } from '@ethersproject/hash'
-import type { JsonRpcSigner } from '@ethersproject/providers'
+import type { JsonRpcProvider, JsonRpcSigner } from '@ethersproject/providers'
 
-// See https://github.com/MetaMask/eth-rpc-errors/blob/b19c8724168eec4ce3f8b1f87642f231f0dd27b2/src/error-constants.ts#L12
-export const INVALID_PARAMS_CODE = -32602
+import { getPeerMeta } from './walletconnect'
+
+// These are wallets which do not implement eth_signTypedData_v4, but *do* implement eth_signTypedData.
+// They are special-cased so that signing will still use EIP-712 (which is safer for the user).
+const WALLETS_LACKING_V4_SUPPORT = ['SafePal Wallet']
+
+function supportsV4(provider: JsonRpcProvider): boolean {
+  const name = getPeerMeta(provider)?.name
+
+  // By default, we assume v4 support.
+  if (name && WALLETS_LACKING_V4_SUPPORT.includes(name)) {
+    return false
+  }
+
+  return true
+}
 
 /**
- * Calls into the eth_signTypedData methods to add support for wallets with spotty EIP-712 support (eg Safepal) or without any (eg Zerion),
- * by first trying eth_signTypedData, and then falling back to either eth_signTyepdData_v4 or eth_sign.
- * The implementation is copied from ethers (and linted).
+ * Signs TypedData with EIP-712, if available, or else by falling back to eth_sign.
+ * Calls eth_signTypedData_v4, or eth_signTypedData for wallets with incomplete EIP-712 support.
+ *
  * @see https://github.com/ethers-io/ethers.js/blob/c80fcddf50a9023486e9f9acb1848aba4c19f7b6/packages/providers/src.ts/json-rpc-provider.ts#L334
- * TODO(https://github.com/ethers-io/ethers.js/pull/3667): Remove if upstreamed.
  */
 export async function signTypedData(
   signer: JsonRpcSigner,
@@ -25,30 +38,16 @@ export async function signTypedData(
     return signer.provider.resolveName(name) as Promise<string>
   })
 
-  const address = await signer.getAddress()
+  const method = supportsV4(signer.provider) ? 'eth_signTypedData_v4' : 'eth_signTypedData'
+  const address = (await signer.getAddress()).toLowerCase()
+  const message = JSON.stringify(_TypedDataEncoder.getPayload(populated.domain, types, populated.value))
 
   try {
-    try {
-      // We must try the unversioned eth_signTypedData first, because some wallets (eg SafePal) will hang on _v4.
-      return await signer.provider.send('eth_signTypedData', [
-        address.toLowerCase(),
-        JSON.stringify(_TypedDataEncoder.getPayload(populated.domain, types, populated.value)),
-      ])
-    } catch (error) {
-      // MetaMask complains that the unversioned eth_signTypedData is formatted incorrectly (32602) - it prefers _v4.
-      if (error.code === INVALID_PARAMS_CODE) {
-        console.warn('eth_signTypedData failed, falling back to eth_signTypedData_v4:', error)
-        return await signer.provider.send('eth_signTypedData_v4', [
-          address.toLowerCase(),
-          JSON.stringify(_TypedDataEncoder.getPayload(populated.domain, types, populated.value)),
-        ])
-      }
-      throw error
-    }
+    return await signer.provider.send(method, [address, message])
   } catch (error) {
-    // If neither other method is available (eg Zerion), fallback to eth_sign.
+    // If eth_signTypedData is unimplemented, fall back to eth_sign.
     if (typeof error.message === 'string' && error.message.match(/not (found|implemented)/i)) {
-      console.warn('eth_signTypedData_* failed, falling back to eth_sign:', error)
+      console.warn('signTypedData: wallet does not implement EIP-712, falling back to eth_sign', error.message)
       const hash = _TypedDataEncoder.hash(populated.domain, types, populated.value)
       return await signer.provider.send('eth_sign', [address, hash])
     }

src/walletconnect.test.ts

@@ -0,0 +1,19 @@
+import { JsonRpcProvider } from '@ethersproject/providers'
+
+import { getPeerMeta } from './walletconnect'
+
+describe('walletconnect', () => {
+  describe('getPeerMeta', () => {
+    it('returns undefined for a non-WalletConnect-derived JsonRpcProvider', () => {
+      const provider = new JsonRpcProvider()
+      expect(getPeerMeta(provider)).toBeUndefined()
+    })
+
+    it('returns peerMeta for a WalletConnect-derived JsonRpcProvider', () => {
+      const provider = new JsonRpcProvider()
+      const peerMeta = {}
+      provider['provider'] = { isWalletConnect: true, connector: { peerMeta } }
+      expect(getPeerMeta(provider)).toBe(peerMeta)
+    })
+  })
+})

src/walletconnect.ts

@@ -0,0 +1,20 @@
+import type { ExternalProvider, JsonRpcProvider, Web3Provider } from '@ethersproject/providers'
+import WalletConnectProvider from '@walletconnect/ethereum-provider'
+
+function isWeb3Provider(provider: JsonRpcProvider): provider is Web3Provider {
+  return 'provider' in provider
+}
+
+function isWalletConnectProvider(provider: ExternalProvider): provider is WalletConnectProvider {
+  return (provider as WalletConnectProvider).isWalletConnect
+}
+
+export type PeerMeta = NonNullable<WalletConnectProvider['connector']['peerMeta']>
+
+export function getPeerMeta(provider: JsonRpcProvider): PeerMeta | undefined {
+  if (isWeb3Provider(provider) && isWalletConnectProvider(provider.provider)) {
+    return provider.provider.connector.peerMeta ?? undefined
+  } else {
+    return
+  }
+}