refactor(cli,world-module-metadata): install metadata module once (#3037

)
latticexyz · Aug 15, 2024 · 5dafc63 · 5dafc63
1 parent 6a66f57
commit 5dafc63
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 12 deletions.
diff --git a/packages/cli/src/deploy/ensureModules.ts b/packages/cli/src/deploy/ensureModules.ts
@@ -59,16 +59,15 @@ export async function ensureModules({
                     args: [moduleAddress, mod.installData],
                   });
             } catch (error) {
-              if (mod.optional) {
-                debug(
-                  `optional module ${mod.name} install failed, skipping\n  ${error instanceof BaseError ? error.shortMessage : error}`,
-                );
-                return;
-              }
               if (error instanceof BaseError && error.message.includes("Module_AlreadyInstalled")) {
                 debug(`module ${mod.name} already installed`);
                 return;
               }
+              if (mod.optional) {
+                debug(`optional module ${mod.name} install failed, skipping`);
+                debug(error);
+                return;
+              }
               throw error;
             }
           },

diff --git a/packages/cli/src/deploy/ensureResourceLabels.ts b/packages/cli/src/deploy/ensureResourceLabels.ts
@@ -38,7 +38,7 @@ export async function ensureResourceLabels({
     return [];
   }
 
-  debug(`setting ${resources.length} resource labels`);
+  debug("setting", resources.length, "resource labels");
   return (
     await Promise.all(
       resourcesToSet.map(async ({ resourceId, label }) => {

diff --git a/packages/world-module-metadata/gas-report.json b/packages/world-module-metadata/gas-report.json
@@ -9,7 +9,7 @@
     "file": "test/MetadataModule.t.sol",
     "test": "testInstall",
     "name": "install metadata module",
-    "gasUsed": 1106562
+    "gasUsed": 1109853
   },
   {
     "file": "test/MetadataModule.t.sol",

diff --git a/packages/world-module-metadata/src/MetadataModule.sol b/packages/world-module-metadata/src/MetadataModule.sol
@@ -26,7 +26,11 @@ contract MetadataModule is Module {
     revert Module_RootInstallNotSupported();
   }
 
-  function install(bytes memory) public {
+  function install(bytes memory args) public {
+    // naive check to ensure this is only installed once
+    // TODO: update this + deployer to be idempotent
+    requireNotInstalled(__self, args);
+
     IBaseWorld world = IBaseWorld(_world());
 
     ResourceId namespace = ResourceTag._tableId.getNamespaceId();

diff --git a/packages/world-module-metadata/test/MetadataModule.t.sol b/packages/world-module-metadata/test/MetadataModule.t.sol
@@ -8,6 +8,7 @@ import { StoreSwitch } from "@latticexyz/store/src/StoreSwitch.sol";
 import { createWorld } from "@latticexyz/world/test/createWorld.sol";
 import { ResourceId, WorldResourceIdLib, WorldResourceIdInstance } from "@latticexyz/world/src/WorldResourceId.sol";
 import { IWorldErrors } from "@latticexyz/world/src/IWorldErrors.sol";
+import { IModuleErrors } from "@latticexyz/world/src/IModuleErrors.sol";
 import { NamespaceOwner } from "@latticexyz/world/src/codegen/tables/NamespaceOwner.sol";
 
 import { MetadataModule } from "../src/MetadataModule.sol";
@@ -19,6 +20,7 @@ contract MetadataModuleTest is Test, GasReporter {
 
   IWorld world;
   MetadataModule metadataModule = new MetadataModule();
+  ResourceId namespace = ResourceTag._tableId.getNamespaceId();
 
   function setUp() public {
     world = IWorld(address(createWorld()));
@@ -30,16 +32,22 @@ contract MetadataModuleTest is Test, GasReporter {
     world.installModule(metadataModule, new bytes(0));
     endGasReport();
 
-    ResourceId namespace = ResourceTag._tableId.getNamespaceId();
     assertEq(NamespaceOwner.get(namespace), address(this));
 
-    // Installing again will revert because metadata namespace isn't owned by the module, so the module is unable to write to it.
+    vm.expectRevert(IModuleErrors.Module_AlreadyInstalled.selector);
+    world.installModule(metadataModule, new bytes(0));
+  }
+
+  function testInstallExistingNamespace() public {
+    world.registerNamespace(namespace);
+
+    // Installing will revert because metadata namespace isn't owned by the module, so the module is unable to write to it.
     vm.expectRevert(
       abi.encodeWithSelector(IWorldErrors.World_AccessDenied.selector, namespace.toString(), address(metadataModule))
     );
     world.installModule(metadataModule, new bytes(0));
 
-    // Transferring the namespace to the module and installing should be a no-op/idempotent and the module will return namespace ownership.
+    // Transferring the namespace to the module and installing will return namespace ownership.
     world.transferOwnership(namespace, address(metadataModule));
     world.installModule(metadataModule, new bytes(0));
     assertEq(NamespaceOwner.get(namespace), address(this));