feat(world): add support for upgrading systems#1378
Conversation
🦋 Changeset detectedLatest commit: d172dcb The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 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 |
alvrs
force-pushed
the
alvrs/upgrade-systems
branch
from
c45be6a to
4217b5e
Compare
|
|
||
| // Require the system to not exist yet | ||
| if (SystemRegistry.get(address(system)) != 0) revert SystemExists(address(system)); | ||
| // Require this system to not be registered at a different resource selector yet |
There was a problem hiding this comment.
ooc why? what if I want the same system in multiple namespaces?
There was a problem hiding this comment.
Since access control is based on addresses that would be a security risk (namespace A might have given system X access, now you register system X in namespace B without realising it might mean whoever has access to the system based on namespace A can now also call the system to access namespace B)
There was a problem hiding this comment.
hmmm, interesting! should we do access based on selector instead of/in addition to address?
There was a problem hiding this comment.
Since access control is also used for EOAs it couldn't be in instead of address (since EOAs don't have a registered selector). It would probably be possible to have an additional concept of access control based on the selector, but there are more complexities: if a system is registered at two selectors, it's not possible to infer the selector based on the system address (since there is no 1-1 mapping anymore), so a system would have to provide its own selector for every call that requires access control. That would require the system to be aware of where its registered (currently it doesn't care) and might have more security implications. Overall I think it's not worth the additional complexity to allow a system to be registered multiple times.
| } else { | ||
| // Otherwise, this is a new system, so register its resource type | ||
| ResourceType.set(resourceSelector, Resource.SYSTEM); | ||
| } |
There was a problem hiding this comment.
idle thought: I wonder if it's worth encoding the resource type into the selector, so selectors become something like
type (bytes2) + namespace (bytes14) + name (bytes16)
this would save a little on storage (but maybe not in any meaningful way if it's only used during registration) and could help during introspection (it's part of the selector/identifier itself rather than a separate lookup)
(Stripe has this for all IDs where each ID has a prefix of the kind of entity it is, e.g. acct_AbCdEf123 for accounts or ch_AbCdEf123 for charges and it's a really nice human-readability thing)
There was a problem hiding this comment.
agree that might make sense!
frolic
left a comment
frolic
left a comment
There was a problem hiding this comment.
implementation looks good, one question around registering the same system at multiple locations (specifically namespaces)
3 participants
fixes #1140. Context in the changeset.
TODO:
Decide on whether we should store the list of addresses with access to a certain resource somewhere to be able to remove access for the previous system.-> I'm tending towards no since granting a system access is a manual action, so it feels wrong to automatically revoke access and grant it to a new system.TODO in followup: Add CLI support for upgrading systems