feat: [v0.8-develop] HookConfig install parameter & internal structure 4/N #109
Conversation
adamegyed
force-pushed
the
adam/hook-config
branch
2 times, most recently
from
bfebb10 to
de4125c
Compare
| function packExecHook(address _module, uint32 _entityId, bool _hasPre, bool _hasPost) | ||
| internal | ||
| pure | ||
| returns (HookConfig) | ||
| { | ||
| return HookConfig.wrap( | ||
| bytes26( | ||
| // module address stored in the first 20 bytes | ||
| bytes26(bytes20(_module)) | ||
| // entityId stored in the 21st - 24th byte | ||
| | bytes26(bytes24(uint192(_entityId))) | ||
| // | bytes26(_HOOK_TYPE_EXEC) // Can omit because exec type is 0 | ||
| | bytes26(_hasPre ? _EXEC_HOOK_HAS_PRE : bytes32(0)) | ||
| | bytes26(_hasPost ? _EXEC_HOOK_HAS_POST : bytes32(0)) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Do we need this function? Seems duplicated code.
This can be achieved through the above function:
packExecHook(ModuleEntityLib.pack(module, entityId), hasPre, hasPost)
There was a problem hiding this comment.
Yeah It's a bit of a duplicate, but it's just for convenience to the caller so they don't have to import two libraries in some cases. I think this function is only linked in some tests, so it doesn't increase the bytecode size for the actual contracts.
adamegyed
force-pushed
the
adam/hook-config
branch
from
de4125c to
167bfcb
Compare
adamegyed
force-pushed
the
adam/manifest-remove-validation
branch
from
04132e0 to
22f26cf
Compare
adamegyed
force-pushed
the
adam/hook-config
branch
from
167bfcb to
ee67d9e
Compare
adamegyed
force-pushed
the
adam/hook-config
branch
from
f5bdd06 to
c8b31e5
Compare
| // Post hook has 1 in 1's bit in the 26th byte | ||
| bytes32 internal constant _EXEC_HOOK_HAS_POST = bytes32(uint256(1) << 48); | ||
|
|
||
| function packValidationHook(ModuleEntity _hookFunction) internal pure returns (HookConfig) { |
There was a problem hiding this comment.
Is there a reason the function parameters in this library are prefixed with an underscore?
There was a problem hiding this comment.
Yeah, the individual accessor methods like module(), entityId(), etc, are also visible within the other internal functions, so if the parameters would be named with the underscore (i.e. just module), then they would shadow the inner functions. It's not necessarily a bad thing since we don't really need to access those functions, but I just wanted to avoid any variable shadowing to prevent the compiler warning.
jaypaik
left a comment
jaypaik
left a comment
There was a problem hiding this comment.
Unblocking! I'll look over this again after catching up on the parent PRs.
Motivation
The parameter format of specifying pre-validation hooks and permission hooks separately for install is redundant, and uses inefficient patterns of ABI encoding for its inner fields.
Solution
Much like
ValidationConfig, introduce aHookConfigUDVT for holding:Use this type for installing validation associated hooks (pre-validation + permission hooks).
Use this type for internal encoding / decoding over the memory type
ExecutionHook, which was only intended for external view functions.Also bumps up the fuzzing runs for local testing - this setting is not used by CI, which uses
optimized-test-deep.