feat(store): add StoreMetadata table for table name and field names #428

alvrs · 2023-02-21T20:50:51Z

Adds an internal StoreMetadata table to store metadata like table name and field names.
Adds a setMetadata method to StoreCore, IStore and World (with access control based on table ownership)

holic · 2023-02-21T20:58:08Z

Can we also add table names? (either here or in another PR)

dk1a · 2023-02-22T00:14:54Z

I wonder if it'd be better to have a separate IStoreMetadata with its own METADATA_SCHEMA_TABLE, that's optionally called after the main registerSchema. This'd avoid polluting IStore and StoreCore, and allow for example renaming fields (registerSchema can't be called twice)
To validate lengths metadata can query the schema

alvrs · 2023-02-22T22:27:47Z

I wonder if it'd be better to have a separate IStoreMetadata with its own METADATA_SCHEMA_TABLE, that's optionally called after the main registerSchema. This'd avoid polluting IStore and StoreCore, and allow for example renaming fields (registerSchema can't be called twice)
To validate lengths metadata can query the schema

Agree with storing it in a separate StoreMetadata table, updated the PR. Still think there should be a method on the store interface (setMetadata) to change the table metadata, since it's optional but still very elemental and setting it should be the default.

dk1a

lgtm, I like how setMetadata is an optional extra method now

holic · 2023-02-23T15:25:10Z

packages/store/src/StoreCore.sol

+
+    // Set metadata
+    StoreMetadataTable.set(table, tableName, fieldNames);
+  }


holic · 2023-02-23T15:51:32Z

Is the idea here that we'll add setMetadata to the autogen'ed libraries (alongside registerSchema)?

holic · 2023-02-23T15:55:38Z

Is it useful or scary to allow overriding metadata after its already been set?

Thinking about the data sync pipeline (events) looking like this:

table schema registered
table metadata set (e.g. position and {x, y})
table record set
table record set
table metadata updated (e.g. position and {x, z})

What should the client do with the second metadata update? Ignore it? Update all values? If code depends on getting position.y and that changes midway through to position.z, sounds like we'd introduce a lot of runtime errors/bugs.

dk1a · 2023-02-23T16:04:48Z

Is the idea here that we'll add setMetadata to the autogen'ed libraries (alongside registerSchema)?

Yep, it's basically 1 line for me

What should the client do with the second metadata update? Ignore it? Update all values? If code depends on getting position.y and that changes midway through to position.z, sounds like we'd introduce a lot of runtime errors/bugs.

The schema is immutable, metadata is just names. I imagine client code can initialize names once per session, and then ignore any changes to them. Haven't thought much about it though

holic · 2023-02-23T19:43:25Z

The schema is immutable, metadata is just names. I imagine client code can initialize names once per session, and then ignore any changes to them. Haven't thought much about it though

That brings up another question for me: if we're gonna generate some types in the frontend for a given table, and those types depend on field names, and there's multiple events for setMetadata (i.e. field names change), which ones do we use? or do we ask users to pick the particular shape they want?

Detected different field names for table position. Which do you want to use to generate types?
> {x, y}
  {x, z}

These kinds of questions make me wonder if we should make metadata immutable as well, if not required.

alvrs · 2023-02-26T13:34:15Z

That brings up another question for me: if we're gonna generate some types in the frontend for a given table, and those types depend on field names, and there's multiple events for setMetadata (i.e. field names change), which ones do we use? or do we ask users to pick the particular shape they want?

I'd say by default the last one, but we could also let users specify custom overrides (for example if you built a frontend for a third party table that changed it's metadata). Once client definitions for the table exist, the client can also just ignore metadata overrides since they're not relevant for decoding. (The client should probably rely on the auto-generated / statically inferred types to name the schema if they exist to avoid the issues you described).

I would assume the rename feature isn't gonna be used too often, but it might be useful in some cases (just like renaming a column in an existing postgres database), so I'd include it by default.

holic · 2023-02-27T17:51:17Z

Another thing I just realized today: MODE will probably need immutable names, otherwise it means a DB migration of column names? Or I guess MODE could store things as fields without names, then separately store the mapping of field indices to names. cc @authcall

alvrs · 2023-02-28T15:04:38Z

I think since the table metadata is optional and possibly added in a separate transaction than the initial registration of the table, the indexer and client would both have to handle renaming anyway (if dynamically picking up new tables is a feature, which is definitely the case for the indexer but unclear for the client). So it might not be much additional effort to also support renaming again after the first metadata was set?

To unblock the network PR work I'd vote for merging this PR as is and possibly add a restriction to being able to modify schema names in a follow up if we the result of our discussion will be that it leads to too much complication too support.

holic · 2023-02-28T16:45:46Z

I am kinda wondering/leaning towards the client doing the mapping between field name and field index, then do queries under the hood based on only field indices. Then MODE doesn't need to know about field names.

dk1a · 2023-02-28T16:50:54Z

lgtm

I am kinda wondering/leaning towards the client doing the mapping between field name and field index, then do queries under the hood based on only field indices. Then MODE doesn't need to know about field names.

Even with table metadata both the client and MODE could choose to ignore it.
As I understand it, metadata mostly helps network output useful errors during decoding, it doesn't have to be used otherwise

holic · 2023-02-28T16:51:18Z

packages/store/src/tables/StoreMetadata.sol

+    bytes memory _blob = StoreSwitch.getField(_tableId, _primaryKeys, 1);
+    bytes memory _newBlob = abi.encodePacked(_blob, bytes(_slice));
+    StoreSwitch.setField(_tableId, _primaryKeys, 1, _newBlob);
+  }


I realize this is autogen but this seems kinda scary for an abi encoded field. I wonder if we should have a per-field way to enable/disable generating push methods.

it totally does, and it shouldn't be used. string[] in general is not in a good place rn

looks like this exists for both bytes and string (not arrays, just dynamic length things)

yeah that's intentional, it's just dangerous when you do abi encoded stuff

I added this to my original message but I wonder if we should have a per-field way to enable/disable generating push/slice methods?

it's not exactly difficult, I think I could make that option very quickly
But it's part of the bigger question of configurability vs simplicity. Currently fields are a simple Record<string, SchemaType>. Should we make this a shorthand, and allow a field object with various options? I'm kind of leaning yes, but I think I tend to be a bit biased to the configurability side

(I have other options in mind that aren't well thought-out yet that could go there too, like enum wrappers)

alvrs force-pushed the alvrs/fieldnames branch from 298b88c to b3d643d Compare February 22, 2023 22:17

alvrs requested a review from holic as a code owner February 22, 2023 22:17

alvrs requested a review from dk1a February 22, 2023 22:23

alvrs changed the title ~~feat(store): add optional field names to registerSchema~~ feat(store): add StoreMetadata table for table name and field names Feb 22, 2023

dk1a previously approved these changes Feb 22, 2023

View reviewed changes

holic reviewed Feb 23, 2023

View reviewed changes

packages/store/src/StoreCore.sol

// Set metadata

StoreMetadataTable.set(table, tableName, fieldNames);

}

Copy link

Member

holic Feb 23, 2023

Choose a reason for hiding this comment

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

so clean!

alvrs added 2 commits February 28, 2023 10:49

feat(store): add metadata table for table and column names

08818e1

feat(store): autogenerate StoreMetadata table

d11f911

alvrs dismissed dk1a’s stale review via d11f911 February 28, 2023 16:31

alvrs force-pushed the alvrs/fieldnames branch from a00c355 to d11f911 Compare February 28, 2023 16:31

alvrs requested a review from a user February 28, 2023 16:31

alvrs changed the base branch from v2 to main February 28, 2023 16:31

alvrs removed the request for review from a user February 28, 2023 16:31

chore(store): remove unused Route table

ee8da5d

holic reviewed Feb 28, 2023

View reviewed changes

holic approved these changes Feb 28, 2023

View reviewed changes

alvrs merged commit ae39ace into main Feb 28, 2023

holic deleted the alvrs/fieldnames branch February 28, 2023 17:03

github-actions bot mentioned this pull request Jul 6, 2023

chore: release 2.0.0-next.0 #1103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(store): add StoreMetadata table for table name and field names #428

feat(store): add StoreMetadata table for table name and field names #428

alvrs commented Feb 21, 2023 •

edited

Loading

holic commented Feb 21, 2023

dk1a commented Feb 22, 2023 •

edited

Loading

alvrs commented Feb 22, 2023

dk1a left a comment

holic Feb 23, 2023

holic commented Feb 23, 2023

holic commented Feb 23, 2023 •

edited

Loading

dk1a commented Feb 23, 2023

holic commented Feb 23, 2023 •

edited

Loading

alvrs commented Feb 26, 2023

holic commented Feb 27, 2023

alvrs commented Feb 28, 2023

holic commented Feb 28, 2023

dk1a commented Feb 28, 2023

holic Feb 28, 2023 •

edited

Loading

dk1a Feb 28, 2023

holic Feb 28, 2023

dk1a Feb 28, 2023

holic Feb 28, 2023

dk1a Feb 28, 2023

dk1a Feb 28, 2023

feat(store): add StoreMetadata table for table name and field names #428

feat(store): add StoreMetadata table for table name and field names #428

Conversation

alvrs commented Feb 21, 2023 • edited Loading

holic commented Feb 21, 2023

dk1a commented Feb 22, 2023 • edited Loading

alvrs commented Feb 22, 2023

dk1a left a comment

Choose a reason for hiding this comment

holic Feb 23, 2023

Choose a reason for hiding this comment

holic commented Feb 23, 2023

holic commented Feb 23, 2023 • edited Loading

dk1a commented Feb 23, 2023

holic commented Feb 23, 2023 • edited Loading

alvrs commented Feb 26, 2023

holic commented Feb 27, 2023

alvrs commented Feb 28, 2023

holic commented Feb 28, 2023

dk1a commented Feb 28, 2023

holic Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

dk1a Feb 28, 2023

Choose a reason for hiding this comment

holic Feb 28, 2023

Choose a reason for hiding this comment

dk1a Feb 28, 2023

Choose a reason for hiding this comment

holic Feb 28, 2023

Choose a reason for hiding this comment

dk1a Feb 28, 2023

Choose a reason for hiding this comment

dk1a Feb 28, 2023

Choose a reason for hiding this comment

alvrs commented Feb 21, 2023 •

edited

Loading

dk1a commented Feb 22, 2023 •

edited

Loading

holic commented Feb 23, 2023 •

edited

Loading

holic commented Feb 23, 2023 •

edited

Loading

holic Feb 28, 2023 •

edited

Loading