Components support eip3770 prefixes #2896

DiogoSoaress · 2021-10-26T11:33:51Z

What it solves

Resolves #2757 and #2842

How this PR fixes it

This PR allows to copy/display addresses as per settings

How to test it

Tweak the Appearence settings and verify if the addresses are displayed/copied with/without the chain prefix

Screenshots

github-actions · 2021-10-26T11:34:09Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2021-10-26T11:39:46Z

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

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2021-10-26T11:40:38Z

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

coveralls · 2021-10-26T11:40:52Z

Pull Request Test Coverage Report for Build 1444457765

5 of 17 (29.41%) changed or added relevant lines in 9 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.03%) to 31.689%

Changes Missing Coverage	Changed/Added Lines	%
src/components/CustomIconText/index.tsx	1	0.0%
src/routes/safe/components/Balances/SendModal/screens/AddressBookInput/index.tsx	1	0.0%
src/routes/safe/components/Transactions/TxList/MethodValue.tsx	1	0.0%
src/routes/safe/components/Transactions/TxList/TxSummary.tsx	1	0.0%
src/components/SafeListSidebar/SafeList/SafeListItem.tsx	2	0.0%
src/routes/safe/components/Settings/Appearance/index.tsx	2	0.0%
src/components/AppLayout/Sidebar/SafeHeader/index.tsx	4	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/screens/OwnerForm/index.tsx	1	0%

Totals
Change from base Build 1443243556:	0.03%
Covered Lines:	2979
Relevant Lines:	8377

💛 - Coveralls

…ding Limit options

github-actions · 2021-10-29T10:00:39Z

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

Failed tests:

❌ Add an existing safe Add an existing safe
❌ Safe Apps List Safe Apps List
❌ Safe Balances Safe Balances

DiogoSoaress · 2021-10-29T10:12:38Z

I did not introduce the prefix where the connected network is explicit:

ProviderInfo where we get the connected wallet info
UserDetails card
Header in the left side menu
SafeList in the left side menu

Also prefixing the InlineEthHashInfo with the shortName caused me some hesitation.

Please let me know what do you think.

iamacook

Have you checked for instances where we render addresses outside of EthHashInfo?

iamacook · 2021-10-29T10:19:18Z

src/components/PrefixedEthHashInfo/index.tsx

+
+type ExplorerInfo = () => { url: string; alt: string }
+
+interface Props {


Why not just import the interface from the component? If it's not exported, you could use Parameters<typeof PrefixedEthHashInfo>[0].

katspaugh · 2021-10-29T10:22:56Z

src/routes/safe/components/Settings/Appearance/index.tsx

@@ -37,8 +37,7 @@ const Appearance = (): ReactElement => {
      <Heading tag="h2">Use Chain-Specific Addresses</Heading>
      <Paragraph>You can choose whether to prepend EIP-3770 short chain names accross Safes.</Paragraph>
      <Paragraph>
-        {showShortName && <strong>{shortName}:</strong>}
-        {safeAddress}
+        <PrefixedEthHashInfo hash={safeAddress} />


katspaugh · 2021-10-29T10:25:16Z

src/components/PrefixedEthHashInfo/index.tsx

+  explorerUrl?: ExplorerInfo
+}
+
+const PrefixedEthHashInfo: React.FC<Props> = ({ hash, ...rest }) => {


A basic test for this new component would be in order.

katspaugh · 2021-11-01T09:35:51Z

src/components/AppLayout/Sidebar/SafeHeader/index.tsx

@@ -158,7 +164,7 @@ const SafeHeader = ({
          <ButtonHelper onClick={onReceiveClick}>
            <Icon size="sm" type="qrCode" tooltip="Show QR" />
          </ButtonHelper>
-          <CopyToClipboardBtn textToCopy={address} />
+          <CopyToClipboardBtn textToCopy={copyChainPrefix ? `${shortName}:${address}` : `${address}`} />


CopyToClipboardBtn is used in two other places, should we also create a wrapper for it?

I was implementing a wrapper for it called CopyAddressToClipboardBtn but the CopyToClipboardBtn is only copying an address in the SafeHeader component.

Is it worthy to implement the wrapper afterall?

francovenica · 2021-11-03T14:56:45Z

Question. Should the checkbox in the modal of the safe QR code remember its status of checked/unchecked after you close it? It seems that it copies the status of the checkbox in Settings > Appearance, so I'm not sure is just "temporary add/remove prefix" kind of thing, or if that checkbox should also change the checkbox in the settings as well.
Check the gif:

DiogoSoaress · 2021-11-03T15:45:53Z

Question. Should the checkbox in the modal of the safe QR code remember its status of checked/unchecked after you close it? It seems that it copies the status of the checkbox in Settings > Appearance, so I'm not sure is just "temporary add/remove prefix" kind of thing, or if that checkbox should also change the checkbox in the settings as well. Check the gif:

The QR modal defaults to the value set in settings. Also, toggling the check box on the Receiving modal deliberately does not update the Settings > Appearance (store value) as described in #2888 (comment)
Can you confirm this is the behavior to carry on @katspaugh ?

katspaugh · 2021-11-04T09:53:47Z

Yes, that's how we intended it to be.
But I also see Franco's point, it might be good to remember the user's choice in the QR modal specifically. Up to you, @DiogoSoaress.

francovenica · 2021-11-08T19:56:17Z

Note:

I'm ignoring inputs where addresses go, like this one:

Only once the input is set and becomes "read only" should have the index in front of it.

Places checked:

Create safe:
-- Review step > Owners addresses
Add safe:
-- Owner step > Owner list
-- Review step > Owner/Safe addresses
-- QR code for the safe > Safe address
Safes Sidebar:
-- Safes list
-- Owned safes list
Send funds / Send collectibles modal:
-- Safe address
-- List of receivers and the receiver itself
-- Review step > Safe and receiver
Contract interaction modal:
-- Contract address
Transactions: (Queue and History)
-- Owners who signed and executed
-- Tx details: Hash, Safe hash, receivers of a tx, contracts, modules, addresses inside "Actions", Safe creation tx
Address Book:
-- Entry list
-- "Send" modal
Settings Appearance:
-- safe address example
Settings Owners:
-- Owner list
-- Edit owner name modal
-- Replace owner step 1 and 2
-- Remove owner modal step 1 and 3
-- Add owner step 3
Settings Spending Limit:
-- Owner list
-- Allowance removal modal
-- Allowance add/edit modal steps 1 and 2
Settings Advanced
-- List of modules
-- List of Tx guards (maybe, I don't have a guard to enable, so I could not test this one)
Settings Remove safe modal

Question:

Should Values that are used as inputs in contract interaction methods should have the prefix? I don't know if all the addresses you will use as values for contract interactions are always from the same chainID or not

Places where the prefix is missing:

1 - Sidebar doesn't show the prefix. Is it because they are already under the network label? Still, I think that for consistency sake they should have it

2 - The dropdown of the owner connected to the safe app:

3 - The safe in the sidebar itself

4 - Address book values:

5 - There is a font that is not correct in the "Spending limit" section. I've checked other PR's but the font is fine on those, so I think it broke just in this PR
To see this go to the spending limit, open the modal by trying to create one
How it looks:

How it should look:

DiogoSoaress · 2021-11-08T22:19:41Z

@francovenica thank you for the detailed QA 🙏

I had left in a commentary above open to discussion all the places you point so thank you for the feedback!

– 4. I'm going to implement the chain prefix for the sake of consistency as you mention 👍
The font seems to be good in the Rinkeby deployment -> (Averta, monospace) so maybe it didn't load when you opened the URL. Did you tried to refresh the page?

github-actions · 2021-11-08T22:40:46Z

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

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

DiogoSoaress · 2021-11-09T08:59:02Z

@francovenica answering to the question

Should Values that are used as inputs in contract interaction methods should have the prefix? I don't know if all the addresses you will use as values for contract interactions are always from the same chainID or not

The answer we got -> "we can now safely assume that addresses are always meant fo the same chain id"
https://gnosisinc.slack.com/archives/CQB729LLF/p1636411988161700

So yeah, we should also display it there. I think is under the scope of #2954 however

iamacook · 2021-11-09T09:01:07Z

I think we agreed internally that you need not include the shortName anywhere in the sidebar, or in the wallet connection information because the network name is clear there.

DiogoSoaress · 2021-11-09T09:11:19Z

@iamacook True! though I found Franco's "sake of consistency" point valid as per the kickoff output. Also I implemented it and looks good indeed --> https://pr2896--safereact.review-safe.gnosisdev.com/app/rinkeby
Would like to welcome a last feedback on it. I would go for it 👍

iamacook · 2021-11-09T09:13:37Z

@iamacook True! though I found Franco's "sake of consistency" point valid as per the kickoff output. Also I implemented it and looks good indeed --> https://pr2896--safereact.review-safe.gnosisdev.com Would like to welcome a last feedback on it. I would go for it 👍

I would argue that all of the stated positions are already truncated. The user needs to see enough of their address to know which one it is.

Your link 404s.

DiogoSoaress · 2021-11-09T09:22:04Z

We show the same number of characters with and without the chain prefix

Again, just welcoming another round of feedback now that it was implemented. @katspaugh @francovenica

iamacook · 2021-11-09T09:58:17Z

I think it's clear enough as is but it's personal opinion.

francovenica · 2021-11-10T03:44:22Z

I think it's clear enough as is but it's personal opinion.

Technically we also have it in most modals, so it shouldn't be necessary there either, but just for the sake of not having to wonder "we should put the prefix here o not" and just put it everywhere we can

francovenica · 2021-11-10T03:47:00Z

I had left in a commentary above open to discussion all the places you point so thank you for the feedback!

I'm a terrible reader. Sorry :(

Still, all the new prefixes in the sidebar, connect and safe list look fine
The ones from the AB are fine as well
I'm ignoring the prefix for addresses used in contract interactions for now then. I'll recheck in the 2954

I'm still seeing the wrong font in the Spending limit modal. I'm using Chrome in windows10 btw. We can ignore it for now since it not part of this ticket, and I'll check again when I do regression in the release branch.

I'm ok approving this ticket. Moving it to QA done

iamacook · 2021-11-10T07:35:42Z

I think it's clear enough as is but it's personal opinion.

Technically we also have it in most modals, so it shouldn't be necessary there either, but just for the sake of not having to wonder "we should put the prefix here o not" and just put it everywhere we can

I feel like modals are a bit more important and should have a shortName because that's when a user sends their funds. Let's perhaps discuss this in our stand-up today? @katspaugh @johannesmoormann

francovenica · 2021-11-10T16:27:51Z

@DiogoSoaress The fix on the sidebar for safe looks fine

…770-prefixes

DiogoSoaress linked an issue Oct 28, 2021 that may be closed by this pull request

[Epic] Show a chain prefix everywhere where we display an address #2842

Closed

Diogo Soares added 3 commits October 28, 2021 22:06

Create hook to return the prefixed address as per settings

8d7d3ee

Apply appearence settings on Sidebar header

9e256b3

Bump @gnosis.pm/safe-react-components to use latest EthHashInfo

a39cf15

DiogoSoaress force-pushed the components-support-eip3770-prefixes branch from 5640525 to a39cf15 Compare October 28, 2021 20:20

Diogo Soares added 7 commits October 28, 2021 22:21

Add EthHashInfo wrapper component for shortName settings

aee9dd2

Use PrefixedEthHashInfo component in the Appearance settings page

9a9e79d

Replace EthHashInfo for the wrapper in the Safe owner management options

a45999b

Replace EthHashInfo for the wrapper in the Balances modal's send screens

b1b0632

Replace EthHashInfo for its wrapper in the Advanced Settings and Spen…

644b2ad

…ding Limit options

Replace EthHashInfo for its wrapper PrefixedEthHashInfo

b53afb3

Remove unnecessarily introduced hook

5e9dbd9

Implement InlineEthHashInfo from the EthHashInfo wrapper

2dd13f3

DiogoSoaress marked this pull request as ready for review October 29, 2021 10:13

DiogoSoaress requested review from katspaugh and iamacook October 29, 2021 10:16

iamacook reviewed Oct 29, 2021

View reviewed changes

katspaugh reviewed Oct 29, 2021

View reviewed changes

Diogo Soares added 3 commits October 29, 2021 13:40

Use lib component props and avoid duplication

c44d55d

Add test for PrefixedEthHashInfo component

98b2b7a

Fix "<p> cannot appear as a descendant of <p>" warning

01e7fca

katspaugh reviewed Nov 1, 2021

View reviewed changes

Unset font-weight in PrefixedEthHashInfo when it is inline.

c07aa71

Do not display chain shortName in AddressBook addresses

82624fb

Use PrefixedEthHashInfo displaying addresses for the sake of consistency

8261959

Merge branch 'dev' into components-support-eip3770-prefixes

7df7897

DiogoSoaress force-pushed the components-support-eip3770-prefixes branch from 16cc7cd to 7df7897 Compare November 8, 2021 22:42

Diogo Soares added 2 commits November 10, 2021 09:34

Merge branch 'dev' into components-support-eip3770-prefixes

0520004

Pass chainName prop to the EthHashInfo in the SafeListSideBar

cb51b39

DiogoSoaress added the do-not-merge label Nov 11, 2021

iamacook mentioned this pull request Nov 12, 2021

Feature: deep-linking a single tx #2974

Merged

Remove temporarly condition to not toggle prefixed addresses in PROD

572817e

DiogoSoaress changed the base branch from dev to feature/eip-3770-prefixes November 15, 2021 15:59

Merge branch 'feature/eip-3770-prefixes' into components-support-eip3…

d01319c

…770-prefixes

DiogoSoaress merged commit 253d4bf into feature/eip-3770-prefixes Nov 16, 2021

DiogoSoaress deleted the components-support-eip3770-prefixes branch November 16, 2021 08:41

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components support eip3770 prefixes #2896

Components support eip3770 prefixes #2896

DiogoSoaress commented Oct 26, 2021 •

edited

Loading

github-actions bot commented Oct 26, 2021

github-actions bot commented Oct 26, 2021 •

edited

Loading

github-actions bot commented Oct 26, 2021

coveralls commented Oct 26, 2021 •

edited

Loading

github-actions bot commented Oct 29, 2021 •

edited

Loading

DiogoSoaress commented Oct 29, 2021

iamacook left a comment

iamacook Oct 29, 2021

katspaugh Oct 29, 2021

katspaugh Oct 29, 2021

katspaugh Nov 1, 2021

DiogoSoaress Nov 1, 2021

francovenica commented Nov 3, 2021

DiogoSoaress commented Nov 3, 2021 •

edited

Loading

katspaugh commented Nov 4, 2021

francovenica commented Nov 8, 2021

DiogoSoaress commented Nov 8, 2021

github-actions bot commented Nov 8, 2021 •

edited

Loading

DiogoSoaress commented Nov 9, 2021 •

edited

Loading

iamacook commented Nov 9, 2021

DiogoSoaress commented Nov 9, 2021 •

edited

Loading

iamacook commented Nov 9, 2021

DiogoSoaress commented Nov 9, 2021

iamacook commented Nov 9, 2021

francovenica commented Nov 10, 2021

francovenica commented Nov 10, 2021 •

edited

Loading

iamacook commented Nov 10, 2021

francovenica commented Nov 10, 2021


		type ExplorerInfo = () => { url: string; alt: string }

		interface Props {

Components support eip3770 prefixes #2896

Components support eip3770 prefixes #2896

Conversation

DiogoSoaress commented Oct 26, 2021 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

github-actions bot commented Oct 26, 2021

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

ESLint Summary View Full Report

github-actions bot commented Oct 26, 2021

coveralls commented Oct 26, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1444457765

💛 - Coveralls

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

DiogoSoaress commented Oct 29, 2021

iamacook left a comment

Choose a reason for hiding this comment

iamacook Oct 29, 2021

Choose a reason for hiding this comment

katspaugh Oct 29, 2021

Choose a reason for hiding this comment

katspaugh Oct 29, 2021

Choose a reason for hiding this comment

katspaugh Nov 1, 2021

Choose a reason for hiding this comment

DiogoSoaress Nov 1, 2021

Choose a reason for hiding this comment

francovenica commented Nov 3, 2021

DiogoSoaress commented Nov 3, 2021 • edited Loading

katspaugh commented Nov 4, 2021

francovenica commented Nov 8, 2021

Note:

Places checked:

Question:

Places where the prefix is missing:

DiogoSoaress commented Nov 8, 2021

github-actions bot commented Nov 8, 2021 • edited Loading

ESLint Summary View Full Report

DiogoSoaress commented Nov 9, 2021 • edited Loading

iamacook commented Nov 9, 2021

DiogoSoaress commented Nov 9, 2021 • edited Loading

iamacook commented Nov 9, 2021

DiogoSoaress commented Nov 9, 2021

iamacook commented Nov 9, 2021

francovenica commented Nov 10, 2021

francovenica commented Nov 10, 2021 • edited Loading

iamacook commented Nov 10, 2021

francovenica commented Nov 10, 2021

DiogoSoaress commented Oct 26, 2021 •

edited

Loading

github-actions bot commented Oct 26, 2021 •

edited

Loading

coveralls commented Oct 26, 2021 •

edited

Loading

github-actions bot commented Oct 29, 2021 •

edited

Loading

DiogoSoaress commented Nov 3, 2021 •

edited

Loading

github-actions bot commented Nov 8, 2021 •

edited

Loading

DiogoSoaress commented Nov 9, 2021 •

edited

Loading

DiogoSoaress commented Nov 9, 2021 •

edited

Loading

francovenica commented Nov 10, 2021 •

edited

Loading