diff --git a/.changeset/beige-radios-drop.md b/.changeset/beige-radios-drop.md new file mode 100644 index 0000000000..c821afbfe4 --- /dev/null +++ b/.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); +``` diff --git a/packages/world/gas-report.json b/packages/world/gas-report.json index 7a66c2d785..b142eb66ee 100644 --- a/packages/world/gas-report.json +++ b/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", diff --git a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol index ceba247354..9f38aa0c33 100644 --- a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol +++ b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol @@ -62,13 +62,19 @@ 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 @@ -76,13 +82,26 @@ contract WorldRegistrationSystem is System, IWorldErrors { 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); diff --git a/packages/world/test/World.t.sol b/packages/world/test/World.t.sol index 5044a3f4d5..f9f47e2756 100644 --- a/packages/world/test/World.t.sol +++ b/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");