Skip to content

feat: replace ephemeral with offchain tables#1496

Closed
dk1a wants to merge 7 commits intomainfrom
dk1a/onlyOffchain
Closed

feat: replace ephemeral with offchain tables#1496
dk1a wants to merge 7 commits intomainfrom
dk1a/onlyOffchain

Conversation

@dk1a
Copy link
Contributor

@dk1a dk1a commented Sep 14, 2023

fixes #1496

@changeset-bot
Copy link

changeset-bot bot commented Sep 14, 2023

⚠️ No Changeset found

Latest commit: f2f47e4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

bytes memory data,
FieldLayout fieldLayout
) internal {
if (!Tables.getOffchainOnly(tableId)) {
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 want/need these checks?

Copy link
Member

Choose a reason for hiding this comment

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

this might be another good use case for a "resource type" encoded in the resource selector, i.e. table ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's pretty important to avoid divergence of offchain and onchain state. Yeah a resource type would be a nice alternative

@dk1a dk1a changed the title WIP onlyOffchain feat: replace ephemeral with offchain tables Sep 15, 2023
@frolic
Copy link
Member

frolic commented Sep 15, 2023

Design question: since we're using regular events and soon we'll encode the notion of "offchain table" into the resource selector/table ID (which we'll ~always have access to in these methods), would it make sense to not have separate methods for emitting and just use e.g. setRecord and deleteRecord?

If so, should we do #1498 first to avoid API churn? (two PRs instead of potentially three)

@dk1a
Copy link
Contributor Author

dk1a commented Sep 15, 2023

would it make sense to not have separate methods for emitting and just use e.g. setRecord and deleteRecord?

I think it'd make sense, but it'd also add a little gas overhead to onchain tables, since we'll always need to extract resource type and check it. The overhead is possibly worth having 2 fewer methods

@frolic
Copy link
Member

frolic commented Sep 15, 2023

from discord:

  • we're gonna pause on this until WIP resource type #1498 is done so we can use it here (avoids one extra refactor PR and API churn)
  • we're gonna remove offchain-specific methods (emitSet, etc.) in favor of the emission-only check (via table type encoded into table ID) being inside existing methods (setRecord, etc.)
  • we're gonna keep the offchainOnly flag in registerTable for clarity of intent and so we can check that the table ID has the proper table type prefix

@frolic
Copy link
Member

frolic commented Sep 20, 2023

closing in favor of #1558

@frolic frolic closed this Sep 20, 2023
@alvrs
Copy link
Member

alvrs commented Sep 20, 2023

For context: decided to start over because we changed the approach away from a separate method for ephemeral tables and towards conditionally early returning in the main StoreCore methods based on the table's resource type, so it seemed like a smaller lift to start fresh than to fix the merge conflicts of this PR with the recent changes

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.

Replace ephemeral tables with offchain tables

3 participants