Plugin updates #297
Plugin updates #297
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| constructor(Plugin[] memory _pluginsToRegister) Router(_pluginsToRegister) {} | ||
|
|
||
| /// @dev Returns whether plug-in can be set in the given execution context. | ||
| function _canSetPlugin() internal view override returns (bool) {} |
There was a problem hiding this comment.
Let's make things explicit and return false here.
|
|
||
| function getAllRegistered() external view returns (ExtensionMap[] memory registered); | ||
| function setPlugin(Plugin memory _plugin) external; |
There was a problem hiding this comment.
Let's rename to addPlugin instead of setPlugin.
add makes the job of the function explicit; set is generic.
|
|
||
| function getAllFunctionsOfPlugin(address _pluginAddress) external view returns (bytes4[] memory registered); | ||
|
|
||
| function getAllRegistered() external view returns (Plugin[] memory registered); |
There was a problem hiding this comment.
Let's rename getAllRegistered -> getAllPlugins.
contracts/extension/plugin/Map.sol
Outdated
| @@ -3,61 +3,103 @@ pragma solidity ^0.8.0; | |||
|
|
|||
| import "../interface/plugin/IMap.sol"; | |||
|
|
|||
| import "../Multicall.sol"; | |||
| import "../../openzeppelin-presets/utils/EnumerableSet.sol"; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Let's cleanup comments before merging.
| import "../../openzeppelin-presets/utils/EnumerableSet.sol"; | ||
|
|
||
| /** | ||
| * TODO: | ||
| * - Remove OZ EnumerableSet external dependency. | ||
| */ | ||
|
|
||
| contract Map is IMap, Multicall { | ||
| abstract contract Map is IMap { |
There was a problem hiding this comment.
Add comments demarcating different sections of the contract like in e.g. Router.
contracts/extension/plugin/Map.sol
Outdated
| } | ||
|
|
||
| function _setPlugin(Plugin memory _plugin) internal { | ||
| require(allSelectors.add(bytes32(_plugin.selector)), "REGISTERED"); |
There was a problem hiding this comment.
Now that we're not facing the contract size limit, let's clean up require statement error messages.
Let's follow the rule that error messages begin as "{Contract Name}: ..." so that it is easier to locate where errors are occurring. The error messages themselves don't need to be elaborate.
contracts/extension/plugin/Map.sol
Outdated
|
|
||
| function _setPlugin(Plugin memory _plugin) internal { | ||
| require(allSelectors.add(bytes32(_plugin.selector)), "REGISTERED"); | ||
| require(_plugin.selector == bytes4(keccak256(abi.encodePacked(_plugin.functionString))), "Incorrect selector"); |
There was a problem hiding this comment.
Write consistent error messages; if we're going for all caps single word messages e.g. REGISTERED, let's keep it consistent across the contract; same applies if we opt to go for phrases e.g. Map: Incorrect selector. (preferred).
contracts/extension/plugin/Map.sol
Outdated
|
|
||
| return ext; | ||
| pluginForSelector[_plugin.selector] = _plugin; | ||
| selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.selector)); |
There was a problem hiding this comment.
Put this add call in a require statement.
If updatePlugin is called twice with the same Plugin struct info, this add call will fail silently and the following remove call will remove the selector as from the selectorsForPlugin set, thus, tainting data / state of the contract.
There was a problem hiding this comment.
Or, you can reverse the order of the add and remove calls.
contracts/extension/plugin/Map.sol
Outdated
| uint256 len = allSelectors.length(); | ||
| functionExtensionPairs = new ExtensionMap[](len); | ||
| functionExtensionPairs = new Plugin[](len); |
There was a problem hiding this comment.
Replace all extension vocabulary with plugin vocabulary, even for variables e.g. functionExtensionPairs.
| function _updatePlugin(Plugin memory _plugin) internal { | ||
| address currentPlugin = getPluginForFunction(_plugin.selector); | ||
| require( | ||
| _plugin.selector == bytes4(keccak256(abi.encodePacked(_plugin.functionString))), | ||
| "Map: Incorrect selector" | ||
| ); | ||
|
|
||
| pluginForSelector[_plugin.selector] = _plugin; | ||
| selectorsForPlugin[currentPlugin].remove(bytes32(_plugin.selector)); | ||
| selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.selector)); | ||
|
|
||
| emit PluginUpdated(_plugin.selector, currentPlugin, _plugin.pluginAddress); | ||
| } |
Check warning
Code scanning / Slither
Unused return
| function _updatePlugin(Plugin memory _plugin) internal { | ||
| address currentPlugin = getPluginForFunction(_plugin.selector); | ||
| require( | ||
| _plugin.selector == bytes4(keccak256(abi.encodePacked(_plugin.functionString))), | ||
| "Map: Incorrect selector" | ||
| ); | ||
|
|
||
| pluginForSelector[_plugin.selector] = _plugin; | ||
| selectorsForPlugin[currentPlugin].remove(bytes32(_plugin.selector)); | ||
| selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.selector)); | ||
|
|
||
| emit PluginUpdated(_plugin.selector, currentPlugin, _plugin.pluginAddress); | ||
| } |
Check warning
Code scanning / Slither
Unused return
| function _addPlugin(Plugin memory _plugin) internal { | ||
| require(allSelectors.add(bytes32(_plugin.selector)), "Map: Selector exists"); | ||
| require( | ||
| _plugin.selector == bytes4(keccak256(abi.encodePacked(_plugin.functionString))), | ||
| "Map: Incorrect selector" | ||
| ); | ||
|
|
||
| pluginForSelector[_plugin.selector] = _plugin; | ||
| selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.selector)); | ||
|
|
||
| emit PluginAdded(_plugin.selector, _plugin.pluginAddress); | ||
| } |
Check warning
Code scanning / Slither
Unused return
| function _removePlugin(bytes4 _selector) internal { | ||
| address currentPlugin = getPluginForFunction(_selector); | ||
|
|
||
| delete pluginForSelector[_selector]; | ||
| allSelectors.remove(_selector); | ||
| selectorsForPlugin[currentPlugin].remove(bytes32(_selector)); | ||
|
|
||
| emit PluginRemoved(_selector, currentPlugin); | ||
| } |
Check warning
Code scanning / Slither
Unused return
| function _removePlugin(bytes4 _selector) internal { | ||
| address currentPlugin = getPluginForFunction(_selector); | ||
|
|
||
| delete pluginForSelector[_selector]; | ||
| allSelectors.remove(_selector); | ||
| selectorsForPlugin[currentPlugin].remove(bytes32(_selector)); | ||
|
|
||
| emit PluginRemoved(_selector, currentPlugin); | ||
| } |
Check warning
Code scanning / Slither
Unused return
No description provided.