feat(store,world): add splice hooks, expose spliceStaticData, spliceDynamicData #1531

alvrs · 2023-09-17T20:05:22Z

remove onBeforeSetField and onAfterSetField, replace with onBeforeSpliceStaticData, onAfterSpliceStaticData, onBeforeDynamicData, onAfterDynamicData so we don't have to read the entire dynamic field when calling partial field methods (push, pop, update)
while replacing the hooks I realized that exposing spliceStaticData and spliceDynamicData as methods on IStore would be really useful for hooks (otherwise even simple hooks like "mirror the data to another table" are very complicated to implement to because the hook data doesn't match any store method)
- exposing those methods is not a concern for diverging onchain and offchain indexer data, since it's already possible to modify records in arbitrary ways by providing different FieldLayout values
- this change triggered a larger refactor of StoreCore to use the new spliceStaticData and spliceDynamicData instead of duplicating the splice logic in every single dynamic data function

will add more detailed context to the changeset once this PR is ready

…eCore

changeset-bot · 2023-09-17T20:05:25Z

🦋 Changeset detected

Latest commit: 5154f15

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

This PR includes changesets to release 29 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/faucet	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

holic · 2023-09-17T20:25:15Z

packages/store/src/IStoreHook.sol

  IStoreHook.onBeforeDeleteRecord.selector ^
  IStoreHook.onAfterDeleteRecord.selector ^
  ERC165_INTERFACE_ID;

 interface IStoreHook is IERC165 {
  function onBeforeSetRecord(
    bytes32 tableId,
-    bytes32[] memory keyTuple,
+    bytes32[] calldata keyTuple,


will we have similar issues as #1528 (comment) by changing this?

Changed all to memory. Had calldata initially bc it would be cheaper if called with calldata, but we're in fact calling all these functions with memory from the World, so needs to be memory here.

holic · 2023-09-17T20:26:31Z

packages/store/src/storeHookTypes.sol

+uint8 constant BEFORE_SPLICE_DYNAMIC_DATA = 1 << 4;
+uint8 constant AFTER_SPLICE_DYNAMIC_DATA = 1 << 5;
+uint8 constant BEFORE_DELETE_RECORD = 1 << 6;
+uint8 constant AFTER_DELETE_RECORD = 1 << 7;


this is what I was referring to btw when I thought we maybe needed more than 8 slots for hooks

do we wanna consider a bump to uint16 or uint32 for future proofing?

I'd prefer to only bump to uint16 once we actually need it (don't think we'll need it for this iteration of the protocol) - although one could argue we could avoid a breaking change if we already use uint16 now and later add additional functionality for other types of hook 🤔

yeah my main motivation is reducing breaking changes later

I guess either way it'd require a redeploy of the world, so maybe it's fine to leave this and expand when we need it

holic · 2023-09-17T20:27:16Z

packages/store/test/EchoSubscriber.sol

@@ -16,7 +16,9 @@ contract EchoSubscriber is StoreHook {
    bytes memory dynamicData,
    FieldLayout fieldLayout
  ) public {
-    emit HookCalled(abi.encode(tableId, keyTuple, staticData, encodedLengths, dynamicData, fieldLayout));
+    emit HookCalled(
+      abi.encodeCall(this.onBeforeSetRecord, (tableId, keyTuple, staticData, encodedLengths, dynamicData, fieldLayout))


holic · 2023-09-17T20:28:18Z

packages/store/test/MirrorSubscriber.sol

  ) public {
-    if (tableId != tableId) revert("invalid tableId");


holic · 2023-09-17T20:30:20Z

packages/store/test/setDynamicDataLengthAtIndex.sol

+import { Storage } from "../src/Storage.sol";
+
+/**
+ * Tes helper function to set the length of the dynamic data (in bytes) for the given value field layout and index


Suggested change

* Tes helper function to set the length of the dynamic data (in bytes) for the given value field layout and index

* Test helper function to set the length of the dynamic data (in bytes) for the given value field layout and index

holic · 2023-09-17T20:30:51Z

packages/store/test/setDynamicDataLengthAtIndex.sol

+  PackedCounter encodedLengths = PackedCounter.wrap(Storage.load({ storagePointer: dynamicDataLengthSlot }));
+
+  // Update the encoded lengths
+  encodedLengths = encodedLengths.setAtIndex(dynamicSchemaIndex, newLengthAtIndex);


Suggested change

encodedLengths = encodedLengths.setAtIndex(dynamicSchemaIndex, newLengthAtIndex);

encodedLengths = encodedLengths.setAtIndex(dynamicFieldIndex, newLengthAtIndex);

holic · 2023-09-17T20:33:20Z

packages/world/src/modules/keysintable/KeysInTableHook.sol

@@ -63,11 +63,41 @@ contract KeysInTableHook is StoreHook {
    // NOOP
  }

-  function onBeforeSetField(bytes32 tableId, bytes32[] memory keyTuple, uint8, bytes memory, FieldLayout) public {
+  function onBeforeSpliceStaticData(bytes32, bytes32[] calldata, uint48, uint40, bytes calldata) public pure {
    // NOOP
  }


if we add method stubs to StoreHook with a NotImplemented error, we could remove these here

holic · 2023-09-17T20:36:00Z

packages/store/test/StoreHook.t.sol

+            BEFORE_SPLICE_STATIC_DATA |
+            AFTER_SPLICE_STATIC_DATA |
+            BEFORE_SPLICE_DYNAMIC_DATA |
+            AFTER_SPLICE_DYNAMIC_DATA |
            BEFORE_DELETE_RECORD |
            AFTER_DELETE_RECORD


a nice thing we can do now that we are operating with bit flags is add shortcuts, e.g. BEFORE_ALL and AFTER_ALL like

uint8 constant BEFORE_ALL = BEFORE_SET_RECORD | BEFORE_SPLICE_STATIC_DATA | BEFORE_SPLICE_DYNAMIC_DATA | BEFORE_DELETE_RECORD;

holic · 2023-09-17T20:48:29Z

packages/store/src/StoreCore.sol

@@ -984,53 +971,13 @@ library StoreCoreInternal {
  }

  /**
-   * Get the length of the dynamic data for the given value field layout and index
+   * Load the encoded dynamic data length from storage for the given tableId and key tuple


should prob be

given table ID and key tuple

or

given tableId and keyTuple

holic · 2023-09-17T20:48:55Z

packages/store/src/StoreCore.sol

-    Storage.store({ storagePointer: dynamicDataLengthSlot, data: encodedLengths.unwrap() });
+    // Load the previous length of the field to set from storage to compute where to start to push
+    PackedCounter oldEncodedLengths = _loadEncodedDynamicDataLength(tableId, keyTuple);
+    uint40 oldFieldLength = uint40(oldEncodedLengths.atIndex(dynamicFieldIndex));


I tend to prefer "previous" instead of "old" for these sorts of operations, feels a bit more specific/accurate.

holic · 2023-09-17T20:54:22Z

packages/store/test/StoreMock.sol

+  ) public virtual {
+    StoreCore.spliceDynamicData(tableId, keyTuple, dynamicFieldIndex, startWithinField, deleteCount, data);
+  }
+
  // Set partial data at schema index


Suggested change

// Set partial data at schema index

// Set partial data at field index

there's a few more below too

holic · 2023-09-18T18:44:01Z

packages/world/test/SystemHook.t.sol

+
+  function testShorthand() public {
+    assertEq(ALL, BEFORE_CALL_SYSTEM | AFTER_CALL_SYSTEM);
+  }


is this a useful test? 😆

haha just want to make sure ALL is updated in case we add more hooks

packages/store/src/StoreCore.sol

alvrs · 2023-09-19T11:01:15Z

skipping CI, only updated a comment since last CI pass

alvrs added 6 commits September 17, 2023 15:56

feat(store): add on(Before|After)Splice(Static|Dynamic)Data

d6ab133

wip: migrate store hooks

56beb1b

refactor(store): add spliceStaticRecord and spliceDynamicData to Stor…

0e134d4

…eCore

add spliceStaticData and spliceDynamicData to IStore

989246a

fix types

2c1e155

fix tests

7920196

alvrs force-pushed the alvrs/splice-hooks branch from 4e0224b to 25f19ae Compare September 17, 2023 20:24

rebuild and gas report

8f913f3

alvrs force-pushed the alvrs/splice-hooks branch from 25f19ae to 8f913f3 Compare September 17, 2023 20:24

vercel bot deployed to Preview September 17, 2023 20:26 View deployment

holic reviewed Sep 17, 2023

View reviewed changes

fix tests and update gas report

d8e28e2

vercel bot deployed to Preview September 18, 2023 14:00 View deployment

add stubs for unimplemented functions

65d0f63

vercel bot deployed to Preview September 18, 2023 14:18 View deployment

alvrs added 2 commits September 18, 2023 15:40

renames

3845ba2

Merge branch 'main' into alvrs/splice-hooks

819e227

vercel bot deployed to Preview September 18, 2023 14:50 View deployment

Create few-papayas-leave.md

007f3c1

vercel bot deployed to Preview September 18, 2023 15:40 View deployment

remove outdated comments

915d45e

alvrs marked this pull request as ready for review September 18, 2023 15:45

alvrs requested a review from dk1a as a code owner September 18, 2023 15:45

vercel bot deployed to Preview September 18, 2023 15:46 View deployment

prettier

ce03439

vercel bot deployed to Preview September 18, 2023 16:24 View deployment

holic reviewed Sep 18, 2023

View reviewed changes

holic previously approved these changes Sep 18, 2023

View reviewed changes

Merge branch 'main' into alvrs/splice-hooks

ec9b7fc

alvrs dismissed holic’s stale review via ec9b7fc September 19, 2023 09:48

vercel bot deployed to Preview September 19, 2023 09:49 View deployment

alvrs commented Sep 19, 2023

View reviewed changes

packages/store/src/StoreCore.sol Outdated Show resolved Hide resolved

Update packages/store/src/StoreCore.sol

5154f15

alvrs merged commit d5094a2 into main Sep 19, 2023

alvrs deleted the alvrs/splice-hooks branch September 19, 2023 11:01

vercel bot deployed to Preview September 19, 2023 11:02 View deployment

alvrs mentioned this pull request Sep 20, 2023

Use splice events for hooks to avoid the linear storage reads #1510

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(store,world): add splice hooks, expose spliceStaticData, spliceDynamicData #1531

feat(store,world): add splice hooks, expose spliceStaticData, spliceDynamicData #1531

alvrs commented Sep 17, 2023

changeset-bot bot commented Sep 17, 2023 •

edited

Loading

holic Sep 17, 2023

alvrs Sep 18, 2023

holic Sep 17, 2023

alvrs Sep 18, 2023

holic Sep 18, 2023

holic Sep 17, 2023

holic Sep 17, 2023

holic Sep 17, 2023

holic Sep 17, 2023

holic Sep 17, 2023 •

edited

Loading

alvrs Sep 18, 2023

holic Sep 17, 2023

holic Sep 17, 2023

holic Sep 17, 2023

holic Sep 17, 2023 •

edited

Loading

holic Sep 18, 2023

alvrs Sep 18, 2023

alvrs commented Sep 19, 2023

		) public {
		if (tableId != tableId) revert("invalid tableId");

	* Tes helper function to set the length of the dynamic data (in bytes) for the given value field layout and index
	* Test helper function to set the length of the dynamic data (in bytes) for the given value field layout and index

	encodedLengths = encodedLengths.setAtIndex(dynamicSchemaIndex, newLengthAtIndex);
	encodedLengths = encodedLengths.setAtIndex(dynamicFieldIndex, newLengthAtIndex);

	// Set partial data at schema index
	// Set partial data at field index

feat(store,world): add splice hooks, expose spliceStaticData, spliceDynamicData #1531

feat(store,world): add splice hooks, expose spliceStaticData, spliceDynamicData #1531

Conversation

alvrs commented Sep 17, 2023

changeset-bot bot commented Sep 17, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Sep 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Sep 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvrs commented Sep 19, 2023

changeset-bot bot commented Sep 17, 2023 •

edited

Loading

holic Sep 17, 2023 •

edited

Loading

holic Sep 17, 2023 •

edited

Loading