feat: [v0.8-develop] Simplify hook declaration and storage #51
Conversation
|
|
||
| // HookData layout: | ||
| // 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF______________________ Hook Function Reference | ||
| // 0x__________________________________________AA____________________ is pre hook |
There was a problem hiding this comment.
Perhaps we could precede the hook function with the type identifier or implement a sorting byte. This way, in storage, all prehooks could be organized before posthooks, allowing the loop to terminate early.
There was a problem hiding this comment.
I agree this would be useful in practice. Currently though, the spec doesn't mandate any ordering for hooks, so can we address this problem in a different PR, for this issue? erc6900/resources#5
I'm wondering whether we should include this in the specifications to prevent developers from making incorrect assumptions. Please disregard this if it has already been mentioned in the spec. |
|
(I almost pressed the green merge button since it's situated very close to the comment section :) It might be a good idea to update the GitHub repository configuration to require at least one approval before merging.) |
This is because branch protections rules were only applied to |
This is described in the 6900 spec, although rather briefly: enum ManifestAssociatedFunctionType {
// Function is not defined.
NONE,
// Function belongs to this plugin.
SELF,
// Function belongs to an external plugin provided as a dependency during plugin installation. Plugins MAY depend
// on external validation functions. It MUST NOT depend on external hooks, or installation will fail.
DEPENDENCY,
//...
}Additionally, there are a few locations where dependencies are referenced as "validation dependencies". This is somewhat descriptive, but I agree we should update the spec to be more explicit about it. Though, the parameter format for depdendencies may change with erc6900/resources#9. |
| isPreHook = (uint256(setValue) >> 80) & 0xFF == 1; | ||
| isPostHook = (uint256(setValue) >> 72) & 0xFF == 1; |
There was a problem hiding this comment.
Curious why 0xFF over 0x01?
There was a problem hiding this comment.
I used 0xFF because I wanted to just compare the least significant byte, and I could have also used 0x01 here. It would behave the same if you only use toHookData and toSetValue to convert between the types, it could differ if someone manually dirties the upper bits of either of the two bytes though.
Motivation
Since v0.7, execution hooks can’t be dependencies anymore. This means that pre and post hooks have to be defined by the same plugin.
We can make a simplification similar to the “validation types” merges (#40, #46) , by treating pre- and post- hooks as an interface, and require pre/post hooks to share a function id.
If there is a many-to-one relationship between pre and post hooks, the plugins can still handle that routing internally.
Solution
ManifestAssociatedFunctiontypes withinManifestExecutionHook, instead just declaring the selector, functionId, and pre/post statuspreHooks,associatedPostHooks, andpostOnlyHooks) to just 1 structure (executionHooks).IAccountLoupeinterface to the new format (FunctionReference+ pre/post status).Notes
ManifestExecutionHooktake up 4 words in memory (4 * 32 = 128 bytes), despite fitting into a singlebytes32word. This is an unfortunate limitation of abi encoding & solidity conventions.HookDatastruct used within the account is kept in memory and occupies more space than is needed.FunctionReference- does this warrant creating a new type for hook function references? How do we handle this going into the future?todoin the interface spec. We can further address this with work connected to manifest simplification.