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.

Sign up for GitHub

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

Jump to bottom

feat(store,world): replace ephemeral tables with offchain tables #1558

Merged
merged 2 commits into from
Sep 21, 2023
Merged

feat(store,world): replace ephemeral tables with offchain tables #1558

merged 2 commits into from
Sep 21, 2023

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Sep 20, 2023 •
edited
Loading

Fixes #1123
Replacement for #1496 (since we changed the approach)

@changeset-bot
Copy link

changeset-bot bot commented Sep 20, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 3a803cc

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

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

@alvrs alvrs changed the base branch from main to alvrs/resource-checks September 20, 2023 21:09
@alvrs alvrs marked this pull request as ready for review September 20, 2023 22:24
@alvrs alvrs requested review from holic and dk1a as code owners September 20, 2023 22:24
Base automatically changed from alvrs/resource-checks to alvrs/namespace-id September 20, 2023 22:29
Base automatically changed from alvrs/namespace-id to alvrs/resourcetype September 20, 2023 22:33
holic
holic approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

so glad to get rid of all the ephemeral specific stuff, this feels a lot cleaner!

but I am surprised gas went up across the board

packages/store/src/StoreCore.sol
// Early return if the table is an offchain table
if (tableId.getType() == RESOURCE_OFFCHAIN_TABLE) {
return;
}
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 support splicing for offchain tables?

Copy link
Member Author

Choose a reason for hiding this comment

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

we do for static fields! Since splicing for static fields doesn't require reading anything from storage, can just support it for static fields without any further changes. For dynamic fields it's not supported because it would require reading the dynamic field's length from storage (but it's not stored in storage), see #1558 (comment)

Codegen doesn't render the dynamic field methods for offchain tables, but does render all other ones, so much more "real" tables than before with ephemeral

Copy link
Member

Choose a reason for hiding this comment

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

ohh I see, I was thinking about push/pop but that's a dynamic field thing 👌

@holic holic mentioned this pull request Sep 20, 2023
Closed
@alvrs
Copy link
Member Author

alvrs commented Sep 20, 2023

but I am surprised gas went up across the board

it's because we're doing an additional check in all store methods to early return if it's an offchain table, and that check costs 200 gas

alvrs
alvrs commented Sep 20, 2023
View reviewed changes
packages/store/src/StoreCore.sol Outdated
Comment on lines 687 to 691
// Splicing dynamic data is not supported for offchain tables, because it
// requires reading the previous encoded lengths from storage
if (tableId.getType() == RESOURCE_OFFCHAIN_TABLE) {
revert IStoreErrors.StoreCore_InvalidResourceType(string(bytes.concat(RESOURCE_OFFCHAIN_TABLE)));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

re: no support for dynamic splice

@alvrs alvrs mentioned this pull request Sep 20, 2023
Merged
4 tasks
holic
holic previously approved these changes Sep 21, 2023
View reviewed changes
holic
holic reviewed Sep 21, 2023
View reviewed changes
.changeset/small-chicken-repair.md Outdated

```diff
- EphemeralTable.emitEphemeral(value);
+ OffchainTable.setRecord(value);
Copy link
Member

@holic holic Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

should this be OffchainTable.set(key, value)?

Base automatically changed from alvrs/resourcetype to main September 21, 2023 16:02
@alvrs alvrs dismissed holic’s stale review September 21, 2023 16:02

The base branch was changed.

@alvrs alvrs merged commit bfcb293 into main Sep 21, 2023
@alvrs alvrs deleted the alvrs/offchain branch September 21, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holic holic holic approved these changes

@dk1a dk1a Awaiting requested review from dk1a

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace ephemeral tables with offchain tables
2 participants
@alvrs @holic