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

feat(world): add support for upgrading systems #1378

Merged
merged 4 commits into from
Sep 5, 2023
+93 −18
Merged

feat(world): add support for upgrading systems #1378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1754596
feat(world): add support for upgrading systems
alvrs Aug 31, 2023
c809cd8
add tests
alvrs Sep 1, 2023
4217b5e
update gas report
alvrs Sep 1, 2023
d172dcb
Create beige-radios-drop.md
alvrs Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/beige-radios-drop.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
"@latticexyz/world": minor
---

It is now possible to upgrade systems by calling `registerSystem` again with an existing system id (resource selector).

```solidity
// Register a system
world.registerSystem(systemId, systemAddress, publicAccess);

// Upgrade the system by calling `registerSystem` with the
// same system id but a new system address or publicAccess flag
world.registerSystem(systemId, newSystemAddress, newPublicAccess);
```
12 changes: 6 additions & 6 deletions packages/world/gas-report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstall",
"name": "install unique entity module",
"gasUsed": 722425
"gasUsed": 726281
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand All @@ -267,7 +267,7 @@
"file": "test/UniqueEntityModule.t.sol",
"test": "testInstallRoot",
"name": "installRoot unique entity module",
"gasUsed": 701392
"gasUsed": 705270
},
{
"file": "test/UniqueEntityModule.t.sol",
Expand Down Expand Up @@ -309,19 +309,19 @@
"file": "test/World.t.sol",
"test": "testRegisterFallbackSystem",
"name": "Register a fallback system",
"gasUsed": 70416
"gasUsed": 70405
},
{
"file": "test/World.t.sol",
"test": "testRegisterFallbackSystem",
"name": "Register a root fallback system",
"gasUsed": 63722
"gasUsed": 63711
},
{
"file": "test/World.t.sol",
"test": "testRegisterFunctionSelector",
"name": "Register a function selector",
"gasUsed": 91010
"gasUsed": 90999
},
{
"file": "test/World.t.sol",
Expand All @@ -333,7 +333,7 @@
"file": "test/World.t.sol",
"test": "testRegisterRootFunctionSelector",
"name": "Register a root function selector",
"gasUsed": 79633
"gasUsed": 79622
},
{
"file": "test/World.t.sol",
Expand Down
31 changes: 25 additions & 6 deletions packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,27 +62,46 @@ contract WorldRegistrationSystem is System, IWorldErrors {
* If the namespace doesn't exist yet, it is registered.
* The system is granted access to its namespace, so it can write to any table in the same namespace.
* If publicAccess is true, no access control check is performed for calling the system.
*
* Note: this function doesn't check whether a system already exists at the given selector,
* making it possible to upgrade systems.
*/
function registerSystem(bytes32 resourceSelector, WorldContextConsumer system, bool publicAccess) public virtual {
// Require the name to not be the namespace's root name
if (resourceSelector.getName() == ROOT_NAME) revert InvalidSelector(resourceSelector.toString());

// 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
Copy link
Member

Choose a reason for hiding this comment

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

ooc why? what if I want the same system in multiple namespaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

@holic holic Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hmmm, interesting! should we do access based on selector instead of/in addition to address?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

bytes32 existingResourceSelector = SystemRegistry.get(address(system));
if (existingResourceSelector != 0 && existingResourceSelector != resourceSelector) {
revert SystemExists(address(system));
}

// If the namespace doesn't exist yet, register it
// otherwise require caller to own the namespace
bytes16 namespace = resourceSelector.getNamespace();
if (ResourceType.get(namespace) == Resource.NONE) registerNamespace(namespace);
else AccessControl.requireOwnerOrSelf(namespace, _msgSender());

// Require no resource to exist at this selector yet
if (ResourceType.get(resourceSelector) != Resource.NONE) {
// Require no resource other than a system to exist at this selector yet
Resource resourceType = ResourceType.get(resourceSelector);
if (resourceType != Resource.NONE && resourceType != Resource.SYSTEM) {
revert ResourceExists(resourceSelector.toString());
}

// Store the system resource type
ResourceType.set(resourceSelector, Resource.SYSTEM);
// Check if a system already exists at this resource selector
address existingSystem = Systems.getSystem(resourceSelector);

// If there is an existing system with this resource selector, remove it
if (existingSystem != address(0)) {
// Remove the existing system from the system registry
SystemRegistry.deleteRecord(existingSystem);

// Remove the existing system's access to its namespace
ResourceAccess.deleteRecord(namespace, existingSystem);
} else {
// Otherwise, this is a new system, so register its resource type
ResourceType.set(resourceSelector, Resource.SYSTEM);
}
Copy link
Member

@holic holic Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

agree that might make sense!

Copy link
Member

Choose a reason for hiding this comment

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

opened an issue here: #1394


// Systems = mapping from resourceSelector to system address and publicAccess
Systems.set(resourceSelector, address(system), publicAccess);
Expand Down
54 changes: 48 additions & 6 deletions packages/world/test/World.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@ import { World } from "../src/World.sol";
import { System } from "../src/System.sol";
import { ResourceSelector } from "../src/ResourceSelector.sol";
import { ROOT_NAMESPACE, ROOT_NAME, UNLIMITED_DELEGATION } from "../src/constants.sol";
import { Resource } from "../src/Types.sol";

import { NamespaceOwner, NamespaceOwnerTableId } from "../src/tables/NamespaceOwner.sol";
import { ResourceAccess } from "../src/tables/ResourceAccess.sol";

import { CoreModule } from "../src/modules/core/CoreModule.sol";
import { Systems } from "../src/modules/core/tables/Systems.sol";
import { SystemRegistry } from "../src/modules/core/tables/SystemRegistry.sol";
import { ResourceType } from "../src/modules/core/tables/ResourceType.sol";

import { IBaseWorld } from "../src/interfaces/IBaseWorld.sol";
import { IWorldErrors } from "../src/interfaces/IWorldErrors.sol";
Expand Down Expand Up @@ -355,16 +358,19 @@ contract WorldTest is Test, GasReporter {
world.registerSystem(ResourceSelector.from("newNamespace", "testSystem"), new System(), false);
assertEq(NamespaceOwner.get(world, "newNamespace"), address(this));

// Expect an error when registering an existing system
// Expect an error when registering an existing system at a new resource selector
vm.expectRevert(abi.encodeWithSelector(IWorldErrors.SystemExists.selector, address(system)));
world.registerSystem(ResourceSelector.from("", "newSystem"), system, true);

// Expect an error when registering a system at an existing resource selector
System newSystem = new System();
// Don't expect an error when updating the public access of an existing system
world.registerSystem(resourceSelector, system, true);

// Expect an error when registering a system at an existing resource selector
vm.expectRevert(abi.encodeWithSelector(IWorldErrors.ResourceExists.selector, resourceSelector.toString()));
world.registerSystem(ResourceSelector.from("", "testSystem"), newSystem, true);
// Expect an error when registering a system at an existing resource selector of a different type
System newSystem = new System();
bytes32 tableId = ResourceSelector.from("", "testTable");
world.registerTable(tableId, defaultKeySchema, Bool.getValueSchema(), new string[](1), new string[](1));
vm.expectRevert(abi.encodeWithSelector(IWorldErrors.ResourceExists.selector, tableId.toString()));
world.registerSystem(tableId, newSystem, true);

// Expect an error when registering a system in a namespace is not owned by the caller
System yetAnotherSystem = new System();
Expand All @@ -376,6 +382,42 @@ contract WorldTest is Test, GasReporter {
world.registerSystem(ResourceSelector.from("", "rootSystem"), yetAnotherSystem, true);
}

function testUpgradeSystem() public {
bytes16 namespace = "testNamespace";
bytes16 systemName = "testSystem";
bytes32 systemId = ResourceSelector.from(namespace, systemName);

// Register a system
System oldSystem = new System();
world.registerSystem(systemId, oldSystem, true);

// Upgrade the system and set public access to false
System newSystem = new System();
world.registerSystem(systemId, newSystem, false);

// Expect the system address and public access to be updated in the System table
(address registeredAddress, bool publicAccess) = Systems.get(world, systemId);
assertEq(registeredAddress, address(newSystem));
assertEq(publicAccess, false);

// Expect the SystemRegistry table to not have a reference to the old system anymore
bytes32 registeredSystemId = SystemRegistry.get(world, address(oldSystem));
assertEq(registeredSystemId, bytes32(0));

// Expect the SystemRegistry table to have a reference to the new system
registeredSystemId = SystemRegistry.get(world, address(newSystem));
assertEq(registeredSystemId, systemId);

// Expect the old system to not have access to the namespace anymore
assertFalse(ResourceAccess.get(world, namespace, address(oldSystem)));

// Expect the new system to have access to the namespace
assertTrue(ResourceAccess.get(world, namespace, address(newSystem)));

// Expect the resource type to still be SYSTEM
assertEq(uint8(ResourceType.get(world, systemId)), uint8(Resource.SYSTEM));
}

function testDuplicateSelectors() public {
// Register a new table
bytes32 resourceSelector = ResourceSelector.from("namespace", "name");
Expand Down