-
Notifications
You must be signed in to change notification settings - Fork 406
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
base: master
Are you sure you want to change the base?
Changes from all commits
fe6f138
dc2aa79
df127ff
82df2e9
433e4e0
06551a2
ae2801d
148767f
7ab5e6e
5438496
d2d036b
c3f1d12
54e3be1
f6e25fa
5e7b143
d014bc9
1a58228
38c713c
8051947
b6b00fc
6696059
b526e3d
16c7698
9b7c958
672d507
26b45ce
199e7f7
c19bf4c
94f5305
dfd00db
7f5c408
9940fbc
faf526d
4b6d4db
ffac7c7
b5717d7
2c55ec6
e1f3155
2f03149
092a374
54566b8
2c9bb8c
8ab89d7
11fbea6
83f25a9
21fa7d8
b5938b1
dfcddd9
ccdb370
1fa3a75
c2cbcac
12b98e9
12ba18d
7e03ce2
2ea9a28
f84565b
c08939b
d9a8457
47c98cd
134d776
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,5 +6,5 @@ gasreport-* | |
*.DS_Store | ||
node_modules | ||
build | ||
out/* | ||
forge-cache | ||
out |
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(); | ||
error InvalidTokenAddress(address); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to have two different errors like |
||
} | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks to me that it's just doing single loop. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or do you mean it loops at |
||
|
||
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe better to rename as |
||
|
||
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we still set resolver etc if there are records? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing |
||
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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
); | ||
resolver.functionCall( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
records[i], | ||
"SubdomainRegistrar: Failed to set Record" | ||
); | ||
} | ||
} | ||
|
||
function _checkParent(bytes32 parentNode, uint64 duration) internal view { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think test coverage exists for these cases
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than overriding |
||
} | ||
|
||
if (block.timestamp > expiry) { | ||
revert ParentExpired(parentNode); | ||
} | ||
parentExpiry = expiry; | ||
} catch { | ||
revert ParentNotWrapped(parentNode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
} |
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
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); | ||
} |
There was a problem hiding this comment.
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