From 6ca1874e02161c8feb08b5fafb20b57ce0c8fe72 Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Fri, 13 Oct 2023 09:03:26 +0100 Subject: [PATCH] feat(world-modules): only install modules once (#1756) --- .changeset/cyan-baboons-breathe.md | 5 +++++ .changeset/rude-cycles-travel.md | 7 ++++++ package.json | 2 +- packages/world-modules/gas-report.json | 22 +++++++++---------- .../modules/keysintable/KeysInTableModule.sol | 7 ++++++ .../keyswithvalue/KeysWithValueModule.sol | 7 ++++++ .../uniqueentity/UniqueEntityModule.sol | 19 +++++++++++++--- .../test/KeysInTableModule.t.sol | 9 +++++++- .../test/KeysWithValueModule.t.sol | 7 ++++++ .../test/UniqueEntityModule.t.sol | 13 +++++++++++ packages/world/src/IModule.sol | 1 + 11 files changed, 83 insertions(+), 16 deletions(-) create mode 100644 .changeset/cyan-baboons-breathe.md create mode 100644 .changeset/rude-cycles-travel.md diff --git a/.changeset/cyan-baboons-breathe.md b/.changeset/cyan-baboons-breathe.md new file mode 100644 index 0000000000..cb44b6fba1 --- /dev/null +++ b/.changeset/cyan-baboons-breathe.md @@ -0,0 +1,5 @@ +--- +"@latticexyz/world": minor +--- + +Added a `Module_AlreadyInstalled` error to `IModule`. diff --git a/.changeset/rude-cycles-travel.md b/.changeset/rude-cycles-travel.md new file mode 100644 index 0000000000..8b57b96da2 --- /dev/null +++ b/.changeset/rude-cycles-travel.md @@ -0,0 +1,7 @@ +--- +"@latticexyz/world-modules": major +--- + +Modules now revert with `Module_AlreadyInstalled` if attempting to install more than once with the same calldata. + +This is a temporary workaround for our deploy pipeline. We'll make these install steps more idempotent in the future. diff --git a/package.json b/package.json index 9152220d1d..419c584001 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "dev": "turbo run dev --concurrency 100", "dist-tag-rm": "pnpm recursive exec -- sh -c 'npm dist-tag rm $(cat package.json | jq -r \".name\") $TAG || true'", "foundryup": "curl -L https://foundry.paradigm.xyz | bash && bash ~/.foundry/bin/foundryup", - "gas-report": "pnpm recursive run gas-report", + "gas-report": "pnpm run --recursive --parallel gas-report", "lint": "pnpm prettier:check && eslint . --ext .ts --ext .tsx", "prepare": "husky install && (forge --version || pnpm foundryup)", "prettier": "prettier --write '**/*.{ts,tsx,css,md,mdx,sol}'", diff --git a/packages/world-modules/gas-report.json b/packages/world-modules/gas-report.json index a32ae89a2f..bff2b59b8c 100644 --- a/packages/world-modules/gas-report.json +++ b/packages/world-modules/gas-report.json @@ -3,13 +3,13 @@ "file": "test/KeysInTableModule.t.sol", "test": "testInstallComposite", "name": "install keys in table module", - "gasUsed": 1407698 + "gasUsed": 1409435 }, { "file": "test/KeysInTableModule.t.sol", "test": "testInstallGas", "name": "install keys in table module", - "gasUsed": 1407698 + "gasUsed": 1409435 }, { "file": "test/KeysInTableModule.t.sol", @@ -21,13 +21,13 @@ "file": "test/KeysInTableModule.t.sol", "test": "testInstallSingleton", "name": "install keys in table module", - "gasUsed": 1407698 + "gasUsed": 1409435 }, { "file": "test/KeysInTableModule.t.sol", "test": "testSetAndDeleteRecordHookCompositeGas", "name": "install keys in table module", - "gasUsed": 1407698 + "gasUsed": 1409435 }, { "file": "test/KeysInTableModule.t.sol", @@ -45,7 +45,7 @@ "file": "test/KeysInTableModule.t.sol", "test": "testSetAndDeleteRecordHookGas", "name": "install keys in table module", - "gasUsed": 1407698 + "gasUsed": 1409435 }, { "file": "test/KeysInTableModule.t.sol", @@ -63,7 +63,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testGetKeysWithValueGas", "name": "install keys with value module", - "gasUsed": 648994 + "gasUsed": 650708 }, { "file": "test/KeysWithValueModule.t.sol", @@ -81,7 +81,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testInstall", "name": "install keys with value module", - "gasUsed": 648994 + "gasUsed": 650708 }, { "file": "test/KeysWithValueModule.t.sol", @@ -93,7 +93,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testSetAndDeleteRecordHook", "name": "install keys with value module", - "gasUsed": 648994 + "gasUsed": 650708 }, { "file": "test/KeysWithValueModule.t.sol", @@ -111,7 +111,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testSetField", "name": "install keys with value module", - "gasUsed": 648994 + "gasUsed": 650708 }, { "file": "test/KeysWithValueModule.t.sol", @@ -219,7 +219,7 @@ "file": "test/UniqueEntityModule.t.sol", "test": "testInstall", "name": "install unique entity module", - "gasUsed": 675275 + "gasUsed": 678996 }, { "file": "test/UniqueEntityModule.t.sol", @@ -231,7 +231,7 @@ "file": "test/UniqueEntityModule.t.sol", "test": "testInstallRoot", "name": "installRoot unique entity module", - "gasUsed": 642601 + "gasUsed": 644433 }, { "file": "test/UniqueEntityModule.t.sol", diff --git a/packages/world-modules/src/modules/keysintable/KeysInTableModule.sol b/packages/world-modules/src/modules/keysintable/KeysInTableModule.sol index f5ec9cc0df..b3172453aa 100644 --- a/packages/world-modules/src/modules/keysintable/KeysInTableModule.sol +++ b/packages/world-modules/src/modules/keysintable/KeysInTableModule.sol @@ -7,6 +7,7 @@ import { ResourceIds } from "@latticexyz/store/src/codegen/tables/ResourceIds.so import { Module } from "@latticexyz/world/src/Module.sol"; import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol"; +import { InstalledModules } from "@latticexyz/world/src/codegen/index.sol"; import { ResourceId, WorldResourceIdInstance } from "@latticexyz/world/src/WorldResourceId.sol"; import { revertWithBytes } from "@latticexyz/world/src/revertWithBytes.sol"; @@ -37,6 +38,12 @@ contract KeysInTableModule is Module { } function installRoot(bytes memory args) public override { + // Naive check to ensure this is only installed once + // TODO: only revert if there's nothing to do + if (InstalledModules.getModuleAddress(getName(), keccak256(args)) != address(0)) { + revert Module_AlreadyInstalled(); + } + // Extract source table id from args ResourceId sourceTableId = ResourceId.wrap(abi.decode(args, (bytes32))); diff --git a/packages/world-modules/src/modules/keyswithvalue/KeysWithValueModule.sol b/packages/world-modules/src/modules/keyswithvalue/KeysWithValueModule.sol index df655d8d18..1810ee7fa8 100644 --- a/packages/world-modules/src/modules/keyswithvalue/KeysWithValueModule.sol +++ b/packages/world-modules/src/modules/keyswithvalue/KeysWithValueModule.sol @@ -5,6 +5,7 @@ import { BEFORE_SET_RECORD, BEFORE_SPLICE_STATIC_DATA, AFTER_SPLICE_STATIC_DATA, import { Module } from "@latticexyz/world/src/Module.sol"; import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol"; +import { InstalledModules } from "@latticexyz/world/src/codegen/index.sol"; import { WorldContextConsumer } from "@latticexyz/world/src/WorldContext.sol"; import { ResourceId, WorldResourceIdInstance } from "@latticexyz/world/src/WorldResourceId.sol"; @@ -38,6 +39,12 @@ contract KeysWithValueModule is Module { } function installRoot(bytes memory args) public { + // Naive check to ensure this is only installed once + // TODO: only revert if there's nothing to do + if (InstalledModules.getModuleAddress(getName(), keccak256(args)) != address(0)) { + revert Module_AlreadyInstalled(); + } + // Extract source table id from args ResourceId sourceTableId = ResourceId.wrap(abi.decode(args, (bytes32))); ResourceId targetTableSelector = getTargetTableId(MODULE_NAMESPACE, sourceTableId); diff --git a/packages/world-modules/src/modules/uniqueentity/UniqueEntityModule.sol b/packages/world-modules/src/modules/uniqueentity/UniqueEntityModule.sol index f3b6a6d39c..af56d05be7 100644 --- a/packages/world-modules/src/modules/uniqueentity/UniqueEntityModule.sol +++ b/packages/world-modules/src/modules/uniqueentity/UniqueEntityModule.sol @@ -2,6 +2,7 @@ pragma solidity >=0.8.21; import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol"; +import { InstalledModules } from "@latticexyz/world/src/codegen/index.sol"; import { Module } from "@latticexyz/world/src/Module.sol"; import { WorldContextConsumer } from "@latticexyz/world/src/WorldContext.sol"; @@ -25,7 +26,13 @@ contract UniqueEntityModule is Module { return MODULE_NAME; } - function installRoot(bytes memory) public { + function installRoot(bytes memory args) public { + // Naive check to ensure this is only installed once + // TODO: only revert if there's nothing to do + if (InstalledModules.getModuleAddress(getName(), keccak256(args)) != address(0)) { + revert Module_AlreadyInstalled(); + } + IBaseWorld world = IBaseWorld(_world()); // Register table @@ -44,11 +51,17 @@ contract UniqueEntityModule is Module { if (!success) revertWithBytes(data); } - function install(bytes memory) public { + function install(bytes memory args) public { + // Naive check to ensure this is only installed once + // TODO: only revert if there's nothing to do + if (InstalledModules.getModuleAddress(getName(), keccak256(args)) != address(0)) { + revert Module_AlreadyInstalled(); + } + IBaseWorld world = IBaseWorld(_world()); // Register table - UniqueEntity.register(world, TABLE_ID); + UniqueEntity.register(TABLE_ID); // Register system world.registerSystem(SYSTEM_ID, uniqueEntitySystem, true); diff --git a/packages/world-modules/test/KeysInTableModule.t.sol b/packages/world-modules/test/KeysInTableModule.t.sol index 7ba837e435..1393b78e23 100644 --- a/packages/world-modules/test/KeysInTableModule.t.sol +++ b/packages/world-modules/test/KeysInTableModule.t.sol @@ -12,6 +12,7 @@ import { SchemaEncodeHelper } from "@latticexyz/store/test/SchemaEncodeHelper.so import { SchemaType } from "@latticexyz/schema-type/src/solidity/SchemaType.sol"; import { World } from "@latticexyz/world/src/World.sol"; +import { IModule } from "@latticexyz/world/src/IModule.sol"; import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol"; import { ResourceId, WorldResourceIdLib, WorldResourceIdInstance } from "@latticexyz/world/src/WorldResourceId.sol"; import { ROOT_NAMESPACE } from "@latticexyz/world/src/constants.sol"; @@ -161,7 +162,13 @@ contract KeysInTableModuleTest is Test, GasReporter { assertEq(keysInTable[0][0], key1); } - function testInstallTwice(bytes32 keyA, bytes32 keyB, uint256 value1, uint256 value2) public { + function testInstallTwice() public { + world.installRootModule(keysInTableModule, abi.encode(tableId)); + vm.expectRevert(IModule.Module_AlreadyInstalled.selector); + world.installRootModule(keysInTableModule, abi.encode(tableId)); + } + + function testInstallMultiple(bytes32 keyA, bytes32 keyB, uint256 value1, uint256 value2) public { _installKeysInTableModule(); bytes32[] memory keyTuple = new bytes32[](1); diff --git a/packages/world-modules/test/KeysWithValueModule.t.sol b/packages/world-modules/test/KeysWithValueModule.t.sol index a01b0d68a2..d50a70326f 100644 --- a/packages/world-modules/test/KeysWithValueModule.t.sol +++ b/packages/world-modules/test/KeysWithValueModule.t.sol @@ -15,6 +15,7 @@ import { FieldLayout } from "@latticexyz/store/src/FieldLayout.sol"; import { FieldLayoutEncodeHelper } from "@latticexyz/store/test/FieldLayoutEncodeHelper.sol"; import { World } from "@latticexyz/world/src/World.sol"; +import { IModule } from "@latticexyz/world/src/IModule.sol"; import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol"; import { WorldResourceIdLib, WorldResourceIdInstance, NAME_BITS, TYPE_BITS } from "@latticexyz/world/src/WorldResourceId.sol"; import { ROOT_NAMESPACE } from "@latticexyz/world/src/constants.sol"; @@ -104,6 +105,12 @@ contract KeysWithValueModuleTest is Test, GasReporter { assertEq(keysWithValue[0], key1); } + function testInstallTwice() public { + world.installRootModule(keysWithValueModule, abi.encode(sourceTableId)); + vm.expectRevert(IModule.Module_AlreadyInstalled.selector); + world.installRootModule(keysWithValueModule, abi.encode(sourceTableId)); + } + function testSetAndDeleteRecordHook() public { _installKeysWithValueModule(); diff --git a/packages/world-modules/test/UniqueEntityModule.t.sol b/packages/world-modules/test/UniqueEntityModule.t.sol index 6b0dd8ea45..5979a67435 100644 --- a/packages/world-modules/test/UniqueEntityModule.t.sol +++ b/packages/world-modules/test/UniqueEntityModule.t.sol @@ -6,6 +6,7 @@ import { GasReporter } from "@latticexyz/gas-report/src/GasReporter.sol"; import { StoreSwitch } from "@latticexyz/store/src/StoreSwitch.sol"; import { World } from "@latticexyz/world/src/World.sol"; +import { IModule } from "@latticexyz/world/src/IModule.sol"; import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol"; import { IWorldErrors } from "@latticexyz/world/src/IWorldErrors.sol"; import { System } from "@latticexyz/world/src/System.sol"; @@ -58,6 +59,12 @@ contract UniqueEntityModuleTest is Test, GasReporter { assertEq(UniqueEntity.get(tableId), uniqueEntity + 1); } + function testInstallTwice() public { + world.installModule(uniqueEntityModule, new bytes(0)); + vm.expectRevert(IModule.Module_AlreadyInstalled.selector); + world.installModule(uniqueEntityModule, new bytes(0)); + } + function testInstallRoot() public { ResourceId tableId = _tableId; @@ -76,6 +83,12 @@ contract UniqueEntityModuleTest is Test, GasReporter { assertEq(UniqueEntity.get(tableId), uniqueEntity + 1); } + function testInstallRootTwice() public { + world.installRootModule(uniqueEntityModule, new bytes(0)); + vm.expectRevert(IModule.Module_AlreadyInstalled.selector); + world.installRootModule(uniqueEntityModule, new bytes(0)); + } + function testPublicAccess() public { ResourceId tableId = _tableId; diff --git a/packages/world/src/IModule.sol b/packages/world/src/IModule.sol index 898cb59d76..a0bf4f8f6d 100644 --- a/packages/world/src/IModule.sol +++ b/packages/world/src/IModule.sol @@ -23,6 +23,7 @@ interface IModule is IERC165 { /// @dev Errors to represent non-support of specific installation types. error Module_RootInstallNotSupported(); error Module_NonRootInstallNotSupported(); + error Module_AlreadyInstalled(); /** * @notice Return the name of the module.