add basic ENS name resolution for address lookup #771

acolytec3 · 2021-10-15T15:31:04Z

Summary

Adds basic ENS name resolution to the address lookup route in the Protocol Explorer.

To Test

<> Open the home page.

<<What to expect?>> Verify it contains "Search by Order ID / ETH Address / ENS Address" in the search box

<> Type in a any valid ENS domain and hit submit

<<What to expect?>> The order history for the address that owns the ENS domain should be displayed
Note: If the provided ENS domain cannot be resolved, just routes to /address/null to display the default error message for an address that cannot be found

Background

ENS resolver function:
- I'm using the web3.eth.ens.getAddress to do the ENS name resolution since that's what is readily available in your web3 provider though that method appears to have been marked deprecated in web3.
- There any number of alternative ENS resolution packages that could be put here but it might involve switching web3 library to ethers or something (which I also see in your package.json)
Address validation regex updates
- I decided to just update the address validation to check for anything that appeared to be a potentially valid top-level DNS domain (e.g. .com, .org, .xyz). It looks like ENS only currently allows two other TLDs at (.xyz, kred) but according to this article any DNS TLD could conceptually be used so no reason to not let anyone enter any arbitrary valid DNS address to support potential future extensions of ENS.

anxolin

Awesome! We love to see contributors. Great initiative

src/hooks/useSearchSubmit.ts

anxolin · 2021-10-15T16:40:45Z

src/utils/miscellaneous.ts

@@ -265,4 +265,4 @@ export const isAnOrderId = (text: string): boolean => text.match(/^0x[a-fA-F0-9]
 *
 * @param text Possible address string to check
 */
-export const isAnAddressAccount = (text: string): boolean => text.match(/^0x[a-fA-F0-9]{40}$/)?.input !== undefined
+export const isAnAddressAccount = (text: string): boolean => text.match(/^0x[a-fA-F0-9]{40}$|[a-zA-Z0-9]+\.[a-zA-Z]+$/)?.input !== undefined


should we have a separate pattern just for the ENS?

anxolin · 2021-10-15T16:45:34Z

src/hooks/useSearchSubmit.ts

+          web3.eth.ens
+            .getAddress(query)
+            .then((res) => res && res.length > 0 && history.push(`/${pathPrefix}/${res}`))
+            .catch(() => history.push(`/address/null`))


For the ENS case, when it cannot be resolved, it should just take you to the default. This is kind of unrelated of this PR (so out of it's scope), however, I mention it because address/null is not a good redirection imo

A good one could be:

In my latest commit, I route to /address/${query} so let me know if it should be something yet different. If it's a valid ENS domain, it will just show no records (since there will be valid ETH address behind it) or else the generic "Page not found" where an invalid address is provided as the query parameter.

alfetopito

First of all, thank you for the time to submit this PR, really appreciate it.

I've left a couple of comments in the code.

Regarding the behaviour, I can't get the ENS resolution to work locally.
Any (valid or invalid) ens fails resolution. Did it work out for you fine?
I wonder if I have any local setting making it fail...

src/hooks/useSearchSubmit.ts

alfetopito · 2021-10-15T16:24:56Z

src/utils/miscellaneous.ts

@@ -265,4 +265,4 @@ export const isAnOrderId = (text: string): boolean => text.match(/^0x[a-fA-F0-9]
 *
 * @param text Possible address string to check
 */
-export const isAnAddressAccount = (text: string): boolean => text.match(/^0x[a-fA-F0-9]{40}$/)?.input !== undefined
+export const isAnAddressAccount = (text: string): boolean => text.match(/^0x[a-fA-F0-9]{40}$|[a-zA-Z0-9]+\.[a-zA-Z]+$/)?.input !== undefined


ESlist fix

Suggested change

export const isAnAddressAccount = (text: string): boolean => text.match(/^0x[a-fA-F0-9]{40}$|[a-zA-Z0-9]+\.[a-zA-Z]+$/)?.input !== undefined

export const isAnAddressAccount = (text: string): boolean =>

text.match(/^0x[a-fA-F0-9]{40}$|[a-zA-Z0-9]+\.[a-zA-Z]+$/)?.input !== undefined

Actually, I think would be best to have a separated query only for ENS addresses.

The way you suggest will force all addresses to be queried against the network, but we do not need that since we already know it's an address simply by the regex.

In the future sure, we could have different checks for smart contracts wallets, tokens, contracts, etc, but that's a task for another time :)

Yep, my bad. I added a check in the callback hook that runs the regex again for just the second half (i.e. [a-zA-Z0-9]+\.[a-zA-Z]+$/) to see if it looks like a domain name since an ETH address will never have a period in it so this bypasses the network check when it's just a normal 0x... address.

src/hooks/useSearchSubmit.ts

src/apps/explorer/components/common/Search/index.tsx

acolytec3 · 2021-10-15T17:57:34Z

Also, I was able to get ens resolution working. I just looked on etherscan for a recent transaction with the settlement contract that comes from a ens domain and used that to test

alfetopito · 2021-10-15T22:26:14Z

Also, I was able to get ens resolution working. I just looked on etherscan for a recent transaction with the settlement contract that comes from a ens domain and used that to test

I'm sorry, you are right.
I thought I was using a valid ENS address but turns out I wasn't :)

alfetopito

Indeed, it's working 👍

Some more suggestions below

alfetopito · 2021-10-15T22:18:53Z

src/hooks/useSearchSubmit.ts

@@ -22,8 +23,22 @@ export function useSearchSubmit(): (query: string) => void {
      // Orders, transactions, tokens, batches
      const path = pathAccordingTo(query)
      const pathPrefix = prefixNetwork ? `${prefixNetwork}/${path}` : `${path}`
-
-      query && query.length > 0 && history.push(`/${pathPrefix}/${query}`)
+      if (pathPrefix === 'address' && query.match(/[a-zA-Z0-9]+\.[a-zA-Z]+$/)?.input !== undefined) {


Could you refactor the ENS regex and reuse here and in the isAnAddressAccount helper?

I'm thinking something like:

isEns(query:string) => boolean

Also, I was testing locally and you'd want to compare 'address' against path, not pathPrefix

Requested changes are made and added a test for the new helper.

Let me know if you see anything else. Otherwise, it seems to be working well locally and hopefully I've dealt with any test coverage issues.

alfetopito · 2021-10-15T22:40:05Z

src/hooks/useSearchSubmit.ts

+                history.push(`/address/${res}`)
+              } else {
+                history.push(`/address/${query}`)
+              }
+            })
+            .catch(() => history.push(`/address/${query}`))


Keep the ${pathPrefix} to keep the network in the query.

Unless you want to always ever redirect to mainnet address?

Nope, just an error on my part in understanding how it works. I added the pathPrefix back.

alfetopito

Awesome!

avsavsavs · 2021-10-28T15:53:20Z

Hey @acolytec3 great contribution!!!

please join our Discord (chat.cowswap.exchange) so that MasterCoW can DM you ;)

acolytec3 · 2021-10-28T15:54:25Z

Already there :-)

avsavsavs · 2021-10-28T15:57:13Z

awesome, found you!!

acolytec3 added 2 commits October 15, 2021 11:13

add basic ENS name resolution for address lookup

a2fdf1d

lint fixes

53f7166

anxolin reviewed Oct 15, 2021

View reviewed changes

alfetopito suggested changes Oct 15, 2021

View reviewed changes

acolytec3 added 2 commits October 15, 2021 13:43

requested changes + limit ENS query to non-ETH addresses

f15d526

requested change when ENS resolution returns null string

ce627dd

alfetopito reviewed Oct 15, 2021

View reviewed changes

add isEns helper and test

8d3e5f1

alfetopito approved these changes Oct 18, 2021

View reviewed changes

alfetopito merged commit 35c4176 into gnosis:develop Oct 18, 2021

This was referenced Nov 16, 2021

Release/2.4.0-RC #838

Merged

Release v2.4.0 #858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add basic ENS name resolution for address lookup #771

add basic ENS name resolution for address lookup #771

acolytec3 commented Oct 15, 2021

anxolin left a comment

anxolin Oct 15, 2021 •

edited

Loading

anxolin Oct 15, 2021

acolytec3 Oct 15, 2021

alfetopito left a comment

alfetopito Oct 15, 2021

alfetopito Oct 15, 2021

acolytec3 Oct 15, 2021

acolytec3 commented Oct 15, 2021

alfetopito commented Oct 15, 2021

alfetopito left a comment

alfetopito Oct 15, 2021

alfetopito Oct 15, 2021

acolytec3 Oct 16, 2021

acolytec3 Oct 16, 2021

alfetopito Oct 15, 2021

acolytec3 Oct 16, 2021

alfetopito left a comment

avsavsavs commented Oct 28, 2021

acolytec3 commented Oct 28, 2021 via email •

edited

Loading

avsavsavs commented Oct 28, 2021

add basic ENS name resolution for address lookup #771

add basic ENS name resolution for address lookup #771

Conversation

acolytec3 commented Oct 15, 2021

Summary

To Test

Background

anxolin left a comment

Choose a reason for hiding this comment

anxolin Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfetopito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolytec3 commented Oct 15, 2021

alfetopito commented Oct 15, 2021

alfetopito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfetopito left a comment

Choose a reason for hiding this comment

avsavsavs commented Oct 28, 2021

acolytec3 commented Oct 28, 2021 via email • edited Loading

avsavsavs commented Oct 28, 2021

anxolin Oct 15, 2021 •

edited

Loading

acolytec3 commented Oct 28, 2021 via email •

edited

Loading