[WIP] Session Activity Factory #230
Conversation
[WIP] Session Activity Factory
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| address private _pauser; | ||
|
|
||
| /// @notice The address for the unpauser role on the SessionActivity contract | ||
| address private _unpauser; |
There was a problem hiding this comment.
Looking for specific feedback and suggestions for effective role management for the Factory + deployed contracts.
There was a problem hiding this comment.
would the same pauser and unpauser be used all the time?
What happens is the pauser or unpauser account are compromised?
There was a problem hiding this comment.
Maybe have setPauserAndUnpauser() function?
| for (uint256 i = 0; i < names.length; i++) { | ||
| if (keccak256(abi.encodePacked(names[i])) == keccak256(abi.encodePacked(name))) { | ||
| revert NameAlreadyRegistered(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Something more robust may be needed here. Is Guild Wars the same as Guildwars or GuildWars?
Another consideration: would we ever want to deploy a separate contract for the same game?
There was a problem hiding this comment.
This construct is super gas inefficient:
- There is a for loop
- The bytes32 for keccak256(abi.encodePacked(name)) happens each loop. This is likely to result in a memory expansion for each time through the loop which will cost $$$LOTS
- keccak256(abi.encodePacked(names[i])) is also going to result in lots of memory expansion
There was a problem hiding this comment.
Change this code to:
if (sessionActivityContracts[name] != address(0)) {
revert NameAlreadyRegistered();
}
| mapping(string name => address deployedContract) public sessionActivityContracts; | ||
|
|
||
| /// @notice Array of deployed SessionActivity contracts | ||
| address[] public deployedContracts; |
There was a problem hiding this comment.
This declaration will result in an accessor that returns an array. If there are a lot of values, this could result in a out of gas error (yes, for a view call)
There was a problem hiding this comment.
That is, if we get to 100,000 (or maybe just 10,000) values. If we feel that it is unlikely to ever happen, then it is probably OK. An alternative would be to have a function to return how many contracts have been deployed, and another function to request, starting at contract N, return information for up to M contracts.
| address[] public deployedContracts; | ||
|
|
||
| /// @notice Array of deployed SessionActivity contract names | ||
| string[] public names; |
No description provided.