Error while validating the address: network does support ENS

[LogvinovLeon]

Playing around with ethers.js and testing it's input validation. Started with the this._ethersProvider.getBalance(owner) function. Tried to pass an incorrect ethereum address (0xdeadbeef) and got the network does support ENS error. While this is technically correct (I'm using ganache and it doesn't support ENS) it's not a very useful error message. Maybe we should first check if the address even "looks like" an ENS name before trying to resolve it? I understand that there might be some overlap between invalid ETH addresses and invalid ENS names, but if I remember it right - ENS should at least have a dot in it.

[ricmoo]

We could possibly filter out anything that begins with a 0x, but technically anything is a valid ENS name, even addresses, it is just recommended to no allow addresses.

A dot is also not required, and that requirement could break functuonality. For example, you can resolve “eth” and “xyz” against the ENS contract.

I will look more into this, but think it would be safe to block anything that Starr’s with 0x, I’m fairly certain I’ve seen Wallets do that.

[LogvinovLeon]

As far as I'm concerned "eth" and "xyz" are not valid ENS names. https://docs.ens.domains/en/latest/introduction.html#ens-on-ethereum The ENS name ends with .eth or .test suffix. The wallets I've seen even prepend the .ens suffix for you. https://mycrypto.com/ens

[ricmoo]

It doesn’t say eth is not a valid name on that link. It says mainnet has two top-level domains, but that doesn’t preclude others. eth must be a valid ENS name, because that is how the owner call determines the address of the hash registrar, to register .eth against the registry, which it can do because it is the owner. :)

Also, I personally deploy ENS to my desktop, I deployed eth as a root registrar, but nothing is stopping me from registering everything at the top-level in my test environment.

Perhaps it would make sense to include an ensRegex on each network, for what domains are available.

Overall though, this doesn’t seem like an out-of-place error. Would you be happier if the human text said: “network does not support ENS or invalid address”?

[LogvinovLeon]

I just consider ENS to be an unnecessary feature overall on the level of abstraction that ethers.js operates on. It's nice to have it on UI so that your users don't make mistakes typing addresses, but I don't want to store ENS names in the database or pass them through my app. Inside of my logic I want each and every thing to have one canonical form so that I can make assumptions about it.

That's why I'm also against A | Promise<A>. We're lucky that ens names are strings, but if we go down that road we will reach smth like that:

getBalance(address: string, blockTag: number): BigNumber;
getBalance(address: string|ENSName, blockTag: number): BigNumber;
getBalance(address: Promise<string|ENSName>|string|ENSName, blockTag: number): BigNumber;

And then we'll notice that some people have their BlockTag as string and don't want to convert it to a number. We've already allowed them to do that for ENS and Promises so what's the deal, right?

getBalance(address: Promise<string|ENSName>|string|ENSName, blockTag: number|string|BigNumber|BigInt|BN): BigNumber;

And promises on top of that ;)

getBalance(address: Promise<string|ENSName>|string|ENSName, blockTag: Promise<number|string|BigNumber|BigInt|BN>|number|string|BigNumber|BigInt|BN): BigNumber;

I know - I'm overexadurating. I just want to say that being too tolerant for user input might lead us to exponential test case explosion and less type safety. Now if I accidentally pass my address instead of a block tag it works because it can be a string.

Also we loose the option to write proper validation. It's easy to validate that something is an ethereum address. It's already not that easy to give a helpful error message for an ETH address or ENS name as we see.

The other thing we loose is performance. If you allow A | B everywhere instead of just A then you need to check for B everywhere and convert it to A. While conversions are usefull - checks are not.

I know it's a bigger conversation and I definitely don't expect you to make any of those changes quickly or even agree. Those are breaking changes and big design decisions. I was just given a task of researching the possibility to drop some of the 0x tools in favor of ethers.js. We now have some good user input validation and I wanted to make sure that migrating would not regress that.

I honestly don't understand why do you prefer to add ENS support to each and every function instead of just exposing two functions and allow users to normalize their addresses only on the entrances and exits to their apps.

[fabioberger]

As per the original comment in this issue, @ricmoo I do think “network does not support ENS or invalid address” would be an improvement to the developer experience, thanks for suggesting that.

I also see the bigger issue raised by @LogvinovLeon in his last comment very important to address.

In the case of ENS names specifically, I do think it should be a UI-level consideration, making life easier for end users who don't want to copy-paste / deal with addresses. Once the user has inputted something however, it should be immediately validated & normalized, and the rest of the app logic should deal with it in a single representation. This gives the developer all the benefits @LogvinovLeon listed above. Right before surfacing this input in the UI, it should again be converted to an ENS name. The user is happy, the developer is happy.

I think coming from Javascript, it is very tempting to accept every possible type per input. This makes development easier because the end-developer has no way to reasonably know what type you were expecting given an untyped interface. In Typescript (and every other typed languages), this flexibility comes at a large cost. For you, the maintainer, as well as any developer that is going to use your library.

Would definitely like to hear your thoughts on this in general and whether you agree with the issues raised.

[ricmoo]

So, there are two conversations here, I think? So I'll break it into two parts. I have a LOT of things on the go right now (Firefly demo tomorrow, slide deck for presenting at DevCon, e-mails stacking up, git issues to work on, projects to release, and so on; you know, a normal day :)) though, so I will try to get a quick meaningful answer out, but won't have time to totally collect my thoughts and present them concisely; expect spaghetti. :)

Conversation A: ENS

I consider ENS to be the most fundamental part of Ethereum. I often say "If ENS were the ONLY thing to come out of Ethereum, I would consider it to have been a wild success" (ignoring the obvious economic paradox within that statement). :)

I regard ENS as dynamic linking where as an address is static linking, each with their own pros and cons, which I won't go too much into details here. The benefits of using ENS names in code, allows a situation where updating the contract requires no updating code "in the wild" (in the case of IPFS, the code may not be possible to update without breaking the multihash), and allowing a Contract to own an ENS name, this can be far more secure, ensuring contract links are only updated with some interesting programatic safety guarantees.

Also, not everything has a "UI", nor should it require one. This code is nice, because it is immediately readable, and pretty obvious what is going on, without the additional call to resolveName:

let abi = [
    "function fee() view returns (uint fee)",
    "function register(string name)"
];
let contract = new Contract("registrar.firefly.eth", abi, signer);
let tx = await contract.register("foobar")

I envision a future, where even developers use ENS names almost exclusively in their code, never hard-coding address, or pulling them from a config. Think of how often you put a URL (with a domain name) in your code, compared to an IP address. Using an IP address in code probably raises red-flags in your developer-mind pretty quickly. And you don't wrap calls to an API service in a:

let url = await getHostDns().resolveName("api.twitter.com").then((ipv4) => {
    return "https://" + ipv4 + path;
});

Basically, I consider ENS to be first class, so it should be treated as such, even if it is still early.

Of course, if you need to verify whether something is an address, there are functions for that, utils.getAddress(addr) will throw if the address is invalid. I do use it occasionally in a UI when I need to know. I use my own library a LOT, which is why these features exist; because I use them a LOT. :)

On that note, I would also like to see non-checksum addresses used exclusively, but that requires a much harder push from the ecosystem at large.

Conversation B: Promise | A

It's not so much about accepting every possible type per input, it's more about accepting every type of input that makes sense, is easy to support and improves developers lives (for both reading and writing).

For example, the BigNumber allows a wide variety of inputs, but only inputs that make sense. It saves me a lot of time, code, and makes things more readable:

let one = bigNumberify(1);

let threeEasy = one.add(2);
let threeHard = one.add(bigNumberify(2))

When dealing with providers, there are a lot of times where the parameters I need to pass in are Promises, and all their methods are async Promises anyways, so accepting them as input does not add any complexity (if you look at the source, accepting A | Promise<A> actually reduced the code size, because everything now passes through utils.resovleProperties and be simply wrapped by the other dependent calls). Async begets async.

It also makes code more efficient as more readable code. Consider the following example:

// Case A: How I would do this
let tx = {
    address: "ricmoose.eth",
    gasPrice: provider.getGasPrice(),
    value: 1234,
    nonce: provider.getTransactionCount(myAddress)
};
tx.gasLimit = provider.estimateGas(tx);
signer.sendTransaction(tx);

// CaseB: How many might try to do the same thing without Promise<A> support
let tx = {
    address: "ricmoose.eth",
    gasPrice: await provider.getGasPrice(),
    value: 1234,
    nonce: await provider.getTransactionCount(myAddress)
};
tx.gasLimit = await provider.estimateGas(tx);
signer.sendTransaction(tx);

// May not be obvious, but case B is half the speed of case A; each await had to stall execution

// Case C: Same performance as A, without Promise<A> support
let results = await Promise.all([
    await provider.getGasPrice(),
    await provider.getTransactionCount(myAddress),
]);
let tx = {
    address: "ricmoose.eth",
    gasPrice: result[0],
    value: 1234,
    nonce: result[1]
}
tx.gasLimit = await provider.estimateGas(tx)
signer.sendTransaction(tx);

// I consider Case A to be the most readable. :)

Again, I use my library a LOT which is why I added it in v4, because I found it was such a common pattern I saw again and again. Empirically is has saved me a lot of code size, time and effort.

More importantly, I feel it has made the code more readable.

Anyways, I recommend trying it out. :)

---

I hope that helped explain a bit, I up for more discussion, but it will have to wait until later. Or come find me at DevCon4. :)

[fabioberger]

Thanks for taking the time to write this up @ricmoo, and we should definitely hang out and chat more at DevCon. Looking forward to it!

[ricmoo]

I think for now I'm going to leave the error as-is, but I'm on the fence. I'm going to close this issue for now, and just leave it with having nudged me a bit more in changing the error's localized text. :)