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

.changeset/beige-radios-drop.md

@@ -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);
+```

packages/world/gas-report.json

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

packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol

@@ -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
+    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);
+    }
 
     // Systems = mapping from resourceSelector to system address and publicAccess
     Systems.set(resourceSelector, address(system), publicAccess);

packages/world/test/World.t.sol

@@ -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";
@@ -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();
@@ -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");