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(store): add field index to Store_SpliceDynamicData event #2279

Merged
merged 16 commits into from Feb 21, 2024

Conversation

yonadaaa
Copy link
Contributor

@yonadaaa yonadaaa commented Feb 19, 2024

Fixes #2222

Copy link

changeset-bot bot commented Feb 19, 2024

🦋 Changeset detected

Latest commit: 81be743

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

This PR includes changesets to release 30 packages
Name Type
@latticexyz/store Major
@latticexyz/store-sync Major
@latticexyz/cli Major
@latticexyz/dev-tools Major
@latticexyz/react Major
@latticexyz/store-indexer Major
@latticexyz/world-modules Major
@latticexyz/world Major
@latticexyz/abi-ts Major
@latticexyz/block-logs-stream Major
@latticexyz/common Major
@latticexyz/config Major
create-mud Major
@latticexyz/ecs-browser Major
@latticexyz/faucet Major
@latticexyz/gas-report Major
@latticexyz/network Major
@latticexyz/noise Major
@latticexyz/phaserx Major
@latticexyz/protocol-parser 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

| `start` | `uint48` | The start position in bytes for the splice operation. |
| `deleteCount` | `uint40` | The number of bytes to delete in the splice operation. |
| `encodedLengths` | `PackedCounter` | The encoded lengths of the dynamic data of the record. |
| `data` | `bytes` | The data to insert into the dynamic data of the record at the start byte. |
Copy link
Member

@holic holic Feb 21, 2024

Choose a reason for hiding this comment

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

Seeing this all laid out, I am wondering:

  • Do we still like the Store_SpliceDynamicData name since splice events are now ~guaranteed to operate on one field at a time and it includes both information for splicing within the field and for all of the dynamic data? Wondering if this should be something like Store_SpliceDynamicField to clarify the intent.
  • With above, I am also wondering if it's worth clarifying some argument names like startWithinField to fieldStart and start -> dynamicDataStart.
  • "dynamic" in dynamicFieldIndex might be redundant since this is a Store_SpliceDynamicData event. Also worth clarifying if this is the field index in the list of fields for the whole schema or field index among dynamic fields (I think it's the former?)

Copy link
Member

Choose a reason for hiding this comment

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

Do we need startWithinField? Couldn't that be computed from dynamicFieldIndex, start and encodedLengths? I'd prefer adding that one additional field to the event than refactoring the event name and other field names. The current naming feels very intuitive if you're thinking of the data as one byte blob, while the Store_SpliceDynamicField naming would be a little more intuitive if you're storing data in separate locations, but I don't there is a strong enough bias in either direction to change the name at this point.

Copy link
Member

Choose a reason for hiding this comment

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

const fieldLengths = getFieldLengths(encodedLengths); // some sort of decoding function
const fieldStart = fieldLengths.slice(0, dynamicFieldIndex).reduce((sum, length) => sum + length, 0);
const startWithinField = start - fieldStart;

checks out ✅

yonadaaa and others added 2 commits February 21, 2024 12:26
Co-authored-by: Kevin Ingersoll <kingersoll@gmail.com>
@holic holic changed the title feat(store): emit field index in Store_SpliceDynamicData feat(store): add field index to Store_SpliceDynamicData event Feb 21, 2024
@yonadaaa yonadaaa merged commit 8193136 into main Feb 21, 2024
11 checks passed
@yonadaaa yonadaaa deleted the yonadaaa/emit-dynamic-field-index branch February 21, 2024 14:01
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.

consider including field index (and field bytes offset) in Store events
3 participants