Feat: blacklist whitelist contacts #1814

bassemmagdy · 2021-12-09T17:28:09Z

Added feature blacklist/whitelist contacts
Separated contact list to blacklisted and whitelisted
By default now, transactions received from blacklisted contacts don't appear in transaction history

Dev -> Main

src/services/TransactionFilterService.ts

src/components/ContactDetailPanel/ContactDetailPanelTs.ts

src/core/utils/CommonHelpers.ts

src/components/TransactionList/TransactionListFilters/TransactionListFiltersTs.ts

bassemmagdy · 2021-12-16T15:25:28Z

@Jaguar0625 All comments are addressed, please recheck :)

Jaguar0625 · 2021-12-17T13:42:27Z

__tests__/utils/CommonHelpers.spec.ts

+            const address = Address.createFromRawAddress('TAD5BAHLOIXCRRB6GU2H72HPXMBBVAEUQRYPHBY');
+            const truncatedAddress = CommonHelpers.truncate(address.plain());
+            const string = 'string';
+            expect(truncatedAddress.length).toBe(15);
+            expect(truncatedAddress.substring(truncatedAddress.length - 6)).toEqual('...HBY');
+            expect(CommonHelpers.truncate(string).length).toBe(6);


it's probably better to rename string to something more descriptive like shortString

in future, generally would try to group Arrange / Act / Assert [see https://hackmd.io/WrKdAaohRXKgCbzd_4ZMxA]

Jaguar0625 · 2021-12-17T13:43:39Z

__tests__/services/TransactionFilterService.spec.ts

@@ -17,6 +17,8 @@ import { TransactionFilterOptionsState, TransactionState } from '@/store/Transac
 import { TransactionFilterService } from '@/services/TransactionFilterService';
 import { getTestAccount } from '@MOCKS/Accounts';
 import { TransferTransaction } from 'symbol-sdk';
+import { addressBookMock } from '@MOCKS/AddressBookMock';
+import { IContact } from 'symbol-address-book';

 const currentSigner = getTestAccount('remoteTestnet');
 const recepient = getTestAccount('remoteTestnetRecipient');


not something you need to fix as part of this PR, but it's a bit confusing to use currentSigner as recipient address

recepient is spelled wrong

Jaguar0625 · 2021-12-17T13:49:57Z

__tests__/components/TransactionList/TransactionListFilters/TransactionListFilter.spec.ts

    test("should not call the 'account/SET_CURRENT_SIGNER' without address", () => {
        vm.onSignerSelectorChange();
        expect(vm.$store.dispatch).not.toBeCalled();
    });
+
+    test("should call 'transaction/filterTransactions' with blacklisted contacts", () => {


would be good to add a test when isBlackListFilterActivated == true to cover else branch too

bassemmagdy · 2021-12-23T16:07:46Z

@Jaguar0625 All feedbacks should be now addressed :)

…feat-blacklist-whitelist-contacts

cryptoBeliever · 2021-12-24T10:27:38Z

Pasting here as a reference. When I'm doing on branch:
npm install
npm run dev

I got error like this (app is starting):

cryptoBeliever · 2021-12-27T09:46:13Z

@bassemmagdy @coiki I think the address book/blocked transactions mechanic and UX/UI has to be re-think. It's not intuitive for me.

I'm able to add mainnet addresses to testnet profile which doesn't make sense I guess. Same with testnet addresses in mainnet profile.
Have we graphics to address the book? Currently white and blacklist sublists are aligned to the left which looks strange. Also empty

Option to import an address book is only available if you have an empty address book (white and black lists). How user should import more addresses to black/white lists? Should be possible?
On the transaction list "Show blacklisted transactions" button has no indicator that it's active. It's very misleading for users. Maybe besides the "active" filtering indicator, we should add some iconographic in the transaction row?

How filtering should work? It should show valid + blacklisted transactions or should show only blacklisted? Currently, it's the second one. Could you tell me how filtering currently works for aggregate transactions where signed and initiator can be different? Which field is used to filter?

https://share.getcloudapp.com/7Kuqg54L

First I have to add to whitelist addresses and next move it to blacklisted. There is no way to add it directly into blacklisted.

https://share.getcloudapp.com/5zunp6Z6

bassemmagdy · 2022-01-03T23:24:45Z

@cryptoBeliever
1- added checker to validate network type before adding new contact.
2- you can find the design here -> https://www.figma.com/file/lkhDQWxq5FlUrMNZYIJ2wT/Symbol-Wallet---Legacy?node-id=168%3A14032
3- Currently importing is overwriting the current list that's why it's only available when the list is empty. We can improve it by letting it add the new ones to the list but, in case the address is found in the list all contact data will be overwritten except the address.
4- I will try to add a shadow to show the difference based on state (preferably we get a design for that.)
5- Mockups didn't include that option in the contact creation form.

Jaguar0625 · 2022-01-04T13:33:24Z

@cryptoBeliever - do you think the new approach is intuitive enough or do you have more suggestions for improvements?

postoronnii · 2022-01-04T17:53:41Z

@cryptoBeliever how to delete address from the blacklist? Do we have this functionality?

cryptoBeliever · 2022-01-04T19:31:50Z

@cryptoBeliever how to delete address from the blacklist? Do we have this functionality?

@NikolaiB by using "Delete Contact" in the address book. Same as for "White List" contacts.

@bassemmagdy

It's fixed.
It doesn't look as on Figma Graphics.

a) Figma:

And wallet:

whitelist and blacklist header: missing grey horizontal line, header text size is same as for "My contacts" but should be way smaller
contact item: here also text seems too big, icon is too small comparing text; icon has also other borders color than on Figma; if name is long enough, it's cut (but there is still plenty of space)

I think we should allow import and overwrite. @coiki what do you think?
As I can see on Figma there is a graphic for the active and inactive state:

Adding to white/blacklist. I think it requires changes. My proposition is to add a select box when adding a new contact with two options blacklist and whitelist. By default could be selected whitelist if we clicked "Add Contact" when whitelist is open or blacklist if we clicked "Add contact" when blacklist is open (but user could change that). Asking user to first add into whitelist and move to blacklist doesn't look good for me. @coiki what do you think?
@coiki Signing partial transactions:

At this moment blacklisting change only visibility on the transaction list. But maybe we should present some warning on transaction details (if the user decides to open a transaction to sign anyway or by mistake). My proposition is to not allow sign partial transactions if the user already added an address to blacklist.

On the list we have now a button to show blacklisted transactions. When we activate it we can see only "blacklisted" transactions. For me, more intuitive would be present in such a situation whitelisted + blacklisted transactions (button would hide/show only blacklisted transactions) - whitelisted always visible. @coiki what do you think it would be better?
Blacklisting transaction rules. @bassemmagdy could you write what rules are for blacklisting transactions? Transaction is blacklisted when transaction signer is on the blacklist?
@bassemmagdy Figma (https://www.figma.com/file/lkhDQWxq5FlUrMNZYIJ2wT/Symbol-Wallet---Legacy?node-id=168%3A14032) contains changed flow for signing partial transactions and saving addresses (black and whitelisting) to address book which weren't implemented. I think it should be part of address book implementation.

View in wallet:

Screens on Figma:

Transaction details when opened (I think for whitelisted addresses we could move to Caution warning and skip this step):

When the user reject there is an option to blacklist contact:

There is also an option to "Archive". @coiki what it does? It should hide transactions? It should be implemented here or there will be a separate issue for the feature (as I understand it's an option to hide transactions without blacklisting address)?

The user can also "Accept..." and on the next screen, he can see Caution, and on the next screen provides a password to accept. At the end of process, there should be a way to add contact into "Whitelist".

Also currently warning is not presented if account is in relation with user multisig (to don't show a lot of warnings). We can communicate around that what flow in case of multisig relation of this account should be.

coiki · 2022-01-06T07:13:08Z

@cryptoBeliever I checked your questions and I’ve tried to sum up my answers. We already spoke on the chat that we need to create a new task for this, or reuse the one that already touched this subject (and delete it if not).

Understood. I agree it doesn’t make sense.
The alignment should be to the left. The design is quite far from what was in the design from Biagio.
Users will add contacts when interacting with transactions, but should always be able to add directly. If I understand correctly we have only 1 address book per whitelist/blacklist? We should have the option to always import an address book and it should just append it to my existing one.

Regarding showing the blacklisted transactions alongside the whitelisted, we want to treat it as a sort of ‘spam folder’ where all the toxic txs live. I think there are use cases where it would be helpful having them alongisde. Let’s see what we can do with this idea. I would also prefer view/hide, but we wanted to manage it like this due to the malicious attacks.

Regarding the new ‘signing area’ of the transactions:
I agree with your point to not allow to sign transactions from blacklisted addresses. Let’s add it to the logic of the new task about the signing area.

Q: What is Archive? A: It just hides that view (so the user only rejects but doesn’t blacklist). Let’s change the text to “Close”.

cryptoBeliever · 2022-01-13T19:30:05Z

@bassemmagdy I reported a few issues:

I think those should be fixed in this PR:
#1823
#1824
#1825

Those could be done in separate PR:
#1826
#1827

Also depends on @coiki answers for questions in https://www.figma.com/file/lkhDQWxq5FlUrMNZYIJ2wT/Symbol-Wallet---Legacy?node-id=168%3A14032 I may report some further issues because currently there is no difference in flow between the situation when accounts is whitelisted/unknown/blacklisted.

lgtm-com · 2022-01-14T19:25:41Z

This pull request introduces 1 alert when merging ad0bde0 into 8b236aa - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

…ted contacts

lgtm-com · 2022-01-18T21:56:17Z

This pull request introduces 1 alert when merging b1e3544 into 8b236aa - view on LGTM.com

new alerts:

1 for Useless conditional

sonarcloud · 2022-01-19T02:28:54Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
2 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

* fix: address book list styles. feat: add option to blacklist new contact * fix: rewrite ModalTransactionCosignature warning section, fix flow, fix checkbox styles, add escape option * feat: add blocked contact transaction view, add unblock option. fix: 'jumping' form * fix: display contact name in the tx signer field * task: replace pretty addresses with a plain ones * feat: add signer address to tx details * feat: make address filed redirectable to explorer page * feat: add address popover with explorer url * task: require blacklist contact name * fix: aggregate txs get random names from the address book, fix tx filter * fix: styles of the new address detected modal * fix: detected address input styles, new transaction incorrect address * fix: the accounts selector jumping action buttons * fix: contact address edition doesn't work * fix: rename blacklist => block and lint * fix: missing property error * fix: the contact list jumping action buttons * fix: rename show blacklisted transactions => show blocked transactions * fix: edit contact with the same address * feat: improve cosignature warning, show unknown signer address, add explorer link, add caution text * fix: account balance widget address label size * task: change address row explorer link popover layout and styles * fix: remove unused commented style props * fix: presentation for multilevel multisig tree * fix: show warning for blocked multisig address * fix: remove unnecessary tx detail scroll to sign form * task: change copy requires => awaiting * task: add ModalTransactionCosignature component test * task: update translations * fix: translation * task: cleanup test * task: update copy * fix: the transaction list action icons position * fix: remove blacklisted contacts from the send form * feat: add tad for blocked addresses, fix modal aligment, remove blacklist contact name input * fix: lint * task: update translations, add locale unit test * feat: add the scam alert link * fix: typo

gimre-xymcity · 2022-05-12T16:42:48Z

__tests__/database/storage/ProfileModelStorage.spec.ts

@@ -1,5 +1,5 @@
 /*
- * (C) Symbol Contributors 2021 Foundation


Merge pull request #1797 from symbol/dev

c9afb54

Dev -> Main

bassemmagdy marked this pull request as draft December 9, 2021 17:28

bassemmagdy force-pushed the feat-blacklist-whitelist-contacts branch 3 times, most recently from 8309396 to b119669 Compare December 14, 2021 15:52

bassemmagdy requested review from 0x6861746366574, gimre-xymcity and Jaguar0625 December 14, 2021 15:52

bassemmagdy linked an issue Dec 14, 2021 that may be closed by this pull request

Changes to be made to address book 2.0 #1739

Closed

bassemmagdy force-pushed the feat-blacklist-whitelist-contacts branch from b119669 to 60fd197 Compare December 14, 2021 16:04

bassemmagdy marked this pull request as ready for review December 14, 2021 16:06

feature: blacklist/whitelist contacts in address book

52baa8a

bassemmagdy force-pushed the feat-blacklist-whitelist-contacts branch from 60fd197 to 52baa8a Compare December 15, 2021 11:34

Jaguar0625 reviewed Dec 15, 2021

View reviewed changes

increase testing coverage and linting

673aad4

bassemmagdy requested a review from Jaguar0625 December 16, 2021 15:24

gimre-xymcity previously approved these changes Dec 17, 2021

View reviewed changes

Jaguar0625 reviewed Dec 17, 2021

View reviewed changes

update unit tests

226f83e

bassemmagdy dismissed gimre-xymcity’s stale review via 226f83e December 23, 2021 16:07

bassemmagdy requested a review from Jaguar0625 December 23, 2021 16:07

bassemmagdy added 2 commits December 23, 2021 18:19

update package-lock

b8f1c5d

Merge branch 'main' of https://github.com/symbol/desktop-wallet into …

2c43be2

…feat-blacklist-whitelist-contacts

fix console error

cd78095

check network type before adding new contact

15f8b79

highlight malicious transactions button when clicked

507a8aa

fix contact list ui

138e7f9

bassemmagdy added 2 commits January 13, 2022 07:36

Add new contact modals

3ed01a6

add translation

54bc9ac

cryptoBeliever mentioned this pull request Jan 13, 2022

[AddressBook] UI issues #1823

Closed

10 tasks

bassemmagdy added 5 commits January 13, 2022 23:32

update

5941d2e

fix msig modification signing

0a491b3

update ModalTransactionCosignature.vue

757c71b

fix ui issues in #1823

11556bd

fix checkbox margin

ad0bde0

bassemmagdy added 2 commits January 17, 2022 22:39

update blacklist filter to show empty list when there is not blacklis…

bb94484

…ted contacts

fix filtering and update unit tests

b1e3544

fix ui issues

919d1a7

bassemmagdy mentioned this pull request Jan 21, 2022

Aggregate transactions get random names from the addressbook #1831

Closed

OlegMakarenko mentioned this pull request Feb 22, 2022

fix: address book #1847

Merged

OlegMakarenko and others added 2 commits May 12, 2022 14:19

task: merge commit

4e5e8b1

gimre-xymcity approved these changes May 12, 2022

View reviewed changes

__tests__/database/storage/ProfileModelStorage.spec.ts

@@ -1,5 +1,5 @@

/*

* (C) Symbol Contributors 2021 Foundation

Copy link

Member

gimre-xymcity May 12, 2022

Choose a reason for hiding this comment

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

😂

yilmazbahadir merged commit c194967 into dev May 13, 2022

yilmazbahadir deleted the feat-blacklist-whitelist-contacts branch May 13, 2022 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: blacklist whitelist contacts #1814

Feat: blacklist whitelist contacts #1814

bassemmagdy commented Dec 9, 2021 •

edited

Loading

bassemmagdy commented Dec 16, 2021

Jaguar0625 Dec 17, 2021

Jaguar0625 Dec 17, 2021

Jaguar0625 Dec 17, 2021

bassemmagdy commented Dec 23, 2021

cryptoBeliever commented Dec 24, 2021 •

edited

Loading

cryptoBeliever commented Dec 27, 2021

bassemmagdy commented Jan 3, 2022

Jaguar0625 commented Jan 4, 2022

postoronnii commented Jan 4, 2022 •

edited

Loading

cryptoBeliever commented Jan 4, 2022 •

edited

Loading

coiki commented Jan 6, 2022

cryptoBeliever commented Jan 13, 2022 •

edited

Loading

lgtm-com bot commented Jan 14, 2022

lgtm-com bot commented Jan 18, 2022

sonarcloud bot commented Jan 19, 2022

gimre-xymcity May 12, 2022

Feat: blacklist whitelist contacts #1814

Feat: blacklist whitelist contacts #1814

Conversation

bassemmagdy commented Dec 9, 2021 • edited Loading

bassemmagdy commented Dec 16, 2021

Jaguar0625 Dec 17, 2021

Choose a reason for hiding this comment

Jaguar0625 Dec 17, 2021

Choose a reason for hiding this comment

Jaguar0625 Dec 17, 2021

Choose a reason for hiding this comment

bassemmagdy commented Dec 23, 2021

cryptoBeliever commented Dec 24, 2021 • edited Loading

cryptoBeliever commented Dec 27, 2021

bassemmagdy commented Jan 3, 2022

Jaguar0625 commented Jan 4, 2022

postoronnii commented Jan 4, 2022 • edited Loading

cryptoBeliever commented Jan 4, 2022 • edited Loading

coiki commented Jan 6, 2022

cryptoBeliever commented Jan 13, 2022 • edited Loading

lgtm-com bot commented Jan 14, 2022

lgtm-com bot commented Jan 18, 2022

sonarcloud bot commented Jan 19, 2022

gimre-xymcity May 12, 2022

Choose a reason for hiding this comment

bassemmagdy commented Dec 9, 2021 •

edited

Loading

cryptoBeliever commented Dec 24, 2021 •

edited

Loading

postoronnii commented Jan 4, 2022 •

edited

Loading

cryptoBeliever commented Jan 4, 2022 •

edited

Loading

cryptoBeliever commented Jan 13, 2022 •

edited

Loading