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

Add ENS library #288

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add ENS library #288

wants to merge 6 commits into from

Conversation

AugustoL
Copy link

Add a ENS library to access to ENS Registry information on the smart contract level. It uses the strings library to split the namehash received in string type and calculate the namehash of the ens.
Using the namehash calculated the smart contracts that use this library can get the owner, resolver and ttl of ens directly from the registry.

@Arachnid
Copy link
Member

Arachnid commented Jul 3, 2018

Thanks for the contribution! ENS really needs some solidity-based functionality that's easy for people to use.

The operations on hashes look good. I have some concerns about the operations on strings, however. An integral part of doing ENS resolution from human-readable names is performing UTS46 normalisation. Without it, it's possible to construct multiple identical looking names that hash to different name hashes. I'm concerned that by introducing a function that does name hashing but not normalisation onchain, we risk encouraging people to use this without normalisation anywhere, opening opportunities for phishing and other attacks based on name normalisation. That's why, so far, we've encouraged people to only deal with name hashes onchain, and do hashing and normalisation offchain instead.

@AugustoL
Copy link
Author

AugustoL commented Jul 9, 2018

@Arachnid Yes, that is a problem. Maybe we can add it with a warning message?
On our project (Winding Tree) we will have a list of hotels with ens domains, we can check the resolver of the namehash agains our record of hotel owners, and therefore use it safely, so I see some use for it on projects that can somehow verify resolvers and owners on chain without UTS46 normalisation on chain.

I think adding a good warning message will be enough, but if you want to remove it we can @Arachnid

@Arachnid
Copy link
Member

Arachnid commented Jul 9, 2018

I'd rather not add support at all, unless there are compelling use-cases for namehashing but not normalising on chain. In your application, can you not do the hashing offchain too?

@AugustoL
Copy link
Author

AugustoL commented Jul 9, 2018

Ok, I will remove it. Yes we would be able to do hashing offchain, but im also looking forward to support ens on chain too, I think since we can check it agains our verified registry it will be safe.

Im going to submit the commit removing the strings methods.

@AugustoL
Copy link
Author

AugustoL commented Jul 9, 2018

Should I remove the hashname(string ensName) function too? maybe this can be used on chain with constant strings on the contract, I see a lot of value here since the interface ens ownership can be used here. For example, on my app I have a centralized smart contract that I want to be able to change the address but right now I have to "update" all smart contracts that are using it, if I use ens I can change it only once.

@Arachnid
Copy link
Member

Arachnid commented Jul 9, 2018

Should I remove the hashname(string ensName) function too?

Yes - same problem here, there's no way to ensure that names are normalised, so it'll almost certainly lead to bad bugs.

For example, on my app I have a centralized smart contract that I want to be able to change the address but right now I have to "update" all smart contracts that are using it, if I use ens I can change it only once.

Can you elaborate? Can't you do this by storing the namehash in the contract instead of the plain text name?

@AugustoL
Copy link
Author

AugustoL commented Jul 9, 2018

@Arachnid sure, you can store the namehash, but you can also store a normalised string on the contract storage as constant and get the namehash from there. But I guess this specific usecase is not enough for having the namehash function.

@Arachnid
Copy link
Member

Arachnid commented Jul 9, 2018

@Arachnid sure, you can store the namehash, but you can also store a normalised string on the contract storage as constant and get the namehash from there. But I guess this specific usecase is not enough for having the namehash function.

There's not really any upside to doing that, though - it costs more in storage costs (at least 2 words of storage, instead of one) and in processing (since you have to parse and hash the string each time).

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.

2 participants