Upgradeable Module Registry #265
Conversation
contracts/libraries/VersionUtils.sol
Outdated
| library VersionUtils { | ||
|
|
||
| /** | ||
| * @notice This function use to validate the inputted version |
There was a problem hiding this comment.
This function is used to validate the version submitted
satyamakgec
changed the title
|
@adamdossa @pabloruiz55 please review it closely.
|
contracts/ModuleRegistry.sol
Outdated
| require(registry[_moduleFactory] != 0, "ModuleFactory type should not be 0"); | ||
| reputation[_moduleFactory].push(msg.sender); | ||
| emit LogModuleUsed(_moduleFactory, msg.sender); | ||
| uint8[] memory _latestVersion = ISecurityTokenRegistry(getAddress(Encoder.getKey('securityTokenRegistry'))).getProtocolVersion(); |
There was a problem hiding this comment.
I think we should be getting the version of the ST, not the latest version from the STR - i.e.:
uint8[] memory _latestVersion = ISecurityToken(msg.sender).getVersion();
contracts/ModuleRegistry.sol
Outdated
| set(Encoder.getKey('registry', _moduleFactory), uint256(moduleType)); | ||
| set(Encoder.getKey('moduleListIndex', _moduleFactory), uint256(getArrayAddress(Encoder.getKey('moduleList', uint256(moduleType))).length)); | ||
| pushArray(Encoder.getKey('moduleList', uint256(moduleType)), _moduleFactory); | ||
| setArray(Encoder.getKey('reputation', _moduleFactory), new address[](0)); |
There was a problem hiding this comment.
Could you check that we're testing reputation in the test cases? Not sure that we need this line.
There was a problem hiding this comment.
Yes, we are checking the reputation it is only used for the initialization of address[], not needed it will be performed automatically. So removed from here
contracts/ModuleRegistry.sol
Outdated
| moduleList[moduleType][index] = temp; | ||
| moduleListIndex[temp] = index; | ||
| // moduleList[moduleType][index] = temp; | ||
| addressArrayStorage[Encoder.getKey('moduleList', moduleType)][index] = temp; |
There was a problem hiding this comment.
can we use the getters here instead of direct access?
contracts/ModuleRegistry.sol
Outdated
| * @notice Called by moduleFactory owner or registry curator to delete a moduleFactory | ||
| * @param _moduleFactory is the address of the module factory to be deleted | ||
| * @notice Called by the ModuleFactory owner or registry curator to delete a ModuleFactory from the registry | ||
| * @param _moduleFactory is the address of the module factory to be deleted from the registry | ||
| * @return bool | ||
| */ | ||
| function removeModule(address _moduleFactory) external whenNotPaused returns(bool) { |
There was a problem hiding this comment.
I think we can remove the returns(bool) as we always return true (or throw)
contracts/ModuleRegistry.sol
Outdated
| } | ||
| } | ||
|
|
||
| /** | ||
| * @notice Called by moduleFactory owner to register new modules for SecurityToken to use | ||
| * @notice Called by the ModuleFactory owner to register new modules for SecurityTokens to use | ||
| * @param _moduleFactory is the address of the module factory to be registered | ||
| * @return bool | ||
| */ | ||
| function registerModule(address _moduleFactory) external whenNotPaused returns(bool) { |
There was a problem hiding this comment.
Think we can remove returns(bool)
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
(Bug fix, feature, docs update, ...)
make upgradeable ModuleRegistry
What is the current behavior?
(You can also link to an open issue here)
#206
What is the new behavior?
(Define and describe any new functionality. Clarify if this is a feature change)
Does this PR introduce a breaking change?
(What changes might users need to make in their application due to this PR?)
Any Other information: