(Fix) Owner replacement transaction details (#688)

* fix: No threshold change for owners replacement

* fix: Extract added owner from addressBook

When replacing an owner, the added owner's name was the same as the removed one

* fix: Add or Update addressBook entry for the newly added owner

* Replace `.then` with `async/await`

* Optimize AddressBook entry `name` update

* Revert "Optimize AddressBook entry `name` update"

This reverts commit 00a75d15

* refactor: AddressBook entry from plain JS object to immutable Record

* fix: merge instead of set for the AddressBookEntry record

* refactor: addOrUpdateAddressBookEntry redux action

changed signature to `addOrUpdateAddressBookEntry(entryAddress, entry)`
Where `entry` is an object with only the required fields to be updated

src/logic/addressBook/model/addressBook.js

@@ -1,5 +1,5 @@
 // @flow
-import type { RecordOf } from 'immutable'
+import { Record, type RecordFactory, type RecordOf } from 'immutable'
 
 export type AddressBookEntry = {
   address: string,
@@ -11,4 +11,10 @@ export type AddressBookProps = {
   addressBook: Map<string, AddressBookEntry>,
 }
 
+export const makeAddressBookEntry: RecordFactory<AddressBookEntry> = Record({
+  address: '',
+  name: '',
+  isOwner: false,
+})
+
 export type AddressBook = RecordOf<AddressBookProps>

src/logic/addressBook/store/actions/addOrUpdateAddressBookEntry.js

@@ -0,0 +1,14 @@
+// @flow
+import { createAction } from 'redux-actions'
+
+import type { AddressBookEntryType } from '~/logic/addressBook/model/addressBook'
+
+export const ADD_OR_UPDATE_ENTRY = 'ADD_OR_UPDATE_ENTRY'
+
+export const addOrUpdateAddressBookEntry = createAction<string, *, *>(
+  ADD_OR_UPDATE_ENTRY,
+  (entryAddress: string, entry: AddressBookEntryType): AddressBookEntryType => ({
+    entryAddress,
+    entry,
+  }),
+)

src/logic/addressBook/store/actions/saveAndUpdateAddressBook.js

@@ -2,13 +2,14 @@
 import type { Dispatch as ReduxDispatch } from 'redux'
 
 import type { AddressBook } from '~/logic/addressBook/model/addressBook'
+import { makeAddressBookEntry } from '~/logic/addressBook/model/addressBook'
 import { updateAddressBookEntry } from '~/logic/addressBook/store/actions/updateAddressBookEntry'
 import { saveAddressBook } from '~/logic/addressBook/utils'
 import { type GlobalState } from '~/store/index'
 
 const saveAndUpdateAddressBook = (addressBook: AddressBook) => async (dispatch: ReduxDispatch<GlobalState>) => {
   try {
-    dispatch(updateAddressBookEntry(addressBook))
+    dispatch(updateAddressBookEntry(makeAddressBookEntry(addressBook)))
     await saveAddressBook(addressBook)
   } catch (err) {
     // eslint-disable-next-line

src/logic/addressBook/store/middleware/addressBookMiddleware.js

@@ -3,6 +3,7 @@ import type { AnyAction, Store } from 'redux'
 
 import type { AddressBookProps } from '~/logic/addressBook/model/addressBook'
 import { ADD_ENTRY } from '~/logic/addressBook/store/actions/addAddressBookEntry'
+import { ADD_OR_UPDATE_ENTRY } from '~/logic/addressBook/store/actions/addOrUpdateAddressBookEntry'
 import { REMOVE_ENTRY } from '~/logic/addressBook/store/actions/removeAddressBookEntry'
 import { UPDATE_ENTRY } from '~/logic/addressBook/store/actions/updateAddressBookEntry'
 import { addressBookMapSelector } from '~/logic/addressBook/store/selectors'
@@ -10,9 +11,9 @@ import { saveAddressBook } from '~/logic/addressBook/utils'
 import { enhanceSnackbarForAction, getNotificationsFromTxType } from '~/logic/notifications'
 import enqueueSnackbar from '~/logic/notifications/store/actions/enqueueSnackbar'
 import { TX_NOTIFICATION_TYPES } from '~/logic/safe/transactions'
-import { type GlobalState } from '~/store/'
+import { type GlobalState } from '~/store'
 
-const watchedActions = [ADD_ENTRY, REMOVE_ENTRY, UPDATE_ENTRY]
+const watchedActions = [ADD_ENTRY, REMOVE_ENTRY, UPDATE_ENTRY, ADD_OR_UPDATE_ENTRY]
 
 const addressBookMiddleware = (store: Store<GlobalState>) => (next: Function) => async (action: AnyAction) => {
   const handledAction = next(action)

src/logic/addressBook/store/reducer/addressBook.js

@@ -3,8 +3,10 @@ import { List, Map } from 'immutable'
 import { type ActionType, handleActions } from 'redux-actions'
 
 import type { AddressBook, AddressBookEntry, AddressBookProps } from '~/logic/addressBook/model/addressBook'
+import { makeAddressBookEntry } from '~/logic/addressBook/model/addressBook'
 import { ADD_ADDRESS_BOOK } from '~/logic/addressBook/store/actions/addAddressBook'
 import { ADD_ENTRY } from '~/logic/addressBook/store/actions/addAddressBookEntry'
+import { ADD_OR_UPDATE_ENTRY } from '~/logic/addressBook/store/actions/addOrUpdateAddressBookEntry'
 import { LOAD_ADDRESS_BOOK } from '~/logic/addressBook/store/actions/loadAddressBook'
 import { REMOVE_ENTRY } from '~/logic/addressBook/store/actions/removeAddressBookEntry'
 import { UPDATE_ENTRY } from '~/logic/addressBook/store/actions/updateAddressBookEntry'
@@ -19,7 +21,8 @@ export const buildAddressBook = (storedAdbk: AddressBook): AddressBookProps => {
   let addressBookBuilt = Map([])
   Object.entries(storedAdbk).forEach((adbkProps: Array<string, AddressBookEntry[]>) => {
     const safeAddress = adbkProps[0]
-    const adbkSafeEntries = List(adbkProps[1])
+    const adbkRecords = adbkProps[1].map(makeAddressBookEntry)
+    const adbkSafeEntries = List(adbkRecords)
     addressBookBuilt = addressBookBuilt.set(safeAddress, adbkSafeEntries)
   })
   return addressBookBuilt
@@ -98,6 +101,28 @@ export default handleActions<State, *>(
       })
       return newState
     },
+    [ADD_OR_UPDATE_ENTRY]: (state: State, action: ActionType<Function>): State => {
+      const { entry, entryAddress } = action.payload
+
+      // Adds or Updates the entry to all the safes
+      return state.withMutations(map => {
+        const addressBook = map.get('addressBook')
+        if (addressBook) {
+          addressBook.keySeq().forEach(safeAddress => {
+            const safeAddressBook: List<AddressBookEntry> = state.getIn(['addressBook', safeAddress])
+            const entryIndex = safeAddressBook.findIndex(entryItem => sameAddress(entryItem.address, entryAddress))
+
+            if (entryIndex !== -1) {
+              const updatedEntriesList = safeAddressBook.update(entryIndex, currentEntry => currentEntry.merge(entry))
+              map.setIn(['addressBook', safeAddress], updatedEntriesList)
+            } else {
+              const updatedSafeAdbkList = safeAddressBook.push(makeAddressBookEntry(entry))
+              map.setIn(['addressBook', safeAddress], updatedSafeAdbkList)
+            }
+          })
+        }
+      })
+    },
   },
   Map(),
 )

src/routes/safe/components/AddressBook/index.jsx

@@ -21,6 +21,7 @@ import Col from '~/components/layout/Col'
 import Img from '~/components/layout/Img'
 import Row from '~/components/layout/Row'
 import type { AddressBookEntry } from '~/logic/addressBook/model/addressBook'
+import { makeAddressBookEntry } from '~/logic/addressBook/model/addressBook'
 import { addAddressBookEntry } from '~/logic/addressBook/store/actions/addAddressBookEntry'
 import { removeAddressBookEntry } from '~/logic/addressBook/store/actions/removeAddressBookEntry'
 import { updateAddressBookEntry } from '~/logic/addressBook/store/actions/updateAddressBookEntry'
@@ -89,13 +90,13 @@ const AddressBookTable = ({ classes }: Props) => {
 
   const newEntryModalHandler = (entry: AddressBookEntry) => {
     setEditCreateEntryModalOpen(false)
-    dispatch(addAddressBookEntry(entry))
+    dispatch(addAddressBookEntry(makeAddressBookEntry(entry)))
   }
 
   const editEntryModalHandler = (entry: AddressBookEntry) => {
     setSelectedEntry(null)
     setEditCreateEntryModalOpen(false)
-    dispatch(updateAddressBookEntry(entry))
+    dispatch(updateAddressBookEntry(makeAddressBookEntry(entry)))
   }
 
   const deleteEntryModalHandler = () => {

src/routes/safe/components/Settings/ManageOwners/EditOwnerModal/index.jsx

@@ -20,6 +20,7 @@ import Button from '~/components/layout/Button'
 import Hairline from '~/components/layout/Hairline'
 import Paragraph from '~/components/layout/Paragraph'
 import Row from '~/components/layout/Row'
+import { makeAddressBookEntry } from '~/logic/addressBook/model/addressBook'
 import { getNotificationsFromTxType, showSnackbar } from '~/logic/notifications'
 import { TX_NOTIFICATION_TYPES } from '~/logic/safe/transactions'
 import { sm } from '~/theme/variables'
@@ -55,7 +56,7 @@ const EditOwnerComponent = ({
   const handleSubmit = values => {
     const { ownerName } = values
     editSafeOwner({ safeAddress, ownerAddress, ownerName })
-    updateAddressBookEntry({ address: ownerAddress, name: ownerName })
+    updateAddressBookEntry(makeAddressBookEntry({ address: ownerAddress, name: ownerName }))
     const notification = getNotificationsFromTxType(TX_NOTIFICATION_TYPES.OWNER_NAME_CHANGE_TX)
     showSnackbar(notification.afterExecution.noMoreConfirmationsNeeded, enqueueSnackbar, closeSnackbar)
 

src/routes/safe/components/Settings/ManageOwners/OwnerAddressTableCell/index.jsx

@@ -10,7 +10,7 @@ type Props = {
   address: string,
   showLinks?: boolean,
   knownAddress?: boolean,
-  userName?: boolean,
+  userName?: string,
 }
 
 const OwnerAddressTableCell = (props: Props) => {

src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/index.jsx

@@ -3,11 +3,13 @@ import { withStyles } from '@material-ui/core/styles'
 import { List } from 'immutable'
 import { withSnackbar } from 'notistack'
 import React, { useEffect, useState } from 'react'
+import { useDispatch } from 'react-redux'
 
 import OwnerForm from './screens/OwnerForm'
 import ReviewReplaceOwner from './screens/Review'
 
 import Modal from '~/components/Modal'
+import { addOrUpdateAddressBookEntry } from '~/logic/addressBook/store/actions/addOrUpdateAddressBookEntry'
 import { SENTINEL_ADDRESS, getGnosisSafeInstanceAt } from '~/logic/contracts/safeContracts'
 import { TX_NOTIFICATION_TYPES } from '~/logic/safe/transactions'
 import { type Owner } from '~/routes/safe/store/models/owner'
@@ -94,6 +96,7 @@ const ReplaceOwner = ({
   safeName,
   threshold,
 }: Props) => {
+  const dispatch = useDispatch()
   const [activeScreen, setActiveScreen] = useState<ActiveScreen>('checkOwner')
   const [values, setValues] = useState<Object>({})
 
@@ -114,10 +117,11 @@ const ReplaceOwner = ({
     setActiveScreen('reviewReplaceOwner')
   }
 
-  const onReplaceOwner = () => {
+  const onReplaceOwner = async () => {
     onClose()
+
     try {
-      sendReplaceOwner(
+      await sendReplaceOwner(
         values,
         safeAddress,
         ownerAddress,
@@ -127,6 +131,13 @@ const ReplaceOwner = ({
         replaceSafeOwner,
         safe,
       )
+
+      dispatch(
+        // Needs the `address` field because we need to provide the minimum required values to ADD a new entry
+        // The reducer will update all the addressBooks stored, so we cannot decide what to do beforehand,
+        // thus, we pass the minimum required fields (name and address)
+        addOrUpdateAddressBookEntry(values.ownerAddress, { name: values.ownerName, address: values.ownerAddress }),
+      )
     } catch (error) {
       console.error('Error while removing an owner', error)
     }

src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/index.jsx

@@ -22,6 +22,7 @@ export const TRANSACTIONS_DESC_CHANGE_THRESHOLD_TEST_ID = 'tx-description-change
 export const TRANSACTIONS_DESC_SEND_TEST_ID = 'tx-description-send'
 export const TRANSACTIONS_DESC_CUSTOM_VALUE_TEST_ID = 'tx-description-custom-value'
 export const TRANSACTIONS_DESC_CUSTOM_DATA_TEST_ID = 'tx-description-custom-data'
+export const TRANSACTIONS_DESC_NO_DATA = 'tx-description-no-data'
 
 export const styles = () => ({
   txDataContainer: {
@@ -52,9 +53,10 @@ type TransferDescProps = {
 }
 
 type DescriptionDescProps = {
-  removedOwner?: string,
+  action: string,
   addedOwner?: string,
   newThreshold?: string,
+  removedOwner?: string,
 }
 
 type CustomDescProps = {
@@ -78,39 +80,81 @@ const TransferDescription = ({ amount = '', recipient }: TransferDescProps) => {
   )
 }
 
-const SettingsDescription = ({ addedOwner, newThreshold, removedOwner }: DescriptionDescProps) => {
-  const ownerChangedName = removedOwner ? getNameFromAddressBook(removedOwner) : getNameFromAddressBook(addedOwner)
+const RemovedOwner = ({ removedOwner }: { removedOwner: string }) => {
+  const ownerChangedName = getNameFromAddressBook(removedOwner)
+
   return (
-    <>
-      {removedOwner && (
-        <Block data-testid={TRANSACTIONS_DESC_REMOVE_OWNER_TEST_ID}>
-          <Bold>Remove owner:</Bold>
-          {ownerChangedName ? (
-            <OwnerAddressTableCell address={removedOwner} knownAddress showLinks userName={ownerChangedName} />
-          ) : (
-            <EtherscanLink knownAddress={false} type="address" value={removedOwner} />
-          )}
-        </Block>
+    <Block data-testid={TRANSACTIONS_DESC_REMOVE_OWNER_TEST_ID}>
+      <Bold>Remove owner:</Bold>
+      {ownerChangedName ? (
+        <OwnerAddressTableCell address={removedOwner} knownAddress showLinks userName={ownerChangedName} />
+      ) : (
+        <EtherscanLink knownAddress={false} type="address" value={removedOwner} />
       )}
-      {addedOwner && (
-        <Block data-testid={TRANSACTIONS_DESC_ADD_OWNER_TEST_ID}>
-          <Bold>Add owner:</Bold>
-          {ownerChangedName ? (
-            <OwnerAddressTableCell address={addedOwner} knownAddress showLinks userName={ownerChangedName} />
-          ) : (
-            <EtherscanLink knownAddress={false} type="address" value={addedOwner} />
-          )}
-        </Block>
+    </Block>
+  )
+}
+
+const AddedOwner = ({ addedOwner }: { addedOwner: string }) => {
+  const ownerChangedName = getNameFromAddressBook(addedOwner)
+
+  return (
+    <Block data-testid={TRANSACTIONS_DESC_ADD_OWNER_TEST_ID}>
+      <Bold>Add owner:</Bold>
+      {ownerChangedName ? (
+        <OwnerAddressTableCell address={addedOwner} knownAddress showLinks userName={ownerChangedName} />
+      ) : (
+        <EtherscanLink knownAddress={false} type="address" value={addedOwner} />
       )}
-      {newThreshold && (
-        <Block data-testid={TRANSACTIONS_DESC_CHANGE_THRESHOLD_TEST_ID}>
-          <Bold>Change required confirmations:</Bold>
-          <Paragraph noMargin size="md">
-            {newThreshold}
-          </Paragraph>
-        </Block>
-      )}
-    </>
+    </Block>
+  )
+}
+
+const NewThreshold = ({ newThreshold }: { newThreshold: string }) => (
+  <Block data-testid={TRANSACTIONS_DESC_CHANGE_THRESHOLD_TEST_ID}>
+    <Bold>Change required confirmations:</Bold>
+    <Paragraph noMargin size="md">
+      {newThreshold}
+    </Paragraph>
+  </Block>
+)
+
+const SettingsDescription = ({ action, addedOwner, newThreshold, removedOwner }: DescriptionDescProps) => {
+  if (action === 'removeOwner' && removedOwner && newThreshold) {
+    return (
+      <>
+        <RemovedOwner removedOwner={removedOwner} />
+        <NewThreshold newThreshold={newThreshold} />
+      </>
+    )
+  }
+
+  if (action === 'changedThreshold' && newThreshold) {
+    return <NewThreshold newThreshold={newThreshold} />
+  }
+
+  if (action === 'addOwnerWithThreshold' && addedOwner && newThreshold) {
+    return (
+      <>
+        <AddedOwner addedOwner={addedOwner} />
+        <NewThreshold newThreshold={newThreshold} />
+      </>
+    )
+  }
+
+  if (action === 'swapOwner' && removedOwner && addedOwner) {
+    return (
+      <>
+        <RemovedOwner removedOwner={removedOwner} />
+        <AddedOwner addedOwner={addedOwner} />
+      </>
+    )
+  }
+
+  return (
+    <Block data-testid={TRANSACTIONS_DESC_NO_DATA}>
+      <Bold>No data available for current transaction</Bold>
+    </Block>
   )
 }
 
@@ -165,6 +209,7 @@ const CustomDescription = ({ amount = 0, classes, data, recipient }: CustomDescP
 
 const TxDescription = ({ classes, tx }: Props) => {
   const {
+    action,
     addedOwner,
     cancellationTx,
     creationTx,
@@ -179,8 +224,13 @@ const TxDescription = ({ classes, tx }: Props) => {
   const amount = getTxAmount(tx)
   return (
     <Block className={classes.txDataContainer}>
-      {modifySettingsTx && (
-        <SettingsDescription addedOwner={addedOwner} newThreshold={newThreshold} removedOwner={removedOwner} />
+      {modifySettingsTx && action && (
+        <SettingsDescription
+          action={action}
+          addedOwner={addedOwner}
+          newThreshold={newThreshold}
+          removedOwner={removedOwner}
+        />
       )}
       {!upgradeTx && customTx && (
         <CustomDescription amount={amount} classes={classes} data={data} recipient={recipient} />

src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.js

@@ -3,6 +3,7 @@ import { getWeb3 } from '~/logic/wallets/getWeb3'
 import { type Transaction } from '~/routes/safe/store/models/transaction'
 
 type DecodedTxData = {
+  action?: string,
   recipient: string,
   value?: string,
   modifySettingsTx?: boolean,
@@ -48,21 +49,20 @@ export const getTxData = (tx: Transaction): DecodedTxData => {
     txData.modifySettingsTx = true
 
     if (tx.decodedParams) {
-      /* eslint-disable */
-      if (tx.decodedParams.methodName === 'removeOwner') {
+      txData.action = tx.decodedParams.methodName
+
+      if (txData.action === 'removeOwner') {
         txData.removedOwner = tx.decodedParams.args[1]
         txData.newThreshold = tx.decodedParams.args[2]
-      } else if (tx.decodedParams.methodName === 'changeThreshold') {
+      } else if (txData.action === 'changeThreshold') {
         txData.newThreshold = tx.decodedParams.args[0]
-      } else if (tx.decodedParams.methodName === 'addOwnerWithThreshold') {
+      } else if (txData.action === 'addOwnerWithThreshold') {
         txData.addedOwner = tx.decodedParams.args[0]
         txData.newThreshold = tx.decodedParams.args[1]
-      } else if (tx.decodedParams.methodName === 'swapOwner') {
-        txData.newThreshold = tx.decodedParams.args[0]
+      } else if (txData.action === 'swapOwner') {
         txData.removedOwner = tx.decodedParams.args[1]
         txData.addedOwner = tx.decodedParams.args[2]
       }
-      /* eslint-enable */
     }
   } else if (tx.cancellationTx) {
     txData.cancellationTx = true

src/routes/safe/store/middleware/safeStorage.js

@@ -2,6 +2,7 @@
 import { List } from 'immutable'
 import type { Action, Store } from 'redux'
 
+import { makeAddressBookEntry } from '~/logic/addressBook/model/addressBook'
 import { addAddressBookEntry } from '~/logic/addressBook/store/actions/addAddressBookEntry'
 import { saveDefaultSafe, saveSafes } from '~/logic/safe/utils'
 import type { Token } from '~/logic/tokens/store/model/token'
@@ -65,7 +66,7 @@ const safeStorageMware = (store: Store<GlobalState>) => (next: Function) => asyn
         const ownersArray = safe.owners.toJS()
         // Adds the owners to the address book
         ownersArray.forEach(owner => {
-          dispatch(addAddressBookEntry({ ...owner, isOwner: true }))
+          dispatch(addAddressBookEntry(makeAddressBookEntry({ ...owner, isOwner: true })))
         })
         break
       }