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.

Sign up for GitHub

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

Jump to bottom

Set subnet owner explicitly + OwnableFacet for Gateway, SA, Registry #785

Merged
merged 7 commits into from
Mar 14, 2024
Merged

Set subnet owner explicitly + OwnableFacet for Gateway, SA, Registry #785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b718ddb
set subnet owner explicitly
Mar 11, 2024
3e2360d
add transfer ownership
Mar 12, 2024
c0264a0
add extra test case; generalize error name.
raulk Mar 13, 2024
bc031d8
ownable facet
Mar 14, 2024
5b1d4f3
merge with upstream
Mar 14, 2024
7b9189d
fix test after merge.
raulk Mar 14, 2024
f2b97c0
aderyn CI check: add ability to suppress specific findings. (#794)
raulk Mar 14, 2024
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
4 changes: 2 additions & 2 deletions contracts/src/SubnetActorDiamond.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add it to the constructor params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, but it seems to go against the grain of it, doesn't it? To have all parameters, plus this on the side?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (params.ipcGatewayAddr == address(0)) {
revert GatewayCannotBeZero();
}
Expand All @@ -58,7 +58,7 @@ contract SubnetActorDiamond {

params.supplySource.validate();

LibDiamond.setContractOwner(msg.sender);
LibDiamond.setContractOwner(owner);
LibDiamond.diamondCut({_diamondCut: _diamondCut, _init: address(0), _calldata: new bytes(0)});

LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
Expand Down
2 changes: 1 addition & 1 deletion contracts/src/subnetregistry/RegisterSubnetFacet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe registry should also accept it as a parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that requires modifying the Rust code as well, so I'll defer to others for what to do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a Ownable contract that handles transfer ownership.


//nonces start with 1, similar to eip 161
++s.userNonces[msg.sender];
Expand Down
6 changes: 3 additions & 3 deletions contracts/test/IntegrationTestBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
})
);

saDiamond = new SubnetActorDiamond(diamondCut, params);
saDiamond = new SubnetActorDiamond(diamondCut, params, address(this));
return saDiamond;
}

Expand Down Expand Up @@ -478,7 +478,7 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,
})
);

SubnetActorDiamond diamond = new SubnetActorDiamond(diamondCut, params);
SubnetActorDiamond diamond = new SubnetActorDiamond(diamondCut, params, address(this));

return diamond;
}
Expand Down Expand Up @@ -555,7 +555,7 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor,

SubnetActorDiamond.ConstructorParams memory params = defaultSubnetActorParamsWith(gw);

SubnetActorDiamond d = new SubnetActorDiamond(diamondCut, params);
SubnetActorDiamond d = new SubnetActorDiamond(diamondCut, params, address(this));

return d;
}
Expand Down
Loading