-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat(store,world): more granularity for onchain hooks #1399
Conversation
🦋 Changeset detectedLatest commit: 5464530 The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
pragma solidity >=0.8.0; | ||
|
||
// 20 bytes address, 1 byte bitmap of enabled hooks | ||
type Hook is bytes21; |
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.
*/ | ||
function encode(address hookAddress, uint8 encodedHooks) internal pure returns (Hook) { | ||
// Move the address to the leftmost 20 bytes and the bitmap to the rightmost byte | ||
return Hook.wrap(bytes21(bytes20(hookAddress)) | bytes21(uint168(encodedHooks))); |
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.
ooc is this more gas efficient than the reverse?
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.
I think it shouldn't make a difference, wdyt @dk1a? Could try it out (either here or in a follow up gas optimization pass)
onAfterSetField: false, | ||
onBeforeDeleteRecord: true, | ||
onAfterDeleteRecord: 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.
I wonder if this would also work
EnabledStoreHooks({
onBeforeSetRecord: true,
onBeforeSetField: true,
onBeforeDeleteRecord: true
})
(only toggling on the ones we want and letting the others default, but not sure if solc will complain)
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.
I also wonder if we should just use a unit8
here instead of the struct so that the ABI (and presumably abi-encoded calldata) is a bit more compact vs (bool,bool,bool,bool,...)
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.
unfortunately solc
does complain :(
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.
I also wonder if we should just use a unit8 here
yeah i think you're right here - we just have to document how to use the helper libraries to encode the bitmap, but it would be much cheaper to encode it in eg. the module the hook is registered and only pass the uint8
between contracts.
…onDeleteRecord into onBeforeDeleteRecord and onAfterDeleteRecord
Fixes #1159 (that issue also includes more context on this approach vs different approaches)
TODOs
onSetRecord
intoonBeforeSetRecord
andonAfterSetRecord
onDeleteRecord
intoonBeforeDeleteRecord
andonAfterDeleteRecord
EnableHooks
toEnableStoreHooks
and each element frombeforeX
toonBeforeX
etcTODOs in follow ups
TODOs depending on #1354
onBeforeSetField
andonAfterSetField
onBeforeSpliceRecord
andonAfterSpliceRecord