Add UpgradeableRegistrarController #123

stevieraykatz · 2025-04-16T22:39:42Z

An Upgradeable version of our RegistrarController. Includes the following changes from our existing contract:

All state moved into EIP-7201 compliant slots
"Launch" premium logic removed
Support for ENS L2ReverseRegistrar added for reverse record setting

… new contract

v5.1.0

cb-heimdall · 2025-04-16T22:39:47Z

✅ Heimdall Review Status

Requirement Status More Info

Reviews

✅ 1/1

Denominator calculation

Show calculation

1 if user is bot 0

1 if user is external 0

2 if repo is sensitive 0

From .codeflow.yml 1

Additional review requirements

Show calculation

Max	0

0

From CODEOWNERS 0

Global minimum 0

Max 1

1

1 if commit is unverified 0

Sum 1

src/L2/UpgradeableRegistrarController.sol

stevieraykatz · 2025-05-09T19:31:12Z

src/L2/UpgradeableRegistrarController.sol

+    /// @notice Getter for fetching token expiry.
+    ///
+    /// @dev If the token returns a `0` expiry time, it hasn't been registered before.
+    ///
+    /// @param tokenId The ID of the token to check for expiry.
+    ///
+    /// @return expires Returns the expiry + GRACE_PERIOD for previously registered names, else 0.
+    function _getExpiry(uint256 tokenId) internal view returns (uint256 expires) {
+        expires = _getURCStorage().base.nameExpires(tokenId);
+        return expires == 0 ? 0 : expires + GRACE_PERIOD;
+    }


This method has changed from RegistrarController to eliminate the launch auction logic.

amiecorso

Really just a few nits! Definitely pieces of this system at large I still don't understand but getting there.

amiecorso · 2025-05-27T19:29:51Z

src/L2/UpgradeableRegistrarController.sol

+    /// @param newPaymentReceiver The address of the new payment receiver.
+    event PaymentReceiverUpdated(address newPaymentReceiver);
+
+    /// @notice Emitted when the price oracle is updated.


nit: the "price oracle" is also referred to as the "pricing oracle" and the "prices oracle" at various places in this file, might be good to standardize

amiecorso · 2025-05-27T19:33:33Z

src/L2/UpgradeableRegistrarController.sol

+        view
+        returns (uint256 price)
+    {
+        uint256 discount = _getURCStorage().discounts[discountKey].discount;


just an observation but seems interesting to me that discounts are fixed and independent of the registerPrice for the name v.s. a percentage discounted etc.

src/L2/UpgradeableRegistrarController.sol

ilikesymmetry

Just finished a full pass on source code. You should probably ignore most of my comments that ask for name changes that impact src non-trivially. Didn't have much time to clean up or sort my thinking sorry, but thought the reactions of someone onboarding to the codebase would still be helpful!

Will review tests later, but think we're good to proceed for now :)

src/L2/interface/IL2ReverseRegistrar.sol

src/L2/interface/IReverseRegistrarV2.sol

ilikesymmetry · 2025-05-27T17:35:26Z

src/L2/interface/IReverseRegistrarV2.sol

+    /// @param owner The address to set as the owner of the reverse record in ENS.
+    ///
+    /// @return The ENS node hash of the base-specific reverse record.
+    function claim(address owner) external returns (bytes32);


I don't really understand this function's purpose from the comments. How is it intended to be used compared to setNameForAddrWithSignature?

msg.sender has to be the address that's getting a reverse record set for the specified owner. It's used by contracts that want to claim their name for reverse record setting (where they can delegate management of their reverse record back to owner).

src/L2/interface/IReverseRegistrarV2.sol

ilikesymmetry · 2025-05-27T18:43:47Z

src/L2/UpgradeableRegistrarController.sol

+    ///
+    /// @param request The `RegisterRequest` struct containing the details for the registration.
+    function _register(RegisterRequest calldata request) internal {
+        bytes32 labelhash = keccak256(bytes(request.name));


nit: seen this inconsistently called label and labelhash. If labelhash, would think labelHash follows convention more?

ugh, i agree but fixing this changes events and i'd rather not break things and expand the scope... leaving as is and acknowledging that the nomenclature is confusing.

src/L2/UpgradeableRegistrarController.sol

ilikesymmetry · 2025-05-27T18:46:10Z

src/L2/UpgradeableRegistrarController.sol

+    /// @param label The keccak256 namehash for the specified name.
+    /// @param data  The abi encoded calldata records that will be used in the multicallable resolver.
+    function _setRecords(address resolverAddress, bytes32 label, bytes[] calldata data) internal {
+        bytes32 nodehash = keccak256(abi.encodePacked(_getURCStorage().rootNode, label));


nit: nodeHash?

I also find it interesting that the label is after the parent node. Was the base.eth node also calculated by having the "eth" node before the "base" node?

yeah the order of hashing is a little confusing: https://docs.ens.domains/resolution/names/#algorithm

the spelling of nodehash is consistent with other uses

ilikesymmetry · 2025-05-27T18:48:39Z

src/L2/UpgradeableRegistrarController.sol

+    /// @dev `multicallWithNodeCheck` ensures that each record being set is for the specified `label`.
+    ///
+    /// @param resolverAddress The address of the resolver to set records on.
+    /// @param label The keccak256 namehash for the specified name.


nit: "namehash" intuits immediately while "label" does not from a naming perspective

Though i agree, this is actually incorrect. A namehash == nodehash in ENS parlance. Fixing

ilikesymmetry · 2025-05-27T18:49:55Z

src/L2/UpgradeableRegistrarController.sol

+        uint256[] memory cointypes,
+        bytes memory signature
+    ) internal {
+        _getURCStorage().reverseRegistrar.setNameForAddrWithSignature(msg.sender, expiry, name, cointypes, signature);


I find it a bit odd that owner arg doesn't seem be used here? What is the significants of using msg.sender here?

Also don't notice resolver being used either

Vestigial from old Registrar Controller and unused. Removing them from the helper signature.

They're no longer needed because:

owner is now implicitly tied to the signature data

resolver isn't set because the records are no longer stored in a generic resolver but directly on the canonical L2ReverseResolver.

cb-heimdall · 2025-05-27T19:49:39Z

Review Error for ilikesymmetry @ 2025-05-27 19:49:39 UTC
User failed mfa authentication, see go/mfa-help

src/L2/UpgradeableRegistrarController.sol

This reverts commit 5a956e9.

cb-heimdall · 2025-05-28T01:00:33Z

Review Error for ilikesymmetry @ 2025-05-28 01:00:33 UTC
User failed mfa authentication, see go/mfa-help

src/L2/UpgradeableRegistrarController.sol

stevieraykatz and others added 19 commits October 1, 2024 13:54

Add upgradeable RegistrarController, make it EIP-7201 compliant'

c307e78

Add interface to new ENS contract, add support for setting primary to…

a578e14

… new contract

Add initializer for reverse resolver contract

5dfed4a

Add shim for migration period

43a53a7

Add tests for Shim

fc735bf

lint

7a0d049

Testing start

8151302

chore: forge fmt

1fc8be7

chore: typos

15ebd14

forge install: openzeppelin-contracts-upgradeable

d256095

v5.1.0

feat: use v5.0.0 of openzeppelin-contracts-upgradeable

5214bc2

feat: update tests to use OpenZeppelin Upgradeable

34a313a

chore: fix typo

78e3f28

Update interface to ENS reverse resolver

5e64d7d

Remove launch logic

5f78d9f

remove shim from this branch

d747ca0

Add support for new cointypes array in reverse set

c2bc49f

Add cointypes to test helper

48feaf1

Remove test for removed method

6211b51

github-advanced-security bot found potential problems Apr 16, 2025

View reviewed changes

src/L2/UpgradeableRegistrarController.sol Fixed Show fixed Hide fixed

stevieraykatz added 9 commits April 16, 2025 15:52

Merge branch 'main' into upgradeable-registrar

0e17adf

attempt to fix ci

148cb2c

Cleanup test mocks

eb85c2e

Remove launch logic, cleanup test base

50a2c3f

Fix missing refs from rename

438d626

Remove reference to unused test var

91ce6d5

fix _getExpiry

2b85da9

Fix Register test to use a duration

e35b156

lint

752aeb3

refactor reverse registration to leverage updated reverse registrar v2

0102186

stevieraykatz dismissed amiecorso’s stale review via 0102186 May 7, 2025 21:47

stevieraykatz added 3 commits May 7, 2025 14:59

finish refactor to IReverseRegistrarV2

e48a19d

Fix tests to account for new reverse registrar v2

b5a815c

lint

cce8e36

stevieraykatz requested review from amiecorso and ilikesymmetry May 9, 2025 19:28

stevieraykatz commented May 9, 2025

View reviewed changes

amiecorso previously approved these changes May 27, 2025

View reviewed changes

ilikesymmetry previously approved these changes May 27, 2025

View reviewed changes

stevieraykatz added 2 commits May 27, 2025 13:36

Nits from PR comments

51dc6e9

cleanup tests based on renamings

d8cb250

stevieraykatz dismissed stale reviews from ilikesymmetry and amiecorso via d8cb250 May 27, 2025 20:40

github-advanced-security bot found potential problems May 27, 2025

View reviewed changes

src/L2/UpgradeableRegistrarController.sol Dismissed Show dismissed Hide dismissed

stevieraykatz requested review from amiecorso and ilikesymmetry May 27, 2025 22:09

stevieraykatz added 2 commits May 27, 2025 15:47

update submodules, pin solady version

5a956e9

Revert "update submodules, pin solady version"

a3eb0c8

This reverts commit 5a956e9.

ilikesymmetry previously approved these changes May 28, 2025

View reviewed changes

Natspec and cleanup from PR

73afd8b

stevieraykatz dismissed ilikesymmetry’s stale review via 73afd8b May 28, 2025 06:08

github-advanced-security bot found potential problems May 28, 2025

View reviewed changes

ilikesymmetry self-requested a review May 28, 2025 15:38

Reorder modifiers

ad21ff6

ilikesymmetry approved these changes May 28, 2025

View reviewed changes

stevieraykatz merged commit 2418f4a into main May 28, 2025
7 checks passed

stevieraykatz deleted the upgradeable-registrar branch May 28, 2025 15:53

Add UpgradeableRegistrarController #123

Add UpgradeableRegistrarController #123

Uh oh!

Conversation

stevieraykatz commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cb-heimdall commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Heimdall Review Status

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amiecorso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilikesymmetry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cb-heimdall commented May 27, 2025

Uh oh!

Uh oh!

cb-heimdall commented May 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevieraykatz commented Apr 16, 2025 •

edited

Loading

cb-heimdall commented Apr 16, 2025 •

edited

Loading