Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/subdomain registrar #98

Draft
wants to merge 60 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
fe6f138
Add basic subdomain registrar
jefflau May 20, 2022
dc2aa79
Add expiries and fees
jefflau May 24, 2022
df127ff
Add ownerOf to INameWrapper
jefflau May 24, 2022
82df2e9
set beneficiary, set records
jefflau May 24, 2022
433e4e0
Remove prettier
jefflau May 24, 2022
06551a2
Cleanup
jefflau May 24, 2022
ae2801d
Change data structure
jefflau May 25, 2022
148767f
Merge with subdomain-registrar
jefflau Jun 15, 2022
7ab5e6e
Fix compilation of SubdomainRegistrar, fix merge issues with NameWrapper
jefflau Jun 15, 2022
5438496
Merge branch 'wrapper-expire' into feature/subdomain-registrar
jefflau Jun 15, 2022
d2d036b
Fix tests from new interface
jefflau Jun 15, 2022
c3f1d12
Switch to ERC20
jefflau Jun 15, 2022
54e3be1
Add a mock ERC20 contract
jefflau Jun 15, 2022
f6e25fa
Merge branch 'master' into feature/subdomain-registrar
jefflau Jul 4, 2022
5e7b143
Allow subdomains to be registered with any ERC20
jefflau Jul 7, 2022
d014bc9
Add tests for renew
jefflau Jul 7, 2022
1a58228
merge with master
jefflau Jul 12, 2022
38c713c
Add batchRegister subnames
jefflau Jul 12, 2022
8051947
Move transfers into batchRegister. Add tests for balances
jefflau Jul 12, 2022
b6b00fc
Refactor register/renew into internal funcs, add parent reverts
jefflau Jul 13, 2022
6696059
Add ISubdomainRegistrar Interface and clean up tests
jefflau Jul 14, 2022
b526e3d
Merge branch 'master' of github.com:ensdomains/ens-contracts into fea…
jefflau Jul 15, 2022
16c7698
Switch compiler versions
jefflau Jul 15, 2022
9b7c958
Merge with master
jefflau Sep 28, 2022
672d507
merge with getData
jefflau Sep 28, 2022
26b45ce
Begin fixing tests
jefflau Sep 30, 2022
199e7f7
Fix tests for SubdomainRegistrar
jefflau Oct 5, 2022
c19bf4c
Pull out contracts into separate files for rental
jefflau Oct 18, 2022
94f5305
WIP
jefflau Oct 19, 2022
dfd00db
Setup foundry
jefflau Oct 20, 2022
7f5c408
Replicate first test
jefflau Oct 21, 2022
9940fbc
Rewrite js tests in solidity
jefflau Oct 21, 2022
faf526d
Remove forge tests
jefflau Feb 1, 2023
4b6d4db
Fix Subdomain Registrar tests
jefflau Feb 1, 2023
ffac7c7
Move generic funcs into BaseSubdomainRegistrar
jefflau Feb 7, 2023
b5717d7
Update rental tests, change fuses to uint16
jefflau Feb 7, 2023
2c55ec6
Fix expiry on forever subdomain registration
jefflau Feb 8, 2023
e1f3155
Merge branch 'master' of github.com:ensdomains/ens-contracts into fea…
jefflau Feb 15, 2023
2f03149
Add interface to rental contract
jefflau Feb 15, 2023
092a374
Add interface
jefflau Feb 21, 2023
54566b8
Resolve conflicts
jefflau Apr 5, 2023
2c9bb8c
Fix compilation errors
jefflau Apr 5, 2023
8ab89d7
Add protection for unsetup names
jefflau Apr 5, 2023
11fbea6
Adjust tests
jefflau Apr 5, 2023
83f25a9
Test for allowing name owners to extend their own expiry
jefflau Apr 5, 2023
21fa7d8
Remove forge folders
jefflau Apr 7, 2023
b5938b1
Remove func
jefflau Apr 7, 2023
dfcddd9
Add inline struct, do not default active to true
jefflau May 8, 2023
ccdb370
Remove unnecessary check for balanceOf token
jefflau May 8, 2023
1fa3a75
Add tests for DurationTooLong()
jefflau May 8, 2023
c2cbcac
Add event for NameSetup() and add tests
jefflau May 8, 2023
12b98e9
Switch modifier to authorised
jefflau May 8, 2023
12ba18d
Remove modification of expiry variable
jefflau May 8, 2023
7e03ce2
WIP - adding pricer contracts
jefflau May 23, 2023
2ea9a28
Add FixedPricer and fix ForeverSubdomain tests
jefflau Sep 21, 2023
f84565b
Fix all tests for batchRegister/renew
jefflau Sep 26, 2023
c08939b
Refactor naming and add public functiins to interface
jefflau Nov 8, 2023
d9a8457
Update readme
jefflau Nov 8, 2023
47c98cd
Add code snippet examples
jefflau Nov 9, 2023
134d776
Merge branch 'master' into feature/subdomain-registrar
makoto Dec 18, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@ gasreport-*
*.DS_Store
node_modules
build
out/*
forge-cache
out
263 changes: 263 additions & 0 deletions contracts/subdomainregistrar/BaseSubdomainRegistrar.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,263 @@
//SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;
import {INameWrapper, PARENT_CANNOT_CONTROL, IS_DOT_ETH} from "../wrapper/INameWrapper.sol";
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
import {ISubdomainPricer} from "./pricers/ISubdomainPricer.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

error Unavailable();
error Unauthorised(bytes32 node);
error NameNotRegistered();
Copy link
Member

Choose a reason for hiding this comment

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

There is no test case to cover this error

error InvalidTokenAddress(address);
Copy link
Member

Choose a reason for hiding this comment

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

There is no test case to cover this error

error NameNotSetup(bytes32 node);
Copy link
Member

Choose a reason for hiding this comment

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

There is no test case to cover this error

error DataMissing();
error ParentExpired(bytes32 node);
error ParentNotWrapped(bytes32 node);
Copy link
Member

Choose a reason for hiding this comment

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

There is no test case to cover this error

error DurationTooLong(bytes32 node);
error ParentNameNotSetup(bytes32 parentNode);

struct Name {
ISubdomainPricer pricer;
address beneficiary;
bool active;
}

abstract contract BaseSubdomainRegistrar {
Copy link
Member

Choose a reason for hiding this comment

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

Please add natspec comment of what this contract does

mapping(bytes32 => Name) public names;
INameWrapper public immutable wrapper;
using Address for address;

event NameRegistered(bytes32 node, uint256 expiry);
event NameRenewed(bytes32 node, uint256 expiry);
event NameSetup(
bytes32 node,
address pricer,
address beneficiary,
bool active
);

uint64 internal GRACE_PERIOD = 90 days;

constructor(address _wrapper) {
wrapper = INameWrapper(_wrapper);
}

modifier authorised(bytes32 node) {
if (!wrapper.canModifyName(node, msg.sender)) {
revert Unauthorised(node);
}
_;
}
modifier canBeRegistered(bytes32 parentNode, uint64 duration) {
_checkParent(parentNode, duration);
_;
}

function available(bytes32 node) public view virtual returns (bool) {
Copy link
Member

Choose a reason for hiding this comment

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

public functions should have comments in natspec format

try wrapper.getData(uint256(node)) returns (
address,
uint32,
uint64 expiry
) {
return expiry < block.timestamp;
} catch {
return true;
}
}

function _setupDomain(
bytes32 node,
ISubdomainPricer pricer,
address beneficiary,
bool active
) internal virtual authorised(node) {
names[node] = Name({
pricer: pricer,
beneficiary: beneficiary,
active: active
});
emit NameSetup(node, address(pricer), beneficiary, active);
}

function _batchRegister(
bytes32 parentNode,
string[] calldata labels,
address[] calldata addresses,
address resolver,
uint16 fuses,
uint64 duration,
bytes[][] calldata records
) internal {
if (
labels.length != addresses.length || labels.length != records.length
) {
revert DataMissing();
Copy link
Member

Choose a reason for hiding this comment

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

Better to have two different errors like AddressesMissing() and RecordsMissing() so that easier to figure out which data is missing.

}

if (!names[parentNode].active) {
revert ParentNameNotSetup(parentNode);
}

_checkParent(parentNode, duration);

_batchPayBeneficiary(parentNode, labels, duration);

//double loop to prevent re-entrancy because _register calls user supplied functions
Copy link
Member

Choose a reason for hiding this comment

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

Looks to me that it's just doing single loop.

Copy link
Member

Choose a reason for hiding this comment

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

or do you mean it loops at _batchRegister and at _batchPayBeneficiary ? If so, worth mentioning in the comment


for (uint256 i = 0; i < labels.length; i++) {
_register(
parentNode,
labels[i],
addresses[i],
resolver,
fuses,
uint64(block.timestamp) + duration,
records[i]
);
}
}

function register(
bytes32 parentNode,
string calldata label,
address newOwner,
address resolver,
uint32 fuses,
uint64 duration,
bytes[] calldata records
) internal {
if (!names[parentNode].active) {
revert ParentNameNotSetup(parentNode);
}

(address token, uint256 fee) = ISubdomainPricer(
names[parentNode].pricer
).price(parentNode, label, duration);

_checkParent(parentNode, duration);

if (fee > 0) {
IERC20(token).transferFrom(
Copy link
Member

Choose a reason for hiding this comment

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

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

msg.sender,
address(names[parentNode].beneficiary),
fee
);
}

_register(
parentNode,
label,
newOwner,
resolver,
fuses,
uint64(block.timestamp) + duration,
records
);
}

/* Internal Functions */
Copy link
Member

Choose a reason for hiding this comment

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

The register function at l120 is also set as internal so this comment should come above l120 (unless you mistakenly marked register as internal when it was meant for public

Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to rename as _singleRegister to be consistent with _batchRegister


function _register(
bytes32 parentNode,
string calldata label,
address newOwner,
address resolver,
uint32 fuses,
uint64 expiry,
bytes[] calldata records
) internal {
bytes32 node = keccak256(
abi.encodePacked(parentNode, keccak256(bytes(label)))
);

if (!available(node)) {
revert Unavailable();
}

if (records.length > 0) {
wrapper.setSubnodeOwner(
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we still set resolver etc if there are records?

Copy link
Member Author

Choose a reason for hiding this comment

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

We set the resolver in the next function call

parentNode,
label,
address(this),
0,
expiry
);
_setRecords(node, resolver, records);
}

wrapper.setSubnodeRecord(
parentNode,
label,
newOwner,
resolver,
0,
fuses | PARENT_CANNOT_CONTROL, // burn the ability for the parent to control
expiry
);

emit NameRegistered(node, expiry);
}

function _batchPayBeneficiary(
bytes32 parentNode,
string[] calldata labels,
uint64 duration
) internal {
ISubdomainPricer pricer = names[parentNode].pricer;
for (uint256 i = 0; i < labels.length; i++) {
(address token, uint256 price) = pricer.price(
parentNode,
labels[i],
duration
);
Copy link
Member

Choose a reason for hiding this comment

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

Missing if (fee > 0) {} which you do when registering single name

IERC20(token).transferFrom(
msg.sender,
names[parentNode].beneficiary,
price
);
}
}

function _setRecords(
bytes32 node,
address resolver,
bytes[] calldata records
) internal {
for (uint256 i = 0; i < records.length; i++) {
// check first few bytes are namehash
bytes32 txNamehash = bytes32(records[i][4:36]);
require(
txNamehash == node,
"SubdomainRegistrar: Namehash on record do not match the name being registered"
Copy link
Member

Choose a reason for hiding this comment

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

does not match ?

);
resolver.functionCall(

Choose a reason for hiding this comment

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

What does functionCall do?
I couldn't find any references to it on resolver.

Choose a reason for hiding this comment

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

functionCall is implemented by the Open Zeppelin Address library, which makes the .call call on the address. This is achieved by declaring the Address implementation for the address type, using Address for address; at the top of the contract.

records[i],
"SubdomainRegistrar: Failed to set Record"
);
}
}

function _checkParent(bytes32 parentNode, uint64 duration) internal view {
Copy link
Member

Choose a reason for hiding this comment

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

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?)

uint64 parentExpiry;
try wrapper.getData(uint256(parentNode)) returns (
address,
uint32 fuses,
uint64 expiry
) {
if (fuses & IS_DOT_ETH == IS_DOT_ETH) {
expiry = expiry - GRACE_PERIOD;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than overriding expiry , setting a new variable like releasedDate makes more sense

}

if (block.timestamp > expiry) {
revert ParentExpired(parentNode);
}
parentExpiry = expiry;
} catch {
revert ParentNotWrapped(parentNode);
Copy link
Member

Choose a reason for hiding this comment

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

Are there any other ways to determine if the parent is wrapped or not? Assuming any error is unwrapped may miss other unexpected errors

Copy link
Member

Choose a reason for hiding this comment

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

Also there is no test case for this error

}

if (duration + block.timestamp > parentExpiry) {
revert DurationTooLong(parentNode);
}
}
}
69 changes: 69 additions & 0 deletions contracts/subdomainregistrar/ForeverSubdomainRegistrar.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
//SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import {INameWrapper, IS_DOT_ETH, PARENT_CANNOT_CONTROL, CAN_EXTEND_EXPIRY} from "../wrapper/INameWrapper.sol";
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {ERC1155Holder} from "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";
import {BaseSubdomainRegistrar, DataMissing, Unavailable, NameNotRegistered} from "./BaseSubdomainRegistrar.sol";
import {IForeverSubdomainRegistrar} from "./IForeverSubdomainRegistrar.sol";
import {ISubdomainPricer} from "./pricers/ISubdomainPricer.sol";

error ParentNameNotSetup(bytes32 parentNode);
Copy link
Member

Choose a reason for hiding this comment

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

There is no test case to cover this error


contract ForeverSubdomainRegistrar is
BaseSubdomainRegistrar,
ERC1155Holder,
IForeverSubdomainRegistrar
{
constructor(address wrapper) BaseSubdomainRegistrar(wrapper) {}

bytes32 private constant ETH_NODE =
Copy link
Member

Choose a reason for hiding this comment

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

ETH_NODE doesn't not seem to be used anywhere.

0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae;

function register(
bytes32 parentNode,
string calldata label,
address newOwner,
address resolver,
uint16 fuses,
bytes[] calldata records
) public payable {
(, uint32 parentFuses, uint64 expiry) = wrapper.getData(
uint256(parentNode)
);
uint64 duration = expiry - uint64(block.timestamp);
if (parentFuses & IS_DOT_ETH == IS_DOT_ETH) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the expected behavior when parent is not .eth? If duration is 0, does it have no expiry date (or same expiry date as parent)? Would be great if it has description about what's the expected behavior.

duration = duration - GRACE_PERIOD;
}
super.register(
parentNode,
label,
newOwner,
resolver,
CAN_EXTEND_EXPIRY | PARENT_CANNOT_CONTROL | uint32(fuses),
duration,
records
);
}

function setupDomain(
bytes32 node,
ISubdomainPricer pricer,
address beneficiary,
bool active
) public override authorised(node) {
_setupDomain(node, pricer, beneficiary, active);
}

function available(
bytes32 node
)
public
view
override(BaseSubdomainRegistrar, IForeverSubdomainRegistrar)
returns (bool)
{
return super.available(node);
}
}
23 changes: 23 additions & 0 deletions contracts/subdomainregistrar/IForeverSubdomainRegistrar.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
import {ISubdomainPricer} from "./pricers/ISubdomainPricer.sol";

interface IForeverSubdomainRegistrar {
function setupDomain(
bytes32 node,
ISubdomainPricer pricer,
address beneficiary,
bool active
) external;

function register(
bytes32 parentNode,
string calldata label,
address newOwner,
address resolver,
uint16 ownerControlledfuses,
bytes[] calldata records
) external payable;

function available(bytes32 node) external view returns (bool);
}
Loading