Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with ENS names #858

Closed
kremalicious opened this issue Sep 15, 2021 · 8 comments
Closed

Deal with ENS names #858

kremalicious opened this issue Sep 15, 2021 · 8 comments
Labels
Type: Enhancement New feature or request

Comments

@kremalicious
Copy link
Contributor

kremalicious commented Sep 15, 2021

This should work for kicking off profile page fetching but think we might prevent fetching in this case:
https://market-git-feature-accountpage-oceanprotocol.vercel.app/profile/officialoceandao.eth

As a generic solution, if an ETH address is attached to an ENS name, it should take over in all display which is an industry standard as per ENS design guidelines. So instead of 0xxx we show e.g. domain.eth in profile page, user wallet, publisher line. So all logic for showing wallet addresses in UI would be:

ENS name || ETH address

3box names are still valid for the main profile title with this logic:

 3box profile name || ENS name || ETH address

A good start: https://docs.ens.domains/dapp-developer-guide/resolving-names

@kremalicious kremalicious added the Type: Enhancement New feature or request label Sep 15, 2021
@kremalicious kremalicious added this to the Account Profiles milestone Sep 15, 2021
@kremalicious
Copy link
Contributor Author

kremalicious commented Sep 16, 2021

Research findings

  • resolving name.eth → 0xxx is easy: web3.eth.ens.getAddress(‘name.eth’)
  • this is where web3.js ends as it lacks proper support for using ENS names in place of addresses internally
  • also no way for reverse resolution which we need, so 0xxx → name.eth, they keep on auto closing ENS Reverse Resolution is not supported web3/web3.js#3749 without any actual fix
  • proper ENS support is only coming with that new v4 mega rewrite, see [Rewrite 4.x] ENS web3/web3.js#4321

Conclusions

  • use ENS names purely visual, meaning internally we have to keep on sending ETH addresses to web3.js and ocean.js
  • most likely makes sense to keep accountId in our Web3Provider as the ETH address, and introduce a new ensName or something which gets filled when found
  • use ens.js for all ENS needs as that supports reverse resolution, still pending some tests on how big that library is and what dependencies it introduces

@kremalicious
Copy link
Contributor Author

kremalicious commented Sep 16, 2021

haha, not a good start, would be the largest library our market uses, bigger than web3.js. What is wrong with js developers?

Screen Shot 2021-09-16 at 13 21 31

But made it work given that a web3 account is connected in #860. We can't use same mechanism for cases where wallet is not connected though, like for publisher line and ultimately not for account profiles. So looks like we have to rather look into hitting the ENS subgraph to get that info https://thegraph.com/legacy-explorer/subgraph/ensdomains/ens

{
  domains(where: { resolvedAddress: "lowercased_accountId" }) {
    name
  }
}

@kremalicious kremalicious mentioned this issue Sep 16, 2021
@kremalicious
Copy link
Contributor Author

kremalicious commented Sep 16, 2021

Created 2 methods over in #860 and made sure both return exactly the same:
https://github.com/oceanprotocol/market/pull/860/files#diff-81b4b8fa07684c9a5f4f4d93fd28197274a5ad1e83b433d7b8de76c1c3e74709

Tending towards only using getEnsName() hitting the subgraph in all cases, although at least for user account display in menu bar the proper decentralized way would be to use web3 calls with getEnsNamewithWeb3() but given that we need the subgraph variant for the publisher line and profile pages of other accounts, seems overkill only using those huge ENS libraries only for displaying account in menu bar.

@mihaisc
Copy link
Contributor

mihaisc commented Sep 17, 2021

Or we can just try to see what that function does and implement it in the market, if it's reasonable.

@kremalicious
Copy link
Contributor Author

I made almost everything work over in #860 regarding functionality, working nicely but of course we get lots of ens subgraph hits on frontpage as this is what the publisher line now does in addition to 3box. There still might be edge cases like multiple domains attached to one address where there might be differences between the direct web3 call & using subgraph for the user wallet display but can deal with this when real problem arises.

Last remaining use case is adjusting profile fetching logic a bit so that /profile/officialoceandao.eth works as a domain, but as web3.js does not properly support ENS names in place of ETH addresses we have to convert this back in background to ETH address and go from there. Oh, and typing generation but would like to keep this issue here for the conceptional part

@kremalicious
Copy link
Contributor Author

Example for ENS name in publisher line:

Screen Shot 2021-09-17 at 12 11 06

Data Union profile is then good example as they have both, ENS & 3box:

Screen Shot 2021-09-17 at 12 07 12

Decided to only display ENS name in place of address, but keep using ETH address in the explorer links & copy action. Reason being that ENS names will work only on networks where ENS contracts are deployed which right now is only ETH networks, which is why https://etherscan.io/address/dataunion.eth would work, but https://polygonscan.com/address/dataunion.eth does not.

@kremalicious
Copy link
Contributor Author

kremalicious commented Sep 17, 2021

Aaaand covered final use case after some refactoring, essentially creating decentralized vanity URLs for publishers, which kinda covers oceanprotocol/pm#117 too:

Screen Shot 2021-09-17 at 13 29 01

When I find an ENS name attached to an ETH address passed in the URL, then that will get replaced too so in UI the ENS names take over. Pretty neat so far and needs testing which we have as soon as that typing stuff is figured out

@kremalicious
Copy link
Contributor Author

kremalicious commented Sep 17, 2021

there we go, some preview example profiles:

as for original reported URL, this now works and everything on the profile is mapped to resolved address as you can see on the explorer links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants