feat: first attempt at v2 ETH registrar #15

hiddentao · 2025-02-11T09:04:17Z

No description provided.

contracts/src/registry/ETHRegistrar.sol

contracts/src/registry/IRegistryController.sol

makoto · 2025-02-12T13:22:36Z

Is this now obsolete? https://github.com/ensdomains/namechain/pull/7/files

makoto · 2025-02-12T13:25:46Z

Shouldn't EthRegistrar now be called EthRegistryController?
According to the design doc https://drive.google.com/file/d/1LoA-xboajzUuWwsWpxJ-h1-WqDEsRlkC/view

4.5.3 L2 .eth Registry Controller

ENS v2 Design Doc 16
Similar to the registrar controller in ENS v1, the .eth registry controller will exist
on L2 and facilitate registration and renewals of names by users. As in v1, this
division exists in order to ensure registration and renewal logic, including
pricing, can be updated, while allowing the .eth registry itself to be locked.
Multiple controllers can be authorised by the DAO as needed.

coveralls · 2025-02-13T05:48:57Z

Pull Request Test Coverage Report for Build 13645233447

Details

71 of 74 (95.95%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+6.8%) to 65.419%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contracts/test/ETHRegistrar.t.sol	9	10	90.0%
contracts/src/registry/ETHRegistry.sol	4	6	66.67%

Totals
Change from base Build 13540426288:	6.8%
Covered Lines:	268
Relevant Lines:	402

💛 - Coveralls

contracts/src/registry/ETHRegistrar.sol

…ment

makoto · 2025-02-26T08:13:30Z

Don't we need to implement commit & register on the registry controller?

hiddentao · 2025-02-26T11:33:31Z

Don't we need to implement commit & register on the registry controller?

I'm not sure. @Arachnid ?

Arachnid · 2025-02-27T20:50:16Z

Yes, the controller will still need to have a commit-reveal implementation.

hiddentao · 2025-03-04T03:40:29Z

@Arachnid Got the commit-reveal in there. As for the v1 reverse resolver stuff, should I put that in too?

Arachnid · 2025-03-04T12:59:23Z

contracts/src/registry/ETHRegistrar.sol

+    error CommitmentTooNew(bytes32 commitment);
+    error CommitmentTooOld(bytes32 commitment);


These should probably include the commitment's actual age and the limit.

Arachnid · 2025-03-04T12:59:43Z

contracts/src/registry/ETHRegistrar.sol

+    error CommitmentTooNew(bytes32 commitment);
+    error CommitmentTooOld(bytes32 commitment);
+    error NameNotAvailable(string name);
+    error InsufficientValue();


This should probably include the expected value and the actual value.

Arachnid · 2025-03-04T12:59:56Z

contracts/src/registry/ETHRegistrar.sol

+
+    error MaxCommitmentAgeTooLow();
+    error UnexpiredCommitmentExists(bytes32 commitment);
+    error DurationTooShort(uint64 duration);


This should include the min duration.

Arachnid · 2025-03-04T13:00:50Z

contracts/src/registry/ETHRegistrar.sol

+    uint256 public immutable minCommitmentAge;
+    uint256 public immutable maxCommitmentAge;


We should consider making these mutable, since the contract already has an owner.

Arachnid · 2025-03-04T13:02:30Z

contracts/src/registry/ETHRegistrar.sol

+        address subregistry = address(registry.getSubregistry(name));
+        return subregistry == address(0);


We want to permit users to set their subregistry to 0 for valid names if they don't have subdomains. We should probably check the expiration date, instead.

Arachnid · 2025-03-04T13:03:24Z

contracts/src/registry/ETHRegistrar.sol

+        if (commitments[commitment] + maxCommitmentAge >= block.timestamp) {
+            revert UnexpiredCommitmentExists(commitment);
+        }
+        commitments[commitment] = block.timestamp;


We should emit an event when a commitment is recorded.

Arachnid · 2025-03-04T13:03:39Z

contracts/src/registry/ETHRegistrar.sol

+    function register(
+        string calldata name,
+        address owner,
+        IRegistry subregistry,


It'd be useful to allow setting the resolver at the same time as registering.

Arachnid · 2025-03-04T13:04:32Z

contracts/src/registry/ETHRegistrar.sol

+        IRegistry subregistry,
+        uint96 flags,
+        uint64 duration
+    ) external payable onlyRole(CONTROLLER_ROLE) returns (uint256 tokenId) {


Suggested change

) external payable onlyRole(CONTROLLER_ROLE) returns (uint256 tokenId) {

) external payable returns (uint256 tokenId) {

I think we want this to be publicly callable now, right?

Arachnid · 2025-03-04T13:05:21Z

contracts/src/registry/ETHRegistrar.sol

+     * @param name The name to renew.
+     * @param duration The duration of the renewal.
+     */
+    function renew(string calldata name, uint64 duration) external payable onlyRole(CONTROLLER_ROLE) {


Suggested change

function renew(string calldata name, uint64 duration) external payable onlyRole(CONTROLLER_ROLE) {

function renew(string calldata name, uint64 duration) external payable {

Arachnid · 2025-03-04T13:06:06Z

contracts/src/registry/ETHRegistrar.sol

+            payable(msg.sender).transfer(msg.value - totalPrice);
+        }
+
+        emit NameRenewed(name, duration, tokenId);


This should include the new expiration date.

Arachnid · 2025-03-04T13:07:50Z

As for the v1 reverse resolver stuff, should I put that in too?

No, we'll use batch calls and smart accounts to handle that.

hiddentao added 2 commits February 11, 2025 17:03

feat: first attempt at v2 ETH registrar

1c36b21

Merge branch 'main' into feature/bet-290-implement

dc2b877

hiddentao requested a review from Arachnid February 11, 2025 10:12

Arachnid requested changes Feb 11, 2025

View reviewed changes

contracts/src/registry/ETHRegistrar.sol Outdated Show resolved Hide resolved

contracts/src/registry/ETHRegistrar.sol Outdated Show resolved Hide resolved

contracts/src/registry/IRegistryController.sol Outdated Show resolved Hide resolved

refactor: use oz access control roles for registrar

a3eb66d

hiddentao requested a review from Arachnid February 11, 2025 10:37

Arachnid requested changes Feb 14, 2025

View reviewed changes

contracts/src/registry/ETHRegistrar.sol Show resolved Hide resolved

hiddentao added 5 commits February 16, 2025 13:06

refactor: rename controller and remove access

5f92f9e

Merge remote-tracking branch 'origin/main' into feature/bet-290-imple…

0fc2ddd

…ment

chore: merge from main

1516906

fix: gitmodules

334f222

fix: remove old submodule

688ba8f

hiddentao marked this pull request as ready for review February 16, 2025 05:53

hiddentao requested a review from Arachnid February 16, 2025 08:20

hiddentao added 4 commits February 28, 2025 16:41

refactor: renewed work on registrar to incorporate v1 features

9b495cf

feat: ethregistrar commit-reveal impl

99261b6

Merge branch 'main' into feature/bet-290-implement

4396dcf

test: improved test coverage

6aa8144

Arachnid requested changes Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: first attempt at v2 ETH registrar #15

feat: first attempt at v2 ETH registrar #15

hiddentao commented Feb 11, 2025

makoto commented Feb 12, 2025

makoto commented Feb 12, 2025

coveralls commented Feb 13, 2025 •

edited

Loading

makoto commented Feb 26, 2025

hiddentao commented Feb 26, 2025

Arachnid commented Feb 27, 2025

hiddentao commented Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid Mar 4, 2025

Arachnid commented Mar 4, 2025 •

edited

Loading

		error CommitmentTooNew(bytes32 commitment);
		error CommitmentTooOld(bytes32 commitment);

		uint256 public immutable minCommitmentAge;
		uint256 public immutable maxCommitmentAge;

		address subregistry = address(registry.getSubregistry(name));
		return subregistry == address(0);

	) external payable onlyRole(CONTROLLER_ROLE) returns (uint256 tokenId) {
	) external payable returns (uint256 tokenId) {

	function renew(string calldata name, uint64 duration) external payable onlyRole(CONTROLLER_ROLE) {
	function renew(string calldata name, uint64 duration) external payable {

feat: first attempt at v2 ETH registrar #15

Are you sure you want to change the base?

feat: first attempt at v2 ETH registrar #15

Conversation

hiddentao commented Feb 11, 2025

makoto commented Feb 12, 2025

makoto commented Feb 12, 2025

coveralls commented Feb 13, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13645233447

Details

💛 - Coveralls

makoto commented Feb 26, 2025

hiddentao commented Feb 26, 2025

Arachnid commented Feb 27, 2025

hiddentao commented Mar 4, 2025

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

Arachnid commented Mar 4, 2025 • edited Loading

coveralls commented Feb 13, 2025 •

edited

Loading

Arachnid commented Mar 4, 2025 •

edited

Loading