(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 <daniel.sanchez@gnosis.pm>
2020-12-15 18:23:12 -03:00 · 2020-12-15 18:23:12 -03:00 · 7c19205e01
parent 71bdd43a80
commit 7c19205e01
6 changed files with 435 additions and 31 deletions
--- 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) => {
--- 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
--- a/src/logic/safe/utils/tests/shouldSafeStoreBeUpdated.test.ts
+++ 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>): 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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      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<SafeRecordProps> = {
+      featuresEnabled: newFeaturesEnabled,
+    }
+
+    // When
+    const expectedResult = shouldSafeStoreBeUpdated(newSafeProps, oldSafe)
+
+    // Then
+    expect(expectedResult).toEqual(true)
+  })
+})
--- a/src/logic/safe/utils/shouldSafeStoreBeUpdated.ts
+++ 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<SafeRecordProps>,
+  oldSafeProps?: SafeRecordProps,
+): boolean => {
+  if (!oldSafeProps) return true
+
+  return !isStateSubset(oldSafeProps, newSafeProps)
+}
--- 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) => (
                      <TableCell
                        align={column.align}
-                        className={cn(
-                          classes.cell,
-                          ['cancelled', 'failed'].includes(row.status) && classes.cancelledRow,
-                        )}
+                        className={cn(['cancelled', 'failed'].includes(row.status) && classes.cancelledRow)}
                        component="td"
                        key={column.id}
                        style={cellWidth(column.width)}
@ -139,4 +139,4 @@ const TxsTable = ({ classes }) => {
  )
 }

-export default withStyles(styles as any)(TxsTable)
+export default TxsTable
--- 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',
  },