Feature/subdomain registrar #98

jefflau · 2022-07-04T12:34:55Z

Subdomain registrar using the new wrapper expiry.

Uses ERC20 for fees
Allows rentable and free subdomains

makoto · 2022-07-04T19:36:25Z

test/subdomainregistrar/TestSubdomainRegistrar.js

+          86400,
+          []
+        )
+      ).to.be.revertedWith(


FYI, once this PR gets merged, this test will start failing as I enabled throwOnCallFailures: false to be able to test CCIP-read.

Please refer to this part for the fix

makoto · 2022-07-05T07:04:56Z

I wonder if it's worth making compatible with the NDT rental EIP which went it into final status recently https://github.com/ethereum/EIPs/blob/master/EIPS/eip-4907.md

…ture/subdomain-registrar

makoto · 2023-12-21T07:51:24Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+
+const labelhash = (label) => utils.keccak256(utils.toUtf8Bytes(label))
+const ROOT_NODE =
+  '0x0000000000000000000000000000000000000000000000000000000000000000'


ROOT_NODE and EMPTY_BYTES32 are the same so no need to hard code twice. It should rather use const { EMPTY_BYTES32 } = require('../test-utils/constants')

makoto · 2023-12-21T07:52:05Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+const GRACE_PERIOD = 86400 * 90
+
+const EMPTY_BYTES32 =
+  '0x0000000000000000000000000000000000000000000000000000000000000000'


Should use const { EMPTY_BYTES32 } = require('../test-utils/constants')

makoto · 2023-12-21T07:52:24Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+
+const EMPTY_BYTES32 =
+  '0x0000000000000000000000000000000000000000000000000000000000000000'
+const EMPTY_ADDRESS = '0x0000000000000000000000000000000000000000'


Should use const { EMPTY_ADDRESS } = require('../test-utils/constants')

makoto · 2023-12-21T07:53:06Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+const EMPTY_ADDRESS = '0x0000000000000000000000000000000000000000'
+const MAX_EXPIRY = 2n ** 64n - 1n
+
+function increaseTime(delay) {


Should put them in test-utils/constants

makoto · 2023-12-21T07:53:13Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+  return ethers.provider.send('evm_increaseTime', [delay])
+}
+
+function mine() {


Should put them in test-utils/constants

makoto · 2023-12-21T07:54:10Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+      'PublicResolver',
+      EnsRegistry.address,
+      NameWrapper.address,
+      '0x0000000000000000000000000000000000000000',


Use EMPTY_ADDRESS

makoto · 2023-12-21T07:56:11Z

contracts/subdomainregistrar/BaseSubdomainRegistrar.sol

+        _checkParent(parentNode, duration);
+
+        if (fee > 0) {
+            IERC20(token).transferFrom(


Any reason why we only accept fee in ERC20 token, rather than using ETH with existing price oracle?

makoto · 2023-12-21T08:10:01Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+      expect(fuses).to.equal(CAN_EXTEND_EXPIRY | PARENT_CANNOT_CONTROL)
+    })
+
+    it('should not allow subdomains to be registerd over another domain', async () => {


Do you mean 'should not allow subdomains to be registered twice, or should not allow subdomains to be registered when already registered? Wasn't quite sure whatover another domain` mean.

makoto · 2023-12-21T08:14:16Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+
+const labelhash = (label) => utils.keccak256(utils.toUtf8Bytes(label))
+const ROOT_NODE =
+  '0x0000000000000000000000000000000000000000000000000000000000000000'


Please reuse from ../test-utils/constants

makoto · 2023-12-21T08:14:24Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+const ROOT_NODE =
+  '0x0000000000000000000000000000000000000000000000000000000000000000'
+
+const EMPTY_BYTES32 =


Please reuse from ../test-utils/constants

makoto · 2023-12-21T08:14:32Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+
+const EMPTY_BYTES32 =
+  '0x0000000000000000000000000000000000000000000000000000000000000000'
+const EMPTY_ADDRESS = '0x0000000000000000000000000000000000000000'


Please reuse from ../test-utils/constants

makoto · 2023-12-21T08:16:19Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+      BaseRegistrar.address,
+    )
+
+    // setup .xyz


Do you test .xyz domains?

makoto · 2023-12-21T08:17:48Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+    )
+
+    // setup .xyz
+    await EnsRegistry.setSubnodeOwner(


same. Do you test .xyz domains?

makoto · 2023-12-21T08:21:15Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+      expect(expiry).to.equal(block.timestamp + duration)
+      expect(fuses).to.equal(PARENT_CANNOT_CONTROL)
+    })
+    it('should not allow subdomains to be created on unapproved parents', async () => {


Should't the same test exist on TestForeverSubdomainRegistrar.js?

makoto · 2023-12-21T08:22:46Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+      )
+    })
+
+    it('should allow subdomains to be registered without a fee', async () => {


Should't the same test exist on TestForeverSubdomainRegistrar.js?

Or extract common functions between rental and forever registrar into seperate test, or use shouldBehaveLike* pattern you used for NameWrapper tests.

makoto · 2023-12-21T08:23:19Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+      expect(fuses).to.equal(PARENT_CANNOT_CONTROL)
+    })
+
+    it('should revert if user has insufficient balance of the token', async () => {


Should't the same test exist on TestForeverSubdomainRegistrar.js?

makoto · 2023-12-21T08:26:12Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+      ).to.be.revertedWith(`ERC20: transfer amount exceeds balance`)
+    })
+
+    it('should revert if parent is expired', async () => {


Should't the same test exist on TestForeverSubdomainRegistrar.js?

makoto · 2023-12-21T08:29:59Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+      expect(await NameWrapper.ownerOf(subNode)).to.equal(account2)
+    })
+    it('should allow subdomains to be renewed', async () => {
+      const [owner, , expiry] = await NameWrapper.getData(


is owner variable in use?

makoto · 2023-12-21T08:31:18Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+      expect(expiry2.toNumber()).to.equal(expiry.toNumber() + 500)
+    })
+
+    it('should allow subdomains to be renewed even with 0 registration fee', async () => {


Is this actually testing what you want to test? The only difference with the test one above is the duration, not registration fee

makoto · 2023-12-21T08:32:24Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+      expect(expiry2.toNumber()).to.equal(expiry.toNumber() + 86400)
+    })
+
+    it('should revert if expiry is greater than parent', async () => {


what is the parent expiry? If 84600, it would be great to set that into a meaningful variable so that the test is easier to read

makoto · 2023-12-22T01:30:41Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+      // expect(await NameWrapper.ownerOf(subNode)).to.equal(account2)
+      // expect(await NameWrapper.ownerOf(subNode2)).to.equal(account3)
+      // expect(await PublicResolver['addr(bytes32)'](subNode)).to.equal(account2)
+      // expect(await PublicResolver['addr(bytes32)'](subNode2)).to.equal(account3)


Can you either uncomment or delete if not needed?

makoto · 2023-12-22T01:35:04Z

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

+      ).to.be.revertedWith(`Unavailable()`)
+    })
+
+    it('Names can extend their own expiry', async () => {


Is this not possible for RenstalSubdomainRegistrar? If not, there should be a test for that. Otherwise, I am not sure if this test is necessary as it's not extending through subname registrar but rather directly interacting with the name wrapper.

makoto · 2023-12-22T01:47:36Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+        true,
+      )
+      await NameWrapper.setApprovalForAll(SubdomainRegistrar.address, true)
+      await Erc20WithAccount2.approve(


Do you need to approve? Assuming you are testing registration without fee as the next one is explicitly testing with fee

makoto · 2023-12-22T01:49:31Z

test/subdomainregistrar/TestRentalSubdomainRegistrar.js

+        true,
+      )
+      const fee = (await SubdomainRegistrar.names(node)).registrationFee
+      const balanceBefore = await Erc20WithAccount2.balanceOf(account2)


this variable is not in use

makoto · 2023-12-22T01:53:03Z

contracts/subdomainregistrar/ForeverSubdomainRegistrar.sol

+import {IForeverSubdomainRegistrar} from "./IForeverSubdomainRegistrar.sol";
+import {ISubdomainPricer} from "./pricers/ISubdomainPricer.sol";
+
+error ParentNameNotSetup(bytes32 parentNode);


There is no test case to cover this error

makoto · 2023-12-22T01:54:37Z

contracts/subdomainregistrar/BaseSubdomainRegistrar.sol

+
+error Unavailable();
+error Unauthorised(bytes32 node);
+error NameNotRegistered();


There is no test case to cover this error

makoto · 2023-12-22T01:54:57Z

contracts/subdomainregistrar/BaseSubdomainRegistrar.sol

+error Unavailable();
+error Unauthorised(bytes32 node);
+error NameNotRegistered();
+error InvalidTokenAddress(address);


There is no test case to cover this error

makoto · 2023-12-22T01:55:24Z

contracts/subdomainregistrar/BaseSubdomainRegistrar.sol

+error Unauthorised(bytes32 node);
+error NameNotRegistered();
+error InvalidTokenAddress(address);
+error NameNotSetup(bytes32 node);


There is no test case to cover this error

makoto · 2023-12-22T01:55:50Z

contracts/subdomainregistrar/BaseSubdomainRegistrar.sol

+error NameNotSetup(bytes32 node);
+error DataMissing();
+error ParentExpired(bytes32 node);
+error ParentNotWrapped(bytes32 node);


There is no test case to cover this error

makoto · 2023-12-22T02:02:50Z

contracts/subdomainregistrar/BaseSubdomainRegistrar.sol

+        }
+    }
+
+    function _checkParent(bytes32 parentNode, uint64 duration) internal view {


I don't think test coverage exists for these cases

parent name is not wrapped -> Error

parent is NOT .eth (DNS, subname)

parent expiry is in grace period

parent expiry does not exist (subname?)

jefflau added 14 commits May 20, 2022 16:52

Add basic subdomain registrar

fe6f138

Add expiries and fees

dc2aa79

Add ownerOf to INameWrapper

df127ff

set beneficiary, set records

82df2e9

Remove prettier

433e4e0

Cleanup

06551a2

Change data structure

ae2801d

Merge with subdomain-registrar

148767f

Fix compilation of SubdomainRegistrar, fix merge issues with NameWrapper

7ab5e6e

Merge branch 'wrapper-expire' into feature/subdomain-registrar

5438496

Fix tests from new interface

d2d036b

Switch to ERC20

c3f1d12

Add a mock ERC20 contract

54e3be1

Merge branch 'master' into feature/subdomain-registrar

f6e25fa

makoto reviewed Jul 4, 2022

View reviewed changes

jefflau added 2 commits July 7, 2022 23:39

Allow subdomains to be registered with any ERC20

5e7b143

Add tests for renew

d014bc9

jefflau force-pushed the feature/subdomain-registrar branch from c2a5417 to d014bc9 Compare July 7, 2022 16:11

jefflau added 4 commits July 12, 2022 14:53

merge with master

1a58228

Add batchRegister subnames

38c713c

Move transfers into batchRegister. Add tests for balances

8051947

Refactor register/renew into internal funcs, add parent reverts

b6b00fc

jefflau force-pushed the feature/subdomain-registrar branch from d1a9d3e to b6b00fc Compare July 13, 2022 11:07

jefflau and others added 6 commits July 14, 2022 14:30

Add ISubdomainRegistrar Interface and clean up tests

6696059

Merge branch 'master' of github.com:ensdomains/ens-contracts into fea…

b526e3d

…ture/subdomain-registrar

Switch compiler versions

16c7698

Merge with master

9b7c958

merge with getData

672d507

Begin fixing tests

26b45ce

makoto reviewed Dec 21, 2023

View reviewed changes

test/subdomainregistrar/TestForeverSubdomainRegistrar.js

BaseRegistrar.address,

)

// setup .xyz

Copy link

Member

makoto Dec 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you test .xyz domains?

makoto reviewed Dec 21, 2023

View reviewed changes

makoto reviewed Dec 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/subdomain registrar #98

Feature/subdomain registrar #98

jefflau commented Jul 4, 2022 •

edited

Loading

makoto Jul 4, 2022 •

edited

Loading

makoto commented Jul 5, 2022

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023 •

edited

Loading

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 21, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

makoto Dec 22, 2023

Feature/subdomain registrar #98

Are you sure you want to change the base?

Feature/subdomain registrar #98

Conversation

jefflau commented Jul 4, 2022 • edited Loading

makoto Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

makoto commented Jul 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makoto Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefflau commented Jul 4, 2022 •

edited

Loading

makoto Jul 4, 2022 •

edited

Loading

makoto Dec 21, 2023 •

edited

Loading