-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
[3/n permissions] feat: add execute user op #69
Conversation
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.
Looks good overall, had a few questions for changes earlier in the stack. Would be nice to have a test verifying that a hook gets an entire user op, too.
@@ -197,7 +230,7 @@ contract UpgradeableModularAccount is | |||
} | |||
|
|||
PostExecToRun[] memory postExecHooks = | |||
_doPreExecHooks(_storage.selectorData[selector].executionHooks, data); | |||
_doPreHooks(_storage.selectorData[selector].executionHooks, data, false); |
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.
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.
not rebased on top of that yet!
@@ -224,9 +257,10 @@ contract UpgradeableModularAccount is | |||
} | |||
|
|||
// Run any pre exec hooks for this selector | |||
PostExecToRun[] memory postExecHooks = _doPreExecHooks( | |||
PostExecToRun[] memory postExecHooks = _doPreHooks( |
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.
Same here, the executeFromPluginExternal
path should be gone in an earlier PR.
(FunctionReference hookFunction, bool isPreHook, bool isPostHook, bool requireUOContext) = | ||
toExecutionHook(key); | ||
|
||
if (!isPackedUO && requireUOContext) { | ||
revert RequireUserOperationContext(); | ||
} | ||
|
||
if (isPreHook) { | ||
bytes memory preExecHookReturnData = _runPreExecHook(hookFunction, data); | ||
bytes memory preExecHookReturnData; | ||
|
||
if (isPackedUO) { | ||
if (requireUOContext) { | ||
preExecHookReturnData = _runPreExecHook(hookFunction, data); | ||
} else { | ||
PackedUserOperation memory uo = abi.decode(data, (PackedUserOperation)); | ||
preExecHookReturnData = | ||
_runPreExecHook(hookFunction, abi.encodePacked(msg.sender, msg.value, uo.callData)); | ||
} | ||
} else { | ||
preExecHookReturnData = | ||
_runPreExecHook(hookFunction, abi.encodePacked(msg.sender, msg.value, data)); | ||
} |
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.
Slight preference for not using nested if/else branches, and instead early-terminating with a continue;
statement to reduce indent depth, but this is just stylistic. The workflow itself looks good for handling the different possible cases.
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.
The difference is mostly bytecode size by reducing JUMP
s? I think this is more readable, will leave it because its RI, but production accounts should consider making this change
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.
Ah no I think it should be equivalent, I meant it as a readability nit.
b183d5a
to
f3ad732
Compare
7d9f003
to
3c65bc6
Compare
de53ae8
to
8e244e6
Compare
4f8c2dc
to
5d2c4ff
Compare
addresses: erc6900/resources#50 |
addresses erc6900/resources#35 |
changes:
executeUserOp
in UO validationabi.encode(PackedUserOperation)
if UO context is required, else sendabi.encodePacked(msg.sender, msg.value, msg.data)
installValidation
anduninstallValidation