From 1754596f4d4f3ee7f503984eaa949528acc0fe48 Mon Sep 17 00:00:00 2001 From: alvrs Date: Thu, 31 Aug 2023 16:50:36 +0200 Subject: [PATCH 1/4] feat(world): add support for upgrading systems --- .../WorldRegistrationSystem.sol | 27 ++++++++++++++----- packages/world/test/World.t.sol | 4 +++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol index ceba247354..c1a06fb87c 100644 --- a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol +++ b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol @@ -62,27 +62,40 @@ 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)); - // 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(resourceSelector, 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..e2bba4a890 100644 --- a/packages/world/test/World.t.sol +++ b/packages/world/test/World.t.sol @@ -376,6 +376,10 @@ contract WorldTest is Test, GasReporter { world.registerSystem(ResourceSelector.from("", "rootSystem"), yetAnotherSystem, true); } + function testUpgradeSystem() public { + revert("TODO"); + } + function testDuplicateSelectors() public { // Register a new table bytes32 resourceSelector = ResourceSelector.from("namespace", "name"); From c809cd894293d34e3f36127cb188bb235b330ee0 Mon Sep 17 00:00:00 2001 From: alvrs Date: Fri, 1 Sep 2023 14:46:00 +0200 Subject: [PATCH 2/4] add tests --- .../WorldRegistrationSystem.sol | 8 ++- packages/world/test/World.t.sol | 52 ++++++++++++++++--- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol index c1a06fb87c..9f38aa0c33 100644 --- a/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol +++ b/packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol @@ -70,6 +70,12 @@ contract WorldRegistrationSystem is System, IWorldErrors { // Require the name to not be the namespace's root name if (resourceSelector.getName() == ROOT_NAME) revert InvalidSelector(resourceSelector.toString()); + // 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(); @@ -91,7 +97,7 @@ contract WorldRegistrationSystem is System, IWorldErrors { SystemRegistry.deleteRecord(existingSystem); // Remove the existing system's access to its namespace - ResourceAccess.deleteRecord(resourceSelector, existingSystem); + ResourceAccess.deleteRecord(namespace, existingSystem); } else { // Otherwise, this is a new system, so register its resource type ResourceType.set(resourceSelector, Resource.SYSTEM); diff --git a/packages/world/test/World.t.sol b/packages/world/test/World.t.sol index e2bba4a890..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(); @@ -377,7 +383,39 @@ contract WorldTest is Test, GasReporter { } function testUpgradeSystem() public { - revert("TODO"); + 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 { From 4217b5e79331e7c72de18fd19fb40a81862a527c Mon Sep 17 00:00:00 2001 From: alvrs Date: Fri, 1 Sep 2023 15:19:16 +0200 Subject: [PATCH 3/4] update gas report --- packages/world/gas-report.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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", From d172dcb9c6f664947e09ce14b5fed6e0876354cf Mon Sep 17 00:00:00 2001 From: alvarius Date: Fri, 1 Sep 2023 16:04:56 +0200 Subject: [PATCH 4/4] Create beige-radios-drop.md --- .changeset/beige-radios-drop.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 .changeset/beige-radios-drop.md 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); +```