From 49f2ea1632adcb9903bb336eb8b19fe92d9603a7 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 22 Jun 2022 12:42:49 +0200 Subject: [PATCH 1/7] fix: revert history loading into separate fns --- .../loadGatewayTransactions.ts | 109 +++++++++--------- src/utils/constants.ts | 2 +- 2 files changed, 53 insertions(+), 58 deletions(-) diff --git a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts index cb58790b22..f1095f83bf 100644 --- a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts +++ b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts @@ -16,52 +16,34 @@ import { getMultisigFilter, getModuleFilter, } from 'src/routes/safe/components/Transactions/TxList/Filter/utils' -import { ChainId } from 'src/config/chain.d' -import { operations } from '@gnosis.pm/safe-react-gateway-sdk/dist/types/api' /*************/ /* HISTORY */ /*************/ -const historyPointers: { - [chainId: string]: { - [safeAddress: string]: { - next?: string - previous?: string - } - } -} = {} -const getHistoryTxListPage = async ( - chainId: ChainId, +export const _getTxHistory = async ( + chainId: string, safeAddress: string, filter?: FilterForm | Partial, -): Promise => { + next?: string, +) => { let txListPage: TransactionListPage = { next: undefined, previous: undefined, results: [], } - const { next } = historyPointers[chainId]?.[safeAddress] || {} - - let query: - | operations['incoming_transfers' | 'incoming_transfers' | 'module_transactions']['parameters']['query'] - | undefined - switch (filter?.[FILTER_TYPE_FIELD_NAME]) { case FilterType.INCOMING: { - query = filter ? getIncomingFilter(filter) : undefined - txListPage = await getIncomingTransfers(chainId, safeAddress, query, next) + txListPage = await getIncomingTransfers(chainId, safeAddress, getIncomingFilter(filter), next) break } case FilterType.MULTISIG: { - query = filter ? getMultisigFilter(filter, true) : undefined - txListPage = await getMultisigTransactions(chainId, safeAddress, query, next) + txListPage = await getMultisigTransactions(chainId, safeAddress, getMultisigFilter(filter, true), next) break } case FilterType.MODULE: { - query = filter ? getModuleFilter(filter) : undefined - txListPage = await getModuleTransactions(chainId, safeAddress, query, next) + txListPage = await getModuleTransactions(chainId, safeAddress, getModuleFilter(filter), next) break } default: { @@ -69,31 +51,41 @@ const getHistoryTxListPage = async ( } } - const getPageUrl = (pageUrl?: string): string | undefined => { - if (!pageUrl || !query) { - return pageUrl - } + return txListPage +} - let url: URL +export const _getHistoryPageUrl = (pageUrl?: string, filter?: FilterForm | Partial): undefined | string => { + if (!pageUrl || !filter) { + return pageUrl + } - try { - url = new URL(pageUrl) - } catch { - return pageUrl - } + let url: URL - Object.entries(query).forEach(([key, value]) => { + try { + url = new URL(pageUrl) + } catch { + return pageUrl + } + + Object.entries(filter) + .filter(([, value]) => Boolean(value)) + .forEach(([key, value]) => { url.searchParams.set(key, String(value)) }) - return url.toString() - } + return url.toString() +} - historyPointers[chainId][safeAddress].next = getPageUrl(txListPage?.next) - historyPointers[chainId][safeAddress].previous = getPageUrl(txListPage?.previous) +const historyPointers: { [chainId: string]: { [safeAddress: string]: { next?: string; previous?: string } } } = {} - return txListPage -} +const getHistoryPointer = ( + next?: string, + previous?: string, + filter?: FilterForm | Partial, +): { next?: string; previous?: string } => ({ + next: _getHistoryPageUrl(next, filter), + previous: _getHistoryPageUrl(previous, filter), +}) /** * Fetch next page if there is a next pointer for the safeAddress. @@ -102,17 +94,26 @@ const getHistoryTxListPage = async ( */ export const loadPagedHistoryTransactions = async ( safeAddress: string, + filter?: FilterForm | Partial, ): Promise<{ values: HistoryGatewayResponse['results']; next?: string } | undefined> => { const chainId = _getChainId() - - if (!historyPointers[chainId]?.[safeAddress]?.next) { + // if `historyPointers[safeAddress] is `undefined` it means `loadHistoryTransactions` wasn't called + // if `historyPointers[safeAddress].next is `null`, it means it reached the last page in gateway-client + if (!historyPointers[chainId][safeAddress]?.next) { throw new CodedException(Errors._608) } try { - const { results, next } = await getHistoryTxListPage(chainId, safeAddress) + const { results, next, previous } = await _getTxHistory( + chainId, + checksumAddress(safeAddress), + filter, + historyPointers[chainId][safeAddress].next, + ) + + historyPointers[chainId][safeAddress] = getHistoryPointer(next, previous, filter) - return { values: results, next } + return { values: results, next: historyPointers[chainId][safeAddress].next } } catch (e) { throw new CodedException(Errors._602, e.message) } @@ -123,20 +124,14 @@ export const loadHistoryTransactions = async ( filter?: FilterForm | Partial, ): Promise => { const chainId = _getChainId() + try { + const { results, next, previous } = await _getTxHistory(chainId, checksumAddress(safeAddress), filter) - if (!historyPointers[chainId]) { - historyPointers[chainId] = {} - } - - if (!historyPointers[chainId][safeAddress] || filter) { - historyPointers[chainId][safeAddress] = { - next: undefined, - previous: undefined, + if (!historyPointers[chainId]) { + historyPointers[chainId] = {} } - } - try { - const { results } = await getHistoryTxListPage(chainId, safeAddress, filter) + historyPointers[chainId][safeAddress] = getHistoryPointer(next, previous, filter) return results } catch (e) { diff --git a/src/utils/constants.ts b/src/utils/constants.ts index 0bed2d4e55..ccbc7972c5 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -17,7 +17,7 @@ export const SAFE_APPS_RPC_TOKEN = process.env.REACT_APP_SAFE_APPS_RPC_INFURA_TO export const LATEST_SAFE_VERSION = process.env.REACT_APP_LATEST_SAFE_VERSION || '1.3.0' export const APP_VERSION = process.env.REACT_APP_APP_VERSION || 'not-defined' export const COLLECTIBLES_SOURCE = process.env.REACT_APP_COLLECTIBLES_SOURCE || 'Gnosis' -export const SAFE_POLLING_INTERVAL = process.env.NODE_ENV === 'test' ? 4500 : 15000 +export const SAFE_POLLING_INTERVAL = process.env.NODE_ENV === 'test' ? 4500 : 4000 export const ETHERSCAN_API_KEY = process.env.REACT_APP_ETHERSCAN_API_KEY || '' export const ETHGASSTATION_API_KEY = process.env.REACT_APP_ETHGASSTATION_API_KEY export const IPFS_GATEWAY = process.env.REACT_APP_IPFS_GATEWAY From 052e6d43a5a008ac53d694b934d613095151b49c Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 22 Jun 2022 12:58:26 +0200 Subject: [PATCH 2/7] fix: add tests --- .../__tests__/loadGatewayTransactions.test.ts | 79 +++++++++++++++++++ .../loadGatewayTransactions.ts | 4 +- 2 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 src/logic/safe/store/actions/transactions/__tests__/loadGatewayTransactions.test.ts diff --git a/src/logic/safe/store/actions/transactions/__tests__/loadGatewayTransactions.test.ts b/src/logic/safe/store/actions/transactions/__tests__/loadGatewayTransactions.test.ts new file mode 100644 index 0000000000..8ee4c81c5d --- /dev/null +++ b/src/logic/safe/store/actions/transactions/__tests__/loadGatewayTransactions.test.ts @@ -0,0 +1,79 @@ +import * as gatewaySDK from '@gnosis.pm/safe-react-gateway-sdk' +import { FilterType } from 'src/routes/safe/components/Transactions/TxList/Filter' +import { _getHistoryPageUrl, _getTxHistory } from '../fetchTransactions/loadGatewayTransactions' + +jest.mock('@gnosis.pm/safe-react-gateway-sdk', () => { + const original = jest.requireActual('@gnosis.pm/safe-react-gateway-sdk') + return { + ...original, + getIncomingTransfers: jest.fn, + getMultisigTransactions: jest.fn, + getModuleTransactions: jest.fn, + getTransactionHistory: jest.fn, + } +}) + +describe('loadGatewayTransactions', () => { + beforeEach(() => { + jest.resetAllMocks() + }) + + describe('getTxHistory', () => { + it('fetches incoming transfers according to type', async () => { + const spy = jest.spyOn(gatewaySDK, 'getIncomingTransfers') + + await _getTxHistory('4', '0x123', { type: FilterType.INCOMING, token_address: '0x456' }) + + expect(spy).toHaveBeenCalledWith('4', '0x123', { token_address: '0x456' }, undefined) + }) + + it('fetches multisig transfers according to type', async () => { + const spy = jest.spyOn(gatewaySDK, 'getMultisigTransactions') + + await _getTxHistory('4', '0x123', { type: FilterType.MULTISIG, to: '0x456' }) + + expect(spy).toHaveBeenCalledWith('4', '0x123', { to: '0x456', executed: 'true' }, undefined) + }) + + it('fetches module transfers according to type', async () => { + const spy = jest.spyOn(gatewaySDK, 'getModuleTransactions') + + await _getTxHistory('4', '0x123', { type: FilterType.MODULE, to: '0x456' }) + + expect(spy).toHaveBeenCalledWith('4', '0x123', { to: '0x456' }, undefined) + }) + + it('fetches historical transfers by default', async () => { + const spy = jest.spyOn(gatewaySDK, 'getTransactionHistory') + + await _getTxHistory('4', '0x123') + + expect(spy).toHaveBeenCalledWith('4', '0x123', undefined) + }) + }) + + describe('getHistoryPageUrl', () => { + it('returns the pageUrl when a falsy pageUrl is provided', () => { + // SDK types should allow for `null` in TransactionListPage['next' | 'previous'] as it's returned by gateway + expect(_getHistoryPageUrl(null as unknown as undefined, { type: FilterType.INCOMING })).toBe(null) + }) + + it('returns the pageUrl when a no filter is provided', () => { + expect(_getHistoryPageUrl('http://test123.com', undefined)).toBe('http://test123.com') + expect(_getHistoryPageUrl('http://test456.com', {})).toBe('http://test456.com') + }) + + it('returns the pageUrl if it is an invalid URL', () => { + expect(_getHistoryPageUrl('test123', { type: FilterType.INCOMING })).toBe('test123') + }) + + it('appends only defined filter values to the pageUrl', () => { + expect(_getHistoryPageUrl('http://test123.com', { type: FilterType.INCOMING, value: undefined })).toBe( + 'http://test123.com/?type=Incoming', + ) + expect( + _getHistoryPageUrl('http://test456.com', { type: FilterType.MULTISIG, execution_date__gte: undefined }), + ).toBe('http://test456.com/?type=Outgoing') + }) + }) +}) diff --git a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts index f1095f83bf..008b4fef5b 100644 --- a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts +++ b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts @@ -55,7 +55,7 @@ export const _getTxHistory = async ( } export const _getHistoryPageUrl = (pageUrl?: string, filter?: FilterForm | Partial): undefined | string => { - if (!pageUrl || !filter) { + if (!pageUrl || !filter || Object.keys(filter).length === 0) { return pageUrl } @@ -68,7 +68,7 @@ export const _getHistoryPageUrl = (pageUrl?: string, filter?: FilterForm | Parti } Object.entries(filter) - .filter(([, value]) => Boolean(value)) + .filter(([, value]) => value !== undefined) .forEach(([key, value]) => { url.searchParams.set(key, String(value)) }) From e38d30951be2518b46cd3f40c465279dd826c7d7 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 22 Jun 2022 13:00:04 +0200 Subject: [PATCH 3/7] fix: remove test value --- src/utils/constants.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/constants.ts b/src/utils/constants.ts index ccbc7972c5..0bed2d4e55 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -17,7 +17,7 @@ export const SAFE_APPS_RPC_TOKEN = process.env.REACT_APP_SAFE_APPS_RPC_INFURA_TO export const LATEST_SAFE_VERSION = process.env.REACT_APP_LATEST_SAFE_VERSION || '1.3.0' export const APP_VERSION = process.env.REACT_APP_APP_VERSION || 'not-defined' export const COLLECTIBLES_SOURCE = process.env.REACT_APP_COLLECTIBLES_SOURCE || 'Gnosis' -export const SAFE_POLLING_INTERVAL = process.env.NODE_ENV === 'test' ? 4500 : 4000 +export const SAFE_POLLING_INTERVAL = process.env.NODE_ENV === 'test' ? 4500 : 15000 export const ETHERSCAN_API_KEY = process.env.REACT_APP_ETHERSCAN_API_KEY || '' export const ETHGASSTATION_API_KEY = process.env.REACT_APP_ETHGASSTATION_API_KEY export const IPFS_GATEWAY = process.env.REACT_APP_IPFS_GATEWAY From b4a0cbd288cd1945f529e5550dc3b3a167243b41 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 22 Jun 2022 13:07:41 +0200 Subject: [PATCH 4/7] fix: add return type --- .../transactions/fetchTransactions/loadGatewayTransactions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts index 008b4fef5b..d58abee23e 100644 --- a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts +++ b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts @@ -26,7 +26,7 @@ export const _getTxHistory = async ( safeAddress: string, filter?: FilterForm | Partial, next?: string, -) => { +): Promise => { let txListPage: TransactionListPage = { next: undefined, previous: undefined, From 33c753206db725feea2469fa0be37ee851184d4d Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 22 Jun 2022 13:27:34 +0200 Subject: [PATCH 5/7] fix: set history pointers conditionally --- .../transactions/fetchTransactions/loadGatewayTransactions.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts index d58abee23e..cefb79470d 100644 --- a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts +++ b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts @@ -131,7 +131,9 @@ export const loadHistoryTransactions = async ( historyPointers[chainId] = {} } - historyPointers[chainId][safeAddress] = getHistoryPointer(next, previous, filter) + if (!historyPointers[chainId][safeAddress]) { + historyPointers[chainId][safeAddress] = getHistoryPointer(next, previous, filter) + } return results } catch (e) { From f2ba75b3353f8e80261e731803bad4c6691f6da5 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 22 Jun 2022 14:54:20 +0200 Subject: [PATCH 6/7] fix: no conditional pointers + clear on navigation --- .../loadGatewayTransactions.ts | 4 +- .../Transactions/TxList/Filter/index.tsx | 38 +++++++++---------- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts index cefb79470d..d58abee23e 100644 --- a/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts +++ b/src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts @@ -131,9 +131,7 @@ export const loadHistoryTransactions = async ( historyPointers[chainId] = {} } - if (!historyPointers[chainId][safeAddress]) { - historyPointers[chainId][safeAddress] = getHistoryPointer(next, previous, filter) - } + historyPointers[chainId][safeAddress] = getHistoryPointer(next, previous, filter) return results } catch (e) { diff --git a/src/routes/safe/components/Transactions/TxList/Filter/index.tsx b/src/routes/safe/components/Transactions/TxList/Filter/index.tsx index 6d54093888..fbd8de2005 100644 --- a/src/routes/safe/components/Transactions/TxList/Filter/index.tsx +++ b/src/routes/safe/components/Transactions/TxList/Filter/index.tsx @@ -1,4 +1,4 @@ -import { ReactElement, useCallback, useEffect, useState } from 'react' +import { ReactElement, useEffect, useState } from 'react' import { Controller, DefaultValues, useForm } from 'react-hook-form' import styled from 'styled-components' import ExpandMoreIcon from '@material-ui/icons/ExpandMore' @@ -144,28 +144,28 @@ const Filter = (): ReactElement => { }) } - const clearFilter = useCallback( - ({ clearSearch = true } = {}) => { - if (search && clearSearch) { - history.replace(pathname) - dispatch(loadTransactions({ chainId, safeAddress: checksumAddress(safeAddress) })) - reset(defaultValues) - } + const clearFilter = () => { + history.replace({ search: '' }) + dispatch(loadTransactions({ chainId, safeAddress: checksumAddress(safeAddress) })) + reset(defaultValues) + hideFilter() + } - hideFilter() - }, - [search, history, pathname, chainId, dispatch, reset, safeAddress], - ) + const filterType = watch(FILTER_TYPE_FIELD_NAME) useEffect(() => { + // We cannot rely on cleanup as setting search params (in onSubmit) unmounts the component + const unsubscribe = history.listen((newLocation) => { + const shouldResetHistory = filterType && newLocation.pathname !== pathname + if (shouldResetHistory) { + dispatch(loadTransactions({ chainId, safeAddress: checksumAddress(safeAddress) })) + } + }) + return () => { - // If search is programatically cleared on unmount, the router routes back to here - // Search is inherently cleared when unmounting either way - clearFilter({ clearSearch: false }) + unsubscribe() } - }, [clearFilter]) - - const filterType = watch(FILTER_TYPE_FIELD_NAME) + }, [history, chainId, dispatch, filterType, pathname, safeAddress]) const onSubmit = (filter: FilterForm) => { // Don't apply the same filter twice @@ -175,7 +175,7 @@ const Filter = (): ReactElement => { const query = Object.fromEntries(Object.entries(filter).filter(([, value]) => !!value)) - history.replace({ pathname, search: `?${new URLSearchParams(query).toString()}` }) + history.replace({ search: `?${new URLSearchParams(query).toString()}` }) dispatch(loadTransactions({ chainId, safeAddress: checksumAddress(safeAddress), filter })) From 3d4c4f0db95fe26555f7e354bcb35968f7232679 Mon Sep 17 00:00:00 2001 From: iamacook Date: Wed, 22 Jun 2022 15:40:01 +0200 Subject: [PATCH 7/7] fix: remove dependencies --- .../safe/components/Transactions/TxList/Filter/index.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/routes/safe/components/Transactions/TxList/Filter/index.tsx b/src/routes/safe/components/Transactions/TxList/Filter/index.tsx index fbd8de2005..30ffdd4995 100644 --- a/src/routes/safe/components/Transactions/TxList/Filter/index.tsx +++ b/src/routes/safe/components/Transactions/TxList/Filter/index.tsx @@ -165,7 +165,8 @@ const Filter = (): ReactElement => { return () => { unsubscribe() } - }, [history, chainId, dispatch, filterType, pathname, safeAddress]) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []) const onSubmit = (filter: FilterForm) => { // Don't apply the same filter twice