fix:HyperlaneIsmFactory doesn't fail with missing chainMetadata err…

…ors (#3142)

### Description

HyperlaneIsmFactory is now wary of (try)getDomainId or (try)getChainName
calls which may fail and handles them appropriately.
- switched from chainName to domain in `routingModuleDelta`
- replace `getDomainId` with `tryGetDomainId` and `getChainName` with
`tryGetChainName` wherever necessary.

### Drive-by changes

None

### Related issues

None

### Backward compatibility

Yes

### Testing

Hardhat unit tests

.changeset/old-lizards-melt.md

@@ -0,0 +1,5 @@
+---
+'@hyperlane-xyz/sdk': minor
+---
+
+HyperlaneIsmFactory is now wary of (try)getDomainId or (try)getChainName calls which may fail and handles them appropriately.

typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts

@@ -1,6 +1,7 @@
 import { expect } from 'chai';
 import { ethers } from 'hardhat';
 
+import { DomainRoutingIsm } from '@hyperlane-xyz/core';
 import { Address, error } from '@hyperlane-xyz/utils';
 
 import { TestChains } from '../consts/chains';
@@ -77,14 +78,15 @@ describe('HyperlaneIsmFactory', async () => {
   let ismFactory: HyperlaneIsmFactory;
   let coreApp: TestCoreApp;
   let multiProvider: MultiProvider;
+  let ismFactoryDeployer: HyperlaneProxyFactoryDeployer;
   let exampleRoutingConfig: RoutingIsmConfig;
   let mailboxAddress: Address, newMailboxAddress: Address;
   const chain = 'test1';
 
   beforeEach(async () => {
     const [signer] = await ethers.getSigners();
     multiProvider = MultiProvider.createTestMultiProvider({ signer });
-    const ismFactoryDeployer = new HyperlaneProxyFactoryDeployer(multiProvider);
+    ismFactoryDeployer = new HyperlaneProxyFactoryDeployer(multiProvider);
     ismFactory = new HyperlaneIsmFactory(
       await ismFactoryDeployer.deploy(multiProvider.mapKnownChains(() => ({}))),
       multiProvider,
@@ -207,6 +209,59 @@ describe('HyperlaneIsmFactory', async () => {
       expect(matches).to.be.true;
     });
 
+    it(`should skip deployment with warning if no chain metadata configured ${type}`, async () => {
+      exampleRoutingConfig.type = type as
+        | IsmType.ROUTING
+        | IsmType.FALLBACK_ROUTING;
+      let matches = true;
+      exampleRoutingConfig.domains['test4'] = {
+        type: IsmType.MESSAGE_ID_MULTISIG,
+        threshold: 1,
+        validators: [randomAddress()],
+      };
+      let ism = await ismFactory.deploy({
+        destination: chain,
+        config: exampleRoutingConfig,
+        mailbox: mailboxAddress,
+      });
+      const existingIsm = ism.address;
+      matches =
+        matches &&
+        existingIsm === ism.address &&
+        (await moduleMatchesConfig(
+          chain,
+          ism.address,
+          exampleRoutingConfig,
+          ismFactory.multiProvider,
+          ismFactory.getContracts(chain),
+          mailboxAddress,
+        ));
+
+      exampleRoutingConfig.domains['test5'] = {
+        type: IsmType.MESSAGE_ID_MULTISIG,
+        threshold: 1,
+        validators: [randomAddress()],
+      };
+      ism = await ismFactory.deploy({
+        destination: chain,
+        config: exampleRoutingConfig,
+        existingIsmAddress: ism.address,
+        mailbox: mailboxAddress,
+      });
+      matches =
+        matches &&
+        existingIsm === ism.address &&
+        (await moduleMatchesConfig(
+          chain,
+          ism.address,
+          exampleRoutingConfig,
+          ismFactory.multiProvider,
+          ismFactory.getContracts(chain),
+          mailboxAddress,
+        ));
+      expect(matches).to.be.true;
+    });
+
     it(`deletes route in an existing ${type}`, async () => {
       exampleRoutingConfig.type = type as
         | IsmType.ROUTING
@@ -240,6 +295,53 @@ describe('HyperlaneIsmFactory', async () => {
       expect(matches).to.be.true;
     });
 
+    it(`deletes route in an existing ${type} even if not in multiprovider`, async () => {
+      exampleRoutingConfig.type = type as
+        | IsmType.ROUTING
+        | IsmType.FALLBACK_ROUTING;
+      let matches = true;
+      let ism = await ismFactory.deploy({
+        destination: chain,
+        config: exampleRoutingConfig,
+        mailbox: mailboxAddress,
+      });
+      const existingIsm = ism.address;
+      const domainsBefore = await (ism as DomainRoutingIsm).domains();
+
+      // deleting the domain and removing from multiprovider should unenroll the domain
+      // NB: we'll deploy new multisigIsms for the domains bc of new factories but the routingIsm address should be the same because of existingIsmAddress
+      delete exampleRoutingConfig.domains['test3'];
+      multiProvider = multiProvider.intersect(['test1', 'test2']).result;
+      ismFactoryDeployer = new HyperlaneProxyFactoryDeployer(multiProvider);
+      ismFactory = new HyperlaneIsmFactory(
+        await ismFactoryDeployer.deploy(
+          multiProvider.mapKnownChains(() => ({})),
+        ),
+        multiProvider,
+      );
+      ism = await ismFactory.deploy({
+        destination: chain,
+        config: exampleRoutingConfig,
+        existingIsmAddress: ism.address,
+        mailbox: mailboxAddress,
+      });
+      const domainsAfter = await (ism as DomainRoutingIsm).domains();
+
+      matches =
+        matches &&
+        existingIsm == ism.address &&
+        (await moduleMatchesConfig(
+          chain,
+          ism.address,
+          exampleRoutingConfig,
+          ismFactory.multiProvider,
+          ismFactory.getContracts(chain),
+          mailboxAddress,
+        ));
+      expect(domainsBefore.length - 1).to.equal(domainsAfter.length);
+      expect(matches).to.be.true;
+    });
+
     it(`updates owner in an existing ${type}`, async () => {
       exampleRoutingConfig.type = type as
         | IsmType.ROUTING

typescript/sdk/src/ism/HyperlaneIsmFactory.ts

@@ -23,9 +23,13 @@ import {
 } from '@hyperlane-xyz/core';
 import {
   Address,
+  Domain,
   eqAddress,
   formatMessage,
   normalizeAddress,
+  objFilter,
+  objMap,
+  warn,
 } from '@hyperlane-xyz/utils';
 
 import { HyperlaneApp } from '../app/HyperlaneApp';
@@ -201,6 +205,22 @@ export class HyperlaneIsmFactory extends HyperlaneApp<ProxyFactoryFactories> {
     const overrides = this.multiProvider.getTransactionOverrides(destination);
     const routingIsmFactory = this.getContracts(destination).routingIsmFactory;
     let routingIsm: DomainRoutingIsm | DefaultFallbackRoutingIsm;
+    // filtering out domains which are not part of the multiprovider
+    config.domains = objFilter(
+      config.domains,
+      (domain, config): config is IsmConfig => {
+        const domainId = this.multiProvider.tryGetDomainId(domain);
+        if (domainId === null) {
+          warn(
+            `Domain ${domain} doesn't have chain metadata provided, skipping ...`,
+          );
+        }
+        return domainId !== null;
+      },
+    );
+    const safeConfigDomains = Object.keys(config.domains).map((domain) =>
+      this.multiProvider.getDomainId(domain),
+    );
     const delta: RoutingIsmDelta = existingIsmAddress
       ? await routingModuleDelta(
           destination,
@@ -212,7 +232,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp<ProxyFactoryFactories> {
         )
       : {
           domainsToUnenroll: [],
-          domainsToEnroll: Object.keys(config.domains),
+          domainsToEnroll: safeConfigDomains,
         };
 
     const signer = this.multiProvider.getSigner(destination);
@@ -227,13 +247,14 @@ export class HyperlaneIsmFactory extends HyperlaneApp<ProxyFactoryFactories> {
     }
     // reconfiguring existing routing ISM
     if (existingIsmAddress && isOwner && !delta.mailbox) {
-      const isms: ChainMap<Address> = {};
+      const isms: Record<Domain, Address> = {};
       routingIsm = DomainRoutingIsm__factory.connect(
         existingIsmAddress,
         this.multiProvider.getSigner(destination),
       );
       // deploying all the ISMs which have to be updated
-      for (const origin of delta.domainsToEnroll) {
+      for (const originDomain of delta.domainsToEnroll) {
+        const origin = this.multiProvider.getChainName(originDomain); // already filtered to only include domains in the multiprovider
         logger(
           `Reconfiguring preexisting routing ISM at for origin ${origin}...`,
         );
@@ -243,23 +264,20 @@ export class HyperlaneIsmFactory extends HyperlaneApp<ProxyFactoryFactories> {
           origin,
           mailbox,
         });
-        isms[origin] = ism.address;
+        isms[originDomain] = ism.address;
         const tx = await routingIsm.set(
-          this.multiProvider.getDomainId(origin),
-          isms[origin],
+          originDomain,
+          isms[originDomain],
           overrides,
         );
         await this.multiProvider.handleTx(destination, tx);
       }
       // unenrolling domains if needed
-      for (const origin of delta.domainsToUnenroll) {
+      for (const originDomain of delta.domainsToUnenroll) {
         logger(
-          `Unenrolling origin ${origin} from preexisting routing ISM at ${existingIsmAddress}...`,
-        );
-        const tx = await routingIsm.remove(
-          this.multiProvider.getDomainId(origin),
-          overrides,
+          `Unenrolling originDomain ${originDomain} from preexisting routing ISM at ${existingIsmAddress}...`,
         );
+        const tx = await routingIsm.remove(originDomain, overrides);
         await this.multiProvider.handleTx(destination, tx);
       }
       // transfer ownership if needed
@@ -280,9 +298,6 @@ export class HyperlaneIsmFactory extends HyperlaneApp<ProxyFactoryFactories> {
         isms[origin] = ism.address;
       }
       const submoduleAddresses = Object.values(isms);
-      const domains = Object.keys(isms).map((chain) =>
-        this.multiProvider.getDomainId(chain),
-      );
       let receipt: ethers.providers.TransactionReceipt;
       if (config.type === IsmType.FALLBACK_ROUTING) {
         // deploying new fallback routing ISM
@@ -302,7 +317,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp<ProxyFactoryFactories> {
           destination,
           routingIsm['initialize(address,uint32[],address[])'](
             config.owner,
-            domains,
+            safeConfigDomains,
             submoduleAddresses,
             overrides,
           ),
@@ -311,7 +326,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp<ProxyFactoryFactories> {
         // deploying new domain routing ISM
         const tx = await routingIsmFactory.deploy(
           config.owner,
-          domains,
+          safeConfigDomains,
           submoduleAddresses,
           overrides,
         );
@@ -428,12 +443,17 @@ export async function moduleCanCertainlyVerify(
   origin: ChainName,
   destination: ChainName,
 ): Promise<boolean> {
+  const originDomainId = multiProvider.tryGetDomainId(origin);
+  const destinationDomainId = multiProvider.tryGetDomainId(destination);
+  if (!originDomainId || !destinationDomainId) {
+    return false;
+  }
   const message = formatMessage(
     0,
     0,
-    multiProvider.getDomainId(origin),
+    originDomainId,
     ethers.constants.AddressZero,
-    multiProvider.getDomainId(destination),
+    destinationDomainId,
     ethers.constants.AddressZero,
     '0x',
   );
@@ -689,7 +709,11 @@ export async function routingModuleDelta(
   const routingIsm = DomainRoutingIsm__factory.connect(moduleAddress, provider);
   const owner = await routingIsm.owner();
   const deployedDomains = (await routingIsm.domains()).map((domain) =>
-    multiProvider.getChainName(domain.toNumber()),
+    domain.toNumber(),
+  );
+  // config.domains is already filtered to only include domains in the multiprovider
+  const safeConfigDomains = objMap(config.domains, (domain) =>
+    multiProvider.getDomainId(domain),
   );
 
   const delta: RoutingIsmDelta = {
@@ -707,16 +731,15 @@ export async function routingModuleDelta(
   }
   // check for exclusion of domains in the config
   delta.domainsToUnenroll = deployedDomains.filter(
-    (domain) => !Object.keys(config.domains).includes(domain),
+    (domain) => !Object.values(safeConfigDomains).includes(domain),
   );
   // check for inclusion of domains in the config
   for (const [origin, subConfig] of Object.entries(config.domains)) {
-    if (!deployedDomains.includes(origin)) {
-      delta.domainsToEnroll.push(origin);
+    const originDomain = safeConfigDomains[origin];
+    if (!deployedDomains.includes(originDomain)) {
+      delta.domainsToEnroll.push(originDomain);
     } else {
-      const subModule = await routingIsm.module(
-        multiProvider.getDomainId(origin),
-      );
+      const subModule = await routingIsm.module(originDomain);
       // Recursively check that the submodule for each configured
       // domain matches the submodule config.
       const subModuleMatches = await moduleMatchesConfig(
@@ -725,9 +748,9 @@ export async function routingModuleDelta(
         subConfig,
         multiProvider,
         contracts,
-        origin,
+        mailbox,
       );
-      if (!subModuleMatches) delta.domainsToEnroll.push(origin);
+      if (!subModuleMatches) delta.domainsToEnroll.push(originDomain);
     }
   }
   return delta;

typescript/sdk/src/ism/types.ts

@@ -5,9 +5,9 @@ import {
   OPStackIsm,
   TestIsm,
 } from '@hyperlane-xyz/core';
-import type { Address, ValueOf } from '@hyperlane-xyz/utils';
+import type { Address, Domain, ValueOf } from '@hyperlane-xyz/utils';
 
-import { ChainMap, ChainName } from '../types';
+import { ChainMap } from '../types';
 
 // this enum should match the IInterchainSecurityModule.sol enum
 // meant for the relayer
@@ -106,8 +106,8 @@ export type DeployedIsm = ValueOf<DeployedIsmType>;
 
 // for finding the difference between the onchain deployment and the config provided
 export type RoutingIsmDelta = {
-  domainsToUnenroll: ChainName[]; // new or updated isms for the domain
-  domainsToEnroll: ChainName[]; // isms to remove
+  domainsToUnenroll: Domain[]; // new or updated isms for the domain
+  domainsToEnroll: Domain[]; // isms to remove
   owner?: Address; // is the owner different
   mailbox?: Address; // is the mailbox different (only for fallback routing)
 };