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

Use 0x00 as key for singleton tables instead of empty array, require at least one element in a keyTuple #1125

Closed
Tracked by #760
alvrs opened this issue Jul 10, 2023 · 10 comments
Assignees

Comments

@alvrs
Copy link
Member

alvrs commented Jul 10, 2023

the current empty array leads to a couple subtle issues: in “KeysWithValue” it’s not possible to distinguish the “singleton key” from “no keys”, and in MODE and the client we currently need custom handling of singleton tables because of the empty array. It might be easier to just use a known key, maybe just “0x00”, to use for singleton tables, so that all other tools and integrations don’t need a special case for it

@alvrs alvrs modified the milestones: MUD stable, Contracts stable Jul 10, 2023
@alvrs alvrs mentioned this issue Jul 10, 2023
17 tasks
@holic
Copy link
Member

holic commented Jul 12, 2023

do we specifically want 0x00 or should we use e.g. 0x01 so we can at least differentiate between initialized/uninitialized values?

also, if we have proper key tuple names registered, do we want to give singleton tables a meaningful key tuple like {singleton: 'bool'} where the key is always true?

@alvrs
Copy link
Member Author

alvrs commented Jul 12, 2023

can you illustrate a situation where using 0x00 as key would be an issue? might be good to have an example to think around, since 0x00 is also a valid key for user defined (non-singleton) tables

@holic
Copy link
Member

holic commented Jul 13, 2023

Hmm, I can't think of anything. Maybe keys aren't an issue with zero/uninitialized values, only values (where it's hard to tell if something is set or not set)

@holic
Copy link
Member

holic commented Aug 10, 2023

in “KeysWithValue” it’s not possible to distinguish the “singleton key” from “no keys”

This seems fine, since a singleton key/empty schema is for the entire table (only one value stored rather than one per key), so you wouldn't even use/need KeysWithValue, right?

We can/should adjust the module to either check for key length within the hook before we do key[0] operations, or better yet check if the table has keys in the install step and fail if its a singleton table.

in MODE and the client we currently need custom handling of singleton tables because of the empty array

The store event has enough information to distinguish singleton keys vs. not (empty array vs. non-empty array), so this seems like something we can/should work around in our indexers.

I think I've got a decent approach for this in the TS sync stack/indexer, where a singleton key (for the purposes of the unique ID column) is effectively 0x and when you decode it it becomes [].

@alvrs
Copy link
Member Author

alvrs commented Sep 11, 2023

In my opinion it would be cleaner to use 0x00 as key for the singleton table to avoid the need for special cases for handling this (ie checking the table's key length). One example use case is a module like the previous SnapSync module that installs KeysInTable on all tables and then uses it to fetch the table's values. It doesn't feel right to handle singleton tables differently here - the SnapSync module would have to be aware of which tables are singleton tables and which are not and then fetch the singleton table's value via a hardcoded [] key. And even with that it wouldn't be possible to distinguish onchain whether the table has a value or no value (it would return the default value even if no value is set).

@alvrs
Copy link
Member Author

alvrs commented Sep 11, 2023

Another approach for how the KeysInTable / KeysWithValue modules could handle this might be storing abiEncodedKeyTuple instead of storing each key in a separate array. That way we would be able to infer whether there is a value set in the table and also would be able to support an arbitrary number of keys (currently the limit is 5). And we'd be able to continue to use the empty array for singleton tables, which is a better "conceptual" match, and slightly cheaper in gas.

@holic
Copy link
Member

holic commented Sep 11, 2023

Another approach for how the KeysInTable / KeysWithValue modules could handle this might be storing abiEncodedKeyTuple instead of storing each key in a separate array. That way we would be able to infer whether there is a value set in the table and also would be able to support an arbitrary number of keys (currently the limit is 5). And we'd be able to continue to use the empty array for singleton tables, which is a better "conceptual" match, and slightly cheaper in gas.

Much more on board with this approach!

@holic
Copy link
Member

holic commented Sep 12, 2023

Started fiddling with this but quickly realized we can't take that approach without support for bytes[]. KeysWithValue and KeysInTable both return an array of keys, and if we switch to ABI-encoded keys (arbitrary length bytes), then we won't be able to easily store these.

We could work around this by storing the hash of the ABI-encoded keys in a bytes32[] and then a look up table for hash -> key, but that seems fairly gassy (one storage read per key in the array).

@alvrs
Copy link
Member Author

alvrs commented Sep 12, 2023

More context from discussion on discord:

  • storing the list of hash of keys as bytes32[] and a "hash to key tuple" lookup table would have the advantage that we don't have to store the hash of a key tuple again if it was indexed at some point before, so it might be more gas efficient if there is a limited number of keys that is shared between multiple tables, or values of the same set of keys change often
  • we agree that it's still more elegant to use [] as key tuple for singleton tables than an explicit "singleton key"

@alvrs alvrs removed this from the Contracts stable milestone Sep 13, 2023
@holic
Copy link
Member

holic commented Sep 29, 2023

we decided to keep the [] singleton key for consistency (which gets encoded as 0x in RECS entity IDs, DB primary keys, etc.)

we'll have to figure out an approach for this in our various modules, but saving that for another time

@holic holic closed this as completed Sep 29, 2023
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

No branches or pull requests

2 participants