FuseController #318
FuseController #318
Conversation
| @@ -3,7 +3,11 @@ | |||
| pragma solidity ^0.8.17; | |||
|
|
|||
| interface IController { | |||
| function ownerOf(bytes calldata tokenData) external view returns (address); | |||
There was a problem hiding this comment.
What is the purpose of this change? Requiring controllers to implement both methods complicates them, and many will have to implement ownerOf simply by calling the registry back to fetch their data from it anywy.
There was a problem hiding this comment.
It saves an external call.
There was a problem hiding this comment.
function ownerOf(bytes32 node) external view returns (address) {
//get the tokenData
bytes memory tokenData = registry.getData(uint256(node));
(address owner, , , , ) = _unpack(tokenData);
return owner;
}
contracts/l2/IControllerUpgrade.sol
Outdated
|
|
||
| import "./IController.sol"; | ||
|
|
||
| interface IControllerUpgrade is IController { |
There was a problem hiding this comment.
Shouldn't this be something like IUpgradeTarget?
|
|
||
| function upgrade(bytes32 node, bytes calldata extraData) external; | ||
|
|
||
| function setUpgradeController( |
There was a problem hiding this comment.
Isn't this an upgrade target, rather than any kind of controller?
There was a problem hiding this comment.
setUpgradeControllerTarget? It's a controller to upgrade to.
| import "@openzeppelin/contracts/interfaces/IERC1155.sol"; | ||
| import "@openzeppelin/contracts/interfaces/IERC165.sol"; | ||
| import "@openzeppelin/contracts/utils/Create2.sol"; | ||
| import "@openzeppelin/contracts/utils/Address.sol"; | ||
| import {IMetadataService} from "../wrapper/IMetadataService.sol"; |
There was a problem hiding this comment.
This should definitely be its own PR. However, I think it makes more sense to generate metadata onchain, to remove the centralised ownership here.
| mapping(address => mapping(address => bool)) approvals; | ||
| mapping(address => uint256) tokenApprovalsNonce; |
There was a problem hiding this comment.
This should also be in a separate PR.
contracts/l2/FuseController.sol
Outdated
| * - Byte 20: owner (address) | ||
| * - Byte 40: resolver (address) | ||
| * _ Byte 60: expiry (uint64) | ||
| * - Byte 68: fuses (uint96) |
There was a problem hiding this comment.
If you reduce this to a uint64, the whole thing will be 96 bytes - saving a storage slot.
| return resolver; | ||
| } | ||
|
|
||
| function expiryOf(bytes32 node) external view returns (uint64) { |
There was a problem hiding this comment.
A generic data method may be more useful here, as well as being more gas efficient.
| revert nameExpired(node); | ||
| } | ||
|
|
||
| // Change the controller to the upgrade contract. |
There was a problem hiding this comment.
You may want to instead allow the upgrade contract to specify the new data, so that a data format migration could take place at the same time as a contract upgrade.
There was a problem hiding this comment.
The upgrade target contract won't have permissions to make any changes if we don't change the controller here? There might be a way to make this more efficient, however.
There was a problem hiding this comment.
Right, but we could have a function on the upgrade contract that we call with the old data, and it returns the new data to set on the registry.
There was a problem hiding this comment.
I don't have an opinion on this, could be either way.
| address(this), | ||
| subnodeOwner, | ||
| subnodeResolver, | ||
| subnodeExpiry, |
There was a problem hiding this comment.
Shouldn't there be some enforcement here that a subnode can't expire after its parent?
| return expiry <= block.timestamp; | ||
| } | ||
|
|
||
| function _unpack( |
There was a problem hiding this comment.
As I mentioned before - just use calldata instead of memory and you don't need assembly.
There was a problem hiding this comment.
Using memory allows for the function ownerOfWithData, which saves an external call.
There was a problem hiding this comment.
Why? ownerOfWithData can specify that its argument is calldata too, and pass it straight to _unpack.
There was a problem hiding this comment.
For example in this function in the controller:
function ownerOf(bytes32 node) external view returns (address) {
//get the tokenData
bytes memory tokenData = registry.getData(uint256(node));
(address owner, , , , ) = _unpack(tokenData);
return owner;
}
I just get the data from the registry and then call _unpack, but tokenData in this case is memory and not calldata.
This is for sure incomplete. I wanted to get some feedback, on the direction, to make sure we are on the same page. I will make changes, and add some more commits. |
nxt3d
force-pushed
the
feature/l2-registry-updates
branch
from
3d8ea04
to
e139421
Compare
Starting with SimpleController, this PR takes some of the features of NameWrapper and integrates them into a new controller called FuseController.