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(world): use ResourceId namespaceId for all methods acting on the namespace as a resource #1555

Merged
merged 4 commits into from
Sep 20, 2023
Merged

feat(world): use ResourceId namespaceId for all methods acting on the namespace as a resource #1555

merged 4 commits into from
Sep 20, 2023

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Sep 20, 2023 •
edited
Loading

Fixes #1552
Followup to #1544

All methods acting on namespaces as resources (ownership and balances of namespaces) now use ResourceId namespaceId instead of bytes14 namespace for clarity and improved compile time type safety (no more implicit casting)

  • WorldRegistrationSystem now uses ResourceId namespaceId instead of bytes14 namespace for registerNamespace
  • BalanceTransferSystem now uses ResourceId namespaceId instead of bytes14 namespace for all methods
  • AccessManagementSystem now uses ResourceId namespaceId instead of bytes14 namespace for transferOwnership and internal ownership checks

@changeset-bot
Copy link

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

🦋 Changeset detected

Latest commit: 7309f54

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

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

holic
holic reviewed Sep 20, 2023
View reviewed changes
packages/world/src/WorldResourceId.sol
@@ -92,7 +92,7 @@ library WorldResourceIdInstance {
string(
abi.encodePacked(
resourceNamespace == ROOT_NAMESPACE ? ROOT_NAMESPACE_STRING : resourceNamespace,
"_",
"/",
resourceName == ROOT_NAME ? ROOT_NAME_STRING : resourceName,
".",
resourceType
Copy link
Member

Choose a reason for hiding this comment

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

I think this is leftover from our "file" analogy, which I think we've moved on from?

We should agree on a stringified version of these resource IDs, and my vote would be something like

type_namespace_name
type:namespace:name

(Stripe uses e.g. acct_1234 and ch_1234)

Copy link
Member Author

Choose a reason for hiding this comment

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

don't have a strong opinion on the separator string, _ was just a bit confusing in ROOT_NAMESPACE_ROOT_NAME. : works for me!

Copy link
Member Author

Choose a reason for hiding this comment

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

how about namespace:name:type? (to make the order match the encoding)

Copy link
Member

Choose a reason for hiding this comment

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

I would still argue for the more human readable form here, and asked in the other PR about moving the order for our helpers

Copy link
Member Author

Choose a reason for hiding this comment

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

from discord for future reference

I’m fine sticking with encoding order for resources
not my favorite but I’m optimizing for consistency and gas

holic
holic reviewed Sep 20, 2023
View reviewed changes
packages/world/src/SystemCall.sol Outdated
Comment on lines 4 to 5
import { console } from "forge-std/console.sol";

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { console } from "forge-std/console.sol";

holic
holic reviewed Sep 20, 2023
View reviewed changes
packages/world/src/interfaces/IAccessManagementSystem.sol
@@ -10,5 +10,5 @@ interface IAccessManagementSystem {

function revokeAccess(ResourceId resourceId, address grantee) external;

function transferOwnership(bytes14 namespace, address newOwner) external;
function transferOwnership(ResourceId namespaceId, address newOwner) external;
Copy link
Member

Choose a reason for hiding this comment

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

We sorta talked about this in the other PR but I wonder if there's any value to having a NamespaceId wrapper rather than full resource ID where we mask/shift/slice the namespace from it.

Copy link
Member Author

Choose a reason for hiding this comment

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

would prefer one user type for all of these because i want to add checks like namespaceId.isType(RESOURCE_NAMESPACE) in the next PR - if namespaceId uses a different user type, we'd have to duplicate the ResourceId library for it. (We can't trust the id provided as argument to actually be a valid namespace id, even if it would be of type NamespaceId, because anyone can just wrap whatever with NamespaceId)

Copy link
Member

Choose a reason for hiding this comment

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

oh right, I forgot we have a namespace type, that makes sense

@alvrs alvrs force-pushed the alvrs/namespace-id branch from 318f722 to 4e4c8b5 Compare September 20, 2023 13:37
@alvrs alvrs changed the title feat(world): use ResourceId namespaceId for all methods acting on the namespace as a resource feat(world): use ResourceId namespaceId for all methods acting on the namespace as a resource Sep 20, 2023
@alvrs alvrs mentioned this pull request Sep 20, 2023
Merged
4 tasks
@alvrs alvrs force-pushed the alvrs/namespace-id branch from 4e4c8b5 to 522b070 Compare September 20, 2023 17:17
@alvrs alvrs force-pushed the alvrs/namespace-id branch from cc53dbc to c64ac48 Compare September 20, 2023 19:33
@alvrs alvrs marked this pull request as ready for review September 20, 2023 19:56
@alvrs alvrs requested a review from dk1a as a code owner September 20, 2023 19:56
@alvrs alvrs requested a review from holic September 20, 2023 19:56
@alvrs @holic
feat(store,world): replace ResourceType table with ResourceId tab…
dec9af7 
…le and corresponding checks (#1557)

Co-authored-by: Kevin Ingersoll <[email protected]>
@alvrs alvrs merged commit c7030fa into alvrs/resourcetype Sep 20, 2023
@alvrs alvrs deleted the alvrs/namespace-id branch September 20, 2023 22:33
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.
2 participants
@alvrs @holic