feat: [v0.8-develop] Add allowlist sample hook, refactor test base #70
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| // SPDX-License-Identifier: UNLICENSED | ||
| pragma solidity ^0.8.25; | ||
|
|
||
| import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; | ||
|
|
||
| import {PluginMetadata, PluginManifest} from "../../interfaces/IPlugin.sol"; | ||
| import {IValidationHook} from "../../interfaces/IValidationHook.sol"; | ||
| import {IStandardExecutor, Call} from "../../interfaces/IStandardExecutor.sol"; | ||
| import {BasePlugin} from "../../plugins/BasePlugin.sol"; | ||
|
|
||
| contract AllowlistPlugin is IValidationHook, BasePlugin { | ||
| enum FunctionId { | ||
| PRE_VALIDATION_HOOK | ||
| } | ||
|
|
||
| struct AllowlistInit { | ||
| address target; | ||
| bool hasSelectorAllowlist; | ||
| bytes4[] selectors; | ||
| } | ||
|
|
||
| struct AllowlistEntry { | ||
| bool allowed; | ||
| bool hasSelectorAllowlist; | ||
| } | ||
|
|
||
| mapping(address target => mapping(address account => AllowlistEntry)) public targetAllowlist; | ||
| mapping(address target => mapping(bytes4 selector => mapping(address account => bool))) public | ||
| selectorAllowlist; | ||
|
|
||
| error TargetNotAllowed(); | ||
| error SelectorNotAllowed(); | ||
| error NoSelectorSpecified(); | ||
|
|
||
| function onInstall(bytes calldata data) external override { | ||
| AllowlistInit[] memory init = abi.decode(data, (AllowlistInit[])); | ||
|
|
||
| for (uint256 i = 0; i < init.length; i++) { | ||
| targetAllowlist[init[i].target][msg.sender] = AllowlistEntry(true, init[i].hasSelectorAllowlist); | ||
|
|
||
| if (init[i].hasSelectorAllowlist) { | ||
| for (uint256 j = 0; j < init[i].selectors.length; j++) { | ||
| selectorAllowlist[init[i].target][init[i].selectors[j]][msg.sender] = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function onUninstall(bytes calldata data) external override { | ||
| AllowlistInit[] memory init = abi.decode(data, (AllowlistInit[])); | ||
|
|
||
| for (uint256 i = 0; i < init.length; i++) { | ||
| delete targetAllowlist[init[i].target][msg.sender]; | ||
|
|
||
| if (init[i].hasSelectorAllowlist) { | ||
| for (uint256 j = 0; j < init[i].selectors.length; j++) { | ||
| delete selectorAllowlist[init[i].target][init[i].selectors[j]][msg.sender]; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function setAllowlistTarget(address target, bool allowed, bool hasSelectorAllowlist) external { | ||
| targetAllowlist[target][msg.sender] = AllowlistEntry(allowed, hasSelectorAllowlist); | ||
| } | ||
|
|
||
| function setAllowlistSelector(address target, bytes4 selector, bool allowed) external { | ||
| selectorAllowlist[target][selector][msg.sender] = allowed; | ||
| } | ||
|
|
||
| function preUserOpValidationHook(uint8 functionId, PackedUserOperation calldata userOp, bytes32) | ||
| external | ||
| view | ||
| override | ||
| returns (uint256) | ||
| { | ||
| if (functionId == uint8(FunctionId.PRE_VALIDATION_HOOK)) { | ||
| _checkAllowlistCalldata(userOp.callData); | ||
| return 0; | ||
| } | ||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function preRuntimeValidationHook(uint8 functionId, address, uint256, bytes calldata data, bytes calldata) | ||
| external | ||
| view | ||
| override | ||
| { | ||
| if (functionId == uint8(FunctionId.PRE_VALIDATION_HOOK)) { | ||
| _checkAllowlistCalldata(data); | ||
| return; | ||
| } | ||
|
|
||
| revert NotImplemented(); | ||
| } | ||
|
|
||
| function pluginMetadata() external pure override returns (PluginMetadata memory) { | ||
| PluginMetadata memory metadata; | ||
| metadata.name = "Allowlist Plugin"; | ||
| metadata.version = "v0.0.1"; | ||
| metadata.author = "ERC-6900 Working Group"; | ||
|
|
||
| return metadata; | ||
| } | ||
|
|
||
| // solhint-disable-next-line no-empty-blocks | ||
| function pluginManifest() external pure override returns (PluginManifest memory) {} | ||
|
There was a problem hiding this comment. Is this blank because it's supposed to only be installed with a user-provided config? There was a problem hiding this comment. Yep, basically. The manifest is empty because this plugin doesn't define anything that the manifest can install:
|
||
|
|
||
| function _checkAllowlistCalldata(bytes calldata callData) internal view { | ||
| if (bytes4(callData[:4]) == IStandardExecutor.execute.selector) { | ||
|
There was a problem hiding this comment. Callout: when executeUserOp is used, we need to check calldata[4:8] instead There was a problem hiding this comment. If this hook doesn't depend on the contents of There was a problem hiding this comment. yeah agree that it should be abstracted from plugins. in the current permissions stack, we directly send over userOp.callData which could be |
||
| (address target,, bytes memory data) = abi.decode(callData[4:], (address, uint256, bytes)); | ||
| _checkCallPermission(msg.sender, target, data); | ||
| } else if (bytes4(callData[:4]) == IStandardExecutor.executeBatch.selector) { | ||
| Call[] memory calls = abi.decode(callData[4:], (Call[])); | ||
|
|
||
| for (uint256 i = 0; i < calls.length; i++) { | ||
| _checkCallPermission(msg.sender, calls[i].target, calls[i].data); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function _checkCallPermission(address account, address target, bytes memory data) internal view { | ||
| AllowlistEntry storage entry = targetAllowlist[target][account]; | ||
| (bool allowed, bool hasSelectorAllowlist) = (entry.allowed, entry.hasSelectorAllowlist); | ||
|
|
||
| if (!allowed) { | ||
| revert TargetNotAllowed(); | ||
| } | ||
|
|
||
| if (hasSelectorAllowlist) { | ||
| if (data.length < 4) { | ||
| revert NoSelectorSpecified(); | ||
| } | ||
|
|
||
| bytes4 selector = bytes4(data); | ||
|
|
||
| if (!selectorAllowlist[target][selector][account]) { | ||
| revert SelectorNotAllowed(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,63 +3,28 @@ pragma solidity ^0.8.25; | |
|
|
||
| import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; | ||
| import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; | ||
| import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; | ||
|
|
||
| import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; | ||
| import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; | ||
|
|
||
| import {MockAccessControlHookPlugin} from "../mocks/plugins/MockAccessControlHookPlugin.sol"; | ||
| import {Counter} from "../mocks/Counter.sol"; | ||
| import {CustomValidationTestBase} from "../utils/CustomValidationTestBase.sol"; | ||
|
|
||
| contract PerHookDataTest is CustomValidationTestBase { | ||
| using MessageHashUtils for bytes32; | ||
|
|
||
| MockAccessControlHookPlugin internal _accessControlHookPlugin; | ||
|
|
||
| Counter internal _counter; | ||
|
|
||
| function setUp() public { | ||
| _counter = new Counter(); | ||
|
|
||
| _accessControlHookPlugin = new MockAccessControlHookPlugin(); | ||
|
|
||
| _customValidationSetup(); | ||
|
There was a problem hiding this comment. Do the old tests here test allowList fails? There was a problem hiding this comment. Yes, depending on the call path, they throw either user op or runtime errors, and the fuzzer expects them to be called. The error to look for is loaded from here: reference-implementation/test/samples/AllowlistPlugin.t.sol Lines 142 to 199 in 8c7133c And the test expects them here: |
||
| } | ||
|
|
||
| function test_passAccessControl_userOp() public { | ||
|
|
@@ -358,4 +323,28 @@ contract PerHookDataTest is AccountTestBase { | |
|
|
||
| return (userOp, userOpHash); | ||
| } | ||
|
|
||
| // Test config | ||
|
|
||
| function _initialValidationConfig() | ||
| internal | ||
| virtual | ||
| override | ||
| returns (FunctionReference, bool, bytes4[] memory, bytes memory, bytes memory, bytes memory) | ||
| { | ||
| FunctionReference accessControlHook = FunctionReferenceLib.pack( | ||
| address(_accessControlHookPlugin), uint8(MockAccessControlHookPlugin.FunctionId.PRE_VALIDATION_HOOK) | ||
| ); | ||
|
|
||
| FunctionReference[] memory preValidationHooks = new FunctionReference[](1); | ||
| preValidationHooks[0] = accessControlHook; | ||
|
|
||
| bytes[] memory preValidationHookData = new bytes[](1); | ||
| // Access control is restricted to only the counter | ||
| preValidationHookData[0] = abi.encode(_counter); | ||
|
|
||
| bytes memory packedPreValidationHooks = abi.encode(preValidationHooks, preValidationHookData); | ||
|
|
||
| return (_ownerValidation, true, new bytes4[](0), abi.encode(owner1), packedPreValidationHooks, ""); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this check, and the similar one in the runtime validation hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other plugin examples we've been using an if-else chain for the internal function dispatcher, so I was following that pattern here too. I suppose we could cut it and just treat every function id the same here, because this one doesn't have multiple implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the idea of not having the functionId, especially the plugins are global singleton.Actually, we should be explicit since we do ask the
functionIdvar to be passed.