Skip to content

Commit

Permalink
Fix crash upon removing contact (#9031)
Browse files Browse the repository at this point in the history
The UI would crash upon deleting a contact from the contact list. This
happened for two reasons: the deletion could result in a re-render
before the `history.push` finished navigating back to the contact list
(it was a race condition), and the contact entry left behind an invalid
`identities` entry when it was removed.

The first problem was fixed by making the container components for view
and edit contact more tolerant of being passed an `address` that
doesn't correspond to a contact. If they are given an address without a
contact, `null` is passed to the component via the `address` prop. The
component will redirect back to the list when this happens instead
rendering. This is more awkward than I'd like, but it was the most
sensible way of handling this I could think of without making much more
drastic changes to how we're handling routing here.

The second problem was caused by the `setAccountLabel` call, which was
used to ensure the contact entry for any wallet accounts was kept
in-sync with the account label. This was being called even for non-
wallet accounts though, which is where this problem arose. This step is
now skipped for non-wallet accounts.

Fixes #9019
  • Loading branch information
Gudahtt authored Jul 20, 2020
1 parent 50c4db7 commit 7cd609b
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export default function RecipientGroup ({ label, items, onSelect, selectedAddres
RecipientGroup.propTypes = {
label: PropTypes.string,
items: PropTypes.arrayOf(PropTypes.shape({
address: PropTypes.string,
address: PropTypes.string.isRequired,
name: PropTypes.string,
})),
onSelect: PropTypes.func.isRequired,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { PureComponent } from 'react'
import PropTypes from 'prop-types'
import { Redirect } from 'react-router-dom'
import Identicon from '../../../../components/ui/identicon'
import Button from '../../../../components/ui/button/button.component'
import TextField from '../../../../components/ui/text-field'
Expand Down Expand Up @@ -28,7 +29,6 @@ export default class EditContact extends PureComponent {

static defaultProps = {
name: '',
address: '',
memo: '',
}

Expand All @@ -55,6 +55,10 @@ export default class EditContact extends PureComponent {
viewRoute,
} = this.props

if (!address) {
return <Redirect to={{ pathname: listRoute }} />
}

return (
<div className="settings-page__content-row address-book__edit-contact">
<div className="settings-page__header address-book__header--edit">
Expand All @@ -68,7 +72,6 @@ export default class EditContact extends PureComponent {
className="settings-page__address-book-button"
onClick={async () => {
await removeFromAddressBook(chainId, address)
history.push(listRoute)
}}
>
{t('deleteAccount')}
Expand Down Expand Up @@ -136,15 +139,19 @@ export default class EditContact extends PureComponent {
if (isValidAddress(this.state.newAddress)) {
await removeFromAddressBook(chainId, address)
await addToAddressBook(this.state.newAddress, this.state.newName || name, this.state.newMemo || memo)
setAccountLabel(this.state.newAddress, this.state.newName || name)
if (showingMyAccounts) {
setAccountLabel(this.state.newAddress, this.state.newName || name)
}
history.push(listRoute)
} else {
this.setState({ error: this.context.t('invalidAddress') })
}
} else {
// update name
await addToAddressBook(address, this.state.newName || name, this.state.newMemo || memo)
setAccountLabel(address, this.state.newName || name)
if (showingMyAccounts) {
setAccountLabel(address, this.state.newName || name)
}
history.push(listRoute)
}
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@ const mapStateToProps = (state, ownProps) => {
const pathNameTailIsAddress = pathNameTail.includes('0x')
const address = pathNameTailIsAddress ? pathNameTail.toLowerCase() : ownProps.match.params.id

const { memo, name } = getAddressBookEntry(state, address) || state.metamask.identities[address]
const contact = getAddressBookEntry(state, address) || state.metamask.identities[address]
const { memo, name } = contact || {}

const chainId = state.metamask.network

const showingMyAccounts = Boolean(pathname.match(CONTACT_MY_ACCOUNTS_EDIT_ROUTE))

return {
address,
address: contact ? address : null,
chainId,
name,
memo,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import React, { PureComponent } from 'react'
import PropTypes from 'prop-types'
import { Redirect } from 'react-router-dom'

import Identicon from '../../../../components/ui/identicon'
import Copy from '../../../../components/ui/icon/copy-icon.component'

import Button from '../../../../components/ui/button/button.component'
import copyToClipboard from 'copy-to-clipboard'

Expand All @@ -23,11 +24,16 @@ export default class ViewContact extends PureComponent {
checkSummedAddress: PropTypes.string,
memo: PropTypes.string,
editRoute: PropTypes.string,
listRoute: PropTypes.string.isRequired,
}

render () {
const { t } = this.context
const { history, name, address, checkSummedAddress, memo, editRoute } = this.props
const { history, name, address, checkSummedAddress, memo, editRoute, listRoute } = this.props

if (!address) {
return <Redirect to={{ pathname: listRoute }} />
}

return (
<div className="settings-page__content-row">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import { getAddressBookEntry } from '../../../../selectors'
import { checksumAddress } from '../../../../helpers/utils/util'
import {
CONTACT_EDIT_ROUTE,
CONTACT_LIST_ROUTE,
CONTACT_MY_ACCOUNTS_EDIT_ROUTE,
CONTACT_MY_ACCOUNTS_ROUTE,
CONTACT_MY_ACCOUNTS_VIEW_ROUTE,
} from '../../../../helpers/constants/routes'

Expand All @@ -17,16 +19,18 @@ const mapStateToProps = (state, ownProps) => {
const pathNameTailIsAddress = pathNameTail.includes('0x')
const address = pathNameTailIsAddress ? pathNameTail.toLowerCase() : ownProps.match.params.id

const { memo, name } = getAddressBookEntry(state, address) || state.metamask.identities[address]
const contact = getAddressBookEntry(state, address) || state.metamask.identities[address]
const { memo, name } = contact || {}

const showingMyAccounts = Boolean(pathname.match(CONTACT_MY_ACCOUNTS_VIEW_ROUTE))

return {
name,
address,
address: contact ? address : null,
checkSummedAddress: checksumAddress(address),
memo,
editRoute: showingMyAccounts ? CONTACT_MY_ACCOUNTS_EDIT_ROUTE : CONTACT_EDIT_ROUTE,
listRoute: showingMyAccounts ? CONTACT_MY_ACCOUNTS_ROUTE : CONTACT_LIST_ROUTE,
}
}

Expand Down

0 comments on commit 7cd609b

Please sign in to comment.