-
Notifications
You must be signed in to change notification settings - Fork 189
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): replace ResourceSelector
with ResourceId
and WorldResourceId
#1544
Conversation
🦋 Changeset detectedLatest commit: de9124e 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 |
95539e3
to
436f814
Compare
bytes32 constant _tableId = bytes32(abi.encodePacked(bytes16(""), bytes16("Multi"))); | ||
bytes32 constant MultiTableId = _tableId; | ||
ResourceId constant _tableId = ResourceId.wrap( | ||
bytes32(abi.encodePacked(RESOURCE_TABLE, bytes14(""), bytes16("Multi"))) |
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.
ooc is there any gas benefit to bytes.concat
instead of abi.encodePacked
here?
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.
bytes.concat
is not considered a static function (so can't be used for constants), abi.encodePacked
is 🤦♂️
error RootInstallModeNotSupported(); | ||
error NonRootInstallNotSupported(); |
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.
maybe for followup: should we make these more consistent by removing "mode" from one or adding it to the other?
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.
can do that in #1126
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.
Oh I mean "root install mode not supported" vs "non root install not supported", we call it a "mode" in one instance but not the other
but yeah, can resolve as part of naming pass in #1126
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.
Oh I mean "root install mode not supported" vs "non root install not supported", we call it a "mode" in one instance but not the other
I understood, I meant we can do this renaming as part of the naming pass in 1126
Co-authored-by: Kevin Ingersoll <[email protected]>
fixes #1394
fixes #1367
TODO
ResourceId
into a user type - had a bunch of runtime errors wherebytes14
was automatically casted intobytes32
, which would have been caught at compile time if resource type was a user typeTODOs in followups
bytes14 namespace
as argument, and useResourceId namespaceId
instead (feat(world): useResourceId namespaceId
for all methods acting on the namespace as a resource #1555)ResourceType
table. Need to find a way to check whether a given resource already exists though (feat(store,world): replaceResourceType
table withResourceId
table and corresponding checks #1557)ResourceType
table withResourceId
table and corresponding checks #1557)