Skip to content
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: replace Schema with FieldLayout for contract internals #1336

Merged
merged 27 commits into from
Sep 13, 2023

Conversation

dk1a
Copy link
Contributor

@dk1a dk1a commented Aug 21, 2023

fixes #1326
see the changeset for more context on this change

@changeset-bot
Copy link

changeset-bot bot commented Aug 21, 2023

🦋 Changeset detected

Latest commit: 5e993cc

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

This PR includes changesets to release 28 packages
Name Type
@latticexyz/cli Major
@latticexyz/protocol-parser Major
@latticexyz/store Major
@latticexyz/world Major
@latticexyz/store-sync Major
@latticexyz/dev-tools Major
@latticexyz/react Major
@latticexyz/store-indexer 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/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
Copy link
Member

alvrs commented Aug 21, 2023

High level feedback on terminology: I think we should keep calling the array of schema types "schema", which is the thing that is used by off-chain consumers to know how to interpret the data, and we should find a new name for the "static byte lengths" object used internally by Store and the table libraries. Otherwise it gets confusing if we use "Schema" for one thing onchain and another thing offchain.

@dk1a
Copy link
Contributor Author

dk1a commented Aug 21, 2023

I think we should keep calling the array of schema types "schema"

Technically no more schema types, just abi types (I wanna ditch the enum and replace it with bytes32 strings that directly mirror config/client).
They can still be called Schema. Or we could rename to something like AbiTypes. getValueAbiTypes getKeyAbiTypes? (doesn't sound as good I guess)

we should find a new name for the "static byte lengths"

Agree, FieldMetadata FieldInfo? This stores the number of fields and their lengths, so FieldLengths wouldn't really work.

@dk1a dk1a changed the title WIP SchemaType to staticByteLength feat: replace Schema with FieldLayout for contract internals Sep 5, 2023
@@ -459,211 +519,217 @@
"file": "test/StoreCoreDynamic.t.sol",
"test": "testGetFieldSlice",
"name": "get field slice (cold, 1 slot)",
"gasUsed": 7990
"gasUsed": 8425
Copy link
Member

Choose a reason for hiding this comment

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

do you think these increases in gas are a result of the previous measurements being invalid (because of #1267) or is using constants instead of hardcoded values somehow more expensive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dunno, I assume it's #1267, but could be the optimizer being bad too

alvrs
alvrs previously approved these changes Sep 13, 2023
Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

code looks good! good to be merged once merge conflicts are resolved

@alvrs alvrs merged commit de151fe into main Sep 13, 2023
@alvrs alvrs deleted the dk1a/schematype-to-length branch September 13, 2023 13:00
@@ -0,0 +1,1734 @@
/* Autogenerated file. Do not edit manually. */
/* tslint:disable */
/* eslint-disable */
Copy link
Member

Choose a reason for hiding this comment

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

these files snuck back in 😭

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.

Replace schema types with static data length
3 participants