Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

feat: add support for unstoppable domains minted on polygon #3930

Merged
merged 4 commits into from
Jun 16, 2022

Conversation

0xfuje
Copy link
Contributor

@0xfuje 0xfuje commented Jun 1, 2022

What it solves

Resolves #3831

How this PR fixes it

  • update @unstoppabledomains/resolution to use the latest version (in package.json)
  • modify address resolution function to use new version (in src/logic/wallets/utils/unstoppableDomains.ts)
  • add .nft, .x, .wallet, .bitcoin, .dao, .888, .coin as valid crypto domain names (in src/logic/wallets/ethAddresses.ts)

How to test it

  • go to address book and add UD addresses with new extensions
  • write new UD extension names to the transaction sending input field

Video

IPFS Link

web3wolf added 3 commits June 1, 2022 12:49
- update @unstoppabledomains/resolution in package.json
- full list: .nft, .x, .wallet, .bitcoin, .dao, .888, .coin
@github-actions
Copy link

github-actions bot commented Jun 1, 2022

CLA Assistant Lite All Contributors have signed the CLA.

@0xfuje
Copy link
Contributor Author

0xfuje commented Jun 1, 2022

I have read the CLA Document and I hereby sign the CLA

@katspaugh
Copy link
Member

Looks great, thank you @0xfuje!

One question though: how does it work with different chains?
This feature is currently enabled only on mainnet via a feature toggle, should it remain so?

@katspaugh
Copy link
Member

recheckcla

@github-actions
Copy link

github-actions bot commented Jun 5, 2022

GitMint NFT preview

Thank you for your contribution! Please, let us know your Ethereum address to receive this NFT on Gnosis Chain.
Cheers! 🏆

@coveralls
Copy link

coveralls commented Jun 5, 2022

Pull Request Test Coverage Report for Build 2508355150

  • 2 of 6 (33.33%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 36.848%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/logic/wallets/utils/unstoppableDomains.ts 0 4 0.0%
Totals Coverage Status
Change from base Build 2502858857: 0.002%
Covered Lines: 3864
Relevant Lines: 9533

💛 - Coveralls

@iamacook
Copy link
Member

iamacook commented Jun 7, 2022

One question though: how does it work with different chains?
This feature is currently enabled only on mainnet via a feature toggle, should it remain so?

It seems like there is a multiChainAddr method that accepts the chainId here, but not a custom RPC. I fear this might have hardcoded RPCs internally.

@0xfuje, have you tested this cross-chain?

@0xfuje
Copy link
Contributor Author

0xfuje commented Jun 8, 2022

Hey @katspaugh and @iamacook! I tested it and the resolution works cross-chain. Should it only be enabled to work on mainnet? I think this works with rate-free Infura RPCs internally. Would that be a problem somehow?

@katspaugh
Copy link
Member

I tested it and the resolution works cross-chain.

I'm not sure how this works for UD, but with ENS, if you created a domain on mainnet, it should only resolve on mainnet. E.g. on Polygon, it could resolve to a different address. Is this the case with UD as well? If so, how does it know which chain to query with the chain config removed?

@iamacook
Copy link
Member

iamacook commented Jun 8, 2022

@0xfuje, no chainId is being passed to the UDResolution.addr so I assume it's only resolving on mainnet at the moment? I don't think the currency being used as a chain reference?

@0xfuje
Copy link
Contributor Author

0xfuje commented Jun 9, 2022

I think it first resolves on mainnet, but if no mainnet address has found it resolves on polygon. I will try to dig deeper into the unstoppable resolution library to find out more and to understand it better. Yeah the currency is probably not a reference to the chain it resolves in.

@katspaugh
Copy link
Member

if no mainnet address has found it resolves on polygon

Which still refers to a mainnet address, if I understand right. Please let us know what you find out!

@jim-unstoppable
Copy link

I think it first resolves on mainnet, but if no mainnet address has found it resolves on polygon. I will try to dig deeper into the unstoppable resolution library to find out more and to understand it better. Yeah the currency is probably not a reference to the chain it resolves in.

Yes - the Resolution library will look for a domain on Polygon mainnet, and if it's not found there will check the Ethereum mainnet. This is because the vast majority of UD domains are Polygon NFTs, and all minted domains since fall are now entirely Polygon based. As you see, the backend handles all that logic so the app itself doesn't need to support those chains. The SDK is simply looking to those chains to get the token addresses which the user has configured, they can be any token / chain type.

The resolution library is simply fetching the address and not actually performing the peer to peer send tx, so no chain id is needed to be passed in. This is standard deployment

@katspaugh
Copy link
Member

Thanks for clarifying, @jim-unstoppable.
Do UD only resolve to mainnet addresses though (even though the NFTs themselves are on Polygon)?

@jim-unstoppable
Copy link

Yup this will resolve mainnet addresses, even though the domains are Polygon. Essentially the information is stored on the Polygon NFT, and the API retrieves it. It's same method whether retrieving the ETH address, BTC address, etc - the library grabs the data from the NFT and returns back the address as a string.

@katspaugh
Copy link
Member

Great, thank you @jim-unstoppable!

Then I think this PR is good to go 👍

@francovenica please test crypto|nft|x|wallet|bitcoin|dao|888|coin domains.

@francovenica
Copy link
Contributor

This PR doesn't have an environment to access to, and I cannot fetch the branch. Both things probably because is an external contributor.

@katspaugh
Copy link
Member

@francovenica I've copied it to #3979, please test on there.

@francovenica
Copy link
Contributor

Looks good.
Tested in the 3979, in the Poly network
I tried the names that the contributor shown in his video plus some names I got for myself. They all worked just fine

Tested here:
https://pr3979--safereact.review-safe.gnosisdev.com/app/matic:0x4E5a1650abFD4F9CaBa67f0BE91845541975C860/address-book

Names used:
brad.crypto
vonsassy.x
charlielowndes.nft
bluepenguin.crypto
lulu520.wallet
francopoly1.888
francopoly1.bitcoin
francopoly1.dao

@katspaugh katspaugh merged commit 45a806e into 5afe:dev Jun 16, 2022
@katspaugh
Copy link
Member

Thanks @0xfuje 🙏

@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2022
@0xfuje 0xfuje deleted the update/unstoppable-domains branch June 23, 2022 09:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update version of Unstoppable Domains resolution
6 participants