Skip to content

Plugin refactor #302

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
Merged
merged 15 commits into from
Jan 4, 2023
Merged

Plugin refactor #302

merged 15 commits into from
Jan 4, 2023

Conversation

kumaryash90
Copy link
Member

No description provided.

@kumaryash90 kumaryash90 requested a review from nkrishang January 3, 2023 13:31
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 3, 2023
View reviewed changes
contracts/extension/plugin/Router.sol
Comment on lines +237 to +247
function _removePlugin(bytes4 _selector) internal {
RouterStorage.Data storage data = RouterStorage.routerStorage();
address currentPlugin = _getPluginForFunction(_selector);
require(currentPlugin != address(0), "Router: No plugin available for selector");

delete data.pluginForSelector[_selector];
data.allSelectors.remove(_selector);
data.selectorsForPlugin[currentPlugin].remove(bytes32(_selector));

emit PluginRemoved(_selector, currentPlugin);
}

Check warning

Code scanning / Slither

Unused return

Router._removePlugin(bytes4) (contracts/extension/plugin/Router.sol#237-247) ignores return value by data.allSelectors.remove(_selector) (contracts/extension/plugin/Router.sol#243)
contracts/extension/plugin/Router.sol Outdated
Comment on lines 205 to 217
function _addPlugin(Plugin memory _plugin) internal {
RouterStorage.Data storage data = RouterStorage.routerStorage();
require(data.allSelectors.add(bytes32(_plugin.selector)), "Router: Selector exists");
require(
_plugin.selector == bytes4(keccak256(abi.encodePacked(_plugin.functionString))),
"Router: Incorrect selector"
);

data.pluginForSelector[_plugin.selector] = _plugin;
data.selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.selector));

emit PluginAdded(_plugin.selector, _plugin.pluginAddress);
}

Check warning

Code scanning / Slither

Unused return

Router._addPlugin(IMap.Plugin) (contracts/extension/plugin/Router.sol#205-217) ignores return value by data.selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.selector)) (contracts/extension/plugin/Router.sol#214)
contracts/extension/plugin/Router.sol
Comment on lines +237 to +247
function _removePlugin(bytes4 _selector) internal {
RouterStorage.Data storage data = RouterStorage.routerStorage();
address currentPlugin = _getPluginForFunction(_selector);
require(currentPlugin != address(0), "Router: No plugin available for selector");

delete data.pluginForSelector[_selector];
data.allSelectors.remove(_selector);
data.selectorsForPlugin[currentPlugin].remove(bytes32(_selector));

emit PluginRemoved(_selector, currentPlugin);
}

Check warning

Code scanning / Slither

Unused return

Router._removePlugin(bytes4) (contracts/extension/plugin/Router.sol#237-247) ignores return value by data.selectorsForPlugin[currentPlugin].remove(bytes32(_selector)) (contracts/extension/plugin/Router.sol#244)
contracts/extension/plugin/Router.sol
Comment on lines +18 to +23
function routerStorage() internal pure returns (Data storage routerData) {
bytes32 position = ROUTER_STORAGE_POSITION;
assembly {
routerData.slot := position
}
}

Check warning

Code scanning / Slither

Assembly usage

RouterStorage.routerStorage() (contracts/extension/plugin/Router.sol#18-23) uses assembly - INLINE ASM (contracts/extension/plugin/Router.sol#20-22)
joaquim-verges
joaquim-verges reviewed Jan 3, 2023
View reviewed changes
contracts/extension/plugin/Map.sol Outdated
@@ -5,7 +5,7 @@ import "../interface/plugin/IMap.sol";

import "../../openzeppelin-presets/utils/EnumerableSet.sol";

abstract contract Map is IMap {
contract Map is IMap {
Copy link
Member

Choose a reason for hiding this comment

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

can we call this PluginMap instead to be specific?

joaquim-verges
joaquim-verges reviewed Jan 3, 2023
View reviewed changes
contracts/extension/interface/plugin/IMap.sol Outdated
@@ -15,23 +15,8 @@ interface IMap {
string functionString;
Copy link
Member

Choose a reason for hiding this comment

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

can we call this functionSignature

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 4, 2023
View reviewed changes
contracts/extension/plugin/Router.sol
}

registered = new Plugin[](totalCount);
uint256 index;

Check warning

Code scanning / Slither

Uninitialized local variables

Router.getAllPlugins().index (contracts/extension/plugin/Router.sol#182) is a local variable never initialized
contracts/extension/plugin/Router.sol
}

registered = new bytes4[](count);
uint256 index;

Check warning

Code scanning / Slither

Uninitialized local variables

Router.getAllFunctionsOfPlugin(address).index (contracts/extension/plugin/Router.sol#146) is a local variable never initialized
contracts/extension/plugin/Router.sol
Comment on lines +232 to +246
function _updatePlugin(Plugin memory _plugin) internal {
address currentPlugin = getPluginForFunction(_plugin.functionSelector);
require(
_plugin.functionSelector == bytes4(keccak256(abi.encodePacked(_plugin.functionSignature))),
"Router: fn selector and signature mismatch."
);

RouterStorage.Data storage data = RouterStorage.routerStorage();
data.allSelectors.add(bytes32(_plugin.functionSelector));
data.pluginForSelector[_plugin.functionSelector] = _plugin;
data.selectorsForPlugin[currentPlugin].remove(bytes32(_plugin.functionSelector));
data.selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.functionSelector));

emit PluginUpdated(_plugin.functionSelector, currentPlugin, _plugin.pluginAddress);
}

Check warning

Code scanning / Slither

Unused return

Router._updatePlugin(IPluginMap.Plugin) (contracts/extension/plugin/Router.sol#232-246) ignores return value by data.allSelectors.add(bytes32(_plugin.functionSelector)) (contracts/extension/plugin/Router.sol#240)
contracts/extension/plugin/Router.sol
Comment on lines +232 to +246
function _updatePlugin(Plugin memory _plugin) internal {
address currentPlugin = getPluginForFunction(_plugin.functionSelector);
require(
_plugin.functionSelector == bytes4(keccak256(abi.encodePacked(_plugin.functionSignature))),
"Router: fn selector and signature mismatch."
);

RouterStorage.Data storage data = RouterStorage.routerStorage();
data.allSelectors.add(bytes32(_plugin.functionSelector));
data.pluginForSelector[_plugin.functionSelector] = _plugin;
data.selectorsForPlugin[currentPlugin].remove(bytes32(_plugin.functionSelector));
data.selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.functionSelector));

emit PluginUpdated(_plugin.functionSelector, currentPlugin, _plugin.pluginAddress);
}

Check warning

Code scanning / Slither

Unused return

Router._updatePlugin(IPluginMap.Plugin) (contracts/extension/plugin/Router.sol#232-246) ignores return value by data.selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.functionSelector)) (contracts/extension/plugin/Router.sol#243)
contracts/extension/plugin/Router.sol
Comment on lines +232 to +246
function _updatePlugin(Plugin memory _plugin) internal {
address currentPlugin = getPluginForFunction(_plugin.functionSelector);
require(
_plugin.functionSelector == bytes4(keccak256(abi.encodePacked(_plugin.functionSignature))),
"Router: fn selector and signature mismatch."
);

RouterStorage.Data storage data = RouterStorage.routerStorage();
data.allSelectors.add(bytes32(_plugin.functionSelector));
data.pluginForSelector[_plugin.functionSelector] = _plugin;
data.selectorsForPlugin[currentPlugin].remove(bytes32(_plugin.functionSelector));
data.selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.functionSelector));

emit PluginUpdated(_plugin.functionSelector, currentPlugin, _plugin.pluginAddress);
}

Check warning

Code scanning / Slither

Unused return

Router._updatePlugin(IPluginMap.Plugin) (contracts/extension/plugin/Router.sol#232-246) ignores return value by data.selectorsForPlugin[currentPlugin].remove(bytes32(_plugin.functionSelector)) (contracts/extension/plugin/Router.sol#242)
contracts/extension/plugin/Router.sol
Comment on lines +210 to +229
function _addPlugin(Plugin memory _plugin) internal {
RouterStorage.Data storage data = RouterStorage.routerStorage();

// Revert: default plugin exists for function; use updatePlugin instead.
try IPluginMap(functionMap).getPluginForFunction(_plugin.functionSelector) returns (address) {
revert("Router: default plugin exists for function.");
} catch {
require(data.allSelectors.add(bytes32(_plugin.functionSelector)), "Router: plugin exists for function.");
}

require(
_plugin.functionSelector == bytes4(keccak256(abi.encodePacked(_plugin.functionSignature))),
"Router: fn selector and signature mismatch."
);

data.pluginForSelector[_plugin.functionSelector] = _plugin;
data.selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.functionSelector));

emit PluginAdded(_plugin.functionSelector, _plugin.pluginAddress);
}

Check warning

Code scanning / Slither

Unused return

Router._addPlugin(IPluginMap.Plugin) (contracts/extension/plugin/Router.sol#210-229) ignores return value by IPluginMap(functionMap).getPluginForFunction(_plugin.functionSelector) (contracts/extension/plugin/Router.sol#214-218)
contracts/extension/plugin/PluginMap.sol
Comment on lines +64 to +75
function _setPlugin(Plugin memory _plugin) internal {
require(allSelectors.add(bytes32(_plugin.functionSelector)), "Map: Selector exists");
require(
_plugin.functionSelector == bytes4(keccak256(abi.encodePacked(_plugin.functionSignature))),
"Map: Incorrect selector"
);

pluginForSelector[_plugin.functionSelector] = _plugin;
selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.functionSelector));

emit PluginSet(_plugin.functionSelector, _plugin.functionSignature, _plugin.pluginAddress);
}

Check warning

Code scanning / Slither

Unused return

PluginMap._setPlugin(IPluginMap.Plugin) (contracts/extension/plugin/PluginMap.sol#64-75) ignores return value by selectorsForPlugin[_plugin.pluginAddress].add(bytes32(_plugin.functionSelector)) (contracts/extension/plugin/PluginMap.sol#72)
@kumaryash90 kumaryash90 merged commit ab250a7 into main Jan 4, 2023
@kumaryash90 kumaryash90 deleted the plugin-refactor branch February 14, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joaquim-verges joaquim-verges joaquim-verges left review comments

@nkrishang nkrishang Awaiting requested review from nkrishang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kumaryash90 @joaquim-verges @nkrishang