Display readable names for known addresses #2816

iamacook · 2021-10-11T15:19:40Z

What it solves

Resolves #1866 (showing names in transaction lists)

How this PR fixes it

Instead of the default text (now used as fallback), the name/logoUri in each transaction is displayed in the transaction list. If a name is present in the address book, that takes priority.

How to test it

Open historical/queued transactions and check that there are names/logos.

Screenshots

github-actions · 2021-10-11T15:20:06Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2021-10-11T15:24:24Z

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	1	0
Ignored	0	N/A

Result: ✅ success
Annotations: 1 total

[warning] react-hooks/exhaustive-deps

verifies the list of dependencies for Hooks like useEffect and similar

src/routes/safe/components/Transactions/TxList/hooks/useTransactionType.ts Line 76 - React Hook useEffect has missing dependencies: 'toAddress?.logoUri' and 'toAddress?.name'. Either include them or remove the dependency array.

^{Report generated by eslint-plus-action}

github-actions · 2021-10-11T15:28:08Z

Safe Multisig app mainnet

github-actions · 2021-10-11T15:28:18Z

Safe Multisig app polygon

github-actions · 2021-10-11T15:28:49Z

Safe Multisig app arbitrum

github-actions · 2021-10-11T15:29:30Z

Safe Multisig app rinkeby

github-actions · 2021-10-11T15:33:28Z

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

katspaugh · 2021-10-12T09:44:46Z

src/routes/safe/components/Transactions/TxList/hooks/useTransactionType.ts

-        setType({ icon: isSendTx ? OutgoingTxIcon : IncomingTxIcon, text: isSendTx ? 'Send' : 'Receive' })
+
+        const icon = isSendTx ? OutgoingTxIcon : IncomingTxIcon
+        const text = toAddress?.name ?? isSendTx ? 'Send' : 'Receive'


So if there's no name it won't display anything?

It will show 'Send' or 'Receive' as it was before.

katspaugh · 2021-10-12T09:45:46Z

src/routes/safe/components/Transactions/TxList/hooks/useTransactionType.ts

+        const icon = knownAddress.isAddressBook
+          ? CustomTxIcon
+          : knownAddress.image ?? toAddress?.logoUri ?? CustomTxIcon
+        const text = knownAddress.isAddressBook ? knownAddress.name : toAddress?.name ?? 'Contract interaction'


Why not just

const text = knownAddress.name || toAddress?.name || 'Contract interaction'

?

katspaugh

Looketh Goode!

katspaugh · 2021-10-12T09:57:06Z

src/routes/safe/components/Transactions/TxList/hooks/useTransactionType.ts

+
+        setType({
+          icon: isSendTx ? OutgoingTxIcon : IncomingTxIcon,
+          text: knownAddressBookAddress.name || toAddress?.name || isSendTx ? 'Send' : 'Receive',


I think you might need a set of parens here

francovenica · 2021-10-12T19:17:22Z

Notes:
Since I don't have a safe with tx from "known" names that I know of and I don't have control over the known safes list, I just took the list we already have here and made tx to them or with them as recipient

Also I cannot use mainnet prod and this mainnet prod because its services are different, therefore they might show different names, like this:

The following is a comparison between this PR and pr2793 that doesn't have this fix. First is shown how in the Other PR the name is "Sent" and in this new one the name is the "known name".

Here I was only able to check it by sending funds to those known addresses, but any type of "contract interaction" type of tx were, in both PRs, have the name recognized just fine
The safes:
https://pr2816--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/balances
https://pr2793--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

So far it looks fine, but I'll check with Liliya if she has a safe where we can see something in the history where those known

iamacook · 2021-10-13T07:03:22Z

Notes: Since I don't have a safe with tx from "known" names that I know of and I don't have control over the known safes list, I just took the list we already have here and made tx to them or with them as recipient

Also I cannot use mainnet prod and this mainnet prod because its services are different, therefore they might show different names, like this:

The following is a comparison between this PR and pr2793 that doesn't have this fix. First is shown how in the Other PR the name is "Sent" and in this new one the name is the "known name".

Here I was only able to check it by sending funds to those known addresses, but any type of "contract interaction" type of tx were, in both PRs, have the name recognized just fine The safes: https://pr2816--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/balances https://pr2793--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

So far it looks fine, but I'll check with Liliya if she has a safe where we can see something in the history where those known

This is a safe with named transactions. I believe it's Liliya's.

francovenica · 2021-10-13T11:50:09Z

Checked that safe, and yes, it shows known addresses as well, (mostly a lili's safe that was added to the list)

Looks good to me

francovenica · 2021-10-13T14:28:22Z

This ticket was kicked back since it seems that there are more requirements that were not fulfilled. We have to check what is the real scope of this ticket

iamacook · 2021-10-13T14:40:33Z

This ticket was kicked back since it seems that there are more requirements that were not fulfilled. We have to check what is the real scope of this ticket

Please see my comment here. @francovenica

liliya-soroka · 2021-10-13T20:26:13Z

@iamacook , @katspaugh "known addresses" were missed on transaction details for incoming/outgoing txs
We should not change it in transactions list for incoming and outgoing txs
From my comment
"The addresses are not taken from the known address list on transaction details. - Outgoing tx /Incoming txs.
Only custom transaction was covered in known addresses v1"

Use logoUri and name from txInfo

7d81eae

iamacook marked this pull request as draft October 11, 2021 15:19

Tweak transaction icons

f7d5baf

Use helper function

0554f57

iamacook requested review from katspaugh and DiogoSoaress October 12, 2021 09:21

iamacook marked this pull request as ready for review October 12, 2021 09:21

katspaugh reviewed Oct 12, 2021

View reviewed changes

Clean up logic

468d979

iamacook requested a review from katspaugh October 12, 2021 09:53

katspaugh approved these changes Oct 12, 2021

View reviewed changes

katspaugh suggested changes Oct 12, 2021

View reviewed changes

Add brackets

ff5c160

katspaugh approved these changes Oct 12, 2021

View reviewed changes

Merge branch 'dev' into display-known-addresses

5ae4999

katspaugh merged commit c64ea1d into dev Oct 14, 2021

katspaugh deleted the display-known-addresses branch October 14, 2021 07:19

github-actions bot locked and limited conversation to collaborators Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display readable names for known addresses #2816

Display readable names for known addresses #2816

iamacook commented Oct 11, 2021 •

edited

Loading

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021 •

edited

Loading

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021 •

edited

Loading

katspaugh Oct 12, 2021

iamacook Oct 12, 2021

katspaugh Oct 12, 2021 •

edited

Loading

katspaugh left a comment

katspaugh Oct 12, 2021 •

edited

Loading

francovenica commented Oct 12, 2021

iamacook commented Oct 13, 2021

francovenica commented Oct 13, 2021

francovenica commented Oct 13, 2021

iamacook commented Oct 13, 2021

liliya-soroka commented Oct 13, 2021

Display readable names for known addresses #2816

Display readable names for known addresses #2816

Conversation

iamacook commented Oct 11, 2021 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021 • edited Loading

ESLint Summary View Full Report

[warning] react-hooks/exhaustive-deps

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021

github-actions bot commented Oct 11, 2021 • edited Loading

katspaugh Oct 12, 2021

Choose a reason for hiding this comment

iamacook Oct 12, 2021

Choose a reason for hiding this comment

katspaugh Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

katspaugh left a comment

Choose a reason for hiding this comment

katspaugh Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

francovenica commented Oct 12, 2021

iamacook commented Oct 13, 2021

francovenica commented Oct 13, 2021

francovenica commented Oct 13, 2021

iamacook commented Oct 13, 2021

liliya-soroka commented Oct 13, 2021

iamacook commented Oct 11, 2021 •

edited

Loading

github-actions bot commented Oct 11, 2021 •

edited

Loading

github-actions bot commented Oct 11, 2021 •

edited

Loading

katspaugh Oct 12, 2021 •

edited

Loading

katspaugh Oct 12, 2021 •

edited

Loading