Comments in this issue are referring to this PR

• ./contracts/src/node-stake/NodeStakeFactory.sol:17: address internal constant SENTINEL_OWNERS = address(0x1); // @todo add comment why this is needed and why it's set to 0x1 (either here or )
• ./contracts/src/node-stake/NodeStakeFactory.sol:18: bytes32 internal r; // @todo why should this only be internal instead of public? wouldn't this be good to be immutable?
• ./contracts/src/node-stake/NodeStakeFactory.sol:19: bytes internal approvalHashSig; // @todo same as above
• ./contracts/src/node-stake/NodeStakeFactory.sol:39: * @param moduleSingletonAddress singleton contract of Safe @todo is this comment correct? It seems that this is the singleton of the HORP node module? Here it would also be nice to point to the specific implementation which you expect to be used here. I guess it is NodeManagementModule.sol?
• ./contracts/src/node-stake/NodeStakeFactory.sol:41: * @param nonce nonce to create salt @todo if we let users give custome nonce, shouldn't we let them give two nonces for both the node module and the Safe deployment?
• ./contracts/src/node-stake/NodeStakeFactory.sol:44: function clone( // @todo shouldn't we pass the user-defined threshold here also so that we can do the complete Safe setup within this clone function?
• ./contracts/src/node-stake/NodeStakeFactory.sol:45: address moduleSingletonAddress, // @todo it would seem to be consistent to then also pass the safe proxy factory via function parameter instead of hardcoding it in the library. Sure, there's compatibility and security issues but that goes really for everything here. That would also make it easier to e.g. deploy on another chain (unless that's not necessary as proxy factories are deterministically deployed to the same addresses on all L1s/L2s)
• ./contracts/src/node-stake/NodeStakeFactory.sol:49: ) public returns (address, address payable) { // @todo it seems a unnecessarily opinionated to return the second address as payable, shouldn't that casting be done - if really necessary - be performed in the calling function?
• ./contracts/src/node-stake/NodeStakeFactory.sol:58: address moduleProxy = moduleSingletonAddress.cloneDeterministic(salt); // @todo for readability it would be easier to first deploy the Safe and then the module
• ./contracts/src/node-stake/NodeStakeFactory.sol:60: // swap one owner for factory @todo add comment why this owner shuffling and setting threshold to 1 is needed? I guess it is so that we can initialize the modules from this proxy? And I also assume that there is no way to somehow multi-call multiple functions in the initializer?
• ./contracts/src/node-stake/NodeStakeFactory.sol:81: // @todo: it seems this is possible within the constructor of the module (even Safe is deployed deterministically so its address is known ahead of time)
• ./contracts/src/node-stake/NodeStakeFactory.sol:105: function prepareSafeTx(Safe safe, uint256 nonce, bytes memory data) private { // @todo add docstring
• ./contracts/src/node-stake/NodeStakeFactory.sol:111: function predictDeterministicAddress(address implementation, bytes32 salt) public view returns (address predicted) { // @todo add docstring
• ./contracts/src/node-stake/NodeSafeRegistry.sol:33: struct NodeSafe { // @todo: why is this struct needed at all? It seems all functions could be called with two separate parameters instead of gluing them together into a struct that seems to just decrease readability
• ./contracts/src/node-stake/NodeSafeRegistry.sol:71: function registerSafeWithNodeSig(NodeSafe memory nodeSafe, bytes calldata sig) external { // @todo If we decide to remove the struct this would work even more efficiently as we could just passs the Safe address here and obtain the other one from the ECDSA recovery which we anyway do.
• ./contracts/src/node-stake/NodeSafeRegistry.sol:80: // verify that signatures is from nodeChainKeyAddress. This signature can only be @todo comment got cropped somehow
• ./contracts/src/node-stake/NodeSafeRegistry.sol:101: ensureNodeIsSafeModuleMember(NodeSafe({safeAddress: msg.sender, nodeChainKeyAddress: nodeAddr})); // @todo cf comment on struct definition: this notation seems clumsy af, why not just use two separate parameters without struct?
• ./contracts/src/node-stake/NodeSafeRegistry.sol:116: * @dev internal funciton to store safe-node pair and emit events // @todo: this comment shows how the choice of a "safe-node pair" (the struct) obfuscates what we actually have to do here under the hood. We actually just "add node to a safe". IMO the function should be called correspondingly.
• ./contracts/src/node-stake/NodeSafeRegistry.sol:137: nodeToSafe[nodeSafe.nodeChainKeyAddress] = nodeSafe.safeAddress; // @todo: that clumsy notation again!!!
• ./contracts/src/node-stake/NodeSafeRegistry.sol:146: function ensureNodeIsSafeModuleMember(NodeSafe memory nodeSafe) internal view { // @todo most effort here is to search "our module" which seems overly complicated with a bunch of reliances on Safe contract infrastructure that is hard to assess without understanding Safe contracts more deeply. Why can't we just point to the module in another mapping (or point the nodeToSafe to a struct that contains both the Safe and the Module instead of just the Safe address). (I hope the answer is not just "so that we save 20k gas on that extra storage slot" :p)
• ./contracts/src/node-stake/NodeSafeRegistry.sol:151: while (nextModule != SENTINEL_MODULES) { // @todo why stop at this module? pls add comment
• ./contracts/src/node-stake/NodeSafeRegistry.sol:153: (modules, nextModule) = IAvatar(nodeSafe.safeAddress).getModulesPaginated(SENTINEL_MODULES, pageSize); // @todo there is no way to add a module that has not been signed off by owners, correct? Just to ensure that this always terminates without running out of blockgaslimit, this is another boundary condition that is out of scope of HOPR and which makes it hard to review security of this contract - and another argument to remove this search-loop approach altogether.
• ./contracts/src/node-stake/permissioned-module/NodeManagementModule.sol:39: Role internal role; // @todo again a good candidate to make public so that users could more easily understand what is going on
• ./contracts/src/node-stake/permissioned-module/NodeManagementModule.sol:45: modifier nodeOnly() { // @todo What is a trust assumption for which the nodeOnly requirement makes sense? It seems that owners (of the Safe) could simply remove the entire module and upgrade it with a version that simply does not respect that role at all? If that is the case, it just bloats code and we might remove it entirely
• ./contracts/src/node-stake/permissioned-module/NodeManagementModule.sol:58: // @todo it seems all of this can be passed into the constructor instead, why make it a separate function with stateful initialization? That would not just be avoiding the intializer state (and security implications) but also allow all values that are set here to be immutable which would further enforce some security assumptions at compile time
• ./contracts/src/node-stake/permissioned-module/NodeManagementModule.sol:59: // @todo why are parameters passed as bytes instead of normal parameters?
• ./contracts/src/node-stake/permissioned-module/SimplifiedModule.sol:39: * @dev Passes a transaction to be executed by the avatar and returns data. // @todo wtf is an avatar?
• ./contracts/src/node-stake/permissioned-module/CapabilityPermissions.sol:54: // @todo I'd make all these public constants
• ./contracts/src/node-stake/permissioned-module/CapabilityPermissions.sol:196: // @todo what happens when this loop runs out of gas? Does the entire call revert? I'm not sure and asking explicitly as sometimes things get delegatecall-ed and do not revert in the usual way.