Fix: Events and AccountLoupe revamp for data availability #122
Conversation
fangting-alchemy
force-pushed
the
fangting/data
branch
from
b8dd333 to
e1a0783
Compare
src/interfaces/IAccountLoupe.sol
Outdated
| // Whether or not a global validation function may be used to validate this function. | ||
| bool allowGlobalValidation; | ||
| // The execution hooks for this function selector. | ||
| bytes32[] executionHooks; |
There was a problem hiding this comment.
These should probably have the HookConfig type, so the caller can interpret them in the same way as they're used in the install interfaces
There was a problem hiding this comment.
Yeah, I was thinking and choose to do the conversion on the SDK to avoid loops on the contract (same reason for the other two fields). Maybe it is worth it for callers not using our SDK.
There was a problem hiding this comment.
Ah I see. I think we can merge the two loops if we want (loop 1 within .values(), and loop 2 in the loupe function). Left a comment with details.
| onUninstallSuccess = _onUninstall(hookModule, hookData); | ||
| hookIndex++; | ||
| } | ||
|
|
||
| for (uint256 i = 0; i < _validationData.permissionHooks.length(); ++i) { | ||
| bytes calldata hookData = hookUninstallDatas[hookIndex]; | ||
| (address hookModule,) = | ||
| ModuleEntityLib.unpack(toModuleEntity(_validationData.permissionHooks.at(i))); | ||
| _onUninstall(hookModule, hookData); | ||
| onUninstallSuccess = _onUninstall(hookModule, hookData); |
There was a problem hiding this comment.
The output of these two calls will be ignored when the value of onUninstallSuccess is overwritten on line 341.
If we want the variable to mean "did any of the calls fail", we can change this to be onUninstallSuccess = onUninstallSuccess && _onUninstall(...);, so that any failure bubbles up as a false.
But maybe we don't want that, since it wouldn't reveal which inner call failed. We could choose to ignore inner onUninstall call failuress instead. Wdyt?
There was a problem hiding this comment.
Good catch.
src/account/AccountLoupe.sol
Outdated
| bytes32[] memory executionHooks = executionData.executionHooks.values(); | ||
| uint256 executionHooksLen = executionHooks.length; | ||
| data.executionHooks = new HookConfig[](executionHooksLen); | ||
| for (uint256 i = 0; i < executionHooksLen; ++i) { | ||
| data.executionHooks[i] = HookConfig.wrap(bytes26(executionHooks[i])); | ||
| } |
There was a problem hiding this comment.
I think we could simplify:
uint256 executionHooksLen = executionHooks.length;
data.executionHooks = new HookConfig[](executionHooksLen);
for (uint256 i = 0; i < executionHooksLen; ++i) {
data.executionHooks[i] = HookConfig.wrap(bytes26(executionData.executionHooks.at(i)));
}Same for the other cases where we use .values() and convert to a different type.
Not totally needed though, just an optimization.
fangting-alchemy
force-pushed
the
fangting/data
branch
from
34e8d8d to
a118f31
Compare
Revamp AccountLoupe and events for data availability