feat: Add ENS lookup methods #136
Conversation
usame-algan
changed the title
|
CLA Assistant Lite All Contributors have signed the CLA. |
|
I have read the CLA Document and I hereby sign the CLA |
| @@ -24,7 +26,7 @@ export interface Web3AdapterConfig { | |||
| } | |||
|
|
|||
| class Web3Adapter implements EthAdapter { | |||
There was a problem hiding this comment.
Adding some context:
EthAdapter is an interface implemented by Web3Adapter and EthersAdapter classes where all the methods are defined in the interface and implemented using Web3 and ethers.js libraries in the 2 classes respectively. This way it doesn't matter if a Dapp uses ethers.js or web3, they will still be able to access the same functionality.
It would be cool if the 2 new methods are included in EthAdapter and EthersAdapter.
| @@ -1,3 +1,5 @@ | |||
| import Web3 from 'web3' | |||
| import { ethers } from 'ethers' | |||
There was a problem hiding this comment.
ethers.js import should not be present in Web3Adapter
|
|
||
| async ensReverseLookup(address: string): Promise<string> { | ||
| const lookup = address.toLowerCase().substr(2) + '.addr.reverse' | ||
| const node = ethers.utils.namehash(lookup) |
There was a problem hiding this comment.
ethers.js library should not be used in Web3Adapter class
There was a problem hiding this comment.
Unfortunately web3 doesn't provide a namehash implementation yet. We could instead include a new package eth-ens-namehash and use it in both places. What do you think?
There was a problem hiding this comment.
@usame-algan, have you checked to see if we use one in the Safe?
There was a problem hiding this comment.
It looks like this is the first place where we need the namehash for the reverse lookup. Ethers can do the reverse lookup without the namehash so this only affects web3 atm.
There was a problem hiding this comment.
@germartinez isn't ethers.js bundled with the Core SDK in any case? Or is there tree-shaking in place?
There was a problem hiding this comment.
eth-ens-namehash is very big too. I think it's better if use ethers temporarily. Web3 will include this function in one of the upcoming versions as per their GitHub.
There was a problem hiding this comment.
Let's not import eth-ens-namehash or ethers.js, but @ethersproject/hash sub-module instead (https://www.npmjs.com/package/@ethersproject/hash?activeTab=readme)
There was a problem hiding this comment.
Good Idea! I removed the eth-ens-namehash and used the sub-module instead.
|
Nice! 😎 |
What it solves
Resolves #3231
How this PR fixes it
Adds two new methods to the Web3Adapter for ens lookup and reverse lookup