Spec update 6 #18
Spec update 6 #18
Conversation
Rename Execution to Call, and update IStandardExecutor
update IPlugin, IPluginManager, IAccountLoupe, BaseModularAccount to match spec update 6
Remove extraneous install/uninstall field checking
…or_faster_test_runs perf: precompile contracts for faster test runs
Post-only hooks & hook refactor
feat: allow overlapping hooks
| AccountStorage storage _storage = getAccountStorage(); | ||
|
|
||
| // Make sure plugin is allowed to spend native token. | ||
| if (value > 0 && value > msg.value && !_storage.pluginData[msg.sender].canSpendNativeToken) { |
There was a problem hiding this comment.
What's the reasoning behind value > msg.value?
Does it mean canSpendNativeToken will be ignored if value <= msg.value?
There was a problem hiding this comment.
This check basically ensures that the plugin is only spending from the account's native token balance if it has permission to do so. If the plugin is managing its own native token balance (outside of the account), and covers the amount sent via executeFromPluginExternal, then it isn't deducting anything from the user's balance and therefore can send a call with value.
| PostExecToRun[] memory postPermittedCallHooks = | ||
| _doPrePermittedCallHooks(IPluginExecutor.executeFromPluginExternal.selector, msg.sender); | ||
| PostExecToRun[] memory postPermittedCallHooks = _doPrePermittedCallHooks( | ||
| getPermittedCallKey(msg.sender, IPluginExecutor.executeFromPluginExternal.selector), msg.data |
There was a problem hiding this comment.
Curious why we use msg.data instead of data here
There was a problem hiding this comment.
The hooks applied to executeFromPluginExternal are tied to the function selector on the account, rather than on the target contract (i.e. you can see in this line that it uses IPluginExecutor.executeFromPluginExternal.selector. ) So, when it is attached to that function, it makes sense to send the calldata for that function call to the hook, rather than the data for the inner call.
By doing this instead of using the inner data field, the hook can be certain that the incoming calldata will be in a predictable format, which is the abi-encoded call to executeFromPluginExternal. It will also get the value to be sent, and which address the account is making the call to, which would not be visible if we just sent data because it is a separate parameter.
There was a problem hiding this comment.
That makes sense, I was wondering why https://github.com/erc6900/reference-implementation/blob/main/src/account/UpgradeableModularAccount.sol#L194 and https://github.com/erc6900/reference-implementation/blob/main/src/account/UpgradeableModularAccount.sol#L202 still stick to data, are there any difference for executeFromPlugin?
There was a problem hiding this comment.
Yes, the difference with executeFromPlugin is that it emulates a call to the fallback, so it needs to use the inner data field. Example:
- Plugin A defines a function
foo(uint256) - Plugin B adds a pre-exec hook over
foo(...)that reads theuint256from calldata and performs a check - Plugin C requests permission to use
foo(...)viaexecuteFromPlugin.
When plugin C uses executeFromPlugin for foo(...), plugin B should get the data for foo(uint256), rather than executeFromPlugin(abi.encodeCall(foo, (...)). If it were to get the second format for the data, then every hook would need two mechanisms to decode calldata rather than just one. By sending the inner data rather than the entire msg.data, it makes it more straightforward to implement hooks.
| // Run any pre exec hooks for this selector | ||
| PostExecToRun[] memory postExecHooks = _doPreExecHooks(IPluginExecutor.executeFromPluginExternal.selector); | ||
| PostExecToRun[] memory postExecHooks = | ||
| _doPreExecHooks(IPluginExecutor.executeFromPluginExternal.selector, msg.data); |
There was a problem hiding this comment.
ditto on the usage of msg.data
There was a problem hiding this comment.
Same reasoning as above for permitted call hooks - the hook function benefits from the standardized format, and gets access to the target address and the value, which are otherwise not visible if we just send data.
Update reference implementation to be fully compliant with updated ERC-6900 with some clarifications and bug fixes.