From f1235738c0184f2ed0d2678075521f4d50977eff Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 31 Mar 2020 08:20:17 -0300 Subject: [PATCH] (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 | 8 +- .../actions/addOrUpdateAddressBookEntry.js | 14 +++ .../store/actions/saveAndUpdateAddressBook.js | 3 +- .../store/middleware/addressBookMiddleware.js | 5 +- .../addressBook/store/reducer/addressBook.js | 27 +++- .../safe/components/AddressBook/index.jsx | 5 +- .../ManageOwners/EditOwnerModal/index.jsx | 3 +- .../OwnerAddressTableCell/index.jsx | 2 +- .../ManageOwners/ReplaceOwnerModal/index.jsx | 15 ++- .../ExpandedTx/TxDescription/index.jsx | 116 +++++++++++++----- .../ExpandedTx/TxDescription/utils.js | 14 +-- .../safe/store/middleware/safeStorage.js | 3 +- 12 files changed, 163 insertions(+), 52 deletions(-) create mode 100644 src/logic/addressBook/store/actions/addOrUpdateAddressBookEntry.js diff --git a/src/logic/addressBook/model/addressBook.js b/src/logic/addressBook/model/addressBook.js index ce769fe1..9cf1bd0e 100644 --- a/src/logic/addressBook/model/addressBook.js +++ b/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, } +export const makeAddressBookEntry: RecordFactory = Record({ + address: '', + name: '', + isOwner: false, +}) + export type AddressBook = RecordOf diff --git a/src/logic/addressBook/store/actions/addOrUpdateAddressBookEntry.js b/src/logic/addressBook/store/actions/addOrUpdateAddressBookEntry.js new file mode 100644 index 00000000..17519cf1 --- /dev/null +++ b/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( + ADD_OR_UPDATE_ENTRY, + (entryAddress: string, entry: AddressBookEntryType): AddressBookEntryType => ({ + entryAddress, + entry, + }), +) diff --git a/src/logic/addressBook/store/actions/saveAndUpdateAddressBook.js b/src/logic/addressBook/store/actions/saveAndUpdateAddressBook.js index 48f0190e..f4063e8a 100644 --- a/src/logic/addressBook/store/actions/saveAndUpdateAddressBook.js +++ b/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) => { try { - dispatch(updateAddressBookEntry(addressBook)) + dispatch(updateAddressBookEntry(makeAddressBookEntry(addressBook))) await saveAddressBook(addressBook) } catch (err) { // eslint-disable-next-line diff --git a/src/logic/addressBook/store/middleware/addressBookMiddleware.js b/src/logic/addressBook/store/middleware/addressBookMiddleware.js index 2cdfdf2f..3482d98e 100644 --- a/src/logic/addressBook/store/middleware/addressBookMiddleware.js +++ b/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) => (next: Function) => async (action: AnyAction) => { const handledAction = next(action) diff --git a/src/logic/addressBook/store/reducer/addressBook.js b/src/logic/addressBook/store/reducer/addressBook.js index 449c74bf..23a5d68f 100644 --- a/src/logic/addressBook/store/reducer/addressBook.js +++ b/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) => { 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( }) return newState }, + [ADD_OR_UPDATE_ENTRY]: (state: State, action: ActionType): 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 = 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(), ) diff --git a/src/routes/safe/components/AddressBook/index.jsx b/src/routes/safe/components/AddressBook/index.jsx index d3176882..87992c0a 100644 --- a/src/routes/safe/components/AddressBook/index.jsx +++ b/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 = () => { diff --git a/src/routes/safe/components/Settings/ManageOwners/EditOwnerModal/index.jsx b/src/routes/safe/components/Settings/ManageOwners/EditOwnerModal/index.jsx index d5746dff..e85ddca4 100644 --- a/src/routes/safe/components/Settings/ManageOwners/EditOwnerModal/index.jsx +++ b/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) diff --git a/src/routes/safe/components/Settings/ManageOwners/OwnerAddressTableCell/index.jsx b/src/routes/safe/components/Settings/ManageOwners/OwnerAddressTableCell/index.jsx index facfaf8c..e1ac6466 100644 --- a/src/routes/safe/components/Settings/ManageOwners/OwnerAddressTableCell/index.jsx +++ b/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) => { diff --git a/src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/index.jsx b/src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/index.jsx index a7e34cda..a814319e 100644 --- a/src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/index.jsx +++ b/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('checkOwner') const [values, setValues] = useState({}) @@ -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) } diff --git a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/index.jsx b/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/index.jsx index 195f54f4..8940eff4 100644 --- a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/index.jsx +++ b/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 && ( - - Remove owner: - {ownerChangedName ? ( - - ) : ( - - )} - + + Remove owner: + {ownerChangedName ? ( + + ) : ( + )} - {addedOwner && ( - - Add owner: - {ownerChangedName ? ( - - ) : ( - - )} - + + ) +} + +const AddedOwner = ({ addedOwner }: { addedOwner: string }) => { + const ownerChangedName = getNameFromAddressBook(addedOwner) + + return ( + + Add owner: + {ownerChangedName ? ( + + ) : ( + )} - {newThreshold && ( - - Change required confirmations: - - {newThreshold} - - - )} - + + ) +} + +const NewThreshold = ({ newThreshold }: { newThreshold: string }) => ( + + Change required confirmations: + + {newThreshold} + + +) + +const SettingsDescription = ({ action, addedOwner, newThreshold, removedOwner }: DescriptionDescProps) => { + if (action === 'removeOwner' && removedOwner && newThreshold) { + return ( + <> + + + + ) + } + + if (action === 'changedThreshold' && newThreshold) { + return + } + + if (action === 'addOwnerWithThreshold' && addedOwner && newThreshold) { + return ( + <> + + + + ) + } + + if (action === 'swapOwner' && removedOwner && addedOwner) { + return ( + <> + + + + ) + } + + return ( + + No data available for current transaction + ) } @@ -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 ( - {modifySettingsTx && ( - + {modifySettingsTx && action && ( + )} {!upgradeTx && customTx && ( diff --git a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.js b/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.js index fc3403c1..7b0bb75a 100644 --- a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.js +++ b/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 diff --git a/src/routes/safe/store/middleware/safeStorage.js b/src/routes/safe/store/middleware/safeStorage.js index aec23055..fbee8b5c 100644 --- a/src/routes/safe/store/middleware/safeStorage.js +++ b/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) => (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 }