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

consider including field index (and field bytes offset) in Store events #2222

Closed
holic opened this issue Feb 1, 2024 · 6 comments · Fixed by #2279
Closed

consider including field index (and field bytes offset) in Store events #2222

holic opened this issue Feb 1, 2024 · 6 comments · Fixed by #2279
Assignees

Comments

@holic
Copy link
Member

holic commented Feb 1, 2024

Store's splice operations on static and dynamic fields use a field index to map to contract storage slots. Without this data expressed in events, offchain clients/indexers are ~forced to model the data as a single bytes blob for static field data and another for dynamic field data.

Since we already have access to the field index in these methods (and the field-specific bytes offset for dynamic fields), we could expose these as part of the event to allow for more flexible ways of modeling data offchain.

We should probably measure the gas of this to make sure it's worth the trade-off. Adding this also means we'd cement the "one splice event = one field update", which seems fine.

@holic holic added this to the v2 MUD stable milestone Feb 1, 2024
@holic
Copy link
Member Author

holic commented Feb 11, 2024

Unclear how to handle the set event - I think we'd have to send down the whole field layout?

@yonadaaa
Copy link
Contributor

yonadaaa commented Feb 12, 2024

I think "one splice event = one field update" is fine, I can't see why we'd splice many fields but not the whole record.

How do we feel then about removing start for the static data event? There's a disconnect that events operate over bytes when the codebase generally uses fields (StoreCore.setField, Number.getValue...).

I guess it would prevent us from building "schemaless" indexers, but table schemas are a Store feature after all - we shouldn't obscure them (in contrast to eg. World namespaces).

@holic
Copy link
Member Author

holic commented Feb 12, 2024

The idea here is that we currently support creating schemaless indexers, but with the constraint that data is stored in two fields: static data and dynamic data.

However, that's not how it's actually stored onchain and I want to explore what additional args we could provide in events to offer indexer developers to have the option to store in a single blob for all like data vs. blob per field without a schema lookup.

You could still build this with the events we have now, but would require an additional schema lookup.

@dk1a
Copy link
Member

dk1a commented Feb 12, 2024

For more schemalessness the indexer devs could use FieldLayout's total static length
It's like how onchain the data is also schemaless and only FieldLayout is used,

it's still a lookup, but not strictly a schema lookup

How do we feel then about removing start for the static data event?

Some thoughts on making start an encapsulated implementation detail, for the more schemaful route:

  • start in spliceStaticData allows editing several (not necessarily all, unlike setRecord) static fields at once, which is mostly useless, and table libs don't take advantage of this
  • start in spliceDynamicData allows editing multiple array elements (or a byte slice) at once, which seems more useful, like a js splice, but table libs once again don't take advantage of this (they could, I just never got around to it - table libs would translate an element index to bytes, they already do similar stuff in some methods)

We can replace start with startElementIndex or the like, without losing functionality, all while making table libs do less translation, but then splice methods in StoreCore would need to get FieldLayout, to get the necessary byte lengths. So the cost would be 1 more storage read I guess (or 1 more arg, if we think just sending it is fine)

@holic
Copy link
Member Author

holic commented Feb 16, 2024

  • add field index to dynamic splice so folks have the option to store each dynamic field separately (only looking at events without schema lookups)
  • maybe could do the same for static field but would need both field index in splice event and potentially field layout in set record event

@yonadaaa
Copy link
Contributor

yonadaaa commented Feb 20, 2024

  • Investigate whether event arguments are packed, if not we could bit-pack for better gas efficiency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: No status
3 participants