feat(world, store): stop loading schema from storage, require schema as an argument #1174

alvrs · 2023-07-18T15:53:31Z

Fixes #1131
Breaking change: All of Store's read and write methods now require the table's value schema to be passed in instead of reading it from storage

changeset-bot · 2023-07-18T15:53:34Z

🦋 Changeset detected

Latest commit: b087930

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

This PR includes changesets to release 26 packages

Name	Type
@latticexyz/cli	Major
@latticexyz/store	Major
@latticexyz/world	Major
create-mud	Major
@latticexyz/std-client	Major
@latticexyz/dev-tools	Major
@latticexyz/react	Major
@latticexyz/store-cache	Major
@latticexyz/store-indexer	Major
@latticexyz/store-sync	Major
@latticexyz/ecs-browser	Major
@latticexyz/block-logs-stream	Major
@latticexyz/common	Major
@latticexyz/config	Major
@latticexyz/gas-report	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-contracts	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 · 2023-07-18T17:54:51Z

We should probably wait for v2 networking to be merged and v1 networking to be removed to avoid double work in integrating this in the old stack.

alvrs · 2023-07-28T15:18:10Z

Current dependencies on/for this PR:

main
- PR refactor: remove v1 network package, remove snap sync module, deprecate std-client #1311
  - PR feat(world, store): stop loading schema from storage, require schema as an argument #1174 👈
    - PR feat(store,world): combine schema and metadata registration, rename getSchema to getValueSchema, change Schema table id #1182
      - PR refactor: combine name and namespace to resource selector in World methods #1208

This comment was auto-generated by Graphite.

alvrs · 2023-08-15T11:41:46Z

packages/store/gas-report.json

@@ -261,13 +261,13 @@
    "file": "test/Mixed.t.sol",
    "test": "testSetAndGet",
    "name": "set record in Mixed",
-    "gasUsed": 110021
+    "gasUsed": 111113


very interesting that the gas cost increased for this one (while decreasing for the other tests). @dk1a do you have an intuition around why?

pretty sure it's cause registerSchema is called in that test (before gas report), which warms the slot, so there's no gas reduction, but there is a new call to getSchema which is very suboptimal (should be better after #1252 and SchemaType->staticLength)

alvrs · 2023-08-16T11:30:51Z

e2e/packages/sync-test/data/encodeTestData.ts

+  return mapObject(testData, (records, table) => {
+    if (!records) return undefined;
+    const keyConfig = config.tables[table].keySchema;
+    return records.map((record) => {
+      const encodedKey = Object.entries(record.key).map(([keyName, keyValue]) => {
+        const keyType = keyConfig[keyName as keyof typeof keyConfig] as StaticAbiType;
+        return encodeAbiParameters([{ type: keyType }], [keyValue]);
+      });
+
+      const encodedValue = encodePacked(Object.values(config.tables[table].schema), Object.values(record.value));
+
+      const encodedValueSchema = schemaToHex(abiTypesToSchema(Object.values(config.tables[table].schema)));
+
+      return {
+        key: encodedKey,
+        value: encodedValue,
+        valueSchema: encodedValueSchema,
+      };
+    });
+  }) as EncodedData<typeof testData>;


this change is fixing a type issue, adding the valueSchema to the encoded data and slightly refactoring because the nested functions became hard to read

alvrs · 2023-08-16T11:31:43Z

packages/cli/src/commands/trace.ts

+      const address = await WorldContract.getField(
+        systemsTableId.toHex(),
+        [systemSelector.toHex()],
+        0,
+        systemTableSchema
+      );


getField now requires the value schema to be passed in

another approach that might be a little less error prone if we change this schema later and forget to update this is to fetch the schema from the chain before we get field

alvrs · 2023-08-16T11:33:19Z

packages/store/src/StoreCore.sol

This file contains the most significant changes (reading the schema from args instead of storage and renaming schema to valueSchema). The interface changes are then propagated through all the interfaces (IStore, World, etc)

alvrs · 2023-08-16T11:34:07Z

packages/store/ts/codegen/field.ts

passing the schema in all methods interacting with the store

alvrs · 2023-08-16T11:35:22Z

This looks like a massive PR but it's mostly small refactors (passing the schema as arg to all methods interacting with the store). Added some PR comments to point out the relevant areas to make reviewing a bit easier.

holic

Looks great! Mostly just a question about where we want to put the schema arg.

holic · 2023-08-16T12:24:27Z

.changeset/few-mirrors-reflect.md

+    bytes32 table,
+    bytes32[] calldata key,
+    bytes calldata data,
+   Schema valueSchema


Does the argument ordering here make sense? I usually order args by specificity and would kind of expect valueSchema to be further ahead in the list of arguments (before or after table perhaps).

Unless it's an optional arg, then it being at the end makes a bit more sense.

i agree with ordering by specificity but would defer this refactor until after this current stack of PRs is merged since they touch lots of the same places in the code and changing it here would likely lead to massive merge conflicts in the upstream branches

holic · 2023-08-16T12:27:26Z

e2e/packages/contracts/worlds.json

@@ -1,5 +1,5 @@
 {
  "31337": {
-    "address": "0x5FbDB2315678afecb367f032d93F642f64180aa3"
+    "address": "0xA7c59f010700930003b33aB25a7a0679C860f29c"


wonder if this will muck with e2e tests or if this will get overwritten in CI after deploy

holic · 2023-08-16T12:30:11Z

e2e/pnpm-lock.yaml

-settings:
-  autoInstallPeers: true
-  excludeLinksFromLockfile: false
-


holic · 2023-08-16T12:32:10Z

packages/cli/src/commands/trace.ts

+      const address = await WorldContract.getField(
+        systemsTableId.toHex(),
+        [systemSelector.toHex()],
+        0,
+        systemTableSchema
+      );


another approach that might be a little less error prone if we change this schema later and forget to update this is to fetch the schema from the chain before we get field

holic · 2023-08-16T12:35:53Z

packages/store/src/codegen/tables/KeyEncoding.sol

@@ -90,7 +90,7 @@ library KeyEncoding {
    _keyTuple[4] = _boolToBytes32(k5);
    _keyTuple[5] = bytes32(uint256(uint8(k6)));

-    bytes memory _blob = StoreSwitch.getField(_tableId, _keyTuple, 0);
+    bytes memory _blob = StoreSwitch.getField(_tableId, _keyTuple, 0, getSchema());


not blocking, just thinking aloud: I don't know the gas implications but I wonder if it would make sense to provide an alternative code path for these cases where we can generate a byte offset rather than a schema index + schema definition.

We could potentially do this for getters even if we don't commit to the bytes offset events changes.

yeah this seems kinda related to #1222 and SchemaType removal, as we are moving away from validation in StoreCore and towards dealing with byte blobs

yeah could add this, but would defer the decision until after #1222

dk1a · 2023-08-16T12:21:58Z

packages/cli/src/commands/trace.ts

@@ -11,8 +11,10 @@ import { resolveWorldConfig, WorldConfig } from "@latticexyz/world";
 import { IBaseWorld } from "@latticexyz/world/types/ethers-contracts/IBaseWorld";
 import IBaseWorldData from "@latticexyz/world/abi/IBaseWorld.sol/IBaseWorld.json" assert { type: "json" };
 import { getChainId, getExistingContracts } from "../utils";
+import { schemaToHex } from "@latticexyz/protocol-parser";


should this be before the local import?

one day I'll add a linter/autofixer for this: #1277

vercel bot deployed to Preview July 18, 2023 15:53 View deployment

vercel bot deployed to Preview July 18, 2023 15:56 View deployment

vercel bot deployed to Preview July 18, 2023 17:40 View deployment

alvrs force-pushed the alvrs/schema-methods branch from 2bc7cc3 to 98b9586 Compare July 19, 2023 11:33

vercel bot deployed to Preview July 19, 2023 11:34 View deployment

This was referenced Jul 28, 2023

feat(store,world): combine schema and metadata registration, rename getSchema to getValueSchema, change Schema table id #1182

Merged

refactor: combine name and namespace to resource selector in World methods #1208

Merged

vercel bot deployed to Preview July 28, 2023 15:26 View deployment

alvrs force-pushed the alvrs/schema-methods branch from 98b9586 to f433947 Compare July 28, 2023 15:30

dk1a mentioned this pull request Jul 31, 2023

refactor(store): separate SchemaLib into specialized libraries #1216

Merged

alvrs force-pushed the alvrs/schema-methods branch from f433947 to 233c35c Compare August 9, 2023 17:18

vercel bot deployed to Preview August 9, 2023 17:19 View deployment

alvrs force-pushed the alvrs/schema-methods branch from 233c35c to d3a7d26 Compare August 15, 2023 10:30

vercel bot deployed to Preview August 15, 2023 10:32 View deployment

vercel bot deployed to Preview August 15, 2023 11:33 View deployment

alvrs commented Aug 15, 2023

View reviewed changes

vercel bot deployed to Preview August 15, 2023 11:50 View deployment

alvrs changed the base branch from main to alvrs/remove-v1-network August 15, 2023 14:56

alvrs force-pushed the alvrs/schema-methods branch from c628440 to f862270 Compare August 15, 2023 14:56

alvrs mentioned this pull request Aug 15, 2023

refactor: remove v1 network package, remove snap sync module, deprecate std-client #1311

Merged

vercel bot deployed to Preview August 15, 2023 14:58 View deployment

alvrs force-pushed the alvrs/schema-methods branch from f862270 to 5908a3d Compare August 15, 2023 16:16

vercel bot deployed to Preview August 15, 2023 16:18 View deployment

alvrs force-pushed the alvrs/remove-v1-network branch from 2c68257 to b806818 Compare August 15, 2023 17:47

alvrs force-pushed the alvrs/schema-methods branch from 5908a3d to 20e5702 Compare August 15, 2023 17:47

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

Base automatically changed from alvrs/remove-v1-network to main August 16, 2023 08:54

Update few-mirrors-reflect.md

d871f3b

alvrs marked this pull request as ready for review August 16, 2023 11:28

alvrs requested a review from holic as a code owner August 16, 2023 11:28

vercel bot deployed to Preview August 16, 2023 11:30 View deployment

alvrs commented Aug 16, 2023

View reviewed changes

packages/store/ts/codegen/field.ts Outdated

Copy link

Member Author

alvrs Aug 16, 2023

Choose a reason for hiding this comment

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

passing the schema in all methods interacting with the store

run prettier

b141c1f

alvrs requested a review from dk1a August 16, 2023 11:36

vercel bot deployed to Preview August 16, 2023 11:37 View deployment

holic previously approved these changes Aug 16, 2023

View reviewed changes

dk1a previously approved these changes Aug 16, 2023

View reviewed changes

clean up trace

cb37445

alvrs dismissed stale reviews from dk1a and holic via cb37445 August 16, 2023 13:21

vercel bot deployed to Preview August 16, 2023 13:22 View deployment

update pnpm

d5ed69d

vercel bot deployed to Preview August 16, 2023 13:25 View deployment

refactor trace

67143bf

vercel bot deployed to Preview August 16, 2023 13:30 View deployment

fix trace

102e83c

vercel bot deployed to Preview August 16, 2023 13:57 View deployment

Merge branch 'main' into alvrs/schema-methods

b087930

vercel bot deployed to Preview August 16, 2023 14:01 View deployment

holic approved these changes Aug 16, 2023

View reviewed changes

alvrs merged commit 952cd53 into main Aug 16, 2023
10 checks passed

alvrs deleted the alvrs/schema-methods branch August 16, 2023 14:26

github-actions bot mentioned this pull request Aug 16, 2023

Version Packages (next) #1313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(world, store): stop loading schema from storage, require schema as an argument #1174

feat(world, store): stop loading schema from storage, require schema as an argument #1174

alvrs commented Jul 18, 2023 •

edited

Loading

changeset-bot bot commented Jul 18, 2023 •

edited

Loading

alvrs commented Jul 18, 2023

alvrs commented Jul 28, 2023 •

edited

Loading

alvrs Aug 15, 2023

dk1a Aug 15, 2023 •

edited

Loading

alvrs Aug 16, 2023

alvrs Aug 16, 2023

holic Aug 16, 2023

alvrs Aug 16, 2023

alvrs Aug 16, 2023

alvrs commented Aug 16, 2023

holic left a comment

holic Aug 16, 2023

alvrs Aug 16, 2023

holic Aug 16, 2023

holic Aug 16, 2023

holic Aug 16, 2023

holic Aug 16, 2023 •

edited

Loading

dk1a Aug 16, 2023

alvrs Aug 16, 2023

dk1a Aug 16, 2023

holic Aug 16, 2023

feat(world, store): stop loading schema from storage, require schema as an argument #1174

feat(world, store): stop loading schema from storage, require schema as an argument #1174

Conversation

alvrs commented Jul 18, 2023 • edited Loading

changeset-bot bot commented Jul 18, 2023 • edited Loading

🦋 Changeset detected

alvrs commented Jul 18, 2023

alvrs commented Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

dk1a Aug 15, 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

Choose a reason for hiding this comment

alvrs commented Aug 16, 2023

holic left a 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 Aug 16, 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

alvrs commented Jul 18, 2023 •

edited

Loading

changeset-bot bot commented Jul 18, 2023 •

edited

Loading

alvrs commented Jul 28, 2023 •

edited

Loading

dk1a Aug 15, 2023 •

edited

Loading

holic Aug 16, 2023 •

edited

Loading