Skip to content

feat(store,world): add ability to unregister hooks#1422

Merged
alvrs merged 6 commits intomainfrom
alvrs/remove-hooks
Sep 11, 2023
Merged

feat(store,world): add ability to unregister hooks#1422
alvrs merged 6 commits intomainfrom
alvrs/remove-hooks

Conversation

@alvrs
Copy link
Member

@alvrs alvrs commented Sep 8, 2023

Fixes #1405
Followup to #1399
Context in the changeset

@changeset-bot
Copy link

changeset-bot bot commented Sep 8, 2023

🦋 Changeset detected

Latest commit: 3474859

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 28 packages
Name Type
@latticexyz/store Major
@latticexyz/world Major
@latticexyz/cli Major
@latticexyz/dev-tools Major
@latticexyz/react Major
@latticexyz/store-indexer Major
@latticexyz/store-sync Major
@latticexyz/abi-ts Major
@latticexyz/block-logs-stream Major
@latticexyz/common Major
@latticexyz/config Major
create-mud Major
@latticexyz/ecs-browser Major
@latticexyz/gas-report Major
@latticexyz/network Major
@latticexyz/noise Major
@latticexyz/phaserx Major
@latticexyz/protocol-parser Major
@latticexyz/recs Major
@latticexyz/schema-type Major
@latticexyz/services Major
@latticexyz/solecs Major
solhint-config-mud Major
solhint-plugin-mud Major
@latticexyz/std-client Major
@latticexyz/std-contracts Major
@latticexyz/store-cache Major
@latticexyz/utils Major

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

@alvrs alvrs force-pushed the alvrs/remove-hooks branch from 1203619 to dbd0dc9 Compare September 8, 2023 11:02
@alvrs alvrs requested a review from dk1a September 8, 2023 11:02
@alvrs alvrs marked this pull request as ready for review September 8, 2023 11:02
@alvrs alvrs requested a review from frolic as a code owner September 8, 2023 11:02
// (Note: this does not update the free memory pointer)
assembly {
mstore(newHooks, newHooksIndex)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the "remove a hook from the list of hooks" is duplicated, should we move this into some sort of hooks lib?

frolic
frolic previously approved these changes Sep 8, 2023
Copy link
Member

@frolic frolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment about code duplication but not blocking!

* Filter the given hook from the hook list at the given key in the given hook table
*/
function filterListByAddress(bytes32 hookTableId, bytes32 key, address hookAddressToRemove) internal {
bytes21[] memory currentHooks = Hooks.get(hookTableId, key);
Copy link
Member

@frolic frolic Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better/more gas efficient to do the get/set here than to pass in the array and get an array back out (a more functional approach)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, unfortunately much more gas efficient to pass as little memory around as possible - passing arrays is pretty expensive. (Otherwise would have preferred a more functional approach with arrays as input/output too)

@alvrs alvrs merged commit 1d60930 into main Sep 11, 2023
@alvrs alvrs deleted the alvrs/remove-hooks branch September 11, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability to remove hooks

2 participants