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): indexed tableId in store events #1520

Merged
merged 8 commits into from
Sep 16, 2023
Merged

Conversation

holic
Copy link
Member

@holic holic commented Sep 16, 2023

closes #1127

@changeset-bot
Copy link

changeset-bot bot commented Sep 16, 2023

🦋 Changeset detected

Latest commit: 97c1f3c

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

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

},
{
"file": "test/StoreCoreDynamic.t.sol",
"test": "testPopFromSecondField",
"name": "pop from field (warm, 1 slot, 1 uint32 item)",
"gasUsed": 15405
"gasUsed": 16244
Copy link
Member Author

Choose a reason for hiding this comment

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

any idea why this would add so much gas here when other operations are like +100 gas?

Copy link
Member

Choose a reason for hiding this comment

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

it looks like it adds around 900 gas for operations where we emit an event, while the other methods are getters (why does it even increase gas for getters at all 🤔)

Copy link
Member Author

@holic holic Sep 16, 2023

Choose a reason for hiding this comment

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

I think the gas for getters is just the usual sway we see between gas reports for some unexplained reason (optimizer differences when bytecode changes maybe?)

gonna revert, 900 per write seems to high for a "maybe nice"

This reverts commit 592500b.
alvrs
alvrs previously approved these changes Sep 16, 2023
@alvrs alvrs changed the title feat(store): indexed tableId feat(store): indexed tableId in store events Sep 16, 2023
@alvrs alvrs merged commit 99ab9cd into main Sep 16, 2023
9 checks passed
@alvrs alvrs deleted the holic/indexed-table-arg branch September 16, 2023 22:19
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.

Add indexed to tableId in all Store update events
2 participants