From 7c19205e01a56e52f6b21c68d4823eddbee6bed0 Mon Sep 17 00:00:00 2001 From: Agustin Pane Date: Tue, 15 Dec 2020 18:23:12 -0300 Subject: [PATCH] (Feature) - Expanded decoded data collapses automatically (#1731) * Adds shouldSafeBeUpdated to avoid updating unnecessary the safe * Fix css typing * Moves equalArrays to utils function Improves how updateSafe is dispatched within checkAndUpdateSafe to avoid unnecessary re renders * Revert some default cases fixes * Adds equalArrays tests * Add test to arrays.test.ts * Improves shouldSafeStoreBeUpdated order and renames it * Adds shouldSafeStoreBeUpdated.test.ts * Uses shouldSafeStoreBeUpdated within safe reducer * Replaces equalArrays with isEqual from lodash * Simplify shouldSafeStoreBeUpdated using isEqual from lodash * Remove lodash to compare Immutable objects correctly Co-authored-by: Daniel Sanchez --- src/logic/safe/store/actions/fetchSafe.ts | 24 +- src/logic/safe/store/reducer/safe.ts | 29 +- .../shouldSafeStoreBeUpdated.test.ts | 369 ++++++++++++++++++ .../safe/utils/shouldSafeStoreBeUpdated.ts | 26 ++ .../Transactions/TxsTable/index.tsx | 14 +- .../components/Transactions/TxsTable/style.ts | 4 +- 6 files changed, 435 insertions(+), 31 deletions(-) create mode 100644 src/logic/safe/utils/__tests__/shouldSafeStoreBeUpdated.test.ts create mode 100644 src/logic/safe/utils/shouldSafeStoreBeUpdated.ts diff --git a/src/logic/safe/store/actions/fetchSafe.ts b/src/logic/safe/store/actions/fetchSafe.ts index 71e2218f..43def162 100644 --- a/src/logic/safe/store/actions/fetchSafe.ts +++ b/src/logic/safe/store/actions/fetchSafe.ts @@ -117,19 +117,17 @@ export const checkAndUpdateSafe = (safeAdd: string) => async (dispatch: Dispatch const modules = await getModules(safeInfo) - dispatch( - updateSafe({ - address: safeAddress, - name: localSafe?.name, - modules, - spendingLimits, - nonce: Number(remoteNonce), - threshold: Number(remoteThreshold), - featuresEnabled: localSafe?.currentVersion - ? enabledFeatures(localSafe?.currentVersion) - : localSafe?.featuresEnabled, - }), - ) + const updatedSafe = { + address: safeAddress, + name: localSafe?.name, + modules, + spendingLimits, + nonce: Number(remoteNonce), + threshold: Number(remoteThreshold), + featuresEnabled: localSafe?.currentVersion ? enabledFeatures(localSafe.currentVersion) : localSafe?.featuresEnabled, + } + + dispatch(updateSafe(updatedSafe)) // If the remote owners does not contain a local address, we remove that local owner localOwners.forEach((localAddress) => { diff --git a/src/logic/safe/store/reducer/safe.ts b/src/logic/safe/store/reducer/safe.ts index 5409c9e5..0d651704 100644 --- a/src/logic/safe/store/reducer/safe.ts +++ b/src/logic/safe/store/reducer/safe.ts @@ -18,6 +18,7 @@ import { checksumAddress } from 'src/utils/checksumAddress' import { SafeReducerMap } from 'src/routes/safe/store/reducer/types/safe' import { ADD_OR_UPDATE_SAFE, buildOwnersFrom } from 'src/logic/safe/store/actions/addOrUpdateSafe' import { sameAddress } from 'src/logic/wallets/ethAddresses' +import { shouldSafeStoreBeUpdated } from 'src/logic/safe/utils/shouldSafeStoreBeUpdated' export const SAFE_REDUCER_ID = 'safes' export const DEFAULT_SAFE_INITIAL_STATE = 'NOT_ASKED' @@ -78,11 +79,15 @@ export default handleActions( const safe = action.payload const safeAddress = safe.address - return state.updateIn( - ['safes', safeAddress], - makeSafe({ name: safe?.name || 'LOADED SAFE', address: safeAddress }), - (prevSafe) => updateSafeProps(prevSafe, safe), - ) + const shouldUpdate = shouldSafeStoreBeUpdated(safe, state.getIn(['safes', safeAddress])) + + return shouldUpdate + ? state.updateIn( + ['safes', safeAddress], + makeSafe({ name: safe?.name || 'LOADED SAFE', address: safeAddress }), + (prevSafe) => updateSafeProps(prevSafe, safe), + ) + : state }, [ACTIVATE_TOKEN_FOR_ALL_SAFES]: (state: SafeReducerMap, action) => { const tokenAddress = action.payload @@ -107,11 +112,15 @@ export default handleActions( return state.setIn(['safes', safe.address], makeSafe(safe)) } - return state.updateIn( - ['safes', safe.address], - makeSafe({ name: safe?.name || 'LOADED SAFE', address: safe.address }), - (prevSafe) => updateSafeProps(prevSafe, safe), - ) + const shouldUpdate = shouldSafeStoreBeUpdated(safe, state.getIn(['safes', safe.safeAddress])) + + return shouldUpdate + ? state.updateIn( + ['safes', safe.address], + makeSafe({ name: safe?.name || 'LOADED SAFE', address: safe.address }), + (prevSafe) => updateSafeProps(prevSafe, safe), + ) + : state }, [REMOVE_SAFE]: (state: SafeReducerMap, action) => { const safeAddress = action.payload diff --git a/src/logic/safe/utils/__tests__/shouldSafeStoreBeUpdated.test.ts b/src/logic/safe/utils/__tests__/shouldSafeStoreBeUpdated.test.ts new file mode 100644 index 00000000..5d029744 --- /dev/null +++ b/src/logic/safe/utils/__tests__/shouldSafeStoreBeUpdated.test.ts @@ -0,0 +1,369 @@ +import { SafeRecordProps } from 'src/logic/safe/store/models/safe' +import { List, Set, Map } from 'immutable' +import { shouldSafeStoreBeUpdated } from 'src/logic/safe/utils/shouldSafeStoreBeUpdated' + +const getMockedOldSafe = ({ + address, + needsUpdate, + balances, + recurringUser, + blacklistedAssets, + blacklistedTokens, + activeAssets, + activeTokens, + owners, + featuresEnabled, + currentVersion, + latestIncomingTxBlock, + ethBalance, + threshold, + name, + nonce, + modules, + spendingLimits, +}: Partial): SafeRecordProps => { + const owner1 = { + name: 'MockedOwner1', + address: '0x3bE3c2dE077FBC409ae50AFFA66a94a9aE669A8d', + } + const owner2 = { + name: 'MockedOwner2', + address: '0xA2366b0c2607de70777d87aCdD1D22F0708fA6a3', + } + const mockedActiveTokenAddress1 = '0x36591cd3DA96b21Ac9ca54cFaf80fe45107294F1' + const mockedActiveTokenAddress2 = '0x92aF97cbF10742dD2527ffaBA70e34C03CFFC2c1' + const mockedActiveAssetsAddress1 = '0x503ab2a6A70c6C6ec8b25a4C87C784e1c8f8e8CD' + const mockedActiveAssetsAddress2 = '0xfdd4E685361CB7E89a4D27e03DCd0001448d731F' + const mockedBlacklistedTokenAddress1 = '0xc7d892dca37a244Fb1A7461e6141e58Ead460282' + const mockedBlacklistedAssetAddress1 = '0x0ac539137c4c99001f16Dd132E282F99A02Ddc3F' + + return { + name: name || 'MockedSafe', + address: address || '0xAE173F30ec9A293d37c44BA68d3fCD35F989Ce9F', + threshold: threshold || 2, + ethBalance: ethBalance || '10', + owners: owners || List([owner1, owner2]), + modules: modules || [], + spendingLimits: spendingLimits || [], + activeTokens: activeTokens || Set([mockedActiveTokenAddress1, mockedActiveTokenAddress2]), + activeAssets: activeAssets || Set([mockedActiveAssetsAddress1, mockedActiveAssetsAddress2]), + blacklistedTokens: blacklistedTokens || Set([mockedBlacklistedTokenAddress1]), + blacklistedAssets: blacklistedAssets || Set([mockedBlacklistedAssetAddress1]), + balances: + balances || + Map({ + [mockedActiveTokenAddress1]: '100', + [mockedActiveTokenAddress2]: '10', + }), + nonce: nonce || 2, + latestIncomingTxBlock: latestIncomingTxBlock || 1, + recurringUser: recurringUser || false, + currentVersion: currentVersion || 'v1.1.1', + needsUpdate: needsUpdate || false, + featuresEnabled: featuresEnabled || [], + } +} + +describe('shouldSafeStoreBeUpdated', () => { + it(`Given two equal safes, should return false`, () => { + // given + const oldSafe = getMockedOldSafe({}) + + // When + const expectedResult = shouldSafeStoreBeUpdated(oldSafe, oldSafe) + + // Then + expect(expectedResult).toEqual(false) + }) + it(`Given an old safe and a new address for the safe, should return true`, () => { + // given + const oldAddress = '0x123' + const newAddress = '0x' + const oldSafe = getMockedOldSafe({ address: oldAddress }) + const newSafeProps: Partial = { + address: newAddress, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old safe and a new name for the safe, should return true`, () => { + // given + const oldName = 'oldName' + const newName = 'newName' + const oldSafe = getMockedOldSafe({ name: oldName }) + const newSafeProps: Partial = { + name: newName, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old safe and a new threshold for the safe, should return true`, () => { + // given + const oldThreshold = 1 + const newThreshold = 2 + const oldSafe = getMockedOldSafe({ threshold: oldThreshold }) + const newSafeProps: Partial = { + threshold: newThreshold, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old ethBalance and a new ethBalance for the safe, should return true`, () => { + // given + const oldEthBalance = '1' + const newEthBalance = '2' + const oldSafe = getMockedOldSafe({ ethBalance: oldEthBalance }) + const newSafeProps: Partial = { + ethBalance: newEthBalance, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old owners list and a new owners list for the safe, should return true`, () => { + // given + const owner1 = { + name: 'MockedOwner1', + address: '0x3bE3c2dE077FBC409ae50AFFA66a94a9aE669A8d', + } + const owner2 = { + name: 'MockedOwner2', + address: '0xA2366b0c2607de70777d87aCdD1D22F0708fA6a3', + } + const oldSafe = getMockedOldSafe({ owners: List([owner1, owner2]) }) + const newSafeProps: Partial = { + owners: List([owner1]), + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old modules list and a new modules list for the safe, should return true`, () => { + // given + const oldModulesList = [] + const newModulesList = null + const oldSafe = getMockedOldSafe({ modules: oldModulesList }) + const newSafeProps: Partial = { + modules: newModulesList, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old spendingLimits list and a new spendingLimits list for the safe, should return true`, () => { + // given + const oldSpendingLimitsList = [] + const newSpendingLimitsList = null + const oldSafe = getMockedOldSafe({ spendingLimits: oldSpendingLimitsList }) + const newSafeProps: Partial = { + modules: newSpendingLimitsList, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old activeTokens list and a new activeTokens list for the safe, should return true`, () => { + // given + const mockedActiveTokenAddress1 = '0x36591cd3DA96b21Ac9ca54cFaf80fe45107294F1' + const mockedActiveTokenAddress2 = '0x92aF97cbF10742dD2527ffaBA70e34C03CFFC2c1' + const oldActiveTokens = Set([mockedActiveTokenAddress1, mockedActiveTokenAddress2]) + const newActiveTokens = Set([mockedActiveTokenAddress1]) + const oldSafe = getMockedOldSafe({ activeTokens: oldActiveTokens }) + const newSafeProps: Partial = { + activeTokens: newActiveTokens, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old activeAssets list and a new activeAssets list for the safe, should return true`, () => { + // given + const mockedActiveTokenAddress1 = '0x36591cd3DA96b21Ac9ca54cFaf80fe45107294F1' + const mockedActiveTokenAddress2 = '0x92aF97cbF10742dD2527ffaBA70e34C03CFFC2c1' + const oldActiveAssets = Set([mockedActiveTokenAddress1, mockedActiveTokenAddress2]) + const newActiveAssets = Set([mockedActiveTokenAddress1]) + const oldSafe = getMockedOldSafe({ activeAssets: oldActiveAssets }) + const newSafeProps: Partial = { + activeAssets: newActiveAssets, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old blacklistedTokens list and a new blacklistedTokens list for the safe, should return true`, () => { + // given + const mockedActiveTokenAddress1 = '0x36591cd3DA96b21Ac9ca54cFaf80fe45107294F1' + const mockedActiveTokenAddress2 = '0x92aF97cbF10742dD2527ffaBA70e34C03CFFC2c1' + const oldBlacklistedTokens = Set([mockedActiveTokenAddress1, mockedActiveTokenAddress2]) + const newBlacklistedTokens = Set([mockedActiveTokenAddress1]) + const oldSafe = getMockedOldSafe({ blacklistedTokens: oldBlacklistedTokens }) + const newSafeProps: Partial = { + blacklistedTokens: newBlacklistedTokens, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old blacklistedAssets list and a new blacklistedAssets list for the safe, should return true`, () => { + // given + const mockedActiveTokenAddress1 = '0x36591cd3DA96b21Ac9ca54cFaf80fe45107294F1' + const mockedActiveTokenAddress2 = '0x92aF97cbF10742dD2527ffaBA70e34C03CFFC2c1' + const oldBlacklistedAssets = Set([mockedActiveTokenAddress1, mockedActiveTokenAddress2]) + const newBlacklistedAssets = Set([mockedActiveTokenAddress1]) + const oldSafe = getMockedOldSafe({ blacklistedAssets: oldBlacklistedAssets }) + const newSafeProps: Partial = { + blacklistedAssets: newBlacklistedAssets, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old balances list and a new balances list for the safe, should return true`, () => { + // given + const mockedActiveTokenAddress1 = '0x36591cd3DA96b21Ac9ca54cFaf80fe45107294F1' + const mockedActiveTokenAddress2 = '0x92aF97cbF10742dD2527ffaBA70e34C03CFFC2c1' + const oldBalances = Map({ + [mockedActiveTokenAddress1]: '100', + [mockedActiveTokenAddress2]: '10', + }) + const newBalances = Map({ + [mockedActiveTokenAddress1]: '100', + }) + const oldSafe = getMockedOldSafe({ balances: oldBalances }) + const newSafeProps: Partial = { + balances: newBalances, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old nonce and a new nonce for the safe, should return true`, () => { + // given + const oldNonce = 1 + const newNonce = 2 + const oldSafe = getMockedOldSafe({ nonce: oldNonce }) + const newSafeProps: Partial = { + nonce: newNonce, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old newLatestIncomingTxBlock and a new newLatestIncomingTxBlock for the safe, should return true`, () => { + // given + const oldLatestIncomingTxBlock = 1 + const newLatestIncomingTxBlock = 2 + const oldSafe = getMockedOldSafe({ latestIncomingTxBlock: oldLatestIncomingTxBlock }) + const newSafeProps: Partial = { + latestIncomingTxBlock: newLatestIncomingTxBlock, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old recurringUser and a new recurringUser for the safe, should return true`, () => { + // given + const oldRecurringUser = true + const newRecurringUser = false + const oldSafe = getMockedOldSafe({ recurringUser: oldRecurringUser }) + const newSafeProps: Partial = { + recurringUser: newRecurringUser, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old recurringUser and a new recurringUser for the safe, should return true`, () => { + // given + const oldCurrentVersion = '1.1.1' + const newCurrentVersion = '1.0.0' + const oldSafe = getMockedOldSafe({ currentVersion: oldCurrentVersion }) + const newSafeProps: Partial = { + currentVersion: newCurrentVersion, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old needsUpdate and a new needsUpdate for the safe, should return true`, () => { + // given + const oldNeedsUpdate = false + const newNeedsUpdate = true + const oldSafe = getMockedOldSafe({ needsUpdate: oldNeedsUpdate }) + const newSafeProps: Partial = { + needsUpdate: newNeedsUpdate, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) + it(`Given an old featuresEnabled and a new featuresEnabled for the safe, should return true`, () => { + // given + const oldFeaturesEnabled = [] + const newFeaturesEnabled = undefined + const oldSafe = getMockedOldSafe({ featuresEnabled: oldFeaturesEnabled }) + const newSafeProps: Partial = { + featuresEnabled: newFeaturesEnabled, + } + + // When + const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe) + + // Then + expect(expectedResult).toEqual(true) + }) +}) diff --git a/src/logic/safe/utils/shouldSafeStoreBeUpdated.ts b/src/logic/safe/utils/shouldSafeStoreBeUpdated.ts new file mode 100644 index 00000000..9247fd48 --- /dev/null +++ b/src/logic/safe/utils/shouldSafeStoreBeUpdated.ts @@ -0,0 +1,26 @@ +import { Map } from 'immutable' + +import { SafeRecordProps } from 'src/logic/safe/store/models/safe' + +// This function checks if an object is a Subset of a Safe State and that they have the same values +const isStateSubset = (superObj, subObj) => { + return Object.keys(subObj).every((key) => { + if (subObj[key] && typeof subObj[key] == 'object') { + if (Map.isMap(subObj[key]) || subObj[key].size >= 0) { + // If type is Immutable Map, List or Object we use Immutable equals + return superObj[key].equals(subObj[key]) + } + return isStateSubset(superObj[key], subObj[key]) + } + return subObj[key] === superObj[key] + }) +} + +export const shouldSafeStoreBeUpdated = ( + newSafeProps: Partial, + oldSafeProps?: SafeRecordProps, +): boolean => { + if (!oldSafeProps) return true + + return !isStateSubset(oldSafeProps, newSafeProps) +} diff --git a/src/routes/safe/components/Transactions/TxsTable/index.tsx b/src/routes/safe/components/Transactions/TxsTable/index.tsx index 9050674b..47dc03ea 100644 --- a/src/routes/safe/components/Transactions/TxsTable/index.tsx +++ b/src/routes/safe/components/Transactions/TxsTable/index.tsx @@ -3,7 +3,7 @@ import IconButton from '@material-ui/core/IconButton' import TableCell from '@material-ui/core/TableCell' import TableContainer from '@material-ui/core/TableContainer' import TableRow from '@material-ui/core/TableRow' -import { withStyles } from '@material-ui/core/styles' +import { makeStyles } from '@material-ui/core/styles' import ExpandLess from '@material-ui/icons/ExpandLess' import ExpandMore from '@material-ui/icons/ExpandMore' import cn from 'classnames' @@ -25,7 +25,10 @@ import { useAnalytics, SAFE_NAVIGATION_EVENT } from 'src/utils/googleAnalytics' export const TRANSACTION_ROW_TEST_ID = 'transaction-row' -const TxsTable = ({ classes }) => { +const useStyles = makeStyles(styles) + +const TxsTable = (): React.ReactElement => { + const classes = useStyles() const [expandedTx, setExpandedTx] = useState(null) const cancellationTransactions = useSelector(safeCancellationTransactionsSelector) const transactions = useSelector(extendedTransactionsSelector) @@ -95,10 +98,7 @@ const TxsTable = ({ classes }) => { {autoColumns.map((column) => ( { ) } -export default withStyles(styles as any)(TxsTable) +export default TxsTable diff --git a/src/routes/safe/components/Transactions/TxsTable/style.ts b/src/routes/safe/components/Transactions/TxsTable/style.ts index 815ab694..06c73255 100644 --- a/src/routes/safe/components/Transactions/TxsTable/style.ts +++ b/src/routes/safe/components/Transactions/TxsTable/style.ts @@ -1,4 +1,6 @@ -export const styles = () => ({ +import { createStyles } from '@material-ui/core' + +export const styles = createStyles({ container: { marginTop: '56px', },