Simplify ownable <-> controllable inheritance

[VargaElod23]

In the new ens-contracts the inheritance between Controllable and Ownable is weirdly set up with the constructor chain calls. This PR fixes that as well as cleans up usages.

[Arachnid]

Thank you for your submission!

Can you elaborate on why you feel it's important to make this change? Changes to deployed contracts need to go through a staging and release process (documented on the README), and we try and avoid making trivial changes, or doing anything that causes our repositories to fall out of sync with deployed code where possible.

[VargaElod23]

First and foremost, in the current state compilation of the affected contracts fails. The constructor calls are not properly chained.

[Arachnid]

The build is currently passing: https://github.com/ensdomains/ens-contracts/actions/runs/7708372603

If it's not building for you, you must have some difference in your build environment - perhaps a different version of solidity?

[pikonha]

It is affecting me as well, see issue.

[VargaElod23]

@Arachnid Both of us with @pikonha specified a solidity version >0.5 when they restricted inheritance calls. That is the source of error. The target compiler for ens-contracts is 0.4.8 which doesn't have this check and thus doesn't throw the error. However, this would be a pretty low-hanging fruit that would give the contract set forward-compatibility. I think you could introduce this with minimal tests as it doesn't affect much.

[Arachnid]

@VargaElod23 We're definitely not using 0.4.8 for anything other than one obsolete contract. The contracts being amended in this PR have pragmas that won't allow them to be compiled in anything older than 0.8.x.

[pikonha]

The contracts that are causing problems for me are:

UniversalResolver fixed by:

contract UniversalResolver is ERC165, Ownable {
    using Address for address;
    using NameEncoder for string;
    using BytesUtils for bytes;
    using HexUtils for bytes;

    string[] public batchGatewayURLs;
    ENS public immutable registry;

+   constructor(address _registry, string[] memory _urls) Ownable(msg.sender) {
        registry = ENS(_registry);
        batchGatewayURLs = _urls;
    }

    ...
}

Controllable fixed by:

contract Controllable is Ownable {
    mapping(address => bool) public controllers;

    event ControllerChanged(address indexed controller, bool enabled);

+   constructor() Ownable(msg.sender) {}

    ...
}

I then ask: Is the No arguments passed to the base constructor. Specify the arguments or mark "UniversalResolver" as abstract. error the expected behavior?

[Arachnid]

No, it's not the expected behaviour, but it compiles fine using 0.8.x in this repo. It would help if we can track down what is different about your environment that is causing compilation to fail.

[pikonha]

I'm using solc 0.8.23 through Foundry, imported the ens-contracts from GitHub and I'm deploying locally using the following script:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Script, console} from "forge-std/Script.sol";

import "../src/Helper.sol";
import "@ens-contracts/registry/ENSRegistry.sol";
import "@ens-contracts/reverseRegistrar/ReverseRegistrar.sol";
import "@ens-contracts/utils/UniversalResolver.sol";
import {PublicResolver, INameWrapper} from "@ens-contracts/resolvers/PublicResolver.sol";

contract PublicResolverScript is Script, ENSHelper {
    function run() external {
        uint256 privateKey = vm.envUint("PRIVATE_KEY");
        address publicKey = vm.addr(privateKey);
        vm.startBroadcast(privateKey);

        ENSRegistry registry = new ENSRegistry();
        string[] memory urls = new string[](1);
        urls[0] = "localhost:8080";
        new UniversalResolver(address(registry), urls);
        ReverseRegistrar registrar = new ReverseRegistrar(registry);

        ...
}

[Arachnid]

Ah, I see. This isn't an issue with the ENS contracts; you're using an incompatible version of openzeppelin-contracts. ens-contracts currently uses 4.1.0, in which Ownable takes 0 arguments. With version 5, they changed it to take one argument.

I'm open to upgrading to OZ 5, but that would be a separate (and likely larger) PR.

[lcfr-eth]

Install the last version of OZ contracts which are acceptable then set the remappings correctly? I did a mock of the issue and builds as below:

lcfr@pandaemonium testens % forge install OpenZeppelin/openzeppelin-contracts@v4.9.5 lcfr@pandaemonium testens % forge install @ensdomains/ens-contracts
lcfr@pandaemonium testens % forge install ensdomains/buffer

lcfr@pandaemonium testens % cat remappings.txt

ds-test/=lib/forge-std/lib/ds-test/src/
@ens-contracts/=lib/ens-contracts/contracts/
forge-std/=lib/forge-std/src/
@ensdomains/buffer/contracts/=lib/buffer/contracts/
@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/

lcfr@pandaemonium testens % cat src/Helper.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

contract ENSHelper {
    bytes32 public rootNode;

    function namehash(bytes32 hash) public view returns(bytes32) {
    }

    function labelhash(string memory lol) public view returns(bytes32) {
    }

}

lcfr@pandaemonium testens % cat script/test.s.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Script, console} from "forge-std/Script.sol";

import "../src/Helper.sol";
import "@ens-contracts/registry/ENSRegistry.sol";
import "@ens-contracts/reverseRegistrar/ReverseRegistrar.sol";
import {PublicResolver, INameWrapper} from "@ens-contracts/resolvers/PublicResolver.sol";

contract PublicResolverScript is Script, ENSHelper {
    function run() external {
        uint256 privateKey = vm.envUint("PRIVATE_KEY");
        address publicKey = vm.addr(privateKey);
        vm.startBroadcast(privateKey);

        ENSRegistry registry = new ENSRegistry();
        ReverseRegistrar registrar = new ReverseRegistrar(registry);

        // .reverse
        registry.setSubnodeOwner(rootNode, labelhash("reverse"), publicKey);
        // addr.reverse
        registry.setSubnodeOwner(namehash("reverse"), labelhash("addr"), address(registrar));

        PublicResolver resolver = new PublicResolver(registry, INameWrapper(publicKey), publicKey, address(registrar));
        registrar.setDefaultResolver(address(resolver));

        // .eth
        registry.setSubnodeRecord(rootNode, labelhash("eth"), publicKey, address(resolver), 100000);
        // public.eth
        registry.setSubnodeRecord(namehash("eth"), labelhash("public"), publicKey, address(resolver), 100000);

        vm.stopBroadcast();
    }
}

lcfr@pandaemonium testens % forge build [⠢] Compiling... No files changed, compilation skipped

[pikonha]

Ah, I see. This isn't an issue with the ENS contracts; you're using an incompatible version of openzeppelin-contracts. ens-contracts currently uses 4.1.0, in which Ownable takes 0 arguments. With version 5, they changed it to take one argument.

I'm open to upgrading to OZ 5, but that would be a separate (and likely larger) PR.

that indeed worked, really appreciate your attention to this.

[lcfr-eth]

Ah, I see. This isn't an issue with the ENS contracts; you're using an incompatible version of openzeppelin-contracts. ens-contracts currently uses 4.1.0, in which Ownable takes 0 arguments. With version 5, they changed it to take one argument. I'm open to upgrading to OZ 5, but that would be a separate (and likely larger) PR.

that indeed worked, really appreciate your attention to this.

After this thread I decided to create a fork and port all the Tests + Deploy scripts to Foundry.

Why? Foundry offers addition testing mechanisms which i'm unsure if hardhat provides. Things like the built in fuzzer or the ability to easily transform foundry tests into formally verifiable tests for use with pyrometer etc.

The repo is: https://github.com/lcfr-eth/ens-contracts

I started with the NameWrapper tests - will continue / update it overtime. If anyone else feels like contributing to the effort they are welcome to etc. ( Current NameWrapper test file is 5k lines :D )

https://github.com/lcfr-eth/ens-contracts/blob/staging/test/foundry/wrapper/NameWrapper.sol

Eventually it could be merged when complete and shouldnt affect the hardhat build while allowing both foundry + hardhat to compile/test - or I can simply maintain the fork and keep my repo in sync with the ENS repo and add/update foundry tests as additional functionality is added etc.

[Arachnid]

Thanks for the help tracking this down!