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: add support for key schemas #480

Merged
merged 8 commits into from
Mar 10, 2023
Merged

feat: add support for key schemas #480

merged 8 commits into from
Mar 10, 2023

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Mar 10, 2023

  • Currently we only store the schema of value columns on-chain, while key columns are implicit by the number of keys emitted in the StoreSetRecord event (and always typed bytes32).
  • This PR adds explicit support for key schemas to Store, World and CLI (tablegen + deploy):
    • Key schemas follow the same format as value schemas and are stored in the Schema table. This changes the schema of the Schema table to { valueSchema: Bytes32, keySchema: Bytes32 } (used to be just { schema: Bytes32 })

@alvrs alvrs requested a review from holic as a code owner March 10, 2023 16:29
@alvrs alvrs requested a review from dk1a March 10, 2023 16:29
@alvrs
Copy link
Member Author

alvrs commented Mar 10, 2023

Adding metadata to the schema table pushes the World contract over the contract size limit, which I'm addressing in my next PR, so gonna leave this change for then.

@holic
Copy link
Member

holic commented Mar 10, 2023

ooc is the world only doing the minimal necessary to bootstrap? It’s not registering userland schemas and systems etc right? That’s done via deploy script?

@alvrs
Copy link
Member Author

alvrs commented Mar 10, 2023

ooc is the world only doing the minimal necessary to bootstrap? It’s not registering userland schemas and systems etc right? That’s done via deploy script?

Correct, the World only sets up its own internal tables (and soon internal systems)

registerSchema(StoreCoreInternal.SCHEMA_TABLE, SchemaLib.encode(SchemaType.BYTES32));
registerSchema(
StoreCoreInternal.SCHEMA_TABLE,
SchemaLib.encode(SchemaType.BYTES32, SchemaType.BYTES32),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the first BYTES32 is supposed to be padding for keys now.
If so this took me a while to understand despite knowing how this all works pretty well, a comment would be useful

Copy link
Member Author

@alvrs alvrs Mar 10, 2023

Choose a reason for hiding this comment

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

The first BYTES32 is the valueSchema, the second BYTES32 is the keySchema. Will add a comment. Also wanna turn the SchemaTable into an autogenerated table at some point, but there might be circular dependencies with the schema existing...

Copy link
Contributor

Choose a reason for hiding this comment

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

The first BYTES32 is the valueSchema, the second BYTES32 is the keySchema

right that makes more sense, it's not just padding. What's the UINT256 one then?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the Schema table's single primary key, which has type UINT256 because it represents a tableId. Added a comment about that too.

Copy link
Contributor

@dk1a dk1a left a comment

Choose a reason for hiding this comment

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

lgtm

@alvrs alvrs merged commit 37aec2e into main Mar 10, 2023
@alvrs alvrs mentioned this pull request Mar 10, 2023
15 tasks
@holic holic deleted the alvrs/keyschema branch June 23, 2023 11:36
@alvrs alvrs mentioned this pull request Jul 10, 2023
22 tasks
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.

3 participants