Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash upon removing contact #9031

Merged
merged 1 commit into from
Jul 20, 2020
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jul 17, 2020

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

@Gudahtt
Copy link
Member Author

Gudahtt commented Jul 17, 2020

This depends upon #9030

@metamaskbot
Copy link
Collaborator

Builds ready [fa426b5]
Page Load Metrics (592 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298942189
domContentLoaded3426615908440
load3436635928440
domInteractive3416615908440

Base automatically changed from hide-contact-delete-button-for-wallet-accounts to develop July 17, 2020 23:30
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
@Gudahtt Gudahtt force-pushed the fix-crash-upon-removing-contact branch from fa426b5 to 31fcfc7 Compare July 17, 2020 23:31
@Gudahtt Gudahtt marked this pull request as ready for review July 17, 2020 23:31
@Gudahtt Gudahtt requested a review from a team as a code owner July 17, 2020 23:31
@metamaskbot
Copy link
Collaborator

Builds ready [31fcfc7]
Page Load Metrics (588 ± 9 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint288139168
domContentLoaded564642586189
load566644588189
domInteractive564642586189

@Gudahtt
Copy link
Member Author

Gudahtt commented Jul 20, 2020

I had originally written a migration to remove any indentities entries that were added for contacts, but there was no need. They get wiped out when the extension gets unlocked after a reload (e.g. after an update), because of the syncAddresses call made during unlock.

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Gudahtt Gudahtt merged commit 7cd609b into develop Jul 20, 2020
@Gudahtt Gudahtt deleted the fix-crash-upon-removing-contact branch July 20, 2020 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error when removing an address book entry
3 participants