feat: Add ENS Reverse Lookup to Create/Load Safe process #3262

usame-algan · 2022-01-10T11:43:01Z

What it solves

Resolves #3231

How this PR fixes it

We do ENS Lookups for Owners when loading and creating a safe and use the return value for the owner input fields

How to test it

Load Safe

Open the Safe App
Load existing Safe that has an owner with registered ENS name

Create Safe

Open the Safe App
Connect with a wallet that has a registered ENS name
Create new Safe

Screenshots

github-actions · 2022-01-10T11:43:14Z

CLA Assistant Lite All Contributors have signed the CLA.

src/routes/LoadSafePage/LoadSafePage.tsx

github-actions · 2022-01-10T11:46:48Z

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}

coveralls · 2022-01-10T11:47:44Z

Pull Request Test Coverage Report for Build 1729147136

57 of 61 (93.44%) changed or added relevant lines in 13 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 32.341%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/routes/CreateSafePage/steps/OwnersAndConfirmationsNewSafeStep.tsx	14	15	93.33%
src/routes/CreateSafePage/components/SafeCreationProcess.tsx	0	3	0.0%

Totals
Change from base Build 1729078921:	0.4%
Covered Lines:	3172
Relevant Lines:	8716

💛 - Coveralls

…safe-react into safe-creation-reverse-ens-lookup

github-actions · 2022-01-13T15:43:43Z

Deployment links

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

github-actions · 2022-01-13T15:50:26Z

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

Failed tests:

❌ Address book Address book
❌ Safe Apps List Safe Apps List
❌ Read-only transaction creation and review Read-only transaction creation and review

src/routes/LoadSafePage/LoadSafePage.tsx

github-actions · 2022-01-14T16:21:11Z

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

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

src/routes/LoadSafePage/steps/ReviewLoadStep.tsx

katspaugh · 2022-01-17T10:09:07Z

Usame:
the actual typed value shouldn't be used as a placeholder.

katspaugh · 2022-01-17T17:18:48Z

Something is wrong with the branch, it shows too many changes.

usame-algan · 2022-01-17T17:27:13Z

Something is wrong with the branch, it shows too many changes.

Had to rebase it to safe-ens-domain-with-provider so we could use reverseENSLookup but probably makes more sense to wait until that branch is merged so we can rebase to dev again

src/logic/wallets/getWeb3.ts

katspaugh

Amazing!

iamacook

🚀

iamacook · 2022-01-18T13:10:59Z

src/logic/wallets/getWeb3.ts

+  }
+
+  const isValidAddress = sameString(verifiedAddress, address)
+  return isValidAddress ? name : ''


This changed in the provider inclusion ticket. Maybe you should rebase on that?

# Conflicts: # package.json # src/logic/wallets/getWeb3.ts

github-actions · 2022-01-18T16:22:06Z

Deployment links

🟠 Rinkeby	⚪ Mainnet	🟣 Polygon	🟡 BSC	⚫ Arbitrum	🟢 Gnosis Chain

francovenica · 2022-01-19T14:16:22Z

This addresses have ENS names:
0x7724b234c9099C205F03b458944942bcEBA13408 francotest2.eth
0x61a0c717d18232711bC788F19C9Cd56a43cc8872 francotest.eth

Issues:
1 - Safe creation:
The name of the same is empty by default and allows me to jump to the next step. There should be a randomly generated name by default.
Is expected that if the input is empty an error shows up that prevents the user to go to the next step

2 - Question/issue
Safe creation step 2
The user that is currently connected shows the ENS name fine (without the .eth), but if I add more addresses that also have ENS names those won't show it, just "Owner name" is set as default.
Should new addresses added show the ENS names?

usame-algan · 2022-01-19T15:41:39Z

Issues: 1 - Safe creation: The name of the same is empty by default and allows me to jump to the next step. There should be a randomly generated name by default.

This happened to me too while testing but I thought it was a hot reload issue. I will look into it!

2 - Question/issue Safe creation step 2 The user that is currently connected shows the ENS name fine (without the .eth), but if I add more addresses that also have ENS names those won't show it, just "Owner name" is set as default. Should new addresses added show the ENS names?

Good catch! I would suggest we also fetch ENS when manually adding new Owners. cc @katspaugh

src/routes/CreateSafePage/steps/NameNewSafeStep.tsx

src/routes/CreateSafePage/steps/OwnersAndConfirmationsNewSafeStep.tsx

francovenica · 2022-01-20T17:27:49Z

1 - Fixed

2 - Is mostly fixed. It still shows "Owner name" when an address is introduced. It changes to the ENS name if you click in the input.

usame-algan · 2022-01-21T11:13:30Z

2 - Is mostly fixed. It still shows "Owner name" when an address is introduced. It changes to the ENS name if you click in the input.

Thanks for testing Franco! I added support to fetch ENS when importing an owner via QR Code.

francovenica · 2022-01-21T14:21:58Z

Looks good to me.

…ens-lookup

feat: Add ENS Reverse Lookup to Load Safe process

a5855ee

usame-algan self-assigned this Jan 10, 2022

katspaugh reviewed Jan 10, 2022

View reviewed changes

src/routes/LoadSafePage/LoadSafePage.tsx Outdated Show resolved Hide resolved

usame-algan and others added 10 commits January 11, 2022 12:37

build: Add ens domain to provider info

a6847e5

feat: Add ens reverse lookup function

d67909a

fix: ENS Lookup only on account change

442fff1

chore: Cleanup code + add types

47e4d0f

fix: Tweak logic + add return type

d94080c

chore: Remove empty string constant

8e8dc68

fix: Compare against ChainId

2f1d876

feat: Add ENS Reverse Lookup to Load Safe process

70d30d5

Merge branch 'safe-creation-reverse-ens-lookup' of github.com:gnosis/…

ec0faee

…safe-react into safe-creation-reverse-ens-lookup

chore: Replace reverse ens lookup function from core sdk

4aae8f8

usame-algan commented Jan 14, 2022

View reviewed changes

src/routes/LoadSafePage/LoadSafePage.tsx Outdated Show resolved Hide resolved

feat: Add reverse ENS Lookup to create safe process

c282e51

chore: Add typing, extract getOwnerName

800c363

katspaugh reviewed Jan 17, 2022

View reviewed changes

src/routes/LoadSafePage/steps/ReviewLoadStep.tsx Outdated Show resolved Hide resolved

usame-algan marked this pull request as ready for review January 17, 2022 09:50

refactor: use getOwnerName for load safe review step

3962480

usame-algan added 3 commits January 17, 2022 11:13

chore: jest spy on ens getResolver to fix failing tests

5ad3945

fix: Don't use typed value as placeholder for owner name

938da79

fix: try catch ens resolver

b14bf08

build: Use ENS domain part for safe creation and loading

1e9aeb4

katspaugh reviewed Jan 18, 2022

View reviewed changes

src/logic/wallets/getWeb3.ts Outdated Show resolved Hide resolved

katspaugh reviewed Jan 18, 2022

View reviewed changes

src/logic/wallets/getWeb3.ts Outdated Show resolved Hide resolved

chore: Rename getDomainPart util function

6863634

usame-algan changed the title ~~feat: Add ENS Reverse Lookup to Load Safe process~~ feat: Add ENS Reverse Lookup to Create/Load Safe process Jan 18, 2022

katspaugh approved these changes Jan 18, 2022

View reviewed changes

iamacook approved these changes Jan 18, 2022

View reviewed changes

Merge branch 'dev' into safe-creation-reverse-ens-lookup

33ee89c

# Conflicts: # package.json # src/logic/wallets/getWeb3.ts

fix: Move initial ENS fetch, fetch ENS when adding new owners

9db6155

katspaugh reviewed Jan 20, 2022

View reviewed changes

src/routes/CreateSafePage/steps/NameNewSafeStep.tsx Show resolved Hide resolved

katspaugh reviewed Jan 20, 2022

View reviewed changes

src/routes/CreateSafePage/steps/OwnersAndConfirmationsNewSafeStep.tsx Outdated Show resolved Hide resolved

usame-algan added 2 commits January 20, 2022 15:41

refactor: use absolute import paths

591b911

refactor: Use absolute paths for imports

6beb294

fix: Get ENS if new owner is imported with qr code

967df77

Merge remote-tracking branch 'origin/dev' into safe-creation-reverse-…

8cbb27e

…ens-lookup

usame-algan merged commit dd8724e into dev Jan 21, 2022

usame-algan deleted the safe-creation-reverse-ens-lookup branch January 21, 2022 14:33

github-actions bot locked and limited conversation to collaborators Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ENS Reverse Lookup to Create/Load Safe process #3262

feat: Add ENS Reverse Lookup to Create/Load Safe process #3262

usame-algan commented Jan 10, 2022 •

edited

Loading

github-actions bot commented Jan 10, 2022

github-actions bot commented Jan 10, 2022 •

edited

Loading

coveralls commented Jan 10, 2022 •

edited

Loading

github-actions bot commented Jan 13, 2022

github-actions bot commented Jan 13, 2022 •

edited

Loading

github-actions bot commented Jan 14, 2022 •

edited

Loading

katspaugh commented Jan 17, 2022

katspaugh commented Jan 17, 2022

usame-algan commented Jan 17, 2022

katspaugh left a comment

iamacook left a comment

iamacook Jan 18, 2022

github-actions bot commented Jan 18, 2022

francovenica commented Jan 19, 2022

usame-algan commented Jan 19, 2022

francovenica commented Jan 20, 2022

usame-algan commented Jan 21, 2022

francovenica commented Jan 21, 2022

feat: Add ENS Reverse Lookup to Create/Load Safe process #3262

feat: Add ENS Reverse Lookup to Create/Load Safe process #3262

Conversation

usame-algan commented Jan 10, 2022 • edited Loading

What it solves

How this PR fixes it

How to test it

Load Safe

Create Safe

Screenshots

github-actions bot commented Jan 10, 2022

github-actions bot commented Jan 10, 2022 • edited Loading

ESLint Summary View Full Report

coveralls commented Jan 10, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1729147136

💛 - Coveralls

github-actions bot commented Jan 13, 2022

Deployment links

github-actions bot commented Jan 13, 2022 • edited Loading

github-actions bot commented Jan 14, 2022 • edited Loading

ESLint Summary View Full Report

katspaugh commented Jan 17, 2022

katspaugh commented Jan 17, 2022

usame-algan commented Jan 17, 2022

katspaugh left a comment

Choose a reason for hiding this comment

iamacook left a comment

Choose a reason for hiding this comment

iamacook Jan 18, 2022

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2022

Deployment links

francovenica commented Jan 19, 2022

usame-algan commented Jan 19, 2022

francovenica commented Jan 20, 2022

usame-algan commented Jan 21, 2022

francovenica commented Jan 21, 2022

usame-algan commented Jan 10, 2022 •

edited

Loading

github-actions bot commented Jan 10, 2022 •

edited

Loading

coveralls commented Jan 10, 2022 •

edited

Loading

github-actions bot commented Jan 13, 2022 •

edited

Loading

github-actions bot commented Jan 14, 2022 •

edited

Loading