Set subnet owner explicitly + OwnableFacet for Gateway, SA, Registry #785
Conversation
| @@ -38,7 +38,7 @@ contract SubnetActorDiamond { | |||
| SubnetID parentId; | |||
| } | |||
|
|
|||
| constructor(IDiamond.FacetCut[] memory _diamondCut, ConstructorParams memory params) { | |||
| constructor(IDiamond.FacetCut[] memory _diamondCut, ConstructorParams memory params, address owner) { | |||
There was a problem hiding this comment.
Why not add it to the constructor params?
There was a problem hiding this comment.
Initially I thought this way will draw much more attention to this parameter as it's perhaps the single most important parameter. Then, from a practical point of view, it will not break a lot of existing code, it's gentler on ipc and unit tests.
There was a problem hiding this comment.
Hm, but it seems to go against the grain of it, doesn't it? To have all parameters, plus this on the side?
There was a problem hiding this comment.
The owner indeed tends to be a "meta" parameter as it governs the contracts themselves, not the logic of the subnet. We should rename ConstructorParams to SubnetConfig (as it represents the subnet configuration and it's not the totality of the constructor params) to make it clearer down the line.
That said, I would move the owner to the 2nd place to have all contract metadata params leading, then followed by the subnet config.
| @@ -58,7 +58,7 @@ contract RegisterSubnetFacet is ReentrancyGuard { | |||
| }); | |||
|
|
|||
| // slither-disable-next-line reentrancy-benign | |||
| subnetAddr = address(new SubnetActorDiamond(diamondCut, _params)); | |||
| subnetAddr = address(new SubnetActorDiamond(diamondCut, _params, msg.sender)); | |||
There was a problem hiding this comment.
It looks like the registry will set it to the sender, which seems reasonable, however my impression was that the creator should be able to specify a multisig address if they wanted to, so the power over the federated weights aren't in one hand.
There was a problem hiding this comment.
Maybe registry should also accept it as a parameter?
There was a problem hiding this comment.
I understand that requires modifying the Rust code as well, so I'll defer to others for what to do here.
There was a problem hiding this comment.
I'd keep it the way this PR proposes. In a monolithic permissioning model, it's highly conventional in smart contracts that the creator of X becomes the first owner and full controller of X; the creator can be the multisig.
My bet is that we will want a finer-grained permissioning model going forward (e.g. perms for relaying, checkpointing submission, upgrades, lifecycle actions, etc.), so it's not worth to introduce something adhoc here now.
One thing we should do is add the OwnershipFacet to expose an ERC173-compliant transferOwnership method. So one could have an EOA create the subnet, and then transfer ownership to the multisig that will manage it.
There was a problem hiding this comment.
I added a Ownable contract that handles transfer ownership.
There was a problem hiding this comment.
This is fine, but it won't be straightforward to create the subnet with a multisig with our current tooling. We should implement OwnershipFacet so we can reuse our existing tooling (and submit the creation from an EOA), and then transfer ownership to a multisig.
| @@ -58,7 +58,7 @@ contract RegisterSubnetFacet is ReentrancyGuard { | |||
| }); | |||
|
|
|||
| // slither-disable-next-line reentrancy-benign | |||
| subnetAddr = address(new SubnetActorDiamond(diamondCut, _params)); | |||
| subnetAddr = address(new SubnetActorDiamond(diamondCut, _params, msg.sender)); | |||
There was a problem hiding this comment.
I'd keep it the way this PR proposes. In a monolithic permissioning model, it's highly conventional in smart contracts that the creator of X becomes the first owner and full controller of X; the creator can be the multisig.
My bet is that we will want a finer-grained permissioning model going forward (e.g. perms for relaying, checkpointing submission, upgrades, lifecycle actions, etc.), so it's not worth to introduce something adhoc here now.
One thing we should do is add the OwnershipFacet to expose an ERC173-compliant transferOwnership method. So one could have an EOA create the subnet, and then transfer ownership to the multisig that will manage it.
contracts/test/unit/Ownable.t.sol
Outdated
| vm.prank(firstOwner); | ||
| o.transferOwnership(secondOwner); | ||
| require(o.owner() == secondOwner, "second owner not correct"); | ||
| } |
There was a problem hiding this comment.
Need zero address test, since that's a scenario we control for.
contracts/src/GatewayDiamond.sol
Outdated
|
|
||
| error FunctionNotFound(bytes4 _functionSelector); | ||
|
|
||
| bool constant FEATURE_MULTILEVEL_CROSSMSG = false; | ||
| bool constant FEATURE_GENERAL_PUPRPOSE_CROSSMSG = true; | ||
| uint8 constant FEATURE_SUBNET_DEPTH = 2; | ||
|
|
||
| contract GatewayDiamond { | ||
| contract GatewayDiamond is Ownable { |
There was a problem hiding this comment.
Don't we need to add the Ownable methods to the diamond function selectors somehow? Otherwise they might not be externally invokable?
There was a problem hiding this comment.
fyi here's how the diamond Upgrade determines if the user making the request is the "contract owner"
It is using the method LibDiamond.enforceIsContractOwner();the upgrade facet is helpful but it only adds in two methods - one that allows for explicitly changing the ownership (seems helpful and worth adding!) and the other returns the owner. I don't think we want to add is Ownable to the contract here because Owning happens in LibDiamond
|
@cryptoAtwill why break the diamond abstraction model and not go with an |
raulk
changed the title
Currently the ownership of the subnet is the
registry, this PR explicitly sets the ownership as a parameter and controlled by the deployer. The deployer could set this to its own address or some other address instead of forcing it to be themsg.sender.