From bd3a2e165beb6e59a0a48089d785897db4f50c23 Mon Sep 17 00:00:00 2001 From: Aaron Cook Date: Fri, 11 Nov 2022 13:43:23 +0100 Subject: [PATCH] fix: checksum lowercase addresses + validate checksums (#1127) * fix: checksum lowercase addresses + validate * fix: cached value * fix: add + fix tests * fix: add uppercase test --- .../common/AddressInput/index.test.tsx | 40 ++++--- src/components/common/AddressInput/index.tsx | 8 +- src/services/ls-migration/addressBook.ts | 4 +- src/services/ls-migration/tests.test.ts | 1 + src/utils/__tests__/addresses.test.ts | 102 ++++++++++++++++++ src/utils/addresses.ts | 23 +++- src/utils/validation.ts | 5 +- 7 files changed, 158 insertions(+), 25 deletions(-) create mode 100644 src/utils/__tests__/addresses.test.ts diff --git a/src/components/common/AddressInput/index.test.tsx b/src/components/common/AddressInput/index.test.tsx index a1b2b331b0..0dc6d25f49 100644 --- a/src/components/common/AddressInput/index.test.tsx +++ b/src/components/common/AddressInput/index.test.tsx @@ -8,9 +8,9 @@ import useNameResolver from '@/components/common/AddressInput/useNameResolver' // mock useCurrentChain jest.mock('@/hooks/useChains', () => ({ useCurrentChain: jest.fn(() => ({ - shortName: 'rin', - chainId: '4', - chainName: 'Rinkeby', + shortName: 'gor', + chainId: '5', + chainName: 'Goerli', features: ['DOMAIN_LOOKUP'], })), })) @@ -99,7 +99,7 @@ describe('AddressInput tests', () => { ) act(() => { - fireEvent.change(input, { target: { value: 'rin:0x123' } }) + fireEvent.change(input, { target: { value: 'gor:0x123' } }) jest.advanceTimersByTime(1000) }) @@ -110,14 +110,14 @@ describe('AddressInput tests', () => { const { input, utils } = setup('', (val) => `${val} is wrong`) act(() => { - fireEvent.change(input, { target: { value: `rin:${TEST_ADDRESS_A}` } }) + fireEvent.change(input, { target: { value: `gor:${TEST_ADDRESS_A}` } }) jest.advanceTimersByTime(1000) }) await waitFor(() => expect(utils.getByLabelText(`${TEST_ADDRESS_A} is wrong`, { exact: false })).toBeDefined()) act(() => { - fireEvent.change(input, { target: { value: `rin:${TEST_ADDRESS_B}` } }) + fireEvent.change(input, { target: { value: `gor:${TEST_ADDRESS_B}` } }) jest.advanceTimersByTime(1000) }) @@ -127,11 +127,12 @@ describe('AddressInput tests', () => { it('should resolve ENS names', async () => { const { input } = setup('') - await act(async () => { + act(() => { fireEvent.change(input, { target: { value: 'zero.eth' } }) - await waitFor(() => expect(input.value).toBe('rin:0x0000000000000000000000000000000000000000')) }) + await waitFor(() => expect(input.value).toBe('0x0000000000000000000000000000000000000000')) + expect(useNameResolver).toHaveBeenCalledWith('zero.eth') }) @@ -149,9 +150,9 @@ describe('AddressInput tests', () => { it('should not resolve ENS names if this feature is disabled', async () => { ;(useCurrentChain as jest.Mock).mockImplementation(() => ({ - shortName: 'rin', - chainId: '4', - chainName: 'Rinkeby', + shortName: 'gor', + chainId: '5', + chainName: 'Goerli', features: [], })) @@ -172,17 +173,24 @@ describe('AddressInput tests', () => { await waitFor(() => expect(input.value).toBe(TEST_ADDRESS_A)) - expect(input.previousElementSibling?.textContent).toBe('rin:') + expect(input.previousElementSibling?.textContent).toBe('gor:') }) it('should not show adornment when the value contains correct prefix', async () => { - const { input } = setup(`rin:${TEST_ADDRESS_B}`) + ;(useCurrentChain as jest.Mock).mockImplementation(() => ({ + shortName: 'gor', + chainId: '5', + chainName: 'Goerli', + features: [], + })) + + const { input } = setup(`gor:${TEST_ADDRESS_A}`) act(() => { - fireEvent.change(input, { target: { value: 'rin:${TEST_ADDRESS_B}' } }) + fireEvent.change(input, { target: { value: `gor:${TEST_ADDRESS_B}` } }) }) - expect(input.previousElementSibling).toBe(null) + await waitFor(() => expect(input.previousElementSibling).toBe(null)) }) it('should keep a bare address in the form state', async () => { @@ -212,7 +220,7 @@ describe('AddressInput tests', () => { const input = utils.getByLabelText('Recipient', { exact: false }) as HTMLInputElement act(() => { - fireEvent.change(input, { target: { value: `rin:${TEST_ADDRESS_A}` } }) + fireEvent.change(input, { target: { value: `gor:${TEST_ADDRESS_A}` } }) }) expect(methods.getValues().recipient).toBe(TEST_ADDRESS_A) diff --git a/src/components/common/AddressInput/index.tsx b/src/components/common/AddressInput/index.tsx index bb8c32cad8..f44c090074 100644 --- a/src/components/common/AddressInput/index.tsx +++ b/src/components/common/AddressInput/index.tsx @@ -89,10 +89,9 @@ const AddressInput = ({ name, validate, required = true, deps, ...props }: Addre required, setValueAs: (value: string): string => { - const { address, prefix } = parsePrefixedAddress(value) rawValueRef.current = value - // Return a bare address if the prefx is the correct shortName - return prefix === currentShortName ? address : value + // This also checksums the address + return parsePrefixedAddress(value).address }, validate: () => { @@ -105,6 +104,9 @@ const AddressInput = ({ name, validate, required = true, deps, ...props }: Addre // Workaround for a bug in react-hook-form that it restores a cached error state on blur onBlur: () => setTimeout(() => trigger(name), 100), })} + // Workaround for a bug in react-hook-form when `register().value` is cached after `setValueAs` + // Only seems to occur on the `/load` route + value={watchedValue} /> diff --git a/src/services/ls-migration/addressBook.ts b/src/services/ls-migration/addressBook.ts index ba6df8e384..63a3d63e11 100644 --- a/src/services/ls-migration/addressBook.ts +++ b/src/services/ls-migration/addressBook.ts @@ -1,5 +1,5 @@ import { type AddressBookState } from '@/store/addressBookSlice' -import { utils } from 'ethers' +import { isChecksummedAddress } from '@/utils/addresses' import type { LOCAL_STORAGE_DATA } from './common' import { parseLsValue } from './common' @@ -13,7 +13,7 @@ export const migrateAddressBook = (lsData: LOCAL_STORAGE_DATA): AddressBookState console.log('Migrating address book') const newAb = legacyAb.reduce((acc, { address, name, chainId }) => { - if (!name || !address || !utils.isAddress(address)) { + if (!name || !address || !isChecksummedAddress(address)) { return acc } acc[chainId] = acc[chainId] || {} diff --git a/src/services/ls-migration/tests.test.ts b/src/services/ls-migration/tests.test.ts index a37264cc6b..c2a63d77c7 100644 --- a/src/services/ls-migration/tests.test.ts +++ b/src/services/ls-migration/tests.test.ts @@ -57,6 +57,7 @@ describe('Local storage migration', () => { { address: '0x1F2504De05f5167650bE5B28c472601Be434b60A', name: 'Alice', chainId: '1' }, { address: 'sdfgsdfg', name: 'Bob', chainId: '1' }, { address: '0x9913B9180C20C6b0F21B6480c84422F6ebc4B808', name: 'Charlie', chainId: '5' }, + { address: '0x62da87ff2e2216f1858603a3db9313e178da3112 ', name: 'Not checksummed', chainId: '5' }, { address: '', name: 'Dave', chainId: '5' }, { address: undefined, name: 'John', chainId: '5' }, ]), diff --git a/src/utils/__tests__/addresses.test.ts b/src/utils/__tests__/addresses.test.ts new file mode 100644 index 0000000000..fcfd3d10c4 --- /dev/null +++ b/src/utils/__tests__/addresses.test.ts @@ -0,0 +1,102 @@ +import { checksumAddress, isChecksummedAddress, parsePrefixedAddress, sameAddress } from '../addresses' + +describe('Addresses', () => { + describe('checksumAddress', () => { + it('should checksum lowercase addresses', () => { + const value = checksumAddress('0x62da87ff2e2216f1858603a3db9313e178da3112') + expect(value).toBe('0x62Da87FF2E2216F1858603A3Db9313E178da3112') + }) + + it('should return checksummed addresses as is', () => { + const value = checksumAddress('0x62Da87FF2E2216F1858603A3Db9313E178da3112') + expect(value).toBe('0x62Da87FF2E2216F1858603A3Db9313E178da3112') + }) + + it('should return mixed case addresses as is', () => { + const value = checksumAddress('0x62Da87ff2E2216F1858603A3Db9313E178da3112') + expect(value).toBe('0x62Da87ff2E2216F1858603A3Db9313E178da3112') + }) + + it('should return uppercase addresses as is', () => { + const value = checksumAddress('0X62DA87FF2E2216F1858603A3DB9313E178DA3112') + expect(value).toBe('0X62DA87FF2E2216F1858603A3DB9313E178DA3112') + }) + + it('should return non-addresses as is', () => { + const value = checksumAddress('sdfgsdfg') + expect(value).toBe('sdfgsdfg') + }) + }) + + describe('isChecksummedAddress', () => { + it('should return true for checksummed addresses', () => { + const value = isChecksummedAddress('0x62Da87FF2E2216F1858603A3Db9313E178da3112') + expect(value).toBe(true) + }) + + it('should return false for lowercase addresses', () => { + const value = isChecksummedAddress('0x62da87ff2e2216f1858603a3db9313e178da3112') + expect(value).toBe(false) + }) + + it('should return false for mixed case addresses', () => { + const value = isChecksummedAddress('0x62Da87ff2E2216F1858603A3Db9313E178da3112') + expect(value).toBe(false) + }) + + it('should return false for uppercase addresses', () => { + const value = isChecksummedAddress('0X62DA87FF2E2216F1858603A3DB9313E178DA3112') + expect(value).toBe(false) + }) + + it('should return false for non-/invalid addresses', () => { + const value = isChecksummedAddress('sdfgsdfg') + expect(value).toBe(false) + }) + }) + + describe('sameAddress', () => { + it('returns false if the first or second address is undefined', () => { + const address = '0x62Da87FF2E2216F1858603A3Db9313E178da3112' + expect(sameAddress(undefined, address)).toBe(false) + expect(sameAddress(address, undefined)).toBe(false) + }) + + it('returns false if the addresses are different', () => { + const address1 = '0x62Da87FF2E2216F1858603A3Db9313E178da3112' + const address2 = '0x62Da87FF2E2216F1858603A3Db9313E178da3113' + expect(sameAddress(address1, address2)).toBe(false) + }) + + it('returns true if the addresses are the same', () => { + const address = '0x62Da87FF2E2216F1858603A3Db9313E178da3112' + expect(sameAddress(address, address)).toBe(true) + }) + }) + + describe('parsePrefixedAddress', () => { + it('should parse a prefixed address', () => { + const { prefix, address } = parsePrefixedAddress('prefix:0x62Da87FF2E2216F1858603A3Db9313E178da3112') + expect(prefix).toBe('prefix') + expect(address).toBe('0x62Da87FF2E2216F1858603A3Db9313E178da3112') + }) + + it('should parse a non-prefixed address', () => { + const { prefix, address } = parsePrefixedAddress('0x62Da87FF2E2216F1858603A3Db9313E178da3112') + expect(prefix).toBeUndefined() + expect(address).toBe('0x62Da87FF2E2216F1858603A3Db9313E178da3112') + }) + + it('should checksum addresses', () => { + const { prefix, address } = parsePrefixedAddress('0x62da87ff2e2216f1858603a3db9313e178da3112') + expect(prefix).toBeUndefined() + expect(address).toBe('0x62Da87FF2E2216F1858603A3Db9313E178da3112') + }) + + it('should parse a non-addresses', () => { + const { prefix, address } = parsePrefixedAddress('sdfgsdfg') + expect(prefix).toBeUndefined() + expect(address).toBe('sdfgsdfg') + }) + }) +}) diff --git a/src/utils/addresses.ts b/src/utils/addresses.ts index 99daad7671..f8323bc610 100644 --- a/src/utils/addresses.ts +++ b/src/utils/addresses.ts @@ -1,6 +1,22 @@ import { getAddress } from 'ethers/lib/utils' import { isAddress } from '@ethersproject/address' +export const checksumAddress = (address: string): string => { + return isAddress(address) ? getAddress(address) : address +} + +export const isChecksummedAddress = (address: string): boolean => { + if (!isAddress(address)) { + return false + } + + try { + return getAddress(address) === address + } catch { + return false + } +} + export const sameAddress = (firstAddress: string | undefined, secondAddress: string | undefined): boolean => { if (!firstAddress || !secondAddress) { return false @@ -14,6 +30,11 @@ export type PrefixedAddress = { address: string } +/** + * Parses a string that may/may not contain an address and returns the `prefix` and checksummed `address` + * @param value (prefixed) address + * @returns `prefix` and checksummed `address` + */ export const parsePrefixedAddress = (value: string): PrefixedAddress => { let [prefix, address] = value.split(':') @@ -24,7 +45,7 @@ export const parsePrefixedAddress = (value: string): PrefixedAddress => { return { prefix: prefix || undefined, - address: isAddress(address) ? getAddress(address) : value, + address: checksumAddress(address), } } diff --git a/src/utils/validation.ts b/src/utils/validation.ts index 7e9d1b08e4..5abff3ba1a 100644 --- a/src/utils/validation.ts +++ b/src/utils/validation.ts @@ -1,6 +1,5 @@ import chains from '@/config/chains' -import { isAddress } from '@ethersproject/address' -import { parsePrefixedAddress, sameAddress } from './addresses' +import { parsePrefixedAddress, sameAddress, isChecksummedAddress } from './addresses' import { safeFormatUnits, safeParseUnits } from './formatters' export const validateAddress = (address: string) => { @@ -10,7 +9,7 @@ export const validateAddress = (address: string) => { return 'Invalid address format' } - if (!isAddress(address)) { + if (!isChecksummedAddress(address)) { return 'Invalid address checksum' } }