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
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
bf9f113
feat(store): emit field index in Store_SpliceDynamicData
yonadaaa b877f16
wip: add startWithinField to event
yonadaaa 647fbe7
chore: docs
yonadaaa 9a707e3
chore: add comments
yonadaaa ec81d27
chore: regenerate test data
yonadaaa 8f4864e
test: update store-sync test
yonadaaa 9743557
test: add back storeCore tests
yonadaaa 7bccf9d
test: fix storecoredynamic test
yonadaaa 70b35c6
docs: expand comment
yonadaaa 40c33eb
chore: remove startWithinField from event
yonadaaa c22ce58
chore: changeset
yonadaaa 9fcdbaf
chore: major bumps
yonadaaa ea035d0
Merge remote-tracking branch 'origin/main' into yonadaaa/emit-dynamic…
yonadaaa 888bf76
chore: gas report
yonadaaa 40950d7
test: fix story-sync tests
yonadaaa 81be743
Update .changeset/rich-deers-doubt.md
holic File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
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 likeStore_SpliceDynamicField
to clarify the intent.startWithinField
tofieldStart
andstart
->dynamicDataStart
.dynamicFieldIndex
might be redundant since this is aStore_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?)There was a problem hiding this comment.
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 fromdynamicFieldIndex
,start
andencodedLengths
? 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 theStore_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checks out ✅