Filter the AddressBook sorted entries before displaying them #3014

DiogoSoaress · 2021-11-16T15:32:22Z

What it solves

Resolves #2965

How this PR fixes it

On searching for an existing AB entry we compare now the addresses checksum.
Before updating the Redux store we filter out AB entries with same address and chainId

How to test it

On any conditions, exporting and importing your address book should not generate any duplicated entries in the "Address Book"

Screenshots

…able

github-actions · 2021-11-16T15:32:40Z

CLA Assistant Lite All Contributors have signed the CLA.

DiogoSoaress · 2021-11-16T15:36:20Z

The bug seems to occur if the user had old data (don't know how old) in the Address Book and on exporting and importing it would create duplicated entries.

I was not able to reproduce it locally but open the PR to validate my solution and to share the deployments with the QA to see if it resolves the problem.

github-actions · 2021-11-16T15:36:38Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2021-11-16T15:40:26Z

Safe Rinkeby
Safe Mainnet
Safe Polygon
Safe BSC
Safe Arbitrum
Unified app on dev: https://safe-team.dev.gnosisdev.com/app/

coveralls · 2021-11-16T15:45:15Z

Pull Request Test Coverage Report for Build 1471733840

4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 31.682%

Totals
Change from base Build 1471506684:	0.02%
Covered Lines:	2989
Relevant Lines:	8394

💛 - Coveralls

github-actions · 2021-11-16T15:46:20Z

E2E Tests Passed ✅
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1471782332

katspaugh · 2021-11-16T15:52:52Z

IMO it's not worth it fixing it like this. We should fix the import.

DiogoSoaress · 2021-11-16T16:18:00Z

IMO it's not worth it fixing it like this. We should fix the import.

Fair enough. The import is working as expected, as it also overrides the duplicated entries https://github.com/gnosis/safe-react/blob/434fa9f45dd586403866cd3096b1280f387621ae/src/logic/addressBook/store/reducer/index.ts#L19-L30

It is very inefficient trying to fix this bug without actually being able to reproduce the faulty behavior.
Is there a more specific suggestion on how to fix the import? Maybe we could pair or should other dev (that can mimic the bug) pick it?

katspaugh · 2021-11-16T16:20:03Z

@liliya-soroka can you share the content of your LS with the dupes?

iamacook · 2021-11-16T16:20:11Z

Does @liliya-soroka's csv file have duplicates in it, or is it a purely visual based bug?

DiogoSoaress · 2021-11-16T16:30:01Z

Does @liliya-soroka's csv file have duplicates in it, or is it a purely visual based bug?

The .csv we used to reproduce it had no duplicates.

src/logic/addressBook/utils/index.ts

katspaugh · 2021-11-17T12:11:17Z

src/logic/addressBook/utils/index.ts

+    try {
+      hasSameChecksumAddress = checksumAddress(address) === checksumAddress(addressBookEntry.address)
+    } catch (e) {
+      console.error(e)


We don't use console.error. I would just return false here.

DiogoSoaress · 2021-11-17T12:17:33Z

I was able to reproduce the bug by making getEntryIndex return false
https://github.com/gnosis/safe-react/blob/1566910b222b49a65e49502fac84d565e1f4ba5f/src/logic/addressBook/utils/index.ts#L60-L72

Used that data with duplicates to implement a filter before we return the reducer state so old duplicates are filtered out.

iamacook

Looks good to me!

github-actions · 2021-11-17T13:23:33Z

Deployment links

🟠 Safe Rinkeby	⚪ Safe Mainnet	🟣 Safe Polygon	🟡 Safe BSC	⚫ Safe Arbitrum	🟢 Safe xDai

francovenica · 2021-11-17T22:58:18Z

So I didn't have issues by importing csv with repeated entries, but I also didn't had issues back in prod when this issue was reported.
I can say that the importing is fine, and when you export it seems that all the repeated entries are removed (I import 110 entries in my file and you export 93 cuz the duplicated ones were not added to the LS), so that's good

@liliya-soroka You describe a way to reproduce it by using the mobile app. I'd like to see if that can be tried in this PR and verify if this works.

katspaugh · 2021-11-18T08:13:14Z

@francovenica you could test by lowercasing all the addresses in your CSV.

francovenica · 2021-11-19T01:37:04Z

Ok, tried by having a csv with the same entry 3 times, one checksummed, and the other 2 with upper and lower case characters.
After importing the address book still shows only 1 entry.

Looks good to me

Filter the AddressBook sorted entries before displaying them in the t…

23634ac

…able

DiogoSoaress requested a review from katspaugh November 16, 2021 18:31

Compare the imported and stored addresses by their checksums

6c53631

DiogoSoaress force-pushed the fix-duplicated-addresses-import branch from f10ff8e to 6c53631 Compare November 16, 2021 18:33

iamacook reviewed Nov 17, 2021

View reviewed changes

src/logic/addressBook/utils/index.ts Outdated Show resolved Hide resolved

katspaugh force-pushed the main branch from 4f2e52f to b9be5e9 Compare November 17, 2021 08:38

DiogoSoaress marked this pull request as draft November 17, 2021 10:25

Diogo Soares added 2 commits November 17, 2021 13:00

Fix: catch checksumAddress throw

93c7062

Filter out duplicates in the store

1566910

DiogoSoaress marked this pull request as ready for review November 17, 2021 12:09

DiogoSoaress requested a review from iamacook November 17, 2021 12:09

katspaugh reviewed Nov 17, 2021

View reviewed changes

Don't use console.error

9f0e5da

DiogoSoaress requested a review from katspaugh November 17, 2021 12:19

iamacook approved these changes Nov 17, 2021

View reviewed changes

katspaugh approved these changes Nov 17, 2021

View reviewed changes

DiogoSoaress changed the base branch from main to dev November 17, 2021 12:42

Use sameAddress for addresses comparision

3fc53cc

DiogoSoaress merged commit 74308d0 into dev Nov 22, 2021

DiogoSoaress deleted the fix-duplicated-addresses-import branch November 22, 2021 12:02

github-actions bot locked and limited conversation to collaborators Nov 22, 2021

Filter the AddressBook sorted entries before displaying them #3014

Filter the AddressBook sorted entries before displaying them #3014

Uh oh!

Conversation

DiogoSoaress commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it solves

How this PR fixes it

How to test it

Screenshots

Uh oh!

github-actions bot commented Nov 16, 2021

Uh oh!

DiogoSoaress commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

github-actions bot commented Nov 16, 2021

Uh oh!

coveralls commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1471733840

💛 - Coveralls

Uh oh!

github-actions bot commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katspaugh commented Nov 16, 2021

Uh oh!

DiogoSoaress commented Nov 16, 2021

Uh oh!

katspaugh commented Nov 16, 2021

Uh oh!

iamacook commented Nov 16, 2021

Uh oh!

DiogoSoaress commented Nov 16, 2021

Uh oh!

Uh oh!

katspaugh Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

DiogoSoaress commented Nov 17, 2021

Uh oh!

iamacook left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 17, 2021

Deployment links

Uh oh!

francovenica commented Nov 17, 2021

Uh oh!

katspaugh commented Nov 18, 2021

Uh oh!

francovenica commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DiogoSoaress commented Nov 16, 2021 •

edited

Loading

DiogoSoaress commented Nov 16, 2021 •

edited

Loading

github-actions bot commented Nov 16, 2021 •

edited

Loading

coveralls commented Nov 16, 2021 •

edited

Loading

github-actions bot commented Nov 16, 2021 •

edited

Loading