-
Notifications
You must be signed in to change notification settings - Fork 196
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,world): use user-types for ResourceId
, FieldLayout
and Schema
in table libraries
#1586
Conversation
🦋 Changeset detectedLatest commit: 55ddc6d The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
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 |
50f79c7
to
26c3b9a
Compare
"gasUsed": 10240 | ||
"gasUsed": 5609 |
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.
these gas changes come from not loading the _tableId
and _keyTuple
from storage in the gas test
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.
same for all tests or just this one? I noticed some other gas improvements below
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.
wrapping
/unwrapping
in itself should not significantly impact gas, so i think most of the gas variation in here comes from the changed gas tests
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.
So nice!
5738256
to
65febd5
Compare
// TODO: adjust when schemas are automatically resolved | ||
export const schemasTable = { | ||
...storeConfig.tables.Tables, | ||
valueSchema: resolveUserTypes(storeConfig.tables.Tables.valueSchema, storeConfig.userTypes), | ||
}; | ||
|
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.
kind of annoying, but we need this workaround for now to land the contract changes (until we have a better way to resolve the user type config with strong types)
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.
hopefully coming in #1561 (but still a lot of work left to do)
ResourceId tableId = _tableId; | ||
bytes32[] memory keyTuple = _keyTuple; |
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.
we could use immutable too, it's about as convenient as storage, but without the gas overhead. It's only useful for constant-like values tho
The base branch was changed.
Thanks to #1566 we can now use the real user-types in table libraries and don't need to manually
wrap
/unwrap
everywhere