feat(world): use ResourceId namespaceId for all methods acting on the namespace as a resource#1555
Conversation
🦋 Changeset detectedLatest commit: 7309f54 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 |
| "/", | ||
| resourceName == ROOT_NAME ? ROOT_NAME_STRING : resourceName, | ||
| ".", | ||
| resourceType |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
don't have a strong opinion on the separator string, _ was just a bit confusing in ROOT_NAMESPACE_ROOT_NAME. : works for me!
There was a problem hiding this comment.
how about namespace:name:type? (to make the order match the encoding)
There was a problem hiding this comment.
I would still argue for the more human readable form here, and asked in the other PR about moving the order for our helpers
There was a problem hiding this comment.
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
packages/world/src/SystemCall.sol
Outdated
| import { console } from "forge-std/console.sol"; | ||
|
|
There was a problem hiding this comment.
| import { console } from "forge-std/console.sol"; |
| function revokeAccess(ResourceId resourceId, address grantee) external; | ||
|
|
||
| function transferOwnership(bytes14 namespace, address newOwner) external; | ||
| function transferOwnership(ResourceId namespaceId, address newOwner) external; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
oh right, I forgot we have a namespace type, that makes sense
318f722 to
4e4c8b5
Compare
ResourceId namespaceId for all methods acting on the namespace as a resource
4e4c8b5 to
522b070
Compare
… namespace as a resource
cc53dbc to
c64ac48
Compare
…le and corresponding checks (#1557) Co-authored-by: Kevin Ingersoll <kingersoll@gmail.com>
Fixes #1552
Followup to #1544
All methods acting on namespaces as resources (ownership and balances of namespaces) now use
ResourceId namespaceIdinstead ofbytes14 namespacefor clarity and improved compile time type safety (no more implicit casting)WorldRegistrationSystemnow usesResourceId namespaceIdinstead ofbytes14 namespaceforregisterNamespaceBalanceTransferSystemnow usesResourceId namespaceIdinstead ofbytes14 namespacefor all methodsAccessManagementSystemnow usesResourceId namespaceIdinstead ofbytes14 namespacefortransferOwnershipand internal ownership checks