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.

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

Merged
merged 4 commits into from Sep 20, 2023

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Sep 20, 2023

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

🦋 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

@@ -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

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";

@@ -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

…le and corresponding checks (#1557)

Co-authored-by: Kevin Ingersoll <kingersoll@gmail.com>
@alvrs alvrs merged commit c7030fa into alvrs/resourcetype Sep 20, 2023
9 of 10 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants