From b478ecf468706545e4349af38e6a273941879fcc Mon Sep 17 00:00:00 2001 From: R-Morpheus Date: Mon, 11 Dec 2023 11:21:18 +0300 Subject: [PATCH 1/5] add: two helpers for installed modules --- .../src/modules/erc20-puppet/ERC20Module.sol | 7 ++----- packages/world/src/Module.sol | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol b/packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol index fe1818cd51..b3dbec5944 100644 --- a/packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol +++ b/packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol @@ -6,7 +6,6 @@ import { ResourceId } from "@latticexyz/store/src/ResourceId.sol"; import { Module } from "@latticexyz/world/src/Module.sol"; import { WorldResourceIdLib } from "@latticexyz/world/src/WorldResourceId.sol"; import { IBaseWorld } from "@latticexyz/world/src/codegen/interfaces/IBaseWorld.sol"; -import { InstalledModules } from "@latticexyz/world/src/codegen/tables/InstalledModules.sol"; import { Puppet } from "../puppet/Puppet.sol"; import { createPuppet } from "../puppet/createPuppet.sol"; @@ -32,16 +31,14 @@ contract ERC20Module is Module { function _requireDependencies() internal view { // Require PuppetModule to be installed - if (InstalledModules.get(PUPPET_MODULE_NAME, keccak256(new bytes(0))) == address(0)) { + if (!isInstalled(PUPPET_MODULE_NAME, abi.encode(keccak256(new bytes(0))))) { revert Module_MissingDependency(string(bytes.concat(PUPPET_MODULE_NAME))); } } function install(bytes memory args) public { // Require the module to not be installed with these args yet - if (InstalledModules.get(MODULE_NAME, keccak256(args)) != address(0)) { - revert Module_AlreadyInstalled(); - } + requireNotInstalled(MODULE_NAME, args); // Extract args (bytes14 namespace, ERC20MetadataData memory metadata) = abi.decode(args, (bytes14, ERC20MetadataData)); diff --git a/packages/world/src/Module.sol b/packages/world/src/Module.sol index 6bf4c64921..4a94a59e74 100644 --- a/packages/world/src/Module.sol +++ b/packages/world/src/Module.sol @@ -4,6 +4,7 @@ pragma solidity >=0.8.21; import { WorldContextConsumer } from "./WorldContext.sol"; import { IModule, MODULE_INTERFACE_ID } from "./IModule.sol"; import { IERC165, ERC165_INTERFACE_ID } from "./IERC165.sol"; +import { InstalledModules } from "./codegen/tables/InstalledModules.sol"; /** * @title Module @@ -21,4 +22,20 @@ abstract contract Module is IModule, WorldContextConsumer { ) public pure virtual override(IERC165, WorldContextConsumer) returns (bool) { return interfaceId == MODULE_INTERFACE_ID || interfaceId == ERC165_INTERFACE_ID; } + + /** + * Check if a module with the given name and arguments is installed. + */ + function isInstalled(bytes16 moduleName, bytes memory args) public view returns (bool) { + return InstalledModules.get(moduleName, keccak256(args)) != address(0); + } + + /** + * Revert if the module with the given name and arguments is already installed. + */ + function requireNotInstalled(bytes16 moduleName, bytes memory args) public view { + if (isInstalled(moduleName, args)) { + revert Module_AlreadyInstalled(); + } + } } From c41c072e1660a4189f26d39f746bf7a7f44d5515 Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Tue, 2 Jan 2024 02:59:19 -0800 Subject: [PATCH 2/5] Apply suggestions from code review --- packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol b/packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol index b3dbec5944..5a65cd5af5 100644 --- a/packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol +++ b/packages/world-modules/src/modules/erc20-puppet/ERC20Module.sol @@ -31,7 +31,7 @@ contract ERC20Module is Module { function _requireDependencies() internal view { // Require PuppetModule to be installed - if (!isInstalled(PUPPET_MODULE_NAME, abi.encode(keccak256(new bytes(0))))) { + if (!isInstalled(PUPPET_MODULE_NAME, new bytes(0))) { revert Module_MissingDependency(string(bytes.concat(PUPPET_MODULE_NAME))); } } From b430248611e5b1afb20717ad55df8f2cd974a9ba Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Tue, 2 Jan 2024 12:09:33 +0000 Subject: [PATCH 3/5] replace more instances --- packages/world-modules/gas-report.json | 22 +++++++++---------- .../modules/erc721-puppet/ERC721Module.sol | 8 +++---- .../modules/keysintable/KeysInTableModule.sol | 4 +--- .../keyswithvalue/KeysWithValueModule.sol | 4 +--- .../uniqueentity/UniqueEntityModule.sol | 8 ++----- packages/world/src/Module.sol | 4 ++-- 6 files changed, 20 insertions(+), 30 deletions(-) diff --git a/packages/world-modules/gas-report.json b/packages/world-modules/gas-report.json index b9e5dddb20..ab8a8fb3dc 100644 --- a/packages/world-modules/gas-report.json +++ b/packages/world-modules/gas-report.json @@ -75,13 +75,13 @@ "file": "test/KeysInTableModule.t.sol", "test": "testInstallComposite", "name": "install keys in table module", - "gasUsed": 1409435 + "gasUsed": 1409529 }, { "file": "test/KeysInTableModule.t.sol", "test": "testInstallGas", "name": "install keys in table module", - "gasUsed": 1409435 + "gasUsed": 1409529 }, { "file": "test/KeysInTableModule.t.sol", @@ -93,13 +93,13 @@ "file": "test/KeysInTableModule.t.sol", "test": "testInstallSingleton", "name": "install keys in table module", - "gasUsed": 1409435 + "gasUsed": 1409529 }, { "file": "test/KeysInTableModule.t.sol", "test": "testSetAndDeleteRecordHookCompositeGas", "name": "install keys in table module", - "gasUsed": 1409435 + "gasUsed": 1409529 }, { "file": "test/KeysInTableModule.t.sol", @@ -117,7 +117,7 @@ "file": "test/KeysInTableModule.t.sol", "test": "testSetAndDeleteRecordHookGas", "name": "install keys in table module", - "gasUsed": 1409435 + "gasUsed": 1409529 }, { "file": "test/KeysInTableModule.t.sol", @@ -135,7 +135,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testGetKeysWithValueGas", "name": "install keys with value module", - "gasUsed": 650708 + "gasUsed": 650796 }, { "file": "test/KeysWithValueModule.t.sol", @@ -153,7 +153,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testInstall", "name": "install keys with value module", - "gasUsed": 650708 + "gasUsed": 650796 }, { "file": "test/KeysWithValueModule.t.sol", @@ -165,7 +165,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testSetAndDeleteRecordHook", "name": "install keys with value module", - "gasUsed": 650708 + "gasUsed": 650796 }, { "file": "test/KeysWithValueModule.t.sol", @@ -183,7 +183,7 @@ "file": "test/KeysWithValueModule.t.sol", "test": "testSetField", "name": "install keys with value module", - "gasUsed": 650708 + "gasUsed": 650796 }, { "file": "test/KeysWithValueModule.t.sol", @@ -303,7 +303,7 @@ "file": "test/UniqueEntityModule.t.sol", "test": "testInstall", "name": "install unique entity module", - "gasUsed": 678996 + "gasUsed": 679102 }, { "file": "test/UniqueEntityModule.t.sol", @@ -315,7 +315,7 @@ "file": "test/UniqueEntityModule.t.sol", "test": "testInstallRoot", "name": "installRoot unique entity module", - "gasUsed": 644433 + "gasUsed": 644539 }, { "file": "test/UniqueEntityModule.t.sol", diff --git a/packages/world-modules/src/modules/erc721-puppet/ERC721Module.sol b/packages/world-modules/src/modules/erc721-puppet/ERC721Module.sol index 446cb4fbfc..6a297e05cd 100644 --- a/packages/world-modules/src/modules/erc721-puppet/ERC721Module.sol +++ b/packages/world-modules/src/modules/erc721-puppet/ERC721Module.sol @@ -33,18 +33,16 @@ contract ERC721Module is Module { return MODULE_NAME; } - function _requireDependencies() internal { + function _requireDependencies() internal view { // Require PuppetModule to be installed - if (InstalledModules.get(PUPPET_MODULE_NAME, keccak256(new bytes(0))) == address(0)) { + if (!isInstalled(PUPPET_MODULE_NAME, new bytes(0))) { revert Module_MissingDependency(string(bytes.concat(PUPPET_MODULE_NAME))); } } function install(bytes memory args) public { // Require the module to not be installed with these args yet - if (InstalledModules.get(MODULE_NAME, keccak256(args)) != address(0)) { - revert Module_AlreadyInstalled(); - } + requireNotInstalled(MODULE_NAME, args); // Extract args (bytes14 namespace, ERC721MetadataData memory metadata) = abi.decode(args, (bytes14, ERC721MetadataData)); diff --git a/packages/world-modules/src/modules/keysintable/KeysInTableModule.sol b/packages/world-modules/src/modules/keysintable/KeysInTableModule.sol index b3172453aa..504a52565a 100644 --- a/packages/world-modules/src/modules/keysintable/KeysInTableModule.sol +++ b/packages/world-modules/src/modules/keysintable/KeysInTableModule.sol @@ -40,9 +40,7 @@ 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(); - } + requireNotInstalled(getName(), args); // 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 1810ee7fa8..7ca3362bba 100644 --- a/packages/world-modules/src/modules/keyswithvalue/KeysWithValueModule.sol +++ b/packages/world-modules/src/modules/keyswithvalue/KeysWithValueModule.sol @@ -41,9 +41,7 @@ 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(); - } + requireNotInstalled(getName(), args); // Extract source table id from args ResourceId sourceTableId = ResourceId.wrap(abi.decode(args, (bytes32))); diff --git a/packages/world-modules/src/modules/uniqueentity/UniqueEntityModule.sol b/packages/world-modules/src/modules/uniqueentity/UniqueEntityModule.sol index af56d05be7..36ddb01e8a 100644 --- a/packages/world-modules/src/modules/uniqueentity/UniqueEntityModule.sol +++ b/packages/world-modules/src/modules/uniqueentity/UniqueEntityModule.sol @@ -29,9 +29,7 @@ contract UniqueEntityModule 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(); - } + requireNotInstalled(getName(), args); IBaseWorld world = IBaseWorld(_world()); @@ -54,9 +52,7 @@ contract UniqueEntityModule is Module { 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(); - } + requireNotInstalled(getName(), args); IBaseWorld world = IBaseWorld(_world()); diff --git a/packages/world/src/Module.sol b/packages/world/src/Module.sol index 4a94a59e74..31c297c9c5 100644 --- a/packages/world/src/Module.sol +++ b/packages/world/src/Module.sol @@ -26,14 +26,14 @@ abstract contract Module is IModule, WorldContextConsumer { /** * Check if a module with the given name and arguments is installed. */ - function isInstalled(bytes16 moduleName, bytes memory args) public view returns (bool) { + function isInstalled(bytes16 moduleName, bytes memory args) internal view returns (bool) { return InstalledModules.get(moduleName, keccak256(args)) != address(0); } /** * Revert if the module with the given name and arguments is already installed. */ - function requireNotInstalled(bytes16 moduleName, bytes memory args) public view { + function requireNotInstalled(bytes16 moduleName, bytes memory args) internal view { if (isInstalled(moduleName, args)) { revert Module_AlreadyInstalled(); } From 335abe33fbeb8ac38ac681d9964086ec0d80c012 Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Tue, 2 Jan 2024 04:16:39 -0800 Subject: [PATCH 4/5] Create grumpy-icons-sleep.md --- .changeset/grumpy-icons-sleep.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/grumpy-icons-sleep.md diff --git a/.changeset/grumpy-icons-sleep.md b/.changeset/grumpy-icons-sleep.md new file mode 100644 index 0000000000..c2fdabc713 --- /dev/null +++ b/.changeset/grumpy-icons-sleep.md @@ -0,0 +1,6 @@ +--- +"@latticexyz/world-modules": patch +"@latticexyz/world": patch +--- + +Added `isInstalled` and `requireNotInstalled` helpers to `Module` base contract. From 957fbfc2a15ecc9b401d50c4904dfb6c47c23be7 Mon Sep 17 00:00:00 2001 From: Kevin Ingersoll Date: Tue, 2 Jan 2024 12:19:38 +0000 Subject: [PATCH 5/5] proper natspec --- packages/world/src/Module.sol | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/world/src/Module.sol b/packages/world/src/Module.sol index 31c297c9c5..a39219e422 100644 --- a/packages/world/src/Module.sol +++ b/packages/world/src/Module.sol @@ -24,14 +24,19 @@ abstract contract Module is IModule, WorldContextConsumer { } /** - * Check if a module with the given name and arguments is installed. + * @dev Check if a module with the given name and arguments is installed. + * @param moduleName The name of the module. + * @param args The arguments for the module installation. + * @return true if the module is installed, false otherwise. */ function isInstalled(bytes16 moduleName, bytes memory args) internal view returns (bool) { return InstalledModules.get(moduleName, keccak256(args)) != address(0); } /** - * Revert if the module with the given name and arguments is already installed. + * @dev Revert if the module with the given name and arguments is already installed. + * @param moduleName The name of the module. + * @param args The arguments for the module installation. */ function requireNotInstalled(bytes16 moduleName, bytes memory args) internal view { if (isInstalled(moduleName, args)) {